Make pre-merge evidence atomic - #1555
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>
![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: 87530636a5
ℹ️ 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".
| cleanup_pre_merge_state() { | ||
| rm -f "$evidence_state" | ||
| } | ||
| trap cleanup_pre_merge_state EXIT |
There was a problem hiding this comment.
In ordinary Claude Code execution, this Step 1 Bash block ends before the separately documented test and diff-inspection phases, so its EXIT trap immediately deletes evidence_state and the shell also loses evidence_state and pr_number. Consequently Step 3 cannot expand pr_number, and Step 4's finish "$evidence_state" cannot reach the opening snapshot; the gate needs to persist the state path across tool calls or execute both collector phases inside one actual shell invocation rather than merely instructing that separate steps share a session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH: the documented fenced steps do not actually share shell variables, and Step 1's EXIT trap deletes the opening state before later calls. The bounded two review/fix rounds are exhausted, so PR #1555 is parked and this canonical thread remains unresolved. Successor acceptance criteria are recorded in #1547: #1547 (comment)
| --arg baseRef "$opening_base_ref" ' | ||
| .id == $runId and | ||
| .name == $workflowName and | ||
| .path == $workflowPath and |
There was a problem hiding this comment.
When a PR changes .github/workflows/ci-required.yml or its local reusable-gitleaks.yml, the pull-request run executes that PR-controlled definition while still reporting the same workflow name and .path. A no-op replacement that preserves CI and Secret Scan / Gitleaks Scan therefore satisfies this metadata predicate and emits Secrets scan: CLEAN; bind the attestation to a trusted base revision of the caller and reusable workflow, or downgrade the verdict whenever the scan definition differs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed HIGH. Live run 30705275967 resolved the reusable workflow from the PR merge ref, so run name/path/head/base metadata does not prove that the PR-controlled scan definition remained trusted. The two review/fix rounds are exhausted; PR #1555 is parked, this thread remains unresolved, and exact-base definition binding plus false-clean canaries are tracked in #1547: #1547 (comment)
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Review triage reply.
| cleanup_pre_merge_state() { | ||
| rm -f "$evidence_state" | ||
| } | ||
| trap cleanup_pre_merge_state EXIT |
There was a problem hiding this comment.
Confirmed HIGH: the documented fenced steps do not actually share shell variables, and Step 1's EXIT trap deletes the opening state before later calls. The bounded two review/fix rounds are exhausted, so PR #1555 is parked and this canonical thread remains unresolved. Successor acceptance criteria are recorded in #1547: #1547 (comment)
Chris0Jeky
left a comment
Chris0Jeky
left a comment
There was a problem hiding this comment.
Duplicate triage reply.
1 participant
Summary
Secrets scan: CLEANonly for a stable successfulSecret Scan / Gitleaks Scanfrom the completedCIActions run at.github/workflows/ci-required.yml, bound to the exact PR head/basereview-and-shippipeline instead of a local self-review workflowParked disposition
The automatic Codex review arrived after the bounded two review/fix rounds and identified two confirmed HIGH blockers. This PR is parked with those canonical threads unresolved; do not merge this head.
Successor acceptance criteria and the non-blocking portability follow-ups are recorded in #1547. The contributor-onramp P2 is recorded in #1544.
Verification
node scripts/check-docs-governance.mjsnode scripts/check-golden-principles.mjsnode scripts/check-github-ops-governance.mjsgit diff --check origin/main...HEADquick_validate.pyfor the Claude skill — NOT VERIFIED because that validator rejects the pre-existing Claude-supporteduser-invocablefield before body validationDocumentation
docs/TESTING_GUIDE.mddocuments the focused syntax/canary commandsdocs/STATUS.md/docs/IMPLEMENTATION_MASTERPLAN.mdnot changed: shipped product behavior and roadmap priority did not changeTracking
Status=Blocked,Priority=Priority ICI Workflow Validation
scripts/**Risk Notes