fix(session-flow): report retro chain coverage and split a comma-joined --sessions

[kyle-sexton]

Closes #1980

Summary

Two ways retro's chain-scoped path produced a wrong answer with no error signal: a comma-joined
--sessions list resolved to nothing, and a chain walk that terminated early was indistinguishable
from a genuinely short chain.

Fix

--sessions comma splitting. The option is declared nargs="+", so --sessions a,b,c was
consumed as one literal token that matched no transcript, and the run reported 0 with transcript
for a chain whose transcripts all existed. Tokens are split on , after parsing — a session id
never contains one, so the split cannot change the meaning of a correctly space-separated
invocation. Empty fragments (a,,b, a trailing comma) are dropped rather than passed on as an id
that cannot exist; a value resolving to no ids at all reaches the existing usage error (exit 2).

chain_coverage. Multi-session output gains requested / found / available / ratio.
available counts the transcripts present in the base directory — the per-project transcript
directory — which is the denominator the previous_handoff walk structurally cannot see. It is
coverage evidence for a reader, not a filter: some sibling transcripts will belong to other work,
which is exactly why the skill surfaces the ratio rather than the parser widening the chain. The
same ratio also rides in the human-readable summary, so it is visible without reading the
structured field. An unreadable base directory degrades available to null instead of failing
the parse.

Skill contract. retro's SKILL.md and context/session.md now require stating the discovery
basis, and forbid presenting a low-coverage chain retrospective silently: below a ratio of ~0.5,
name found and available and offer --sessions with the ids enumerated.

Verification

• plugins/session-flow/skills/retro/scripts/test_parse_transcript.py — 35 passed (was 30). New
  cases: comma-joined list resolves the same list as the space-separated form and keeps its order
  (first id = current session); mixed separators with empty fragments; a --sessions , value that
  yields no ids exits 2; coverage reported as 2-of-5 with ratio 0.4 and the ratio present in
  summary; full coverage reports ratio 1.0.
• plugins/session-flow/skills/retro/scripts/parse-transcript.test.sh — passes.
• ruff check and ruff format --check — clean.
• scripts/check-changed-skills.sh origin/main — retro PASS, 0 errors.
• scripts/check-changelog-parity.sh --check-bump origin/main and --check-order, and
  markdownlint-cli2 on the three touched markdown files — clean.

Fresh-docs mandate: no WebFetch was required and none was performed. This changes a script's own
CLI behavior, its JSON output shape, and skill prose — no plugin manifest field beyond the version
bump, no hook contract, no documented harness behavior.

Related

• Refs perf(hook-utils): cut three subprocess spawns per hook invocation #1979 and fix(guardrails): stop blocking a PowerShell call of a constant target #1974 — other fixes draining the same audit inbox. No file overlap with either:
  session-flow carries no copy of the shared hook library, so it is untouched by the 16-plugin
  version bump in perf(hook-utils): cut three subprocess spawns per hook invocation #1979.
• The same audit report's third finding (a subagent completion-signal contract in orchestrate)
  overlaps an audit item resolved separately and is deliberately out of scope here.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @kyle-sexton's task in 3m 52s —— View job

---

Review: fix(session-flow): report retro chain coverage and split a comma-joined --sessions

• Gather context (PR diff, changed files, custom instructions)
• Read parse_transcript.py diff (comma-split + chain_coverage)
• Read SKILL.md / context/session.md diff
• Read test_parse_transcript.py diff
• Read CHANGELOG.md / plugin.json diff
• Trace edge cases (duplicate ids, empty-chain path) against the new invariants
• Post final review

Reviewed against origin/main (single commit 55e41ef). Scope matches the changed-files list; no unrelated code was touched.

Overall

The fix is well-targeted and the reasoning in the commit message/PR body checks out against the code:

• nargs="+" really does swallow a,b,c as one token (confirmed by tracing _parse_legacy_or_argparse → main()), and the post-parse comma-split at parse_transcript.py#L752-L768 is the right layer to fix it at — it can't change the meaning of an already-correct space-separated call, and an all-empty result correctly falls through to the existing usage error (verified by tracing into main()'s if ns.sessions: / if not session_id: checks).
• chain_coverage in build_multi_session_output is a sound way to surface the walk's blind spot — it's explicit about being evidence, not a filter, and the available == 0/None → omit-ratio handling is correct and matches the docstring.
• Version bump (0.17.24 → 0.18.0, minor for an additive field + additive CLI parsing) and CHANGELOG entries are appropriately scoped and match the code.
• Skill prose (SKILL.md, context/session.md) states the discovery basis and the ~0.5 threshold consistently in both places.
• New tests are well-named and cover the stated cases (comma-joined, mixed separators + empty fragments, all-empty → exit 2, partial coverage ratio, full coverage ratio).

Findings

1. chain_coverage.ratio can exceed the documented 0.0–1.0 range when --sessions contains a duplicate id.
parse_transcript.py#L617-L641 increments transcripts_present once per entry in session_ids, but available counts distinct *.jsonl files in base_path. The new comma-split at L752-L768 strips/filters empty fragments but never dedupes tokens. --sessions sid-a,sid-a,sid-b against a base dir holding only sid-a.jsonl/sid-b.jsonl yields requested=3, found=3, available=2, ratio=1.5 — a coverage percentage over 100%, contradicting the docstring's own # found / available; omitted when available is 0/null / "0.0-1.0" contract at L33-L36 (traced statically; I wasn't able to execute the script in this sandbox to confirm the exact numbers). Duplicate ids were already possible pre-PR (nargs="+" allows repeated space-separated tokens), but there was no ratio invariant to violate before this change — and the skill's own remediation text ("offer --sessions with the ids enumerated") is exactly the human-driven path most likely to introduce an accidental repeat. Worth deduping ns.sessions (order-preserving, since order encodes current-vs-previous) alongside the comma-split, and adding a test with a repeated id.
Fix this →

2. (Minor) The pre-existing empty-session_ids path never gained a chain_coverage key.
parse_transcript.py#L596-L602 (unchanged by this PR) returns early without chain_coverage, while the docstring at L29-L36 now documents it as part of every multi-session response. This is reachable only via --chain-from with an empty walk and no --current-session — in this skill's own invocation pattern (context/session.md), --current-session is always passed alongside --chain-from, so session_ids can't actually be empty there, but a direct script caller following the docstring's --chain-from example without --current-session could still hit it and then find chain_coverage missing from an "error"-status response. Low severity given the skill never triggers it, but worth a one-line fix for consistency with the new contract (either add an empty/None chain_coverage block there, or note in the docstring that it's the one status where the field is absent).

Neither finding blocks the PR's stated purpose (both original bugs — comma-split resolution and silent short-chain retros — are fixed correctly); they're edge cases in the new chain_coverage field's own invariants.

[claude]

Claude finished @kyle-sexton's task in 1m 57s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Review parse_transcript.py changes (comma-splitting, chain_coverage, base directory handling)
• Review test file changes
• Review SKILL.md / context/session.md prose changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

No security issues found.

This PR is scoped to a local, stdlib-only Python CLI script (parse_transcript.py) plus its tests and skill prose. I reviewed each changed file for injection surfaces, authorization/trust-boundary issues, path traversal, and unsafe data handling:

• --sessions comma-splitting (parse_transcript.py:749-767): tokens are split on , and stripped/filtered for emptiness via pure string operations — no eval, no shell interpolation, no regex with attacker-controlled backtracking shape. This doesn't change the trust model: --sessions already accepted arbitrary caller-supplied strings as discrete tokens via nargs="+" before this change (including anything a caller could put in a single arg, e.g. ../../x), and those tokens already flowed into base_path / f"{session_id}.jsonl" (parse_transcript.py:482-483, unmodified by this PR). Comma-splitting a token into more tokens of the same kind doesn't widen what was reachable — it's not a new path-traversal surface introduced here, and this diff doesn't touch how session_id is turned into a path.
• chain_coverage / available count (parse_transcript.py:640-666): base_path.glob("*.jsonl") is wrapped in try/except OSError and degrades to None rather than raising — no information disclosure beyond a count of files already implicitly readable by the pre-existing per-session parse, and no shell/subprocess involved.
• No subprocess, os.system, eval/exec, template rendering, or network calls anywhere in the diff.
• plugin.json/CHANGELOG.md changes are a version bump and prose only — no permission or manifest-contract widening.
• SKILL.md/context/session.md changes are prose instructing the skill to state its discovery basis and offer --sessions with explicit ids; the example invocation passes ids as separate shell args (not a single interpolated/comma string), so no injection surface there either.

No CI/workflow files are touched by this PR, so there's nothing in scope for the pull_request_target/permissions/supply-chain lane.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 55e41efc33

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 2m 43s —— View job

---

Code review: fix(session-flow): report retro chain coverage and split a comma-joined --sessions

• Gather context (PR diff vs origin/main, changed files)
• Review parse_transcript.py diff (new commit 92b50c9: dedupe fix for chain_coverage)
• Check whether prior review findings (duplicate-id ratio >1.0, empty-chain chain_coverage key) were addressed
• Review test coverage for the new commit
• Review SKILL.md / session.md / CHANGELOG / plugin.json for consistency
• Post final review

Summary

This update (commit 92b50c9) fixes the duplicate-id coverage bug both the earlier Claude review and Codex flagged against 55e41ef. I traced the fix statically (could not execute pytest/ruff in this sandbox — commands required approval that wasn't available; see note at the end) and it holds up.

Verified: duplicate-id fix is correct

build_multi_session_output now does session_ids = list(dict.fromkeys(session_ids)) immediately after the empty-list early return, before any counting happens. This is the right layer: it's the single function every entry point (--sessions, --chain-from with --current-session prepend, and the legacy positional path) funnels through, so a caller can't bypass it.

• Order preservation is correct. dict.fromkeys keeps first-occurrence order, which matters because index 0 drives the "current" vs "previous" role assignment at L629. The new test_multi_session_dedupe_preserves_first_seen_order test exercises exactly this (sid-curr sid-prev sid-curr → roles stay current/previous, not relabeled).
• The ratio invariant now holds. found (transcripts_present) is a count of distinct deduped ids with an existing transcript file, and available counts distinct *.jsonl files in the same base dir — since each distinct id maps to at most one file, found <= available is now structurally guaranteed, so ratio can no longer exceed 1.0. This directly resolves both the earlier Claude finding and the Codex P2 comment on parse_transcript.py:682.
• extract_chain_from_handoff's own seen guard is correctly left in place (per the code comment) — it terminates a pointer-cycle walk, which is a different concern from de-duplicating the final aggregation list, so keeping both isn't redundant.
• chain_coverage.requested and the pass-status comparison read the deduped list, so --sessions sid-a,sid-a sid-a correctly reports requested: 1 and status: pass rather than under-reporting coverage for a chain that's actually complete. Confirmed by test_multi_session_repeated_id_counts_once.

Test additions (test_parse_transcript.py +41 lines, 2 new cases) are well-targeted at exactly this regression and match the PR's reported 37-passed count.

Minor, non-blocking

1. Docstring for requested doesn't mention that duplicates are now collapsed. L34 still reads "requested": N, # session-ids this run was asked to parse. Since requested now reports the de-duplicated count (per the fix at L619), a reader relying only on the docstring (not the CHANGELOG) could reasonably expect it to echo the raw --sessions token count. A one-clause addition (e.g. # distinct session-ids this run was asked to parse — repeats count once) would keep the module docstring self-sufficient.

2. Pre-existing, still open: the empty-session_ids early return has no chain_coverage key. L596-L601 still returns before the field is computed, while the module docstring documents chain_coverage as part of every multi-session response shape. This was raised on the prior commit and untouched by 92b50c9 — still low severity (the skill's own invocations always pair --chain-from with --current-session, so this path isn't reachable from context/session.md), but a direct-script caller following the --chain-from-only usage line in the docstring would still hit an "error" response missing the field the docstring promises.

Neither item blocks — both are documentation/consistency polish on a fix that is otherwise correctly scoped and well-tested.

Note on unexecuted verification: pytest/ruff invocations required approval that wasn't available in this run, so the above is a static trace of the diff rather than an executed run; the PR body's own reported results (37 passed, ruff clean) are consistent with what the diff implies.

[claude]

Claude finished @kyle-sexton's task in 1m 12s —— View job

---

Security review

• Gather context (PR diff, changed files)
• Review parse_transcript.py changes (dedup fix in build_multi_session_output, comma-split, chain_coverage)
• Review test file changes
• Review SKILL.md / context/session.md prose changes
• Review plugin.json / CHANGELOG.md changes
• Post findings

No security issues found.

Reviewed the full diff against origin/main (commit bebc2dc) scoped to plugins/session-flow/**, which matches this PR's 6 changed files exactly. The new commit since my last pass (92b50c9) adds the id-deduplication fix in build_multi_session_output — this is a correctness fix (it resolves the ratio > 1.0 finding from the earlier review round and from Codex's inline comment), not a security-relevant change:

• parse_transcript.py#L617 — session_ids = list(dict.fromkeys(session_ids)) deduplicates via pure, non-attacker-influenced control flow (order-preserving, first-occurrence-wins). No injection surface, no new trust boundary.
• This still doesn't touch how a session_id is turned into a filesystem path (base_path / f"{session_id}.jsonl", unmodified by this PR) — the trust model for --sessions tokens is unchanged from before this PR.

Everything else (comma-splitting, chain_coverage/available glob-with-try/except OSError, the new tests, and the SKILL.md/context/session.md/CHANGELOG.md/plugin.json prose and version-bump changes) matches what I reviewed in the prior pass: stdlib-only Python, no subprocess/eval/exec/network calls, no shell interpolation, and no CI/workflow files touched.