fix(dev-team): address session-review findings #2103-#2111 - #2116
Merged
Conversation
…published skills/ dir scripts/assemble-docs.sh copies only each plugin's docs/ directory into the published MkDocs site — never skills/ or commands/. build_skills_index.py's generated "File" column linked with a relative ../skills/<name>/SKILL.md path, which resolves in a repo checkout (and passes the repo-tree-scoped lychee link check) but 404s once deployed, since that directory was never copied into the assembled site. Link to the file's GitHub source instead, which resolves both in a checkout and on the live site. Regenerated the three affected catalogs (dev-team, security-assessment, marketplace-dev) and added a regression test asserting every generated link points at github.com rather than a relative path. Fixes #2103 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
…ly diffs change_shape.py's runtime-surface gate correctly treats functional Claude-config markdown (agents/, skills/, knowledge/, .claude/ paths) as runtime surface, so performance-review/correctness-review still run against it. But security-review, domain-review, test-review, and performance-review review a code-level property no markdown file can exhibit, functional or not — an injection vulnerability, a domain-boundary leak, a test-coverage gap, or an N+1 query. Dispatching all four against a single-file skill-markdown diff produced elaborate findings framed as security/domain/ test/performance issues that were really prose/spec-consistency nitpicks. Add a new, independent `is_prose_only` signal (every changed file is .md/.mdx, no functional-config carve-out) and PROSE_ONLY_SKIP_LENSES (security-review, domain-review, test-review, performance-review). Each of those four agents' own `## Skip` clause already self-reports skip on a documentation-only target, so this pays for what would otherwise be a self-reported skip or an ungrounded finding. correctness-review, doc-review, spec-compliance-review, structure-review, naming-review, and arch-review stay in the roster. Deliberately narrower than select_lenses.py's own NON_EXECUTABLE_SKIP_ELIGIBLE allowlist, which considered and rejected filtering security-review/domain-review for its broader "non-executable" category (docs and config/lockfiles/assets) — that gate never widens past .md/.mdx, so the rejection's rationale (security-review's credential-scanning duty over config files) doesn't apply here. Fixes #2104 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
… regexes Both assertions in tests/skills/test_setup_yes_flag.py greeped the whole collapsed SKILL.md rather than the specific bullet they claimed to verify, so each could pass without checking its real target text: - test_setup_never_guesses_stack_under_yes grepped the whole document for "never guess a stack". The intended target (the Conservative-bucket "Unrecognized / ambiguous stack" bullet) actually reads "never guesses a toolchain" — no match. The only text that matched was an unrelated Step 6 mutation-tooling clause, so the test passed off the wrong occurrence. - test_project_init_yes_auto_confirms_plan_and_keyless_pair's first assertion's `three-column plan.*proceed` alternative, applied against the whole document collapsed to one line, could span from the Step 3 bullet to any later, unrelated "proceed" occurrence — several exist — making that branch nearly always true regardless of doc content. Bound both to their specific target bullet via section_outside_code(), mirroring the pattern already used elsewhere in this file, and fixed the first assertion's wording to match what the bullet actually says. Fixes #2105 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
…on path Diagnosis: the session digest's gate.gate_ran_absent: 68/68 (100%) finding does NOT mean the pre-commit gate never runs. Two things checked out clean: - hooks/pre_commit_review.py is documented as an intentional no-op (#1886): the review-corroboration gate deliberately moved from `git commit` to `gh pr create` time (hooks/pre_pr_review.py, correctly registered in settings.json). Its absence from settings.json is by design, not a defect. - .husky/pre-commit (the real git-native gate #2037 instruments) correctly emits a `gate-ran` boundary event at every exit point, verified live in this session: three ordinary commits each produced a correlated `gate-ran-allow` record in .claude/metrics/boundary-events.jsonl. The actual bug: `cmd_sync` (session_report.py --sync-out, the path that durably archives onto .claude/metrics/session-digest.jsonl at every SessionStart and feeds /session-review's cross-session digest) called extract_maintainer() without its `boundary_events_path` argument. That parameter has no internal default — `_read_gate_ran_events(None)` returns `[]` unconditionally — so every non-bypassed commit attempt in every synced session classified `gate_ran_absent`, regardless of whether the gate actually ran. `cmd_sync` already derives the session's own `.claude/metrics/boundary-events.jsonl` path two lines below (for plugin_version resolution, #2018); thread that same path through to the gate correlation too. Added two regression tests reproducing the exact symptom against `--sync-out` (one proving a real gate-ran-allow event now correlates instead of reading absent, one proving a session with no matching event still correctly reports absent) — confirmed the first fails against the pre-fix code. Fixes #2106 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
Session digest found /build and software-engineer carry the highest correction rates in the plugin (correction_rate_by_skill.build: 0.8689 vs. the next highest at 0.052; correction_rate_by_agent.software-engineer: 1.05 — more corrections than dispatches). quality-gate-pipeline's Phase 2 already states the right bar (tests, build, lint as "Required Evidence" before any completion claim), but nothing in /build's own per-step cadence made it mandatory at the moment a checkbox actually flips. The per-step mini-cycle hard-gates on tests (TEST phase, sub-step 2) and re-runs them after REFACTOR (sub-step 3) — but REFACTOR can rename/restructure code in ways a test suite alone won't catch, and neither lint nor type-check were ever mentioned in the per-step cycle at all, so a step could reach "Mark step done" (sub-step 5) with a stale or unclean lint state that only surfaces later, in Step 6's backstop review. - build/SKILL.md sub-step 3 (REFACTOR): require lint/type-check tools, when the project has them, to run and pass alongside the re-run tests. - build/SKILL.md sub-step 5 (Mark step done): require that evidence be fresh from this session before flipping any checkbox — not recalled, not assumed. - agents/software-engineer.md: added a hard Constraints bullet making quality-gate-pipeline Phase 2's evidence requirement mandatory before signaling any step or task done, including outside /build's own cadence. Fixes #2107 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
… entry Investigated the two churn-heaviest files from the session digest: - testimprove_phase_scope_guard.py does not exist anywhere in this repo's git history (git log --all finds nothing, on any name variant). Given this repo's recent Wave 3/4 script-consolidation PRs (#2083-#2088) renamed/merged a large slice of the telemetry scripting tree, the digest's filename most likely refers to something renamed away since — or the digest itself (an AI-authored report) mis-transcribed a real path. Nothing to investigate further against a file that was never real. - bash_failure_taxonomy.py's churn was three legitimate review rounds fixing genuinely hard compound-shell-command edge cases (#2085's PR history), each already closed with its own regression test at the time. The module also carries an explicit, deliberate "self-contained by design (post plan-review-design)" policy — it intentionally keeps its own shell-segmentation logic rather than importing session_log's, for a stated privacy-boundary reason. A "consolidate duplicate parsers" check would contradict that already-reasoned decision, so none was added here. That same PR's review history did surface one real, mechanically-checkable, twice-reported defect that was never ratcheted: round 2 fixed churn_recurrence.py's render_text() reading report["window"] via a bare index (KeyError for a caller rendering rank_all_files()'s own output, which never sets that key); round 3's very next review pass found the IDENTICAL bug in churn_coupling_report.py's sibling render_text(). Two occurrences of the same fact is this repo's own trigger to convert it into a repo_invariants.py CHECKS entry — done here, one round late. Fixes #2108 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
…lls into software-engineer Session digest found software-engineer dispatches carrying far more context per dispatch than any other agent type (context_per_dispatch: 13,072,914 vs. fork: 3,396,501 / general-purpose: 6,579,396 / quality-reviewer: 6,405,061), without a matching accuracy payoff (only 20 dispatches, highest correction rate in the digest). Dispatched token-efficiency-review to investigate. Finding: software-engineer's `skills:` frontmatter (ADR 0028: every agent with a `## Skills` section must preload every listed skill on every dispatch, unconditionally) declared nine skills, five of which the agent's own body already documents as conditional — "invoke when structuring new services" (hexagonal-architecture), "invoke when modeling business domains" (domain-driven-design), "invoke when implementing APIs" (api-design), "invoke when modifying ... legacy code" (legacy-code), "invoke when assessing ... mutation" (mutation-testing). These five total ~68KB (~17K tokens) of SKILL.md prose loaded as a per-dispatch baseline regardless of whether the step ever touches any of them — and because a subagent's context gets cache-read again on every subsequent turn, this baseline compounds across a many-turn implementation dispatch. ADR 0028's gate only requires every FRONTMATTER-declared name to trace back to the `## Skills` section text, not the reverse — so removing these five from `skills:` while keeping their "invoke when X" bullets in the section (now split into a Preloaded/On-demand grouping) satisfies the gate and stays fully documented and invocable, just no longer force-loaded on every dispatch. quality-gate-pipeline, test-driven-development, systematic-debugging, and code-review stay preloaded — quality-gate-pipeline in particular backs #2107's hard self-verification constraint and must not be dropped. Fixes #2109 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
Session digest found a large gap between two rework counters (retried_bash_commands: 3947 vs. repeated_verify_runs: 1) with no per-skill/ per-agent breakdown to trace the volume to a specific stop-condition gap. signals.track_bash's retry detection moves from a post-loop histogram sum (sum(n-1 for n>1) over a thread's whole bash-command Counter, computed after all records in a transcript file were processed) to live, per-event detection: each retry is now attributed, at the moment it's detected, to whichever skill/agent is the thread's current sticky active pointer (the same pointer accumulate_skill_agent_signals maintains and the existing correction_by_skill/by_agent signal already attributes against) -- falling back to "unattributed" when neither is set. The scalar retried_bash_commands total is now derived as the sum of the by_skill breakdown, never a second independent computation, matching the lesson from #2108's window-key bug. Threaded the new retried_bash_commands_by_skill/by_agent fields through both extractors end to end: extract_maintainer, extract_downstream/combine, sync_record (the --sync-out per-session record durably archived at every SessionStart), and rollup (the cross-session aggregate /session-review reads) -- including the peer-input sanitization pass (_normalize_name_dicts) a synced record's name-bearing dicts already get, extended to cover these two new fields against the same hostile-key threat model as accuracy.by_skill/ by_agent. Fixes #2110 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
… token count #2109's software-engineer.md edit changed its measured token size; the hand-maintained ~Tokens column in agent-registry.md is a real, checked figure (scripts/measure_tokens.py --verify, run by the pre-push gate) and was left stale, flagged DEVIATED at 13.7% drift (2,122 declared vs. 2,458 measured). Refreshed to the current measured value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01URnco7Z5bKfKBMdNiZGQL4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nine issues filed from the 2026-09-04 session-review digest, addressed as one batch (each on its own investigation, several with an unexpected root cause once traced):
docs/skills.md's generated "File" column linked skills via a relative../skills/...path;scripts/assemble-docs.shnever copiesskills//commands/into the published MkDocs site, so the link 404s live. Now links to GitHub source. Filed docs: hand-written docs pages have ~134 more cross-tree links that 404 on the published site #2113 for the same defect class found in ~134 hand-written links elsewhere in the docs./code-review'ssecurity-review/domain-review/test-review/performance-reviewall fired on markdown-only diffs (including functional Claude-config markdown), producing ungrounded findings. Newis_prose_onlysignal drops those four while keepingcorrectness-review/doc-review/etc.test_setup_yes_flag.pyweren't actually scoped to the bullets they claimed to verify (one matched an unrelated section, one could span to any later occurrence after collapsing whitespace). Bounded both withsection_outside_code()..husky/pre-commitcorrectly emitsgate-ranevents, verified live in this session), butcmd_sync(the--sync-outpath that feeds/session-review) never passedboundary_events_pathto the correlator, so every synced session read as "gate never ran" regardless of reality./buildandsoftware-engineer, closing the gap between the TEST-phase hard gate and the "mark done" moment that a REFACTOR pass could silently regress.testimprove_phase_scope_guard.py) doesn't exist in this repo's history (likely stale/hallucinated digest data). The other's real repeated defect — the identicalreport["window"]bare-index bug fixed in two sibling renderers across two review rounds — is now arepo_invariants.pyCHECKS entry.software-engineerunconditionally preloaded 9 skills (ADR 0028) on every dispatch; 5 are already documented as occasion-specific ("invoke when..."). Trimmed to 4 core preloaded skills (~17K tokens/dispatch saved), verified via a dispatchedtoken-efficiency-review.retried_bash_commandshad no skill/agent breakdown to trace 3947 retries to a cause. Retry detection moved from a post-loop histogram sum to live, per-event attribution against the same sticky skill/agent pointer correction turns already use, threaded through both extractors,--sync-out, and--rollup.Test Plan
python3 -m pytest plugins/dev-team/tests tests/repo tests/agents tests/commands tests/docs tests/knowledge tests/stack_aware tests/skills tests/scripts tests/hooks -q -n auto --dist loadgroup— 10,251+ passed, 0 failed at every commit in this branchpython3 scripts/check_md_references.py— cleanscripts/ci-local.sh(full local CI mirror, run bypre-push) — all checks green, including the Python 3.10 floor slice and the agent-registry token-drift check/harness-audit's own review-agent/token gates: N/A (out of this PR's scope for chore(harness): audit never-observed agents and skills for removal or better triggering #2111 — see issue comment)Closes #2103
Closes #2104
Closes #2105
Closes #2106
Closes #2107
Closes #2108
Closes #2109
Closes #2110
Closes #2111
🤖 Generated with Claude Code
Generated by Claude Code