feat(claude-security-review): declare the lane's execution verdict as workflow outputs by kyle-sexton · Pull Request #461 · melodic-software/ci-workflows

kyle-sexton · 2026-08-13T01:18:13Z

Closes #460

Summary

The security lane already computes an authoritative verdict on every run — the claude-lane-outcome composite classifies the attempt — but the verdict never left the job. on.workflow_call declared no outputs: and neither did the security-review job, so a caller that must decide whether a review actually happened had only the job log to read.

A log is not a contract, and melodic-software/claude-code-plugins#2517 is what that costs: its evidence guard grepped the log for the phrases naming a workflow-validation skip and matched the inline github-script SOURCE that mentions them as string literals, reddening exactly the in-scope pull requests the guard exists to approve.

Fix

on.workflow_call.outputs now declares four values, and the security-review job forwards three of them from the review-outcome step:

relevant — from the changes job, so a consumer can tell an out-of-scope skip from a no-verdict skip
review-ran, review-failed, failure-class — from the composite that owns the classification

Every output is EMPTY when security-review does not run, and the declaration says so in place: a consumer must read empty as "no verdict", never as a pass. The four legitimate no-verdict paths (out of scope, fork PR, skip-listed actor, either kill-switch) plus a superseded head all land there.

review-detail is deliberately not surfaced — it is a free-text projection shaped for a human reading a marker comment, and a consumer branching on it would be back to parsing prose.

The change is additive. No input, job name, check name, or permission changes, so every consumer on the current pin is unaffected until it re-pins.

Verification

New contract test .github/scripts/claude-security-review-declared-outputs.test.cjs (6 cases) pins the declared names, the wiring of each output to the job that computes it, the job-level forwarding, the agreement between these names and claude-lane-outcome's own declared outputs (so a rename cannot silently empty them), and that review-detail stays unsurfaced.
node --test .github/scripts/*.test.cjs — 700 pass, 0 fail.
actionlint clean on the edited workflow (the three property ... is not defined findings raised against the first commit's intermediate state were the job-level outputs: block not yet added; the final tree is clean).

… workflow outputs The lane already computes an authoritative verdict — the claude-lane-outcome composite classifies every attempt — but it never left the job. With no workflow_call outputs and no job-level outputs, a caller that must decide whether a review actually happened had only the job log to read, and a log is not a contract: claude-code-plugins#2517 grepped it for the phrases naming a validation skip and matched the inline github-script source that mentions them, reddening the pull requests the guard exists to approve. Surface relevant, review-ran, review-failed, and failure-class instead. review-detail stays unsurfaced — it is prose shaped for a human reading a marker comment, and branching on it is the log-grepping this replaces. Purely additive: no input, job name, check name, or permission changes, so consumers on the current pin are unaffected until they re-pin. Closes #460 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeEvWwrK8KdQTFb5n9CrUP

cursor · 2026-08-13T01:18:17Z

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

claude · 2026-08-13T01:18:34Z

Claude finished @kyle-sexton's task in 3m 20s —— View job

Code review

Todo list

Gather context (read changed files, diff, CLAUDE.md)
Invoke /review:code-review and follow its instructions
Post inline findings
Post final summary

(Note: the /review:code-review skill invocation failed silently in this environment — no skill content loaded despite multiple attempts. I proceeded with the equivalent manual review: correctness/maintainability focus, scoped out of security since this repo has a dedicated security-review lane.)

Summary

Traced the new outputs: wiring end-to-end against the code it describes:

on.workflow_call.outputs.relevant → jobs.changes.outputs.relevant → steps.filter.outputs.relevant — the changes job has no top-level if, so this is set on every non-cancelled run, matching the doc comment.
review-ran / review-failed / failure-class → jobs.security-review.outputs.* → steps.review-outcome.outputs.* (the claude-lane-outcome composite) — names, descriptions, and semantics match the composite's own declarations verbatim, and the composite's step is correctly gated (!cancelled() && steps.freshness.outputs.superseded != 'true') so a superseded run leaves these empty, as claimed.
review-detail is correctly left unsurfaced at both the call and job level.
New test file logic (claude-security-review-declared-outputs.test.cjs) checks out on manual read — its blockBody/declaredKeys parsing correctly isolates the call-level vs. job-level outputs: blocks and pins the composite/workflow name agreement.

One finding posted inline: .github/workflows/claude-security-review.yml#L459-464 — the new "DECLARED EVIDENCE SURFACE" comment enumerates "the four legitimate no-verdict paths" (out of scope, fork PR, skip-listed actor, kill-switch), but the job's pre-existing if: condition (lines 916-927) has a fifth, independent skip clause — a built-in bot-actor exclusion (non-dependabot[bot] bots on PRs from non-OWNER/MEMBER/COLLABORATOR authors) — that is distinct from inputs.skip-actors. When that clause trips, relevant stays 'true' while the other three outputs go empty, which is exactly the no-verdict shape the comment describes but doesn't name. Since this comment is meant to be the authoritative contract a consumer reasons from, the omission undercuts the goal of the PR.

Everything else — the wiring, the job/composite name agreement, the empty-on-skip semantics, and the deliberate omission of review-detail — is consistent between the workflow, the composite, and the new test.

Note: I was unable to execute node --test or actionlint in this sandbox (Bash commands beyond trivial ones required approval I couldn't obtain), so the PR's own verification claims (700 passing tests, clean actionlint) were not independently re-run — only verified by manual reading.

claude · 2026-08-13T01:18:35Z

Security Review In Progress

Gather context (PR diff, changed files, repo guidelines)
Review .github/workflows/claude-security-review.yml changes for Actions-security implications
Review new contract test .github/scripts/claude-security-review-declared-outputs.test.cjs
Post findings

Working on this now...

A GitHub expression in a plain string trips noTemplateCurlyInString; the repository's convention is an escaped template literal. Formatting is biome's own output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeEvWwrK8KdQTFb5n9CrUP

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 43deeac351

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

claude · 2026-08-13T01:21:35Z

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

Security Review Complete ✅

Todo list

Gather context (PR diff, changed files, repo guidelines)
Review .github/workflows/claude-security-review.yml changes for Actions-security implications
Review new contract test .github/scripts/claude-security-review-declared-outputs.test.cjs
Post findings

Summary

This is a pure outputs: declaration/forwarding change to a reusable workflow — no new inputs, secrets, permissions, triggers, or run-step logic. I traced the full data path and it holds together:

on.workflow_call.outputs (L448-492) → jobs.changes.outputs.relevant and jobs.security-review.outputs.{review-ran,review-failed,failure-class}.
The security-review job's new outputs: block (L939-942) forwards steps.review-outcome.outputs.* verbatim, and that step (L1479-1486) is the claude-lane-outcome composite invocation.
I diffed the forwarded names against the composite's own declared outputs (.github/actions/claude-lane-outcome/action.yml L41-60) — review-failed, review-ran, failure-class match exactly, so the contract test's "agreement" assertion is sound, not just self-referential.
failure-class is a closed enum computed in classify.cjs (auth | rate-limit | overloaded | other | skipped-validation), driven only by an HTTP status or an allowlisted substring match against known Anthropic error-type tokens — never raw model/PR-controlled text. No injection or public-log leakage risk in the newly-surfaced value.
The security-review job's if: skip conditions (kill-switches, out-of-scope, skip-listed actor, fork PR) and the review-outcome/attempt steps' superseded-head guard match the "four no-verdict paths + superseded head → all outputs empty" claim in the PR description.
No duplicate outputs: keys were introduced — confirmed only one outputs: block per job scope (call surface, changes job, security-review job).
review-detail is correctly left unsurfaced (verified by grep — not present in either new outputs: block), consistent with the stated rationale that it's free text unsuitable for a consumer to branch on.

I wasn't able to execute node --test or actionlint in this sandboxed session (Bash commands here require interactive approval I don't have), so I verified the wiring by static cross-reference of the workflow, the composite action, and the test's own regex assertions rather than by running the suite. Everything I could check by hand matches the PR's stated verification results.

Findings

None. This is a well-scoped, additive change with no security-relevant issues. The extensive in-file comments correctly document the no-verdict/empty-output contract, and the new test pins the exact wiring so a future rename in the composite or a re-ordering in the workflow can't silently start emptying these outputs.

github-actions · 2026-08-13T01:22:15Z

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

github-actions · 2026-08-13T01:23:30Z

Last security-reviewed head: 64c2e55d15fa9866b12899504b4142b6f2bbfd6e. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

… review Two review findings, both on the new block's prose, both real. The `relevant` output is not set on every non-cancelled run. The relevance step hard-errors on a rejected `!` / `?` / `+` pattern, which leaves the output unset while the lane fails OPEN and reviews anyway — so empty is a third state, and a consumer reading it as 'false' would wave a reviewed in-scope run through as out of scope. Documented as such, and pinned by a test. The skip enumeration undercounted: the job's `if:` also excludes a non-dependabot bot actor whose PR author is not OWNER/MEMBER/COLLABORATOR, a built-in gate distinct from the caller-supplied `skip-actors` list. Named, and the paragraph no longer reads as an exhaustive tally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KeEvWwrK8KdQTFb5n9CrUP

claude · 2026-08-13T01:28:54Z

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

Security Review Complete ✅

Todo list

Gather context (PR diff, changed files, prior review comments)
Invoke /review:security-review — the skill invocation failed silently in this environment (no content loaded, same behavior noted in this thread's earlier /review:code-review attempt), so I proceeded with the equivalent manual security review
Post findings

Summary

The last security-reviewed head was 2aec858 (per the bot's tracking comment above), so the only new material since then is commit 64c2e55, which is doc-only: it rewords the on.workflow_call.outputs header comment and the relevant output's description in .github/workflows/claude-security-review.yml, plus one new pinning test in .github/scripts/claude-security-review-declared-outputs.test.cjs. No logic, permissions, triggers, or outputs changed — only prose.

I traced both findings raised earlier in this thread against the current tree and both are now resolved:

Codex's finding (empty relevant as an unhandled third state when the relevance step hard-errors on a !/?/+ pattern): the relevant output's description (L470-482) now explicitly documents EMPTY as a third state ("scope unknown," not "no review"), and I verified against the actual step logic (L779-800) that this is accurate: the guard does exit 1 leaving relevant unset, and security-review gates on !cancelled() (L923) — a status function that overrides the default needs-failure skip — so the job still runs and the three verdict outputs still carry a real result. The description's claim matches the code.
My own earlier finding (the undercounted "four legitimate no-verdict paths," missing the built-in non-dependabot-bot/non-OWNER-MEMBER-COLLABORATOR exclusion): the header comment (L458-467) now enumerates all five independent skip clauses in the job's if:, and I cross-checked the enumeration against the actual condition (L924-933) — out-of-scope, the two kill-switches, skip-actors, the bot-actor exclusion, and the fork check — it's accurate and no longer presented as exhaustive-by-omission.
The new test (the relevant output documents its third state) pins the exact phrases (EMPTY is a THIRD state, fails OPEN) now present in the description, so a future edit that silently drops this documentation fails the suite rather than just reading stale.

No new inputs, secrets, permissions, or trust-boundary-relevant code paths were introduced by this commit — it's a comment/description correction plus a test that pins it. I found no security-relevant issues.

I was unable to execute node --test .github/scripts/*.test.cjs or actionlint in this sandboxed session (Bash commands beyond trivial ones require interactive approval I don't have), so the PR's own verification claims were checked by manual reading of the guard logic and the job if: condition rather than by running the suite.

Findings

None.
· branch feat/lane-declared-outputs

…its log (#2548) Closes #2541 ## Summary The security-evidence guard inferred a verdict the lane had already computed, through two channels that were wrong in the same way. It downloaded the lane's job log and grepped it. The lane's `Report review outcome` step is an inline `github-script` whose SOURCE is echoed into that same log and contains the skip phrases as string literals, so the grep matched every successful in-scope pull request and reddened exactly the ones the guard exists to approve (#2517). Anchoring to `##[warning]`/`##[error]` lines removed the false positive but left the guard coupled to log text nothing upstream pins. It also re-derived the PR's security scope in its own Python `fnmatch` implementation while the lane matches with `git check-ignore` — two matchers that disagree on exactly the patterns that distinguish them, and where the `set -e` scope-verdict defect lived. ## Fix Both channels are replaced by the lane's declared outputs (melodic-software/ci-workflows#461), passed in from `needs.security-review`: | lane says | guard does | | --- | --- | | job `skipped` / `cancelled` | not applicable | | job not `success` | defer to the job's own red | | `relevant: false` | out of scope, not applicable | | `review-ran: true` | evidence OK | | `review-ran: false`, `review-failed: true` | defer — the lane rules this GREEN on purpose so a provider outage cannot lock every merge; the guard says loudly that nothing was reviewed and does not overturn it | | `review-ran: false`, `review-failed: false` | FAIL — the action skipped itself; merging the caller change clears it, a re-run cannot | | no verdict at all | FAIL, unless the live head has moved — a run retired as superseded is the one legitimate cause, and it is identified by comparing the live head against the event head rather than assumed | | `LANE_RESULT` empty | FAIL — the guard is not wired to the lane, so it can determine nothing | The guard is now coupled to the pin: it fails closed when the outputs are absent at an unmoved head, so re-pinning BACKWARDS past the release carrying them turns it red rather than quietly blind. The caller's pin is bumped in this PR for exactly that reason. `actions: read` is gone with the log read; `pull-requests: read` covers the one remaining API call. ## Verification - `scripts/verify-security-review-evidence.sh.test.sh` rewritten to EXECUTE the guard against environment fixtures in an `env -i` child shell, instead of re-implementing its regexes inside the harness and asserting on the copies — the shape that let the tests agree with a guard that was failing every pull request. 20 cases, all pass locally, covering every row of the table above plus two static checks (no log read, no second paths matcher). - `shellcheck --rcfile .shellcheckrc` clean on both scripts, including the deliberately-enabled SC2310: the absent-verdict classifier reports on stdout with exit reserved for faults, the same verdict/status split the scope check needed. - The tests now run in CI, in the guard's own job, ahead of the guard. They ran nowhere before. ## Related - #2517 — the log-grep false positive this removes the possibility of - #2337 — the false-pass class the guard exists to catch - melodic-software/ci-workflows#461 — the declared outputs this consumes --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread .github/workflows/claude-security-review.yml

claude Bot reviewed Aug 13, 2026

View reviewed changes

Comment thread .github/workflows/claude-security-review.yml Outdated

kyle-sexton added the priority: needs-triage Default until a priority tier is assigned. label Aug 13, 2026

kyle-sexton merged commit 7107b34 into main Aug 13, 2026
41 checks passed

kyle-sexton deleted the feat/lane-declared-outputs branch August 13, 2026 01:34

kyle-sexton mentioned this pull request Aug 13, 2026

fix(security-evidence): decide from the lane's declared verdict, not its log melodic-software/claude-code-plugins#2548

Merged

Navigation Menu

Uh oh!

feat(claude-security-review): declare the lane's execution verdict as workflow outputs - #461

feat(claude-security-review): declare the lane's execution verdict as workflow outputs#461
kyle-sexton merged 3 commits into
mainfrom
feat/lane-declared-outputs

kyle-sexton commented Aug 13, 2026

Uh oh!

cursor Bot commented Aug 13, 2026

Uh oh!

claude Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

claude Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

github-actions Bot commented Aug 13, 2026

Uh oh!

github-actions Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Aug 13, 2026

Summary

Fix

Verification

Related

Uh oh!

cursor Bot commented Aug 13, 2026

Uh oh!

claude Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review

Summary

Uh oh!

claude Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review In Progress

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review Complete ✅

Summary

Findings

Uh oh!

Uh oh!

github-actions Bot commented Aug 13, 2026

Uh oh!

github-actions Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review Complete ✅

Summary

Findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

claude Bot commented Aug 13, 2026 •

edited

Loading

claude Bot commented Aug 13, 2026 •

edited

Loading

claude Bot commented Aug 13, 2026 •

edited

Loading

github-actions Bot commented Aug 13, 2026 •

edited

Loading

claude Bot commented Aug 13, 2026 •

edited

Loading