Harden pre-merge evidence sessions - #1557
Conversation
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Fresh-context adversarial review completed at 8f0a70c39310c87b12db062d13f664f88ce65fd8. I found no confirmed CRITICAL or HIGH correctness, security, or data-loss defect. The checkout-derived single-use state binds the exact PR/head/base across separate processes, and CLEAN now requires opening-base blob equality for the CI caller, selected reusable Gitleaks workflow, .gitleaks.toml, and .gitleaksignore. The direct canaries cover missing/consumed/substituted/cross-checkout state and independent same-name no-op replacements. Hosted CI remains a separate merge gate; this review does not recommend or perform a merge.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f0a70c393
ℹ️ 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".
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f2cc25367
ℹ️ 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".
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Focused fresh-context re-review completed for the review-fix delta at 2f2cc253675606f409cf7b8ab066e1cb29bd9bbf. Both confirmed HIGH defects are closed: diff inspection no longer relies on process-local pr_number, and an expired session can restart only after an explicit abort validates the checkout-bound state. Missing, ambiguous, substituted, and cross-checkout states still fail closed; finish still re-proves exact identities, feedback, checks, scan provenance, and definition equality before CLEAN. No new CRITICAL/HIGH defect was found. LOW only: abort does not fully revalidate every opening-snapshot field before discarding a path-matching local state; that cannot escape the evidence directory or emit false-clean evidence and does not reopen the bounded pipeline. Hosted exact-head CI remains separate; no merge is performed.
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e87a7472a
ℹ️ 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".
| --json number,headRefName,headRefOid,baseRefName,baseRefOid,mergeable,updatedAt,url \ | ||
| >"$temp_dir/closing.json" || die "cannot resolve the closing PR identity" | ||
| validate_pr_snapshot "$temp_dir/closing.json" | ||
| "$jq_executable" '.opening' "$state_file" >"$temp_dir/opening.json" |
There was a problem hiding this comment.
Fresh evidence after the earlier state-binding fix: finish authenticates the file only when entering load_opening_state, then rereads .opening here without authenticating that exact snapshot. If PR-controlled checks leave a background process running, it can wait until collection begins and rewrite the state with a newly pushed head and timestamp; the closing comparison then uses the modified values, while assert_clean_exact_checkout still uses the old opening_head local, allowing a COMPLETE packet for head B even though local checks ran at head A. Parse one immutable snapshot, authenticate that snapshot, and use it exclusively through comparison, emission, and consumption.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH at final head. The token-bound digest is checked only on entry; later reads of the mutable state file can diverge from the authenticated locals. A PR-controlled background process can therefore create a head/timestamp split and let the packet describe a different PR head from the locally tested checkout. This arrived after the permitted focused reopen, so the two-round ceiling binds: #1557 is parked, not owner-ready, and this blocker remains intentionally unresolved.
| worktree_status="$("$git_executable" status --porcelain=v1 --untracked-files=all)" || | ||
| die "cannot inspect worktree status" | ||
| [[ -z "$worktree_status" ]] || die "exact-head evidence requires a clean worktree" |
There was a problem hiding this comment.
When any tracked file is marked assume-unchanged or skip-worktree, git status --porcelain can remain empty even after that file's worktree bytes diverge from HEAD. A contributor with a locally hidden config file—or an untrusted check that sets the flag before modifying a later check's inputs—can therefore pass both calls to this function while the reported local tests did not run against the asserted exact head. Reject these index flags or compare the actual tracked worktree contents to the index before accepting the checkout as clean.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH despite the P2 badge. assume-unchanged/skip-worktree can hide tracked-byte divergence from both clean-worktree checks, giving a direct path to report local exact-head tests that actually ran against modified bytes. This is a false-evidence path in the changed gate. The two-round ceiling now requires parking rather than another fix cascade, so this blocker remains intentionally unresolved.
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Focused final-head adversarial review of 5ebf330: no remaining CRITICAL/HIGH finding. The full canonical opening record is authenticated by an operator-carried random token whose value is not persisted; state substitution cannot forge the token-bound digest, and finish validates it before consuming any opening identity. Token-authenticated abort remains fail-closed and restartable. The final operational instructions also keep the token visible across genuinely separate tool shells and forbid process-local/environment/checkout persistence; the 19th static canary directly enforces that continuity. Exact-head hosted CI remains the final owner-ready gate.
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Correction to my earlier final-head review after the late connector threads were fetched: #1557 is PARKED, not owner-ready. Two confirmed HIGH false-evidence paths remain at this exact head: finish authenticates mutable state only once and later rereads it, permitting a background identity rewrite during collection; and hidden index flags can conceal tracked-byte divergence from both clean-worktree checks. Exact-head CI and the 19 local canaries are green, but they do not exercise those races. The review ceiling binds, so no further fix cascade is authorized in this PR; both blocker threads remain unresolved.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ebf330e1f
ℹ️ 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".
| base_blob="$("$git_executable" rev-parse "$opening_base:$path")" || return 1 | ||
| head_blob="$("$git_executable" rev-parse "HEAD:$path")" || return 1 | ||
| if [[ "$head_blob" != "$base_blob" ]]; then |
There was a problem hiding this comment.
A PR-controlled check can run git replace <opening-base> <head> after local validation; replacement refs live only under the Git directory, so the checkout remains clean, but these ordinary object reads resolve every base path through the replacement and make changed Gitleaks workflows/configuration appear byte-identical to HEAD. A weakened scan can therefore pass hosted CI and still produce definitionsVerified: true and secrets.verdict: CLEAN; perform the exact-object reads with Git's documented --no-replace-objects option or reject replacement refs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH. Git replacement refs live outside the clean-worktree surface and can redirect the opening-base object reads so changed scan definitions appear equal, directly restoring a false CLEAN secrets verdict. #1557 is already parked at the review ceiling; this blocker remains intentionally unresolved.
| ```bash | ||
| gh pr checks $ARGUMENTS | ||
| if ! evidence_packet="$( | ||
| bash scripts/github/collect-pre-merge-evidence.sh finish VALIDATED_SESSION_TOKEN |
There was a problem hiding this comment.
Fresh evidence beyond the existing post-load state race is that this command exposes the authentication token in the finish process's argv. A same-user background process left by PR-controlled checks can read /proc/<pid>/cmdline while finish starts, recover the token, and recompute a valid binding for rewritten state; this defeats the token boundary even if the later state reread is made immutable. Supply the token through protected stdin or a non-inherited file descriptor rather than a command-line argument.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH under the PR-controlled-background-process threat model already established by the state race. Passing the token in argv exposes it through same-user process inspection and permits forging a new valid state binding. #1557 remains parked; this blocker is intentionally unresolved.
1 participant
Disposition
PARKED — not owner-ready. Exact-head CI and the local canaries are green, but four confirmed HIGH false-evidence paths remain intentionally unresolved under the two-round review ceiling:
assume-unchangedorskip-worktreecan conceal modified tracked bytes from the clean-worktree checks;Do not merge this head. A future successor must consume one immutable authenticated snapshot throughout finish, reject hidden index/replacement-ref state, and receive the token through protected stdin or a non-inherited descriptor.
Summary
.gitleaks.toml, and.gitleaksignorebefore emittingCLEANVerification
scripts/github/test-collect-pre-merge-evidence.sh: 19/19 canaries passed at the final headafca675f; the repaired final harness passedVALIDATED_SESSION_TOKENfinish/abort invocationsgit diff --check origin/main...HEADnode scripts/check-docs-governance.mjsnode scripts/check-golden-principles.mjsnode scripts/check-github-ops-governance.mjsDocumentation
docs/STATUS.md— not required; this head is unmerged and parkeddocs/IMPLEMENTATION_MASTERPLAN.md— not required; roadmap priorities are unchangeddocs/TESTING_GUIDE.mddocuments the changed evidence-gate verification flowTracking
Closes #1547
Related context: #1555, #1532, #1543, #1544.
CI Workflow Validation
Risk Notes
jq, BSDmktemp, contributor wording, optional GraphQL shape hardening, and review-checklist consolidation remain deliberately out of scope.