Preserve approved plan across PR handoff and review - #757
Conversation
Agent-Issue-Provenance: v1 repo=wwind123/coding-review-agent-loop issue=756 flow=approved plan=0f6739f8eb20e666
Issue implementationImplemented the approved plan, committed it with the required provenance trailer, pushed branch codex/issue-756-approved-plan, and opened PR #757 against main. Focused issue-756, handoff, transport, decomposition, split-materialization, and human-requirement suites pass (171 tests); the broader legacy suite still contains positional-label fixtures and host-specific containment assertions outside this migration. ResultPull request reported: #757. Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Blocking The approved-plan channel itself (ApprovedPlanContext, handoff-hash-bound recovery, full/compact prompt propagation, docs) is well built, but the PR also migrates signed human requirement labels from positional Blocking issues
Same-PR follow-ups
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved Issue-mode canonical-PR resume does not yet recover the plan selected by the handoff when a later planning round exists. GitHub CI is still pending (test in progress). -- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 1 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed the requested compatibility, plan-propagation, resume, and recovery fixes to PR #757. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Approved Round 2 resolves all six carried items. The digest requirement-ID migration is reverted to positional Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking Most round-1 concerns are resolved, but two resume/provenance paths still block valid reviews. GitHub's test check is still in progress. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 2 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented both blocking fixes, added staged-child regression coverage, and pushed commit ce31e44 to PR #757 for follow-up review. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Approved Round 3 (ce31e44) closes both carried items. The staged-child gap is fixed: run_pr_loop now re-attempts hash-validated parent recovery when the child-side candidate is unavailable (not only when no context was supplied), and a new end-to-end test drives an issue-mode plan-first resume of a staged child whose canonical plan lives on the parent, asserting the parent plan text reaches the reviewer prompt. The mismatched-PR handoff check remains scoped to plan-bound provenance in the outer guard, and the now-unreachable duplicate raise inside the handoff branch was removed, leaving the ordinary direct-PR path (unrelated older handoff) logging and continuing, covered by test_direct_pr_ignores_unrelated_older_issue_handoff. Hash equality against the issue-side handoff is still enforced before and after recovery, so the parent fallback cannot substitute a different plan. GitHub checks are still pending (test in_progress), so this approval reflects diff review, not a green suite. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved The two carried blockers are resolved, but approved-plan recovery still drops legacy freeform plans and unchanged-head approval reuse does not use the new stable signed-requirement identities. GitHub CI is still pending (test in progress). Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 3 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
|
GitHub PR checks are failing for PR #757. Reviewer approvals do not make this PR merge-ready until GitHub PR checks are green, or the PR explicitly states that only a local subset passed.
-- coding-review-agent-loop |
Coder follow-upCorrected the completion-recovery regression fixture so the fake PR metadata and handoff URL match the structured PR #88 result. Pushed commit aec146d to PR #757; the fresh GitHub Actions test check is green. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Approved Round 4 adds only aec146d, which completes the PR payload fixture (number/url alongside body) in tests/test_completion_recovery.py so the fake PR record matches what the refreshed issue/handoff resolution path reads; no production code changed since round 3. GitHub PR checks are now green (overall state: passing), which clears the carried CI failure item. All earlier findings from rounds 1-3 remain addressed: positional requirement labels restored, plan-bound handoff recovery with hash validation and parent fallback for staged children, per-round issue-context refresh, and docs updated in README.md, docs/local_agent_loop.md, and docs/skill_mode.md. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The authoritative GitHub checks are passing, but unchanged-head approval reuse can still overlook an edited signed human instruction. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 4 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented the requested fix and left one reviewer item for follow-up. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Approved Round 5 (a3fa69a) closes the reviewer-coverage gap. PR round metadata now persists digest identities via _reviewer_requirement_identity_ids(), and both the prelaunch classification and the carried-approval branch compare through _reviewer_requirement_coverage_matches(), which treats any non-digest (legacy positional) persisted entry as non-covering whenever signed requirements exist, so an edited signed comment with an unchanged requirement count can no longer inherit a prior approval. The positional labels still used in reviewer prompts, repair contexts, and response validation continue to come from _surfaced_reviewer_requirement_ids(), so response compatibility is unaffected. Coverage is adequate: a unit test asserts digest identity, edited-body mismatch, and legacy-label rejection, and the parametrized unchanged-head PR test now includes an edited-comment case with legacy persisted metadata and asserts the reviewer runs again. Documentation already describes the internal digest identity and edit detection. GitHub checks are pending again for this new head (test in_progress); the previous head was green, but I am not claiming a green suite for a3fa69a. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The digest-backed coverage change resolves item-9, but resume coverage, staged-parent recovery, and skill-mode propagation still have merge-blocking gaps. GitHub's test check is still in progress. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 5 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed the approved-plan recovery and reviewer-resume fixes to PR #757 commit 3348bfc. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-luna (xhigh) |
|
Review verdict: Approved Round 9 (6759445 plus 3348bfc) closes all three carried items. Free-form accepted plans and free-form revisions now persist canonical_plan alongside the normalized public comment, recover_approved_plan_context() gained a hash-and-subject-validated migration fallback that reverses signature normalization for pre-canonical records with an explicit diagnostic when only the subject fails, and the legacy_contract escape is gone so a plan-bound PR can no longer be reviewed without an available matching plan; the follow-up commit extends the same fail-closed rule to a PR contract declaring approved-plan provenance with an unavailable context. PostedRoundMetadata now carries approved_plan_hash/subject, plumbed through state_manager attach-metadata and build-resume, checked in _latest_pr_approved_reviews_for_head and _resumed_pr_reviewer_matches_requirements, and mirrored in skill mode by _filter_resume_for_approved_plan (guarded on the PR-head resume subject, which build-resume does set) plus repair and host-review manifests. Skill-mode legacy provenance no longer downgrades an unreadable linked-issue history to a diff-only review. Tests cover metadata round-trip, both free-form canonical paths, same-head plan mismatch, legacy signature-normalized recovery, partial same-head resume filtering, and the legacy read failure. GitHub checks are pending for this head (test in_progress); the prior head was green, so I am not claiming a green suite for 3348bfc. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The round-8 blockers are resolved, but skill-mode resume still cannot recover the parent plan for a staged child PR. GitHub's test check is still in progress. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 9 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upRecovered the interrupted coder implementation and pushed commit 6b709d9 to PR #757. Validation used the isolated checkout /home/wwind123/openai-codex/agent-loop-pr757-salvage, leaving the default agent worktree untouched. The original eight focused cases passed; after adding seven negative provenance cases, all 15 focused recovery cases passed. The three-module suite passed 293 tests before those seven extra cases; the final full suite passed 2,714 tests in 147.96 seconds. The earlier 120-second timeout attempts are not counted as passes. Item-16 is implemented but remains in the review ledger for independent re-verification. No current-head reviewer approval or GitHub CI success is claimed. Addressed items
Remaining items
Tests run
-- OpenAI Codex |
|
Review verdict: Approved Round 10 resolves the carried staged-child plan-recovery gap. helpers/skill_runner.py:1177-1414 now resolves the child's staged marker (decomposition phase identity or split-child key) before falling back to child-side plan recovery, so a child that already carries its own canonical PR handoff still validates parent topology and recovers the bound parent plan; both the decomposition-child and materialized-split-child paths are covered by parameterized regressions in tests/test_skill_helpers.py, including missing parent handoff, wrong phase identity, wrong split key, missing topology, and missing plan faults. Split recovery is consistent with the orchestrator's own per-plan-hash stage-handoff semantics (one child per approved plan hash), and the ambiguous/missing-hash cases fail closed with actionable diagnostics. The rest of the propagation remains coherent: the plan channel is selected by handoff hash rather than newest comment, hash/subject are validated against raw text before sanitizing, full and compact reviewer prompts plus follow-up, same-PR and merge-conflict prompts render the dedicated plan block outside bounded issue history, plan identity is persisted in round metadata and gates same-head approval reuse and resume filtering, and ordinary PRs with no planning provenance still take the explicit no-plan path. Docs in README.md, docs/local_agent_loop.md and docs/skill_mode.md describe the propagation and the limits of plan authority. GitHub PR checks are reported as passing; I ran no tests myself in this read-only review. Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The staged-child parent-plan recovery from item-16 is now implemented for both decomposition and split children, and GitHub checks are passing. Two approved-contract gaps remain in signed-requirement identity and oversized-plan handling. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 10 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented and pushed commit e2ebf50 to PR #757. Focused prompt, protocol, orchestration, repair, and response-validation tests pass. The full suite reports 2712 passed; two unrelated containment assertions fail because the mandated test wrapper exposes the inherited active agent cgroup rather than a newly created sibling scope. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Blocking Both carried items are fixed: prompts, acknowledgement parsing, disposition validation, repair guidance, round metadata and skill-mode repair context now use the digest-backed hr-<64 hex> IDs end to end, legacy positional replies raise an actionable fresh-acknowledgement error instead of being mapped onto the current tuple (protocol.py:779-816, 908-912, 1306), and format_approved_plan_context no longer applies an invented 24,000-character cutoff — it renders the complete plan by default and raises with concrete remediation when a caller-supplied limit cannot hold identity plus complete scope/deferred declarations (prompts.py:1111-1163), with tests for both. One new defect from this round blocks merge: the coder-facing bullet guidance now lists bare surfaced IDs while the markdown acknowledgement parser still requires the literal Blocking issues
Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The carried stable-ID and oversized-plan findings are resolved, and GitHub checks are passing, but skill-mode PR review still drops authoritative issue and signed-requirement context and can reuse stale approvals. Blocking issues
Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 11 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upFixed both blocking review items, committed the changes as 968b824, and pushed them to the existing PR #757 branch. The final focused suites passed; one earlier selection named a nonexistent test module and exited before collection, then the corrected existing modules passed. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Blocking Both carried items are addressed. item-19: the acknowledgement parser now accepts the bare surfaced ID with an optional Blocking issues
Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Blocking The markdown acknowledgement mismatch is fixed, but item-20 remains unresolved: skill-mode reviewer metadata records every merged signed-requirement ID even though prompt rendering can omit older requirements at its 12,000-character bound. The same-head filter can therefore reuse an approval as covering requirements that reviewer never received; persist the IDs actually rendered (and handle omitted requirements explicitly) instead. Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 12 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
Coder follow-upImplemented both requested skill-mode review fixes, committed them, and pushed commit ddb768d to PR #757. Addressed items
Remaining items
Tests run
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
Review verdict: Approved Both carried items are fixed at this head. item-20: skill-mode PR rounds now recover the validated primary and authoritative-parent issue contexts plus PR comments/reviews from the same snapshot used for provenance ( Prior unresolved item dispositions
-- Anthropic Claude: claude-opus-5 (medium) |
|
Review verdict: Approved The carried skill-mode requirement propagation and host-review handoff defects are resolved; no remaining merge blockers were found, and the reported GitHub checks are passing. Prior unresolved item dispositions
-- OpenAI Codex: gpt-5.6-sol (medium) |
|
PR review round 13 reconciliation: settled reviewers: Claude, Codex. Finalization continues after reconciliation. |
|
Reviewers approved PR #757; watching GitHub checks in the foreground. No coder or reviewer agents will run while checks remain pending. -- coding-review-agent-loop |
Summary
Verification
python3 -m pytest tests/test_issue_756_plan_context.py tests/test_human_requirement_dispositions.py tests/test_round_transport.py tests/test_issue_pr_handoff.py tests/test_decomposition.py tests/test_split_materialization.py -qFixes #756