Claim command dispatches before execution#380
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed June 29, 2026, 4:17 PM ET / 20:17 UTC. Summary Reproducibility: yes. source inspection gives a high-confidence current-main path: dispatch-bearing commands execute before the ledger is appended and written, so a failure after dispatch but before the final write can replay the same command version. I did not run a destructive live dispatch. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this only with maintainer-approved at-most-once semantics, or add stale-claim recovery so duplicate dispatches are prevented without silently dropping commands after a pre-dispatch failure. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence current-main path: dispatch-bearing commands execute before the ledger is appended and written, so a failure after dispatch but before the final write can replay the same command version. I did not run a destructive live dispatch. Is this the best way to solve the issue? Unclear. The implementation is narrow and tested, but treating AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against ae63b16d6c74. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e0325d6ce
ℹ️ 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".
| report.ledger_claimed = measure("claim_dispatch_commands", () => | ||
| claimDispatchCommands(actionable), | ||
| ); | ||
| if (report.ledger_claimed) writeLedger(ledgerPath(), ledger); |
There was a problem hiding this comment.
Avoid suppressing commands on failed dispatch attempts
This writes a terminal claimed ledger entry before any executeCommand dispatch has completed. In repair-comment-router.yml the ledger publish step runs with if: always(), and claimed entries are included in shouldSuppressProcessedCommentVersion, so if a dispatch command throws after this write (for example gh workflow run/repository_dispatch fails or the process crashes before executeCommand reaches the dispatch), the failed claim is published and future router runs skip the same comment version instead of retrying it. That can permanently drop re-review/repair/assist requests without ever starting the worker.
Useful? React with 👍 / 👎.
Summary
claimedcommand-ledger state for dispatch-bearing comment-router commandsclaimedentries toexecutedorwaitingRoot cause
The comment router previously executed ready commands before appending them to
results/comment-router.json. A command that emitted arepository_dispatchor workflow dispatch could therefore be replayed if the process failed before the final ledger write.Why it matters
The local ledger is the durable idempotency boundary for maintainer commands, while GitHub dispatch APIs own the external side effect. Claiming dispatch-bearing command versions before emitting external work keeps retry runs from launching duplicate review/repair workers after a later failure.
Validation
pnpm run build:repair/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin/node --test test/repair/comment-router-utils.test.ts test/repair/comment-router-core.test.tsPATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run test:repairPATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run lint:repairPATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run lint:scriptsPATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run format:checkPATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run check:active-surfacePATH=/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/node/bin:/Users/alexgild/.cache/codex-runtimes/codex-primary-runtime/dependencies/bin:$PATH pnpm run check:limitsMaintainer note
This makes dispatch-bearing commands at-most-once across crashes by treating the pre-dispatch claim as terminal for replay suppression. If automatic recovery for the small pre-dispatch crash window is important, that can be added separately as an explicit stale-claim recovery path rather than replaying commands by default.