Skip to content

fix(claude-lane-outcome): classify a missing execution file as no-execution - #664

Closed
kyle-sexton wants to merge 2 commits into
mainfrom
cursor/no-execution-failure-class-dd48
Closed

kyle-sexton wants to merge 2 commits into
mainfrom
cursor/no-execution-failure-class-dd48

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #657.

Changes

  • claude-lane-outcome/classify.cjs: when a review step ends in failure and leaves no execution file, classifyExecutionFile now returns failure-class=no-execution instead of other. A step timeout and an early crash both end up here. An execution file that exists but can't be parsed still returns other.
  • claude-review.yml and claude-security-review.yml: the status step has a new no-execution) arm with the message "the review step produced no output before it ended (likely the step timeout); re-run the job". It still runs exit 1, so a PR never looks reviewed when no review ran.
  • Updated the failure-class output descriptions in action.yml, the review workflows, the intake-triage workflow and README.md.
  • Did the issue's optional item in a separate commit: the security-review step timeout goes from 14 to 10 minutes and its job timeout from 16 to 12. claude-code-plugins data supports this: the 55 most recent successful runs took p95 221 s and max 294 s.

Follow-up

The three reusable workflows still pin claude-lane-outcome at ac06265 (# v0.27.0). Consumers see no-execution only after a follow-up PR repoints that pin at a SHA that includes this change. Until then, the new status-step arm is in place but nothing triggers it.

Testing

  • node --test .github/scripts/*.test.cjs .github/actions/claude-lane-outcome/*.test.cjs passes 213 of 213. New tests cover the classifier, the composite outcome step, status-arm parity between the two review workflows, and output wiring.
  • actionlint 1.7.12 reports nothing on either review workflow.
Open in Web Open in Cursor 

cursoragent and others added 2 commits October 4, 2026 01:15
…cution

A step timeout and an early crash both end the review with outcome
failure and no execution file. That shape was folded into `other`, so
the status check read as an unclassified failure. Classify it as
`no-execution` and name it in both review status steps.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Successful security reviews on claude-code-plugins take p95 221 s and
max 294 s, so 14 minutes of step budget is unused. Cap the step at 10
minutes and the job at 12 so a hang fails faster while keeping the
same overhead margin as the code-review lane.

Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-04T01:21:07.040811Z 2a564ea PR opened
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 9s —— View job


Code review complete

  • Gather context (PR diff, changed files)
  • Review classify.cjs changes
  • Review workflow status-step changes (claude-review.yml, claude-security-review.yml)
  • Review tests and docs updates
  • Post findings

No blocking findings. The change is small, self-consistent, and well covered:

  • classify.cjs's new order-0 check (!executionFilePath || !fs.existsSync(...) → no-execution) runs only on the non-success path in action.yml's composite script — the success-with-no-evidence shape still goes to the separate skipped-validation branch, so the two don't collide.
  • The status-step no-execution) arm text and ordering match byte-for-byte between claude-review.yml and claude-security-review.yml (confirmed via diff), and claude-lane-status-check.test.cjs's new namedCaseArms/parity tests would catch future drift.
  • claude-security-review.yml's timeout arithmetic checks out: step 600 s + measured worst-case overhead 82 s + 38 s buffer = job's 720 s (12 min), the same 38 s buffer the prior 840/960 s pair implied. The claimed p95/max figures underlying the 10-minute step cap (in the PR description, not the diff) are author-reported and not something I independently verified.
  • claude-lane-outcome stays pinned at ac06265 (v0.27.0) in the three reusable workflows, so per the PR's own "Follow-up" note, consumers won't see no-execution until a later PR repoints that pin — expected, not a defect in this change.
  • Test coverage (classify.test.cjs, outcome-step.test.cjs, claude-lane-scope.test.cjs, claude-lane-status-check.test.cjs, claude-review-outcome-wiring.test.cjs) matches the behavior change and adds parity checks across both review lanes. I wasn't able to execute node --test or actionlint myself in this sandbox (no shell-command approval available in this context), so the PR's "213 of 213" and actionlint-clean claims are author-reported and unverified by me — worth a maintainer confirming CI is green before merge.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 2s —— View job


Security review — no findings

  • Gather context (PR diff, changed files)
  • Review classify.cjs logic change
  • Review workflow status-step changes (claude-review.yml, claude-security-review.yml)
  • Review intake-triage / action.yml / README doc changes
  • Check for instruction-surface deletions against protected classes
  • Post findings

Scope: classify.cjs, action.yml, the two review workflows' status-step case arm, claude-intake-triage.yml, README.md, and the accompanying test files.

Analysis:

  • The behavior change is a pure taxonomy split: a missing/unreadable execution-file on a failed outcome now returns failure-class=no-execution instead of other. This doesn't touch the model-output sanitization boundary — result/errors[] are still only read inside classify.cjs, and the new path returns before that parsing is even attempted (classify.cjs:87-93). No new data crosses the module boundary.
  • The new no-execution) arm added to both review workflows' status case "$FAILURE_CLASS" in block (claude-review.yml, claude-security-review.yml) sets why to a static string literal — no interpolation of attacker-influenced content, so no new command/expression-injection surface.
  • FAILURE_CLASS itself is still constrained to the closed enum classify.cjs emits (auth | rate-limit | overloaded | no-execution | other | skipped-validation | no-outcome); this PR doesn't change how that variable is populated or widen what feeds it.
  • The security-review step/job timeout reduction (14→10 min / 16→12 min) is an availability/reliability tuning, not a security control — it still ends in continue-on-error: true → the existing outcome-classification step, same as before, so a timeout still surfaces as a visible failed check rather than a silently-skipped review.
  • No changes to triggers, permissions:, secrets handling, or action/dependency pins (the claude-code-action SHA pin is unchanged other than the comment # v1.0.240 already present).
  • No CLAUDE.md/AGENTS.md/rules-file edits in this diff, so the instruction-surface-deletion lens doesn't apply.
  • Test/doc-only files (*.test.cjs, README.md) carry no security surface.

No security issues found in this PR.
· branch

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

PR body contract — issue linkage

This PR body does not yet satisfy the issue-linkage contract:

  • Missing a "## Summary" section. Describe what this PR changes and why, in a sentence or two.
  • Missing a "## Fix" section. State the concrete change and how it addresses the problem.
  • Missing a "## Verification" section. Record concrete evidence the change works (commands, gates, output).
  • Missing a "## Related" section. List related PRs, ADRs, or decision-log entries this PR does not close.

Edit the body and this comment updates itself on the next run.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

claude-security-review has reviewed this pull request through 2a564ea; a later push is reviewed from there.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

claude-review has reviewed this pull request through 2a564ea; a later push is reviewed from there.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a564eaf15

ℹ️ 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".

auth) why="the Claude credential was rejected (401/402/403); check the OAuth token and the plan's usage" ;;
rate-limit) why="the usage limit was hit (429); re-run once it resets" ;;
overloaded) why="the Claude service was overloaded (5xx); re-run later" ;;
no-execution) why="the review step produced no output before it ended (likely the step timeout); re-run the job" ;;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Repoint the workflows to the updated classifier

This arm is unreachable for callers of the reusable workflow at this commit: the Report review outcome step still pins ac062650..., whose classifyExecutionFile returns other when the execution file is absent. Consequently, a timed-out or early-crashed review continues to be reported as an unclassified failure despite the newly documented no-execution contract; publish the updated composite and repoint all three workflow consumers in the same change.

Useful? React with 👍 / 👎.

kyle-sexton added a commit that referenced this pull request Oct 4, 2026
…669)

No related issue: Phase 4 of the GitHub Actions conventions program
(tracking: melodic-software/standards#672)

## Summary

Renames this repository's workflows and actions to the org naming
convention (`<stage>-<function>[-<modifier>]`, `name:` = file stem), as
recorded in standards
`components/github-actions-conventions/rename-map.json`. It also folds
the three `-self` dogfood callers into thin jobs in
`pr-require-checks.yml` (formerly `ci.yml`). This is the content of the
v0.34.0 restructure release; consumers keep working on their SHA pins
until they repin.

## Fix

- `d1b5dc0`: `git mv` of 45 files to their mapped paths, `job_renames`
applied (including `needs:` and `needs.<id>.result`), and reusables
reference their own actions with `$/.github/actions/<x>` instead of a
pinned full path.
- `a724617`: deletes `claude-review-self.yml`,
`claude-security-review-self.yml` and `issue-triage-label-self.yml`;
adds jobs `pr-review`, `pr-review-security` and
`intake-label-needs-triage`, each pinned to the v0.33.0 SHA of its old
path with the same permissions and the one secret the `-self` file
passed. It adds the `ready_for_review` and `issues: [opened, reopened]`
triggers and the event guards, keeps the three folded jobs out of
`ci-status`, and deletes `composites-head`, because `$/` from a `./`
call already runs HEAD's composites.
- `6c7ba42`: the two standards-synced composites (`comment-hygiene` and
`machine-specific-paths`) are reached through `$/`. Their directories
keep their old names until the standards sync destinations move.
- `b2f99f9`: the folded review jobs run only on opened, synchronize,
reopened and ready_for_review, which is what the `-self` files ran on.
- `ci-status` keeps its job id and stays the only required check.

## Verification

- Fresh-context verifier: 8 of 9 criteria pass. The ninth fails on one
pre-existing naming-lint warning (job id `plan` in
`maintenance-sync-standards.yml`). It is non-blocking, and the job id is
unchanged from main.
- `actionlint`: only the `$/ ... ref is missing` errors, which
standards#675 waives through the synced config.
- `node --test .github/scripts/*.test.cjs`: 192/192 pass.
- naming-lint enforcing: only the two sync-managed directories remain,
and they move with the standards sync-destination change.
- The github-iac governance-verify draft (github-iac#598) passes against
this branch.

## Related

- Prerequisites, all merged: standards#675 (actionlint `$/`), #676
(lockstep drift), #677 (repin path mapping), #678 (runner-policy `$/`),
and the Dependabot ignores in six repos.
- Lands in lockstep with github-iac#598, followed by a self-repin PR and
the v0.34.0 release.
- Open PRs #663, #664 and #665 touch renamed paths and need a rebase
after this merges.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Closing: stale agent PR built on pre-rename paths (v0.34.0 renamed the workflows); superseded by the conventions migration.

@kyle-sexton kyle-sexton closed this Oct 7, 2026
@kyle-sexton
kyle-sexton deleted the cursor/no-execution-failure-class-dd48 branch October 7, 2026 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(claude-lane-outcome): classify a review step with no execution file as its own failure class

2 participants