Evidence-backed review findings #486

Problem Statement

Every factory pull request now merges itself, so the code review inside the session, plus CI, is the only check before the Base branch. That review asks two same-model sub-agents for their Standards and Spec findings in prose. The author fixes the findings it agrees with and lists the rest as Unaddressed findings, "so a human can decide on it". No human does. Since 25 September, authors have declined about 1,150 findings, about 7 per pull request, and nobody has read them.

The record shows what that costs:

• Two of the six timing bugs that reached main, Repair loop keeps re-checking the same commit after a CI-fix Repair makes no change #102 and A Pickup run takes a Spec whose Tickets are all closed on every pass, and never reaches the next Ready issue #303, were caught by the review. The author declined both as needing a decision from the Spec or the Day shift, and both PRs merged themselves, so nobody made that decision.
• A finding says the code is wrong without anything having been run. A decline says it isn't wrong with nothing to back it.
• Nothing shows which files a reviewer read. In the research, reviewers left changed files unread in 67.9% of review runs, and mostly didn't say so.
• On Codex, the reviewers' reports go to files in /tmp, and the Session log holds none of them. Nothing can check later what the review said about anything Codex wrote.

Solution

The review shows its work, and every decline cites its evidence:

• Correctness findings carry evidence. A finding that says behaviour is wrong comes with a test or a command, written out by the reviewer. The author runs it as written before deciding. If it fails as the finding said, the author fixes the code and keeps the test. If it passes, the author may decline, citing the run.
• Spec findings quote the requirement, as they do today.
• Every decline cites what refutes the finding: a line of code, a plan decision, an ADR, or the run. Standards findings keep citing the standard they break or naming the smell, since a smell can't fail a test.
• A call that isn't the agent's becomes an issue. When the author declines a finding because a Spec or the Day shift has to decide, it files a needs-triage issue, unless an open issue already covers it, and links it from the Unaddressed findings.
• Reviewers list the files they read. A changed file a reviewer didn't read goes back to that reviewer once. Any changed file still unread is named in the review's summary and in the pull request body.
• thirdshift keeps each reviewer's report beside the session's Session log, the same way on every Harness.

This applies to every session that runs the code review: the implement session, fresh or a Continuation; the Spec review; and the Repair that reviews Foreign commits in a Merge run.

User Stories

1. As the Day shift, I want every correctness finding to come with a test or command, so that whether it holds is decided by running code, not by the author's view of its own work.
2. As the Day shift, I want a finding whose test fails to be fixed, with the test kept, so that the bug stays fixed after a later change.
3. As the Day shift, I want every decline to cite the line, plan decision, ADR or run that refutes the finding, so that a decline can be checked without me.
4. As the Day shift, I want a finding declined because the call isn't the agent's to become a needs-triage issue, so that the decision reaches someone who can make it before the bug ships.
5. As the Day shift, I want that issue skipped when an open issue already covers the finding, so that my triage queue doesn't fill with duplicates.
6. As the Day shift, I want that issue linked from the pull request's Unaddressed findings, so that I can trace a decision back to the change that raised it.
7. As the Day shift, I want those issues to pause nothing, so that Weeding and Pickup runs carry on while they wait for triage.
8. As the Day shift, I want each reviewer to list the files it read, so that a review that skipped part of the change can't pass as complete.
9. As the Day shift, I want a changed file a reviewer skipped to go back to that reviewer once, so that most gaps close without a loop.
10. As the Day shift, I want any changed file still unread named in the pull request body, so that a gap stays visible after the Self-merge.
11. As the Day shift, I want each reviewer's report kept beside its Session log, so that I can read later what the review found and what it read.
12. As the Day shift, I want the reports kept the same way on every Harness, so that a Codex or OpenCode session leaves as much behind as a Claude one.
13. As the Day shift, I want Standards findings to keep citing the standard or naming the smell, so that a smell isn't forced into a test it can't fail.
14. As the Day shift, I want Spec findings to keep quoting the requirement, so that a Spec finding still points at what the Spec asked for.
15. As an implement session's agent, I want the Session prompt to say how to run a finding's evidence and what follows from the result, so that I decide every finding the same way.
16. As an implement session's agent, I want to run a reviewer's test as written, so that the run checks the reviewer's claim and not my reading of it.
17. As an implement session's agent, I want the Session prompt to say where the reviewers' reports go, so that thirdshift can keep them whatever Harness I run on.
18. As a reviewer sub-agent, I want a brief that asks for evidence only for correctness findings, so that I don't invent tests for naming or smells.
19. As a reviewer sub-agent, I want to be told to list the files I read, so that my coverage is part of my report.
20. As a reviewer sub-agent sent back to files I skipped, I want only those files, so that the second pass is short.
21. As the Spec review, I want the same rules over the whole Spec branch, so that what I find and decline is held to the same standard as a Ticket's review.
22. As the Repair that reviews Foreign commits, I want the same rules for the commits I review, so that Foreign commits get an evidence-backed review before the Self-merge.
23. As the Day shift, I want a session that leaves no reports to say so in a progress line rather than fail its Run, so that a missing report never costs a pull request.
24. As whoever runs a later replay or trial, I want each session's reports kept with its Session log, so that there is a baseline to compare a second reviewer against.
25. As whoever runs a later replay or trial, I want each report to carry its files-read list, so that coverage can be compared as well as findings.
26. As the Day shift, I want the Unaddressed finding entry in CONTEXT.md to say what a decline carries, so that the glossary matches what the factory does.
27. As the Day shift, I want the prompts page to show the new wording, so that I can read exactly what each session is told.
28. As the Day shift, I want a passing run to count as evidence only when it exercises that particular finding, so that a green test suite can't wave a finding through.
29. As the Day shift, I want this to change nothing about what counts as a Failed run, the Repair loop or the Self-merge, so that delivery behaves as before apart from the review.
30. As the Day shift, I want the review's word limits to leave room for the evidence, so that evidence isn't cut to fit the old limit.

Implementation Decisions

• The thirdshift-code-review Factory skill changes:
  • Each sub-agent's brief asks for a test or command, written out in full, for every finding that says behaviour is wrong, whichever axis raises it. Spec findings keep quoting the requirement, and Standards findings keep citing the standard or naming the smell.
  • Each brief asks the sub-agent to list the files it read. The skill compares each list with the change's files and sends any changed file a sub-agent didn't read back to it once. On a Harness where a finished sub-agent can't be asked again, a fresh one reads just those files.
  • The aggregated report names any changed file still unread.
  • Each axis's report, with its files-read list, is written to the place the Session prompt names, if it names one.
  • The briefs' word limits grow enough to hold the evidence.
• The Session prompts change: the implement session (fresh and Continuation), the Spec review, and the Repair that reviews Foreign commits. Each prompt now tells the agent to:
  • run each correctness finding's test or command as written before deciding it. A failure obliges the fix, and the test is kept. A pass allows a decline that cites the run.
  • give each entry in the pull request's Unaddressed findings the line, plan decision, ADR or run that refutes it, in place of "at least a one-line reason".
  • file a needs-triage issue for a finding declined because a Spec or the Day shift has to decide, unless an open issue already covers it, and link that issue from the entry. The issue says what the finding is, what evidence it has, which pull request raised it, and why the call isn't the agent's. It carries no other label, so it pauses nothing: only an Architect idea pauses Weeding.
  • name in the pull request body any changed file the review left unread.
  • write the reviewers' reports to the place thirdshift names.
• The prompts page is regenerated with its update switch, as AGENTS.md describes, so it shows the new wording.
• The Sessions module keeps each reviewer's report beside the session's Session log:
  • For each session that runs the review, it names a place for the reports that git ignores, and that place goes into the session's prompt.
  • When the session ends, its Resume included, it keeps whatever reports are there beside the Session log, sharing the log's name with the axis added in place of its extension.
  • It works the same for every Harness, since the place is named in the prompt rather than left to the Harness.
  • A session that leaves no reports gets a progress line, never a failure.
  • thirdshift keeps the reports. It doesn't read them or decide anything from them.
• CONTEXT.md changes:
  • The Unaddressed finding entry says each entry carries what refutes the finding, or the issue filed for a decision the agent may not make.
  • The Session log entry says a session that ran the review has its reviewers' reports kept beside it.
• Nothing else changes: what counts as a Failed run, the Repair loop, the Self-merge, the Claim, and one Harness per Command.

Testing Decisions

• What a good test checks here: what the factory sends and what it keeps, not how it builds either.
• The Session prompts, through the prompts page. The test that regenerates the prompts page shows each prompt's new wording for the implement session, the Spec review and the Repair that reviews Foreign commits. That test is the prior art, and the prompts page diff is the review surface.
• The Sessions module, through the seam its execution tests already use, with the session faked. The existing session execution tests are the prior art. New cases:
  • a session that leaves both reports in the named place has them kept beside its Session log, under the agreed names;
  • a session that leaves none gets a progress line, and its Run carries on;
  • a Resume's reports are the ones kept when the Resume ran the review;
  • the behaviour is the same whichever Harness the faked session stands for.
• The skill's text has no automated test. The Spec review reads it, and the replays in Multi-family review: have another model family review a Run's work #429 measure the review later.

Out of Scope

• A Second opinion, a second model reviewing the change. That waits on the replays in Multi-family review: have another model family review a Run's work #429.
• A timing checklist in the review. The six-bug replay in Multi-family review: have another model family review a Run's work #429 decides it.
• Holding the Self-merge for an unaddressed finding, or letting the author depart from a Spec to fix a bug it found.
• Checking an Architect plan before it is marked ready.
• A Harness, Model or Effort per kind of session.
• Evidence for Standards findings beyond citing the standard or naming the smell.
• thirdshift reading the reports or deciding anything from them.

Further Notes

• This comes from the grilling on Multi-family review: have another model family review a Run's work #429, multi-family review, which keeps that issue open for the replays. The research behind it is in docs/research/multi-family-review.md.
• The research's caution on declines: a passing generated test is weak evidence that a finding is wrong. A decline should cite a test that exercises that particular finding, not a green suite. Unchecked evidence raised false rejections as fast as real catches, which is why the evidence has to be run.
• About 12 of the 1,152 declined findings so far left the call to a human in so many words. That's roughly one needs-triage issue per 17 merged pull requests, a floor since the wording varies.
• The Multi-family review: have another model family review a Run's work #429 replays compare against the review as it was before this Spec lands, using a pinned copy of the skill.
• The change reaches the factory with the next release, since the Factory skills are built into the binary (ADR 0001).
• Tickets: at least two.
  • The review's evidence rules: the skill, the prompts, the prompts page and the Unaddressed finding entry.
  • thirdshift keeping the reports: the Sessions module and the Session log entry, blocked by the first.
  • More Tickets if that keeps each implement session's context small.