Tolerate failed Codex review executions - #3807
Conversation
Codex failures previously terminated the shell before the existing fallback review could be generated and posted to Slack. Handle failed invocations as non-blocking so the workflow publishes its fallback review instead.
PR SummaryLow Risk Overview Both Codex paths (embedded diff with read-only sandbox and large-diff URL fetch with Downstream behavior is unchanged: if Reviewed by Cursor Bugbot for commit b3f6cfb. Bugbot is set up for automated code reviews on this repo. Configure here. |
![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: da2e54edf7
ℹ️ 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".
| > codex.log 2>&1; then | ||
| echo "Codex review failed; publishing fallback review." >&2 | ||
| fi |
There was a problem hiding this comment.
When the Codex command exits non-zero without producing review.md, the workflow now logs the failure only to the Actions log and then uploads the generic _Codex produced no output._ fallback to Slack while the job can still finish green. In that failure scenario, the Slack audience has no visible indication that the automated security review failed, which can make a missing review look like an empty review; write the failure reason into review.md or otherwise make the Slack artifact clearly identify the failed invocation.
Useful? React with 👍 / 👎.
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly makes Codex CLI failures non-blocking by wrapping both codex exec invocations in if ! ...; then guards, which suppresses set -e abort so the existing defensive review.md fallback and Slack-post steps still run. The change is symmetric across both diff-size branches and preserves the env -i secret scrubbing; it is safe to merge.
Findings: 0 blocking | 2 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion review (cursor-review.md) is empty — that pass produced no output. Codex's pass reported "No material issues found," consistent with this review.
- Nit: the stderr log message "Codex review failed; publishing fallback review." is slightly inaccurate for the embedded-diff path, where there is no distinct fallback review — the code simply falls through to the defensive
review.mdplaceholder ("Codex produced no output."). Cosmetic only.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward CI workflow fix.
Extended reasoning...
Overview
This PR modifies only .github/workflows/pr-to-slack-codex.yml, wrapping the two codex ... exec invocations in if ! ...; then ... fi blocks. Previously, under set -euo pipefail, a non-zero exit from the Codex CLI would abort the step immediately, skipping the existing fallback logic that writes _Codex produced no output._ into review.md when it's missing/empty. Now a failed Codex run logs a message to stderr and falls through to that existing fallback path, letting the rest of the job (Slack posting) proceed as originally intended.
Security risks
None. This is CI-only shell control flow; no application code, auth, or data paths are touched. Secret handling (OPENAI_API_KEY, SLACK_BOT_TOKEN) is unchanged — the env scrubbing (env -i ...) and downstream Slack upload logic are identical to before.
Level of scrutiny
Low. This is a small, mechanical, self-contained change to a non-production CI workflow, fixing a clear and narrowly-scoped bug (premature script termination) with a minimal, idiomatic bash fix. The change is easy to reason about by inspection and doesn't introduce any new failure modes beyond what already existed (the pre-existing fallback path is unchanged, just now reachable).
Other factors
No bugs were reported by the bug hunting system, and my own read of the diff confirms the fix is correct: both call sites are updated consistently, and the existing if [ ! -s review.md ] defensive check downstream still applies. Cursor's bot summary also concurs this is low risk.
There was a problem hiding this comment.
The PR wraps both codex exec invocations in if ! ...; then guards so that under set -euo pipefail a Codex failure no longer aborts the step, letting the existing defensive fallback populate review.md and the Slack-posting steps run. The change is correct and achieves its stated goal; only a minor logging inconsistency is worth noting.
Findings: 0 blocking | 3 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor review file (cursor-review.md) is empty — that second-opinion pass produced no output. Codex review reported no material issues.
- Logging inconsistency between the two failure branches: the large-diff branch dumps the Codex log (
cat codex.log >&2) on failure, but the embedded-diff branch only echoes a message. Consider dumpingcodex.login both failure paths so embedded-diff failures are equally diagnosable. - The fallback placeholder written to review.md (
_Codex produced no output._) does not distinguish a Codex execution failure from Codex running successfully but returning nothing; a distinct message on the failure path would make Slack readers aware the review was not actually performed.
There was a problem hiding this comment.
A small, correct CI-workflow change that wraps the two codex exec invocations in if ! ...; then so Codex failures no longer abort the step under set -euo pipefail, allowing the fallback/placeholder review and Slack-posting steps to run. Low risk; no blocking issues.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor second-opinion review (
cursor-review.md) is empty — that pass produced no output. Codex review reported no material issues. - Consider generating an actual fallback review on failure rather than only the
_Codex produced no output._placeholder, if the intent (per the PR description) is to publish a meaningful fallback to Slack. - The raw
${{ secrets.OPENAI_API_KEY }}interpolation into theenv -icommand line is pre-existing (not introduced here), but appears in log-adjacent context; worth confirming CI logs don't echo the expanded command. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| < prompt.txt \ | ||
| > codex.log 2>&1; then | ||
| echo "Codex review failed; publishing fallback review." >&2 | ||
| fi |
There was a problem hiding this comment.
[nit] Nit: for symmetry with the large-diff branch (which does cat codex.log >&2 on failure at line 142), consider also dumping codex.log here so an embedded-path failure surfaces its cause in the CI logs.
There was a problem hiding this comment.
LGTM — straightforward CI-only resilience fix.
Extended reasoning...
Overview
This PR modifies a single GitHub Actions workflow (.github/workflows/pr-to-slack-codex.yml) that runs an automated Codex AppSec review and posts it to Slack. The change wraps the two codex ... exec invocations (embedded-diff and large-diff fallback paths) in if ! ...; then ... fi blocks so that a non-zero exit from Codex no longer aborts the script under set -euo pipefail. This lets the existing downstream fallback (writing _Codex produced no output._ to review.md when it's empty) run and the Slack post still happen.
Security risks
None. This is CI-only tooling that has no bearing on the sei-chain application, consensus, or state machine — it is correctly labeled non-app-hash-breaking. The workflow does not change permission scopes, secret handling, or sandbox settings (--sandbox read-only / --sandbox danger-full-access usage is unchanged from before this PR).
Level of scrutiny
Low. This is a small, mechanical, well-understood shell idiom (if ! cmd; then ...; fi to tolerate a single command's failure under pipefail) applied symmetrically to both existing Codex invocation sites. The change is self-contained to one file and does not touch any production code path.
Other factors
The bug-hunting system found no issues. A P2 comment from the Codex bot itself notes that Slack won't clearly distinguish "Codex failed" from "Codex found nothing" — this is a minor observability nit for an internal tooling workflow, not a correctness or security bug, so it doesn't warrant blocking approval.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3807 +/- ##
==========================================
- Coverage 60.18% 59.21% -0.97%
==========================================
Files 2327 2235 -92
Lines 194602 184102 -10500
==========================================
- Hits 117112 109017 -8095
+ Misses 66918 65366 -1552
+ Partials 10572 9719 -853
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM — this is a minimal, self-contained CI workflow fix that does exactly what the description says.
What was reviewed: Both Codex invocation blocks (embedded-diff and large-diff paths) — confirmed each is now wrapped in if ! ...; then so a non-zero Codex exit no longer trips set -euo pipefail and aborts the job before the fallback review.md write. Confirmed downstream fallback ("Codex produced no output.") and Slack upload steps are unchanged. The two open review comments (Codex's P2 about surfacing failure text in the Slack artifact, and the seidroid nit about symmetric cat codex.log) are UX/observability suggestions, not correctness bugs.
Extended reasoning...
Overview
Single-file change to .github/workflows/pr-to-slack-codex.yml, a CI workflow that runs Codex-based AppSec review and posts results to Slack. The change wraps the two codex exec invocations (embedded-diff path and large-diff/URL-fetch path) in if ! ...; then ... fi blocks so a non-zero exit code from Codex is caught and logged instead of terminating the script under set -euo pipefail. This lets the existing fallback logic (writing _Codex produced no output._ to review.md when the file is missing/empty) run and post to Slack as before.
Security risks
None. This is purely CI orchestration code with no application, auth, crypto, or data-path changes. Secrets handling (OPENAI_API_KEY, Slack tokens) is unchanged from before this PR — same scrubbed env -i pattern, same sandbox flags (read-only / danger-full-access) per path.
Level of scrutiny
Low. This is a CI-only resilience fix, mechanical in nature (turning an unguarded command into an if ! guarded one, twice, following the same pattern already used for other defensive checks in this file like if [ ! -s review.md ]). It carries the non-app-hash-breaking label and cannot affect chain consensus, application logic, or security boundaries.
Other factors
Two automated reviewers (chatgpt-codex-connector, seidroid) left minor suggestions — surfacing the failure reason in the Slack artifact, and adding a symmetric cat codex.log >&2 to the first branch. Both are observability nits, not bugs; the workflow behaves correctly and safely either way. No outstanding unaddressed substantive concerns block approval.
|
/backport |
|
Successfully created backport PR for |
Backport of #3807 to `release/v6.6`. Co-authored-by: Masih H. Derkani <m@derkani.org> Co-authored-by: FromTheRain <bdchatham@gmail.com>
3 participants
Codex failures previously terminated the shell before the existing fallback review could be generated and posted to Slack. Handle failed invocations as non-blocking so the workflow publishes its fallback review instead.