Skip to content

fix(cli): authorize the sender before judging the command shape - #221

Open
cdotta wants to merge 1 commit into
theam:mainfrom
cdotta:fix/trigger-authorization-order
Open

fix(cli): authorize the sender before judging the command shape#221
cdotta wants to merge 1 commit into
theam:mainfrom
cdotta:fix/trigger-authorization-order

Conversation

@cdotta

@cdotta cdotta commented Aug 30, 2026

Copy link
Copy Markdown

Relates to #218.

If this does not match how you want the triggers to behave, please just close it — no explanation needed. It changes behavior rather than fixing something plainly broken, so the call is yours. I wrote it because I had the reproduction in front of me and it seemed more useful to show the change than describe it. The evidence stands in #218 either way.

Problem

Both agent triggers validate command shape and exit non-zero before checking whether the sender has write access. In facility-crew.yml the error is at line 110 and the permission gate at line 160.

The job-level prefilter matches the command anywhere in a comment body, so any account on a public repository reaches that step. An account with read permission commenting prose produced:

##[error]No supported agent command found. Use /builder or /architect at the start of a line.
##[error]Process completed with exit code 1.

It also affects maintainers: writing about the commands in an issue turns the repository's checks red. The step's own comment says the line-anchored pattern "ignores prose examples (ask /architect about it)" — it does ignore them for dispatch, but the run still fails loudly rather than skipping.

facility-codex.yml already handles the same input the way I would expect (::notice::No Codex command starts a line; treating the mention as prose), so the two sibling workflows currently disagree.

Change

The authorization gate runs first, and a body with no line-anchored command becomes a run=false skip rather than a hard error — matching what codex already does. A genuinely ambiguous request from an authorized sender (both commands, line-anchored) stays a hard error.

Test

packages/cli/test/crew-resolve.test.mjs renders both workflows through the real installer, lifts the resolve step's run: block out of the rendered YAML, and executes it under bash against real event payloads with a stubbed gh permission lookup.

Statement order is not observable from a text assertion, which is why CONTRIBUTING's rule about exercising the rendered workflow rather than checking text matters here. 8 tests; 3 fail on main.

The tests require bash 4 for mapfile, as the shipped script does, and skip on older shells — macOS still ships bash 3.2, so a contributor there would otherwise see a false failure.

Deliberately not included

The unguarded toolchain steps. A refused request still checks out the repository and runs pnpm install --frozen-lockfile, because {{TOOLCHAIN_STEPS}} carries no if: while the steps either side of it do — the job then reports success. Guarding those steps is the right fix, but toolchainSteps()'s existing conditional option guards on steps.workflow-change.outputs.changed, so it is not a drop-in, and the shape of that fix is yours to choose. Evidence and run logs in #218.

Both agent triggers validated command shape and exited non-zero before
checking whether the sender had write access, so any account on a public
repository could produce a red run by commenting prose that merely names a
command:

  ##[error]No supported agent command found.
  ##[error]Process completed with exit code 1.

In facility-crew.yml the error was at line 110 and the permission gate at
line 160. The gate now runs first, and a body with no line-anchored command
is a run=false skip rather than a hard error, which is what
facility-codex.yml already did for the same input. A genuinely ambiguous
request from an authorized sender stays a hard error.

Tests render the workflows through the installer and execute the resolve
step against real event payloads with a stubbed permission lookup, since a
text assertion cannot observe statement order. They require bash 4 for
mapfile and skip on older shells.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the contribution!

The resolver now rejects unauthorized senders before command-shape validation, but in facility-crew that decision still does not gate checkout or {{TOOLCHAIN_STEPS}}. On pull_request_review_comment, the default checkout uses the PR merge ref, so an outsider can post a comment containing /builder and make the job run pnpm install, npm ci, or yarn install from PR-controlled code even though the resolver writes run=false.

Please guard checkout and every generated setup/install step with steps.requested-agent.outputs.run == true, and add rendered-workflow coverage proving a rejected request cannot execute them. Once that authorization boundary is enforced, this should be ready.

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.

2 participants