Repository navigation
feat: study briefing — cohesive loop connecting content, review, and sessions - #4
Conversation
- session.py: _render_activity_feed() returned wrapper div causing duplicate id="activity-feed" in DOM when HTMX SSE swapped innerHTML. Now returns only inner content. - services/review.py: restore flashcard_count key (was renamed to card_count breaking frontend app.js and web API tests) - test_mcp_tools.py: match restored flashcard_count key - test_e2e_session_demo.py: 8-test E2E suite covering full session lifecycle (dashboard, SSE feed, terminal proxy, pop-out, WS relay, LAN auth, cleanup). Doubles as recordable demo via --video=on. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sessions Topic resolver (logic/topic_resolver.py): - Pure function: resolve_topic() -> ResolveResult with 4-tier cascade (exact name → name substring → tag match → difflib fuzzy) - Uses str.casefold() for Unicode-correct matching - Interactive picker in CLI shell when ambiguous (click.prompt) - 24 tests, zero mocking Study briefing (logic/briefing_logic.py + cli/_study.py): - BriefingData + ReviewContext + ContentContext dataclasses (FCIS boundary) - format_study_briefing() pure formatter → markdown for agent persona - Shell-side gatherers with independent fault tolerance - Struggling count derived from get_due(), not get_wrong() - topic_slug + topic_config_name stored in session state Post-session flashcard generation (services/flashcard_writer.py): - write_session_flashcards() generates JSON from session wins/insights - Question-form fronts, hash dedup, 15-char minimum filter - Hooked into cleanup.py with logger.warning on failure Performance fix: - get_course_stats() correlated subquery → CTE + ROW_NUMBER() Tests: 602 passed (+60 new), 7 skipped, lint clean Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Connects content, review, and study sessions into a tighter feedback loop by resolving free-text topics to configured courses, injecting a structured “study briefing” at session start, and generating flashcards from session wins/insights on session end (plus a review DB stats performance improvement and web session feed rendering fix).
Changes:
- Added a pure topic resolver (exact/substring/tag/fuzzy) and wired it into
studyctl studywith an interactive disambiguation picker. - Added study briefing formatting logic and session-start data gatherers to inject review + content inventory context into the agent persona.
- Added post-session flashcard writer + session-end hook to emit deduped flashcard JSON; improved
get_course_stats()mastered calculation via CTE/window function.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/studyctl/tests/test_web_session.py | Updates assertions to match activity-feed fragment now being inner HTML only. |
| packages/studyctl/tests/test_topic_resolver.py | New unit tests for resolver cascade behavior, precedence, and Unicode casefold handling. |
| packages/studyctl/tests/test_mcp_tools.py | Updates expected key name to flashcard_count. |
| packages/studyctl/tests/test_flashcard_writer.py | New tests for win/insight flashcard generation, dedup, and JSON structure. |
| packages/studyctl/tests/test_e2e_session_demo.py | New Playwright-based E2E/demo test covering web UI + ttyd + WS proxy + LAN auth. |
| packages/studyctl/tests/test_briefing_logic.py | New tests for briefing formatting and degradation behavior; light integration coverage via mocks. |
| packages/studyctl/src/studyctl/web/routes/session.py | Fixes SSE activity-feed rendering to avoid duplicating the swap target element/id. |
| packages/studyctl/src/studyctl/session/cleanup.py | Hooks session-end cleanup to generate flashcards from wins/insights. |
| packages/studyctl/src/studyctl/services/review.py | Renames card_count → flashcard_count in course summaries. |
| packages/studyctl/src/studyctl/services/flashcard_writer.py | New flashcard writer service for post-session JSON generation with hash-based dedup. |
| packages/studyctl/src/studyctl/review_db.py | Replaces correlated subquery with CTE + window function for mastered stats. |
| packages/studyctl/src/studyctl/logic/topic_resolver.py | New pure resolver implementing the 4-tier match cascade. |
| packages/studyctl/src/studyctl/logic/briefing_logic.py | New pure briefing formatter and data structures. |
| packages/studyctl/src/studyctl/cli/_study.py | Wires topic resolution + briefing assembly into studyctl study startup flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "name": name, | ||
| "card_count": fc_count, | ||
| "flashcard_count": fc_count, | ||
| "quiz_count": quiz_count, | ||
| "due_count": due, |
There was a problem hiding this comment.
list_course_summaries() now returns the key flashcard_count, but the function docstring still documents the return shape using card_count. This is likely to confuse API consumers and future maintainers—please update the docstring (and any related docs) to match the renamed field.
| def write_session_flashcards( | ||
| content_base: Path, | ||
| topic_slug: str, | ||
| session_id: str, | ||
| topic_entries: list[TopicEntry], | ||
| ) -> int: |
There was a problem hiding this comment.
session_id is documented as being used for source attribution, but it is currently unused and the generated card source field only includes the date. Either remove session_id or include it in the written payload/filename/source so the provenance matches the API contract.
| CONFIG_DIR = Path.home() / ".config" / "studyctl" | ||
| STATE_FILE = CONFIG_DIR / "session-state.json" | ||
| TOPICS_FILE = CONFIG_DIR / "session-topics.md" | ||
| PARKING_FILE = CONFIG_DIR / "session-parking.md" | ||
| ONELINE_FILE = CONFIG_DIR / "session-oneline.txt" | ||
| SESSIONS_DIR = CONFIG_DIR / "sessions" |
There was a problem hiding this comment.
This test hard-codes its working files under Path.home()/.config/studyctl. That means running the test locally can read/overwrite a developer’s real studyctl state and requires aggressive cleanup. Consider deriving these paths from an isolated temp HOME/XDG_CONFIG_HOME set in a fixture (and avoid module-level constants so the env override takes effect).
| CONFIG_DIR = Path.home() / ".config" / "studyctl" | |
| STATE_FILE = CONFIG_DIR / "session-state.json" | |
| TOPICS_FILE = CONFIG_DIR / "session-topics.md" | |
| PARKING_FILE = CONFIG_DIR / "session-parking.md" | |
| ONELINE_FILE = CONFIG_DIR / "session-oneline.txt" | |
| SESSIONS_DIR = CONFIG_DIR / "sessions" | |
| def _config_dir() -> Path: | |
| xdg_config_home = os.environ.get("XDG_CONFIG_HOME") | |
| if xdg_config_home: | |
| return Path(xdg_config_home) / "studyctl" | |
| return Path.home() / ".config" / "studyctl" | |
| def _state_file() -> Path: | |
| return _config_dir() / "session-state.json" | |
| def _topics_file() -> Path: | |
| return _config_dir() / "session-topics.md" | |
| def _parking_file() -> Path: | |
| return _config_dir() / "session-parking.md" | |
| def _oneline_file() -> Path: | |
| return _config_dir() / "session-oneline.txt" | |
| def _sessions_dir() -> Path: | |
| return _config_dir() / "sessions" |
| # Kill study tmux sessions | ||
| result = _tmux("list-sessions", "-F", "#{session_name}") | ||
| if result.returncode == 0: | ||
| for name in result.stdout.strip().splitlines(): | ||
| if name.startswith("study-") or name.startswith("e2e-"): | ||
| _tmux("kill-session", "-t", name) |
There was a problem hiding this comment.
_cleanup_all() kills any tmux session whose name starts with study- or e2e-, which can terminate unrelated user sessions if this test is run on a dev machine. Please scope cleanup to only the session(s) created by this test (e.g., exact study-e2e-demo... prefix or PID-based ownership) rather than broad prefixes.
| for pattern in ( | ||
| "studyctl.tui.sidebar", | ||
| "mock-agent", | ||
| f"ttyd.*{TTYD_PORT}", | ||
| f"studyctl.*{WEB_PORT}", | ||
| ): | ||
| with contextlib.suppress(Exception): | ||
| subprocess.run(["pkill", "-f", pattern], capture_output=True, check=False) |
There was a problem hiding this comment.
Using pkill -f with broad patterns (e.g. studyctl.*{WEB_PORT}) is risky in a local environment because it can match and kill unrelated processes. Prefer tracking PIDs you started (from Popen) and terminating those, or narrowing patterns with unique test identifiers.
| WEB_PORT = 18567 | ||
| TTYD_PORT = 17681 | ||
| LAN_PASSWORD = "e2e-test-pass" # pragma: allowlist secret | ||
| STUDYCTL = f"uv run --project {PROJECT_DIR} studyctl" |
There was a problem hiding this comment.
The E2E test uses fixed ports (18567 / 17681), which can make the test flaky if those ports are already in use on a developer machine or CI runner. Consider dynamically selecting free ports at runtime (and writing them into the temp config) to reduce collisions.
Amendment #4 at the top of the plan. Closes out yesterday's TodoWrite items PR-B-1 through PR-B-4 and PR-B-9; next handoff is §1.5 wiring. Captures: - Four sub-PRs shipped this session with commit SHAs (0e26d8c, 4f418ff, c35224f) plus the rename-hygiene commit b15fb5c that unblocked CI. - Two real production bugs that PR-B-4's TDD flushed out of PR-B-2's shipped code: the process-global SIGCHLD registration that broke per-loop semantics, and the double-emit in cancel() that silently downgraded user cancellations to reason="exit". Both fixed in c35224f; documented here so §1.5 readers don't hit them cold. - Test-suite state: 1974 passed, 145 deselected, 0 failures. +928 tests over Phase 1 start, driven mostly by the rename fixes repairing harness scripts that had been silently skipping earlier. - Open follow-ups scoped for the next PR: §1.5 FastAPI wiring, §1.6-1.9 xterm.js UI, Origin-guard blocker B1 (lands with §1.5), and eventual removal of the legacy session_runtime/ module once the web route no longer reaches into it. No code change — documentation only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
feat: study briefing — cohesive loop connecting content, review, and sessions
…d flattened onto one line [plan 5, F]
The END-armour walk-back required a line break before the armour, so a block
whose BEGIN line was lost and whose newlines became spaces ("<b64> <b64>
-----END RSA PRIVATE KEY-----") redacted only the END armour and every body
run survived in the text (lane L, found-not-fixed #4).
_flattened_tail_start mirrors the flattened BEGIN rule backwards: from the
armour, take whitespace-separated runs of body length (the same 20-character
floor as _INLINE_BODY_RUN, so "see page 12 of the notes" before an END stays
prose) until a shorter word, a line break, another armour or an earlier
pass's NUL mask. The run nearest the armour may also be glued onto it (a
paste that dropped its newlines) or a PGP CRC line "=XXXX". Each step moves
strictly left and stops at the previous armour's dashes, so 20 000 END
armours on one line walk their own runs once (139 ms for 3.2 MB; 1 MB body
then END 29-32 ms). The line-break form is untouched (disjoint on the
character before END), and the reference-algorithm corpus is byte-identical.
The run alphabet is built from string.ascii_letters/digits: written out, the
65 base64 characters read as a high-entropy secret to detect-secrets.
test_readiness_scaling.py imported the package conftest by name at six sites to call the hooks under test. Under the workspace default --import-mode=importlib that executes packages/studyloop/tests/conftest.py a second time as a separate module (sys.modules['conftest'] beside the registered 'packages.studyloop.tests.conftest'), so the tests patched and called a copy nothing else runs; the pid markers only kept the re-run from re-pointing the sandbox (lane O, review 4, found-not-fixed #4). _loaded_package_conftest moves out of test_suite_isolation.py into tests/_conftest_access.py as loaded_package_conftest(config) and both modules use it. Two new isolation tests prove the property: an AST scan that no test module imports 'conftest' by name, and a runtime check that exactly one module object executed from the file is in sys.modules and it is the registered plugin (red on 137ccad under both the package rootdir and the workspace rootdir).
…tears the adapter down Review 2026-10-21, remediation plan 5, larger #1 (lane 2's found-not-fixed #1, #3, #4). ``session/active.py`` ``release()`` ran ``clear_session_files()`` unconditionally, so three things went wrong on every web ending: (a) The reaper's ``state_file_replaced`` release of session A deleted the state, topics and parking files a later session B had just written. ``release()`` now reads the state file first and touches nothing when it names another session. (b) The ``mode=ended`` state the end paths write (``persist_session_end``) was deleted with the rest, so ``/api/session/state`` answered ``{}`` and the SSE summary view (``_render.py`` ``mode == "ended"``) was unreachable for browser sessions. ``release()`` keeps a saved (``mode=ended``) state file; an unsaved or id-less one still goes, so a live-looking claim owned by this process's pid never outlives the slot. The marker is never written by ``release()``: it means "persisted" and only the code that persisted may say so. (c) Stop, agent exit, grace expiry, the reaper and shutdown saved the session (F-WEB-02) but never ran ``teardown_session``, so Kiro's agent-JSON restore and the persona-file unlink happened only on End. ``_grace._end_before_release`` (was ``_persist_before_release``) now runs ``end_session_common(raise_if_not_persisted=False)``: the same persist-then-teardown, in the same order, the End route and the CLI's ``cleanup_on_exit`` already run. A later End on the kept ended file is now idempotent (200 after an idempotent teardown) rather than 404, which the End dialog reads as "could not end, try again" and keeps open; lane 2's repeated-end test pins the new answer and that nothing is persisted twice.
…wo natural verbs, four reclassified (pass 10 Q5) Council pass 10 Q5 (lane A's RC3). Seats split three ways (leave / convert all nine / leave); the ruling takes the text seriously and the label honestly: - #3 "A struggle **repair** carries an energy demand of its own, which SHALL be derived once …" -> "A struggle **repair** SHALL carry an energy demand of its own, derived once …": the modal sits on the sentence's own finite verb instead of an inserted relative clause. - #6 "Rule 3 SHALL be: It names the plan's next milestone …" -> "It SHALL name the plan's next milestone …": the scaffold stated nothing; the obligation is the sentence's. - #1, #2, #8, #9 keep their text but move from `inherit-shall` to `modalise`: they are closing or tail sentences, not list items under "It SHALL apply these rules", so the kind now says what happened (one modal, one verb form; the plural "need" keeps its form). - #4, #5, #7 stay `inherit-shall`: genuine list items under that lead-in. Checker: 0 fail, 29 warn, 452 drift (inherit-shall 3, modalise 30). Strict valid. No test or doc cites either changed sentence (the headings they sit under are unchanged).
Summary
Connects the three disconnected systems (content pipeline, review, study sessions) into a single feedback loop:
studyctl study "Python Decorators"resolves toTopicConfig(name="Python")via 4-tier cascade (exact name → substring → tag → difflib fuzzy). Interactive picker on ambiguity.studyctl webreview on next loadget_course_stats()correlated subquery replaced with CTE + window function (O(n²) → O(n))Test plan
🤖 Generated with Claude Code