From a01e3005c2efd26deaa05f357b716a30b71fe13a Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 29 Aug 2026 16:29:01 -0400 Subject: [PATCH 1/4] fix(ci): salvage Codex review output on PTY-shutdown hang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex CLI leaves a PTY descendant holding the action's inherited stdio after the turn completes; the action wrapper waits on a `close` event that never fires, so the job hangs until timeout-minutes kills it. The CLI writes the finished review to the output file before the hang, so the result is always present on disk — the job just never reads it. Add a salvage step that runs with `if: always()` after the Codex step: prefer the action's `final-message` output on a clean exit; fall back to the output file when the step timed out. Shape-validate the recovered JSON (non-empty object, has `overall_risk`); fail hard if neither source is present. Set `timeout-minutes: 20` and `continue-on-error: true` on the Codex step so a hang costs 20 minutes instead of 30 and the salvage step still runs. The output file is written to `runner.temp` (outside both checkouts) and the salvage step receives the action output as an env var, not from the PR-controlled tree, preserving the existing isolation posture. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/codex-security-review.yml | 54 ++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index df558a877d7..6a9d65d3c2a 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -227,8 +227,10 @@ jobs: REVIEW_CONTEXT: review-context REVIEW_REPOSITORY: review-target REVIEW_DIFF_FILE: .git/codex-review.diff + # Written by the action before the hang; lives outside the PR checkout. + CODEX_OUTPUT_FILE: ${{ runner.temp }}/codex-review.json outputs: - review_json: ${{ steps.run_codex.outputs.final-message }} + review_json: ${{ steps.salvage.outputs.review_json }} steps: - name: Checkout exact pull request head uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -287,6 +289,12 @@ jobs: # action's local proxy rather than entering the Codex subprocess. - name: Review pull request id: run_codex + # Codex CLI ≥0.149.x can leave a PTY descendant holding inherited stdio + # after the turn completes, stalling the action indefinitely. The output + # file is written before the hang, so a timeout here wastes at most 20 + # minutes instead of the full 30, and the salvage step recovers the result. + timeout-minutes: 20 + continue-on-error: true uses: openai/codex-action@86365089eb2b84e0a8fb0717b304f8bdcb13b20e # v1.12 env: # Checkout and fetch are complete. Remove runner credentials from the @@ -306,6 +314,8 @@ jobs: safety-strategy: drop-sudo permission-profile: ':read-only' working-directory: ${{ github.workspace }}/${{ env.REVIEW_CONTEXT }} + # Written before the hang; salvaged below if the step times out. + output-file: ${{ env.CODEX_OUTPUT_FILE }} output-schema: | { "type": "object", @@ -442,6 +452,48 @@ jobs: assumptions. Review only the authorized PR range and ground every finding in a changed hunk and a plausible failure or abuse path. + # Salvage the finished review whether the Codex step completed cleanly or + # timed out due to the PTY-shutdown hang. Prefer the action's final-message + # output (set on a clean exit); fall back to the output file written by the + # CLI before the hang. Fail the job only when neither source is available or + # the recovered JSON is not a valid review shape. + - name: Salvage review output + id: salvage + if: always() + env: + FINAL_MESSAGE: ${{ steps.run_codex.outputs.final-message }} + run: | + json="" + + # Prefer the action output set on a clean exit. + if [ -n "$FINAL_MESSAGE" ]; then + json="$FINAL_MESSAGE" + echo "source=action-output" >> "$GITHUB_STEP_SUMMARY" + elif [ -s "$CODEX_OUTPUT_FILE" ]; then + json="$(cat "$CODEX_OUTPUT_FILE")" + echo "source=output-file" >> "$GITHUB_STEP_SUMMARY" + else + echo "No review output from action or output file." >&2 + exit 1 + fi + + # Minimal shape validation: non-empty JSON object with overall_risk. + if ! echo "$json" | python3 -c " + import sys, json + d = json.load(sys.stdin) + assert isinstance(d, dict), 'not an object' + assert 'overall_risk' in d, 'missing overall_risk' + "; then + echo "Review JSON failed shape validation." >&2 + exit 1 + fi + + # Write as a multiline output (GitHub-safe delimiter). + EOF=$(dd if=/dev/urandom bs=15 count=1 2>/dev/null | base64) + echo "review_json<<${EOF}" >> "$GITHUB_OUTPUT" + echo "$json" >> "$GITHUB_OUTPUT" + echo "${EOF}" >> "$GITHUB_OUTPUT" + post-review: name: Post Codex Security Review needs: [prepare-review, security-review] From d97a4bbdb1a9536266451f41827066085c0a0fcd Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 29 Aug 2026 16:47:27 -0400 Subject: [PATCH 2/4] fix(ci): move runner.temp to step scope and add actionlint gate `runner` context is not allowed in `jobs..env`; the merged workflow would be rejected at load time, disabling the security review. Move `runner.temp` to the two step-level locations where it is valid: the Codex step's `output-file` input (inside `with`) and the salvage step's own `env` block. While here, fix a SC2129 shellcheck style warning in the salvage script by grouping the GITHUB_OUTPUT writes. Add `actionlint .github/workflows/codex-security-review.yml` to the `security-review-check` Justfile recipe so expression-validity errors are caught locally and in CI. Wire a pinned actionlint install (v1.7.12, SHA-256 verified) into the `changes` job in ci.yml immediately before the `just security-review-check` step. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/ci.yml | 6 ++++++ .github/workflows/codex-security-review.yml | 13 +++++++------ Justfile | 1 + 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 25a59c32432..4404792a54a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -89,6 +89,12 @@ jobs: scripts/test-mobile-release-candidate-publisher.sh - name: Mobile worktree identity contract run: scripts/test-mobile-worktree-overrides.sh + - name: Install actionlint + run: | + curl -fsSL --output /tmp/actionlint.tar.gz \ + https://github.com/rhysd/actionlint/releases/download/v1.7.12/actionlint_1.7.12_linux_amd64.tar.gz + echo "8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8 /tmp/actionlint.tar.gz" | sha256sum -c + tar -xzf /tmp/actionlint.tar.gz -C /usr/local/bin actionlint - name: Codex security review contract run: just security-review-check - name: Rust cache contract diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index 6a9d65d3c2a..af0d0b22107 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -227,8 +227,6 @@ jobs: REVIEW_CONTEXT: review-context REVIEW_REPOSITORY: review-target REVIEW_DIFF_FILE: .git/codex-review.diff - # Written by the action before the hang; lives outside the PR checkout. - CODEX_OUTPUT_FILE: ${{ runner.temp }}/codex-review.json outputs: review_json: ${{ steps.salvage.outputs.review_json }} steps: @@ -315,7 +313,7 @@ jobs: permission-profile: ':read-only' working-directory: ${{ github.workspace }}/${{ env.REVIEW_CONTEXT }} # Written before the hang; salvaged below if the step times out. - output-file: ${{ env.CODEX_OUTPUT_FILE }} + output-file: ${{ runner.temp }}/codex-review.json output-schema: | { "type": "object", @@ -462,6 +460,7 @@ jobs: if: always() env: FINAL_MESSAGE: ${{ steps.run_codex.outputs.final-message }} + CODEX_OUTPUT_FILE: ${{ runner.temp }}/codex-review.json run: | json="" @@ -490,9 +489,11 @@ jobs: # Write as a multiline output (GitHub-safe delimiter). EOF=$(dd if=/dev/urandom bs=15 count=1 2>/dev/null | base64) - echo "review_json<<${EOF}" >> "$GITHUB_OUTPUT" - echo "$json" >> "$GITHUB_OUTPUT" - echo "${EOF}" >> "$GITHUB_OUTPUT" + { + echo "review_json<<${EOF}" + echo "$json" + echo "${EOF}" + } >> "$GITHUB_OUTPUT" post-review: name: Post Codex Security Review diff --git a/Justfile b/Justfile index 32d83355e1c..b73529d1f99 100644 --- a/Justfile +++ b/Justfile @@ -99,6 +99,7 @@ check: fmt-check clippy desktop-check desktop-tauri-fmt-check desktop-tauri-clip security-review-check: node --check .github/scripts/codex-security-review.js node --test .github/scripts/codex-security-review.test.js + actionlint .github/workflows/codex-security-review.yml # Run the repository-wide differential file-size ratchet and its policy tests. # The ratchet inspects only files changed from the merge base, so this stays From 4e64099c8a808a003679f7b276ee6f775a97c9ba Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 29 Aug 2026 17:25:17 -0400 Subject: [PATCH 3/4] fix(ci): provision actionlint via Hermit instead of inline curl The previous approach installed actionlint with a custom curl/checksum/tar step in ci.yml, which kept the binary outside the Hermit environment. This made `just security-review-check` fail with `command not found` on any fresh checkout using the documented Hermit-only PATH (`just ci` / `just bootstrap` paths), while CI stayed green only because the custom install step masked the gap. Track actionlint-1.7.12 in Hermit so both local and CI environments get the same pinned binary through the same provisioning path. Remove the six-line curl/checksum/tar install step from the ci.yml contracts job; cashapp/ activate-hermit already runs there and will lazily provision the binary. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/ci.yml | 6 ------ bin/.actionlint-1.7.12.pkg | 1 + bin/actionlint | 1 + 3 files changed, 2 insertions(+), 6 deletions(-) create mode 120000 bin/.actionlint-1.7.12.pkg create mode 120000 bin/actionlint diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4404792a54a..25a59c32432 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -89,12 +89,6 @@ jobs: scripts/test-mobile-release-candidate-publisher.sh - name: Mobile worktree identity contract run: scripts/test-mobile-worktree-overrides.sh - - name: Install actionlint - run: | - curl -fsSL --output /tmp/actionlint.tar.gz \ - https://github.com/rhysd/actionlint/releases/download/v1.7.12/actionlint_1.7.12_linux_amd64.tar.gz - echo "8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8 /tmp/actionlint.tar.gz" | sha256sum -c - tar -xzf /tmp/actionlint.tar.gz -C /usr/local/bin actionlint - name: Codex security review contract run: just security-review-check - name: Rust cache contract diff --git a/bin/.actionlint-1.7.12.pkg b/bin/.actionlint-1.7.12.pkg new file mode 120000 index 00000000000..383f4511d44 --- /dev/null +++ b/bin/.actionlint-1.7.12.pkg @@ -0,0 +1 @@ +hermit \ No newline at end of file diff --git a/bin/actionlint b/bin/actionlint new file mode 120000 index 00000000000..432f25e505e --- /dev/null +++ b/bin/actionlint @@ -0,0 +1 @@ +.actionlint-1.7.12.pkg \ No newline at end of file From 4d09f4afce0fc8a0a32a0e23a1ac650782e76269 Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 31 Aug 2026 11:20:09 -0400 Subject: [PATCH 4/4] fix(ci): raise Codex step timeout to 30 min and job timeout to 40 min The 20-minute step timeout was below the only bounded timing evidence for a real heavy review: output appeared at 28m46s after step start in run 33114428326. A legitimate review could be killed before the salvage file is written, discarding a valid result. Separate the step and job deadlines to preserve the 30-minute Codex execution budget while still leaving headroom for setup, step cancellation, and salvage to complete within the job ceiling. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .github/workflows/codex-security-review.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index af0d0b22107..68ff0553c07 100644 --- a/.github/workflows/codex-security-review.yml +++ b/.github/workflows/codex-security-review.yml @@ -214,7 +214,7 @@ jobs: if: needs.prepare-review.outputs.authorized == 'true' runs-on: ubuntu-latest environment: codex-review - timeout-minutes: 30 + timeout-minutes: 40 concurrency: group: codex-security-review-${{ needs.prepare-review.outputs.pr_number }} cancel-in-progress: true @@ -289,9 +289,9 @@ jobs: id: run_codex # Codex CLI ≥0.149.x can leave a PTY descendant holding inherited stdio # after the turn completes, stalling the action indefinitely. The output - # file is written before the hang, so a timeout here wastes at most 20 - # minutes instead of the full 30, and the salvage step recovers the result. - timeout-minutes: 20 + # file is written before the hang, so a timeout here wastes at most 30 + # minutes instead of the full 40, and the salvage step recovers the result. + timeout-minutes: 30 continue-on-error: true uses: openai/codex-action@86365089eb2b84e0a8fb0717b304f8bdcb13b20e # v1.12 env: