fix(002): parse findings at emit time — review counts can no longer be blind - #19
Merged
Conversation
…hat the reviewer found Live incident, four arcs: a codex review reporting three real findings (two high, incl. an audit race emitting stale prior values) emitted findings:[] with p1/p2 counts 0, stop_rule_met true, and a notice recommending the author declare convergence. Root cause: the reviewer became pluggable and codex kept its NATIVE line shape, while the round-tracking counter still recognises only the legacy [P1] contract used by the constructed gemini/claude prompts — so every codex review counts zero and the stop-rule fires on round one. A signal that cannot fail is worse than none: silence invites a look, 'converged' does not.
…ored single-source parser + fixture verifier
…een, then migration/CI/docs
The artifact now carries parsed_findings (machine-parsed from the reviewer's native line shape) alongside an untouched findings:[] — so a reader asking 'what did the reviewer find' has an answer, and an empty findings[] can never be mistaken for a clean review. Priority counts are max(native parse, legacy grep), so neither supported shape can zero the other and no single format change can blind the signal again. Two blindnesses fixed, not one: the native (codex) shape was never parsed, AND the legacy counter never matched '- [P1]' bullet-prefixed lines — which is the exact format that made project-arc's own rounds report zero. Replaying tonight's artifacts through the fix: reviews that reported 'stop-rule met, declare convergence' actually carried up to 4 P1 + 14 P2. Verified by scripts/verify-parser.py, which extracts the parser from the hook between unique anchors (single-sourced — no second copy to drift) and runs four recorded fixtures: the live incident (3 findings), the legacy shape, an honest NO FINDINGS, and prose-with-pipes as the false-positive guard. Wired into CI. Schema 3→4 with a migration that keeps older artifacts readable; hook marker 4→5 so installed copies upgrade via /review-setup.
…e first real run
The end-to-end run of this very change parsed three findings out of its
own diff: a reviewer quoting a hunk of a file that contains
finding-shaped lines ('+path | logic | high | ...') manufactured
findings. The file component must look like a path — a markdown list
marker is allowed, a diff marker never is. Regression fixture
tests/fixtures/diff-quoted.txt covers additions, removals, and a
line-less form.
…er counted The reviewer's output was captured correctly all along; only the parsing was blind. So the upgrade path re-READS what is already on disk rather than stubbing an empty array: migrate-reviews.py locates the installed hook (honouring core.hooksPath, falling back to the cwd's repo), extracts the anchored parser from it — single-sourced, so backfill can never drift from the emitter — parses each artifact's raw_review, and recomputes the counts. A round with findings has stop_rule_met forced false, retracting every 'declare convergence' the blind counter emitted. Every other field is preserved byte-for-byte (raw_review, findings[], resolutions, ci_status). Idempotent: a second run reports 0 migrated. --dry-run shows what would be recovered before writing. Verified on copies of live artifacts from two repos: 3 findings recovered from one, a false stop_rule_met corrected on the other. No re-review needed anywhere — re-running codex would cost hours and be LESS accurate, since several affected branches have since merged.
…edger The *.json glob swept the round-tracking ledger and reshaped it into a review artifact, destroying its rounds[] history — done to a live ledger before the guard existed. Skip underscore-prefixed files by name AND require raw_review by shape, so any future internal state file is safe by default. Verified: a ledger-shaped file passes through untouched while the artifact beside it still migrates.
…oke assertions
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
1 participant
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.
The bug, found in production use
Wingman wrote
findings: []on reviews where the reviewer had actually reported findings in prose. The parse happened somewhere downstream of the artifact, so the artifact recorded zero and every consumer believed it.It was caught live during a PP arc session: a session reported "latest backend review: 0 findings" from the artifact, and the migration backfill then recovered 16 findings across 5 branches that had been reported and never counted. In one repo the visible finding count went from a handful to 56.
That is the worst failure mode for a review tool — not missing a problem, but reporting confidence it hasn't earned.
What this does
parsed_findingsfield (schema v4), so the artifact reflects what the reviewer actually said at the moment it said it.scripts/migrate-reviews.py) recovers findings from v1 artifacts, idempotently._convergence.jsonis a ledger, never an artifact to migrate.Lifecycle
Full Spec-Kit pack: spec → plan → tasks → implementation, tests-first (fixtures and verifier red before the parser landed). 36 files, +4,866.
Known issue worth a follow-up
Running the migration against an already-v4 tree,
--dry-runreports files as "would migrate" while the real run correctly reports them "unchanged" — observed on a 38-file tree. Dry-run and apply disagree, which undermines exactly the confidence dry-run exists to provide. Not fixed here; raising it rather than letting it sit unnoticed.