From 5f0b47a81b5bf5731643b4ad6eccb2167f7bb934 Mon Sep 17 00:00:00 2001 From: brady gaster Date: Mon, 24 Aug 2026 11:37:44 -0700 Subject: [PATCH 1/2] fix(ci): guard the activation CLI pin against silent release drift The activation pin in workflows/shared/squad.md decays every npm release with nothing detecting it. It was introduced correct on 2026-08-07 at 0.11.0, 0.12.0 published on 2026-08-13, and the pin sat stale 8 days until a cold start against a throwaway repo happened to surface it (PR #1818). `git log -S` shows the line had been modified exactly once, ever -- at introduction. The pin stays: activation runs on an unrestricted-network job and hands state to the sandboxed agent job, so a bad point release mid-hop is expensive to debug. The missing drift mechanism was the defect. Two halves, each checking what the other structurally cannot: - offline, every PR: the file agrees with itself - online, daily: the file agrees with npm's published dist-tag Neither reads packages/squad-cli/package.json, which holds the next unreleased version (0.13.0, E404 on npm) -- the trap that would reproduce #1818. Also removes an unreachable `${SQUAD_CLI_VERSION:-0.12.0}` shell fallback that could never fire, since the env value is always non-empty. Being unreachable, it would have gone stale invisibly. Closes #1825 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df --- .github/workflows/squad-cli-pin-drift.yml | 137 ++++++++++++++++++++++ test/squad-cli-pin.test.ts | 133 +++++++++++++++++++++ workflows/shared/squad.md | 2 +- 3 files changed, 271 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/squad-cli-pin-drift.yml create mode 100644 test/squad-cli-pin.test.ts diff --git a/.github/workflows/squad-cli-pin-drift.yml b/.github/workflows/squad-cli-pin-drift.yml new file mode 100644 index 000000000..6fe402454 --- /dev/null +++ b/.github/workflows/squad-cli-pin-drift.yml @@ -0,0 +1,137 @@ +# Squad CLI pin drift guard (#1825) +# +# `workflows/shared/squad.md` pins the Squad CLI version that new-repo activation +# installs. That pin decays silently with every npm release: it was introduced +# correct on 2026-08-07 referencing 0.11.0, 0.12.0 published on 2026-08-13, and the +# pin then sat stale for 8 days with nobody touching it. `git log -S` shows the line +# had been modified exactly once, ever — at introduction. It was caught only because +# an agent happened to run a cold start against a throwaway repo (fixed in PR #1818). +# +# The pin itself is correct and stays: activation runs on an unrestricted-network job +# and hands state to the sandboxed agent job, so a bad point release mid-hop is +# expensive to debug. The absence of a drift mechanism was the defect, not the pin. +# +# This job compares the pin against the **published** npm dist-tag. It deliberately +# does NOT read `packages/squad-cli/package.json`: that is the next unreleased version +# (0.13.0 at time of writing) and resolves to E404 on npm, so automating from it would +# drive straight into the failure PR #1818 just fixed. A guard that reads the same +# source as the thing it guards is decoration (.squad/decisions.md:604) — so the two +# sources here are independent by construction. +# +# On drift it opens an issue rather than only failing, so the alert arrives with a fix +# path attached instead of an unexplained red run. + +name: Squad CLI Pin Drift + +on: + schedule: + # Daily at 07:15 UTC. Off the hour to avoid the scheduler's peak-minute backlog. + - cron: '15 7 * * *' + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: squad-cli-pin-drift + cancel-in-progress: false + +jobs: + check-pin: + name: Compare activation pin against published npm release + runs-on: ubuntu-latest + permissions: + contents: read + issues: write + steps: + - name: Checkout + uses: actions/checkout@v6.0.0 + + - name: Compare pin against npm dist-tag + id: compare + env: + PIN_FILE: workflows/shared/squad.md + NPM_PACKAGE: '@bradygaster/squad-cli' + run: | + set -euo pipefail + + # The pin as activation actually resolves it: the literal fallback in + # `vars.SQUAD_CLI_VERSION || ''`. Extracted with sed rather than a + # PCRE grep because the escaping stays readable, and an extractor nobody can + # read is an extractor nobody will notice has stopped matching. + pinned="$(sed -n "s/.*SQUAD_CLI_VERSION: .*|| '\([^']*\)'.*/\1/p" "$PIN_FILE" | head -1)" + if [ -z "$pinned" ]; then + echo "::error file=$PIN_FILE::Could not locate the SQUAD_CLI_VERSION pin. If the pin moved or changed shape, update this guard — a guard that silently stops finding its target is worse than no guard." + exit 1 + fi + + latest="$(npm view "$NPM_PACKAGE" dist-tags.latest 2>/dev/null || true)" + if [ -z "$latest" ]; then + echo "::error::Could not read dist-tags.latest for $NPM_PACKAGE from npm." + exit 1 + fi + + echo "pinned=$pinned" >> "$GITHUB_OUTPUT" + echo "latest=$latest" >> "$GITHUB_OUTPUT" + echo "Pinned in $PIN_FILE : $pinned" + echo "Published npm latest : $latest" + + if [ "$pinned" = "$latest" ]; then + echo "drift=false" >> "$GITHUB_OUTPUT" + echo "✅ Activation pin matches the published release." + else + echo "drift=true" >> "$GITHUB_OUTPUT" + echo "::warning::Activation pin ($pinned) is behind the published release ($latest)." + fi + + - name: Open drift issue + if: steps.compare.outputs.drift == 'true' + env: + GH_TOKEN: ${{ github.token }} + PINNED: ${{ steps.compare.outputs.pinned }} + LATEST: ${{ steps.compare.outputs.latest }} + TITLE: 'Squad CLI activation pin is behind the published release' + run: | + set -euo pipefail + + # Only one open issue at a time: this runs daily, and a guard that files a + # duplicate every morning trains people to ignore it. + existing="$(gh issue list --state open --search "$TITLE in:title" --json number --jq '.[0].number // empty')" + if [ -n "$existing" ]; then + echo "Drift issue already open: #$existing — not filing a duplicate." + exit 0 + fi + + # Built with printf rather than a heredoc: heredoc bodies must sit at column 0, + # which terminates the YAML block scalar this script lives in. + { + printf '%s\n\n' 'The Squad CLI version pinned for new-repo activation is behind the published npm release.' + printf '%s\n' '| | Version |' + printf '%s\n' '|---|---|' + printf '| Pinned in `workflows/shared/squad.md` | `%s` |\n' "$PINNED" + printf '| Published npm `dist-tags.latest` | `%s` |\n\n' "$LATEST" + printf 'New-repo activation installs `%s`, so anything that shipped in `%s` is missing from a cold start.\n\n' "$PINNED" "$LATEST" + printf '%s\n\n' '### Fix' + printf 'Update **both** literals in `workflows/shared/squad.md` to `%s`:\n\n' "$LATEST" + printf '%s\n' '1. the `Default is .` line in the header comment' + printf '%s\n\n' '2. the `SQUAD_CLI_VERSION` fallback literal in the activation `env:` block' + printf '%s\n\n' '`test/squad-cli-pin.test.ts` fails if those two disagree, so they cannot be updated by halves.' + printf '%s\n' '> Do **not** copy the version from the CLI package manifest in this repo. That is the' + printf '%s\n\n' '> next *unreleased* version and resolves to E404 on npm — the exact failure PR #1818 fixed.' + printf '%s\n' 'Filed automatically by `.github/workflows/squad-cli-pin-drift.yml` (#1825).' + } > "$RUNNER_TEMP/drift-body.md" + + gh issue create --title "$TITLE" --body-file "$RUNNER_TEMP/drift-body.md" + + - name: Fail the run on drift + if: steps.compare.outputs.drift == 'true' + env: + PINNED: ${{ steps.compare.outputs.pinned }} + LATEST: ${{ steps.compare.outputs.latest }} + run: | + # Values pass through env rather than expression interpolation, per the + # shell-input contract in workflows/squad.md: interpolation splices text into + # the script before the shell parses it, so the quoting that would contain a + # hostile value never gets a chance to apply. + echo "::error::Activation pin $PINNED != published $LATEST. See the issue filed by the previous step." + exit 1 diff --git a/test/squad-cli-pin.test.ts b/test/squad-cli-pin.test.ts new file mode 100644 index 000000000..a5175dd83 --- /dev/null +++ b/test/squad-cli-pin.test.ts @@ -0,0 +1,133 @@ +/** + * Squad CLI activation pin consistency (#1825) + * + * `workflows/shared/squad.md` states the pinned CLI version twice: once as prose in + * the header comment that documents `vars.SQUAD_CLI_VERSION`, and once as the literal + * fallback that activation actually resolves. Two copies of a version number drift + * against each other, and the prose copy is the one a human reads when deciding + * whether activation is current — so a stale comment misinforms precisely the person + * trying to verify the pin. + * + * This suite is the offline half of the guard. The online half + * (`.github/workflows/squad-cli-pin-drift.yml`) compares the pin against npm's + * published `dist-tags.latest` on a schedule; it cannot run here because it needs the + * network, and a network call in the unit suite fails closed on an offline machine. + * + * Split that way, each half checks something the other structurally cannot: + * - here: the file agrees with itself, on every PR, deterministically + * - there: the file agrees with reality, daily + * + * Neither reads `packages/squad-cli/package.json`. That holds the next *unreleased* + * version — 0.13.0 while this was written, which resolves to E404 on npm — so deriving + * the pin from it reproduces the exact breakage PR #1818 fixed. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const PIN_FILE = join(process.cwd(), 'workflows', 'shared', 'squad.md'); +const DRIFT_WORKFLOW = join( + process.cwd(), + '.github', + 'workflows', + 'squad-cli-pin-drift.yml', +); + +function readPinFile(): string { + return readFileSync(PIN_FILE, 'utf8'); +} + +/** The literal activation resolves: `vars.SQUAD_CLI_VERSION || ''`. */ +function effectivePin(source: string): string | undefined { + return source.match(/SQUAD_CLI_VERSION:\s*\$\{\{\s*vars\.SQUAD_CLI_VERSION\s*\|\|\s*'([^']+)'/)?.[1]; +} + +/** The version quoted in the header comment that documents the default. */ +function documentedPin(source: string): string | undefined { + return source.match(/^#\s*Default is ([0-9][^\s.]*(?:\.[^\s.]+)*)\.\s*$/m)?.[1]; +} + +describe('Squad CLI activation pin (#1825)', () => { + it('states a resolvable pin', () => { + const pin = effectivePin(readPinFile()); + + // If this stops matching, the drift workflow's extraction has almost certainly + // stopped matching too — and it would report "no drift" against a pin it never + // found. Failing here makes that visible on the PR that reshapes the line. + expect(pin, 'could not locate the SQUAD_CLI_VERSION fallback literal').toBeDefined(); + expect(pin).toMatch(/^\d+\.\d+\.\d+/); + }); + + it('documents the same version it resolves', () => { + const source = readPinFile(); + + expect(documentedPin(source), 'header comment does not document a default version').toBeDefined(); + expect(documentedPin(source)).toBe(effectivePin(source)); + }); + + it('does not carry a second, unreachable default', () => { + const source = readPinFile(); + const pin = effectivePin(source); + + // `SQUAD_CLI_VERSION` is set from `vars.X || ''`, which always yields a + // non-empty string, so a shell `${SQUAD_CLI_VERSION:-}` fallback can never + // fire. It is unreachable code whose only real effect is to be a third place the + // version can go stale — and being unreachable, it goes stale invisibly. + expect(source).not.toMatch(/\$\{SQUAD_CLI_VERSION:-/); + + // Belt and braces: no *other* copy of the version string in an npx invocation. + const npxLines = source.split(/\r?\n/).filter((l) => l.includes('squad-cli@')); + for (const line of npxLines) { + expect(line, `npx line hardcodes a version instead of using the env var: ${line.trim()}`) + .not.toContain(`squad-cli@${pin}`); + } + }); + + it('keeps the drift guard pointed at npm rather than the repo', () => { + const workflow = readFileSync(DRIFT_WORKFLOW, 'utf8'); + + // Comments are stripped first. The header comment names the trap deliberately, to + // explain why the guard avoids it — documenting a hazard should not trip the check + // that enforces it. What must not appear is the path in *executable* lines. + const executable = workflow + .split(/\r?\n/) + .filter((l) => !l.trimStart().startsWith('#')) + .join('\n'); + + // The trap this issue was filed to avoid: the CLI package manifest holds the next + // unreleased version, so a guard reading it would compare the repo to itself and + // then "fix" the pin to something npm cannot install. + expect(executable).not.toMatch(/packages\/squad-cli\/package\.json/); + expect(executable).toMatch(/dist-tags\.latest/); + }); + + it('passes values into shell through env, not expression interpolation', () => { + const workflow = readFileSync(DRIFT_WORKFLOW, 'utf8'); + + // Per the shell-input contract in workflows/squad.md: `${{ }}` is substituted into + // the script text *before* the shell parses it, so quoting cannot contain a hostile + // value — the value has already become code. Only `env:` mapping is safe. + // + // This guard also catches a subtler mistake made while writing this workflow: a + // literal `${{ ... }}` intended as documentation inside an issue body was silently + // evaluated by Actions instead of printed. + const lines = workflow.split(/\r?\n/); + const offenders: string[] = []; + let inRun = false; + let runIndent = 0; + + for (const line of lines) { + const indent = line.length - line.trimStart().length; + if (inRun && line.trim() !== '' && indent <= runIndent) inRun = false; + if (inRun && line.includes('${{')) offenders.push(line.trim()); + if (/^\s*run:\s*\|/.test(line)) { + inRun = true; + runIndent = indent; + } + } + + expect(offenders, `expression interpolation inside run: blocks:\n${offenders.join('\n')}`) + .toEqual([]); + }); +}); diff --git a/workflows/shared/squad.md b/workflows/shared/squad.md index 0f7264724..465d008c6 100644 --- a/workflows/shared/squad.md +++ b/workflows/shared/squad.md @@ -87,7 +87,7 @@ jobs: echo "✓ Existing squad team detected with roster entries — skipping init." else echo "No existing squad team found — running squad init." - npx --yes "@bradygaster/squad-cli@${SQUAD_CLI_VERSION:-0.12.0}" init --preset default --state-backend local + npx --yes "@bradygaster/squad-cli@${SQUAD_CLI_VERSION}" init --preset default --state-backend local fi - name: Upload Squad state artifact From 2390557dc01fdd64afbe7e2a0ececf5084531e61 Mon Sep 17 00:00:00 2001 From: brady gaster Date: Mon, 24 Aug 2026 12:01:49 -0700 Subject: [PATCH 2/2] fix(ci): scope an SC2016 suppression and restore its coverage by test CI lints with shellcheck 0.10.0 pinned; my workstation had no shellcheck at all, so local actionlint silently skipped its shellcheck integration and the drift workflow looked clean. Installed shellcheck 0.10.0 locally, reproduced the CI failure exactly, then fixed it. SC2016 fires on the markdown BACKTICKS in the issue-body printf format strings: shellcheck reads ` as command substitution and correctly reports it will not expand inside single quotes. That is the intent -- they are literal code spans in the rendered issue, and every real value ($PINNED, $LATEST) is already passed as a printf argument rather than interpolated into a format. Suppressed by exact code, scoped to that block with a written rationale, not file-wide and not by widening .github/actionlint.yaml -- same standard applied in #1827. SC2016 also catches a genuine defect: '$VAR' in single quotes when expansion was intended, which emits the literal text. The disable would hide that too, so `test/squad-cli-pin.test.ts` now asserts no single-quoted literal in the workflow contains a $expansion. Mutation-verified: planting `version is $LATEST` inside the suppressed block leaves actionlint CLEAN -- shellcheck is blind to it, as designed -- while the new test fails. The suppression removes the false positives without removing the check. 6/6 pass restored. Refs #1825 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 20afe6d2-444e-414e-8a39-e67ab67ca6df --- .github/workflows/squad-cli-pin-drift.yml | 10 ++++++++++ test/squad-cli-pin.test.ts | 24 +++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/.github/workflows/squad-cli-pin-drift.yml b/.github/workflows/squad-cli-pin-drift.yml index 6fe402454..fc491a4dc 100644 --- a/.github/workflows/squad-cli-pin-drift.yml +++ b/.github/workflows/squad-cli-pin-drift.yml @@ -104,6 +104,16 @@ jobs: # Built with printf rather than a heredoc: heredoc bodies must sit at column 0, # which terminates the YAML block scalar this script lives in. + # + # shellcheck disable=SC2016 + # SC2016 ("expressions don't expand in single quotes") fires on the markdown + # BACKTICKS below -- shellcheck reads ` as command substitution, and inside + # single quotes it correctly notes it will not expand. That is exactly what we + # want: these are literal code spans in the issue body, not shell expansions. + # Every real value ($PINNED, $LATEST) is passed as a printf ARGUMENT, never + # interpolated into the format string. Scoped to this block, not the file, and + # `test/squad-cli-pin.test.ts` asserts no single-quoted literal here contains a + # `$name` expansion -- so this suppression cannot mask a genuine SC2016 defect. { printf '%s\n\n' 'The Squad CLI version pinned for new-repo activation is behind the published npm release.' printf '%s\n' '| | Version |' diff --git a/test/squad-cli-pin.test.ts b/test/squad-cli-pin.test.ts index a5175dd83..1a7e9667f 100644 --- a/test/squad-cli-pin.test.ts +++ b/test/squad-cli-pin.test.ts @@ -130,4 +130,28 @@ describe('Squad CLI activation pin (#1825)', () => { expect(offenders, `expression interpolation inside run: blocks:\n${offenders.join('\n')}`) .toEqual([]); }); + + it('keeps its SC2016 suppression honest', () => { + const workflow = readFileSync(DRIFT_WORKFLOW, 'utf8'); + + // The issue-body builder disables SC2016 because shellcheck reads the markdown + // BACKTICKS in its printf format strings as command substitution and, correctly, + // reports that they will not expand inside single quotes. That is the intended + // behaviour — they are literal code spans in the rendered issue. + // + // But SC2016 also catches a real defect class: writing '$VAR' in single quotes + // while expecting expansion, which silently emits the literal text "$VAR". The + // suppression would hide that too. This test restores exactly that coverage, so + // the disable removes the false positives without removing the check. + // + // Values must reach printf as ARGUMENTS ("$PINNED"), never inside the format string. + const singleQuoted = [...workflow.matchAll(/'([^'\n]*)'/g)].map(m => m[1]); + const offenders = singleQuoted.filter(s => /\$[A-Za-z_{]/.test(s)); + + expect( + offenders, + 'single-quoted literals containing a $expansion — these will emit the literal text, ' + + `not the value, and SC2016 is suppressed here so shellcheck will not say so:\n${offenders.join('\n')}`, + ).toEqual([]); + }); });