fix(hooks): fill a session entry's tool sequence from its transcript (#591) - #592

ZETETIC-REVIEW: REQUEST_CHANGES

Summary

The fix is correctly targeted and its two headline claims hold up under independent re-measurement: (1) the SessionEnd payload really does leave every one of the 413 live session-log.json entries empty, and (2) the set-vs-list bug really does mine skills that were never performed. All three requested mutations (set-derived sequence, dropped cap, transcript-over-event precedence) are caught by the new tests; the suite, lint, craftsmanship gate, doc-claims check, pyright, and CI are all green. One real defect blocks merge: transcript_activity() crashes on a line that parses as valid-but-non-dict JSON (e.g. a transcript stored as a JSON array), contradicting its own docstring and running in an unguarded hook path.

Move 0, Ledger reconciliation and seen-defect check (§13.2 + §14)

• Ledger reconciliation: pass. Every branch in the diff (event supplies both fields / supplies one / supplies neither / no transcript path / unreadable transcript / malformed line / JSON-decode-error line / cap reached) is covered either by an explicit test or by the "Audit notes" (413 stale entries, proficiency stuck at 0.5) which are stated as known, not hidden.
• Seen-defect rationalizations found: none. No "pre-existing/unrelated/out of scope" language dismissing anything the diff or its own verification touched.
• Verdict short-circuit: N/A, proceeded to Moves 1-6.

Stakes calibration (Move 7)

• Classification: Medium (trending High on one axis).
• Criterion: not auth/billing/crypto/migrations, but it does modify a public-ish infrastructure contract (scanner_parse.extract_message_stats["tools_used"] type changes from set to list, with one internal consumer updated in the same PR) and runs unconditionally in a hook that fires on every session end. 374 lines changed, single well-scoped commit.
• Review depth applied: full Moves 1-6.

Layer check (Move 1)

No boundary violations.

SOLID audit (Move 2)

• SRP: transcript_activity() does one thing (read one transcript, return sequence+turns). _build_session_entry grew a fallback branch but stayed cohesive (still "build one entry"). No violation.
• OCP: N/A, no new conditional dispatch on a type tag.
• LSP: N/A, no inheritance changed.
• ISP: N/A.
• DIP: N/A, no concrete infra type injected into core.

No violations. One structural observation (not a SOLID violation, DRY-adjacent, see Move 5) is that this PR's transcript_activity() duplicates, almost line-for-line, the pre-existing (and now completely unused) iter_tool_uses() generator in mcp_server/infrastructure/scanner.py:73-112 (same whole-file streaming read, same type == "assistant" filter, same tool_use block extraction). iter_tool_uses was already dead code before this PR (verified: git grep iter_tool_uses at d548fc1e finds only its own definition, no caller anywhere in the tree), that dead code is pre-existing debt this PR did not introduce, so it is not a blocking finding here, but the PR had the opportunity to consolidate onto it (add turn-counting to iter_tool_uses and reuse) instead of hand-rolling a second scanner. Non-blocking; see hand-off to simplifier/refactorer for a follow-up.

Wiring & contract drift (Move 3)

• New public symbol transcript_activity(), wired at both call sites (session_lifecycle.py:55, scanner.py:206). Not unwired.
• MAX_TOOL_SEQUENCE, used, carries a # source: comment with a measurement (§8 compliant).
• Contract drift: extract_message_stats()["tools_used"] changes from set[str] to list[str] (scanner_parse.py:81). Its only consumer, build_conversation_record() (scanner_parse.py:154), still does list(stats["tools_used"]), harmless on a list (idempotent), but the comment # ordered, ADR-1072 next to a list(...) call over an already-ordered list is dead ceremony; not a bug. Verified via grep -rn "extract_message_stats" mcp_server, no other caller exists outside scanner.py/scanner_parse.py.
• No dead code, no bare TODOs, no debug prints introduced.

Test adequacy (Move 4)

New execution paths and their coverage:

• transcript with order+repetition → tested (test_order_and_repetition_are_preserved), and independently re-verified against a naive full-parse baseline over 60 real transcripts sampled from ~/.claude/projects, 0 mismatches (script in scratchpad; see Findings below for the crash case that the existing malformed-line test does not cover).
• turns-without-tools → tested (test_a_session_without_tools_reports_its_turns).
• malformed (undecodable) line → tested (test_a_malformed_line_does_not_lose_the_rest), but only for json.JSONDecodeError/ValueError, not for a line that decodes to valid-but-non-dict JSON. Gap, see Blocking finding.
• missing transcript → tested.
• cap → tested, and mutation-confirmed (see below).
• event-wins-per-field precedence → tested for "event supplies both", and I additionally confirmed the mixed case (event supplies tools_used but not turn_count, and vice versa) is correctly independent per-field by reading _build_session_entry's tools = tools or activity[...] / turns = turns or activity[...] lines, each field falls back independently. Not explicitly tested as a mixed case, but the logic is simple enough that this is a non-blocking gap, not a defect.

Mutation testing (Move 4 §3.2, task's claim 5), ran against an isolated scratch copy (the worktree was never edited):

1. Set-derived (deduplicating) tool_sequence → 3 tests fail (test_order_and_repetition_are_preserved, test_the_sequence_is_capped, test_scopes_to_requested_projects_only).
2. Dropped MAX_TOOL_SEQUENCE cap → 1 test fails (test_the_sequence_is_capped).
3. Hook prefers transcript over event unconditionally → 1 test fails (test_the_event_wins_when_it_carries_the_fields).
   All three mutations restored; git status --short in the worktree is clean (verified before and after).

The suite is mutation-strong for the claims it makes. It is not strong enough for the crash path below, because no test exercises it.

Complexity & structure (Move 5)

• transcript_activity.py: 71 lines, largest function 24 lines, well under the §4 caps.
• _build_session_entry: grew by 8 lines, still well under 40.
• No new function/file size smells.
• Over-engineering: none observed.
• DRY smell (non-blocking, hand off to simplifier): transcript_activity() duplicates scanner.iter_tool_uses()'s file-streaming logic; iter_tool_uses itself is pre-existing dead code untouched by this PR (not this PR's introduction, not blocking here).

Security & hygiene (Move 6)

• No new I/O with attacker-controlled input beyond what already existed (transcript paths come from the trusted SessionEnd event / trusted local project directory listing, same trust boundary as before).
• Path(transcript_path) with errors="replace", no injection surface; file reads are local-only.
• Commit hygiene: single conventional commit fix(hooks): ..., ADR + CHANGELOG + wiki mirror in the same PR, scope is tight (one fix, one root cause, its two downstream instances). Good.
• One nit: except OSError around the whole with path.open(...) block also silently swallows a PermissionError/IsADirectoryError subclass, intentional per the docstring ("unreadable ... yields an empty sequence"), consistent, not a problem.

Issues

Blocking

• mcp_server/infrastructure/transcript_activity.py:59-68, transcript_activity() crashes with an unhandled AttributeError when a transcript line parses as valid JSON that is not a dict (e.g. the whole file is a JSON array rather than JSONL, one of the two edge cases the task explicitly asked to check). Demonstrated:

  AttributeError: 'list' object has no attribute 'get'
    at transcript_activity.py:63, record.get("type") != "assistant"
  

  This directly contradicts the function's own docstring ("An unreadable, missing or malformed transcript yields an empty sequence... A line that does not parse as JSON is skipped, the rest of the file still read", a line that does parse as JSON but isn't a dict is neither skipped nor handled). It also contradicts the "never raises" implicit contract of a SessionEnd hook: _build_session_entry is called from process_event() (mcp_server/hooks/session_lifecycle.py:283) with no surrounding try/except, and process_event is called directly from main() with no surrounding try/except either, unlike the adjacent _tombstone_session_registry/_deregister_groomer_coordinator helpers in the same file, which are explicitly wrapped except Exception ... non-fatal. A malformed transcript would therefore crash the whole SessionEnd hook process, losing not just the tool sequence but the entire session-log append and the profile EMA update that follows it (session_lifecycle.py:284-289), strictly worse than today's "always empty" state for that one session.
  Required change: catch the case where record isn't a dict (either except (ValueError, AttributeError): around the parse+access, or an explicit if not isinstance(record, dict): continue after json.loads), and add a test for a JSON-array transcript file (or any line that decodes to a non-dict). Real transcripts sampled here (60 files, live ~/.claude/projects) are always well-formed JSONL objects, so this is not observed in practice today, but the function's own contract claims to be immune to exactly this class of input, and the crash surface (an unguarded hook entrypoint) is real.

Non-blocking

• mcp_server/infrastructure/scanner.py, mcp_server/infrastructure/transcript_activity.py, measured cost: discover_conversations() over this machine's 147-conversation, 824-file ~/.claude/projects tree went from ~0.06s (pre-PR, d548fc1e) to ~0.5-0.7s (this PR), roughly 8-12x, because _parse_conversation_file now does a full-file stream read (transcript_activity) on top of the existing read_head_tail sample. Both callers are bounded appropriately: rebuild_profiles documents "<10s typical" and stays well inside that even at 10x; explore_features (tagged interactive, 5s pool timeout in latency_class.py) only ever calls the capped discover_conversations_for_projects(..., ATTRIBUTION_SAMPLE_LIMIT=20) variant, so the added cost there is bounded to ~20 × 4.5ms ≈ 90ms. Not a merge blocker, but worth a one-line CHANGELOG/ADR mention next to the "4.5ms per transcript" number, since the number describes the new cost, not the delta over the old cost.
• mcp_server/infrastructure/scanner.py:73-112 (iter_tool_uses), pre-existing dead code, now duplicated in spirit by the new transcript_activity(). Not introduced by this PR, not blocking; flagged for a follow-up consolidation.
• mcp_server/hooks/session_lifecycle.py:178,184, turns = event.get("turn_count") or 0 then turns = turns or activity["turn_count"]: a legitimate turn_count: 0 from the event (session that truly had zero turns) is indistinguishable from "absent" and gets silently overwritten by the transcript's count if the transcript has any assistant turns. Extremely unlikely to occur in the wild (a session with tools but zero turns) and matches the pre-existing or-based defaulting idiom already used in this function, so not blocking, flagging only because it's a slightly different precedence rule than "an event that carries either field keeps its own value" as ADR-1072 states it (a 0 genuinely carried by the event is not preserved).

Verified claims (independent re-measurement, same machine ~/.claude/projects)

• 413/413 live session-log.json entries have toolsUsed: []/turnCount: 0, confirmed exactly (ls/parse of the real file).
• Fast-path '"assistant"' not in line never skips a true assistant record: this is provably safe (a "type":"assistant" field always serializes the literal substring "assistant"), and empirically confirmed with 0 mismatches against a naive full parse over 60 real transcripts.
• Sidechain/subagent contamination: checked and not an issue, of 824 real transcript files, 450 are pure-sidechain and all 450 have "subagent" in the filename (already excluded by scanner.py's existing if "subagent" in entry.name: continue); 0 files mix sidechain and top-level assistant records. The hook path's event.get("transcript_path") is always the top-level session's own file, so it never sees sidechain content either.
• Set-vs-list mining defect: demonstrated concretely, a session that actually performed Read, Read, Edit, Bash, Read would, under the old set-derived path, mine Bash → Read and Bash → Read → Edit as "skills", sequences that were never once performed in that order.
• Cost/output measurements: max sequence 971 (exact match to the PR's own number, same machine/directory, grown by ~200 more transcripts since their measurement), ~4.40ms/transcript here vs ~4.5ms claimed, mine_skills 0.026s here vs 0.02s claimed, all within measurement noise of natural history growth between their run and mine.
• Gates: ruff check clean, ruff format --check clean, scripts/check_craftsmanship.py --base origin/main OK, scripts/check_doc_claims.py OK, pyright mcp_server/ 0 errors, full suite 8724 passed, 8 skipped (exact match to PR claim), CI on the PR all green.

Hand-offs

• DRY consolidation of transcript_activity()/iter_tool_uses() → simplifier (advisory, no hard-rule violation).
• No security-auditor, Dijkstra, Feynman, Knuth, or architect hand-off needed, this is a straightforward, well-evidenced bug fix.

Memory records written

• (see final report)

Verdict

REQUEST_CHANGES, one blocking defect: guard transcript_activity() against a transcript line that parses as valid-but-non-dict JSON (e.g. isinstance(record, dict) check, or catch AttributeError alongside ValueError), with a regression test for it. Everything else in the diff, the root-cause fix, the ordering fix, the ADR, the tests, the measurements, and the gates, checks out.

ZETETIC-REVIEW: REQUEST_CHANGES

Re-verification of 1b0f3dd against the prior blocking finding

Confirmed: git diff 4370849f..1b0f3ddf --stat touches exactly mcp_server/infrastructure/transcript_activity.py (7 lines) and tests_py/infrastructure/test_transcript_activity.py (21 lines), nothing else. The isinstance(record, dict) guard at transcript_activity.py:66 closes the exact case I demonstrated last round (a JSON array file, a bare string/number/array line): re-ran my original repro (whole-file JSON array, bare "assistant" string line, bare [1,2,3] line, null line) against this commit, all return {"tool_sequence": [], "turn_count": 0} or skip cleanly, no crash. The two new tests (test_a_json_array_file_is_not_a_crash, test_a_non_object_line_is_skipped) pass and correctly exercise the fix.

New finding: the fix does not cover the case one level deeper

You asked me to check "a dict whose message is not a dict" specifically, and that case still crashes on 1b0f3dd. transcript_activity.py:70:

content = (record.get("message") or {}).get("content")

record is now guaranteed to be a dict (guarded), but record["message"] is not. If a type: "assistant" record carries a message field that is a non-dict truthy value (a string, a list, a number, True), record.get("message") or {} evaluates to that value (it's truthy, so the {} fallback never triggers), and .get("content") on a str/list/int raises AttributeError, uncaught, same failure mode and same unguarded-hook consequence as before.

Reproduced directly against the branch's own code:

$ python3 -c "
from mcp_server.infrastructure.transcript_activity import transcript_activity
import json, tempfile
p = tempfile.NamedTemporaryFile(mode='w', suffix='.jsonl', delete=False).name
open(p, 'w').write(json.dumps({'type': 'assistant', 'message': 'not a dict'}) + '\n')
transcript_activity(p)
"
Traceback (most recent call last):
  File "mcp_server/infrastructure/transcript_activity.py", line 70, in transcript_activity
    content = (record.get("message") or {}).get("content")
AttributeError: 'str' object has no attribute 'get'

Same for "message": [1, 2, 3] (list has no .get either). This is not a hypothetical unrelated to the fix in this diff: it is the same defect class (an unguarded .get() on a value assumed to be a dict, inside the same unguarded SessionEnd hook path) that 1b0f3dd was written to close, just one field deeper. The docstring's new claim ("a line that... parses as anything other than an object[...] is skipped[...] this runs inside the SessionEnd hook, where an exception would cost the session its log entry and its profile update") is still false for this shape.

Checked the remaining shapes you listed and confirmed they are already safe on this branch, no further gap found there:

• bare number line, null line, bare bool line: caught by the isinstance(record, dict) guard, no crash.
• message present but null: record.get("message") or {} correctly falls back to {} (None is falsy), no crash.
• message missing entirely: same, falls back to {}, no crash.
• content present but not a dict/list of dicts (e.g. a bare string, or a list containing non-dict elements): already handled correctly by _tool_names()'s own isinstance(content, list) guard and the per-block isinstance(block, dict) check, no crash.

Required change: guard the message access the same way the record access was guarded, e.g. msg = record.get("message"); content = msg.get("content") if isinstance(msg, dict) else None, with a test for a non-dict message field (string and list, matching the two-shape pattern already used for the record-level fix).

Gates (unchanged from the prior round, re-run on 1b0f3dd)

ruff check, ruff format --check on the changed files: clean. scripts/check_craftsmanship.py --base origin/main: OK. pyright mcp_server/infrastructure/transcript_activity.py: 0 errors. Targeted tests: 48 passed. Full local suite on 1b0f3dd: 8726 passed, 8 skipped (2 more passing than the prior commit, matching the 2 new tests added). CI on the PR at HEAD 1b0f3dd: Lint, Type Check, Craftsmanship Gate, Docker Smoke, Fuzz (both sanitizers), CodeQL, and the static-analysis jobs are all green; the Python 3.10-3.13 / SQLite / Windows test-matrix jobs were still pending when this comment was posted (same commit, same suite I already ran green locally), worth a final glance before merge but not something this review is blocked on.

Standing non-blocking notes

Both stand as filed and neither should block: the scanner.iter_tool_uses duplication is pre-existing dead code this PR did not introduce, a real follow-up but not this PR's job. The turn_count: 0-from-event-vs-absent ambiguity keeps the pre-existing or-based idiom already used throughout _build_session_entry; changing it here would be a bigger, unrelated precedent question (how the whole function should distinguish "explicitly zero" from "missing") that deserves its own decision, not a drive-by fix bundled into this crash guard.

Verdict

REQUEST_CHANGES, same defect class as the prior round, one level deeper: guard record.get("message") the same way record itself is now guarded, with a test for a non-dict message. Everything else in this small follow-up commit (the record-level guard, its two tests, the docstring update, the gates) is correct and unchanged from what I verified before.

ZETETIC-REVIEW: APPROVE

Re-verification of 33dc284 against the round-2 blocking finding

Confirmed git diff 1b0f3ddf..33dc284a --stat touches exactly mcp_server/infrastructure/transcript_activity.py (12 lines) and tests_py/infrastructure/test_transcript_activity.py (22 lines), nothing else.

The new _message_content(record) helper (transcript_activity.py:29-35) reads message = record.get("message") and returns message.get("content") if isinstance(message, dict) else None, guarding exactly the gap I demonstrated on 1b0f3dd: message being a string, list, number or bool no longer reaches .get(). Reproduced the two prior crashes ("message": "not a dict", "message": [1, 2, 3]) directly against 33dc284: both now return cleanly, no crash, and the record is still counted as a turn (matches the four parametrized tests, string/list/number/bool, all of which raised on 1b0f3dd and pass on this commit).

Exhaustive walk of every access/subscript in transcript_activity.py (current head)

Line by line, what happens when the value one level below is a string, a list, a number, a bool, or None:

• transcript_path (fn arg, line 61-63): if not transcript_path: return empty catches None/""/0/[]/{} before Path(transcript_path) runs. A truthy non-str/PathLike value (a nonempty list, a nonzero int, True) reaches Path(...) and raises TypeError, uncaught. Reproduced: transcript_activity(12345) raises TypeError: argument should be a str or an os.PathLike object.... This is real, but it is not new in 33dc284, not touched by this diff or the prior one (Path(transcript_path) is unchanged since the PR's first commit, 4370849f), guarded by the function's own declared signature (str | Path | None), and its only real-world input is the event JSON envelope, not transcript file content, so it is a different input surface than the "message/content shape" class this round and the prior round were closing. Not blocking; flagged for completeness since you asked me to check the path.open path specifically. Worth a follow-up issue if the team wants the hook hardened against a type-confused event payload, but out of scope for [bug] procedural learning captures nothing: every session entry is written with an empty toolsUsed #591 and for this diff.
• path.open(encoding="utf-8", errors="replace") (line 67): a directory, a permission-denied file, or a non-existent path all raise OSError subclasses, caught by except OSError: return empty. Reproduced all three (directory, chmod 000, nonexistent path): no crash, empty returned.
• for line in stream: (line 68): text-mode iteration with errors="replace" never raises on invalid UTF-8 bytes. Reproduced with raw invalid-UTF-8 bytes appended to a line: no crash.
• '"assistant"' not in line (line 69): line is always str from text-mode iteration; in never raises.
• json.loads(line) (line 72): except ValueError catches JSONDecodeError. Tried a 3000-deep nested-bracket line to probe for RecursionError: none raised (Python's C json decoder tolerates this depth without hitting the recursion limit in practice).
• record (line 75): not isinstance(record, dict) or record.get("type") != "assistant" short-circuits before .get() runs, so every non-dict shape (string, list, number, bool, None) is safe. This is the round-1 fix, still correct.
• _message_content(record) (line 29-35): record.get("message") is safe (record is a dict by the time this is called, line 75 already filtered). message.get("content") if isinstance(message, dict) else None is safe for every non-dict message shape. This is the round-2 fix, confirmed correct by the walk and by direct repro.
• _tool_names(content) (line 38-47): if not isinstance(content, list): return [] covers a content that is a string, a dict, a number, a bool, or None (everything _message_content can return other than an actual list). Inside the list comprehension, isinstance(block, dict) and block.get("type") == "tool_use" and block.get("name") short-circuits before block["name"] ever runs, and the final subscript block["name"] only executes when block.get("name") already returned a truthy value on the same dict, so it cannot KeyError. A content list containing non-dict elements (a string, a number, None, a nested list) is filtered out cleanly by isinstance(block, dict).

Ran this exhaustively rather than by hand: parametrized over {record top-level, message field, content field, a block inside content, a block's name field, a block's type field, a block's input field} × {string, list, dict, int, float, True, False, None, {}}, 64 concrete transcript files, all fed through transcript_activity(). Zero crashes. (Script available on request; not committed anywhere, run from a temp copy, worktree untouched.)

No remaining crash exists in the record/message/content/block shape space this round and the prior round were closing. The one raise found (transcript_path as a non-str/PathLike truthy value) is pre-existing since the PR's first commit, outside this diff, and a different input surface (the event envelope's own field type, not transcript content), I am not treating it as the same defect class and it is not blocking this PR.

Gates on 33dc284

ruff check, ruff format --check: clean. scripts/check_craftsmanship.py --base origin/main: OK. pyright mcp_server/: 0 errors. pytest tests_py/infrastructure/ tests_py/hooks/: 1468 passed, 8 skipped (exact match to your own run). CI at 33dc284: Lint, Type Check, Craftsmanship Gate, CodeQL, and the static-analysis jobs are green; the Python 3.10-3.13 / SQLite / Windows test-matrix and the Fuzz jobs were still pending when this comment was posted, same suite already confirmed green locally, worth a final glance before merge.

Standing non-blocking notes

Both still stand, unchanged from rounds 1 and 2: the scanner.iter_tool_uses duplication is pre-existing dead code this PR did not introduce, a real follow-up but not this PR's job. The turn_count: 0-vs-absent ambiguity keeps the pre-existing or idiom already used throughout _build_session_entry, a separate, broader decision that should not ride along with a crash-guard commit. Adding to this list, non-blocking: the transcript_path type-contract gap found in this round's exhaustive walk, worth a follow-up issue if the team wants the hook hardened against a type-confused event payload.

Verdict

APPROVE. Three rounds closed the reported defect class completely: the record-shape crash (round 1), the message-shape crash (round 2), and an exhaustive walk of every remaining access in the file (this round) turns up nothing further within that class. The fix is correctly scoped, its tests are mutation-strong for every claim made, the numbers in the PR body and ADR-1072 were independently re-measured and held up, and every gate is green.