Skip to content

Preserve reviewer and coder explanations across review rounds - #759

Open
wwind123 wants to merge 2 commits into
mainfrom
codex/carried-review-rationale
Open

Preserve reviewer and coder explanations across review rounds#759
wwind123 wants to merge 2 commits into
mainfrom
codex/carried-review-rationale

Conversation

@wwind123

@wwind123 wwind123 commented Sep 10, 2026

Copy link
Copy Markdown
Owner

Why

PR #757's item-12 loop exposed feedback-delivery gaps in both directions. A reviewer kept an item blocking while putting the remaining implementation defects in its summary, without a disposition note. The coder's next prompt repeated the original item but omitted that latest summary. Conversely, the coder's per-item fix explanations were posted to GitHub but omitted from the next reviewer prompt.

These omissions encouraged partial fixes and repeated arguments. This PR preserves the explanations without deciding the technical merits of item-12 or modifying PR #757. It stays separate from #758's GitHub CI-feedback changes.

Changes

  • Require an item-specific note in new PR reviews that keep a carried item blocking or same-pr. Reject missing notes and bare status restatements, with guidance to supply the remaining defect, evidence, and required change/test.
  • Include the latest available reviewer summaries in CLI coder follow-ups as separately attributed, round/head-labeled context. Cover serial, parallel, compact, and resumed reviews, plus the same-PR-only follow-up path.
  • Include the latest structured coder summary, reported tests with caveats, addressed/remaining item notes, and dispute evidence in CLI reviewer prompts. Cover serial/parallel, full/compact, and PR-mode resume using saved structured responses.
  • Bind coder context to its recorded PR head and label it with the coder and review round. Omit old-head explanations with a notice; do not invent missing or malformed legacy resolution details. Compact mode places the changing coder block in the volatile tail.
  • Keep original claims and item IDs unchanged. Coder explanations are claims for reviewers to verify, not automatic item resolutions or proof of successful tests. Reviewer summaries do not become new findings or another reviewer's item-specific evidence.
  • Keep historical review parsing permissive. When an older review kept an item active without a note, explicitly identify that missing explanation in the ledger instead of inventing one.
  • Update reviewer and repair instructions. Repair may reuse clearly attributable original explanations, but must preserve the summary and may not invent evidence or change a verdict to bypass validation.

This reuses existing response fields and saved round metadata; no protocol version, migration, or model call is added. The prompt plumbing changes cover the CLI orchestrator; separate skill-runner context plumbing is not expanded here. Structural validation does not guarantee the semantic quality of an explanation. A new review with genuinely missing rationale can still fail validation and require a reviewer rerun.

Validation

  • Focused context and prompt tests: 205 passed.
  • python3 -m pytest tests/ -q --tb=short --maxfail=1: 2,716 passed in 344.33 seconds, at commit 8b2bf91. No early stop occurred.
  • git diff --check: passed.

An earlier suite run failed one new test because its direct resume fixture used a dictionary instead of the parsed comment object. Two new sanitizer assertions also initially expected preservation within a marker-bearing line, contrary to the existing sanitizer policy. These test fixtures/assertions were corrected; they are included in the final rerun, not treated as pre-existing failures.

A separate timeout 900 python3 -m pytest tests/ -q --tb=short invocation ran in the restricted filesystem sandbox and reported failures including Read-only file system while creating default subprocess logs under ~/.cache/coding-review-agent-loop. That run was interrupted to collect diagnostics and is not counted as a passing suite. The completed ordinary pytest invocation above passed all tests at the same committed head.

Regression coverage includes carried blockers/same-PR items, explanation publication/reconciliation, latest coder notes in both reviewer modes and launch modes, metadata-backed resume, stale heads, missing legacy details, initial implementation test caveats, and historical marker neutralization.

Separate Follow-Ups

-- OpenAI Codex

@wwind123 wwind123 changed the title Preserve carried-review explanations in coder follow-ups Preserve reviewer and coder explanations across review rounds Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant