Skip to content

fix(ce-code-review): bind finish workers to the reviewed checkout - #1845

Closed
arimetabot wants to merge 1 commit into
EveryInc:mainfrom
arimetabot:codex/review-checkout-binding
Closed

arimetabot wants to merge 1 commit into
EveryInc:mainfrom
arimetabot:codex/review-checkout-binding

Conversation

@arimetabot

Copy link
Copy Markdown

Summary

Fresh review workers now receive the checkout that produced the diff instead of relying on their inherited working directory. This prevents a same-branch, same-HEAD checkout with different staged changes from being treated as the reviewed tree.

The binding is captured during scope collection and checked by finish leaves and validators. The index is fingerprinted separately because an unstaged edit can cancel a staged change in the working-tree diff. Remote reviews retain their existing reviewed-ref rules; local apply authority is unchanged.

Fixes #1844.

Validation

  • Focused regression suite: 139 passed. Includes same-HEAD clones with different staged contents and an index change hidden by restored working-tree contents.
  • Skill guards: 798 passed, 9 skipped. Typecheck and release metadata validation passed.
  • Full suite on Bun 1.4.2 with the catalog's historical refs fetched: 4,518 passed, 10 skipped, zero failures (175 files). The first runs exposed Bun 1.3.14 runner-format failures, a timeout, and missing historical refs; the final run passed without changes to those tests or the global runtime.
  • A fresh Codex source-read canary selected the correct synthetic checkout after verifying root, HEAD, index, and tracked diff. This did not exercise a complete merge/validator/report round, remote scope, or an apply batch. An independent Codex review found the late-capture ordering issue; scope collection now loads the binding contract before producing the diff, with an early-load regression guard.
  • A second fresh Codex canary followed the updated Stage 1 procedure and captured the original root, HEAD, index and tracked-diff hashes before review. The fixture's HEAD, hashes and porcelain were unchanged afterward. This bounded canary omitted full-round stage logging.
  • Claude behavioral evaluation was not run: this contribution's execution policy permits Codex-native review only. Plugin schema validation is left to upstream CI, as permitted by CONTRIBUTING.md.

No installed plugin or host configuration was changed. No claim is made about time or token savings.

Security Disclosure

This changes checkout-path and state-validation instructions. Missing or mismatched local bindings stop finish or leave validation unresolved. The checks assume a trusted, cooperative checkout; they are not a sandbox and do not prevent concurrent changes between verification and use. No dependencies, credentials, permissions, or external-provider routes changed.

Agent Disclosure

  • Model: Codex desktop; GPT-5 family label exposed in context. Exact served model is unverified.

@tmchow

tmchow commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this, and for filing #1844 first with a clear repro. I'm going to close it though.

I'm not convinced the failure happens in normal use yet. I've asked for the real-world case on #1844. If it turns out to be real, the fix we'd merge is just project_root in the handoff, with workers using it as their working directory.

The index and worktree SHA-256 fingerprints and the recheck steps are more than we'd take even then. Once workers run in the right root, what the fingerprints add is detection of concurrent drift, which the PR itself says it doesn't protect against. They'd also fail the whole finish after all the reviewers have already run, which is a rough trade when someone keeps editing during a long review. And hashing raw stdout is left for the model to do in prose, so the dispatcher and a leaf can easily compute it differently and fail on a false mismatch.

If #1844 shows a real harness doing this, happy to take a PR for the project_root-only version.

@tmchow tmchow closed this Oct 11, 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.

ce-code-review finish handoff omits the reviewed checkout root

3 participants