Fail loudly when the OIDC workflow-file check silently skips a run - #55
Conversation
…kips a run
Upstream's own run.ts leaves its conclusion output unset on exactly one
reachable code path through this wrapper: setupGitHubToken() throwing
WorkflowValidationSkipError, when github_token is left unset (the
direct-form App-identity path) and GitHub's OIDC platform check rejects
the workflow file used for this run as not byte-identical to the current
default branch. Upstream treats this as a soft skip and returns cleanly,
so the calling step reports success with nothing having actually run --
no review, no comment, no visible signal anything is wrong. The other
code path that leaves conclusion unset ("no trigger found") is provably
unreachable through this wrapper, which always sets a non-empty prompt,
so a winning attempt with no conclusion at all can only mean the
workflow-validation skip.
Resolve Claude Code result now detects this and fails the job outright
with an error naming both possible causes (this run's own workflow file
was edited in the pull request, or the branch has fallen behind a
since-changed copy of the file on the default branch) instead of letting
the job report a misleading green success. Confirmed against a real
occurrence on adpeak/adpeak-mono, where a branch had fallen behind a
same-day rewrite of its own claude-review.yml on main.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verified the core claim this PR relies on directly against upstream's pinned SHA (e8c2d7c1..., v1.0.208):
src/entrypoints/run.ts'sWorkflowValidationSkipErrorcatch sets noconclusionoutput and returns cleanly (soft skip, exit 0).- The only other early return before
conclusionis set is the "no trigger found" path in the same file, gated oncheckContainsTrigger(src/github/validation/trigger.ts), whose very first check isif (prompt) return true— and this wrapper's "Compose prompt" step always emits a non-emptypromptfor every mode (includinggeneric, via its requiredPROMPT_FILE). So that path is genuinely unreachable through this wrapper, exactly as the PR body claims. - Upstream's own
action.ymloutputs:block (same SHA) does not exposeskipped_due_to_workflow_validation_mismatchat all, confirming the wrapper has no direct signal to read and must infer the skip from an emptyconclusionon a winning attempt — which is exactly what the new check does.
Traced the new action.yml block's integration with the rest of "Resolve Claude Code result": it runs after the existing "no winner" check and before the output-emission loop, so on trigger it produces steps.claude.outcome == failure with session_id left unset — every downstream step (wrap-up, fix pass, structured summary, progress-comment update) already gates on outcome/session_id correctly for that case, so nothing downstream is left in a bad state. The bash (${!WINNER_CONCLUSION_VAR:-} indirect-expansion pattern) matches existing idioms already used in the same script and is safe under set -euo pipefail.
The README/CLAUDE.md wording change (they're the same file — CLAUDE.md is a symlink to README.md) accurately describes the mechanism and gives a correct, actionable next step for both real causes (self-editing the workflow file vs. a stale branch).
No related open issue exists to link (the repository currently has zero issues, open or closed). Of the other open PRs, only #47 (dependabot bump of the pinned upstream SHA to 1.0.215) touches the same general area (the pinned action and its run.ts behaviour this PR's reasoning depends on) — worth a quick re-check of this same reasoning against 1.0.215 once that lands, but nothing to block on here since it hasn't merged yet.
Title and description are already complete and accurate (clear rationale, real-world trigger via the adpeak/adpeak-mono incident, and a concrete manual test plan), so no rewrite needed.
No Blocker or Should-fix findings. Approving.
|
🗜️ Headroom context compression
|
|
🎉 This PR is included in version 1.10.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Traced a real "why did this review not run" question on adpeak/adpeak-mono to this: github_token was unset (direct form, App identity), GitHub's own OIDC platform check rejected the workflow file as not byte-identical to the default branch, and upstream's run.ts treats that as a soft skip -- it returns cleanly with no error, so the whole job reports green with nothing having actually happened. In that case the PR's branch had simply fallen behind a same-day rewrite of claude-review.yml on main; the PR itself never touched any workflow file.
Traced through upstream's actual run.ts: conclusion is left unset by exactly two code paths -- the workflow-validation skip, and a "no trigger found" skip that's provably unreachable through this wrapper (every mode here always sets a non-empty prompt, which is what upstream's own trigger check requires). So a winning credential attempt with no conclusion output at all can only mean the workflow-validation skip.
Resolve Claude Code result now checks for exactly that and fails the job outright with an error naming both real causes (this PR edited a workflow file, or the branch fell behind main), instead of a silent green success.
Test plan
npm run lint/typecheck/format:checkall pass