Reconcile later-completed audit targets

[justin808]

Summary

• allow completed-batch publication to reconcile a valid terminal abandoned or superseded lane only when the same exact target is independently authenticated as merged or closed after the lane closeout
• preserve the immutable coordination closeout, including any historical target state, and record the later-target completion mode in the publication snapshot
• keep active/nonterminal lanes, open or unauthenticated targets, malformed terminal timestamps, and normal done lanes without coordination evidence blocked
• add the helper suite to the repository validation gate and mirror the contract in the shared skill and workflow docs

Reproduction and fix

The HiChee PR #10027 lane was immutably closed abandoned, then the exact reviewed PR was merged by a maintainer. Before this change, the completed-batch preflight rejected that sequence based on the historical lane state even though its live target verifier authenticated the later merge.

The fix stays in the publication consumer. It does not rewrite coordination history or widen the coordination backend schema. The helper accepts the reconciliation only after its existing authenticated exact-target verifier confirms both the required terminal target state and a merged_at/closed_at timestamp strictly later than the lane closeout.

Validation

• ruby skills/post-merge-audit/bin/completed-batch-publication-preflight-test.rb — 46 runs, 272 assertions, including target-completed-before-closeout, stale-coordination-evidence, nonterminal-lane diagnostic, and v1 snapshot-compatibility regressions; historical normal-done snapshot digests are unchanged
• ruby skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb — 70 runs, 572 assertions; the sibling verifier fixture now exercises the authenticated completion-timestamp contract
• independent Sol/xhigh forward review — CLEAN before the final ordering hardening; exact-head reviews found and drove the timestamp-ordering fix (b8556b7), stale-evidence guard (7aa5877), sibling fixture update (86e1ad9), v1 replay-compatibility/shared-predicate fix (c0696ab), explicit reconciliation-decision/nonterminal-diagnostic fix (3b7e343), and dead-lookup cleanup (deeba8cf); final exact-head review passed with no blocking issues and no unresolved threads
• live HiChee batch hc-pr10027-20260802 replay on final head — ELIGIBLE, no blockers, authenticated merged_at 06:03:15Z after lane closed_at 03:45:49Z, original terminal abandoned preserved with authenticated_target_after_coordination_closeout; snapshot sha256:78834a09ecdec78c8d28ebdea5d29eeba3fff966fc72a024ed99cf82be28fd2c
• git diff --check
• targeted RuboCop on completed-batch-publication-preflight-test.rb — clean
• bin/validate — exact-head hosted validation passed in 11m53s (run); local macOS validation reached the known unrelated cold-exec race in test_git_probes_fail_closed_when_they_time_out, and three bounded focused retries reproduced the same missing-git.pid race. Both the failing test and implementation are byte-identical to origin/main; tracked by check-agent-workflow-drift-test: hanging-git pid precondition races, flaking bin/validate #237 and Two bin/ tests flake on macOS from the same cold-exec stub race as #222 (drift git probes, doctor secret redaction) #239.

Fixes shakacode/hichee#10154

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b44513bc-c40f-4237-83e5-f470d7f05998

📥 Commits

Reviewing files that changed from the base of the PR and between b923c53 and deeba8c.

📒 Files selected for processing (7)

• CHANGELOG.md
• bin/validate
• skills/post-merge-audit/SKILL.md
• skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb
• skills/post-merge-audit/bin/completed-batch-publication-preflight
• skills/post-merge-audit/bin/completed-batch-publication-preflight-test.rb
• workflows/post-merge-audit.md

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review

Overview

This PR relaxes completed-batch-publication-preflight so a batch coordination lane that closed as immutable terminal abandoned/superseded can still be published as eligible when the same exact target is independently re-authenticated (via live gh api) as merged/closed. The original coordination closeout is preserved verbatim in the snapshot (nothing is rewritten), and a new completion_mode: "authenticated_target_after_coordination_closeout" marker records that the reconciliation happened. Normal terminal done lanes are untouched — they still require matching pr_state and non-empty terminal evidence.

Code quality

• The fix is scoped correctly: canonical_lane_snapshots now takes the already-computed canonical_targets so it can check the authenticated state/verification-source before allowing the abandoned/superseded shortcut (skills/post-merge-audit/bin/completed-batch-publication-preflight:464-500). It does not trust the raw target_snapshots row on its own — later_target_completion requires verification_source == "authenticated gh api", which can only be set when the real target_verifier corroborates the declared state (line 570-579). Good fail-closed design.
• The companion change in canonical_target_snapshots (line 563-569) only suppresses the "coordination and target state disagree" blocker for lanes that are validly terminal abandoned/superseded; it does not suppress the "target is not <expected_state>" or "not authenticated or fresh" blockers, so a lane can't sneak through on a merely-declared (unauthenticated) or still-open target. I traced this against all the new test cases and the logic holds up.
• valid_terminal_lane? cleanly generalizes the old inline done-only terminal check to the new TERMINAL_LANE_STATES set without changing behavior for done lanes — verified by reading through the equivalent old/new branches.
• Reordering canonical_lane_snapshots after canonical_target_snapshots in assess (line 82-90) is the only call site for both functions, so no other consumer needed updating.
• The new completion_mode key is additive/optional on the lane snapshot hash; grepped for other consumers of snapshot.coordination.lanes (the receipt binary/tests) and none assert an exact key set on lane rows, so this doesn't break downstream shape checks.

Test coverage

Six new test cases cover: successful reconciliation for both abandoned and superseded, blocked-when-target-not-actually-completed, blocked-when-unauthenticated, blocked-for-nonterminal lanes, blocked-for-malformed closed_at, and confirmation that normal done lanes still require evidence. Left one inline suggestion: all new cases use a pull_request target — an issue-type case (closed instead of merged) would exercise the expected_state branch and the no-PR/head-SHA code path that PRs don't hit.

Docs

SKILL.md and workflows/post-merge-audit.md are updated to match the new contract, and the wording is accurate (verified against the implementation, not just prose).

Security

No security concerns — the reconciliation path still fails closed if the live gh api verification is unavailable/mismatched, and it can't be triggered by the caller-supplied target_snapshots/coordination_status alone since verification_source can only be stamped by the real verifier callback.

Overall this looks correct and well-tested; only a minor test-coverage suggestion above.

[claude]

Review

Overview

This PR relaxes completed-batch-publication-preflight so that a batch can still be published when a coordination lane's immutable terminal disposition was abandoned/superseded, but the exact same target was independently, later confirmed merged/closed via authenticated gh api. It preserves the original (historical) lane closeout in the snapshot and stamps a new completion_mode: "authenticated_target_after_coordination_closeout" marker, while normal done lanes keep their existing evidence requirements. It also wires the helper's own test suite into bin/validate (previously not gated in CI), and mirrors the contract into SKILL.md/workflows/post-merge-audit.md.

Correctness

Traced the new logic in canonical_lane_snapshots / canonical_target_snapshots end-to-end:

• Reconciliation (later_target_completion) only fires when the live, independently-authenticated target snapshot (verification_source == "authenticated gh api") shows the exact terminal state expected for the target type (merged for PRs, closed for issues). This can't be spoofed via the input JSON alone — target_verifier is a trusted callback backed by real gh api calls, and canonical_target_snapshots already requires verified["state"] == state before trusting the declared state.
• Nonterminal lanes, invalid closed_at timestamps, and lane/status mismatches remain blocked regardless of authenticated target completion, because valid_terminal_lane? gates a separate "coordination lane is nonterminal" blocker independent of the reconciliation branch. Verified this with the new tests (test_authenticated_target_completion_does_not_rescue_nonterminal_lane, ..._invalid_terminal_timestamp).
• The "coordination and target state disagree" suppression (historical_incomplete_closeout) only suppresses that one specific blocker for abandoned/superseded lanes — it doesn't bypass the "target is not #{expected_state}" or "target state/head is not authenticated" checks, so an open/unauthenticated target still blocks (test_abandoned_lane_stays_blocked_when_target_is_not_later_completed, ..._without_authenticated_target_completion).
• QA evidence requirements (canonical_qa_snapshots) are untouched by this change — the reconciliation path only affects the coordination-lane check, so it doesn't weaken the QA gate for reconciled targets.
• Reordering canonical_target_snapshots/canonical_lane_snapshots in assess is necessary (lane snapshots now need the computed target snapshots) and doesn't change blocker semantics since blockers are .uniq'd before use.

Test coverage

Very thorough — new tests cover: PR and issue reconciliation, preserving the historical (possibly conflicting) target_state/status, the not-yet-completed case, the unauthenticated-verifier case, invalid terminal timestamp, nonterminal lane, and a regression test that a normal done lane still requires evidence. This is exactly the right shape of test matrix for a security-sensitive gate like this.

Nits (posted inline)

• %w[abandoned superseded] is duplicated across canonical_lane_snapshots and canonical_target_snapshots. Worth hoisting into a shared constant next to TERMINAL_LANE_STATES to avoid the two call sites drifting.
• canonical_lane_snapshots has grown noticeably more complex with this change; consider extracting the reconciliation decision into a small named helper for readability. Not blocking — logic is correct and well tested.

Other

• Good catch adding completed-batch-publication-preflight-test.rb to bin/validate — it wasn't previously part of the gated suite (bin/validate lists tests explicitly, no glob), so this closes a real CI coverage gap independent of the main fix.
• Docs (SKILL.md, workflows/post-merge-audit.md) and CHANGELOG.md accurately describe the new behavior and its limits.

Security

No issues found. The reconciliation path is gated behind the same authenticated-verifier trust boundary as the rest of the helper, and doesn't widen what an attacker-controlled input alone can force through — both the historical coordination fact and the live target fact must come from trusted verifiers.

No blocking issues found — approve modulo the two style nits above (non-blocking).

[claude]

Review

Reviewed the diff (completed-batch-publication-preflight reconciliation logic, its test suite, bin/validate, and the doc updates in SKILL.md/workflows/post-merge-audit.md).

Summary: This is a well-scoped, carefully guarded change. I traced the new branch logic in canonical_lane_snapshots and canonical_target_snapshots against all 8 new test cases and the reasoning holds up:

• later_target_completion only fires when terminal_state is abandoned/superseded and the target verifier independently returns "authenticated gh api" for the exact target with the expected terminal state — a normal done lane still goes through the unchanged coordination_completion path requiring its own pr_state/evidence_url, so nothing weakens the existing evidence requirement for the happy path (covered by test_done_lane_still_requires_coordination_terminal_evidence).
• completion_mode is only stamped when terminal && later_target_completion, so a malformed/nonterminal lane (bad closed_at, mismatched status/terminal) can never get silently reconciled — it still trips the "coordination lane is nonterminal" blocker (valid_terminal_lane? gates on status == terminal_state and a valid ISO-8601 closed_at).
• The "coordination and target state disagree" suppression in canonical_target_snapshots is scoped tightly to abandoned/superseded + valid terminal lane, so it can't be used to mask a real disagreement on a done lane.
• Reordering canonical_target_snapshots before canonical_lane_snapshots doesn't introduce a circular dependency — the target-state disagreement check still reads from the raw lane_entries, not the canonical lanes, so there's no cycle.

No functional or security issues found. Two small observations, non-blocking:

1. bin/completed-batch-publication-preflight:481-485 — the blocker-suppression conditional means a lane that's abandoned/superseded and gets reconciled via live authentication will never get evidence/pr_state blockers even if those fields are simply malformed garbage rather than absent (e.g. pr_state: "definitely not a real state"). Since eligibility still requires the independent authenticated target check to agree, this can't actually flip eligibility incorrectly — just flagging that malformed legacy fields on a reconciled lane pass through silently into the snapshot rather than being blocked or normalized. Given the "preserve historical state" design goal this looks intentional.
2. Minor style nit: line 482 uses a backslash line continuation for the unless modifier, which is a bit less idiomatic than the rest of the file's postfix-conditional style — purely cosmetic, rubocop apparently accepts it per the PR's validation run.

Test coverage is thorough — the 7 new test cases in completed-batch-publication-preflight-test.rb hit the happy path (PR and issue targets), missing-later-completion, nonterminal-lane rejection, unauthenticated-target rejection, malformed-timestamp rejection, and the "done lane still needs its own evidence" regression guard. bin/validate was correctly updated to include the new test file.

I wasn't able to execute the Ruby test suite or rubocop directly in this sandboxed environment, so this review is based on static/manual tracing of the logic against the new assertions rather than a live test run.

[claude]

Review: Reconcile later-completed audit targets (#315)

Overview

This PR extends completed-batch-publication-preflight so that a lane whose coordination record is immutably closed abandoned/superseded can still be published as complete, provided the helper's authenticated, live target verifier independently confirms the exact same target later reached the required terminal state (merged for PRs, closed for issues). The original coordination closeout is preserved verbatim in the snapshot; a new completion_mode: "authenticated_target_after_coordination_closeout" marker records the reconciliation. Docs (SKILL.md, workflows/post-merge-audit.md, CHANGELOG.md) and bin/validate are updated to match, and the change ships with an extensive new test matrix (7 new test cases covering the happy path, PR/issue targets, non-reconciled-yet targets, nonterminal lanes, missing authentication, malformed timestamps, and normal done lanes).

Code quality

• The change is narrowly scoped to the publication consumer — it does not touch the coordination backend/schema, matching the stated design goal ("does not rewrite coordination history").
• TERMINAL_LANE_STATES / RECONCILABLE_TERMINAL_LANE_STATES are cleanly factored constants, and valid_terminal_lane? is a nice extraction reused in both canonical_lane_snapshots and the new historical_incomplete_closeout check in canonical_target_snapshots — good DRY-ness.
• Reordering canonical_lane_snapshots to run after canonical_target_snapshots (so it can consume the already-authenticated canonical_targets) is necessary and correctly threaded — canonical_lane_snapshots now consumes the verified snapshot rather than re-deriving authentication itself, so the reconciliation path can't be spoofed by just editing the raw target_snapshots input (the target_verifier cross-check on state/head_sha/verification_source still gates everything, per canonical_target_snapshots lines ~571-588).
• Test coverage is thorough: both PR and issue targets, both abandoned and superseded states, the not-yet-completed case, missing-authentication, nonterminal-lane, and malformed-timestamp cases are all exercised. This is the right shape of regression protection for a fail-closed, security-relevant preflight.

Minor observations (non-blocking)

• authenticated_target_completion / later_target_completion in canonical_lane_snapshots (skills/post-merge-audit/bin/completed-batch-publication-preflight:476-481) use continuation-line indentation that's a bit inconsistent with the rest of the file's style. Purely cosmetic.
• The historical_incomplete_closeout bypass of the "coordination and target state disagree" blocker (same file, ~566-569) doesn't itself check authenticated_target_completion/verification_source — it fires for any terminal abandoned/superseded lane whose declared pr_state differs from the raw declared target state, even before the live verifier has run. This is safe in practice because eligibility still requires the unconditional state == expected_state check (line 550) and the target_verifier authentication check (lines 571-580) to independently pass — so a forged/stale declared state can't achieve eligibility through this bypass alone — but it's worth confirming no other blocker relies on "state disagree" firing as a fail-safe for this specific combination, since the bypass is now unconditional on authentication status.

Security

No new attack surface: the reconciliation path is gated by the same target_verifier (authenticated gh api) seam already used elsewhere, and the coordination record itself remains immutable — only the publication snapshot changes, and only after live re-authentication of the exact target. Fail-closed defaults are preserved for nonterminal lanes, unauthenticated targets, and malformed timestamps.

Performance

Negligible — one extra hash lookup (targets_by_key) over an already-small in-memory array per batch.

Test coverage

Strong. New tests assert both success and failure paths, including that a normal done lane still requires terminal evidence (regression guard) and that authentication alone doesn't rescue a nonterminal lane or invalid timestamp.

Overall: a well-scoped, well-tested change that matches its stated design constraints. No blocking issues found.

[justin808]

Address-review summary

Scan scope: Full review history through current head bad3ad07e5a14fb71b2072ab292ba5d5bb7fea35.

Must-fix / discuss: None remaining.

Addressed:

• Added an explicit issue-target-to-closed reconciliation regression; the original thread is resolved.
• Added the shared RECONCILABLE_TERMINAL_LANE_STATES constant; both duplicate-literal threads are resolved.

Dispositioned without code changes:

• Kept the completion decision inline because its named booleans are local to one snapshot operation and extracting a wrapper would add indirection without reducing security-relevant branching.
• Current-head Claude's indentation observation is cosmetic.
• Current-head Claude's historical-state observation remains fail-closed: target terminal state and authenticated gh api verification are unconditional eligibility requirements, so bypassing the redundant state-disagreement blocker cannot make stale or forged input eligible.
• CodeRabbit's current-head rate-limit status is capacity information, not review evidence; it is not counted as approval.

Independent evidence: Current-head Claude review found no blocking issue; an independent Sol/xhigh adversarial review also returned CLEAN after the final fix.

Next default scan: Re-fetch all review threads if the head changes; otherwise no review action remains.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ffda4b68ef

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7aa5877390

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review: Reconcile later-completed audit targets

Overview

Adds a narrow reconciliation path so completed-batch-publication-preflight can accept a completed-batch publication when a lane is immutably closed abandoned/superseded but the same exact target is independently authenticated (via target_verifier) as merged/closed at a timestamp strictly after the lane's closed_at. The original coordination terminal/state is preserved in the snapshot; only a new completion_mode marker is added. Nonterminal lanes, mismatched/unauthenticated targets, and malformed timestamps stay blocked. Docs (SKILL.md, workflows/post-merge-audit.md) and bin/validate are updated to match, and the new helper test file adds 10 focused scenarios covering the happy path and each blocking condition.

Code quality

• The core logic in canonical_lane_snapshots/canonical_target_snapshots is careful about fail-closed defaults: later_target_completion/authenticated_later_completion require valid_terminal_lane? (status/terminal agreement + valid closed_at), an authenticated verification_source, and a strict timestamp_after? check. Good test coverage of the negative cases (target completed before closeout, unauthenticated target, malformed terminal timestamp, non-terminal lane with the "abandoned" label).
• Preserving the historical target_state/evidence fields on the reconciled lane snapshot (rather than overwriting them with the new authenticated facts) is a nice touch that matches the stated goal of not rewriting coordination history — confirmed by test_abandoned_lane_preserves_historical_open_state_when_target_later_authenticates_as_merged.
• Minor nit: timestamp_after? calls valid_timestamp? (which itself parses with Time.iso8601) and then re-parses both values with Time.iso8601 again — a small amount of redundant work, not a correctness issue.

Potential issue (please verify before merging)

verified_shape in canonical_target_snapshots (line ~570) now requires the target_verifier callback to return a completed_at key (verified.keys.sort == %w[completed_at head_sha state target verification_source], plus valid_timestamp?(verified["completed_at"])). This is a breaking change to that callback's contract.

skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb (not touched by this PR) has its own target_verifier stub in the publication_preflight test helper (~line 2697) that only returns target/state/head_sha/verification_source — no completed_at. That means verified_shape will now be false for every preflight built via that helper, which adds an unconditional "target state/head is not authenticated or fresh" blocker and makes eligible false everywhere it's used (e.g. test_complete_publication_reauthenticates_waiver_receipt_before_post, test_complete_publication_accepts_only_the_unchanged_authenticated_waiver, and the other callers at lines 399, 426, 462, 561). Since CompletedBatchAuditReceipt.validate_publication_preflight! raises whenever eligible is false, this looks like it should fail those tests.

That test file is wired into bin/validate (line 103), so it's worth confirming the PR's validation run actually exercised it, and updating the stub to include a valid completed_at before merging. Left as an inline comment on the diff line that introduces the new requirement, since the file that needs updating isn't part of this diff.

Security / performance

No security concerns — the change only tightens/reconciles an existing fail-closed gate and doesn't relax authentication requirements (the reconciliation path still requires authenticated verifier confirmation and strict timestamp ordering). No performance concerns; all data is bounded per-batch, in-memory.

[claude]

Review: Reconcile later-completed audit targets (#315)

Overview

This PR extends completed-batch-publication-preflight so that a completed batch can reconcile a coordination lane that is immutably terminal abandoned/superseded when the same exact target is independently authenticated (via target_verifier) as merged/closed at a timestamp strictly after the lane's closed_at. The original lane terminal state is preserved in the snapshot, and a new completion_mode: "authenticated_target_after_coordination_closeout" marker records the reconciliation. Docs (SKILL.md, workflows/post-merge-audit.md) and CHANGELOG.md are updated to match, and the helper suite is added to bin/validate.

The core logic in canonical_lane_snapshots / canonical_target_snapshots is fail-closed and well-covered: nonterminal lanes, stale pre-closeout coordination evidence, out-of-order completion, malformed timestamps, and unauthenticated target facts are all still blocked, each with a dedicated regression test. The target_verifier is now called once and its result reused by both the lane and target reconciliation paths, avoiding a TOCTOU where it could be invoked twice with different live answers.

🔴 Likely regression: stale test stub breaks completed-batch-audit-receipt-test.rb

canonical_target_snapshots in completed-batch-publication-preflight now requires target_verifier to return a hash with exactly these keys: %w[completed_at head_sha state target verification_source], with completed_at passing valid_timestamp?:

verified_shape = verified.is_a?(Hash) &&
                 verified.keys.sort == %w[completed_at head_sha state target verification_source] &&
                 ...
                 valid_timestamp?(verified["completed_at"]) &&
                 verified["verification_source"] == "authenticated gh api"

completed-batch-publication-preflight-test.rb's shared valid_target_verifier helper was correctly updated to include completed_at. However, completed-batch-audit-receipt-test.rb's shared fixture builder publication_preflight (around line 2626, target_verifier: lambda at line ~2697-2703) was not updated — it still returns only target/state/head_sha/verification_source, with no completed_at key.

Since verified.keys.sort there is a 4-element array, it can never equal the new 5-element required set, so verified_shape is deterministically false for every call built through this helper, and assess unconditionally adds "... target state/head is not authenticated or fresh". That flips eligible to false on every preflight this helper constructs — which will make CompletedBatchAuditReceipt.validate_publication_preflight! raise "completed-batch publication preflight is blocked" in tests that expect a clean publish, e.g.:

• test_complete_publication_accepts_only_the_unchanged_authenticated_waiver
• test_complete_publication_accepts_matching_real_backend_after_live_coordination_refresh
• test_complete_publication_blocks_public_claim_fallback_without_private_coordination
• test_complete_publication_accepts_matching_no_backend_without_live_coordination_call
• test_publish_binds_snapshot_and_replay_blocks_a_refreshed_snapshot_mismatch

skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb is already wired into bin/validate (line 103, unchanged by this PR), so this should surface as real failures on a clean run of bin/validate, not just the "known unrelated macOS cold-exec race in test_git_probes_fail_closed_when_they_time_out" mentioned in the PR's Validation section. Worth re-running ruby skills/post-merge-audit/bin/completed-batch-audit-receipt-test.rb in isolation to confirm before merge — if it's actually green, I'd want to understand what I'm missing about how verified_shape is being satisfied, since the key-set comparison looks unconditional.

Minor observations

• In canonical_lane_snapshots, when a reconcilable (abandoned/superseded) lane fails reconciliation, the code can emit both "target completion is not authenticated after coordination closeout" and "coordination target state is not merged" / "coordination terminal evidence is absent" for the same lane, even though those two messages are about the (now-irrelevant) stale pr_state/evidence_url fields rather than the live target. Functionally harmless since it correctly keeps eligible: false either way, but the extra blockers could read as confusing/misleading to a human debugging why publication is blocked (implying the lane thinks the PR isn't merged, when the real gate is "no fresh evidence after closeout").
• later_target_completion in canonical_lane_snapshots is computed from RECONCILABLE_TERMINAL_LANE_STATES.include?(lane["terminal"]) alone, without also requiring valid_terminal_lane?(lane) (i.e. it doesn't check lane["status"] == lane["terminal"]). This doesn't currently leak into an actual bypass — valid_terminal_lane? is checked separately and unconditionally adds "coordination lane is nonterminal" when it fails, so eligible still ends up false — but the variable is doing something subtly different from what its name/gating elsewhere implies, which could bite a future refactor. Might be worth threading terminal through explicitly for clarity.

Nice-to-haves already done well

• timestamp_after? / valid_timestamp? are fail-closed (rescue ArgumentError => false), and Time.iso8601 correctly rejects malformed timestamps.
• Digest/hash values in the test file were correctly updated (snapshot_digest values changed since the snapshot shape gained completed_at/completion_mode).
• verified_target_api_snapshot requires valid_timestamp?(payload["merged_at"]) / valid_timestamp?(payload["closed_at"]) before accepting, so a malformed GitHub API response can't produce a bogus completed_at.

Overall the design and test coverage for the new reconciliation logic itself look solid; the main actionable item is the stale test stub above.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 86e1ad9481

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

Overview

Extends completed-batch-publication-preflight so a completed batch can reconcile a lane whose coordination closeout is immutably abandoned or superseded when — and only when — the helper's own authenticated live-target verifier independently confirms the same exact target reached the expected terminal state (merged/closed) at a timestamp strictly later than the lane's closed_at. The original coordination terminal is preserved in the snapshot and a completion_mode: "authenticated_target_after_coordination_closeout" marker is recorded. Normal done lanes are unaffected and still require matching pr_state + evidence. Docs (SKILL.md, workflows/post-merge-audit.md, CHANGELOG.md) and bin/validate are updated to match.

Code quality

• The change is well-scoped to the publication consumer, doesn't touch the coordination schema, and reuses the existing authenticated-verifier pattern rather than trusting any caller-supplied fact — good adherence to the "fail closed" posture used throughout this file.
• timestamp_after? and valid_terminal_lane? are clean, small, well-named helpers.
• Left an inline comment: the "authenticated later completion" predicate is implemented twice (canonical_lane_snapshots and canonical_target_snapshots) with slightly different guards (one checks valid_terminal_lane?, the other doesn't). It doesn't currently cause an incorrect ELIGIBLE verdict — the nonterminal-lane blocker still fires either way — but the duplication is a drift risk worth consolidating into one shared predicate.
• Reordering canonical_lane_snapshots to run after canonical_target_snapshots (so it can see the authenticated target snapshot) is a reasonable, minimal change; the resulting snapshot_digest fixture updates in the test file are expected fallout from that reordering plus the new completed_at field, not signs of unrelated behavior change.

Correctness

• Traced through the blocker logic for done lanes and confirmed it's unchanged: coordination_completion still requires terminal_state == "done" && pr_state == expected_state && evidence, so pre-existing behavior for the common case is preserved (backed by test_done_lane_still_requires_coordination_terminal_evidence).
• For abandoned/superseded lanes, reconciliation is gated on target_snapshot["verification_source"] == "authenticated gh api", which is only set when the target_verifier's returned shape (including state) matches — so an attacker/caller can't fake reconciliation purely via unauthenticated target_snapshots input; the live gh api response (verified_target_api_snapshot) is the actual source of truth for completed_at/state.
• timestamp_after? correctly requires strictly later (not >=), matching the stated "strictly later than lane closeout" contract.
• Test coverage is thorough: reconciled PR and issue targets, preserved historical open state, pre-closeout completion, missing verifier data, nonterminal lane, malformed closed_at, and reuse-of-stale-coordination-evidence are all covered as negative/positive cases.

Security

• No new trust boundary is introduced; the reconciliation path still requires the same authenticated gh api call as the pre-existing "target state/head is not authenticated or fresh" check, and it can't rewrite or launder the original coordination closeout (it's preserved verbatim in the snapshot, only an additive completion_mode field is added).

Performance

• No material impact — the additional target_snapshots.to_h lookup is O(n) over an already-small per-batch target list, and no extra gh api/agent-coord calls are introduced (still exactly one target_verifier call per target, as before).

Overall this looks solid and consistent with the stated goal (unblocking the HiChee #10154 case without weakening the fail-closed guarantees elsewhere). The one actionable suggestion is consolidating the duplicated reconciliation predicate flagged inline.

[claude]

Review: Reconcile later-completed audit targets (#315)

Overview

Extends completed-batch-publication-preflight so a completed batch can reconcile an immutable terminal abandoned/superseded coordination lane when the same exact target is independently re-authenticated (via the live target_verifier, backed by gh api) as merged/closed at a timestamp strictly later than the lane's closed_at. The original coordination closeout is preserved verbatim in the snapshot; only a new completion_mode: "authenticated_target_after_coordination_closeout" marker and a completed_at field are added when reconciliation applies. Normal done lanes are untouched — they still require pr_state/evidence_url. Docs (SKILL.md, workflows/post-merge-audit.md, CHANGELOG.md) and bin/validate are updated to match, and the helper's test file adds solid coverage for the happy path (PR + issue targets) and the negative paths (not later, before closeout, unauthenticated, nonterminal lane, malformed timestamp, normal done still gated on evidence).

Strengths

• Authentication chain is preserved. The reconciliation timestamp comes from the live target_verifier (pinned to html_url/number via verified_target_api_snapshot) and the lane's closed_at comes from the freshly authenticated coordination_verifier result — neither is attacker-controllable input, so this doesn't open a spoofing path.
• Strict, not >=, timestamp comparison (timestamp_after?) — avoids same-instant ambiguity.
• verified_target_api_snapshot now also validates payload["closed_at"] for the issue-closed path (previously unchecked), a nice incidental correctness fix beyond just plumbing for the new feature.
• Schema stays additive within VERSION = 1 — old receipts without completed_at/completion_mode remain valid under valid_receipt?, and a test explicitly locks the golden digest for the plain terminal-done path to prove the digest didn't drift.
• Test coverage is thorough for a change this size (11 new/updated test cases across the happy path, both target types, and every failure mode called out in the PR description).

Issues raised inline

1. Fragile implicit coupling between the two call sites of authenticated_later_target_completion? (canonical_target_snapshots and canonical_lane_snapshots) — the second call reconstitutes the check from an already-canonicalized hash rather than reusing the boolean computed the first time, relying on an unstated invariant about which keys canonical_target_snapshots populates. See inline comment.
2. Minor misleading blocker text for lanes that are simply nonterminal but happen to have terminal: "abandoned"/"superseded" set (status doesn't match) — both "coordination lane is nonterminal" and "target completion is not authenticated after coordination closeout" fire together, even though no terminal-lane reconciliation was actually attempted. See inline comment.

Minor/non-blocking observations (not filed inline)

• For a reconcilable lane that fails reconciliation with pr_state/evidence_url absent (the common abandoned-lane shape), up to three blockers fire at once. Functionally correct, just noisy — could be worth collapsing into one message for lanes in RECONCILABLE_TERMINAL_LANE_STATES once evidence/pr_state are known to be intentionally absent.
• Negative-path tests are only exercised against abandoned; since superseded shares the exact same code path via RECONCILABLE_TERMINAL_LANE_STATES, this is low-risk, but a symmetric negative test for superseded would close the gap for free.

No security or performance concerns beyond the above — the added hash lookups are O(1)/O(n) and don't change the complexity class of the existing preflight pass.

[claude]

Review: Reconcile later-completed audit targets (PR #315)

Overview
Extends completed-batch-publication-preflight so a completed-batch-audit publication can proceed when a coordination lane is immutably closed abandoned/superseded, but the same exact target is independently authenticated (via the existing target_verifier, "authenticated gh api" source) as merged/closed at a timestamp strictly after the lane's closed_at. The original lane fields (status, terminal, closed_at, target_state, evidence) are preserved verbatim in the snapshot; a new completion_mode: "authenticated_target_after_coordination_closeout" marker is added instead of rewriting history. Normal done lanes are unaffected — they still require matching pr_state + evidence_url.

Strengths

• Fail-closed design is consistent: reconciliation only fires when the lane is a strictly-formed terminal (valid_terminal_lane?), the terminal state is in the reconcilable allowlist (abandoned/superseded, deliberately excluding done), the target-verifier shape is fully authenticated, and the completion timestamp is strictly later than closeout (timestamp_after?, using Time.iso8601 with a rescued ArgumentError).
• Reuses the pre-existing "authenticated gh api" trust boundary (target_verifier) rather than introducing a new one — no new source of truth, no coordination-schema changes, matching the PR's stated intent.
• v1 snapshot/digest compatibility is explicitly protected: completed_at is only added to a target snapshot when the later-completion path is actually taken, and a test asserts no completed_at key appears for the historical non-reconciled path (unchanged digest).
• Good negative-test coverage: nonterminal lane not rescued, malformed closed_at not rescued, target completed before closeout not rescued, stale/absent target verification not rescued, and pre-closeout pr_state/evidence_url cannot be reused to satisfy the new path.
• Docs (SKILL.md, workflows/post-merge-audit.md, CHANGELOG.md) and the validation gate (bin/validate) were all updated to match.

Issue found

• Dead code: in canonical_lane_snapshots (skills/post-merge-audit/bin/completed-batch-publication-preflight), the new target_snapshots parameter is only used to build targets_by_key, which is only used to assign a per-lane local target_snapshot that is never read afterward. The reconciliation decision is driven entirely by the authenticated_later_target_keys map (already computed in canonical_target_snapshots). Recommend deleting the unused parameter, the targets_by_key hash build, and the target_snapshot = ... assignment. Left inline comments with details (including a correction after my first suggestion range was too broad).

Other observations (non-blocking)

• The blocker message about unauthenticated target completion can co-occur with the coordination-target-state and terminal-evidence blockers for the same lane when none of the reconciliation conditions hold — verbose but not incorrect; tests assert via assert_includes rather than exact-list equality, so this is fine.
• No security concerns beyond the dead code: the new path strictly narrows (adds a stricter conjunctive gate) rather than widens what already-authenticated data can unlock, and does not trust any less-verified field than the pre-existing code paths did.

Validation note: did not re-run the Ruby test suites in this environment (sandbox blocked the bare ruby invocation); reviewed statically against the PR's reported test run description (46/270 and 70/572 assertions).

[claude]

Review: Reconcile later-completed audit targets

Overview

This PR lets completed-batch-publication-preflight reconcile a lane whose coordination status is an immutable terminal abandoned/superseded when the same exact target is independently and authenticably confirmed (merged/closed) with a completion timestamp strictly after the lane's closed_at. It preserves the historical lane terminal/state instead of rewriting it, and threads a new completed_at field through the live target_verifier contract, coordination snapshot, and publication snapshot (completion_mode: authenticated_target_after_coordination_closeout). Docs (SKILL.md, workflows/post-merge-audit.md, CHANGELOG.md) and bin/validate are updated in lockstep.

Code quality

• The refactor to return [canonical_targets, authenticated_later_target_keys] from canonical_target_snapshots and feed that into canonical_lane_snapshots is a clean way to let the "was this target authenticated as completed later" fact flow from the target layer to the lane layer without re-deriving it twice.
• valid_terminal_lane? and timestamp_after? are properly extracted as shared helpers rather than duplicated logic — good DRY given both canonical_lane_snapshots and authenticated_later_target_completion? need the same terminal-lane validity check.
• Generalizing TERMINAL_LANE_STATES to %w[done abandoned superseded] (previously only done counted as terminal) matches how abandoned is already a recognized terminal disposition at the agent-coord layer (e.g. --terminal abandoned in skills/pr-batch/bin/single_target_entrypoint_test.rb), so this isn't introducing a new vocabulary, just catching this consumer up to it.
• The strict shape check on target_verifier's return value (verified.keys.sort == %w[completed_at head_sha state target verification_source]) correctly forces the new completed_at field to flow through the authenticated verifier contract rather than being trusted from unauthenticated input — authenticated_later_target_completion? only ever sees a completed_at/verification_source that passed that shape check, so there's no way to spoof "later completion" via the raw target_snapshots input alone. Good.

Potential issues / nits

• Minor UX-only nit in canonical_lane_snapshots (line ~478-486): when an abandoned/superseded lane fails to reconcile, both "target completion is not authenticated after coordination closeout" and the generic "coordination target state is not X" / "coordination terminal evidence is absent" blockers can fire together for the same lane (see e.g. test_abandoned_lane_stays_blocked_when_target_is_not_later_completed, which asserts both messages present). Not a bug — eligibility is correctly false either way — but slightly redundant blocker output for a human reading the receipt. Could consider suppressing the generic messages once the reconciliation-specific one fires, if terser output is desired.
• bin/validate previously never ran completed-batch-publication-preflight-test.rb at all — this PR adds it to the gate. Worth double-checking (outside this PR) how long that test suite was excluded from CI-equivalent validation, since it means prior changes to this file could have silently broken tests without failing bin/validate.

Security

• Reconciliation is gated behind the existing authenticated gh api verifier contract (verification_source == "authenticated gh api"), requires exact target match, exact expected state (merged/closed), and a strictly-later authenticated timestamp vs. the immutable lane closed_at. This closes the original bug (a legitimately-merged PR being blocked by a stale abandoned lane) without weakening the fail-closed posture — nonterminal lanes, unauthenticated target facts, and malformed timestamps all remain blocked, and coordination history is never rewritten (only the publication snapshot annotates completion_mode).
• verified_target_api_snapshot now also requires valid_timestamp?(payload["closed_at"]) for issues (previously unchecked), which is a small hardening improvement (rejects malformed/missing closed_at) even independent of the main feature.

Test coverage

• Test coverage is thorough: reconciliation success for both PR and issue targets, target not-later-completed, target completed-before-closeout, reuse-of-stale-evidence rejection, historical-state preservation on reconcile, nonterminal-lane non-rescue, missing-verifier-completion rejection, malformed-timestamp rejection, and a v1 snapshot-digest regression test confirming normal done lanes are byte-identical (no completed_at key leaks into unrelated snapshots).
• Digest determinism is preserved via the existing canonicalize/key-sort approach even though completed_at is now conditionally present on target/lane snapshots.

Overall this is a well-scoped, well-tested fix that stays narrowly focused on the consumer side (as the PR description states) without touching the coordination backend schema. No blocking issues found.

[justin808]

Address-review summary

Scan scope: full history via check all reviews, followed by a final exact-head inventory at deeba8cf9e175a9bb5fb90dd85ab1b834f4c4346.

Mattered

• Fixed the implicit reconciliation-decision coupling by passing the authenticated per-target decision explicitly (3b7e343).
• Fixed the misleading reconciliation blocker on nonterminal lanes and added regression coverage (3b7e343).
• Removed the obsolete target-snapshot lookup identified by the exact-head review, using the reviewer’s corrected narrow scope (deeba8cf).
• Final exact-head Claude review reported no blocking issues; all review threads are resolved.

Optional

• Declined further blocker-message deduplication as non-blocking UX polish; the current messages are correct and covered, and another code cycle would add churn without changing eligibility.

Skipped

• The reviewer’s initial broad deletion suggestion was superseded by its correction; only the verified narrow dead-code removal was applied.
• No separate historical audit of how long the preflight suite was absent from bin/validate; this PR closes the forward validation gap by adding the suite to the gate.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

[justin808]

---

head_sha: deeba8c
triggered_gates:

• commit-count-limit
  rollback_disposition: Code-only rollback is established by reverting PR Reconcile later-completed audit targets #315 if needed.
  decision: approve
  approved_by: justin808
  source: direct-user-task
  evidence: Codex task /root direct user approval on 2026-08-04 Asia/Tokyo.
  ...