[bradygaster]

Working as Procedures (Prompt Architecture).

Closes #1824.

The defect

## Parse Command in workflows/squad.md said "Strip /squad prefix, trim whitespace" — a position-0 assumption — and had no failure branch at all. Step 4, "Default to cast if empty", was the silent-success path. There was no state in which the router could report "I did not understand this," so a run that cast nothing and a run that worked were byte-identical to the user.

The fix

Priority follows the issue's own framing: (2) fail loudly is the deliverable, (1) parse anywhere is today's instance.

• PC-1 scans the whole body for the first /squad token and prints either the argument text or the NO_COMMAND sentinel.
• PC-2 routes NO_COMMAND to PC-3 and explicitly forbids falling back to cast.
• PC-3 (mandatory) greps the body, emits ::error:: with the offending text quoted verbatim, comments on the issue, and exits non-zero. Never noop, never a success summary.

How this is verified

The parser is an LLM prompt, not code, so there is no function to unit-test. The parse is instead declared as concrete shell commands in the markdown, and test/gh-aw-command-parse.test.ts extracts those exact commands and executes them against real issue bodies. The declared contract and its observed behavior are independent sources — neither re-reads the other. This follows the existing TG-1 precedent in test/gh-aw-quality.test.ts.

A test asserting "a command at position 0 parses" would guard the one case that was never broken. The load-bearing assertion is the inverse: a body that yields no mode must produce an explicit sentinel and a diagnostic naming the offending text.

Proved it can fail

State	Result
Pre-fix workflows/squad.md	14 of 15 red
Fix applied	15 of 15 green

Honest caveat: that red is structural ("### Step PC-1 ..." is missing), not behavioral. It proves coupling to the new contract, not sensitivity to a subtly-wrong parser. A behavioral pre-fix red is not constructible — there was no prior implementation to run, only prose. The mutations below are what actually tested sensitivity, and one of them found a real hole.

Mutation 1 — reintroduce position-0 anchoring

Changed only the awk regex inside the fixed file; headings and prose untouched.

Only 1 of 14 assertions caught it. The headline case — prose, blank line, then the command, the literal #1824 scenario — passed under the reintroduced bug.

Cause: ^ in awk anchors per record, not per body. printf feeds awk line-by-line, so a line-anchored parser still handles a command sitting on its own line after prose. That case cannot distinguish the two parsers.

Added two cases whose only job is to carry that weight — an indented command and a mid-sentence command, both invisible to a line-anchored scan. Re-running the mutation then produced 2 failures naming the specific input. They look redundant next to the blank-line case; there is a comment in the test saying they are not.

Mutation 2 — generic diagnostic (the #1793 shape)

Replaced PC-3's grep with echo 'unrecognized command'. Every status-shaped signal stayed intact: ::error:: still present, PC-2 still forbids the fallback, the run still fails. A status-only assertion passes this clean — exactly how a truncating parser slipped through on #1793.

Caught by 3 assertions, because they require the diagnostic to contain the offending text, not merely to exist:

→ expected 'unrecognized command' to contain '/Squad cast'
→ expected 'unrecognized command' to contain '/squadcast'
→ expected 'unrecognized command' to be 'NO_SQUAD_TEXT_IN_BODY'

The gate cannot silently skip

HAS_POSIX_SHELL = existsSync('/bin/sh') is false on Windows, which silently skips 102 behavioral assertions in gh-aw-quality.test.ts — a permanently-green gate. This suite resolves Git Bash instead, and hard-asserts that a shell was found, so a suite that could not run reports red rather than green.

Deliberate non-actions

• The scan matches /squad inside quoted lines and fenced blocks. Excluding them reintroduces a silent-skip path — the exact bug class PC-3 exists to remove. gh-aw's slash_command trigger has already decided this is a squad command before the prompt sees the body. Documented in-file.
• The 102 /bin/sh skips were not fixed. Real, and worth its own issue; out of scope here and would blow Diff Size Guard.
• plan activate reads a hardcoded roster, not .squad/team.md — and reports a false provenance (re-diagnoses #1784) #1812 not touched. Verified separate: it is a hardcoded roster inside plan activate, downstream of routing. No shared code path with Parse Command.

Checks

• Targeted gh-aw suites: 150 passed — ambient budget still under 40 KB, byte conservation and skill extraction intact.
• npm run build passes. Build churn (two SKILL.md templates) was reverted, not committed; SKIP_BUILD_BUMP=1 suppressed the version bumps.
• Full suite: 11 failures, all pre-existing. Verified by stashing this branch's changes — 5 reproduce on clean dev; the other 6 pass when run in a smaller batch (15s/9s/5s durations in the full run mark them as parallel-load timeouts).
• No changeset: workflows/ and test/ do not match SDK_CLI_PATH_REGEX.

[github-actions]

🟡 Impact Analysis — PR #1832

Risk tier: 🟡 MEDIUM

📊 Summary

Metric	Count
Files changed	2
Files added	1
Files modified	1
Files deleted	0
Modules touched	2

🎯 Risk Factors

• 2 files changed (≤5 → LOW)
• 2 modules touched (2-4 → MEDIUM)

📦 Modules Affected

root (1 file)

• workflows/squad.md

tests (1 file)

• test/gh-aw-command-parse.test.ts

---

This report is generated automatically for every PR. See #733 for details.

[github-actions]

🛫 PR Readiness Check

ℹ️ This comment updates on each push. Last checked: commit 6e7628c

PR Scope: 🔧 Infrastructure

⚠️ 3 item(s) to address before review

Status	Check	Details
❌	Single commit	2 commits — consider squashing before review
✅	Not in draft	Ready for review
✅	Branch up to date	Up to date with dev
❌	Copilot review	No Copilot review yet — it may still be processing
✅	Changeset present	No source files changed — changeset not required
✅	Scope clean	No .squad/ or docs/proposals/ files
✅	No merge conflicts	No merge conflicts
❌	Copilot threads resolved	2 unresolved Copilot thread(s) — fix and resolve before merging
✅	CI passing	All checks passing

Files Changed (2 files, +445 −8)

File	+/−
test/gh-aw-command-parse.test.ts	+280 −0
workflows/squad.md	+165 −8

Total: +445 −8

---

This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.

[Copilot]

🟡 Changes recommended

Unresolved dispatch handling, shell-injection risk, and insufficient failure-path test coverage block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates the Squad workflow to parse /squad commands throughout issue text and fail visibly when none is found.

Changes:

• Adds sentinel-based parsing and explicit failure routing.
• Adds shell-backed parser and diagnostic tests.

File summaries

File	Review findings
workflows/squad.md	Moderate (3 votes): manual dispatch values bypass /squad parsing. Critical (1 vote): verbatim issue text is interpolated unsafely into shell commands.
test/gh-aw-command-parse.test.ts	Moderate (2 votes): PC-3 tests do not execute or validate the diagnostic emission and non-zero exit.

Review details

Suppressed comments (3)

test/gh-aw-command-parse.test.ts:91

• The positive parser cases here all exercise slash-prefixed bodies; none models a workflow_dispatch event, where the authoritative command input is bare (implement in squad-implement-worker.md). There is therefore no test that feeds that raw input through the same resolution/normalization, so the current regression (every manual dispatch yielding NO_COMMAND) remains green. Add a dispatch-input case and assert that it reaches the requested mode.

    const cases: Array<{ name: string; body: string; expected: string }> = [
      // The regression that was never broken — kept only as a floor.
      { name: 'command at position 0', body: '/squad cast', expected: 'cast' },

workflows/squad.md:244

• The sub(/^.*\/squad/...) expression is greedy, so when a matching line contains multiple tokens (for example, Please run /squad cast, then /squad status) it strips through the last /squad and returns status, despite the contract saying to take the first token. Extract the remainder from the first match() result and add a multiple-token fixture.

printf '%s\n' "$SQUAD_TRIGGER_BODY" | awk '{sub(/\r$/,"")} !f && /(^|[[:space:]])\/squad([[:space:]]|$)/ {f=1; sub(/^.*\/squad/,""); sub(/^[[:space:]]+/,""); sub(/[[:space:]]+$/,""); print} END{if(!f) print "NO_COMMAND"}'

workflows/squad.md:280

• PC-3 always requires a comment on a triggering issue, but workflow_dispatch also reaches this step for a non-empty invalid command (for example, cst) without a triggering issue or issue_number; that input does not match the issue-bound activation guard. There is no issue target to comment on, so this path can fail before delivering the promised diagnostic/comment. Make the comment conditional on a resolved issue target and retain the annotation/non-zero failure for targetless manual runs.

3. Post one comment on the triggering issue reproducing that same text verbatim
   and listing the valid commands from the Modes table.

• Files reviewed: 2/2 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bradygaster]

FIDO review — APPROVE WITH NITS

(GitHub blocks a formal Approve on an own-account PR, so this lands as a comment. Verdict is in the text.)

Adversarial re-derivation, not a read-through. I reproduced every claim independently in the worktree. Measured vs. assumed:

Verified by measurement

• Mutation 1 (reintroduce position-0 line anchoring): applied → 2 failed / 13 passed. The headline /squad cast silently no-ops with a green check unless the command starts the issue body #1824 case — prose, blank line, then command — passed under the reintroduced bug. Confirms the diagnosis: awk ^ anchors per record, printf feeds line-by-line, so a line-anchored parser still handles a command on its own line after prose. The obvious test is structurally blind to /squad cast silently no-ops with a green check unless the command starts the issue body #1824's own bug. Only the indented and mid-sentence cases caught it. Inverse check: removed those two, kept the mutation → 13/13 green, mutation escapes. They are the sole catchers (I measured 0 catch without them — marginally stronger than "1 of 14").
• Mutation 2 (generic diagnostic echo 'unrecognized command'): applied → 3 failed / 12 passed. Only the content-requiring assertions caught it; the status-shaped assertions (::error:: present, fallback forbidden, run fails) stayed green against the gutted diagnostic. That's the #1790's *.mjs text eol=lf does not repair existing working trees — 3 suites stay dead after merge #1793 shape, and this suite closes it by requiring the diagnostic to contain the offending text.
• Independence: the test extracts the fenced bash after each PC heading and execFileSyncs it. Mutating the declared parser flipped behavioral verdicts → not circular; a broken declared parser fails the suite.
• Anti-permanently-green: forced resolvePosixShell() → null. Suite went 13 red / 2 passed, guard test failing loudly. It reports red, not a green skip, when no shell resolves. Verified by force, not inferred.
• plan activate reads a hardcoded roster, not .squad/team.md — and reports a false provenance (re-diagnoses #1784) #1812 separation: diff touches only the routing section; the plan activate roster is a separate skill downstream of routing. No shared path.
• My own /squad cast silently no-ops with a green check unless the command starts the issue body #1824 acceptance test: ran the exact PC-1/PC-3 commands against empty / only-fenced / HTML-comment / URL / Cyrillic look-alike / two-per-line / squadify / tab / CRLF / quoted / zero-width / no-slash bodies. Every zero-action body → NO_COMMAND → PC-3 loud fail. I could not construct a silent no-op or a green-with-no-cast.

Nits (non-blocking)

1. <!-- /squad cast --> parses cast and would cast — documented "widening"; defensible, but invisible comment text triggering a real cast is worth a follow-up thought. It acts loudly, so it's the opposite of the /squad cast silently no-ops with a green check unless the command starts the issue body #1824 bug.
2. Two /squad on one line → greedy sub(/^.*\/squad/,"") picks the last token (…cast … plan → plan). Ordering surprise, still loud, not a no-op. Rare.
3. The "102 skips" figure isn't reproducible as-stated — I measure 13 skipped tests / 28 expect() in the two skipIf blocks. Cosmetic; the mechanism (permanently-green on Windows) is real.

Residual I cannot close (stated plainly)

PC-2/PC-3's "exit non-zero / never noop" are prose instructions to an LLM, not executed code. The suite verifies the diagnostic command is content-bearing and that the contract is declared — it cannot verify the runtime agent actually exits non-zero. Inherent to gh-aw; acknowledged by the author. The observable diagnostic is now mutation-sensitive, which is the strongest guarantee at this layer.

Ship it. Every load-bearing claim reproduced; the suite is mutation-sensitive on content (not status), fails red when it can't run, and is independent of the prose it guards. I tried to build a bypass and couldn't — and I'd rather say that than hand you a confident green.

— FIDO

[bradygaster]

FIDO — adjudicating the automated reviewer's findings (measured, not reasoned)

My original review stands. Coordinator asked me to adjudicate the two findings my first pass didn't cover, plus confirm the convergence. Where I couldn't measure, I say so.

Finding 1 — shell injection → NON-BLOCKING as stated; real hardening gap

Ran 8 hostile payloads through the exact PC-1 + PC-3 pipelines with the body via the environment (the channel the tests use): $(touch), `touch`, "; touch x; echo ", /squad cast; touch x, %s%s%s%n, /squad %n%n%n, -e /squad, --version.

• Zero side effects — no marker file ever created. The pipeline is injection-safe. printf '%s\n' "$VAR" puts the body in an argument slot, never a format slot (%s%s%s%n → NO_COMMAND, no crash, no write). grep -F -- '/squad' takes the pattern as a literal with the body on stdin, so -e/--version are data, not args. Every hop double-quoted.
• The reviewer's mechanism — "verbatim issue text interpolated into shell commands" — is not reproducible against the diff. The body is referenced ("$VAR"), not interpolated.
• Real residual: line 240 ("Assign the resolved trigger body to SQUAD_TRIGGER_BODY") doesn't say HOW. If an agent implements that by pasting the body into SQUAD_TRIGGER_BODY="<body>", that channel is an RCE — I demonstrated it (body hello"; touch pwned; echo " created the marker). There's no compiled squad.lock.yml in this repo, so I cannot measure the runtime channel and won't infer it.
• Required hardening (not a merge blocker for a demonstrated exploit): pin the assignment to a named gh-aw env var; add one line forbidding interpolation of body text into any script.

Finding 2 — workflow_dispatch bare command regresses to a loud failure → BLOCKING

• Measured: PC1("implement") = NO_COMMAND; PC1("research") = NO_COMMAND.
• That's exactly what dispatch sends — squad-implement-worker.md:259: "command": "implement" (bare). The activation guard (squad.md:158-160) itself lists implement/research/triage/plan* as valid bare dispatched commands.
• squad.md:240 names "the dispatched command" as a SQUAD_TRIGGER_BODY source and PC-1 is [MANDATORY] → implement → NO_COMMAND → PC-3 → exit non-zero. Pre-fix ("strip /squad, match longest-prefix, default cast") matched implement and worked — so this converts a working relay into a guaranteed false failure.
• The contradiction is the defect: 176-177 treats the dispatched command as pre-resolved; PC-1 demands a /squad token it never carries. Either the agent obeys [MANDATORY] PC-1 and hard-fails the core path, or it ignores a [MANDATORY] step (undefined). Untested — the suite has zero dispatch coverage.
• Fix shape: normalize dispatch input before PC-1 (treat a bare inputs.command as already-resolved, or synthesize SQUAD_TRIGGER_BODY="/squad ${command}"). Normalize the input; don't loosen PC-1 (that reopens /squad cast silently no-ops with a green check unless the command starts the issue body #1824).

Finding 3 — greedy sub(/^.*\/squad/,"") picks the LAST token → NON-BLOCKING, real defect (confirmed)

• Measured: PC1("Please run /squad cast, then /squad status") = status; contract (squad.md:247, "first /squad token") says cast. Two independent reviewers hit the same line → promoted from nit. Needs two /squad on one line and still yields a loud action (not the /squad cast silently no-ops with a green check unless the command starts the issue body #1824 class), so non-blocking — but it's on the same awk line, fix it here.

Finding 4 — PC-3 emission/exit untested: partly closable, mostly inherent

The diagnostic command is executed and content-asserted by the suite (Mutation 2 proved it). The ::error:: emission is prose with a placeholder (only its declaration is assertable — the test already does that). "Post a comment" and "exit non-zero" are gh-aw runtime actions, not reachable from a Vitest unit test. The reviewer's "test gap" and my "inherent limit" are the same boundary — recording it as a known limitation so it isn't re-discovered every review.

---

Verdict

#	Finding	Call
1	Shell injection	NON-BLOCKING (pipeline provably safe) + required hardening on the assignment channel
2	Dispatch bare-command regression	BLOCKING — net regression on the autonomous relay
3	Greedy last-token	NON-BLOCKING, fix in-PR (same awk line)
4	PC-3 emission/exit	Inherent; record as known limit; diagnostic half already closed

Overall: MERGE AFTER FIXES — rework the dispatch path. Finding 2 blocks. Finding 3 rides the same line. Finding 1 isn't a demonstrated exploit in the diff but leaves a security-critical step to LLM discretion — pin a safe channel. My #1824 assessment is unchanged: the issue-path fix is sound; the dispatch-path handling introduced alongside it is not.

— FIDO. Measured what I could; named what I couldn't.

[bradygaster]

FIDO — Pass 3 re-review of rework 6e7628c5 · Verdict: APPROVE (merge as-is)

Same adversarial bar as passes 1–2: extract the declared bash from workflows/squad.md, execute it, mutate the markdown, observe. Measured in a worktree on the branch @ 6e7628c5 with node_modules. Red is not enough — a failure must name the offending input.

Claims — verified by measurement

#	Claim	Result
1	PC-0 fixes the BLOCKING dispatch regression (my Finding 2)	✅ PC0("implement")→"/squad implement", idempotent, trims, ""→EMPTY_DISPATCH; all 6 dispatch modes resolve via PC1(PC0(x)). Resolved.
2	PC-1 deliberately NOT loosened; guard fires under mutation	✅ mutation (c) fires the dedicated guard and reopens the #1824 headline
3	Greedy last-token fix (my Finding 3)	✅ "Please /squad cast, then /squad status"→"cast, then /squad status" (first token wins)
4	Text contradiction resolved	✅ :177 "skip the remaining sources" = source selection; :184-187 "Choosing a source never skips parsing … MUST be normalized by PC-0 before PC-1". PC-1 still MANDATORY.
5	Three mutations red-AND-naming	✅ all three (below)
6	Tests 15 → 27	✅ 27 passed (27)
7	RETRO contract, hop-1 UNMEASURED, gate → #1834	✅ text says the gate is not implemented, "reviewed by hand, not enforced by CI"; cannot be misread as enforced
8	PC-3 exit-non-zero = known limitation (my Finding 4)	✅ recorded as inherent to gh-aw

Mutations (each red + names the input)

• (a) PC-0 pass-through → 7 red: A workflow_dispatch of "implement" must resolve … expected 'NO_COMMAND' to be 'implement'
• (b) greedy sub(/^.*\/squad/,"") restored → 2 red: PC-1 must extract "cast, then /squad status" from "Please /squad cast, then /squad status" … expected 'status'
• (c) PC-1 loosened → 9 red incl. PC-1 must still reject "implement" … expected 'implement' to be 'NO_COMMAND' and /squad cast silently no-ops with a green check unless the command starts the issue body #1824 headline We should improve the docs. returned as prose

File restored via git checkout -- after each; final tree clean.

The two probes raised

• Probe A — PC-0 NR==1 leading-newline: MEASURED, mechanism REAL, NON-BLOCKING (nit). PC0($'\nimplement')→"EMPTY_DISPATCH" (silent halt on a value carrying implement); PC0($'implement\nfoo')→"/squad implement" (line 2 dropped); CRLF handled. But command is a workflow_dispatch string input (squad.md:17-19); the only producers are the literal relay squad-implement-worker.md:259 "command":"implement" and the single-line "Run workflow" UI — neither can carry a newline. Reachable only via a hand-crafted actions:write API dispatch. Untested. Not a merge blocker, but a genuine latent silent-halt of this PR's own defect class — recommend a follow-up: pin it with a leading-newline test and/or scan the first non-empty line instead of binding NR==1.
• Probe B — enumerate every caller: MEASURED, clean. Grep of workflows/*.md for "command": producers → exactly one (squad-implement-worker.md:259). No third bare-command dispatcher. The "hardened in isolation" bug class has no other instance.

CI

All green: Diff Size Guard, Architectural Review, Security Review, test (4m51s), Policy Gates, Changeset Drift. docs-quality/Scope Boundary skipping. MERGEABLE/CLEAN.

Verdict — APPROVE (merge as-is)

The pass-2 BLOCKING regression is fixed and regression-guarded by input-naming tests; Finding 3 fixed; the contradiction resolved; all mutations red-and-naming; the two inherent limits written down. One NON-BLOCKING nit: the PC-0 NR==1 leading-newline edge (Probe A) — latent, unreachable via known producers, untested; worth a follow-up test, not a blocker.

(Posted as COMMENTED — GitHub blocks a formal Approve from the PR-owning account.)