* test(#4109): add zsh regression coverage for gate-check and invoke_reviewers dispatch Extends tests/review-plan-coverage-manifest.test.cjs to cover the two DISPATCH_SLUGS consumption sites the existing #3301 coverage-check tests don't reach: the write_reviews gate-check block (ALL_LANES_SKIPPED / TOTAL_LANE_FAILURE counting) and invoke_reviewers' dispatch + join loops. Both extract the real shipped bash from review.md and run it under bash and zsh, same convention as the existing coverage-check rows. Expected RED under zsh pre-fix, GREEN post-fix. * fix(#4109): rewrap unquoted DISPATCH_SLUGS-shaped consumption to survive zsh Bash word-splits an unquoted scalar on IFS by default; zsh does not (no setopt SH_WORD_SPLIT anywhere in these files), so a scalar accumulated from multiple space-separated tokens and then consumed via bare `for x in $VAR` collapses onto one bogus iteration under zsh whenever it holds 2+ tokens. Fixes all four review.md sites reported by #4108/#4109 (invoke_reviewers dispatch + join loops, the gate-check block, and the #3301 coverage-check block), plus the identical pattern found by a repo-wide sweep in complete-milestone.md, code-review.md, pr-branch.md, sync-skills.md, and execute-phase/steps/per-plan-worktree-gate.md. Each site is rewrapped in unquoted command substitution (`$(printf '%s' "$VAR")`), which re-splits identically under both shells regardless of SH_WORD_SPLIT — the same mechanism that already made the accumulator-building loops in these files shell-safe. * test(#4109): warn loudly when the zsh probe fails instead of skipping silently detectShells() drops the zsh test lane whenever a live zsh probe fails (e.g. on a CI runner without zsh installed), and a dropped lane reads identically to a passing one in the suite's own output — exactly the blind spot that let #4109's bug class ship undetected. Both copies of this helper (this file and its byte-identical duplicate in review-build-prompt-optional-sections.test.cjs) now emit a greppable console.warn when the probe fails, so a run without zsh reads as "zsh coverage unknown" rather than "all lanes green". * ci(#4109): gate workflows/*.md's embedded bash blocks in lint:ci Adds scripts/lint-workflow-shellcheck.cjs, wired into npm run lint:ci, so this bug class can't land undetected a third time. Two independent checks run over every ```bash block in gsd-core/workflows/**/*.md: - ShellCheck (new `shellcheck` devDependency, downloads and caches the real koalaman/shellcheck binary) catches the general unquoted-expansion/ word-splitting family in argument position. 212 pre-existing findings across the tree are absorbed into scripts/lint-workflow-shellcheck-baseline.json (matched on {file, code, message}, not line number, so unrelated edits elsewhere in a file can't spuriously un-baseline anything) — fixing all of them is out of scope for this issue; only NEW findings fail the build. - A custom structural check specifically for #4109's own shape: ShellCheck does not flag a bare `for x in $VAR` word-list — it treats that as an intentional idiom under any ruleset (confirmed empirically). This check does, and gates the build on any occurrence, with zero tolerance (no baseline) since every known site was already swept and fixed on this branch. * test(#4109): update pr-branch cherry-pick loop test anchor for the zsh fix extractPickLoop()'s PICK_LOOP_MARKER located the create_pr_branch cherry-pick loop by the literal substring "for HASH in $INCLUDED_COMMITS", which no longer appears verbatim after this issue's fix rewrapped that loop in unquoted command substitution. Updates the anchor (and its error message, now derived from the same constant instead of duplicating stale text) to the new literal form. No behavior change to the extraction logic itself. Emitted-Drift-Ack-Growth: review.md — #4109's fix adds explanatory comments at 4 sites; net code is functionally equivalent, comment expansion grows the file Emitted-Drift-Ack-Growth: complete-milestone.md — #4109's fix adds explanatory comments at 3 sites documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: code-review.md — #4109's fix adds an explanatory comment documenting the zsh word-splitting divergence Emitted-Drift-Ack-Growth: pr-branch.md — #4109's fix adds explanatory comments at 3 sites (TRANSIENT_DIRS, INCLUDED_COMMITS, FILTER_PATHS) Emitted-Drift-Ack-Growth: sync-skills.md — #4109's fix adds explanatory comments at 2 sites (CREATE_LIST/UPDATE_LIST, REMOVE_LIST) * test(#4109): cover lint-workflow-shellcheck parsers + add timeout Adds tests/lint-workflow-shellcheck.test.cjs covering the hand-rolled parser/logic functions exported by scripts/lint-workflow-shellcheck.cjs (stripCommandSubstitutions, stripShellComments, substitutePlaceholders, extractForLoops, findBareForLoopSplits, findingKey, partitionAgainstBaseline) that shipped with zero coverage — CLAUDE.md requires at least one fast-check property test for parsers, included here (bare/braced forms always flagged naming the variable; quoted/substituted/literal forms never are). Also bounds runShellcheck's subprocess with a 60s timeout: the `shellcheck` npm package's own API has no timeout option and internally blocks on a synchronous spawnSync, so this reimplements the binary resolve/download step via the package's own exported config/download and calls spawnSync directly with a native timeout. And guards the CLI entry point with `if (require.main === module)`, matching this repo's sibling dual-purpose lint scripts — without it, requiring the module for its exported functions (as the new test file does) also triggered a live ShellCheck run as a side effect. * docs(#4109): add changeset for the zsh word-splitting fix * chore(#4109): backfill changeset PR number (#4116) --------- Co-authored-by: sim <sim@local>
This commit is contained in:
5
.changeset/sharp-orcas-purr.md
Normal file
5
.changeset/sharp-orcas-purr.md
Normal file
@@ -0,0 +1,5 @@
|
||||
---
|
||||
type: Fixed
|
||||
pr: 4116
|
||||
---
|
||||
**`/gsd:review` no longer misdispatches or undercounts reviewer lanes under zsh** — a shell word-splitting bug collapsed multiple selected reviewers onto one bogus iteration when the workflow's dispatch, gate-check, and plan-coverage logic ran under zsh (the macOS default shell); all affected sites across gsd-core/workflows/*.md are fixed, and a new ShellCheck + structural lint gate catches this bug class in CI going forward. (#4109)
|
||||
@@ -140,7 +140,10 @@ if [ -z "$FILES_OVERRIDE" ]; then
|
||||
REVIEW_FILES=()
|
||||
|
||||
if [ -n "$SUMMARIES" ]; then
|
||||
for summary in $SUMMARIES; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for summary in $(printf '%s' "$SUMMARIES"); do
|
||||
# Extract key_files.created and key_files.modified using node for reliable YAML parsing
|
||||
# This avoids fragile awk parsing that breaks on indentation differences
|
||||
EXTRACTED=$(node -e "
|
||||
|
||||
@@ -775,7 +775,10 @@ CURRENT_BRANCH=$(git branch --show-current)
|
||||
git checkout ${BASE_BRANCH}
|
||||
|
||||
if [ "$BRANCHING_STRATEGY" = "phase" ]; then
|
||||
for branch in $PHASE_BRANCHES; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for branch in $(printf '%s' "$PHASE_BRANCHES"); do
|
||||
git merge --squash "$branch"
|
||||
# Strip .planning/ from staging if commit_docs is false
|
||||
if [ "$COMMIT_DOCS" = "false" ]; then
|
||||
@@ -804,7 +807,10 @@ CURRENT_BRANCH=$(git branch --show-current)
|
||||
git checkout ${BASE_BRANCH}
|
||||
|
||||
if [ "$BRANCHING_STRATEGY" = "phase" ]; then
|
||||
for branch in $PHASE_BRANCHES; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for branch in $(printf '%s' "$PHASE_BRANCHES"); do
|
||||
git merge --no-ff --no-commit "$branch"
|
||||
# Strip .planning/ from staging if commit_docs is false
|
||||
if [ "$COMMIT_DOCS" = "false" ]; then
|
||||
@@ -830,7 +836,10 @@ git checkout "$CURRENT_BRANCH"
|
||||
|
||||
```bash
|
||||
if [ "$BRANCHING_STRATEGY" = "phase" ]; then
|
||||
for branch in $PHASE_BRANCHES; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for branch in $(printf '%s' "$PHASE_BRANCHES"); do
|
||||
git branch -d "$branch" 2>/dev/null || git branch -D "$branch"
|
||||
done
|
||||
fi
|
||||
|
||||
@@ -48,12 +48,18 @@ if [ -n "$SUBMODULE_PATHS" ] && [ "$USE_WORKTREES_FOR_PLAN" != "false" ]; then
|
||||
# submodule "vendor/foo".
|
||||
INTERSECT=""
|
||||
set -f # disable globbing while iterating literal patterns
|
||||
for sm_raw in $SUBMODULE_PATHS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for sm_raw in $(printf '%s' "$SUBMODULE_PATHS"); do
|
||||
# Normalize submodule path: strip ./ prefix and trailing /
|
||||
sm="${sm_raw#./}"
|
||||
sm="${sm%/}"
|
||||
[ -z "$sm" ] && continue
|
||||
for pf_raw in $PLAN_SCOPE_PATHS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for pf_raw in $(printf '%s' "$PLAN_SCOPE_PATHS"); do
|
||||
# Normalize planned path the same way
|
||||
pf="${pf_raw#./}"
|
||||
pf="${pf%/}"
|
||||
|
||||
@@ -261,7 +261,10 @@ if [ "$PR_STRICT" = "true" ]; then
|
||||
FILTER_PATHS=".planning/"
|
||||
FORBIDDEN_RE="^\.planning/"
|
||||
else
|
||||
FILTER_PATHS=$(for d in $TRANSIENT_DIRS; do printf '.planning/%s/ ' "$d"; done)
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
FILTER_PATHS=$(for d in $(printf '%s' "$TRANSIENT_DIRS"); do printf '.planning/%s/ ' "$d"; done)
|
||||
FORBIDDEN_RE="^\.planning/($(echo "$TRANSIENT_DIRS" | tr ' ' '|'))/"
|
||||
fi
|
||||
```
|
||||
@@ -322,12 +325,15 @@ touching the same planning path makes `git cherry-pick` abort with *"untracked w
|
||||
files would be overwritten by merge"*, and every remaining commit is silently dropped.
|
||||
|
||||
```bash
|
||||
for HASH in $INCLUDED_COMMITS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for HASH in $(printf '%s' "$INCLUDED_COMMITS"); do
|
||||
# A modify/delete conflict on a filtered path is EXPECTED and is resolved below — the
|
||||
# filtered path is absent from HEAD by construction. Do not treat it as a failure here.
|
||||
git cherry-pick --no-commit "$HASH" || true
|
||||
|
||||
for P in $FILTER_PATHS; do
|
||||
for P in $(printf '%s' "$FILTER_PATHS"); do
|
||||
git rm -r -f -q --ignore-unmatch -- "$P" 2>/dev/null || true
|
||||
git checkout HEAD -- "$P" 2>/dev/null || true
|
||||
done
|
||||
|
||||
@@ -490,7 +490,14 @@ for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
|
||||
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
|
||||
done
|
||||
|
||||
for SLUG in $DISPATCH_SLUGS; do
|
||||
# Rewrapped through unquoted command substitution, not consumed as a bare
|
||||
# `$DISPATCH_SLUGS`: bash word-splits an unquoted scalar on IFS by default,
|
||||
# but zsh does not, so a bare re-split collapsed every slug onto one
|
||||
# iteration under zsh whenever 2+ reviewers were selected (gsd-core#4109).
|
||||
# Unquoted `$(...)` re-splits identically under both shells regardless of
|
||||
# `SH_WORD_SPLIT` — same reason the accumulator-building loop above already
|
||||
# works under both.
|
||||
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
|
||||
if [ "$PARALLEL_LANES" = "true" ]; then
|
||||
run_review_lane "$SLUG" &
|
||||
else
|
||||
@@ -508,7 +515,9 @@ wait
|
||||
# produces is byte-identical to the one a sequential run produces. This is post-join and therefore
|
||||
# single-threaded, so `>>` here is safe. A lane that was budget-skipped, or that never started,
|
||||
# leaves no result file and correctly contributes no line.
|
||||
for SLUG in $DISPATCH_SLUGS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109) — see the
|
||||
# dispatch loop above for why a bare `$DISPATCH_SLUGS` collapses under zsh.
|
||||
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
|
||||
LANE_RESULT="$RUN_DIR/gsd-review-lane-result-$SLUG.json"
|
||||
if [ -f "$LANE_RESULT" ]; then
|
||||
cat "$LANE_RESULT" >> "$RUN_DIR/gsd-review-lane-results.jsonl"
|
||||
@@ -569,7 +578,10 @@ if [ "${LANE_LINES:-0}" -eq 0 ]; then
|
||||
# failure stub does not. If a slug has no stub at all, it is not a skip.
|
||||
DISPATCHED_COUNT=0
|
||||
SKIPPED_COUNT=0
|
||||
for SLUG in $DISPATCH_SLUGS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$DISPATCH_SLUGS` word-splits under bash but not zsh, collapsing every
|
||||
# slug onto one iteration there whenever 2+ reviewers were selected.
|
||||
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
|
||||
DISPATCHED_COUNT=$((DISPATCHED_COUNT + 1))
|
||||
STUB="$RUN_DIR/gsd-review-$SLUG.md"
|
||||
if [ -f "$STUB" ] && grep -q "review skipped: prompt budget" "$STUB" 2>/dev/null; then
|
||||
@@ -627,7 +639,10 @@ for SLUG in $(echo "$SELECTED_REVIEWERS" | tr ',' ' '); do
|
||||
DISPATCH_SLUGS="$DISPATCH_SLUGS $SLUG"
|
||||
done
|
||||
|
||||
for SLUG in $DISPATCH_SLUGS; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$DISPATCH_SLUGS` word-splits under bash but not zsh, collapsing every
|
||||
# slug onto one iteration there whenever 2+ reviewers were selected.
|
||||
for SLUG in $(printf '%s' "$DISPATCH_SLUGS"); do
|
||||
[ "$SLUG" = "coderabbit" ] && continue
|
||||
REVIEW_FILE="$RUN_DIR/gsd-review-$SLUG.md"
|
||||
[ -f "$REVIEW_FILE" ] || continue
|
||||
|
||||
@@ -237,12 +237,18 @@ mkdir -p "$DEST_ROOT"
|
||||
# empty. If per-runtime conversion is ever wired in, this is where it would go;
|
||||
# until then the cp -r must never run for a destination != source.
|
||||
|
||||
for SKILL in $CREATE_LIST $UPDATE_LIST; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for SKILL in $(printf '%s' "$CREATE_LIST") $(printf '%s' "$UPDATE_LIST"); do
|
||||
rm -rf "$DEST_ROOT/$SKILL"
|
||||
cp -r "$SRC_SKILLS_ROOT/$SKILL" "$DEST_ROOT/$SKILL"
|
||||
done
|
||||
|
||||
for SKILL in $REMOVE_LIST; do
|
||||
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
|
||||
# `$VAR` word-splits under bash but not zsh, collapsing every element onto
|
||||
# one iteration there.
|
||||
for SKILL in $(printf '%s' "$REMOVE_LIST"); do
|
||||
rm -rf "$DEST_ROOT/$SKILL"
|
||||
done
|
||||
```
|
||||
|
||||
1351
package-lock.json
generated
1351
package-lock.json
generated
File diff suppressed because it is too large
Load Diff
@@ -78,6 +78,7 @@
|
||||
"js-yaml": "^4.3.1",
|
||||
"mutation-testing-metrics": "^3.7.3",
|
||||
"re2js": "^2.8.6",
|
||||
"shellcheck": "^4.1.0",
|
||||
"typescript": "^6.0.3",
|
||||
"typescript-eslint": "^8.60.0"
|
||||
},
|
||||
@@ -121,7 +122,7 @@
|
||||
"lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs",
|
||||
"lint:frontmatter-scalar-broad-grep": "node scripts/lint-frontmatter-scalar-broad-grep.cjs",
|
||||
"lint:removed-but-needed": "node scripts/lint-removed-but-needed.cjs",
|
||||
"lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs",
|
||||
"lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-tests.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs && node scripts/lint-milestone-window-drift.cjs && node scripts/lint-phase-enumeration-drift.cjs && node scripts/lint-planning-prompt-drift.cjs && node scripts/lint-unreachable-guard-drift.cjs && node scripts/lint-completion-ratio-drift.cjs && node scripts/lint-slug-derivation-drift.cjs && node scripts/lint-state-field-drift.cjs && node scripts/lint-state-write-path-drift.cjs && node scripts/lint-completion-predicate-drift.cjs && node scripts/lint-planning-snapshot-bypass-drift.cjs && node scripts/lint-health-diagnostic-rule-table.cjs && node scripts/lint-planning-artifact-writer-drift.cjs && node scripts/lint-frontmatter-scalar-broad-grep.cjs && node scripts/lint-removed-but-needed.cjs && node scripts/lint-no-adhoc-regex-escape.cjs && node scripts/lint-vendored-deps.cjs && node scripts/lint-docs-guard-registration.cjs && node scripts/lint-source-test-name-collision.cjs && npm run lint:hooks-runtime-build-seam && node scripts/check-contract-drift.cjs && node scripts/lint-mutation-test-derivation-drift.cjs && node scripts/lint-seam-enforcement.cjs && node scripts/lint-workflow-shellcheck.cjs",
|
||||
"lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs",
|
||||
"lint:regression-names": "node scripts/lint-regression-test-names.cjs",
|
||||
"lint:descriptions": "node scripts/lint-descriptions.cjs",
|
||||
|
||||
1062
scripts/lint-workflow-shellcheck-baseline.json
Normal file
1062
scripts/lint-workflow-shellcheck-baseline.json
Normal file
File diff suppressed because it is too large
Load Diff
652
scripts/lint-workflow-shellcheck.cjs
Normal file
652
scripts/lint-workflow-shellcheck.cjs
Normal file
@@ -0,0 +1,652 @@
|
||||
#!/usr/bin/env node
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* lint-workflow-shellcheck.cjs
|
||||
*
|
||||
* Systemic prevention for the zsh/bash word-splitting bug class (#4109):
|
||||
* every ```bash fenced block embedded in gsd-core/workflows/*.md (and the
|
||||
* nested gsd-core/workflows/<workflow>/steps/*.md / modes/*.md / etc. layer)
|
||||
* is extracted and run through the real ShellCheck binary. Any finding fails
|
||||
* the lint with a non-zero exit — this is what stops the SC2086-class bug
|
||||
* (unquoted variable expansion, word-split/glob differently under zsh vs
|
||||
* bash) from landing undetected a second time (it already landed 4 times in
|
||||
* this repo's workflow templates before #4109's fix).
|
||||
*
|
||||
* ShellCheck source: the `shellcheck` npm package (gunar/shellcheck), a thin
|
||||
* wrapper that downloads the official koalaman/shellcheck binary on first
|
||||
* use and caches it under node_modules/shellcheck/bin/. Chosen over the
|
||||
* alternatives surveyed (node-shellcheck: ~5 weekly downloads, last
|
||||
* published 2022; shellcheck-binaries: ~280 weekly downloads, last
|
||||
* published 2022) because it has ~80k weekly downloads and is the
|
||||
* only actively-maintained wrapper — it downloads the CURRENT upstream
|
||||
* ShellCheck release rather than vendoring a stale binary snapshot.
|
||||
*
|
||||
* Extraction: reuses scanFencedBlocks from markdown-sectionizer.cts (the
|
||||
* canonical fence-scanning engine — see tests/review-plan-coverage-manifest
|
||||
* .test.cjs's extractAllBashBlocks for the precedent this follows) rather
|
||||
* than a bespoke regex.
|
||||
*
|
||||
* Placeholder handling: workflow blocks reference template placeholders —
|
||||
* both single-token (`{run_dir}`, `{N}`) and multi-word prose (`{discovered
|
||||
* test command}`, `{each unique directory from resolved paths}`) — that are
|
||||
* not valid shell and would misparse as ShellCheck syntax errors unrelated to
|
||||
* the word-splitting class this lint targets. Every such placeholder (NOT
|
||||
* `${identifier}`, which is a real parameter expansion, and NOT real brace
|
||||
* syntax like `{1..5}`/`{a,b,c}`/`{ cmd; }` — see `substitutePlaceholders`'s
|
||||
* own comment for the exact discriminating rule) is substituted with a
|
||||
* shell-safe bareword before staging, generalizing the test harness's
|
||||
* single-placeholder `body.split('{run_dir}').join(runDir)` substitution to
|
||||
* the general case.
|
||||
*
|
||||
* Rule selection (documented per the brief's requirement to justify the
|
||||
* include/exclude choice):
|
||||
* - SC2086 (double-quote to prevent globbing/word splitting) is the exact
|
||||
* bug class #4109 fixes and MUST be enabled — it is ShellCheck's default
|
||||
* behavior and is never excluded here.
|
||||
* - The rest of ShellCheck's DEFAULT rule set is also left enabled: most of
|
||||
* it (SC2046, SC2068, SC2145, SC2206, SC2207, etc.) is the SAME
|
||||
* quoting/word-splitting/array-expansion family SC2086 belongs to, and is
|
||||
* exactly the kind of finding this lint exists to catch.
|
||||
* - Three codes are explicitly EXCLUDED because they produce structural
|
||||
* false positives in this templated, cross-block, agent-populated
|
||||
* context rather than real defects:
|
||||
* SC1091 — "not following sourced file": blocks `source`/`.` files
|
||||
* that exist only at run time in the calling agent's real RUN_DIR, not
|
||||
* in this lint's throwaway single-block temp file.
|
||||
* SC2154 — "var is referenced but not assigned": workflow blocks
|
||||
* routinely reference variables the CALLING AGENT exports as env vars,
|
||||
* or that a DIFFERENT fenced block earlier in the same workflow
|
||||
* assigned — invisible to a scan of one isolated block.
|
||||
* SC2034 — "var appears unused": the mirror image of SC2154 — a var
|
||||
* assigned in this block is frequently consumed by a LATER block in
|
||||
* the same workflow, again invisible to a single-block scan.
|
||||
* SC2148 ("shell directive missing") is not in this exclude list because
|
||||
* passing `--shell=bash` to ShellCheck (all these blocks are already
|
||||
* fenced ```bash, i.e. self-declared) prevents it from firing at all.
|
||||
*
|
||||
* Exit 0 with no output on a clean tree (or a tree whose only findings are
|
||||
* already accepted in the baseline, see below); exit 1 with every NEW
|
||||
* finding (file, line, ShellCheck code, message) printed to stderr otherwise.
|
||||
*
|
||||
* Baseline (pre-existing findings, #4109 follow-up):
|
||||
* Landing this lint against the real repo surfaced ~212 pre-existing
|
||||
* ShellCheck findings across ~60 files that are unrelated to #4109's actual
|
||||
* fix (a zsh word-splitting bug already fixed at its 6 sites). Requiring all
|
||||
* 212 to be fixed in the same PR that adds the lint would block CI for
|
||||
* reasons orthogonal to the issue. Instead, `scripts/lint-workflow-
|
||||
* shellcheck-baseline.json` records the *accepted* pre-existing findings as
|
||||
* of the baseline's generation, and this script only fails on findings NOT
|
||||
* present in that baseline ("new" findings) — a standard ratchet: today's
|
||||
* findings can never silently grow, but paying down the backlog is a
|
||||
* separate, incremental effort.
|
||||
*
|
||||
* Baseline shape: a flat JSON array of `{file, code, message}` triples (see
|
||||
* BASELINE_PATH below). `file` is the workflow-relative path (matches a
|
||||
* finding's mapped `block.file`), `code` is the bare ShellCheck code number
|
||||
* (e.g. `"2086"`, matches `f.code`), `message` is ShellCheck's finding text
|
||||
* verbatim (matches `f.message`).
|
||||
*
|
||||
* Matching strategy — deliberately EXCLUDES line/column: matching on exact
|
||||
* line number would make the baseline brittle to totally unrelated edits.
|
||||
* E.g. inserting one line near the top of a large workflow file shifts every
|
||||
* subsequent line number, which would make every already-accepted finding
|
||||
* below that point look "new" on the next lint run — a spurious CI failure
|
||||
* with no relationship to any real regression. `{file, code, message}` is
|
||||
* stable under such reflow: the finding's identity (what rule fired, what it
|
||||
* says, which file) doesn't move just because line numbers shift.
|
||||
*
|
||||
* This does mean two textually-identical findings in the same file (same
|
||||
* code, same message) are indistinguishable by key alone. Findings are
|
||||
* matched as a MULTISET, not a set: the baseline is loaded into a
|
||||
* `key -> count` map, and each current finding consumes one count of its key
|
||||
* if available (marking it "baselined") or is reported "new" once the
|
||||
* baseline's count for that key is exhausted. This preserves ratchet
|
||||
* semantics per-file-per-rule-per-message (a THIRD occurrence of a message
|
||||
* that only had two accepted instances IS reported as new) without being
|
||||
* sensitive to which physical line within the file each occurrence sits on.
|
||||
*/
|
||||
|
||||
const fs = require('node:fs');
|
||||
const os = require('node:os');
|
||||
const path = require('node:path');
|
||||
const childProcess = require('node:child_process');
|
||||
const { ExitError, runMain } = require('./lib/cli-exit.cjs');
|
||||
|
||||
// Hard bound on the ShellCheck binary's run time, matching this repo's
|
||||
// npm-subprocess timeout convention (5-30s git, 60s npm — same "external
|
||||
// process that could hang" hazard class). See runShellcheck's comment for
|
||||
// why this cannot be applied via the `shellcheck` npm package's own API.
|
||||
const SHELLCHECK_TIMEOUT_MS = 60_000;
|
||||
|
||||
const ROOT = path.join(__dirname, '..');
|
||||
const WORKFLOWS_DIR = path.join(ROOT, 'gsd-core', 'workflows');
|
||||
const SECTIONIZER_PATH = path.join(ROOT, 'gsd-core', 'bin', 'lib', 'markdown-sectionizer.cjs');
|
||||
const SHELLCHECK_BIN_MODULE = path.join(ROOT, 'node_modules', 'shellcheck', 'build', 'index.js');
|
||||
// The top-level SHELLCHECK_BIN_MODULE barrel (build/index.js) does NOT
|
||||
// re-export `configs/index.js` (verified: `export *`-ing helpers/logger/
|
||||
// utils/shellcheck.js only — no configs), so `config` (which carries the
|
||||
// resolved binary path used by runShellcheck's own spawnSync call, see
|
||||
// below) has to be imported from its own submodule directly.
|
||||
const SHELLCHECK_CONFIG_MODULE = path.join(ROOT, 'node_modules', 'shellcheck', 'build', 'configs', 'index.js');
|
||||
const BASELINE_PATH = path.join(__dirname, 'lint-workflow-shellcheck-baseline.json');
|
||||
|
||||
// Codes excluded for structural reasons documented in the module header above.
|
||||
const EXCLUDED_CODES = ['SC1091', 'SC2154', 'SC2034'];
|
||||
|
||||
/** Every bare `{identifier}` (not `${identifier}`) → a shell-safe bareword. */
|
||||
function substitutePlaceholders(body) {
|
||||
// Only matches content that is ALREADY known-safe to be workflow-template
|
||||
// prose: starts with a letter, then nothing but letters/digits/underscore/
|
||||
// hyphen/space. This deliberately excludes every real shell use of `{...}`
|
||||
// that could otherwise collide with a placeholder-shaped token:
|
||||
// - `${var}` parameter expansion — excluded by the `(?<!\$)` lookbehind.
|
||||
// - `{1..5}` / `{01..10}` numeric ranges — digit-first or contain `.`.
|
||||
// - `{a,b,c}` brace-expansion lists — contain `,`.
|
||||
// - `{ cmd; }` / `{ cmd1; cmd2; }` compound-command grouping — POSIX
|
||||
// requires whitespace immediately after the opening `{` (it is only a
|
||||
// reserved word when blank-separated), and the body always carries a
|
||||
// `;`/pipe/redirect/quote — none of which this charset admits, so a
|
||||
// real command group can never match this regex.
|
||||
// - JSON-shaped literals like `{"key": "value"}` — contain `"`/`:`.
|
||||
// Everything workflow authors actually use as a template placeholder in
|
||||
// this repo (`{run_dir}`, `{N}`, `{discovered test command}`, `{scenario
|
||||
// keyword}`, `{expected}`, `{implementation file}`, …) is pure prose text
|
||||
// and matches; nothing else does.
|
||||
return body.replace(/(?<!\$)\{([A-Za-z][A-Za-z0-9_ -]*)\}/g, (match, inner) => {
|
||||
const safe = inner.trim().replace(/[^a-zA-Z0-9_]+/g, '_').replace(/^_+|_+$/g, '') || 'X';
|
||||
return `PLACEHOLDER_${safe}`;
|
||||
});
|
||||
}
|
||||
|
||||
/** Recursively collect every `.md` file under `dir`. */
|
||||
function collectMarkdownFiles(dir) {
|
||||
const out = [];
|
||||
for (const entry of fs.readdirSync(dir, { withFileTypes: true, recursive: true })) {
|
||||
if (!entry.isFile() || !entry.name.endsWith('.md')) continue;
|
||||
// Node's recursive readdir sets entry.parentPath (>=20.12) / entry.path (older).
|
||||
const parent = entry.parentPath ?? entry.path;
|
||||
out.push(path.join(parent, entry.name));
|
||||
}
|
||||
return out.sort();
|
||||
}
|
||||
|
||||
/**
|
||||
* Every ```bash fenced block across every workflow .md file, with enough
|
||||
* metadata to map a ShellCheck finding back to its original source location.
|
||||
*/
|
||||
function extractBashBlocks(sectionizer) {
|
||||
const files = collectMarkdownFiles(WORKFLOWS_DIR);
|
||||
const blocks = [];
|
||||
for (const file of files) {
|
||||
const content = fs.readFileSync(file, 'utf8');
|
||||
const lines = content.split(/\r?\n/);
|
||||
const relFile = path.relative(ROOT, file);
|
||||
const fenced = sectionizer.scanFencedBlocks(lines);
|
||||
let blockIdx = 0;
|
||||
for (const b of fenced) {
|
||||
if (b.closeLineIdx === -1) continue; // unterminated fence — nothing well-defined to check
|
||||
if ((b.infoString || '').trim() !== 'bash') continue;
|
||||
const body = lines.slice(b.openLineIdx + 1, b.closeLineIdx).join('\n');
|
||||
blocks.push({
|
||||
file: relFile,
|
||||
blockIdx: blockIdx++,
|
||||
// 1-based source line of the FIRST body line — a JSON finding's own
|
||||
// `line` (1-based, relative to the staged single-block temp file) is
|
||||
// added to this minus 1 to recover the real workflow-file line.
|
||||
firstBodyLine: b.openLineIdx + 2,
|
||||
body,
|
||||
});
|
||||
}
|
||||
}
|
||||
return blocks;
|
||||
}
|
||||
|
||||
/** Stable identity key for a mapped finding — see the "Matching strategy" note above. */
|
||||
function findingKey(f) {
|
||||
return `${f.file} ${f.code} ${f.message}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Structural check (separate from the ShellCheck pass above): catches the
|
||||
* exact #4109 bug shape — `for x in $VAR; do` / `for x in ${VAR}; do` with a
|
||||
* BARE, unquoted scalar variable reference in the for-list position.
|
||||
*
|
||||
* ShellCheck does NOT flag this pattern under any ruleset, confirmed
|
||||
* empirically by reintroducing the exact bug and running this script's own
|
||||
* ShellCheck invocation (including `--enable=all`): a bare `$VAR` directly in
|
||||
* a for-list is a deliberately-accepted, common bash idiom to ShellCheck, so
|
||||
* SC2086 and friends never fire on it. That idiom is exactly what silently
|
||||
* diverges between bash (word-splits it) and zsh (does not) — the root cause
|
||||
* of #4109. Hence this dedicated structural pass, run in the SAME invocation
|
||||
* as the ShellCheck pass, over the SAME extracted ```bash blocks.
|
||||
*
|
||||
* Algorithm per for-loop found in a block body:
|
||||
* 1. Locate `for <ident> in <list-expr>` and capture <list-expr> up to the
|
||||
* first `;` or newline that is NOT nested inside a `$( ... )` span (a
|
||||
* paren-depth scan, not a naive `[^;]*` regex slice) — a for-list that
|
||||
* itself contains a `;` inside a command substitution must not have its
|
||||
* capture truncated early.
|
||||
* 2. Strip every `$( ... )` command-substitution span out of <list-expr>.
|
||||
* Command substitution ALWAYS word-splits its result in both bash AND
|
||||
* zsh — that is the actual #4109 fix pattern applied at every known
|
||||
* site (`$(printf '%s' "$VAR")`), so a bare `$VAR` INSIDE a `$(...)`
|
||||
* span is safe and must never be flagged.
|
||||
* 3. Search what remains for a bare `$IDENT` / `${IDENT}` that is NOT
|
||||
* immediately preceded by `"` — a `"$VAR"` reference is a different,
|
||||
* also-safe idiom (single-token literal-list iteration), not the
|
||||
* splitting bug.
|
||||
*
|
||||
* Findings from this pass are NEVER baselined (unlike the ShellCheck pass) —
|
||||
* this check is new-by-construction and every workflow site known to be
|
||||
* vulnerable was already swept as part of #4109's fix, so any finding here
|
||||
* is a genuinely new/missed site worth surfacing distinctly rather than
|
||||
* silently absorbing into scripts/lint-workflow-shellcheck-baseline.json.
|
||||
*/
|
||||
|
||||
/** Strip every balanced `$( ... )` span from `text`, preserving everything else. */
|
||||
function stripCommandSubstitutions(text) {
|
||||
let out = '';
|
||||
let i = 0;
|
||||
while (i < text.length) {
|
||||
if (text[i] === '$' && text[i + 1] === '(') {
|
||||
let depth = 1;
|
||||
let j = i + 2;
|
||||
while (j < text.length && depth > 0) {
|
||||
if (text[j] === '(') depth++;
|
||||
else if (text[j] === ')') depth--;
|
||||
j++;
|
||||
}
|
||||
i = j;
|
||||
continue;
|
||||
}
|
||||
out += text[i];
|
||||
i++;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
// A bare `$IDENT` / `${IDENT}` not immediately preceded by `"`.
|
||||
const BARE_VAR_RE = /(^|[^"])\$\{?([A-Za-z_][A-Za-z0-9_]*)\}?/;
|
||||
|
||||
/**
|
||||
* Blank out `# ...` shell comments (to end of line), preserving every other
|
||||
* character's position 1:1 (comment text is replaced with spaces, newlines
|
||||
* are kept) so downstream character-offset -> line-number mapping stays
|
||||
* valid without needing a second pass. A `#` only starts a comment when it
|
||||
* is the first character of a "word" (start of line, or preceded by
|
||||
* whitespace) — matching real shell comment semantics and, deliberately,
|
||||
* NOT stripping `${VAR#pattern}` parameter-expansion `#`s (always preceded
|
||||
* by a non-whitespace identifier character, e.g. `${sm_raw#./}`). Prose
|
||||
* inside a `#` comment (e.g. a changelog note quoting `for x in $VAR` as an
|
||||
* example of a PAST bug) must never be mistaken for live code — this is
|
||||
* what stops that false positive.
|
||||
*/
|
||||
function stripShellComments(body) {
|
||||
let out = '';
|
||||
let inSingle = false;
|
||||
let inDouble = false;
|
||||
let i = 0;
|
||||
while (i < body.length) {
|
||||
const ch = body[i];
|
||||
if (inSingle) {
|
||||
out += ch;
|
||||
if (ch === "'") inSingle = false;
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (inDouble) {
|
||||
out += ch;
|
||||
if (ch === '"') inDouble = false;
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (ch === "'") {
|
||||
inSingle = true;
|
||||
out += ch;
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
if (ch === '"') {
|
||||
inDouble = true;
|
||||
out += ch;
|
||||
i++;
|
||||
continue;
|
||||
}
|
||||
const prev = i === 0 ? '\n' : body[i - 1];
|
||||
if (ch === '#' && /\s/.test(prev)) {
|
||||
while (i < body.length && body[i] !== '\n') {
|
||||
out += ' ';
|
||||
i++;
|
||||
}
|
||||
continue; // the '\n' itself (if any) is handled by the next loop iteration
|
||||
}
|
||||
out += ch;
|
||||
i++;
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
/**
|
||||
* Every `for <ident> in <list-expr>` for-loop header in `body`, with the raw
|
||||
* list-expression text and the 0-based character offset of the `for` keyword
|
||||
* (used by the caller to recover a line number).
|
||||
*/
|
||||
function extractForLoops(body) {
|
||||
const results = [];
|
||||
const headerRe = /\bfor\s+([A-Za-z_][A-Za-z0-9_]*)\s+in\s+/g;
|
||||
let m;
|
||||
while ((m = headerRe.exec(body)) !== null) {
|
||||
const start = headerRe.lastIndex;
|
||||
let i = start;
|
||||
let depth = 0;
|
||||
while (i < body.length) {
|
||||
const ch = body[i];
|
||||
if (ch === '(') depth++;
|
||||
else if (ch === ')') depth--;
|
||||
else if (depth === 0 && (ch === ';' || ch === '\n')) break;
|
||||
i++;
|
||||
}
|
||||
results.push({ loopVar: m[1], listExpr: body.slice(start, i), matchIndex: m.index });
|
||||
headerRe.lastIndex = i;
|
||||
}
|
||||
return results;
|
||||
}
|
||||
|
||||
/** 1-based line number of `charIndex` within `body` (0-based first line = 1). */
|
||||
function lineOffsetOf(body, charIndex) {
|
||||
let line = 1;
|
||||
for (let i = 0; i < charIndex && i < body.length; i++) {
|
||||
if (body[i] === '\n') line++;
|
||||
}
|
||||
return line;
|
||||
}
|
||||
|
||||
/**
|
||||
* Scan every extracted block for the bare unquoted `for x in $VAR` shape.
|
||||
* Returns mapped findings (`{file, line, loopVar, varName, listExpr,
|
||||
* blockIdx}`), analogous in shape to the ShellCheck findings above but never
|
||||
* baselined — see this section's header note.
|
||||
*/
|
||||
function findBareForLoopSplits(blocks) {
|
||||
const findings = [];
|
||||
for (const block of blocks) {
|
||||
const codeOnly = stripShellComments(block.body);
|
||||
for (const loop of extractForLoops(codeOnly)) {
|
||||
const stripped = stripCommandSubstitutions(loop.listExpr);
|
||||
const bare = BARE_VAR_RE.exec(stripped);
|
||||
if (!bare) continue;
|
||||
findings.push({
|
||||
file: block.file,
|
||||
line: block.firstBodyLine + lineOffsetOf(block.body, loop.matchIndex) - 1,
|
||||
blockIdx: block.blockIdx,
|
||||
loopVar: loop.loopVar,
|
||||
varName: bare[2],
|
||||
listExpr: loop.listExpr.trim(),
|
||||
});
|
||||
}
|
||||
}
|
||||
return findings;
|
||||
}
|
||||
|
||||
/** Load the baseline array (empty if the file does not exist yet). */
|
||||
function loadBaseline() {
|
||||
if (!fs.existsSync(BASELINE_PATH)) return [];
|
||||
const raw = fs.readFileSync(BASELINE_PATH, 'utf8');
|
||||
const parsed = JSON.parse(raw);
|
||||
if (!Array.isArray(parsed)) {
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: ${path.relative(ROOT, BASELINE_PATH)} must be a JSON array of ` +
|
||||
`{file, code, message} objects.`,
|
||||
);
|
||||
}
|
||||
return parsed;
|
||||
}
|
||||
|
||||
/**
|
||||
* Partition `mappedFindings` (each `{file, code, message, ...}`) into
|
||||
* `{newFindings, baselinedFindings}` against the baseline multiset. See the
|
||||
* "Matching strategy" note in the module header for why this is a
|
||||
* key -> count multiset match rather than exact-line matching.
|
||||
*/
|
||||
function partitionAgainstBaseline(mappedFindings, baseline) {
|
||||
const remaining = new Map();
|
||||
for (const entry of baseline) {
|
||||
const key = findingKey(entry);
|
||||
remaining.set(key, (remaining.get(key) || 0) + 1);
|
||||
}
|
||||
const newFindings = [];
|
||||
const baselinedFindings = [];
|
||||
for (const f of mappedFindings) {
|
||||
const key = findingKey(f);
|
||||
const count = remaining.get(key) || 0;
|
||||
if (count > 0) {
|
||||
remaining.set(key, count - 1);
|
||||
baselinedFindings.push(f);
|
||||
} else {
|
||||
newFindings.push(f);
|
||||
}
|
||||
}
|
||||
return { newFindings, baselinedFindings };
|
||||
}
|
||||
|
||||
function loadSectionizer() {
|
||||
try {
|
||||
return require(SECTIONIZER_PATH);
|
||||
} catch (e) {
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: cannot load the markdown-sectionizer seam at ` +
|
||||
`${path.relative(ROOT, SECTIONIZER_PATH)} — run 'npm run build:lib' first (${e.message})`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/** Load the `shellcheck` npm package's programmatic API — its own `shellcheck()`
|
||||
* function transparently downloads the real binary to node_modules/shellcheck/
|
||||
* bin/shellcheck (caching it there) on first use if it is not already present. */
|
||||
async function loadShellcheckModule() {
|
||||
try {
|
||||
const mod = await import(SHELLCHECK_BIN_MODULE);
|
||||
// See SHELLCHECK_CONFIG_MODULE's comment above — `config` is not part of
|
||||
// the top-level barrel's exports, so it is imported separately and
|
||||
// attached here for runShellcheck's direct spawnSync call to consume.
|
||||
const { config } = await import(SHELLCHECK_CONFIG_MODULE);
|
||||
return { ...mod, config };
|
||||
} catch (e) {
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: cannot load the 'shellcheck' npm package at ` +
|
||||
`${path.relative(ROOT, SHELLCHECK_BIN_MODULE)} — run 'npm install' first (${e.message})`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Run ShellCheck (json1 output) over every staged temp file in one invocation,
|
||||
* bounded by SHELLCHECK_TIMEOUT_MS.
|
||||
*
|
||||
* The `shellcheck` npm package's own `shellcheck()` function does NOT accept a
|
||||
* `timeout` — its `ShellCheckArgs` type is `{bin, args, stdio, token}` only
|
||||
* (verified against node_modules/shellcheck/build/shellcheck.d.ts and .js),
|
||||
* and internally it hardcodes `child_process.spawnSync(opts.bin, opts.args, {
|
||||
* stdio: opts.stdio })` with no pass-through for extra spawnSync options.
|
||||
* Wrapping the call in `Promise.race` against a timer would not help either:
|
||||
* spawnSync is synchronous and blocks the event loop for its whole duration,
|
||||
* so a timer callback racing it can never fire before it returns (or hangs).
|
||||
* Instead, this reimplements the same binary-resolve-and-download step the
|
||||
* wrapper performs (via the package's own exported `config`/`download`), then
|
||||
* invokes `child_process.spawnSync` directly with a native `timeout` so a
|
||||
* hung ShellCheck binary is killed (Node sets `result.error.code ===
|
||||
* 'ETIMEDOUT'` and `result.signal` on expiry) rather than hanging this lint —
|
||||
* and, transitively, CI — indefinitely.
|
||||
*/
|
||||
async function runShellcheck(mod, filePaths) {
|
||||
const args = [
|
||||
'--shell=bash',
|
||||
'--format=json1',
|
||||
`--exclude=${EXCLUDED_CODES.join(',')}`,
|
||||
...filePaths,
|
||||
];
|
||||
const bin = mod.config.bin;
|
||||
try {
|
||||
fs.accessSync(bin, fs.constants.F_OK | fs.constants.X_OK);
|
||||
} catch {
|
||||
await mod.download({ destination: bin, token: process.env.GITHUB_TOKEN });
|
||||
}
|
||||
const result = childProcess.spawnSync(bin, args, { stdio: 'pipe', timeout: SHELLCHECK_TIMEOUT_MS });
|
||||
if (result.error) {
|
||||
const timedOut = result.error.code === 'ETIMEDOUT';
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: ShellCheck invocation ${
|
||||
timedOut ? `timed out after ${SHELLCHECK_TIMEOUT_MS}ms` : 'failed'
|
||||
}: ${result.error.message}`,
|
||||
);
|
||||
}
|
||||
const stdout = Buffer.isBuffer(result.stdout) ? result.stdout.toString('utf8') : (result.stdout || '');
|
||||
if (stdout.trim() === '') {
|
||||
// ShellCheck produced no output at all — genuine infra failure (crash,
|
||||
// bad binary, etc.), not "zero findings" (which is `{"comments":[]}`).
|
||||
const stderr = Buffer.isBuffer(result.stderr) ? result.stderr.toString('utf8') : (result.stderr || '');
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: ShellCheck produced no output (exit ${result.status}). stderr: ${stderr}`,
|
||||
);
|
||||
}
|
||||
try {
|
||||
return JSON.parse(stdout).comments || [];
|
||||
} catch (e) {
|
||||
throw new ExitError(
|
||||
1,
|
||||
`lint-workflow-shellcheck: could not parse ShellCheck json1 output: ${e.message}\n${stdout}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async function main() {
|
||||
const sectionizer = loadSectionizer();
|
||||
const blocks = extractBashBlocks(sectionizer);
|
||||
|
||||
if (blocks.length === 0) {
|
||||
process.stdout.write('ok lint-workflow-shellcheck: no ```bash blocks found under gsd-core/workflows/\n');
|
||||
return 0;
|
||||
}
|
||||
|
||||
// Structural pass (see findBareForLoopSplits's header comment) — runs
|
||||
// independently of ShellCheck. As of the #4109 sweep, every previously
|
||||
// KNOWN site is fixed (0 structural findings on a clean tree), so this now
|
||||
// GATES the exit code exactly like the ShellCheck-baseline-diff check
|
||||
// below: a non-empty structuralFindings fails main() even if ShellCheck
|
||||
// itself reports nothing new. Every finding is printed prominently below
|
||||
// regardless of outcome; the two checks are combined into one final exit
|
||||
// decision so a run with both kinds of findings reports both.
|
||||
const structuralFindings = findBareForLoopSplits(blocks);
|
||||
if (structuralFindings.length > 0) {
|
||||
process.stdout.write(
|
||||
`\nSTRUCTURAL FINDING (not ShellCheck, not baselined) lint-workflow-shellcheck: ` +
|
||||
`${structuralFindings.length} bare unquoted \`for x in $VAR\` for-loop(s) — the #4109 bash/zsh ` +
|
||||
`word-splitting bug shape ShellCheck itself does not detect:\n\n`,
|
||||
);
|
||||
for (const f of structuralFindings) {
|
||||
process.stdout.write(
|
||||
` ${f.file}:${f.line} (block #${f.blockIdx}) — ` +
|
||||
`for ${f.loopVar} in ${f.listExpr} — bare $${f.varName} is unquoted and not inside $(...); ` +
|
||||
`wrap in $(printf '%s' "$${f.varName}") to split identically under bash and zsh.\n`,
|
||||
);
|
||||
}
|
||||
process.stdout.write('\n');
|
||||
}
|
||||
const structuralFailed = structuralFindings.length > 0;
|
||||
|
||||
const shellcheckModule = await loadShellcheckModule();
|
||||
|
||||
const stageDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-workflow-shellcheck-'));
|
||||
try {
|
||||
const stagedPaths = [];
|
||||
const byPath = new Map();
|
||||
blocks.forEach((block, i) => {
|
||||
const scriptPath = path.join(stageDir, `block-${i}.sh`);
|
||||
fs.writeFileSync(scriptPath, substitutePlaceholders(block.body));
|
||||
stagedPaths.push(scriptPath);
|
||||
byPath.set(scriptPath, block);
|
||||
});
|
||||
|
||||
const findings = await runShellcheck(shellcheckModule, stagedPaths);
|
||||
|
||||
if (findings.length === 0) {
|
||||
process.stdout.write(
|
||||
`ok lint-workflow-shellcheck: ${blocks.length} \`\`\`bash block(s) across ` +
|
||||
`${new Set(blocks.map((b) => b.file)).size} workflow file(s) checked, 0 ShellCheck findings\n`,
|
||||
);
|
||||
return structuralFailed ? 1 : 0;
|
||||
}
|
||||
|
||||
const mappedFindings = findings.map((f) => {
|
||||
const block = byPath.get(f.file);
|
||||
return {
|
||||
file: block ? block.file : f.file,
|
||||
line: block ? block.firstBodyLine + f.line - 1 : f.line,
|
||||
column: f.column,
|
||||
code: String(f.code),
|
||||
level: f.level,
|
||||
message: f.message,
|
||||
blockIdx: block ? block.blockIdx : undefined,
|
||||
};
|
||||
});
|
||||
|
||||
const baseline = loadBaseline();
|
||||
const { newFindings, baselinedFindings } = partitionAgainstBaseline(mappedFindings, baseline);
|
||||
|
||||
if (newFindings.length === 0) {
|
||||
process.stdout.write(
|
||||
`ok lint-workflow-shellcheck: ${baselinedFindings.length} pre-existing finding(s) from baseline, ` +
|
||||
`0 new\n`,
|
||||
);
|
||||
return structuralFailed ? 1 : 0;
|
||||
}
|
||||
|
||||
process.stderr.write(
|
||||
`\nERROR lint-workflow-shellcheck: ${newFindings.length} NEW ShellCheck finding(s) in ` +
|
||||
`gsd-core/workflows/ \`\`\`bash block(s) (#4109 word-splitting/quoting prevention) not present in ` +
|
||||
`${path.relative(ROOT, BASELINE_PATH)}.\n\n`,
|
||||
);
|
||||
for (const f of newFindings) {
|
||||
const loc = f.blockIdx !== undefined
|
||||
? `${f.file} (block #${f.blockIdx}, line ${f.line}, col ${f.column})`
|
||||
: `${f.file}:${f.line}:${f.column}`;
|
||||
process.stderr.write(` ${loc} — SC${f.code} (${f.level}): ${f.message}\n`);
|
||||
}
|
||||
if (baselinedFindings.length > 0) {
|
||||
process.stderr.write(`\n(${baselinedFindings.length} other pre-existing finding(s) from baseline, not shown.)\n`);
|
||||
}
|
||||
process.stderr.write('\n');
|
||||
return 1;
|
||||
} finally {
|
||||
fs.rmSync(stageDir, { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
// Guarded so requiring this module (e.g. from tests/lint-workflow-shellcheck
|
||||
// .test.cjs, to exercise the exported pure parser/logic functions) does not
|
||||
// ALSO trigger a full ShellCheck run as an unwanted side effect of require()
|
||||
// — matches the established convention in this repo's other dual-purpose
|
||||
// script+module lint scripts, e.g. scripts/lint-docs-required.cjs's own
|
||||
// `if (require.main === module) runMain(main);`.
|
||||
if (require.main === module) runMain(main);
|
||||
|
||||
module.exports = {
|
||||
substitutePlaceholders,
|
||||
collectMarkdownFiles,
|
||||
extractBashBlocks,
|
||||
EXCLUDED_CODES,
|
||||
findingKey,
|
||||
loadBaseline,
|
||||
partitionAgainstBaseline,
|
||||
BASELINE_PATH,
|
||||
stripCommandSubstitutions,
|
||||
stripShellComments,
|
||||
extractForLoops,
|
||||
findBareForLoopSplits,
|
||||
};
|
||||
@@ -85,7 +85,7 @@ const readWorkflow = () => parseWorkflow(fs.readFileSync(WORKFLOW_PATH, 'utf-8')
|
||||
|
||||
const BASH_FENCE_OPEN_RE = /^```bash\s*$/;
|
||||
const BASH_FENCE_CLOSE_RE = /^```\s*$/;
|
||||
const PICK_LOOP_MARKER = 'for HASH in $INCLUDED_COMMITS';
|
||||
const PICK_LOOP_MARKER = 'for HASH in $(printf \'%s\' "$INCLUDED_COMMITS")';
|
||||
|
||||
// Scans `text` for fenced ```bash blocks and returns the verbatim body
|
||||
// (fence markers stripped, lines rejoined with '\n') of the single block
|
||||
@@ -119,7 +119,7 @@ const extractPickLoop = (text) => {
|
||||
}
|
||||
|
||||
if (matches.length === 0) {
|
||||
throw new Error('pr-branch.md: no create_pr_branch cherry-pick loop found (expected a bash block containing "for HASH in $INCLUDED_COMMITS")');
|
||||
throw new Error(`pr-branch.md: no create_pr_branch cherry-pick loop found (expected a bash block containing "${PICK_LOOP_MARKER}")`);
|
||||
}
|
||||
if (matches.length > 1) {
|
||||
throw new Error(`pr-branch.md: cherry-pick loop found in ${matches.length} bash blocks — the recipe must have exactly one canonical form`);
|
||||
|
||||
277
tests/lint-workflow-shellcheck.test.cjs
Normal file
277
tests/lint-workflow-shellcheck.test.cjs
Normal file
@@ -0,0 +1,277 @@
|
||||
'use strict';
|
||||
|
||||
/**
|
||||
* lint-workflow-shellcheck.test.cjs — unit + property coverage for the
|
||||
* hand-rolled shell-text parser/logic functions exported by
|
||||
* scripts/lint-workflow-shellcheck.cjs (#4109 follow-up).
|
||||
*
|
||||
* Per this repo's CLAUDE.md: "Parsers, budget limits, and bijective
|
||||
* contracts must include at least one fast-check (`fc`) property test."
|
||||
* These functions ARE parsers (shell-text extraction/transformation over
|
||||
* workflow-authored markdown), so the property-test requirement below is a
|
||||
* binding gate, not optional polish.
|
||||
*
|
||||
* Note: scripts/lint-workflow-shellcheck.cjs guards its CLI entry point
|
||||
* with `if (require.main === module) runMain(main);`, so requiring it here
|
||||
* for its exported pure functions does not also trigger a live ShellCheck
|
||||
* run against the real gsd-core/workflows/ tree.
|
||||
*/
|
||||
|
||||
const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('node:path');
|
||||
const fc = require('./helpers/fast-check-setup.cjs');
|
||||
|
||||
const {
|
||||
substitutePlaceholders,
|
||||
stripCommandSubstitutions,
|
||||
stripShellComments,
|
||||
extractForLoops,
|
||||
findBareForLoopSplits,
|
||||
findingKey,
|
||||
partitionAgainstBaseline,
|
||||
} = require(path.join(__dirname, '..', 'scripts', 'lint-workflow-shellcheck.cjs'));
|
||||
|
||||
/** Minimal block shape findBareForLoopSplits expects (see the source's own
|
||||
* extractBashBlocks for the real shape this stands in for). */
|
||||
function mkBlock(body, overrides = {}) {
|
||||
return { file: 'w.md', blockIdx: 0, firstBodyLine: 1, body, ...overrides };
|
||||
}
|
||||
|
||||
describe('stripCommandSubstitutions', () => {
|
||||
test('empty string', () => {
|
||||
assert.equal(stripCommandSubstitutions(''), '');
|
||||
});
|
||||
|
||||
test('no substitution present leaves the string intact', () => {
|
||||
assert.equal(stripCommandSubstitutions('echo hello'), 'echo hello');
|
||||
});
|
||||
|
||||
test('substitution at the start', () => {
|
||||
assert.equal(stripCommandSubstitutions('$(echo hi) world'), ' world');
|
||||
});
|
||||
|
||||
test('substitution in the middle', () => {
|
||||
assert.equal(stripCommandSubstitutions('a $(echo hi) b'), 'a b');
|
||||
});
|
||||
|
||||
test('substitution at the end', () => {
|
||||
assert.equal(stripCommandSubstitutions('a $(echo hi)'), 'a ');
|
||||
});
|
||||
|
||||
test('nested command substitution — $(echo $(nested))', () => {
|
||||
assert.equal(stripCommandSubstitutions('pre $(echo $(nested)) post'), 'pre post');
|
||||
});
|
||||
|
||||
test('unterminated substitution does not crash or hang, consumes to end of string', () => {
|
||||
assert.equal(stripCommandSubstitutions('a $(echo hi'), 'a ');
|
||||
});
|
||||
|
||||
test('malformed nested-unterminated input does not crash or hang', () => {
|
||||
assert.equal(stripCommandSubstitutions('a $(echo $(nested'), 'a ');
|
||||
});
|
||||
});
|
||||
|
||||
describe('stripShellComments', () => {
|
||||
test('strips a full-line comment to spaces, preserving the newline', () => {
|
||||
assert.equal(stripShellComments('# a comment\necho hi'), ' \necho hi');
|
||||
});
|
||||
|
||||
test('strips a trailing comment after code, preserving preceding code', () => {
|
||||
assert.equal(stripShellComments('echo hi # trailing'), 'echo hi ');
|
||||
});
|
||||
|
||||
test('does NOT strip ${VAR#pattern} parameter-expansion # (the false-positive this pass exists to avoid)', () => {
|
||||
assert.equal(stripShellComments('sm_raw=${sm_raw#./}'), 'sm_raw=${sm_raw#./}');
|
||||
});
|
||||
|
||||
test('does not strip a # immediately following a non-whitespace identifier character', () => {
|
||||
assert.equal(stripShellComments('x=${foo#bar} y=1'), 'x=${foo#bar} y=1');
|
||||
});
|
||||
|
||||
test('a # inside single quotes is preserved literally, not treated as a comment start', () => {
|
||||
assert.equal(stripShellComments("echo 'a # b'"), "echo 'a # b'");
|
||||
});
|
||||
});
|
||||
|
||||
describe('substitutePlaceholders', () => {
|
||||
test('substitutes a single-token placeholder ({run_dir}) to a shell-safe bareword', () => {
|
||||
assert.equal(substitutePlaceholders('cd {run_dir}'), 'cd PLACEHOLDER_run_dir');
|
||||
});
|
||||
|
||||
test('substitutes a multi-word prose placeholder ({discovered test command})', () => {
|
||||
assert.equal(
|
||||
substitutePlaceholders('run {discovered test command}'),
|
||||
'run PLACEHOLDER_discovered_test_command',
|
||||
);
|
||||
});
|
||||
|
||||
test('does NOT substitute real ${VAR} parameter expansion', () => {
|
||||
assert.equal(substitutePlaceholders('echo ${VAR}'), 'echo ${VAR}');
|
||||
});
|
||||
|
||||
test('does NOT clobber {1..5} POSIX numeric brace expansion (pinning the corrected behavior)', () => {
|
||||
assert.equal(substitutePlaceholders('echo {1..5}'), 'echo {1..5}');
|
||||
});
|
||||
|
||||
test('does NOT clobber {a,b,c} POSIX brace-expansion list (pinning the corrected behavior)', () => {
|
||||
assert.equal(substitutePlaceholders('echo {a,b,c}'), 'echo {a,b,c}');
|
||||
});
|
||||
|
||||
test('does NOT clobber { cmd1; cmd2; } compound-command grouping (pinning the corrected behavior)', () => {
|
||||
const input = '{ echo hi; echo bye; }';
|
||||
assert.equal(substitutePlaceholders(input), input);
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractForLoops', () => {
|
||||
test('captures loopVar and listExpr for a simple for-header', () => {
|
||||
const [loop] = extractForLoops('for x in $VAR; do\n echo "$x"\ndone\n');
|
||||
assert.equal(loop.loopVar, 'x');
|
||||
assert.equal(loop.listExpr, '$VAR');
|
||||
});
|
||||
|
||||
test('does not truncate the list-expr at a `;` nested inside $( ... )', () => {
|
||||
const [loop] = extractForLoops('for x in $(echo a; echo b); do\n echo "$x"\ndone\n');
|
||||
assert.equal(loop.listExpr, '$(echo a; echo b)');
|
||||
});
|
||||
|
||||
test('finds multiple for-loops in one body', () => {
|
||||
const loops = extractForLoops('for a in 1 2; do :; done\nfor b in $Y; do :; done\n');
|
||||
assert.equal(loops.length, 2);
|
||||
assert.equal(loops[0].loopVar, 'a');
|
||||
assert.equal(loops[1].loopVar, 'b');
|
||||
});
|
||||
});
|
||||
|
||||
describe('findBareForLoopSplits — the core #4109 detector', () => {
|
||||
test('POSITIVE: bare unquoted `for x in $VAR; do ... done` is flagged', () => {
|
||||
const findings = findBareForLoopSplits([mkBlock('for x in $VAR; do\n echo "$x"\ndone\n')]);
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].loopVar, 'x');
|
||||
assert.equal(findings[0].varName, 'VAR');
|
||||
});
|
||||
|
||||
test('POSITIVE: bare unquoted `${VAR}` braced form is also flagged', () => {
|
||||
const findings = findBareForLoopSplits([mkBlock('for x in ${VAR}; do\n echo "$x"\ndone\n')]);
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].varName, 'VAR');
|
||||
});
|
||||
|
||||
test('NEGATIVE: command-substitution-wrapped form (the actual #4109 fix pattern) is NOT flagged', () => {
|
||||
const findings = findBareForLoopSplits([
|
||||
mkBlock('for x in $(printf \'%s\' "$VAR"); do\n echo "$x"\ndone\n'),
|
||||
]);
|
||||
assert.equal(findings.length, 0);
|
||||
});
|
||||
|
||||
test('NEGATIVE: quoted `"$VAR"` form is NOT flagged', () => {
|
||||
const findings = findBareForLoopSplits([mkBlock('for x in "$VAR"; do\n echo "$x"\ndone\n')]);
|
||||
assert.equal(findings.length, 0);
|
||||
});
|
||||
|
||||
test('NEGATIVE: literal words (no variable at all) are NOT flagged', () => {
|
||||
const findings = findBareForLoopSplits([mkBlock('for x in a b c; do\n echo "$x"\ndone\n')]);
|
||||
assert.equal(findings.length, 0);
|
||||
});
|
||||
|
||||
test('a for-loop shape quoted inside a `#` comment (prose referencing a past bug) is NOT flagged', () => {
|
||||
const findings = findBareForLoopSplits([
|
||||
mkBlock('# old bug: for x in $VAR; do ... done\necho ok\n'),
|
||||
]);
|
||||
assert.equal(findings.length, 0);
|
||||
});
|
||||
|
||||
// --- fast-check property test (CLAUDE.md-mandated for parsers) ---
|
||||
//
|
||||
// Property: for any generated loop-var/var-name pair and any of the SAFE
|
||||
// forms (quoted, command-substitution-wrapped, literal-words), the
|
||||
// detector reports zero findings; for either UNSAFE bare form (bare $VAR
|
||||
// or braced ${VAR}), it reports exactly one finding naming that variable.
|
||||
const identArb = fc.stringMatching(/^[A-Za-z_][A-Za-z0-9_]{0,6}$/);
|
||||
const formArb = fc.constantFrom('bare', 'braced', 'quoted', 'substituted', 'literal');
|
||||
|
||||
function buildLoopBody(loopVar, varName, form) {
|
||||
let listExpr;
|
||||
switch (form) {
|
||||
case 'bare':
|
||||
listExpr = `$${varName}`;
|
||||
break;
|
||||
case 'braced':
|
||||
listExpr = `\${${varName}}`;
|
||||
break;
|
||||
case 'quoted':
|
||||
listExpr = `"$${varName}"`;
|
||||
break;
|
||||
case 'substituted':
|
||||
listExpr = `$(printf '%s' "$${varName}")`;
|
||||
break;
|
||||
case 'literal':
|
||||
listExpr = 'a b c';
|
||||
break;
|
||||
default:
|
||||
throw new Error(`unreachable form: ${form}`);
|
||||
}
|
||||
return `for ${loopVar} in ${listExpr}; do\n echo "$${loopVar}"\ndone\n`;
|
||||
}
|
||||
|
||||
test('property: bare/braced forms are flagged naming the variable; quoted/substituted/literal forms never are', () => {
|
||||
fc.assert(
|
||||
fc.property(identArb, identArb, formArb, (loopVar, varName, form) => {
|
||||
const body = buildLoopBody(loopVar, varName, form);
|
||||
const findings = findBareForLoopSplits([mkBlock(body)]);
|
||||
if (form === 'bare' || form === 'braced') {
|
||||
assert.equal(findings.length, 1);
|
||||
assert.equal(findings[0].varName, varName);
|
||||
} else {
|
||||
assert.equal(findings.length, 0);
|
||||
}
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('findingKey / partitionAgainstBaseline', () => {
|
||||
test('findingKey matches on {file, code, message} only — ignores line number', () => {
|
||||
const a = { file: 'w.md', code: '2086', message: 'msg', line: 10 };
|
||||
const b = { file: 'w.md', code: '2086', message: 'msg', line: 999 };
|
||||
assert.equal(findingKey(a), findingKey(b));
|
||||
});
|
||||
|
||||
test('a finding shifted to a different line, same file/code/message, still matches the baseline (drift tolerance)', () => {
|
||||
const baseline = [{ file: 'w.md', code: '2086', message: 'msg' }];
|
||||
const current = [{ file: 'w.md', code: '2086', message: 'msg', line: 999 }];
|
||||
const { newFindings, baselinedFindings } = partitionAgainstBaseline(current, baseline);
|
||||
assert.equal(newFindings.length, 0);
|
||||
assert.equal(baselinedFindings.length, 1);
|
||||
});
|
||||
|
||||
test('a THIRD occurrence of a message with only two accepted baseline instances is reported new (multiset semantics)', () => {
|
||||
const baseline = [
|
||||
{ file: 'w.md', code: '2086', message: 'msg' },
|
||||
{ file: 'w.md', code: '2086', message: 'msg' },
|
||||
];
|
||||
const current = [
|
||||
{ file: 'w.md', code: '2086', message: 'msg', line: 1 },
|
||||
{ file: 'w.md', code: '2086', message: 'msg', line: 2 },
|
||||
{ file: 'w.md', code: '2086', message: 'msg', line: 3 },
|
||||
];
|
||||
const { newFindings, baselinedFindings } = partitionAgainstBaseline(current, baseline);
|
||||
assert.equal(newFindings.length, 1);
|
||||
assert.equal(baselinedFindings.length, 2);
|
||||
});
|
||||
|
||||
test('a finding with a different code (same file/message) is NOT matched against the baseline', () => {
|
||||
const baseline = [{ file: 'w.md', code: '2086', message: 'msg' }];
|
||||
const current = [{ file: 'w.md', code: '2046', message: 'msg', line: 1 }];
|
||||
const { newFindings } = partitionAgainstBaseline(current, baseline);
|
||||
assert.equal(newFindings.length, 1);
|
||||
});
|
||||
|
||||
test('an empty baseline reports every current finding as new', () => {
|
||||
const current = [{ file: 'w.md', code: '2086', message: 'msg', line: 1 }];
|
||||
const { newFindings, baselinedFindings } = partitionAgainstBaseline(current, []);
|
||||
assert.equal(newFindings.length, 1);
|
||||
assert.equal(baselinedFindings.length, 0);
|
||||
});
|
||||
});
|
||||
@@ -57,7 +57,18 @@ const STDIN_BOUND_MS = 5000;
|
||||
function detectShells() {
|
||||
const shells = [{ name: 'bash', cmd: 'bash' }];
|
||||
const probe = spawnSync('zsh', ['-c', 'exit 0'], { timeout: PROBE_TIMEOUT_MS, windowsHide: true });
|
||||
if (!probe.error && probe.status === 0) shells.push({ name: 'zsh', cmd: 'zsh' });
|
||||
if (!probe.error && probe.status === 0) {
|
||||
shells.push({ name: 'zsh', cmd: 'zsh' });
|
||||
} else {
|
||||
// gsd-core#4109: a skipped zsh lane reads identically to a passing one in
|
||||
// this suite's own output, which is exactly why the bash/zsh
|
||||
// word-splitting bug class went undetected in CI as long as it did. Make
|
||||
// the skip loud so a zsh-less run (e.g. some ubuntu CI images) reads as
|
||||
// "zsh coverage unknown", not "all lanes green".
|
||||
console.warn(
|
||||
'[review-build-prompt-optional-sections.test.cjs] zsh not available — zsh-lane tests SKIPPED, coverage for this shell is UNKNOWN, not verified',
|
||||
);
|
||||
}
|
||||
return shells;
|
||||
}
|
||||
const SHELLS = detectShells();
|
||||
|
||||
@@ -44,7 +44,18 @@ const REPO_ROOT = path.join(__dirname, '..');
|
||||
function detectShells() {
|
||||
const shells = [{ name: 'bash', cmd: 'bash' }];
|
||||
const probe = spawnSync('zsh', ['-c', 'exit 0'], { timeout: PROBE_TIMEOUT_MS, windowsHide: true });
|
||||
if (!probe.error && probe.status === 0) shells.push({ name: 'zsh', cmd: 'zsh' });
|
||||
if (!probe.error && probe.status === 0) {
|
||||
shells.push({ name: 'zsh', cmd: 'zsh' });
|
||||
} else {
|
||||
// gsd-core#4109: a skipped zsh lane reads identically to a passing one in
|
||||
// this suite's own output, which is exactly why the bash/zsh
|
||||
// word-splitting bug class went undetected in CI as long as it did. Make
|
||||
// the skip loud so a zsh-less run (e.g. some ubuntu CI images) reads as
|
||||
// "zsh coverage unknown", not "all lanes green".
|
||||
console.warn(
|
||||
'[review-plan-coverage-manifest.test.cjs] zsh not available — zsh-lane tests SKIPPED, coverage for this shell is UNKNOWN, not verified',
|
||||
);
|
||||
}
|
||||
return shells;
|
||||
}
|
||||
const SHELLS = detectShells();
|
||||
@@ -186,6 +197,101 @@ function readCoverageJson(runDir, slug) {
|
||||
return fs.existsSync(p) ? JSON.parse(fs.readFileSync(p, 'utf8')) : null;
|
||||
}
|
||||
|
||||
/**
|
||||
* #4109 — extract the write_reviews gate-check block that counts dispatched
|
||||
* vs skipped lanes and sets ALL_LANES_SKIPPED / TOTAL_LANE_FAILURE. Anchored
|
||||
* on the JSONL variable at the top of that block (distinct from, and earlier
|
||||
* than, the .plans-manifest.md anchor extractCoverageCheckBlock() uses).
|
||||
*/
|
||||
function extractGateCheckBlock() {
|
||||
const content = readWorkflowCombined(REVIEW_WORKFLOW);
|
||||
const anchorIdx = content.indexOf('JSONL="$RUN_DIR/gsd-review-lane-results.jsonl"');
|
||||
assert.notEqual(anchorIdx, -1, 'write_reviews must reference gsd-review-lane-results.jsonl — the gate-check block is missing');
|
||||
const before = content.slice(0, anchorIdx);
|
||||
const fenceOpenRe = /```bash\r?\n/g;
|
||||
let lastOpen = -1;
|
||||
let m;
|
||||
while ((m = fenceOpenRe.exec(before)) !== null) lastOpen = m.index + m[0].length;
|
||||
assert.notEqual(lastOpen, -1, 'gsd-review-lane-results.jsonl reference is not inside a ```bash fence of write_reviews');
|
||||
const after = content.slice(lastOpen);
|
||||
const closeIdx = after.indexOf('\n```');
|
||||
assert.notEqual(closeIdx, -1, 'unterminated ```bash fence around the gate-check block');
|
||||
const body = after.slice(0, closeIdx);
|
||||
assert.ok(
|
||||
body.includes('ALL_LANES_SKIPPED'),
|
||||
'extracted block references JSONL but not ALL_LANES_SKIPPED — wrong block',
|
||||
);
|
||||
return body;
|
||||
}
|
||||
|
||||
/**
|
||||
* #4109 — extract the invoke_reviewers dispatch + join loops, from the
|
||||
* "Split ONCE, de-duplicated" comment (immediately before the DISPATCH_SLUGS
|
||||
* accumulator at this site) through the block's own closing fence. This span
|
||||
* references run_review_lane() and PARALLEL_LANES, both defined earlier in
|
||||
* the SAME fence but out of scope for this extractor — callers must prepend
|
||||
* a stub (see buildDispatchFixture / DISPATCH_JOIN_STUB below).
|
||||
*/
|
||||
function extractDispatchJoinBlock() {
|
||||
const content = readWorkflowCombined(REVIEW_WORKFLOW);
|
||||
const anchorIdx = content.indexOf('# Split ONCE, de-duplicated');
|
||||
assert.notEqual(anchorIdx, -1, 'invoke_reviewers must contain the "Split ONCE, de-duplicated" comment anchor');
|
||||
const after = content.slice(anchorIdx);
|
||||
const closeIdx = after.indexOf('\n```');
|
||||
assert.notEqual(closeIdx, -1, 'unterminated ```bash fence after the dispatch/join anchor');
|
||||
const body = after.slice(0, closeIdx);
|
||||
assert.ok(body.includes('DISPATCH_SLUGS='), 'extracted span does not include the DISPATCH_SLUGS accumulator — wrong anchor');
|
||||
assert.ok(body.includes('wait'), 'extracted span does not include the join `wait` — anchor did not reach the join loop');
|
||||
return body;
|
||||
}
|
||||
|
||||
const DISPATCH_JOIN_STUB = 'run_review_lane() { echo "$1" >> "$RUN_DIR/dispatch-log.txt"; }\nPARALLEL_LANES="false"\n';
|
||||
|
||||
/** Fixture for the gate-check block: a RUN_DIR with per-slug stub files, no aggregate JSONL. */
|
||||
function buildGateCheckFixture(slugs, skippedSlugs) {
|
||||
const root = createTempDir('gsd-4109-gate-');
|
||||
const runDir = path.join(root, 'run');
|
||||
fs.mkdirSync(runDir);
|
||||
for (const slug of slugs) {
|
||||
if (skippedSlugs.includes(slug)) {
|
||||
fs.writeFileSync(
|
||||
path.join(runDir, `gsd-review-${slug}.md`),
|
||||
`${slug} review skipped: prompt budget (500 tokens) too small for the minimum review set.\n`,
|
||||
);
|
||||
}
|
||||
}
|
||||
return {
|
||||
root,
|
||||
runDir,
|
||||
env: { SELECTED_REVIEWERS: slugs.join(',') },
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Fixture for the dispatch/join loops: a RUN_DIR and SELECTED_REVIEWERS.
|
||||
* RUN_DIR is passed as an env var (not the `{run_dir}` placeholder) because
|
||||
* extractDispatchJoinBlock() starts at the "Split ONCE" comment, AFTER the
|
||||
* `RUN_DIR="{run_dir}"` assignment earlier in the same real fence — the
|
||||
* aggregate/join loop still references `$RUN_DIR` directly, so it must come
|
||||
* from the environment here.
|
||||
*/
|
||||
function buildDispatchFixture(selectedReviewersCsv) {
|
||||
const root = createTempDir('gsd-4109-dispatch-');
|
||||
const runDir = path.join(root, 'run');
|
||||
fs.mkdirSync(runDir);
|
||||
return {
|
||||
root,
|
||||
runDir,
|
||||
env: { SELECTED_REVIEWERS: selectedReviewersCsv, RUN_DIR: runDir },
|
||||
};
|
||||
}
|
||||
|
||||
function readDispatchLog(runDir) {
|
||||
const content = readIfPresent(path.join(runDir, 'dispatch-log.txt'));
|
||||
if (content === null) return [];
|
||||
return content.split(/\r?\n/).map((l) => l.trim()).filter((l) => l.length > 0);
|
||||
}
|
||||
|
||||
describe('#3301 build_prompt derives and appends a plan coverage manifest', () => {
|
||||
for (const shell of SHELLS) {
|
||||
test(`[${shell.name}] zero plans: manifest reports Total plans: 0 and no ids`, (t) => {
|
||||
@@ -357,6 +463,11 @@ describe('#3301 write_reviews grades each lane against the plan coverage manifes
|
||||
const offenders = extractAllBashBlocks().filter((b) => /\[\s*"\$SLUG"\s*=\s*"(?!coderabbit)/.test(b));
|
||||
assert.deepEqual(offenders, [], 'only the coderabbit exemption may hardcode a slug comparison');
|
||||
});
|
||||
|
||||
test('structural: no bare-unquoted DISPATCH_SLUGS consumption remains (#4109)', () => {
|
||||
const offenders = extractAllBashBlocks().filter((b) => /for SLUG in \$DISPATCH_SLUGS;/.test(b));
|
||||
assert.deepEqual(offenders, [], 'DISPATCH_SLUGS must be quoted or word-split explicitly, not consumed bare — zsh does not IFS-split an unquoted scalar');
|
||||
});
|
||||
});
|
||||
|
||||
describe('#3301 REVIEWS.md documents the plan_coverage frontmatter key', () => {
|
||||
@@ -370,3 +481,109 @@ describe('#3301 REVIEWS.md documents the plan_coverage frontmatter key', () => {
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* #4109 — a zsh word-splitting bug: DISPATCH_SLUGS is built as a
|
||||
* space-separated scalar accumulator and consumed via unquoted
|
||||
* `for SLUG in $DISPATCH_SLUGS; do`. Bash IFS-splits an unquoted scalar by
|
||||
* default; zsh does not, so the entire accumulator (leading space and all)
|
||||
* collapses onto ONE bogus iteration under zsh whenever 2+ reviewers are
|
||||
* selected. These rows extract the REAL shipped bash from the two remaining
|
||||
* untested sites (write_reviews' gate-check block, and invoke_reviewers'
|
||||
* dispatch + join loops) and execute it under both shells.
|
||||
*/
|
||||
describe('#4109 gate-check counts every dispatched reviewer under both shells', () => {
|
||||
for (const shell of SHELLS) {
|
||||
test(`[${shell.name}] 2 reviewers both skipped: dispatched=2 skipped=2 all_lanes_skipped=true`, (t) => {
|
||||
const fx = buildGateCheckFixture(['claude', 'codex'], ['claude', 'codex']);
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = extractGateCheckBlock()
|
||||
+ '\necho "{\\"dispatched_count\\":${DISPATCHED_COUNT:-0},\\"skipped_count\\":${SKIPPED_COUNT:-0},'
|
||||
+ '\\"all_lanes_skipped\\":\\"${ALL_LANES_SKIPPED:-false}\\",\\"total_lane_failure\\":\\"${TOTAL_LANE_FAILURE:-false}\\"}"\n';
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const out = JSON.parse(res.stdout.trim());
|
||||
assert.strictEqual(out.dispatched_count, 2, `expected 2 dispatched slugs, got ${res.stdout}`);
|
||||
assert.strictEqual(out.skipped_count, 2, `expected 2 skipped slugs, got ${res.stdout}`);
|
||||
assert.strictEqual(out.all_lanes_skipped, 'true');
|
||||
assert.strictEqual(out.total_lane_failure, 'false');
|
||||
});
|
||||
|
||||
test(`[${shell.name}] mixed skip and total failure: dispatched=2 skipped=1 total_lane_failure=true`, (t) => {
|
||||
const fx = buildGateCheckFixture(['claude', 'codex'], ['claude']);
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = extractGateCheckBlock()
|
||||
+ '\necho "{\\"dispatched_count\\":${DISPATCHED_COUNT:-0},\\"skipped_count\\":${SKIPPED_COUNT:-0},'
|
||||
+ '\\"all_lanes_skipped\\":\\"${ALL_LANES_SKIPPED:-false}\\",\\"total_lane_failure\\":\\"${TOTAL_LANE_FAILURE:-false}\\"}"\n';
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const out = JSON.parse(res.stdout.trim());
|
||||
assert.strictEqual(out.dispatched_count, 2, `expected 2 dispatched slugs, got ${res.stdout}`);
|
||||
assert.strictEqual(out.skipped_count, 1, `expected 1 skipped slug, got ${res.stdout}`);
|
||||
assert.strictEqual(out.all_lanes_skipped, 'false');
|
||||
assert.strictEqual(out.total_lane_failure, 'true');
|
||||
});
|
||||
|
||||
test(`[${shell.name}] single reviewer still counts correctly (boundary)`, (t) => {
|
||||
const fx = buildGateCheckFixture(['claude'], ['claude']);
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = extractGateCheckBlock()
|
||||
+ '\necho "{\\"dispatched_count\\":${DISPATCHED_COUNT:-0},\\"skipped_count\\":${SKIPPED_COUNT:-0},'
|
||||
+ '\\"all_lanes_skipped\\":\\"${ALL_LANES_SKIPPED:-false}\\",\\"total_lane_failure\\":\\"${TOTAL_LANE_FAILURE:-false}\\"}"\n';
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const out = JSON.parse(res.stdout.trim());
|
||||
assert.strictEqual(out.dispatched_count, 1, `expected 1 dispatched slug, got ${res.stdout}`);
|
||||
assert.strictEqual(out.skipped_count, 1, `expected 1 skipped slug, got ${res.stdout}`);
|
||||
assert.strictEqual(out.all_lanes_skipped, 'true');
|
||||
});
|
||||
|
||||
test(`[${shell.name}] 3 reviewers all skipped (boundary)`, (t) => {
|
||||
const fx = buildGateCheckFixture(['claude', 'codex', 'gemini'], ['claude', 'codex', 'gemini']);
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = extractGateCheckBlock()
|
||||
+ '\necho "{\\"dispatched_count\\":${DISPATCHED_COUNT:-0},\\"skipped_count\\":${SKIPPED_COUNT:-0},'
|
||||
+ '\\"all_lanes_skipped\\":\\"${ALL_LANES_SKIPPED:-false}\\",\\"total_lane_failure\\":\\"${TOTAL_LANE_FAILURE:-false}\\"}"\n';
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const out = JSON.parse(res.stdout.trim());
|
||||
assert.strictEqual(out.dispatched_count, 3, `expected 3 dispatched slugs, got ${res.stdout}`);
|
||||
assert.strictEqual(out.skipped_count, 3, `expected 3 skipped slugs, got ${res.stdout}`);
|
||||
assert.strictEqual(out.all_lanes_skipped, 'true');
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
describe('#4109 invoke_reviewers dispatches every deduped reviewer exactly once under both shells', () => {
|
||||
for (const shell of SHELLS) {
|
||||
test(`[${shell.name}] 2 reviewers each dispatched exactly once`, (t) => {
|
||||
const fx = buildDispatchFixture('claude,codex');
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = DISPATCH_JOIN_STUB + '\n' + extractDispatchJoinBlock();
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const lines = readDispatchLog(fx.runDir);
|
||||
assert.deepEqual(lines, ['claude', 'codex']);
|
||||
});
|
||||
|
||||
test(`[${shell.name}] duplicate slug in SELECTED_REVIEWERS deduped, dispatched once`, (t) => {
|
||||
const fx = buildDispatchFixture('claude,codex,claude');
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = DISPATCH_JOIN_STUB + '\n' + extractDispatchJoinBlock();
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const lines = readDispatchLog(fx.runDir);
|
||||
assert.deepEqual(lines, ['claude', 'codex']);
|
||||
});
|
||||
|
||||
test(`[${shell.name}] 3 reviewers each dispatched exactly once (boundary)`, (t) => {
|
||||
const fx = buildDispatchFixture('claude,codex,gemini');
|
||||
t.after(() => cleanup(fx.root));
|
||||
const body = DISPATCH_JOIN_STUB + '\n' + extractDispatchJoinBlock();
|
||||
const res = runScript(shell, body, fx.root, fx.runDir, fx.env, REPO_ROOT);
|
||||
assert.strictEqual(res.status, 0, `block exited ${res.status}: ${res.stderr}`);
|
||||
const lines = readDispatchLog(fx.runDir);
|
||||
assert.deepEqual(lines, ['claude', 'codex', 'gemini']);
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user