feat(review): abort-on-cheap-blocker, countable test-review, tiered findings by bdfinst · Pull Request #2197 · bdfinst/agentic-dev-team

bdfinst · 2026-09-22T22:51:30Z

Summary

Slices 3-5 of epic #2164's hybrid review flow:

feat(code-review): abort remaining panel dispatches on a cheap-lens blocker #2168 — checkpoint_abort.py: abort remaining opus-tier lens dispatch on a cheap-tier error/high blocker, wired into /build SKILL.md sub-steps 4/6.
feat(agents): countable pre-phase in test-review, as a pilot #2169 — test_review_mechanics.py: mechanical Phase 0 pre-phase for test-review, with [MECHANICAL]/[JUDGMENT] annotations in agents/test-review.md, wired into /code-review step 2b's static-analysis pre-pass.
feat(code-review): tiered findings output with on-demand expansion #2170 — render_tiered_findings.py: Tier-1/Tier-2 /code-review finding rendering with --expand, wired into /code-review step 7 + output-format.md.

This PR also includes a full /code-review --internal backstop review round (12 agents dispatched: naming, performance, spec-compliance, structure, test-review, test-smell, arch, correctness, domain, security, doc, concurrency) and the resulting fix loop, which found and fixed real defects beyond the original three slices:

Two error-severity correctness bugs in test_review_mechanics.py: a multi-line Python signature parser bug that produced false no-assertion errors on black-formatted test functions, and a regex bug (\b boundary) that misidentified RegExp .test( member-access calls as it()/test() declarations.
An unwaived internal-collaborator double in the new test file (missing # double-waiver: B1 markers on three subprocess stubs).
render_tiered_findings.py's _load_findings silently rendered a false "clean pass" for the actual aggregated --json shape (topFindings, not findings) and a per-agent {status, issues} shape.
checkpoint_abort.py's severity floor duplicated finding_signature.py's is_actionable with a case-sensitivity mismatch (now imports it directly); its three CLI --mode handlers were deduplicated into a shared helper.
compare_eval_results.py moved from the shipped plugins/dev-team/scripts/ tree to repo-root scripts/ per ADR 0032 (it's monorepo-dev-only tooling, unlike its cited eval_ablation.py precedent).
build/SKILL.md's abort-check merge wording clarified so a converged fix loop can actually report a clean pass instead of being pinned on the original triggering finding.
A wording conflict ("do not re-report" vs. "report its findings") and a scope mismatch between test-review.md's Hunt prose and its actual test-files-only wiring, both resolved.
Added missing per-language test coverage (C#/Java/Python branches) and CLI error-path coverage the panel's test-review lens flagged as gaps.

Also merges in origin/main's #2195 (verdict ledger / boundary-events write guard), which landed on main while this branch was in progress.

Test Plan

Full pre-push suite green: 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 → 11184 passed, 48 skipped, 0 failed.
ruff check / mypy clean on every changed Python file.
/code-review --internal backstop review round dispatched (12 agents); every actionable (error/warning, high/medium-confidence) finding fixed and independently re-verified against the source, not just the agents' self-reports.
Farley Score on the batch's new/changed test files: 8.2/10 (Good).
python3 scripts/check_md_references.py and python3 plugins/dev-team/hooks/lib/build_knowledge_index.py clean.
Local pre-push gate (scripts/ci-local.sh) passes in full, including the shipped-Python-3.10-floor slice and the semgrep rule-fixture audit.

Closes #2168
Closes #2169
Closes #2170
Part of #2164

🤖 Generated with Claude Code

https://claude.ai/code/session_01KGHpm7gtqNb8NM2Smf76u7

Generated by Claude Code

…batch Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…x severity-floor filtering Slice 1 review checkpoint (#2168) surfaced 9 confirmed findings across concurrency/doc/naming/performance/structure/test/test-smell/spec-compliance/ arch/correctness/domain/security-review and refactor-opportunity-review: - checkpoint_abort.py's merge_findings/compute_round_outcome had no CLI, so SKILL.md prose telling the orchestrator to "call" them was unexecutable (domain-review + arch-review, independently confirmed). Added --mode {abort,outcome,merge} to main(). - The abort-check paragraph was duplicated near-verbatim across build SKILL.md sub-steps 4 and 6 (structure-review + refactor-opportunity-review). Extracted into one shared block matching this file's existing cross-checkpoint-rule convention. - decide_abort's "abort" output key vs compute_round_outcome's "aborted" parameter were inconsistently named (naming-review). Standardized on "aborted". - compute_round_outcome blocked on any finding, not filtered by the shared severity floor (error/warning at high/medium confidence) SKILL.md explicitly binds it to (correctness-review). Added the filter. - _validate_cheap_results didn't validate the agent field, silently corrupting deferredLenses/triggeringAgent on malformed input (correctness-review). Now fails loud. - Documented _finding_key's deliberate divergence from finding_signature.py's canonical identity relation, mirroring gate_retry_state.py's established precedent (domain-review + arch-review). - Added missing test coverage: compute_round_outcome's reason field, --cheap-results-from's file-path branch (test-review). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…r test-review Step 2.2 per-step review checkpoint (#2169) surfaced 13 confirmed findings across concurrency/doc/naming/performance/structure/test/ test-smell/spec-compliance/arch/correctness/domain/security-review: - The only self-computed gating check (no-assertion) scanned the test's title along with its body, so the most common JS/TS naming convention (it('should ...')) never gated (domain-review + correctness-review, independently confirmed). Split region extraction (for boundary-finding) from content scanning (for assertion/await checks), and made paren/brace balancing string/comment-aware to prevent both spurious and missed findings. - Several detection signatures dropped exemption clauses test-review.md documents (fake-timer suppression, mock re-instantiation, qualified tolerated-deviation markers) or omitted common test-declaration forms (async def, it.each, [Theory]/[TestCase], @ParameterizedTest), causing fail-open gaps (correctness-review). - The reused internal_double_detector.py's test-directory scoping gap (only test/tests-named dirs) was undisclosed, and its failure paths degraded to suggestion-severity, indistinguishable from a clean pass (arch-review). Added explicit out-of-scope/unavailable findings and a doublingCheckRan flag. - No drift guard tied test-review.md's [MECHANICAL] annotations to this script's _GATING_CATEGORIES (domain-review); test-review.md itself still named a nonexistent severity field on the reused detector. - Magic values, an unclear function name, duplicate region extraction, and an unconditional subprocess spawn coupling unrelated tests to the detector (naming/structure/test/test-smell-review). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…via /agent-eval Adds Phase 0 to test-review.md's Protocol: the caller computes test_review_mechanics.py's result per file (no *-review.md agent has a Bash tool) and supplies it as context, mirroring /code-review's static-analysis pre-pass architecture. A mechanicalFail:true result skips Phase 1/2 for that file; mechanicalFail:false surfaces its warning-tier findings alongside Phase 1/2. Adds compare_eval_results.py, a stdlib-only gate diffing two /agent-eval actuals JSON files per fixture (true-positive count for defect fixtures, false-positive count for clean fixtures) and exiting non-zero on regression. Verified via a real /agent-eval run across test-review's 13 fixtures; 4 fixtures showed a single-trial diff against the hard bound, but 3-trial spot-checks put every one within the before-side's own observed sampling spread — documented as a judgment call in the plan's Risks & Open Questions, not a gate weakening. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…h, harden regression gate Step 2.3's review checkpoint (7 agents) cross-confirmed that test_review_mechanics.py had no dispatch site anywhere in the repo — test-review.md's Phase 0 named "the caller dispatching this agent" as responsible for computing and supplying the mechanical result, but no skill named that caller, so Phase 0 silently never fired. Adds a third pre-pass block to /code-review step 2b, alongside the established repo_invariants.py and internal_double_detector.py precedents, scoped per test file (Phase 0 needs each file's own result). Pinned by a new content-guard marker test. Hardens compare_eval_results.py's regression gate: a defect fixture present in "before" with no recorded result in "after" (agent errored or timed out) is now scored as a regression instead of silently skipped as "not comparable" — total detection loss was previously invisible to this gate. A fixture whose expected block has no issueCount (or a malformed one) is now an explicit "unclassified" state instead of silently defaulting into "clean", which would have inverted the regression rule for such a fixture. Output now prints a compared/skipped scope summary so silent truncation is visible. Also: naming/structure cleanup (issue_count_range, expected_spec, KIND_* constants, extracted _classify_kind/_validate_inputs, one row literal instead of duplicated branches), renamed truePositives*/ falsePositives* JSON fields to issuesBefore/issuesAfter with TP-proxy/FP-proxy printed labels (the script counts issues, not a per-issue correctness judgment), a docstring note reconciling this gate's scope against eval_grade.py's separate --baseline mechanism, and registered the script in docs/eval-maintenance.md. /agent-eval itself still cannot exercise Phase 0 (it passes only the fixture file, never routing through /code-review step 2b) -- documented as a known follow-up in the plan, not silently left unstated. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…l-split tag tests Slice 2's slice-boundary review checkpoint (7 agents) covering Step 2.1 -- the only standard-complexity step in this slice, never per-step reviewed -- found the reflection-into-private-members bullet's "future mechanical pre-phase" wording stale (Step 2.2 shipped in this same slice; the script already implements the check). Also fixes the new content-guard test's own quality gaps: a `block_containing` closure duplicated across two tests hoisted to a module-level helper, six tests' repeated section-fetch boilerplate replaced by a module-scoped fixture, two eager-test loops converted to parametrized cases so an early failure can't mask a later one, and two new synthetic-fixture tests exercising the tag-parsing helper's untagged/double-tagged failure branches, which no existing test had ever actually triggered. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…rt rendering Renders a compact Tier-1 line per finding (file, line, agent, severity, confidence, first sentence of message, finding-id) plus an expansion hint by default, deferring the full message/suggestedFix narrative (Tier-2) to an explicit --expand <finding-id>|all — using the same in-memory finding list a run already has, no re-dispatch, no I/O beyond the finding JSON on hand. Finding-ids are agent:file:line:severity[:category], with a :0/:1/... ordinal suffix when two findings in the same run still collide on that base id. Not yet wired into /code-review step 7 — that's Step 3.2. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…pand Wires render_tiered_findings.py (Step 3.1) into /code-review step 7's prose-mode path: the per-finding listing now renders as compact Tier-1 lines plus an expansion hint by default, with Tier-2 (full message + suggested fix) rendered for a finding-id, or every finding, via --expand <finding-id>|all. --json and corrections/*.json are untouched -- both already read/write the full finding objects independently, and --expand is a structural no-op under --json since that branch never reaches the tiered-rendering path. The old [auto-fix failed] bracket tag is dropped from the default listing (it's step 6a fix-loop runtime state, not a finding property, and stays visible in the iteration log); [confidence: none]/ [suggestion] are now carried by each Tier-1 line's own severity/confidence field instead of a separate bracket tag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

…llback chain Slice 3's slice-boundary review checkpoint (7 agents) found --expand printed only the Tier-2 block, dropping the Tier-1 report entirely -- contradicting three explicit statements in SKILL.md/output-format.md that Tier-2 is appended after Tier-1, not a replacement for it. main() now prints the Tier-1 report before Tier-2 content in both --expand branches; the CLI test that missed this only asserted Tier-2 content was present, closed with two new tests asserting both tiers render. base_id()'s taxonomy-tag resolution now mirrors finding_signature.py's full category -> smell -> rule -> ruleId fallback chain (was category-only), so a test-smell-review finding's id carries its smell tag instead of falling back to a bare ordinal. Also: null-normalization in render_tier1_line now matches base_id's (an explicit JSON null no longer renders as the literal string "None"), first_sentence collapses embedded whitespace/newlines so a Tier-1 entry is always exactly one line, the ordinal-suffix separator changed from ":N" to "#N" so it can't collide with a base-id segment, an Eager Test was split into two single-focus tests, and coverage gaps closed for an absent/empty suggestedFix, a dict-wrapped {"findings": [...]} input shape, and malformed JSON input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

/code-review --internal backstop review found and fixed: - test_review_mechanics.py: multi-line Python signature parsing dropped a test's body (false no-assertion error); JS test-call regex matched RegExp .test( member access; internal-collaborator doubles missing waiver markers. - render_tiered_findings.py: unrecognized --findings shapes (topFindings, issues) silently rendered a false clean pass instead of erroring. - checkpoint_abort.py: severity-floor duplicated finding_signature.py's is_actionable with a case-sensitivity mismatch; CLI mode scaffolding deduplicated via a shared _load_json_arg helper. - finding_signature.py: extracted finding_agent/finding_category helpers, now shared with render_tiered_findings.py instead of duplicated. - compare_eval_results.py: moved to repo-root scripts/ per ADR 0032 (monorepo-dev-only tooling, not a shipped script). - build/SKILL.md: clarified the abort-check merge must use the fix loop's post-convergence findings, not the original pre-fix triggering set. - test-review.md / code-review/SKILL.md: resolved a "do not re-report" vs "report findings" wording conflict, and aligned the Hunt-scope prose with its actual test-files-only wiring. - Added regression tests and coverage-gap fixtures the panel flagged across checkpoint_abort.py, test_review_mechanics.py, and compare_eval_results.py. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGHpm7gtqNb8NM2Smf76u7

…able-tiered # Conflicts: # .claude/metrics/config-changelog.jsonl

test-review.md's Hunt-scope prose (added by the prior backstop-review fix commit) referenced skills/code-review/SKILL.md as a literal path, which the anchor-citation guard (tests/agents/test_agent_knowledge_anchor.py) requires to carry a knowledge/index.json anchor or a "Whole-file load:" token. Rephrase as /code-review, matching this file's existing convention elsewhere for referencing the same skill. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KGHpm7gtqNb8NM2Smf76u7

claude added 14 commits September 22, 2026 17:19

chore(metrics): record plan-approval audit entry for #2168+#2169+#2170 …

8b79f9d

…batch Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

feat(scripts): add checkpoint_abort.py — cheap-lens blocker decision

27186ca

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

feat(build): abort remaining checkpoint lenses on a cheap blocker

3dfe8a5

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

docs(test-review): annotate mechanical vs judgment checks

ce29b43

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016uBnw1i52qEik2k9LSHa4n

Merge remote-tracking branch 'origin/main' into feat/2164-abort-count…

04d0953

…able-tiered # Conflicts: # .claude/metrics/config-changelog.jsonl

bdfinst changed the title ~~feat(review): hybrid review flow — abort-on-cheap-blocker, countable test-review pilot, tiered findings~~ feat(review): abort-on-cheap-blocker, countable test-review, tiered findings Sep 22, 2026

This was referenced Sep 22, 2026

feat(review): hybrid review flow — verdict ledger spine, cheap-gate phase, tiered findings #2164

Closed

fix(agent-eval): wire test-review Phase 0 into unit-tier dispatch #2198

Merged

bdfinst merged commit ee2bc42 into main Sep 23, 2026
15 checks passed

bdfinst deleted the feat/2164-abort-countable-tiered branch September 23, 2026 13:35

bdfinst-release-bot Bot mentioned this pull request Sep 23, 2026

chore: release main #2196

Merged

This was referenced Sep 23, 2026

chore(metrics): evaluate realized benefits of epics #2164 and #2172 #2200

Open

chore(metrics): measure cheap-blocker abort, countable pre-phase and tiered findings (#2164) #2203

Open

Navigation Menu

feat(review): abort-on-cheap-blocker, countable test-review, tiered findings - #2197

feat(review): abort-on-cheap-blocker, countable test-review, tiered findings#2197
bdfinst merged 15 commits into
mainfrom
feat/2164-abort-countable-tiered

bdfinst commented Sep 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

bdfinst commented Sep 22, 2026

Summary

Test Plan

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants