Reviewer final reports must pass schema verification before output #391

New Issue

2026-07-07T08:57:21-05:00

jcwalker3 commented

2026-07-07 08:57:21 -05:00

The canonical review/merge workflow defines a detailed final report and controller handoff schema, but LLM sessions still produce incomplete or contradictory free-form summaries.

Problem:
Recent reviewer summaries have:

omitted required controller fields
claimed validation passed while tests were skipped
confused PENDING review state with APPROVED
included approve/merge replay commands in blocked handoffs
said no mutations while creating files, touching worktrees, or updating refs
reused stale "ready to merge" state across MCP failure/restart

Required behavior:
A reviewer final report must be machine-validated before the session completes.

Acceptance criteria:

Add a verifier such as gitea_validate_review_final_report.
The verifier checks all required canonical fields are present.
It rejects legacy fields such as:
- Workspace mutations
- Scratch worktree used
- Pinned reviewed head
- vague Mutations: none
It requires precise mutation categories:
- File edits by reviewer
- Worktree/index mutations
- Git ref mutations
- MCP/Gitea mutations
- Review mutations
- Merge mutations
- Cleanup mutations
- External-state mutations
- Read-only diagnostics
It rejects "reviewed head SHA" unless validation and diff review passed.
It rejects "merged" without merge result and target ancestry proof.
It rejects "issue closed" without live linked-issue verification after merge.
It rejects "full suite passed" when tests were ignored unless the report explicitly says what was ignored.
It rejects blocked handoffs that include direct approve or merge replay commands.
It rejects final reports where narrative and controller handoff disagree.
It distinguishes:
- official review submitted
- finding ready but not submitted
- dry-run only
- mutation blocked
Tests cover all rejection cases above.

Non-goals:

Do not replace the canonical workflow.
Do not allow free-form summaries to bypass the verifier.

The canonical review/merge workflow defines a detailed final report and controller handoff schema, but LLM sessions still produce incomplete or contradictory free-form summaries. Problem: Recent reviewer summaries have: - omitted required controller fields - claimed validation passed while tests were skipped - confused PENDING review state with APPROVED - included approve/merge replay commands in blocked handoffs - said no mutations while creating files, touching worktrees, or updating refs - reused stale "ready to merge" state across MCP failure/restart Required behavior: A reviewer final report must be machine-validated before the session completes. Acceptance criteria: 1. Add a verifier such as `gitea_validate_review_final_report`. 2. The verifier checks all required canonical fields are present. 3. It rejects legacy fields such as: - `Workspace mutations` - `Scratch worktree used` - `Pinned reviewed head` - vague `Mutations: none` 4. It requires precise mutation categories: - File edits by reviewer - Worktree/index mutations - Git ref mutations - MCP/Gitea mutations - Review mutations - Merge mutations - Cleanup mutations - External-state mutations - Read-only diagnostics 5. It rejects "reviewed head SHA" unless validation and diff review passed. 6. It rejects "merged" without merge result and target ancestry proof. 7. It rejects "issue closed" without live linked-issue verification after merge. 8. It rejects "full suite passed" when tests were ignored unless the report explicitly says what was ignored. 9. It rejects blocked handoffs that include direct approve or merge replay commands. 10. It rejects final reports where narrative and controller handoff disagree. 11. It distinguishes: - official review submitted - finding ready but not submitted - dry-run only - mutation blocked 12. Tests cover all rejection cases above. Non-goals: - Do not replace the canonical workflow. - Do not allow free-form summaries to bypass the verifier.

jcwalker3 referenced this issue from a commit

2026-07-07 09:04:10 -05:00

feat: add reviewer final-report schema MCP validator (Closes #391)

jcwalker3 referenced a pull request that will close this issue

2026-07-07 09:04:36 -05:00

feat: add reviewer final-report schema MCP validator (Closes #391) #394

jcwalker3 commented

2026-07-07 09:13:18 -05:00

Evidence update — PR #393 reviewer report shows final-report verifier gaps.

A reviewer report for PR #393 included several claims that a schema verifier should reject or downgrade:

Official validation appears contaminated by local merge integration.
The reviewer created a PR review worktree, then ran:
git -C branches/review-feat-issue-390-pr-queue-cleanup-mode merge master -m "Merge master"

The canonical workflow says official validation must run against the unmodified PR head. After a local merge into the review worktree, the worktree no longer represents the pinned PR head unless treated as a diagnostic merge simulation with full proof and cleanup/abort/reset reporting.
Reviewed head claim is unsupported after local merge.
The final report lists candidate/reviewed head as a2bd23da144a7186d214aa148071953ad4cea926, but validation was reported after merging master into the review worktree. The verifier should reject reviewed head SHA when validation ran on a locally modified/merged worktree rather than the exact PR head.
Inventory pagination proof was overclaimed.
The report says inventory was complete because 15 PRs were returned and page size was 50. The canonical workflow forbids inferring final-page proof from result count alone unless the tool provides final-page/no-next-page/metadata proof. This should be classified as INVENTORY_PAGINATION_UNPROVEN unless the tool response proves final page.
Baseline comparison was under-specified.
PR run reported 1464 passed, 1 failed, 6 skipped; baseline reported 1264 passed, 1 failed, 6 skipped. Different test counts require explanation before claiming the single failure is pre-existing/same-as-master.
Mutation ledger missed local merge simulation.
The report listed worktree additions/removals but did not clearly list the local git merge master -m "Merge master" as a worktree/index mutation or diagnostic merge simulation.

Expected verifier behavior:

reject official validation marked passed when validation was run after local merge contamination
reject reviewed head SHA unless the validation worktree HEAD exactly matches candidate head
reject inventory-complete claims without final-page proof
reject same-as-master/pre-existing failure claims when baseline and PR test counts differ without explanation
require local merge simulation to be recorded under worktree/index mutations with clean-before/after proof

Evidence update — PR #393 reviewer report shows final-report verifier gaps. A reviewer report for PR #393 included several claims that a schema verifier should reject or downgrade: 1. **Official validation appears contaminated by local merge integration.** The reviewer created a PR review worktree, then ran: `git -C branches/review-feat-issue-390-pr-queue-cleanup-mode merge master -m "Merge master"` The canonical workflow says official validation must run against the unmodified PR head. After a local merge into the review worktree, the worktree no longer represents the pinned PR head unless treated as a diagnostic merge simulation with full proof and cleanup/abort/reset reporting. 2. **Reviewed head claim is unsupported after local merge.** The final report lists candidate/reviewed head as `a2bd23da144a7186d214aa148071953ad4cea926`, but validation was reported after merging master into the review worktree. The verifier should reject `reviewed head SHA` when validation ran on a locally modified/merged worktree rather than the exact PR head. 3. **Inventory pagination proof was overclaimed.** The report says inventory was complete because 15 PRs were returned and page size was 50. The canonical workflow forbids inferring final-page proof from result count alone unless the tool provides final-page/no-next-page/metadata proof. This should be classified as `INVENTORY_PAGINATION_UNPROVEN` unless the tool response proves final page. 4. **Baseline comparison was under-specified.** PR run reported `1464 passed, 1 failed, 6 skipped`; baseline reported `1264 passed, 1 failed, 6 skipped`. Different test counts require explanation before claiming the single failure is pre-existing/same-as-master. 5. **Mutation ledger missed local merge simulation.** The report listed worktree additions/removals but did not clearly list the local `git merge master -m "Merge master"` as a worktree/index mutation or diagnostic merge simulation. Expected verifier behavior: - reject official validation marked `passed` when validation was run after local merge contamination - reject `reviewed head SHA` unless the validation worktree HEAD exactly matches candidate head - reject inventory-complete claims without final-page proof - reject same-as-master/pre-existing failure claims when baseline and PR test counts differ without explanation - require local merge simulation to be recorded under worktree/index mutations with clean-before/after proof

jcwalker3 commented

2026-07-07 09:20:42 -05:00

Evidence update — verifier should accept correct reviewer collision stops while requiring explicit stale-state fields.

A reviewer run for PR #380 correctly stopped when another sysadmin / prgs-reviewer session approved/merged the same PR mid-run. The final state changed after selection/validation: PR #380 became closed/approved, the target branch advanced from 103364a to b5ee656, and the original run’s pending approval submit failed closed instead of replaying stale ready state.

This is good stop behavior, but the final-report verifier should require explicit fields for this class of collision:

concurrent reviewer detection: yes/no, evidence source, actor if available
selected PR stale before mutation: yes/no, old state, new state, timestamp
target branch stale: yes/no, old target SHA, new target SHA
approval/merge ready state reusable: no after any external mutation
terminal disposition: stopped due external reviewer mutation, not approved/merged by this session

Expected verifier behavior:

accept a blocked/stopped handoff when the workflow fails closed after external approval/merge
reject reports that keep a reusable “ready to approve/merge” state after a concurrent reviewer mutation
reject reports that omit stale target branch or stale selected-PR fields when the transcript shows an external queue mutation

Evidence update — verifier should accept correct reviewer collision stops while requiring explicit stale-state fields. A reviewer run for PR #380 correctly stopped when another `sysadmin / prgs-reviewer` session approved/merged the same PR mid-run. The final state changed after selection/validation: PR #380 became closed/approved, the target branch advanced from `103364a` to `b5ee656`, and the original run’s pending approval submit failed closed instead of replaying stale ready state. This is good stop behavior, but the final-report verifier should require explicit fields for this class of collision: - concurrent reviewer detection: yes/no, evidence source, actor if available - selected PR stale before mutation: yes/no, old state, new state, timestamp - target branch stale: yes/no, old target SHA, new target SHA - approval/merge ready state reusable: no after any external mutation - terminal disposition: stopped due external reviewer mutation, not approved/merged by this session Expected verifier behavior: - accept a blocked/stopped handoff when the workflow fails closed after external approval/merge - reject reports that keep a reusable “ready to approve/merge” state after a concurrent reviewer mutation - reject reports that omit stale target branch or stale selected-PR fields when the transcript shows an external queue mutation

jcwalker3 commented

2026-07-07 09:22:21 -05:00

Evidence update — PR #372 reviewer report repeated canonical final-report violations.

Observed in the PR #372 reviewer run:

The main checkout was updated before review with git pull prgs master.
The reviewer report says master was merged into the review worktree before validation:
git -C branches/review-feat-issue-300-linked-issue-live-proof merge master -m "Merge master"
Official validation was reported as passed even though validation appears to have run after local integration, not on the unmodified PR head.
The report lists reviewed head SHA as the original PR head, but the tested worktree had been locally merged with master.
Inventory completeness was inferred from 12 < 50 instead of explicit final-page/no-next-page pagination proof.
The local merge integration was not listed in Worktree/index mutations.
The files-reviewed list disagreed between the narrative and controller handoff.

Expected verifier behavior:

reject official validation after local merge contamination
reject reviewed-head claims when validation HEAD differs from candidate head
reject inventory-complete claims without explicit pagination/final-page proof
require local merge simulation/integration to be listed as a worktree/index mutation
reject narrative/handoff contradictions such as mismatched files-reviewed lists

This is another example showing that final reports need machine validation before output, not free-form self-certification.

Evidence update — PR #372 reviewer report repeated canonical final-report violations. Observed in the PR #372 reviewer run: 1. The main checkout was updated before review with `git pull prgs master`. 2. The reviewer report says master was merged into the review worktree before validation: `git -C branches/review-feat-issue-300-linked-issue-live-proof merge master -m "Merge master"` 3. Official validation was reported as passed even though validation appears to have run after local integration, not on the unmodified PR head. 4. The report lists reviewed head SHA as the original PR head, but the tested worktree had been locally merged with master. 5. Inventory completeness was inferred from `12 < 50` instead of explicit final-page/no-next-page pagination proof. 6. The local merge integration was not listed in Worktree/index mutations. 7. The files-reviewed list disagreed between the narrative and controller handoff. Expected verifier behavior: - reject official validation after local merge contamination - reject reviewed-head claims when validation HEAD differs from candidate head - reject inventory-complete claims without explicit pagination/final-page proof - require local merge simulation/integration to be listed as a worktree/index mutation - reject narrative/handoff contradictions such as mismatched files-reviewed lists This is another example showing that final reports need machine validation before output, not free-form self-certification.

jcwalker3 commented

2026-07-07 09:38:40 -05:00

Evidence update — PR #374 reviewer report omitted earlier session activity and should be caught by final-report verification.

The PR #374 reviewer run eventually produced a structured final report and merged PR #374, but the full action log shows earlier activity that was not represented cleanly in the final report.

Observed gaps:

The session inspected local Gitea/config/auth-related files and code before the final review flow, including gitea-mcp.json, gitea_config.py, gitea_auth.py, MCP tool schemas, and local helper scripts. The final report did not clearly classify this as local config/profile inspection or explain why it was allowed during review mode.
The session ran broad test commands in the main project checkout before the isolated PR review worktree was created, including full/short-traceback pytest runs. The final report focused on the isolated review worktree and did not ledger the earlier main-checkout validation attempts.
The final report says “strict compliance,” but the log includes earlier exploratory environment/config work and main-checkout test runs that should either be prohibited or explicitly classified.
The session used gitea_activate_profile during reviewer flow. If profile activation is allowed, it needs exact capability/policy proof and should be explicitly classified as a session/runtime mutation. If not allowed during canonical review, it should be blocked.
The report claims all validations ran in an isolated review worktree, but earlier validation commands ran outside that worktree before PR #374 validation began.
The final report should distinguish:
- pre-review diagnostics
- official PR-head validation
- main-checkout commands
- local config/profile inspection
- review mutations
- merge mutations
- cleanup mutations

Expected verifier behavior:

reject final reports that omit earlier commands from the same session
reject “all validation ran in isolated worktree” if any validation/test command ran in the main checkout during the run
require local config/profile inspection to be classified and justified
require profile activation to be classified as a runtime/session mutation with exact capability/policy proof
reject “strict compliance” safety statements when unclassified pre-review commands occurred
require final report narrative and mutation ledger to cover the full run, not only the successful final segment

Evidence update — PR #374 reviewer report omitted earlier session activity and should be caught by final-report verification. The PR #374 reviewer run eventually produced a structured final report and merged PR #374, but the full action log shows earlier activity that was not represented cleanly in the final report. Observed gaps: 1. The session inspected local Gitea/config/auth-related files and code before the final review flow, including `gitea-mcp.json`, `gitea_config.py`, `gitea_auth.py`, MCP tool schemas, and local helper scripts. The final report did not clearly classify this as local config/profile inspection or explain why it was allowed during review mode. 2. The session ran broad test commands in the main project checkout before the isolated PR review worktree was created, including full/short-traceback pytest runs. The final report focused on the isolated review worktree and did not ledger the earlier main-checkout validation attempts. 3. The final report says “strict compliance,” but the log includes earlier exploratory environment/config work and main-checkout test runs that should either be prohibited or explicitly classified. 4. The session used `gitea_activate_profile` during reviewer flow. If profile activation is allowed, it needs exact capability/policy proof and should be explicitly classified as a session/runtime mutation. If not allowed during canonical review, it should be blocked. 5. The report claims all validations ran in an isolated review worktree, but earlier validation commands ran outside that worktree before PR #374 validation began. 6. The final report should distinguish: - pre-review diagnostics - official PR-head validation - main-checkout commands - local config/profile inspection - review mutations - merge mutations - cleanup mutations Expected verifier behavior: - reject final reports that omit earlier commands from the same session - reject “all validation ran in isolated worktree” if any validation/test command ran in the main checkout during the run - require local config/profile inspection to be classified and justified - require profile activation to be classified as a runtime/session mutation with exact capability/policy proof - reject “strict compliance” safety statements when unclassified pre-review commands occurred - require final report narrative and mutation ledger to cover the full run, not only the successful final segment

jcwalker3 referenced this issue

2026-07-07 09:41:10 -05:00

Workflow-load gate must establish reviewer session mode boundaries and classify pre-review commands #403

jcwalker3 referenced this issue

2026-07-07 10:11:07 -05:00

Add precise validation-status vocabulary for raw PR-head failures resolved by merge simulation #406

sysadmin referenced this issue

2026-07-07 10:15:32 -05:00

feat: add reviewer final-report schema MCP validator (Closes #391) #394

sysadmin closed this issue

2026-07-07 10:15:42 -05:00

sysadmin referenced this issue from a commit