From 17ecf0b8c9c0f4bb9d098f25c2a2ece6a3500a4c Mon Sep 17 00:00:00 2001 From: Will Pfleger Date: Mon, 31 Aug 2026 13:31:22 -0400 Subject: [PATCH] fix(ci): salvage Codex review output on PTY-shutdown hang (#7042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex CLI can leave a PTY descendant holding the action's inherited stdio after the turn completes. The `runCodexExec.ts` wrapper waits on a `close` event that never fires, so the `Review pull request` step hangs until the job timeout kills it — discarding the finished review the CLI already wrote to disk. The CLI writes the completed review to the `--output-last-message` file (exposed as `output-file`) **before** the hang. This PR adds a salvage step that recovers it, and sets the step and job timeouts to preserve the full 30-minute Codex execution budget. **Changes (`codex-security-review.yml`):** - Add `output-file: ${{ runner.temp }}/codex-review.json` to the `Review pull request` step so the CLI writes the result before the hang. (`runner` context is valid in `steps.with`; not in `jobs.env`.) - Add `timeout-minutes: 30` and `continue-on-error: true` to the Codex step — a hang now costs ≤30 minutes instead of 40, and the salvage step still runs. - Set job `timeout-minutes: 40` to give setup, step cancellation, and salvage sufficient headroom without colliding with the Codex execution budget. The original 30-minute job timeout was too narrow: evidence from run [33114428326](https://github.com/block/buzz/actions/runs/33114428326/job/98665369165) shows completed output appearing 28m46s after step start, meaning a 20-minute step timeout could kill a legitimate review before the salvage file exists. - Add a `Salvage review output` step with `if: always()`: prefers `steps.run_codex.outputs.final-message` on a clean exit; falls back to the output file when the step timed out. The output file path is set in the step's own `env` block (`CODEX_OUTPUT_FILE: ${{ runner.temp }}/codex-review.json`), where `runner` is valid. Validates shape (non-empty JSON object, has `overall_risk`); fails the job hard if neither source is present. - Wire the job `outputs.review_json` to `steps.salvage.outputs.review_json`. **Changes (`Justfile`, `ci.yml`):** - Add `actionlint .github/workflows/codex-security-review.yml` to `security-review-check` so expression-validity errors are caught locally. - Provision `actionlint` via Hermit (pinned v1.7.12) rather than a one-off `Install actionlint` curl step, so the same binary is used locally and in CI. **Security posture is unchanged:** the salvage step reads the action's own output and a file written to `runner.temp` — neither is PR-controlled. Credential-stripping env block on the Codex step is untouched. Note this is a temporary workaround until https://github.com/openai/codex-action/issues/169 is addressed --------- Signed-off-by: Will Pfleger Co-authored-by: Duncan --- .github/workflows/codex-security-review.yml | 57 ++++++++++++++++++++- Justfile | 1 + bin/.actionlint-1.7.12.pkg | 1 + bin/actionlint | 1 + 4 files changed, 58 insertions(+), 2 deletions(-) create mode 120000 bin/.actionlint-1.7.12.pkg create mode 120000 bin/actionlint diff --git a/.github/workflows/codex-security-review.yml b/.github/workflows/codex-security-review.yml index df558a877d..68ff0553c0 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 @@ -228,7 +228,7 @@ jobs: REVIEW_REPOSITORY: review-target REVIEW_DIFF_FILE: .git/codex-review.diff 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 +287,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 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: # Checkout and fetch are complete. Remove runner credentials from the @@ -306,6 +312,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: ${{ runner.temp }}/codex-review.json output-schema: | { "type": "object", @@ -442,6 +450,51 @@ 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 }} + CODEX_OUTPUT_FILE: ${{ runner.temp }}/codex-review.json + 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}" + echo "$json" + echo "${EOF}" + } >> "$GITHUB_OUTPUT" + post-review: name: Post Codex Security Review needs: [prepare-review, security-review] diff --git a/Justfile b/Justfile index 32d83355e1..b73529d1f9 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 diff --git a/bin/.actionlint-1.7.12.pkg b/bin/.actionlint-1.7.12.pkg new file mode 120000 index 0000000000..383f4511d4 --- /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 0000000000..432f25e505 --- /dev/null +++ b/bin/actionlint @@ -0,0 +1 @@ +.actionlint-1.7.12.pkg \ No newline at end of file