diff --git a/.changeset/humble-cats-sprint.md b/.changeset/humble-cats-sprint.md new file mode 100644 index 000000000..c8ebb2171 --- /dev/null +++ b/.changeset/humble-cats-sprint.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3590 +--- +**GSD no longer commits `.planning/` files you told it to ignore** — several workflow steps staged planning artifacts with raw `git add`, bypassing the `commit_docs` setting and the `.gitignore` auto-detect entirely, so planning docs reached shared history anyway. (#3585) diff --git a/CONTEXT.md b/CONTEXT.md index 78f1bd37f..89586848a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -221,6 +221,9 @@ Module owning agent-presence resolution and verification, extracted from the Cor ### Config Loader Module Module owning project configuration loading: reads `.planning/config.json`, merges built-in defaults (`CONFIG_DEFAULTS`/`CANONICAL_CONFIG_DEFAULTS`), normalizes legacy keys, applies the active-workstream overlay, validates against the config schema, and warns on unknown keys/profile overrides. Primary interface: `loadConfigResolved(cwd, options) → ConfigResolution { config, source, degraded }` (provenance-aware, ADR-1411 P2 / #1415) — `source` ∈ `'workstream' | 'root' | 'builtin-defaults' | 'global-defaults'`; `degraded:true` when a workstream was requested but its config.json was absent (fell back to root config). `loadConfig(cwd, options) → Record` is the back-compat thin wrapper over `loadConfigResolved` (byte-identical result). Resolution is **caller-anchored, not loader-anchored**: `loadConfigResolved` resolves `cwd` as-is (no walk-up), so `loadConfig` stays byte-identical for its callers; callers that need cwd-drift tolerance (e.g. `cmdAgentSkills`) anchor to the project root via `findProjectRoot` (Project-Root Resolution Module) *before* calling `loadConfigResolved`. Helper exports: `_deepMergeConfig`, `isGitIgnored`, `_warnUnknownProfileOverrides`. Depends only on leaf modules (`configuration`, `config-schema`, `planning-workspace`, `shell-command-projection`, `core-utils`, `model-catalog`) — no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2e (#885) as the prerequisite for the model-resolver extraction (the resolvers call `loadConfig`); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/config-loader.cjs` (generated from `src/config-loader.cts`). +### Planning Commit Gate (`commit_docs`) +The seam deciding whether `.planning/` artifacts reach git. `commit_docs` resolves in the Config Loader Module (`loadConfigResolved`) through an ordered chain — an explicit value (top-level `commit_docs` or its `planning.commit_docs` alias) wins; absent that, `isGitIgnored(cwd, '.planning/')` auto-resolves it to `false`; otherwise the manifest default (`true`) applies. `cmdCommit` (Command Module) is the ONLY sanctioned writer: it returns the typed skip envelope `{ committed: false, skipped: true, hash: null, reason }` with `reason ∈ { 'skipped_commit_docs_false', 'skipped_gitignored' }` rather than erroring, and `skipped: true` is explicit so agent prompts match a first-class success signal instead of inferring a skip from a missing `committed` and improvising a raw-git fallback (#3678). Those `reason` strings are a de facto public contract — `agents/gsd-executor.md` pattern-matches on them — so they are additive-only. **The two reasons are ordered, not peers, and `skipped_gitignored` is near-unreachable in a real project** (measured #3585): `cmdCommit` tests resolved `commit_docs` FIRST, and whenever `.planning/config.json` exists — which it does in every initialized project — the loader's gitignore auto-detect has already resolved that value to `false`, so the first branch returns `skipped_commit_docs_false` and the `isGitIgnored` branch below it is never reached. `skipped_gitignored` fires only when `config.json` is absent entirely, so the loader falls back to the `true` default and `cmdCommit`'s own check is what fires. A gitignore-driven skip therefore reports the config-driven reason; both members are behaviorally pinned by `tests/commit-docs-bypass.test.cjs` (B1-B3 and G1) so a rename fails loudly, but the reason a user sees does not distinguish *which* input suppressed the commit. **The gate is bypassable only from OUTSIDE the code**: a workflow step that types `git add` into its own shell reaches the index without passing through `cmdCommit`, and no code change can intercept that. Two guards therefore enforce it as text rather than at runtime: `tests/commit-files-pathspec.test.cjs` (#2269) requires every shipped `commit` invocation to declare `--files`, and `tests/commit-docs-bypass.test.cjs` (#1783, made repo-wide by #3585) requires every shipped `git add` able to reach `.planning/` to sit inside an **executable** `commit_docs` check — a markdown prose conditional ("**If `commit_docs` is true:**") is not a guard, because the bash block below it runs regardless. Both consume one shell tokenizer (`tests/helpers/shipped-command-scan.cjs`) and one exemption marker (`# gsd-scan-ignore: #NNN`, reason must cite a tracking ref per ADR-456). Guard state does not cross a fenced-block boundary: each fenced block is its own shell, so a guard opened in one block does not protect a `git add` in the next. Known limit: `.gitignore` has no effect on files git already TRACKS, so a project that committed `.planning/` before ignoring it keeps staging those paths — see `gsd-core/references/planning-config.md` and #3586. + ### Model Catalog Module Leaf module owning the **static** model tables and the closed vocabularies derived from them — the tier/runtime/provider enums (`VALID_TIERS`, `VALID_AGENT_TIERS`, `KNOWN_RUNTIMES`, `KNOWN_PROVIDERS`, `RUNTIMES_WITH_REASONING_EFFORT`, `RUNTIMES_WITH_FAST_MODE`, `ADAPTIVE_TIER_VALUES`), the alias and profile maps (`MODEL_ALIAS_MAP`, `RUNTIME_PROFILE_MAP`, `PROVIDER_PRESETS`), effort rendering (`renderEffortForRuntime`), and the agent→model projections (`getAgentToModelMapForProfile`, `formatAgentToModelMapAsTable`). A **genuine leaf**: it imports `node:path` and its own `model-catalog.json` and nothing else, which is what makes it the correct home for anything several unrelated surfaces must agree on. Model *ids* live in `model-catalog.json`, never inline — changing one means regenerating goldens (`UPDATE_GOLDEN`). Also owns the **Anthropic-flavored-model rule** (#3241, ADR-2313): `CLAUDE_AGENT_ALIASES` (the frozen four-alias set `opus`/`sonnet`/`haiku`/`fable`) and `isAnthropicFlavoredModel(model)`, which is true for a bare tier alias or for any `claude-*` id in any provider namespacing (`anthropic/claude-*`, `us.anthropic.claude-*`); no OpenAI/Codex model id contains "claude", so the case-insensitive substring test is safe and exhaustive. It was **moved down here from the Model Resolver Module** rather than shared from there, because the Codex posture check (`agent-install-check`, epic #2313 Phase 2) and the Codex `.toml` sync (`commands`, Phase 3) both need the rule and neither may take the `config-loader` dependency `model-resolver` would have brought; `model-resolver` re-exports it for back-compat and a parity test fails if the two ever fork. _Avoid_: "the model list" (ambiguous between the catalog JSON and the derived enums). See Model Resolver Module, ADR-2313, and ADR-0003. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 93408a848..b66d8a175 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1119,6 +1119,74 @@ commit, so you are told which of the two problems you actually have. A marker th survives shell tokenization as an *argument* declares nothing: it reached argv, which means the runtime executed the line. +### Every `git add` in shipped content that can reach `.planning/` must sit inside an *executable* `commit_docs` check + +`tests/commit-docs-bypass.test.cjs` scans every `.md` under `gsd-core/workflows/`, +`gsd-core/references/`, `agents/`, `commands/` and `skills/` and fails if a `git add` that could +stage `.planning/` is not enclosed by a `commit_docs` check that actually runs. + +This is the sibling of the `--files` guard above, and it exists for the complementary hole. +That one keeps a *seam* invocation honest; this one catches the steps that never reach the seam +at all. `commit_docs` is resolved and enforced inside `cmdCommit`, so a step that types +`git add` into its own shell bypasses it completely — no code change can intercept that, only a +guard over the shipped text. + +**Prose is not a guard.** This is the failure the scan was written for. All three of these +*looked* gated and none of them were: + +````markdown +**If `commit_docs` is true:** +```bash +git add "${EVAL_REVIEW_FILE}" ← runs unconditionally; the bold line is markdown +``` +```` + +The bash block executes whatever the sentence above it says. Write the check in shell, which is +the form `gsd-core/workflows/quick/steps/worktree-pre-dispatch-commit.md` already uses: + +```bash +COMMIT_DOCS=$(gsd_run query config-get commit_docs 2>/dev/null || echo "true") +if [ "$COMMIT_DOCS" != "false" ]; then + git add "${ARTIFACT}" +fi +``` + +Both polarities are accepted (`!= "false"` and `= "true"`). The `|| echo "true"` fallback is +deliberate: a tooling failure must fail *open*, or a broken `gsd-tools` silently stops committing +planning docs for someone who wants them. + +Three consequences worth knowing before you edit shipped content: + +**Guard state does not cross a fenced block.** Each fenced block is its own shell, so an `if` +opened in one block does not protect a `git add` in the next — the same reason +`new-milestone.md` warns that a `GSD_WS` guard set in an earlier step reads as unset later. Put +the check and the `git add` in the same block. + +**An unresolvable path is treated as reaching `.planning/`.** `git add "${ARTIFACT}"` is flagged, +because whether `$ARTIFACT` expands under `.planning/` is not knowable statically and the scan +fails closed. A `{placeholder}` in braces with no `$` is documentation notation and does not +trigger it. If your `git add` genuinely cannot touch `.planning/`, name the path literally. + +**Only fenced lines are scanned.** Inline-backtick prose — including the anti-pattern +documentation that tells you never to run `git add -A` — is not executable and is not flagged. + +The same `# gsd-scan-ignore: #NNN` declaration as the `--files` guard exempts a deliberate +counter-example, on the invocation's own line, in shell-comment position, with a reason naming a +tracking issue or URL. Both guards share one tokenizer and one marker implementation +(`tests/helpers/shipped-command-scan.cjs`) so the two conventions can never drift into two rules +wearing one name. + +**Known limits.** The scan (`tests/helpers/planning-add-guard.cjs`) is a token-oriented text scan, +not a shell interpreter, and it targets accidental reintroduction of an unguarded stage by a +contributor editing shipped content — not a determined bypass. Four shapes are confirmed (#3585) +to stage `.planning/` at runtime while scoring zero offenders, and none is a shape GSD content +actually uses: `eval "git add -A"`, `find .planning -type f | xargs git add`, a one-line shell +function body (`f() { git add -A; }`), and a backslash line-continuation split across two physical +lines. The scan also only models `git add` and `git commit -a`/`--all` as staging commands — it +does not recognize `git stash`, `git rm --cached`, `git restore --staged`, or +`git update-index --add`, any of which can also move `.planning/` content into a state a later +commit picks up. + ### CI Test Quality Checks The following checks run on every PR in addition to the test suite: diff --git a/gsd-core/workflows/ai-integration-phase.md b/gsd-core/workflows/ai-integration-phase.md index c87022888..5e24cfbcf 100644 --- a/gsd-core/workflows/ai-integration-phase.md +++ b/gsd-core/workflows/ai-integration-phase.md @@ -258,10 +258,8 @@ Read the completed AI-SPEC.md. Check that: ## 11. Commit -**If `commit_docs` is true:** ```bash -git add "${AI_SPEC_FILE}" -git commit -m "docs({phase_slug}): generate AI-SPEC.md — {primary_framework} + domain context + eval strategy" +gsd_run query commit "docs({phase_slug}): generate AI-SPEC.md — {primary_framework} + domain context + eval strategy" --files "${AI_SPEC_FILE}" ``` ## 12. Display Completion diff --git a/gsd-core/workflows/eval-review.md b/gsd-core/workflows/eval-review.md index f2ce62917..8b121c6e5 100644 --- a/gsd-core/workflows/eval-review.md +++ b/gsd-core/workflows/eval-review.md @@ -143,10 +143,8 @@ Read the written EVAL-REVIEW.md. Extract: ## 6. Commit -**If `commit_docs` is true:** ```bash -git add "${EVAL_REVIEW_FILE}" -git commit -m "docs({phase_slug}): add EVAL-REVIEW.md — score {overall_score}/100 ({verdict})" +gsd_run query commit "docs({phase_slug}): add EVAL-REVIEW.md — score {overall_score}/100 ({verdict})" --files "${EVAL_REVIEW_FILE}" ``` diff --git a/gsd-core/workflows/fast.md b/gsd-core/workflows/fast.md index 7ca9adb93..4a0b64415 100644 --- a/gsd-core/workflows/fast.md +++ b/gsd-core/workflows/fast.md @@ -55,7 +55,16 @@ Do the work directly: Commit the change atomically: ```bash -git add -A +_GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}"; GSD_TOOLS="${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}"; if [ -f "$GSD_TOOLS" ]; then gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif command -v gsd-tools >/dev/null 2>&1; then GSD_TOOLS="$(command -v gsd-tools)"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif [ -f "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd-tools is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; if [ -n "${CLAUDE_ENV_FILE:-}" ] && [ -n "${GSD_TOOLS:-}" ]; then printf "export PATH='%s':\"\$PATH\"\n" "${GSD_TOOLS%/*}" >> "$CLAUDE_ENV_FILE" 2>/dev/null || true; fi +# fast writes no planning artifacts (its own guardrails forbid PLAN.md/SUMMARY.md); .planning/ +# is excluded from staging ONLY when commit_docs is false — otherwise this path is +# byte-identical to an unconditional `git add -A`, unchanged from before #3585. +COMMIT_DOCS=$(gsd_run query config-get commit_docs 2>/dev/null || echo "true") +if [ "$COMMIT_DOCS" = "false" ]; then + git add -A -- ':!.planning' +else + git add -A +fi git commit -m "fix: {concise description of what changed}" ``` @@ -72,7 +81,6 @@ a column count — this replaces the prior inline `awk NF-2` arithmetic that was the root cause of #2133. ```bash -_GSD_SHIM_NAME="gsd-tools.cjs"; _GSD_RUNTIME_ROOT="${RUNTIME_DIR:-$(git rev-parse --show-toplevel 2>/dev/null || pwd)}"; GSD_TOOLS="${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}"; if [ -f "$GSD_TOOLS" ]; then gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif command -v gsd-tools >/dev/null 2>&1; then GSD_TOOLS="$(command -v gsd-tools)"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif [ -f "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; elif [ -f "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}" ]; then GSD_TOOLS="${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd-tools is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; if [ -n "${CLAUDE_ENV_FILE:-}" ] && [ -n "${GSD_TOOLS:-}" ]; then printf "export PATH='%s':\"\$PATH\"\n" "${GSD_TOOLS%/*}" >> "$CLAUDE_ENV_FILE" 2>/dev/null || true; fi # Detect whether STATE.md has a Quick Tasks Completed table if grep -q "Quick Tasks Completed" .planning/STATE.md 2>/dev/null; then gsd_run quick-tasks-append --task "$TASK" || echo "⚠ fast.md log_to_state: could not append Quick Tasks row (see message above); continuing." diff --git a/gsd-core/workflows/new-milestone.md b/gsd-core/workflows/new-milestone.md index fd808c7fa..ba3feb9f4 100644 --- a/gsd-core/workflows/new-milestone.md +++ b/gsd-core/workflows/new-milestone.md @@ -259,9 +259,14 @@ rejects any `--archive-version` value that is not a plain version token (no path Stage the phase archive move + source removal so they land in the same commit as the milestone start (atomic — no orphaned uncommitted deletions, no un-archived dirs carried forward). `phases.clear` archives each non-999 dir to `milestones/-phases/`; staging both dirs captures the new archive and the removals together (#1871). ```bash -git add .planning/milestones/ .planning/phases/ 2>/dev/null || true +COMMIT_DOCS=$(gsd_run query config-get commit_docs 2>/dev/null || echo "true") +if [ "$COMMIT_DOCS" != "false" ]; then + git add .planning/milestones/ .planning/phases/ 2>/dev/null || true +fi ``` +When `commit_docs` is false, the archive move and phase removals are deliberately left unstaged here — not a bug — since Step 6's commit is skipped too. + Stage PROJECT.md in both modes. Step 4's Part A guard — not this commit — is what protects the shared `## Current Milestone` heading (#2308): when a workstream is active Part A never writes it, so the only change PROJECT.md can carry here is Part B's idempotent `## Evolution` backfill, which must be committed rather than stranded as a dangling edit. Do NOT reintroduce a `[ -n "$GSD_WS" ]` branch around this commit: `GSD_WS` is set in Step 1's shell and each step's bash block runs in its own shell (the same reason Step 5 round-trips `OUTGOING_MILESTONE` through a file), so such a guard reads an unset variable, always takes the flat-mode branch, and only appears to work. ```bash diff --git a/gsd-core/workflows/spec-phase.md b/gsd-core/workflows/spec-phase.md index e483875f1..fb95e6e69 100644 --- a/gsd-core/workflows/spec-phase.md +++ b/gsd-core/workflows/spec-phase.md @@ -477,11 +477,10 @@ Write to: `{phase_dir}/{padded_phase}-SPEC.md` ## Step 7: Commit ```bash -git add "${phase_dir}/${padded_phase}-SPEC.md" -git commit -m "spec(phase-${phase_number}): add SPEC.md for ${phase_name} — ${requirement_count} requirements (#2213)" -- "${phase_dir}/${padded_phase}-SPEC.md" +gsd_run query commit "spec(phase-${phase_number}): add SPEC.md for ${phase_name} — ${requirement_count} requirements (#2213)" --files "${phase_dir}/${padded_phase}-SPEC.md" ``` -If `commit_docs` is false: Skip commit. Note that SPEC.md was written but not committed. +If `commit_docs` is false the CLI returns `skipped`; SPEC.md is written, not committed. ## Step 8: Wrap Up diff --git a/tests/commit-docs-bypass.test.cjs b/tests/commit-docs-bypass.test.cjs index 83cf6c654..4aed70652 100644 --- a/tests/commit-docs-bypass.test.cjs +++ b/tests/commit-docs-bypass.test.cjs @@ -1,99 +1,381 @@ /** - * commit_docs bypass guard tests (#1783) + * commit_docs bypass guard (#1783; superseded/widened by #3585) * - * When users set commit_docs: false during /gsd-new-project, .planning/ - * files should never be staged or committed. The gsd-tools.cjs commit - * wrapper already checks this flag, but three locations in execute-phase.md - * and quick.md used raw `git add .planning/` commands that bypassed it. + * #1783: when users set commit_docs: false during /gsd-new-project, + * .planning/ files should never be staged or committed. The gsd-tools.cjs + * commit wrapper already checks this flag, but three locations in + * execute-phase.md and quick.md used raw `git add .planning/` commands that + * bypassed it. * - * These tests verify that every `git add .planning/` invocation (explicit - * or via file_list) is preceded by a commit_docs config check. + * The original guard here was a two-file allowlist (execute-phase.md, + * quick.md) whose regex required the literal substring `.planning/` on the + * SAME line as `git add`. That is structurally blind to `git add -A` / + * `git add .` / `git add -u` — none of those lines mention `.planning/` at + * all, yet every one of them stages the whole index, including + * `.planning/`, regardless of the config. #3585 replaces it with a + * repo-wide scan that classifies `git add` invocations by what they can + * actually reach (the blanket/wildcard forms, `.planning`-qualified paths in + * either separator convention, and any argument carrying an unresolved + * shell variable, fail-closed), across every scan root that carries live + * workflow/agent/command/skill/reference content — not just the original + * two files. */ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); -const fs = require('fs'); const path = require('path'); +const fc = require('fast-check'); +const { + hasReachingGitAdd, scanText, scanRepo, SCAN_ROOTS, +} = require('./helpers/planning-add-guard.cjs'); -const EXECUTE_PHASE_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'execute-phase.md'); -const QUICK_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'quick.md'); +const REPO_ROOT = path.join(__dirname, '..'); -describe('commit_docs bypass guard (#1783)', () => { +describe('commit_docs bypass guard (#1783, repo-wide per #3585)', () => { + // ── Layer 1: the pure classifier, over inline string fixtures ────────── - test('execute-phase.md: every git add .planning/ has a commit_docs guard', () => { - const content = fs.readFileSync(EXECUTE_PHASE_PATH, 'utf-8'); - const lines = content.split(/\r?\n/); + describe('A: argument-reach rules', () => { + test('A1: git add .planning/STATE.md reaches', () => { + assert.strictEqual(hasReachingGitAdd('git add .planning/STATE.md'), true); + }); - for (let i = 0; i < lines.length; i++) { - if (/git add\b.*\.planning\//.test(lines[i])) { - // Search backwards from this line for a config-get commit_docs check - const windowStart = Math.max(0, i - 10); - const window = lines.slice(windowStart, i).join('\n'); - assert.ok( - window.includes('config-get commit_docs'), - `git add .planning/ at line ${i + 1} in execute-phase.md must be guarded by a commit_docs config check` - ); - } - } + test('A2: git add -A reaches', () => { + assert.strictEqual(hasReachingGitAdd('git add -A'), true); + }); + + test('A3: git add --all reaches', () => { + assert.strictEqual(hasReachingGitAdd('git add --all'), true); + }); + + test('A4: git add . reaches', () => { + assert.strictEqual(hasReachingGitAdd('git add .'), true); + }); + + test('A5: git add -u reaches', () => { + assert.strictEqual(hasReachingGitAdd('git add -u'), true); + }); + + test('A6: git add src/api/auth.ts does NOT reach', () => { + assert.strictEqual(hasReachingGitAdd('git add src/api/auth.ts'), false); + }); + + test('A7: git add {test_files} does NOT reach (brace placeholder, no $)', () => { + assert.strictEqual(hasReachingGitAdd('git add {test_files}'), false); + }); + + test('A8: git add "${QUICK_DIR}/x-PLAN.md" reaches (rule c: unresolved variable)', () => { + assert.strictEqual(hasReachingGitAdd('git add "${QUICK_DIR}/x-PLAN.md"'), true); + }); + + test('A9: git add .planning\\STATE.md reaches (Windows separator)', () => { + assert.strictEqual(hasReachingGitAdd('git add .planning\\STATE.md'), true); + }); + + test("A10: git add -A -- ':!.planning' does NOT reach (exclude pathspec overrides -A)", () => { + assert.strictEqual(hasReachingGitAdd("git add -A -- ':!.planning'"), false); + }); + + test('A11: V=$(git add -A) reaches (substitution-glued assignment prefix, #3585)', () => { + assert.strictEqual(hasReachingGitAdd('V=$(git add -A)'), true); + }); + + test('A12: FOO=1 git add -A still reaches (plain assignment prefix, not a substitution)', () => { + assert.strictEqual(hasReachingGitAdd('FOO=1 git add -A'), true); + }); + + test('A13: git -C /tmp/x add -A reaches (git-level -C flag consumes its value, #3585)', () => { + assert.strictEqual(hasReachingGitAdd('git -C /tmp/x add -A'), true); + }); + + test('A14: git --no-pager add -A reaches (a value-less flag must not swallow "add")', () => { + assert.strictEqual(hasReachingGitAdd('git --no-pager add -A'), true); + }); + + test('A15: git -c foo.bar=1 add -A reaches (git-level -c flag consumes its value)', () => { + assert.strictEqual(hasReachingGitAdd('git -c foo.bar=1 add -A'), true); + }); + + test('A16: git add "$(cat list.txt)" reaches (command substitution, #3585 F2)', () => { + assert.strictEqual(hasReachingGitAdd('git add "$(cat list.txt)"'), true); + }); + + test('A17: git add --pathspec-from-file=paths.txt reaches (#3585 F2)', () => { + assert.strictEqual(hasReachingGitAdd('git add --pathspec-from-file=paths.txt'), true); + }); + + test('A18: git commit -a -m "x" reaches (#3585 F3)', () => { + assert.strictEqual(hasReachingGitAdd('git commit -a -m "x"'), true); + }); + + test('A19: git commit -am "x" reaches (combined short cluster, #3585 F3)', () => { + assert.strictEqual(hasReachingGitAdd('git commit -am "x"'), true); + }); + + test('A20: git commit -m "x" does NOT reach (no -a/--all flag, #3585 F3)', () => { + assert.strictEqual(hasReachingGitAdd('git commit -m "x"'), false); + }); }); - test('quick.md: every git add .planning/ has a commit_docs guard', () => { - const content = fs.readFileSync(QUICK_PATH, 'utf-8'); - const lines = content.split(/\r?\n/); + describe('B: invocation recognition', () => { + test('B1: a bare fenced line is an invocation', () => { + assert.strictEqual(hasReachingGitAdd('git add -A'), true); + }); - for (let i = 0; i < lines.length; i++) { - if (/git add\b.*\.planning\//.test(lines[i])) { - const windowStart = Math.max(0, i - 10); - const window = lines.slice(windowStart, i).join('\n'); - assert.ok( - window.includes('config-get commit_docs'), - `git add .planning/ at line ${i + 1} in quick.md must be guarded by a commit_docs config check` - ); - } - } + test('B3: a quoted echo of "git add -A" is NOT an invocation', () => { + assert.strictEqual( + hasReachingGitAdd('[ -n "$X" ] && echo " 1. git add -A && git commit -m \'wip\'" >&2'), + false, + ); + }); + + test('B5: bash -c "git add -A" IS an invocation', () => { + assert.strictEqual(hasReachingGitAdd('bash -c "git add -A"'), true); + }); + + test('B6: echo bash -c "git add -A" is NOT an invocation (echo prints, does not run)', () => { + assert.strictEqual(hasReachingGitAdd('echo bash -c "git add -A"'), false); + }); + + test('B7: cd "$X" && git add -A IS an invocation', () => { + assert.strictEqual(hasReachingGitAdd('cd "$X" && git add -A'), true); + }); + + test('B8: # git add -A (a shell comment) is NOT an invocation', () => { + assert.strictEqual(hasReachingGitAdd('# git add -A'), false); + }); }); - test('quick.md: git add ${file_list} has a commit_docs guard for .planning/ filtering', () => { - const content = fs.readFileSync(QUICK_PATH, 'utf-8'); - const lines = content.split(/\r?\n/); + // ── Layer 1 continued: fence + commit_docs guard state (needs multi-line + // file context, so these drive scanText over small synthetic documents) ── - // Find the line(s) that do `git add ${file_list}` — this variable - // includes .planning/STATE.md so it needs a commit_docs guard too - for (let i = 0; i < lines.length; i++) { - if (/git add\s+\$\{?file_list/.test(lines[i])) { - const windowStart = Math.max(0, i - 10); - const window = lines.slice(windowStart, i + 1).join('\n'); - assert.ok( - window.includes('config-get commit_docs'), - `git add \${file_list} at line ${i + 1} in quick.md must be guarded by a commit_docs check ` + - `because file_list includes .planning/ files` - ); - } - } + describe('C: fence and commit_docs guard tracking', () => { + const fenced = (bodyLines) => ['```bash', ...bodyLines, '```'].join('\n'); + + test('C1: a guard enclosing the add is guarded (no offender)', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'COMMIT_DOCS=$(gsd_run query config-get commit_docs --default true)', + 'if [ "$COMMIT_DOCS" != "false" ]; then', + ' git add .planning/STATE.md', + 'fi', + ])); + assert.deepStrictEqual(offenders, []); + }); + + test('C2: an add BEFORE the if is unguarded', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'git add .planning/STATE.md', + 'if [ "$COMMIT_DOCS" != "false" ]; then', + ' echo noop', + 'fi', + ])); + assert.strictEqual(offenders.length, 1); + }); + + test('C3: an add AFTER the fi is unguarded', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'if [ "$COMMIT_DOCS" != "false" ]; then', + ' echo noop', + 'fi', + 'git add .planning/STATE.md', + ])); + assert.strictEqual(offenders.length, 1); + }); + + test('C4: markdown prose "**If commit_docs is true:**" outside the fence is not a guard', () => { + const text = [ + '**If `commit_docs` is true:**', + '```bash', + 'git add "${EVAL_REVIEW_FILE}"', + '```', + ].join('\n'); + const { offenders } = scanText('fixture.md', text); + assert.strictEqual(offenders.length, 1); + }); + + test('C5: the real eval-review.md shape (prose line immediately before the fence) is not a guard', () => { + const text = [ + '## 6. Commit', + '', + '**If `commit_docs` is true:**', + '```bash', + 'git add "${EVAL_REVIEW_FILE}"', + 'git commit -m "docs: add EVAL-REVIEW.md"', + '```', + ].join('\n'); + const { offenders } = scanText('fixture.md', text); + assert.strictEqual(offenders.length, 1); + }); + + test('C6: a nested if inside the guard stays guarded', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'COMMIT_DOCS=$(gsd_run query config-get commit_docs --default true)', + 'if [ "$COMMIT_DOCS" != "false" ]; then', + ' if [ -f somefile ]; then', + ' git add .planning/STATE.md', + ' fi', + 'fi', + ])); + assert.deepStrictEqual(offenders, []); + }); + + test('C7: a guard in a DIFFERENT fenced block does not carry over', () => { + const text = [ + '```bash', + 'if [ "$COMMIT_DOCS" != "false" ]; then', + '```', + '', + '```bash', + 'git add .planning/STATE.md', + '```', + ].join('\n'); + const { offenders } = scanText('fixture.md', text); + assert.strictEqual(offenders.length, 1); + }); + + test('C8: CRLF line endings behave identically to LF (the C1 fixture, \\r\\n joined)', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'COMMIT_DOCS=$(gsd_run query config-get commit_docs --default true)', + 'if [ "$COMMIT_DOCS" != "false" ]; then', + ' git add .planning/STATE.md', + 'fi', + ]).split('\n').join('\r\n')); + assert.deepStrictEqual(offenders, []); + }); + + test('C9: the = "true" polarity is a guard too (both polarities accepted)', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'COMMIT_DOCS=$(gsd_run query config-get commit_docs --default true)', + 'if [ "$COMMIT_DOCS" = "true" ]; then', + ' git add .planning/STATE.md', + 'fi', + ])); + assert.deepStrictEqual(offenders, []); + }); + + // PIN, not a failing-first regression test (#3585 F6): this assertion + // does not distinguish the unclamped-ifDepth code from the clamped + // fix — a 200k-case differential fuzz over `if`/`fi` nestings found zero + // divergence between them, and this exact fixture passes identically on + // pre-fix code. That is because guardOpenDepth is only ever SET from a + // commit_docs/tracked-var-mentioning `if`, and that guard's own `fi` + // always closes it via a RELATIVE depth comparison regardless of the + // (possibly negative, pre-clamp) baseline. The `Math.max(0, ...)` clamp + // is kept anyway as DEFENSIVELY correct — no input was found, in 200k + // fuzzed cases, that makes the unclamped code diverge from the clamped + // one. This test PINS the malformed-shell ("stray fi") behavior for + // both versions; it does not demonstrate a fix for a reproduced defect. + test('C10: pins malformed-shell behavior — a stray fi before an unrelated if does not manufacture a guard (fails closed)', () => { + const { offenders } = scanText('fixture.md', fenced([ + 'fi', + 'if [ "$X" = "1" ]; then', + ' git add -A', + 'fi', + ])); + assert.strictEqual(offenders.length, 1, 'git add -A must still be reported as unguarded'); + }); }); - test('no raw git add .planning/ without commit_docs guard in any workflow', () => { - const workflows = [ - { name: 'execute-phase.md', path: EXECUTE_PHASE_PATH }, - { name: 'quick.md', path: QUICK_PATH }, - ]; + describe('D: gsd-scan-ignore declaration handling', () => { + const fenced = (line) => ['```bash', line, '```'].join('\n'); - for (const wf of workflows) { - const content = fs.readFileSync(wf.path, 'utf-8'); + test('D1: declared with a tracked issue (#3585) is exempt', () => { + const { offenders, untracked } = scanText( + 'fixture.md', + fenced('git add -A # gsd-scan-ignore: #3585 demonstrating the unscoped shape'), + ); + assert.deepStrictEqual(offenders, []); + assert.deepStrictEqual(untracked, []); + }); - // Find all occurrences of git add that reference .planning/ - const regex = /git add\b[^\r\n]*\.planning\//g; - let match; - while ((match = regex.exec(content)) !== null) { - // Get the 500-char window before this match - const before = content.slice(Math.max(0, match.index - 500), match.index); - assert.ok( - before.includes('config-get commit_docs'), - `${wf.name}: found unguarded git add .planning/ near offset ${match.index}. ` + - `All raw git add .planning/ commands must check commit_docs config first.` - ); - } - } + test('D2: an empty reason is malformed, not exempt', () => { + const { offenders, untracked } = scanText( + 'fixture.md', + fenced('git add -A # gsd-scan-ignore:'), + ); + assert.strictEqual(offenders.length, 1, 'not exempted — still an offender'); + assert.strictEqual(untracked.length, 1, 'and reported as a malformed declaration'); + }); + + test('D3: "just a note" (no tracking reference) is malformed, not exempt', () => { + const { offenders, untracked } = scanText( + 'fixture.md', + fenced('git add -A # gsd-scan-ignore: just a note'), + ); + assert.strictEqual(offenders.length, 1, 'not exempted — still an offender'); + assert.strictEqual(untracked.length, 1, 'and reported as a malformed declaration'); + }); + + test('D4: a marker surviving as an argv token declares nothing', () => { + const { offenders, untracked } = scanText( + 'fixture.md', + fenced('git add .planning/STATE.md "gsd-scan-ignore: #3585"'), + ); + assert.strictEqual(offenders.length, 1, 'plain unguarded offender'); + assert.deepStrictEqual(untracked, [], 'the marker never reached comment position, so no declaration was attempted'); + }); + }); + + // ── Property test (#3585 F5): a parser needs a property test, per + // CLAUDE.md, mirroring the fast-check coverage already carried by the + // sibling classifier in tests/commit-files-pathspec.test.cjs. ─────────── + + describe('P: property — hasReachingGitAdd', () => { + // Bounded alphabet, deliberately small and non-overlapping with the + // trigger set below so a filler token never accidentally reaches on its + // own — every failure the property can find is attributable to the + // trigger token, not to generator noise. + const fillerToken = fc.constantFrom('src/a.ts', 'docs/readme.md', 'foo', 'bar123', 'lib/x.js'); + const blanketFlag = fc.constantFrom('-A', '--all', '.', '-u'); + const planningPath = fc.constantFrom( + '.planning/STATE.md', + '.planning/todos/pending/x.md', + '.planning/ROADMAP.md', + ); + const triggerToken = fc.oneof(blanketFlag, planningPath); + + test('P1: a line with a blanket flag or a .planning-rooted path, and no exclude pathspec, always reaches', () => { + fc.assert( + fc.property( + fc.array(fillerToken, { maxLength: 4 }), + triggerToken, + fc.array(fillerToken, { maxLength: 4 }), + (before, trigger, after) => { + const line = ['git', 'add', ...before, trigger, ...after].join(' '); + assert.strictEqual( + hasReachingGitAdd(line), + true, + `a git add line carrying a blanket flag or a .planning-rooted path, with no ` + + `exclude pathspec, must reach .planning/: ${line}`, + ); + }, + ), + // Pinned seed + bounded numRuns per CONTRIBUTING.md/CLAUDE.md + // determinism rule — deterministic, replayable failures. + { seed: 3585, numRuns: 200 }, + ); + }); + }); + + // ── Layer 2: the real tree ────────────────────────────────────────────── + + test('every git add invocation in shipped content is guarded or declared', () => { + const { offenders, untracked } = scanRepo(REPO_ROOT, SCAN_ROOTS); + + assert.deepStrictEqual( + untracked, + [], + 'gsd-scan-ignore: declarations without a tracking reference. Add a #NNN issue ' + + 'number or an http(s):// URL to the reason, per ADR-456:\n' + + untracked.map((u) => `${u.file}:${u.line}: ${u.text}`).join('\n'), + ); + + assert.deepStrictEqual( + offenders, + [], + 'git add invocations that can reach .planning/ without a commit_docs guard or a ' + + 'tracked gsd-scan-ignore declaration (#1783 / #3585):\n\n' + + offenders.map((o) => `${o.file}:${o.line}: ${o.text}`).join('\n'), + ); }); }); @@ -243,8 +525,13 @@ const REPO_ROOT = path.resolve(__dirname, '..'); const EXECUTOR_AGENT = path.join(REPO_ROOT, 'agents', 'gsd-executor.md'); -// Frozen reason enum mirrors the SDK source — keep in sync with -// `cmdCommit` in gsd-core/bin/lib/commands.cjs. +// Frozen reason enum mirrors the SDK source. Both members are now +// behaviorally pinned against the live envelope returned by `cmdCommit` in +// gsd-core/bin/lib/commands.cjs, not kept in sync by hand-reading the +// source: SKIPPED_COMMIT_DOCS_FALSE by describe block B (B1-B3, explicit +// `commit_docs: false`) and SKIPPED_GITIGNORED by describe block G (G1-G3, +// gitignore auto-detect). A production rename of either string now fails +// these tests instead of drifting silently behind the comment (#3585). const COMMIT_REASON = Object.freeze({ SKIPPED_COMMIT_DOCS_FALSE: 'skipped_commit_docs_false', SKIPPED_GITIGNORED: 'skipped_gitignored', @@ -369,6 +656,87 @@ describe('bug #3678 — executor must respect commit_docs:false', () => { }); }); + describe('G — gitignore auto-detect pins the other COMMIT_REASON member', () => { + // Parity anchor for #3585's Generative Fix Divergence finding: describe + // block B above behaviorally pins SKIPPED_COMMIT_DOCS_FALSE (an explicit + // `commit_docs: false` in config.json), but SKIPPED_GITIGNORED was pinned + // by nothing except the "keep in sync" comment on COMMIT_REASON above — + // production could rename that string and every existing test here would + // still pass. This block drives the OTHER path: `.planning/config.json` + // is left absent entirely (not present-with-key-unset — a present, + // even-empty config.json routes through loadConfigResolved's own + // gitignore auto-detect override, which resolves `commit_docs` to + // `false` and returns SKIPPED_COMMIT_DOCS_FALSE first, never reaching + // the gitignored reason), so `commit_docs` is never set anywhere and the + // skip is driven purely by `.gitignore` containing `.planning/`. + let tmpDir; + + beforeEach(() => { + tmpDir = createTempGitProject(); + // .planning/ already exists from createTempGitProject's setup, and no + // config.json is written — commit_docs is left completely absent. + const gitignorePath = path.join(tmpDir, '.gitignore'); + fs.writeFileSync(gitignorePath, '.planning/\n'); + git(['add', gitignorePath], tmpDir); + git(['commit', '-m', 'chore: add gitignore'], tmpDir); + + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + if (!fs.existsSync(statePath)) { + fs.writeFileSync(statePath, '---\nproject: test\n---\n# State\n'); + } + fs.appendFileSync(statePath, '\n\n'); + }); + + afterEach(() => cleanup(tmpDir)); + + test('G1: commit returns skipped:true with reason SKIPPED_GITIGNORED (parity anchor for the enum member the comment claims is kept in sync)', () => { + const result = runGsdTools( + 'commit "docs(test): noop" --files .planning/STATE.md', + tmpDir, + ); + assert.ok(result.success, `gsd-tools commit should exit 0 even when skipped: ${result.error || ''}`); + const envelope = JSON.parse(result.output); + assert.strictEqual(envelope.skipped, true, 'envelope must carry skipped:true for the gitignore auto-detect path'); + assert.strictEqual( + envelope.reason, + COMMIT_REASON.SKIPPED_GITIGNORED, + 'reason must be the canonical skipped_gitignored code (frozen enum) — this is the ' + + 'behavioral pin for the member describe block B never exercises', + ); + }); + + test('G2: gitignore auto-detect skip leaves the git index empty (no .planning/ staged)', () => { + runGsdTools( + 'commit "docs(test): noop" --files .planning/STATE.md', + tmpDir, + ); + const stagedAll = git(['diff', '--cached', '--name-only'], tmpDir); + const stagedPlanning = stagedAll + .split(/\r?\n/) + .map(s => s.trim()) + .filter(s => s.startsWith('.planning/')); + assert.deepStrictEqual( + stagedPlanning, + [], + 'no .planning/ files should be staged when .planning/ is gitignored', + ); + }); + + test('G3: gitignore auto-detect skip produces no new commits', () => { + const headBefore = git(['rev-parse', 'HEAD'], tmpDir).trim(); + runGsdTools( + 'commit "docs(test): noop" --files .planning/STATE.md', + tmpDir, + ); + const headAfter = git(['rev-parse', 'HEAD'], tmpDir).trim(); + assert.strictEqual( + headAfter, + headBefore, + 'HEAD must not advance when .planning/ is gitignored', + ); + }); + }); + test('checklist carve-out preserved for intentional skip', () => { const body = fs.readFileSync(EXECUTOR_AGENT, 'utf-8'); const checklistLine = body diff --git a/tests/commit-files-pathspec.test.cjs b/tests/commit-files-pathspec.test.cjs index 548e78322..755480a50 100644 --- a/tests/commit-files-pathspec.test.cjs +++ b/tests/commit-files-pathspec.test.cjs @@ -22,6 +22,10 @@ const { createTempGitProject, cleanup, runGsdTools } = require('./helpers.cjs'); const { gitOrThrow } = require('./helpers/git-fixture.cjs'); // #3145: class-norm timeout, not a per-suite value — see helpers/timeouts.cjs. const { GIT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { + bareCommandName, tokenize, shellDashCPayloads, commentPortion, + ISSUE_REF_RE, declarationReason, isDeclared, isUntrackedDeclaration, +} = require('./helpers/shipped-command-scan.cjs'); describe('commit --files: pathspec honors declared scope (#2112)', () => { let tmpDir; @@ -877,160 +881,8 @@ describe('workflow call sites declare --files (#2269)', () => { // // WHAT PRECEDES THE OPENER IS NOT ENUMERABLE, so it is not enumerated. The // first cut of this rule keyed on an assignment prefix, because 437 of the - // 443 live substitution sites are `VAR=$(gsd_run …)`. That is the majority - // and it is the wrong generalisation — the remaining six are neither, and - // they are live today: - // for REVIEW_FLAG in $(gsd_run review-lane flags) (×3) - // ${PLAN_PRE_HOOKS_JSON:-$(gsd_run loop render-hooks …)} (×3) - // A parameter-expansion default glues just as hard as an assignment, and so - // does ordinary text (`pre$(…)`) and an indexed assignment (`A[0]=$(…)`). - // Enumerating prefixes means a new one every time the shell is used - // idiomatically, and each miss is silent. So the rule is positional instead: - // strip everything up to and including the LAST substitution opener in the - // token. Whatever preceded it was, by construction, not the command. - // - // ARITHMETIC IS EXCLUDED BY THE SAME MECHANISM, not by a special case. - // `$((x))` leaves a `(` in front of the name after the strip, and a leading - // `(` is not part of any binary name — so it is refused. That mirrors the - // shell, which also requires `$( (` with a space before it will read a nested - // subshell rather than an arithmetic expansion. - // - // The ARRAY LITERAL stays invisible for free: `arr=(a b c)` contains no `$(` - // at all, so neither alternative fires and the token is left whole. It is - // pinned as a zero-candidate case regardless, because that is a property of - // this regex rather than of the grammar and the next edit could lose it. - // THE STRIP IS REFUSED ON A TOKEN THAT CARRIED A QUOTE, because quoting is - // exactly what decides whether an opener is syntax or data. `printf %s - // '$(gsd_run' query commit fixup` runs no gsd command — the opener is - // single-quoted literal text — but quote removal leaves the token `$(gsd_run` - // and a strip would then read it as a binary, fabricating an invocation out - // of a string. The tokenizer already knows; it just was not saying. - // - // RESIDUAL, named rather than implied: this closes the strip's exposure, not - // the tokenizer's quote-blindness in general. `printf %s 'gsd_run' query - // commit fixup` still reads as an invocation, because no markup is stripped - // there — the token's value simply IS the binary name. That predates this - // rule and is unchanged by it; closing it needs quote provenance carried - // through the command-shape test, not just through the strip. The direction - // is a visible false positive with a declared remedy, not a silent miss. - const bareCommandName = (t) => { - const mask = t.qmask || '0'.repeat(t.value.length); - // The LAST opener whose own two characters were unquoted. Per-character, - // not per-token: a quote elsewhere in the word protects only itself, and - // `echo "pre"$(gsd_run …)` really does run. - for (let i = t.value.length - 2; i >= 0; i -= 1) { - if (t.value[i] === '$' && t.value[i + 1] === '(' && mask[i] === '0' && mask[i + 1] === '0') { - return t.value.slice(i + 2); - } - } - let j = 0; - while (j < t.value.length && t.value[j] === '(' && mask[j] === '0') j += 1; - return t.value.slice(j); - }; const isGsdBinary = (t) => GSD_BINARY_RE.test(bareCommandName(t)); - // Shell-like word splitting. Honours BOTH quote characters (the previous - // parity walk counted only `"`, so a single-quoted message was transparent - // to it in both directions), backslash escapes, and unquoted control - // operators — which become their own tokens, so an operator glued to its - // neighbour (`"a"&&gsd_run`) still separates. Quoted operators and quoted - // whitespace stay literal, which is what keeps a command substitution inside - // a message ("$(gsd-tools query phase-list)") from reading as a second - // invocation, with no separator lookahead needed. - // - // Beyond the operators, the shell consumes THREE more things before argv - // exists, and each one previously leaked into the token list as a value — - // so `--files >/dev/null 2>&1 || true` (a live tail shape in - // gsd-core/references/execute-phase-requirement-revert.md) scored as scoped - // while the runtime saw files=[] and took the blanket-.planning/ default: - // - a single `&` is a control operator like `;` (`&&` is checked first); - // - an unquoted `#` at the START of a word begins a comment and ends the - // command (mid-word `#` stays literal, as in the shell); - // - redirections (`> x`, `>> x`, `2>&1`, `< x`, with or without a space - // before the target) are consumed as `redir: true` tokens, along with a - // glued all-digit IO number, and consumers exclude them from argv. - // Variable tokens ($VAR / ${VAR}) stay ordinary values DELIBERATELY: whether - // one expands to something is unknowable statically, and the three original - // #2269 fix sites all pass `--files "${PHASE_DIR}/…"` — flagging `$`-tokens - // would false-flag every one of them. - // - // Never throws, and an unterminated quote simply runs to end of input: these - // inputs are substrings sliced out of prose, so a torn quote is expected - // rather than exceptional. Tokens carry offsets so callers can slice the - // ORIGINAL text and report a real excerpt instead of a re-joined echo. - // - // Operators are marked STRUCTURALLY (`op: true`), never re-identified by - // comparing a token's text against the operator set. That distinction is the - // whole point of tokenizing: `commit '|' --files a.md` produces a token - // whose VALUE is `|` and which is ordinary message text, and a consumer that - // matched on value would split there and score a scoped invocation as - // unscoped — the identify-structure-by-text mistake this rewrite exists to - // remove, reintroduced one layer up. (Caught by the delimiter-generating - // property below, on its first run.) - const tokenize = (str) => { - const tokens = []; - let cur = ''; - let started = false; - let start = 0; - let quote = null; - // A PER-CHARACTER quote mask, parallel to `cur`: '1' where the character was - // quoted or backslash-escaped (i.e. protected, therefore data), '0' where it - // was bare syntax. A per-TOKEN boolean is the obvious shape and it is wrong - // in the unaffordable direction — `echo "pre"$(gsd_run …)` executes, and a - // token-wide flag suppresses the strip on it, turning a false positive into - // a silent false negative. - let curMask = ''; - const add = (text, protectedChar) => { - cur += text; - curMask += (protectedChar ? '1' : '0').repeat(text.length); - }; - const begin = (i) => { - if (!started) { started = true; start = i; } - }; - const flush = (end) => { - if (started) tokens.push({ value: cur, start, end, qmask: curMask }); - cur = ''; - curMask = ''; - started = false; - }; - for (let i = 0; i < str.length; i += 1) { - const ch = str[i]; - if (quote) { - if (ch === '\\' && quote === '"' && i + 1 < str.length) { add(str[i + 1], true); i += 1; continue; } - if (ch === quote) { quote = null; continue; } - add(ch, true); - continue; - } - if (ch === '\\' && i + 1 < str.length) { begin(i); add(str[i + 1], true); i += 1; continue; } - if (ch === '"' || ch === "'") { begin(i); quote = ch; continue; } - if (/\s/.test(ch)) { flush(i); continue; } - // An unquoted # at the start of a word ends the command line — the rest - // is comment and never reaches argv. Mid-word (`PR#42`) it is literal. - if (ch === '#' && !started) { flush(i); break; } - // Redirections: the operator and its target are consumed by the shell, - // not passed as arguments. A glued all-digit word is an IO number - // (`2>&1`) and belongs to the redirection, not to argv. - if (ch === '>' || ch === '<') { - if (started && /^[0-9]+$/.test(cur)) { cur = ''; curMask = ''; started = false; } else { flush(i); } - let j = i + 1; - if (str[j] === ch) j += 1; // >> / << - if (str[j] === '&') j += 1; // >& (2>&1) - while (j < str.length && /[ \t]/.test(str[j])) j += 1; // gap before target - while (j < str.length && !/[\s;|&<>]/.test(str[j])) j += 1; // the target word - tokens.push({ value: str.slice(i, j), start: i, end: j, redir: true }); - i = j - 1; - continue; - } - const pair = str.slice(i, i + 2); - if (pair === '&&' || pair === '||') { flush(i); tokens.push({ value: pair, start: i, end: i + 2, op: true }); i += 1; continue; } - if (ch === ';' || ch === '|' || ch === '&') { flush(i); tokens.push({ value: ch, start: i, end: i + 1, op: true }); continue; } - begin(i); - add(ch, false); - } - flush(str.length); - return tokens; - }; - // routeCommit's predicate, verbatim, over one invocation's tokens. Stops at // the first control operator so that a later command's --files can never // vouch for this one even when the caller passes an unsegmented line, and @@ -1174,168 +1026,6 @@ describe('workflow call sites declare --files (#2269)', () => { return spans; }; - // The comment portion of a line — the same rule tokenize() applies, so the - // two cannot disagree about where the command ends: an unquoted `#` at word - // start begins a comment, and a `#` inside quotes or mid-word is literal - // (ship.md commits with `PR #${PR_NUMBER}` in the message). - // It tracks WORD START the way tokenize does, not "preceded by whitespace". - // Those differ, and the difference is exploitable: in `commit docs:\ # x` the - // backslash escapes the space, so the shell keeps `docs: #` as ONE word and - // `#` is literal — while a preceded-by-whitespace test reads it as a comment. - // The scan would then exempt a line the runtime still executes unscoped. - const commentPortion = (line) => { - let quote = null; - let started = false; - for (let i = 0; i < line.length; i += 1) { - const ch = line[i]; - if (quote) { - if (ch === '\\' && quote === '"' && i + 1 < line.length) { i += 1; continue; } - if (ch === quote) quote = null; - continue; - } - if (ch === '\\' && i + 1 < line.length) { started = true; i += 1; continue; } - if (ch === '"' || ch === "'") { started = true; quote = ch; continue; } - if (/\s/.test(ch)) { started = false; continue; } - if (ch === '#' && !started) return line.slice(i); - if (ch === '>' || ch === '<' || ch === ';' || ch === '|' || ch === '&') { started = false; continue; } - started = true; - } - return ''; - }; - - // The explicit opt-out for content that shows the defect ON PURPOSE. A - // wrong-example is otherwise indistinguishable from a regression — which is - // exactly why no property of the surrounding markup can stand in for the - // author's intent — so the author declares it, with a reason. - // - // IT IS ONLY A MARKER IN COMMENT POSITION. Matching the token anywhere on - // the line let a commit MESSAGE carry it — `commit "docs: explain - // gsd-scan-ignore: semantics"` — and silently exempt a real offender. A - // false-negative escape hatch is the one thing this file must not ship, and - // an exemption keyed to attacker-controlled-looking text is precisely that. - // AND IT MUST CARRY A TRACKING ISSUE. An exemption whose reason is free text - // has no expiry and no ledger — the `permanent-allow-test-rule` shape - // RULESET.TESTS.delete-bad-tests names. The repo already answered this for - // its sibling convention under ADR-456 (see #2269), enforced by - // scripts/lint-allow-test-rule-refs. That lint walks tests/ only, so it - // cannot see a marker living in a .md file; rather than teach a second token - // to a script whose whole contract is the ESLint comment form, this scan - // enforces the same rule over its own roots — it already runs in CI on every - // shard. - // - // THE PREDICATE IS THAT LINT'S, MIRRORED RATHER THAN RESTATED. Its - // ISSUE_REF_RE is `/#\d+|https?:\/\//` — `http://` counts, and `#\d+` has no - // trailing boundary. A hand-written near-copy drifts from it in exactly those - // details, and a contributor who satisfies one marker and not the other has - // been given two conventions wearing one name. (Both details were found by - // review against prose here that claimed HTTPS-only. The regex was right and - // the sentence describing it was not, which is its own lesson.) - const ISSUE_REF_RE = /#\d+|https?:\/\//; - // - // The reason is EXTRACTED, not pattern-matched in one shot, so that a marker - // with no reason at all is still recognized as an ATTEMPT. A rejected - // declaration must not fail as a MYSTERY: left un-exempt and nothing more, - // its author is told their commit is unscoped — a true statement about a line - // they had already explained, which is the mangle-until-it-shuts-up loop the - // marker exists to prevent. `# gsd-scan-ignore:` with an empty reason is the - // likeliest way to get this wrong and so is the case that most needs the - // specific diagnosis; a `\S`-requiring pattern silently drops it. - const SCAN_IGNORE_REASON_RE = /gsd-scan-ignore:(.*)$/; - const declarationReason = (portion) => { - const m = SCAN_IGNORE_REASON_RE.exec(portion); - return m === null ? null : m[1]; - }; - // Two independent conditions, and the second is the structural one: the - // marker must be in comment position AND must not survive tokenization as an - // ARGUMENT. tokenize() drops everything from a real comment onward, so a - // genuine declaration leaves no token carrying the token; anything that does - // reached argv, which means the runtime would have executed it. That makes - // "authored argument text can declare the line exempt" impossible by - // construction rather than by getting commentPortion's edges exactly right — - // and commentPortion has edges (redirection targets, escaped separators) - // where mirroring the tokenizer perfectly is fiddly and a miss is silent. - // The marker's reason, but only when the marker is in COMMENT position and - // survives neither as an argv token. Returns null when there is no attempt. - const declaredReasonFor = (line) => { - if (tokenize(line).some((t) => /gsd-scan-ignore:/.test(t.value))) return null; - return declarationReason(commentPortion(line)); - }; - const isDeclared = (line) => { - const reason = declaredReasonFor(line); - return reason !== null && ISSUE_REF_RE.test(reason); - }; - // An ATTEMPTED declaration that carries no tracking reference — including one - // with no reason at all. Reported on its own terms rather than silently - // failing to exempt: see the extraction note above. - const isUntrackedDeclaration = (line) => { - const reason = declaredReasonFor(line); - return reason !== null && !ISSUE_REF_RE.test(reason); - }; - - // A shell invoked with -c runs its next argument AS A COMMAND, so the - // invocation lives inside a quoted token and no amount of markup-stripping - // reaches it: `bash -c "gsd_run query commit fixup"` scored zero candidates - // while being perfectly executable. The recursion is keyed on the INVOKER, - // not on "a quoted token that parses as an invocation" — that wider rule - // would flag a commit MESSAGE quoting a command, a false positive against - // ordinary documentation. Here the outer command is bash, so a gsd commit - // message can never reach it. - // The invoker set, widened past the four spellings the first cut guessed at. - // `ash`, `csh`, `tcsh`, `fish` and `yash` all take -c and all run what - // follows; leaving them out is a silent false negative in a function whose - // entire job is reaching a command the tokenizer cannot see. - const SHELL_INVOKER_RE = /^(?:.*\/)?(?:ba|da|k|z|a|c|tc|fi|ya)?sh$/; - const shellDashCPayloads = (str) => { - const payloads = []; - let group = []; - const drain = () => { - if (!group.length) { return; } - // THE INVOKER MUST BE THE COMMAND, not merely present in the segment. - // Searching anywhere made `echo bash -c "gsd_run query commit fixup"` a - // candidate — `echo` prints the string, it does not run it — which is a - // false positive against prose that merely quotes a command line. Only an - // env assignment, a shell keyword, or a list/prompt marker may precede the - // command name. Those are not commands; `echo` is, which is exactly the - // line the search-anywhere version got wrong. - // Shell keywords, prompt/list markers, and the command MODIFIERS that - // pass straight through to the command after them. The modifiers are the - // mirror of the invoker-set gap: omitting them is a false negative in the - // same direction, since `time bash -c "…"` really does run the payload. - // - // RESIDUAL, named rather than half-solved: a modifier that takes its OWN - // flags (`sudo -u alice bash -c …`) still stops the search, because - // skipping arbitrary flag/value pairs here would mean modelling each - // modifier's option grammar. No live instance exists in the six roots; - // the failure is a missed candidate, and the declaration marker is not - // involved either way. - const NON_COMMAND_PREFIX = new Set([ - 'then', 'else', 'do', '$', '-', '*', '&&', '||', - 'time', 'exec', 'nohup', 'env', 'command', - ]); - // AN ASSIGNMENT IS A SKIPPABLE PREFIX ONLY WHEN IT IS NOT A SUBSTITUTION. - // `FOO=1 bash -c "…"` prefixes the command; `V=$(bash -c "…")` IS the - // command, and skipping it walks the search past the invoker onto `-c`, - // which is not one — so the payload is never reached. Found by sweeping - // the substitution-glues-to-the-binary class across every command-name - // test rather than only the one the finding named. - const skippable = (t) => t.redir - || (/^[A-Za-z_][A-Za-z0-9_]*(\[[^\]]*\])?=/.test(t.value) && !t.value.includes('$(')) - || NON_COMMAND_PREFIX.has(t.value); - const gi = group.findIndex((t) => !skippable(t)); - if (gi !== -1 && SHELL_INVOKER_RE.test(bareCommandName(group[gi]))) { - const ci = group.findIndex((t, k) => k > gi && !t.redir && t.value === '-c'); - const payload = ci !== -1 ? group.slice(ci + 1).find((t) => !t.redir) : undefined; - if (payload) payloads.push(payload.value); - } - group = []; - }; - for (const t of tokenize(str)) { - if (t.op) { drain(); } else { group.push(t); } - } - drain(); - return payloads; - }; - // Candidates for one logical line: the whole line, unioned with each of its // inline code spans and each shell -c payload. See the header for why the // union rather than a choice. diff --git a/tests/emitted-drift-acks/3585-planning-commit-guard.json b/tests/emitted-drift-acks/3585-planning-commit-guard.json new file mode 100644 index 000000000..f5af31b35 --- /dev/null +++ b/tests/emitted-drift-acks/3585-planning-commit-guard.json @@ -0,0 +1,11 @@ +{ + "version": 1, + "paths": { + "fast.md": { + "reason": "#3585: the commit step ran `git add -A`, sweeping .planning/ into every /gsd:fast commit regardless of commit_docs. Gating it needs `gsd_run` in that block, so the launcher preamble was MOVED from the later log_to_state step into the commit step (moved, not duplicated — log_to_state now relies on the one-preamble-per-workflow convention every other workflow uses). Growth is the preamble's new position plus four lines of guard; the commit_docs=true path stays byte-identical to the previous unconditional `git add -A`." + }, + "new-milestone.md": { + "reason": "#3585: `git add .planning/milestones/ .planning/phases/` was ungated, and the correctly-gated `query commit` that follows it skips under commit_docs:false — leaving those paths in the index for the next commit to absorb. Growth is the executable commit_docs guard wrapping the stage, plus one sentence recording that the unstaged archive move is deliberate rather than a bug." + } + } +} diff --git a/tests/helpers/planning-add-guard.cjs b/tests/helpers/planning-add-guard.cjs new file mode 100644 index 000000000..44a60d6f8 --- /dev/null +++ b/tests/helpers/planning-add-guard.cjs @@ -0,0 +1,395 @@ +'use strict'; + +/** + * Repo-wide `git add` -> `.planning/` reach guard (#1783, superseding the + * per-file `execute-phase.md`/`quick.md` scan; extended for #3585). + * + * The prior guard (tests/commit-docs-bypass.test.cjs before this module + * existed) matched a hardcoded regex requiring the literal substring + * `.planning/` on the SAME line as `git add`, over exactly two allowlisted + * files. That is structurally blind to `git add -A` / `git add .` / `git add + * -u` (no `.planning/` text on the line at all, yet every one of them stages + * the whole index including `.planning/`), and it never looked at any file + * outside its two-item allowlist. + * + * This module classifies `git add` (and `git commit -a`/`--all`) invocations + * INSIDE fenced code blocks across the repo's live + * workflow/agent/command/skill/reference surface and decides, per + * invocation, whether it can reach `.planning/` at all — covering the + * wildcard/blanket forms, `.planning`-qualified paths (either path-separator + * convention), any argument carrying an unresolved shell variable, a + * command/backtick substitution, or a `--pathspec-from-file` argument + * (fail-closed in all four cases: none of them is statically resolvable, and + * any one might expand to something planning-rooted). A `git commit -a` + * (including a combined short cluster like `-am`) is classified the same way + * `git add -A` is, because `-a` stages every tracked modification — + * including tracked `.planning/` files — before the commit runs (#3585 F3). + * An invocation that reaches is a violation UNLESS it sits inside an open + * `commit_docs` conditional in the SAME fenced block, carries an exclude + * pathspec naming `.planning`, or the line carries a tracked + * `# gsd-scan-ignore: #NNN` declaration (shared machinery — see + * tests/helpers/shipped-command-scan.cjs). + * + * ## Known limits + * + * This guard is a lightweight, line/token-oriented scan, not a shell + * interpreter — it targets ACCIDENTAL reintroduction of an unguarded + * `.planning/`-reaching stage by a contributor editing shipped content, not + * a determined attempt to defeat it. An isolated security review (#3585) + * empirically confirmed the following shapes stage `.planning/` at runtime + * while scoring ZERO offenders here, and none of them is a shape GSD + * workflow content actually uses — chasing them means writing a shell + * interpreter: + * - `eval "git add -A"` — the invocation lives inside a string literal + * `eval` re-parses at runtime, not inside argv this scan can see. + * - `find .planning -type f | xargs git add` — the reaching argument + * arrives via a pipeline at runtime, never appearing as a literal + * `git add` argument in the source text. + * - `f() { git add -A; }` — a one-line shell function body; this scan has + * no notion of function definitions or of a later, unseen call site. + * - a backslash line-continuation splitting one logical command across two + * physical lines — this scan is line-based and never rejoins them. + * Separately, and independently of the above, this module models only + * `git add` and `git commit -a`/`--all` as staging commands. It does NOT + * model `git stash`, `git rm --cached`, `git restore --staged`, or + * `git update-index --add` — each of these can also move `.planning/` files + * into a state a subsequent commit picks up, and none of them is recognized + * by this scan. + */ + +const fs = require('fs'); +const path = require('path'); +const { + tokenize, bareCommandName, shellDashCPayloads, isDeclared, isUntrackedDeclaration, +} = require('./shipped-command-scan.cjs'); + +// A fenced code block opens/closes on a line whose TRIMMED form starts with +// a run of 3+ backticks or 3+ tildes. Only content INSIDE such a block is +// executable shell — everything else (prose, inline `git add` mentions, bare +// bullet lists) is documentation and is never a candidate. +const FENCE_RE = /^(`{3,}|~{3,})/; + +// Shell prefixes that may legitimately precede the git binary itself without +// being the command: keywords that introduce a new command position, the +// modifiers that pass straight through to what follows, and the prompt/list +// markers a doc author might glue onto an example line. Kept in sync (by +// hand, deliberately — this file's domain is narrower than +// shellDashCPayloads's invoker search, so the sets are not shared) with the +// spec's literal list. +const NON_COMMAND_PREFIX = new Set(['then', 'else', 'do', 'time', 'exec', 'nohup', 'env', 'command', '$', '-', '*']); + +// AN ASSIGNMENT IS A SKIPPABLE PREFIX ONLY WHEN IT IS NOT A SUBSTITUTION — +// deliberately the same rule as shellDashCPayloads's `skippable` in +// shipped-command-scan.cjs (see its comment for the `V=$(git add -A)` vs +// `FOO=1 git add -A` distinction). This divergence is precisely what the +// shared-helper extraction (#3585) was meant to prevent; kept duplicated +// here only because this file's domain (git-add reach) is narrower than +// that file's invoker search, per the NON_COMMAND_PREFIX comment above. +const isSkippable = (t) => t.redir + || (/^[A-Za-z_][A-Za-z0-9_]*(\[[^\]]*\])?=/.test(t.value) && !t.value.includes('$(')) + || NON_COMMAND_PREFIX.has(t.value); + +// One line, split into shell segments (top-level `;`/`&`/`&&`/`||` +// boundaries), unioned with every `shellDashCPayloads` extraction so +// `bash -c "git add -A"` is reached exactly as a bare `git add -A` is. Each +// segment carries its own SOURCE TEXT (the line, or the extracted payload +// string) so a classifier can slice raw, unescaped substrings out of it — +// see reachesPlanning's comment for why that matters. +const collectSegments = (line) => { + const sources = [line, ...shellDashCPayloads(line)]; + const segments = []; + for (const src of sources) { + let group = []; + for (const tok of tokenize(src)) { + if (tok.op) { + if (group.length) segments.push({ tokens: group, src }); + group = []; + } else { + group.push(tok); + } + } + if (group.length) segments.push({ tokens: group, src }); + } + return segments; +}; + +// The RAW, as-authored substring behind a token — start/end are source +// offsets tokenize() carries regardless of what it did to escape sequences +// while building `t.value`. This matters for exactly one reason: tokenize() +// applies real (POSIX) unquoted-backslash-escape semantics, which consumes a +// literal `\` before whatever follows it (`.planning\STATE.md` dequotes to +// `.planningSTATE.md`). That is correct for what a POSIX shell would actually +// execute, but this scan is reading DOCUMENTATION that may show a Windows +// path verbatim — the separator itself is the thing being tested for, and +// the escape-eaten `t.value` would hide it. Slicing the original text instead +// preserves it. +const rawSlice = (src, t) => src.slice(t.start, t.end); + +// Reach rules, in the order the spec states them. `-A`/`--all`/`.`/`-u` are +// exact-flag matches against the dequoted value (no escape/quote ambiguity +// possible for a bare flag); the `.planning` substring and the unresolved- +// variable check both read the RAW slice for the reason above. +// +// THE EXCLUDE PATHSPEC IS A FULL OVERRIDE, not just a veto on rule (b). Git's +// own semantics are why: `git add -A -- ':!.planning'` stages everything +// EXCEPT `.planning/`, so an exclude pathspec naming `.planning` neutralizes +// the wildcard/blanket forms too, not only a literal `.planning` path +// argument. Checked first, over every arg, before any reach rule fires. +// Shared with the `git commit -a` check below (#3585 F3) — the same +// pathspec override applies to a commit invocation carrying an exclude arg. +const hasPlanningExcludePathspec = (argTokens, src) => argTokens.some( + (t) => /:!.*\.planning/.test(rawSlice(src, t)), +); + +const reachesPlanning = (argTokens, src) => { + if (hasPlanningExcludePathspec(argTokens, src)) return false; + for (const t of argTokens) { + if (t.value === '-A' || t.value === '--all' || t.value === '.' || t.value === '-u') return true; + } + for (const t of argTokens) { + const raw = rawSlice(src, t); + // Either path-separator convention: `.planning/x`, `.planning\x`, or a + // bare `.planning` token with nothing after it. + if (/\.planning(?:[/\\]|$)/.test(raw)) return true; + // An unresolved shell variable ($VAR / ${VAR}). `{placeholder}` with NO + // `$` is doc notation, not a shell variable, and must NOT trigger this — + // the regex requires the `$` explicitly so it never does. + if (/\$\{?[A-Za-z_]/.test(raw)) return true; + // Command substitution ($( or backtick) — equally opaque to static + // analysis as $VAR/${VAR} above, and for the same reason: whatever it + // expands to might be planning-rooted, and this scan cannot run the + // shell to find out. Fail closed (#3585 F2). + if (raw.includes('$(') || raw.includes('`')) return true; + // --pathspec-from-file names an external file listing the paths to + // stage; the file's contents are exactly as unknowable statically as an + // unresolved shell variable, so this also fails closed (#3585 F2). + if (/^--pathspec-from-file(?:=|$)/.test(raw)) return true; + } + return false; +}; + +// One segment -> null (not a git-add invocation) or { reaches, argTokens }. +// "git" is recognized by bareCommandName so a substitution/subshell-glued +// spelling still resolves (`$(git add -A)`), and by a trailing `/git` so an +// absolute/relative invocation (`/usr/bin/git add -A`) is still caught. +const classifySegment = ({ tokens, src }) => { + let ci = -1; + for (let i = 0; i < tokens.length; i += 1) { + if (isSkippable(tokens[i])) continue; + ci = i; + break; + } + if (ci === -1) return null; + const bare = bareCommandName(tokens[ci]); + if (bare !== 'git' && !/\/git$/.test(bare)) return null; + + // A flag's VALUE is not itself a flag. `git -C add -A` must still + // reach `add` — `-C`, `--git-dir`, `--work-tree`, `--namespace`, and `-c` + // (in their SEPARATE-value spellings) all consume the next token as their + // argument. The glued forms (`--git-dir=

`, `-C

`) already carry + // their value in the same token and need no extra consumption; an explicit + // set (rather than "consume the token after every flag") is what keeps + // `git --no-pager add -A` from swallowing `add` as `--no-pager`'s value. + const GIT_VALUE_FLAGS = new Set(['-C', '-c', '--git-dir', '--work-tree', '--namespace']); + let addIdx = -1; + for (let i = ci + 1; i < tokens.length; i += 1) { + if (tokens[i].redir) continue; + if (tokens[i].value.startsWith('-')) { + if (GIT_VALUE_FLAGS.has(tokens[i].value)) i += 1; // consume the flag's separate value + continue; // a flag on git itself; keep looking + } + addIdx = i; + break; + } + if (addIdx === -1) return null; + const subcommand = tokens[addIdx].value; + if (subcommand !== 'add' && subcommand !== 'commit') return null; + + const argTokens = tokens.slice(addIdx + 1).filter((t) => !t.redir); + + // tokenize() has no notion of subshell structure: `V=$(git add -A)`'s + // closing `)` is not consumed as syntax, it is just another character + // glued onto whatever token happens to be last (`-A)`). That is invisible + // to the substring-based reach rules (`.planning`, `$VAR`) but breaks the + // EXACT-flag comparisons (`t.value === '-A'`) reachesPlanning also relies + // on. Only strip it when the command itself was resolved via a `$(` glue + // (tokens[ci] carrying `$(` is how bareCommandName found "git" at all — + // see the comment above), and only the one trailing, unquoted `)` that + // substitution's own opener is owed. + if (tokens[ci].value.includes('$(') && argTokens.length) { + const last = argTokens[argTokens.length - 1]; + const mask = last.qmask || '0'.repeat(last.value.length); + if (last.value.endsWith(')') && mask[mask.length - 1] === '0') { + argTokens[argTokens.length - 1] = { ...last, value: last.value.slice(0, -1) }; + } + } + + if (subcommand === 'commit') { + // `git commit -a`/`--all` (including a combined short cluster like + // `-am`) stages every tracked modification, which includes tracked + // `.planning/` files, so it bypasses the gate exactly as `git add -A` + // does (#3585 F3). A short cluster is any single-dash, letters-only + // flag that carries `a` among its letters (`-am`, `-ma`); `--amend` is + // a distinct double-dash flag and is never matched by this pattern. + const hasCommitAllFlag = argTokens.some((t) => { + if (t.value === '--all') return true; + return /^-[a-zA-Z]+$/.test(t.value) && t.value.includes('a'); + }); + if (!hasCommitAllFlag) return { reaches: false }; + if (hasPlanningExcludePathspec(argTokens, src)) return { reaches: false }; + return { reaches: true }; + } + + return { reaches: reachesPlanning(argTokens, src) }; +}; + +// Whether LINE, considered in isolation (no fence/guard context), contains +// at least one git-add invocation whose arguments reach `.planning/`. Used +// directly by the pure per-line classifier tests; the file scanner below +// layers fence and commit_docs-guard state on top of this. +const hasReachingGitAdd = (line) => collectSegments(line).some((seg) => { + const result = classifySegment(seg); + return result !== null && result.reaches; +}); + +// A `VAR=$(... config-get commit_docs ...)` assignment — the shell-variable +// half of the guard trigger. Matched on the TRIMMED line; `config-get +// commit_docs` may carry trailing flags (`--default true`) after the name. +const CONFIG_GET_ASSIGN_RE = /^([A-Za-z_][A-Za-z0-9_]*)=.*config-get\s+commit_docs\b/; + +/** + * Scan one document's TEXT for unguarded, undeclared `git add` invocations + * that can reach `.planning/`. + * + * State machine, one pass over `text.split(/\r?\n/)`: + * - fence tracking: only lines inside a ``` or ~~~ block are candidates. + * - inside a fence, an `if`/`fi` depth counter (first-token match) tracks + * nesting; a commit_docs guard OPENS at the depth an `if` is entered when + * its condition text mentions `commit_docs` or a tracked config-get + * variable, and CLOSES the first time depth drops below the depth it + * opened at (so a nested `if`/`fi` inside the guard leaves it open, and + * an `else` branch of the SAME `if` stays covered — the guard tracks the + * conditional's extent, not which branch is truthy). + * - guard state (depth, open-guard, tracked vars) resets at every fence + * boundary: each fenced block is its own shell, so a guard opened in one + * block can never protect a `git add` in a different one. + */ +const scanText = (file, text) => { + const lines = text.split(/\r?\n/); + const offenders = []; + const untracked = []; + + let inFence = false; + let fenceChar = null; + let fenceLen = 0; + let ifDepth = 0; + let guardOpenDepth = null; + let guardVars = new Set(); + + const resetGuardState = () => { + ifDepth = 0; + guardOpenDepth = null; + guardVars = new Set(); + }; + + for (let i = 0; i < lines.length; i += 1) { + const raw = lines[i]; + const trimmed = raw.trim(); + const fenceMatch = trimmed.match(FENCE_RE); + + if (!inFence) { + if (fenceMatch) { + inFence = true; + fenceChar = fenceMatch[1][0]; + fenceLen = fenceMatch[1].length; + resetGuardState(); + } + continue; + } + + // Inside a fence: a same-character run at least as long as the opener, + // with nothing else on the line, closes it. + if (fenceMatch && fenceMatch[1][0] === fenceChar && fenceMatch[1].length >= fenceLen + && /^(`+|~+)\s*$/.test(trimmed)) { + inFence = false; + fenceChar = null; + fenceLen = 0; + resetGuardState(); + continue; + } + + // A `# gsd-scan-ignore:` attempt with no tracking reference is a + // malformed declaration — reported on its own terms, never silently + // folded into "unguarded" (see shipped-command-scan.cjs's declaration + // comment for why the diagnosis must be specific). + if (isUntrackedDeclaration(raw)) { + untracked.push({ file, line: i + 1, text: raw.trim() }); + } + + const assignMatch = trimmed.match(CONFIG_GET_ASSIGN_RE); + if (assignMatch) guardVars.add(assignMatch[1]); + + const firstToken = trimmed.split(/\s+/)[0] || ''; + if (firstToken === 'if') { + ifDepth += 1; + const cond = trimmed.slice(firstToken.length); + const mentionsCommitDocs = /commit_docs/.test(cond); + const mentionsTrackedVar = [...guardVars].some( + (v) => new RegExp(`\\$\\{?${v}\\b`).test(cond), + ); + if ((mentionsCommitDocs || mentionsTrackedVar) && guardOpenDepth === null) { + guardOpenDepth = ifDepth; + } + } else if (firstToken === 'fi') { + // Clamped, not raw decrement. A stray/unbalanced `fi` must fail CLOSED + // (report), never open: an unclamped negative ifDepth lets a LATER + // `if` set guardOpenDepth = 0, and the guard test `guardOpenDepth !== + // null` reads zero as an OPEN guard — silently guarding an unrelated + // `git add -A` that follows. + ifDepth = Math.max(0, ifDepth - 1); + if (guardOpenDepth !== null && ifDepth < guardOpenDepth) guardOpenDepth = null; + } + + const guarded = guardOpenDepth !== null; + if (!guarded && hasReachingGitAdd(raw) && !isDeclared(raw)) { + offenders.push({ file, line: i + 1, text: raw.trim() }); + } + } + + return { offenders, untracked }; +}; + +// The repo-wide walk: every `.md` file (recursive) under each scan root. +const SCAN_ROOTS = [ + 'gsd-core/workflows', + 'gsd-core/references', + 'agents', + 'commands', + 'skills', +]; + +const scanRepo = (repoRoot, roots = SCAN_ROOTS) => { + const offenders = []; + const untracked = []; + for (const root of roots) { + const rootDir = path.join(repoRoot, root); + if (!fs.existsSync(rootDir)) continue; + const mdFiles = fs.readdirSync(rootDir, { recursive: true }).filter((f) => String(f).endsWith('.md')); + for (const file of mdFiles) { + const normalized = String(file).split(path.sep).join('/'); + const label = `${root}/${normalized}`; + const text = fs.readFileSync(path.join(rootDir, String(file)), 'utf-8'); + const result = scanText(label, text); + offenders.push(...result.offenders); + untracked.push(...result.untracked); + } + } + return { offenders, untracked }; +}; + +module.exports = { + hasReachingGitAdd, + scanText, + scanRepo, + SCAN_ROOTS, +}; diff --git a/tests/helpers/shipped-command-scan.cjs b/tests/helpers/shipped-command-scan.cjs new file mode 100644 index 000000000..2f8e86799 --- /dev/null +++ b/tests/helpers/shipped-command-scan.cjs @@ -0,0 +1,343 @@ +'use strict'; + +/** + * Extracted from tests/commit-files-pathspec.test.cjs (#2269) so the #3585 + * commit_docs guard consumes ONE shell tokenizer rather than hand-rolling a + * second. Only the primitives BOTH guards need live here — the shell + * tokenizer, the bare-command-name extraction, the -c payload search, and + * the `gsd-scan-ignore:` declaration machinery. + * + * Commit-specific logic stays in the test file and is deliberately NOT + * moved here: `isCommitInvocation`, `hasScopedFiles`, `SYNOPSIS_TOKEN_RE`, + * `METAVAR_REDIR_RE`, `segmentInvocations`, `codeSpans`, and + * `invocationCandidates` all encode assumptions specific to the commit + * scan and do not generalize to other guards. + */ + +// 443 live substitution sites are `VAR=$(gsd_run …)`. That is the majority +// and it is the wrong generalisation — the remaining six are neither, and +// they are live today: +// for REVIEW_FLAG in $(gsd_run review-lane flags) (×3) +// ${PLAN_PRE_HOOKS_JSON:-$(gsd_run loop render-hooks …)} (×3) +// A parameter-expansion default glues just as hard as an assignment, and so +// does ordinary text (`pre$(…)`) and an indexed assignment (`A[0]=$(…)`). +// Enumerating prefixes means a new one every time the shell is used +// idiomatically, and each miss is silent. So the rule is positional instead: +// strip everything up to and including the LAST substitution opener in the +// token. Whatever preceded it was, by construction, not the command. +// +// ARITHMETIC IS EXCLUDED BY THE SAME MECHANISM, not by a special case. +// `$((x))` leaves a `(` in front of the name after the strip, and a leading +// `(` is not part of any binary name — so it is refused. That mirrors the +// shell, which also requires `$( (` with a space before it will read a nested +// subshell rather than an arithmetic expansion. +// +// The ARRAY LITERAL stays invisible for free: `arr=(a b c)` contains no `$(` +// at all, so neither alternative fires and the token is left whole. It is +// pinned as a zero-candidate case regardless, because that is a property of +// this regex rather than of the grammar and the next edit could lose it. +// THE STRIP IS REFUSED ON A TOKEN THAT CARRIED A QUOTE, because quoting is +// exactly what decides whether an opener is syntax or data. `printf %s +// '$(gsd_run' query commit fixup` runs no gsd command — the opener is +// single-quoted literal text — but quote removal leaves the token `$(gsd_run` +// and a strip would then read it as a binary, fabricating an invocation out +// of a string. The tokenizer already knows; it just was not saying. +// +// RESIDUAL, named rather than implied: this closes the strip's exposure, not +// the tokenizer's quote-blindness in general. `printf %s 'gsd_run' query +// commit fixup` still reads as an invocation, because no markup is stripped +// there — the token's value simply IS the binary name. That predates this +// rule and is unchanged by it; closing it needs quote provenance carried +// through the command-shape test, not just through the strip. The direction +// is a visible false positive with a declared remedy, not a silent miss. +const bareCommandName = (t) => { + const mask = t.qmask || '0'.repeat(t.value.length); + // The LAST opener whose own two characters were unquoted. Per-character, + // not per-token: a quote elsewhere in the word protects only itself, and + // `echo "pre"$(gsd_run …)` really does run. + for (let i = t.value.length - 2; i >= 0; i -= 1) { + if (t.value[i] === '$' && t.value[i + 1] === '(' && mask[i] === '0' && mask[i + 1] === '0') { + return t.value.slice(i + 2); + } + } + let j = 0; + while (j < t.value.length && t.value[j] === '(' && mask[j] === '0') j += 1; + return t.value.slice(j); +}; + +// Shell-like word splitting. Honours BOTH quote characters (the previous +// parity walk counted only `"`, so a single-quoted message was transparent +// to it in both directions), backslash escapes, and unquoted control +// operators — which become their own tokens, so an operator glued to its +// neighbour (`"a"&&gsd_run`) still separates. Quoted operators and quoted +// whitespace stay literal, which is what keeps a command substitution inside +// a message ("$(gsd-tools query phase-list)") from reading as a second +// invocation, with no separator lookahead needed. +// +// Beyond the operators, the shell consumes THREE more things before argv +// exists, and each one previously leaked into the token list as a value — +// so `--files >/dev/null 2>&1 || true` (a live tail shape in +// gsd-core/references/execute-phase-requirement-revert.md) scored as scoped +// while the runtime saw files=[] and took the blanket-.planning/ default: +// - a single `&` is a control operator like `;` (`&&` is checked first); +// - an unquoted `#` at the START of a word begins a comment and ends the +// command (mid-word `#` stays literal, as in the shell); +// - redirections (`> x`, `>> x`, `2>&1`, `< x`, with or without a space +// before the target) are consumed as `redir: true` tokens, along with a +// glued all-digit IO number, and consumers exclude them from argv. +// Variable tokens ($VAR / ${VAR}) stay ordinary values DELIBERATELY: whether +// one expands to something is unknowable statically, and the three original +// #2269 fix sites all pass `--files "${PHASE_DIR}/…"` — flagging `$`-tokens +// would false-flag every one of them. +// +// Never throws, and an unterminated quote simply runs to end of input: these +// inputs are substrings sliced out of prose, so a torn quote is expected +// rather than exceptional. Tokens carry offsets so callers can slice the +// ORIGINAL text and report a real excerpt instead of a re-joined echo. +// +// Operators are marked STRUCTURALLY (`op: true`), never re-identified by +// comparing a token's text against the operator set. That distinction is the +// whole point of tokenizing: `commit '|' --files a.md` produces a token +// whose VALUE is `|` and which is ordinary message text, and a consumer that +// matched on value would split there and score a scoped invocation as +// unscoped — the identify-structure-by-text mistake this rewrite exists to +// remove, reintroduced one layer up. (Caught by the delimiter-generating +// property below, on its first run.) +const tokenize = (str) => { + const tokens = []; + let cur = ''; + let started = false; + let start = 0; + let quote = null; + // A PER-CHARACTER quote mask, parallel to `cur`: '1' where the character was + // quoted or backslash-escaped (i.e. protected, therefore data), '0' where it + // was bare syntax. A per-TOKEN boolean is the obvious shape and it is wrong + // in the unaffordable direction — `echo "pre"$(gsd_run …)` executes, and a + // token-wide flag suppresses the strip on it, turning a false positive into + // a silent false negative. + let curMask = ''; + const add = (text, protectedChar) => { + cur += text; + curMask += (protectedChar ? '1' : '0').repeat(text.length); + }; + const begin = (i) => { + if (!started) { started = true; start = i; } + }; + const flush = (end) => { + if (started) tokens.push({ value: cur, start, end, qmask: curMask }); + cur = ''; + curMask = ''; + started = false; + }; + for (let i = 0; i < str.length; i += 1) { + const ch = str[i]; + if (quote) { + if (ch === '\\' && quote === '"' && i + 1 < str.length) { add(str[i + 1], true); i += 1; continue; } + if (ch === quote) { quote = null; continue; } + add(ch, true); + continue; + } + if (ch === '\\' && i + 1 < str.length) { begin(i); add(str[i + 1], true); i += 1; continue; } + if (ch === '"' || ch === "'") { begin(i); quote = ch; continue; } + if (/\s/.test(ch)) { flush(i); continue; } + // An unquoted # at the start of a word ends the command line — the rest + // is comment and never reaches argv. Mid-word (`PR#42`) it is literal. + if (ch === '#' && !started) { flush(i); break; } + // Redirections: the operator and its target are consumed by the shell, + // not passed as arguments. A glued all-digit word is an IO number + // (`2>&1`) and belongs to the redirection, not to argv. + if (ch === '>' || ch === '<') { + if (started && /^[0-9]+$/.test(cur)) { cur = ''; curMask = ''; started = false; } else { flush(i); } + let j = i + 1; + if (str[j] === ch) j += 1; // >> / << + if (str[j] === '&') j += 1; // >& (2>&1) + while (j < str.length && /[ \t]/.test(str[j])) j += 1; // gap before target + while (j < str.length && !/[\s;|&<>]/.test(str[j])) j += 1; // the target word + tokens.push({ value: str.slice(i, j), start: i, end: j, redir: true }); + i = j - 1; + continue; + } + const pair = str.slice(i, i + 2); + if (pair === '&&' || pair === '||') { flush(i); tokens.push({ value: pair, start: i, end: i + 2, op: true }); i += 1; continue; } + if (ch === ';' || ch === '|' || ch === '&') { flush(i); tokens.push({ value: ch, start: i, end: i + 1, op: true }); continue; } + begin(i); + add(ch, false); + } + flush(str.length); + return tokens; +}; + +// The comment portion of a line — the same rule tokenize() applies, so the +// two cannot disagree about where the command ends: an unquoted `#` at word +// start begins a comment, and a `#` inside quotes or mid-word is literal +// (ship.md commits with `PR #${PR_NUMBER}` in the message). +// It tracks WORD START the way tokenize does, not "preceded by whitespace". +// Those differ, and the difference is exploitable: in `commit docs:\ # x` the +// backslash escapes the space, so the shell keeps `docs: #` as ONE word and +// `#` is literal — while a preceded-by-whitespace test reads it as a comment. +// The scan would then exempt a line the runtime still executes unscoped. +const commentPortion = (line) => { + let quote = null; + let started = false; + for (let i = 0; i < line.length; i += 1) { + const ch = line[i]; + if (quote) { + if (ch === '\\' && quote === '"' && i + 1 < line.length) { i += 1; continue; } + if (ch === quote) quote = null; + continue; + } + if (ch === '\\' && i + 1 < line.length) { started = true; i += 1; continue; } + if (ch === '"' || ch === "'") { started = true; quote = ch; continue; } + if (/\s/.test(ch)) { started = false; continue; } + if (ch === '#' && !started) return line.slice(i); + if (ch === '>' || ch === '<' || ch === ';' || ch === '|' || ch === '&') { started = false; continue; } + started = true; + } + return ''; +}; + +// The explicit opt-out for content that shows the defect ON PURPOSE. A +// wrong-example is otherwise indistinguishable from a regression — which is +// exactly why no property of the surrounding markup can stand in for the +// author's intent — so the author declares it, with a reason. +// +// IT IS ONLY A MARKER IN COMMENT POSITION. Matching the token anywhere on +// the line let a commit MESSAGE carry it — `commit "docs: explain +// gsd-scan-ignore: semantics"` — and silently exempt a real offender. A +// false-negative escape hatch is the one thing this file must not ship, and +// an exemption keyed to attacker-controlled-looking text is precisely that. +// AND IT MUST CARRY A TRACKING ISSUE. An exemption whose reason is free text +// has no expiry and no ledger — the `permanent-allow-test-rule` shape +// RULESET.TESTS.delete-bad-tests names. The repo already answered this for +// its sibling convention under ADR-456 (see #2269), enforced by +// scripts/lint-allow-test-rule-refs. That lint walks tests/ only, so it +// cannot see a marker living in a .md file; rather than teach a second token +// to a script whose whole contract is the ESLint comment form, this scan +// enforces the same rule over its own roots — it already runs in CI on every +// shard. +// +// THE PREDICATE IS THAT LINT'S, MIRRORED RATHER THAN RESTATED. Its +// ISSUE_REF_RE is `/#\d+|https?:\/\//` — `http://` counts, and `#\d+` has no +// trailing boundary. A hand-written near-copy drifts from it in exactly those +// details, and a contributor who satisfies one marker and not the other has +// been given two conventions wearing one name. (Both details were found by +// review against prose here that claimed HTTPS-only. The regex was right and +// the sentence describing it was not, which is its own lesson.) +const ISSUE_REF_RE = /#\d+|https?:\/\//; +// +// The reason is EXTRACTED, not pattern-matched in one shot, so that a marker +// with no reason at all is still recognized as an ATTEMPT. A rejected +// declaration must not fail as a MYSTERY: left un-exempt and nothing more, +// its author is told their commit is unscoped — a true statement about a line +// they had already explained, which is the mangle-until-it-shuts-up loop the +// marker exists to prevent. `# gsd-scan-ignore:` with an empty reason is the +// likeliest way to get this wrong and so is the case that most needs the +// specific diagnosis; a `\S`-requiring pattern silently drops it. +const SCAN_IGNORE_REASON_RE = /gsd-scan-ignore:(.*)$/; +const declarationReason = (portion) => { + const m = SCAN_IGNORE_REASON_RE.exec(portion); + return m === null ? null : m[1]; +}; +// Two independent conditions, and the second is the structural one: the +// marker must be in comment position AND must not survive tokenization as an +// ARGUMENT. tokenize() drops everything from a real comment onward, so a +// genuine declaration leaves no token carrying the token; anything that does +// reached argv, which means the runtime would have executed it. That makes +// "authored argument text can declare the line exempt" impossible by +// construction rather than by getting commentPortion's edges exactly right — +// and commentPortion has edges (redirection targets, escaped separators) +// where mirroring the tokenizer perfectly is fiddly and a miss is silent. +// The marker's reason, but only when the marker is in COMMENT position and +// survives neither as an argv token. Returns null when there is no attempt. +const declaredReasonFor = (line) => { + if (tokenize(line).some((t) => /gsd-scan-ignore:/.test(t.value))) return null; + return declarationReason(commentPortion(line)); +}; +const isDeclared = (line) => { + const reason = declaredReasonFor(line); + return reason !== null && ISSUE_REF_RE.test(reason); +}; +// An ATTEMPTED declaration that carries no tracking reference — including one +// with no reason at all. Reported on its own terms rather than silently +// failing to exempt: see the extraction note above. +const isUntrackedDeclaration = (line) => { + const reason = declaredReasonFor(line); + return reason !== null && !ISSUE_REF_RE.test(reason); +}; + +// A shell invoked with -c runs its next argument AS A COMMAND, so the +// invocation lives inside a quoted token and no amount of markup-stripping +// reaches it: `bash -c "gsd_run query commit fixup"` scored zero candidates +// while being perfectly executable. The recursion is keyed on the INVOKER, +// not on "a quoted token that parses as an invocation" — that wider rule +// would flag a commit MESSAGE quoting a command, a false positive against +// ordinary documentation. Here the outer command is bash, so a gsd commit +// message can never reach it. +// The invoker set, widened past the four spellings the first cut guessed at. +// `ash`, `csh`, `tcsh`, `fish` and `yash` all take -c and all run what +// follows; leaving them out is a silent false negative in a function whose +// entire job is reaching a command the tokenizer cannot see. +const SHELL_INVOKER_RE = /^(?:.*\/)?(?:ba|da|k|z|a|c|tc|fi|ya)?sh$/; +const shellDashCPayloads = (str) => { + const payloads = []; + let group = []; + const drain = () => { + if (!group.length) { return; } + // THE INVOKER MUST BE THE COMMAND, not merely present in the segment. + // Searching anywhere made `echo bash -c "gsd_run query commit fixup"` a + // candidate — `echo` prints the string, it does not run it — which is a + // false positive against prose that merely quotes a command line. Only an + // env assignment, a shell keyword, or a list/prompt marker may precede the + // command name. Those are not commands; `echo` is, which is exactly the + // line the search-anywhere version got wrong. + // Shell keywords, prompt/list markers, and the command MODIFIERS that + // pass straight through to the command after them. The modifiers are the + // mirror of the invoker-set gap: omitting them is a false negative in the + // same direction, since `time bash -c "…"` really does run the payload. + // + // RESIDUAL, named rather than half-solved: a modifier that takes its OWN + // flags (`sudo -u alice bash -c …`) still stops the search, because + // skipping arbitrary flag/value pairs here would mean modelling each + // modifier's option grammar. No live instance exists in the six roots; + // the failure is a missed candidate, and the declaration marker is not + // involved either way. + const NON_COMMAND_PREFIX = new Set([ + 'then', 'else', 'do', '$', '-', '*', '&&', '||', + 'time', 'exec', 'nohup', 'env', 'command', + ]); + // AN ASSIGNMENT IS A SKIPPABLE PREFIX ONLY WHEN IT IS NOT A SUBSTITUTION. + // `FOO=1 bash -c "…"` prefixes the command; `V=$(bash -c "…")` IS the + // command, and skipping it walks the search past the invoker onto `-c`, + // which is not one — so the payload is never reached. Found by sweeping + // the substitution-glues-to-the-binary class across every command-name + // test rather than only the one the finding named. + const skippable = (t) => t.redir + || (/^[A-Za-z_][A-Za-z0-9_]*(\[[^\]]*\])?=/.test(t.value) && !t.value.includes('$(')) + || NON_COMMAND_PREFIX.has(t.value); + const gi = group.findIndex((t) => !skippable(t)); + if (gi !== -1 && SHELL_INVOKER_RE.test(bareCommandName(group[gi]))) { + const ci = group.findIndex((t, k) => k > gi && !t.redir && t.value === '-c'); + const payload = ci !== -1 ? group.slice(ci + 1).find((t) => !t.redir) : undefined; + if (payload) payloads.push(payload.value); + } + group = []; + }; + for (const t of tokenize(str)) { + if (t.op) { drain(); } else { group.push(t); } + } + drain(); + return payloads; +}; + +module.exports = { + bareCommandName, + tokenize, + shellDashCPayloads, + SHELL_INVOKER_RE, + commentPortion, + ISSUE_REF_RE, + declarationReason, + declaredReasonFor, + isDeclared, + isUntrackedDeclaration, +};