diff --git a/.changeset/lively-sloths-wave.md b/.changeset/lively-sloths-wave.md new file mode 100644 index 000000000..293904c46 --- /dev/null +++ b/.changeset/lively-sloths-wave.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3648 +--- +**`git.protected_branches` config field warns on additional shared branches, not just the resolved base branch** — a git-flow project whose GitHub-default branch differs from its actual integration branch (e.g. `main` vs. `develop`) can now list `develop`/`staging`/etc. so `execute-phase`'s `handle_branching` "none" strategy and `/gsd-ship`'s preflight warn on any of them, not only the one resolved base branch. Optional and additive — absent by default, existing projects see no behavior change. (#3552) diff --git a/CONTEXT.md b/CONTEXT.md index 8acdf67a2..9eb66d28c 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -203,7 +203,7 @@ Workflow contract seam covering agent worktree lifecycle orchestration rules. Th Adapter Module owning linked-worktree root mapping and metadata-prune policy (`git worktree prune` non-destructive default) for planning/workstream callers. ### Git Query Module -Module owning bounded, never-throw git repository introspection — the single seam for read-only git queries that degrade gracefully rather than throwing. **Adapter 1 — base-branch detection** (`gsd_run query git.base-branch`): Implements a full precedence ladder: (1) `git.base_branch` config override from `.planning/config.json`; (2) `git symbolic-ref --short refs/remotes/origin/HEAD`; (3) `git remote show origin` HEAD branch (authoritative when origin/HEAD is unset — the common case for `git init + remote add + fetch` without `set-head`); (4) local branch existence (`master` present and `main` absent → `master`; `main` present → `main`); (5) `"main"` last-resort default. All git subprocesses are bounded with timeouts (5–15 s) and degrade gracefully to the next tier; the function never throws. Replaces duplicated per-workflow bash detection that silently fell through to `:-main` on master repos (#1146). **Adapter 2 — worktree-info detection**: `gitWorktreeInfoInternal` (`git rev-parse --is-inside-work-tree` + `--show-toplevel`), absorbed from the Core module when the `core.cjs` re-export spine was retired and aligned to this module's bounded-timeout / degrade-don't-throw convention (worktree-info detection is a query concern, distinct from the Worktree Safety Policy Module's lifecycle policy). **Adapter 3 — phase change-set detection** (#1953): `phaseStartCommit` resolves the commit that ADDED a phase's `PLAN.md` (`git log --diff-filter=A -1`) — the anchor for "what did this phase touch", since STATE.md records no phase-start sha — and `changedFilesSince` returns the changed paths from that anchor to HEAD. `changedFilesSince` uses `-z` with `core.quotepath=false` and splits on NUL, because git otherwise quotes and escapes non-ASCII paths (lossy round-trip) and a `\n` split corrupts a filename containing a newline; the ref precedes a literal `--` so a dash-leading path cannot be read as an option. Both bounded at 15 s and both degrade to `null`. Consumed by the Complexity Trigger Module. Source: `src/git-base-branch.cts` → `gsd-core/bin/lib/git-base-branch.cjs`. Wired into `execute-phase.md`, `quick.md`, `ship.md`, `complete-milestone.md`, and `pr-branch.md`. +Module owning bounded, never-throw git repository introspection — the single seam for read-only git queries that degrade gracefully rather than throwing. **Adapter 1 — base-branch detection** (`gsd_run query git.base-branch`): Implements a full precedence ladder: (1) `git.base_branch` from the EFFECTIVE configuration — since #3648 this tier is resolved by the Config Loader Module's `loadConfig`, not by a direct `.planning/config.json` read, so it carries the root+workstream deep merge, flat-then-nested key lookup, and builtin/federated defaults that authority applies; (2) `git symbolic-ref --short refs/remotes/origin/HEAD`; (3) `git remote show origin` HEAD branch (authoritative when origin/HEAD is unset — the common case for `git init + remote add + fetch` without `set-head`); (4) local branch existence (`master` present and `main` absent → `master`; `main` present → `main`); (5) `"main"` last-resort default. All git subprocesses are bounded with timeouts (5–15 s) and degrade gracefully to the next tier; the function never throws. The plain base-branch adapter also resolves through the Config Loader with `persist: false`: it retains loader stderr diagnostics while suppressing legacy-key migration; its stdout remains the resolved branch string. Replaces duplicated per-workflow bash detection that silently fell through to `:-main` on master repos (#1146). **Adapter 1a — protected-branch predicate** (`gsd_run query git.base-branch --is-protected `, #3552): `resolveProtectedBranchStatus` answers whether a branch is shared-and-protected, resolving the base branch through the same ladder and extending it with the optional `git.protected_branches` list, so a git-flow project whose GitHub default (`main`) differs from its integration branch (`develop`) can protect both. Matching is by exact name — no globs. Two invariants distinguish this adapter from the plain query and must hold together: it **fails CLOSED**, rendering `true` with a stderr diagnostic when a tier-2/3/4 git query **timed out or could not be spawned** (`verified:false`, #3057 B4) — note the narrowness, since a git command that RUNS and exits non-zero is `verified:true` by that same contract, so a cwd that is not a repository at all answers `false`, not `true` — and it **must not write**, so its config read passes `persist: false` and the Config Loader's normalize-then-write-back is suppressed — a predicate invoked on every `execute-phase` and `ship` run cannot be allowed to rewrite the config it is asking about (#3648). Per-entry validation drops only unusable `git.protected_branches` names and reports each on stderr, rather than discarding the whole list, so a hand-edited config cannot fail the guard open. Consumed by `execute-phase.md` (`handle_branching`, "none" strategy) and `ship.md` (preflight); both emit an advisory warning and continue. **Adapter 2 — worktree-info detection**: `gitWorktreeInfoInternal` (`git rev-parse --is-inside-work-tree` + `--show-toplevel`), absorbed from the Core module when the `core.cjs` re-export spine was retired and aligned to this module's bounded-timeout / degrade-don't-throw convention (worktree-info detection is a query concern, distinct from the Worktree Safety Policy Module's lifecycle policy). **Adapter 3 — phase change-set detection** (#1953): `phaseStartCommit` resolves the commit that ADDED a phase's `PLAN.md` (`git log --diff-filter=A -1`) — the anchor for "what did this phase touch", since STATE.md records no phase-start sha — and `changedFilesSince` returns the changed paths from that anchor to HEAD. `changedFilesSince` uses `-z` with `core.quotepath=false` and splits on NUL, because git otherwise quotes and escapes non-ASCII paths (lossy round-trip) and a `\n` split corrupts a filename containing a newline; the ref precedes a literal `--` so a dash-leading path cannot be read as an option. Both bounded at 15 s and both degrade to `null`. Consumed by the Complexity Trigger Module. Source: `src/git-base-branch.cts` → `gsd-core/bin/lib/git-base-branch.cjs`. Wired into `execute-phase.md`, `quick.md`, `ship.md`, `complete-milestone.md`, and `pr-branch.md`. `SEAM.git-query-readonly-seam.owns=bounded, never-throw git repository introspection — base-branch detection, worktree-info detection, phase change-set detection` `SEAM.git-query-readonly-seam.enforced-by=test:tests/git-base-branch.test.cjs` @@ -257,7 +257,7 @@ Module owning the shared low-level utility primitives extracted from Core: POSIX Module owning agent-presence resolution and verification, extracted from the Core module as the cleanup step that retired the `core.cjs` re-export spine (the final ADR-857 decomposition, epic #1267). Interface: `getAgentsDir(runtime?, projectRoot?)` — env-var-aware, runtime-aware agents-directory resolution; Claude resolves `__dirname`-relative unless that path contains a `node_modules` path segment, in which case it falls back to `getGlobalConfigDir('claude')/agents` (#3203); that segment test is lexical and case-sensitive rather than an install-shape guarantee — it targets the layouts where the sibling `agents/` is the package's own bundled copy and the check would otherwise validate the package against itself, and any path merely carrying a directory of that name resolves the same way. Other runtimes prefer a manifest-backed project-local agents directory before their global configuration home. The manifest gate is intentional: runtime-native project agents must not shadow a working global GSD install. `checkAgentsInstalled(...)` validates `gsd-file-manifest.json` completeness and confirms the declared agents exist on disk. Pure read/verify — no install-write side effects (writes remain the Installer Module's). Consumed by the Init Command Module, the verify workflow, and the docs workflow. Source of truth: `gsd-core/bin/lib/agent-install-check.cjs` (generated from `src/agent-install-check.cts`); replaced the two functions that squatted in `core.cts`. See Installer Module and ADR-857. ### 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`). +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). Loading is **not side-effect-free by default**: normalizing a legacy key marks the config dirty and writes the migrated shape back to disk, which is how a legacy project is migrated by ordinary use. `options.persist: false` (#3648) suppresses exactly that write while leaving resolution identical — the option a caller passes when it is only ASKING the config something (the Git Query Module's protected-branch predicate runs on every `execute-phase` and `ship`, and a question must not rewrite the file it asks about). It is opt-OUT, so every pre-existing caller keeps the historical write-back. 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 Publication Gate (`planning.pr_strict`) The seam deciding whether `.planning/` artifacts reach the REMOTE — distinct from the Planning Commit Gate below, which decides whether they reach git at all. `planning.pr_strict` (boolean, manifest default `false`) resolves through the same `loadConfigResolved` chain as its `planning.*` siblings (explicit top-level `pr_strict`, then the `planning.pr_strict` alias, then the manifest default), and is additionally registered in `SCHEMA_DEFAULTS` (`src/config.cts`) so `query config-get planning.pr_strict` answers `false` for an absent key rather than `Key not found` — `gsd-core/workflows/pr-branch.md` reads it with a plain `config-get` and must not special-case a missing key. It selects between two filter modes in that workflow: default preserves the five structural planning files plus `milestones/**` and drops nine transient subdirectories; strict drops every `.planning/` path and includes a commit only when it touches at least one file outside `.planning/`. The two path lists are declared ONCE in the workflow (`TRANSIENT_DIRS`, `STRUCTURAL_RE`) and both the un-stage step and the verification assertion are derived from them, because the prior shape declared them twice and the two steps disagreed by construction — `verify` asserted zero `.planning/` paths while `create_pr_branch` was specified to preserve five, so a correct run reported itself as failed on every phase (#2971). A third gate is deliberately NOT derived from those declarations: `PLANNING_DELETIONS` (#3679) counts DELETED `.planning/` paths via `git diff --name-status --no-renames` and must be `0` in every mode — a deleted planning path is data loss, not filtering, and name-only counting cannot see status. The two gates are independent but not orthogonal in effect: `pr_strict` is inert when `commit_docs` is `false`, since nothing is committed for the PR-branch filter to remove. Source of truth: `gsd-core/workflows/pr-branch.md`; key registered in `gsd-core/bin/shared/config-{defaults,schema}.manifest.json`. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index d427a9cc4..5b8ef11f9 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1081,11 +1081,37 @@ All four fields are **optional and additive** — STATE.md files without them ke |---------|------|---------|-------------| | `git.branching_strategy` | enum | `none` | `none`, `phase`, or `milestone` | | `git.base_branch` | string | `main` | The integration branch that phase/milestone branches are created from and merged back into. Override when your repo uses `master` or a release branch | +| `git.protected_branches` | array of non-empty strings | (none) | Optional additional shared branches that should trigger protected-branch warnings alongside the resolved base branch | | `git.create_tag` | boolean | `true` | Create a git tag (`v[X.Y]`) on milestone completion. Set to `false` for projects with their own release flow | | `git.phase_branch_template` | string | `gsd/phase-{phase}-{slug}` | Branch name template for phase strategy | | `git.milestone_branch_template` | string | `gsd/{milestone}-{slug}` | Branch name template for milestone strategy | | `git.quick_branch_template` | string or null | `null` | Optional branch name template for `/gsd-quick` tasks | +### Protected Branch Warnings + +`git.protected_branches` is optional and has no persisted default. When the field is absent, +GSD protects only the resolved base branch, preserving existing project behavior. Every configured +item must be a non-empty string. The configured list extends the resolved base branch; it never +replaces that branch or changes base-branch detection. + +A match produces an advisory warning at execute-phase and ship and does not change +`git.branching_strategy: "none"`: GSD still continues on the current branch and ship still offers +to create a feature branch. + +Matching is by exact branch name — there is no glob or prefix support, so a git-flow +layout must name each `release/*` or `hotfix/*` branch it wants protected. An entry that +is not a non-empty string is ignored with a warning naming it, and the remaining names +still apply. + +```json +{ + "git": { + "branching_strategy": "none", + "protected_branches": ["develop", "staging"] + } +} +``` + ### Strategy Comparison | Strategy | Creates Branch | Scope | Merge Point | Best For | @@ -2072,6 +2098,14 @@ Two different rules apply, and the difference is deliberate ([#3532](https://git - **`effort` is the exception**: the install-time effort channel always merges `~/.gsd/defaults.json` with the project config (that is how `effort sync` works), so a global `effort` block keeps working in projects and does not trigger the warning. +- **The whole `git.*` namespace is project-scoped and never resolves from the global file**, + in either directory shape — not `git.base_branch`, not `git.protected_branches`, not + `git.branching_strategy` or the branch templates. Branch policy is a property of the + repository, not of the machine, so it is read only from that project's + `.planning/config.json`. A `git` block in `~/.gsd/defaults.json` still seeds new projects + (`/gsd-new-project` copies globals into the new `config.json`), but it never takes effect + at runtime on its own. It is outside the shadowed-key warning above, which covers the + model-resolution set only. --- diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 8aa59352f..6838a373c 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -608,6 +608,7 @@ "execute-phase/steps/per-plan-executor-routing.md", "execute-phase/steps/per-plan-worktree-gate.md", "execute-phase/steps/post-merge-gate.md", + "execute-phase/steps/protected-branch.md", "execute-phase/steps/regression-gate-run.md", "execute-phase/steps/regression-gate.md", "execute-phase/steps/wave-post-gate-hooks.md", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 869a153e1..4fa4a36ca 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -529,7 +529,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `frontmatter.cjs` | YAML frontmatter CRUD operations | | `gap-checker.cjs` | Post-planning gap analysis (#2493): unified REQUIREMENTS.md + CONTEXT.md decisions vs PLAN.md coverage report (`gsd-tools gap-analysis`) | | `gate-predicate-evaluator.cjs` | Evaluates capability gate predicates — `command-exit-zero` and `artifact-frontmatter` (#2008) | -| `git-base-branch.cjs` | Single base-branch resolver (`gsd_run query git.base-branch`) with full precedence ladder: config override → origin/HEAD symref → `git remote show origin` → local branch presence → "main". Eliminates per-workflow duplicated bash detection (#1146) | +| `git-base-branch.cjs` | Single base-branch resolver (`gsd_run query git.base-branch`) with full precedence ladder: effective-config override (`git.base_branch`, resolved through `config-loader.cjs`) → origin/HEAD symref → `git remote show origin` → local branch presence → "main". Eliminates per-workflow duplicated bash detection (#1146). Also hosts the protected-branch predicate `--is-protected ` (#3552), which extends the resolved base branch with the optional `git.protected_branches` list, matches by exact name, fails closed when the base branch is unverified, and reads config with `persist: false` so the query never rewrites `.planning/config.json` | | `graphify.cjs` | Knowledge-graph build/query/status/diff for `/gsd-graphify` | | `graphify-command-router.cjs` | ADR-959 capability command router for `gsd-tools graphify` — dispatches build/query/status/diff subcommands; first real capability command cutover (phase 4d-impl-2) | | `gsd2-import.cjs` | External-plan ingest for `/gsd-import --from-gsd2` | diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index 6a055de41..f9bda88b0 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -44,6 +44,7 @@ "ship.pr_body_sections", "git.branching_strategy", "git.base_branch", + "git.protected_branches", "git.create_tag", "git.phase_branch_template", "git.milestone_branch_template", diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index cfc3cac21..ff7ce352a 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -12,6 +12,7 @@ Configuration options for `.planning/` directory behavior. "git": { "branching_strategy": "none", "base_branch": null, + "protected_branches": ["develop", "staging"], "phase_branch_template": "gsd/phase-{phase}-{slug}", "milestone_branch_template": "gsd/{milestone}-{slug}", "quick_branch_template": null @@ -32,6 +33,7 @@ Configuration options for `.planning/` directory behavior. | `search_gitignored` | `false` | Add `--no-ignore` to broad rg searches | | `git.branching_strategy` | `"none"` | Git branching approach: `"none"`, `"phase"`, or `"milestone"` | | `git.base_branch` | `null` (auto-detect) | Target branch for PRs and merges (e.g. `"master"`, `"develop"`). When `null`, auto-detects from `git symbolic-ref refs/remotes/origin/HEAD`, falling back to `"main"`. | +| `git.protected_branches` | (none) | Optional array of non-empty strings naming additional shared branches that should trigger protected-branch warnings | | `git.create_tag` | `true` | Create git tags on milestone completion | | `git.phase_branch_template` | `"gsd/phase-{phase}-{slug}"` | Branch template for phase strategy | | `git.milestone_branch_template` | `"gsd/{milestone}-{slug}"` | Branch template for milestone strategy | @@ -48,6 +50,26 @@ Configuration options for `.planning/` directory behavior. | `response_language` | `null` | Language for user-facing questions and prompts across all phases/subagents (e.g. `"Portuguese"`, `"Japanese"`, `"Spanish"`). When set, all spawned agents include a directive to respond in this language. | +`git.protected_branches` has no persisted default. When it is absent, only the resolved base branch +is protected, preserving existing project behavior. Every configured item must be a non-empty +string. The configured list extends the resolved base branch; it never replaces the base or changes +the resolution ladder. A match produces an advisory warning at execute-phase and ship and does not +change `git.branching_strategy: "none"`. + +Matching is by exact branch name — there is no glob or prefix support, so a git-flow +layout must name each `release/*` or `hotfix/*` branch it wants protected. An entry that +is not a non-empty string is ignored with a warning naming it, and the remaining names +still apply. + +```json +{ + "git": { + "branching_strategy": "none", + "protected_branches": ["develop", "staging"] + } +} +``` + **When `commit_docs: true` (default):** @@ -306,6 +328,7 @@ Set via `git.*` namespace (e.g., `"git": { "branching_strategy": "phase" }`). |-----|------|---------|----------------|-------------| | `git.branching_strategy` | string | `"none"` | `"none"`, `"phase"`, `"milestone"` | Git branching approach for phase/milestone isolation | | `git.base_branch` | string\|null | `null` (auto-detect) | Any branch name | Target branch for PRs and merges; auto-detects from `origin/HEAD` when `null` | +| `git.protected_branches` | array of non-empty strings | (none) | Non-empty branch names | Optional protected names added to the resolved base branch for execute-phase and ship warnings | | `git.create_tag` | boolean | `true` | `true`, `false` | Create git tags on milestone completion | | `git.phase_branch_template` | string | `"gsd/phase-{phase}-{slug}"` | Template with `{phase}`, `{slug}` | Branch naming template for `phase` strategy | | `git.milestone_branch_template` | string | `"gsd/{milestone}-{slug}"` | Template with `{milestone}`, `{slug}` | Branch naming template for `milestone` strategy | diff --git a/gsd-core/workflows/execute-phase.md b/gsd-core/workflows/execute-phase.md index b81dad976..d5828be01 100644 --- a/gsd-core/workflows/execute-phase.md +++ b/gsd-core/workflows/execute-phase.md @@ -280,7 +280,7 @@ checkpoints between tasks. The user can review, modify, or redirect work at any Check `branching_strategy` from init: -**"none":** Skip, continue on current branch. +**"none":** Read and execute `execute-phase/steps/protected-branch.md`. **"phase" or "milestone":** Use pre-computed `branch_name` from init. diff --git a/gsd-core/workflows/execute-phase/steps/protected-branch.md b/gsd-core/workflows/execute-phase/steps/protected-branch.md new file mode 100644 index 000000000..c5f12b38d --- /dev/null +++ b/gsd-core/workflows/execute-phase/steps/protected-branch.md @@ -0,0 +1,21 @@ +# Protected-branch warning for `branching_strategy: none` (#3552) + +Run this from the `handle_branching` step's `"none"` arm, after deciding to +continue on the current branch. It warns without refusing execution — the +`"none"` strategy still runs on whatever branch it started on. + +```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}"; _gsd_at() { for _p; do if [ -f "$_p" ]; then GSD_TOOLS="$_p"; return 0; fi; done; return 1; }; if _gsd_at "${_GSD_RUNTIME_ROOT}/gsd-core/bin/${_GSD_SHIM_NAME}" "${_GSD_RUNTIME_ROOT}/.claude/gsd-core/bin/${_GSD_SHIM_NAME}" "${_GSD_RUNTIME_ROOT}/.codex/gsd-core/bin/${_GSD_SHIM_NAME}"; then gsd_run() { node "$GSD_TOOLS" "$@"; }; elif unset -f gsd_run; _G="$(command -v gsd_run)"; then GSD_TOOLS="$_G"; gsd_run() { "$GSD_TOOLS" "$@"; }; elif _gsd_at "${CLAUDE_CONFIG_DIR:-$HOME/.claude}/gsd-core/bin/${_GSD_SHIM_NAME}" "${HERMES_HOME:-$HOME/.hermes}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CURSOR_CONFIG_DIR:-$HOME/.cursor}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CODEX_HOME:-$HOME/.codex}/gsd-core/bin/${_GSD_SHIM_NAME}" "${GEMINI_CONFIG_DIR:-$HOME/.gemini}/gsd-core/bin/${_GSD_SHIM_NAME}" "${COPILOT_CONFIG_DIR:-$HOME/.copilot}/gsd-core/bin/${_GSD_SHIM_NAME}" "${WINDSURF_CONFIG_DIR:-$HOME/.codeium/windsurf}/gsd-core/bin/${_GSD_SHIM_NAME}" "${AUGMENT_CONFIG_DIR:-$HOME/.augment}/gsd-core/bin/${_GSD_SHIM_NAME}" "${TRAE_CONFIG_DIR:-$HOME/.trae}/gsd-core/bin/${_GSD_SHIM_NAME}" "${QWEN_CONFIG_DIR:-$HOME/.qwen}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CODEBUDDY_CONFIG_DIR:-$HOME/.codebuddy}/gsd-core/bin/${_GSD_SHIM_NAME}" "${CLINE_CONFIG_DIR:-$HOME/.cline}/gsd-core/bin/${_GSD_SHIM_NAME}" "${GROK_AGENTS_HOME:-$HOME/.agents}/gsd-core/bin/${_GSD_SHIM_NAME}" "${ANTIGRAVITY_CONFIG_DIR:-$HOME/.gemini/antigravity}/gsd-core/bin/${_GSD_SHIM_NAME}" "${OPENCODE_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/opencode}/gsd-core/bin/${_GSD_SHIM_NAME}" "${KILO_CONFIG_DIR:-${XDG_CONFIG_HOME:-$HOME/.config}/kilo}/gsd-core/bin/${_GSD_SHIM_NAME}"; then gsd_run() { node "$GSD_TOOLS" "$@"; }; else echo "ERROR: gsd-tools.cjs not found at $GSD_TOOLS and gsd_run is not on PATH. Run: npx -y @opengsd/gsd-core@latest --claude --local" >&2; exit 1; fi; GSD_IDENTITY_STATUS=unverified; case "$(gsd_run runtime-identity --raw 2>/dev/null || true)" in '{"packageName":"@opengsd/gsd-core"'*'}') GSD_IDENTITY_STATUS=ok;; esac; export GSD_IDENTITY_STATUS; [ "$GSD_IDENTITY_STATUS" = ok ] || echo "WARNING: \"$GSD_TOOLS\" did not prove it is @opengsd/gsd-core - it is either a different package or an @opengsd/gsd-core older than the runtime-identity verb. See docs/how-to/diagnose-a-foreign-gsd-tools.md" >&2; 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 +CURRENT_BRANCH=$(git branch --show-current 2>/dev/null || true) +IS_PROTECTED=$(gsd_run query git.base-branch --is-protected "$CURRENT_BRANCH") || IS_PROTECTED="" +if [ "$IS_PROTECTED" = true ]; then + echo "⚠ Current branch '$CURRENT_BRANCH' is a protected branch; branching_strategy=none will continue here." >&2 +elif [ -z "$IS_PROTECTED" ]; then + echo "⚠ Could not determine whether '$CURRENT_BRANCH' is protected — the query failed. Continuing." >&2 +fi +``` + +The `IS_PROTECTED=""` fallback on the first line, and the second `elif`, exist +so a failed or missing `gsd_run` invocation degrades **visibly** (an explicit +"could not determine" warning) rather than silently reading as "not +protected" under `set -e` (#3648 review, round 5). diff --git a/gsd-core/workflows/ship.md b/gsd-core/workflows/ship.md index be5786ba0..09293868f 100644 --- a/gsd-core/workflows/ship.md +++ b/gsd-core/workflows/ship.md @@ -74,9 +74,15 @@ Verify the work is ready to ship: 3. **On correct branch?** ```bash - CURRENT_BRANCH=$(git branch --show-current) + CURRENT_BRANCH=$(git branch --show-current 2>/dev/null || true) + IS_PROTECTED=$(gsd_run query git.base-branch --is-protected "$CURRENT_BRANCH") || IS_PROTECTED="" + if [ "$IS_PROTECTED" = true ]; then + echo "⚠ Current branch '$CURRENT_BRANCH' is a protected branch; shipping should happen from a feature branch." >&2 + elif [ -z "$IS_PROTECTED" ]; then + echo "⚠ Could not determine whether '$CURRENT_BRANCH' is protected — the query failed. Continuing." >&2 + fi ``` - If on `${BASE_BRANCH}`: warn — should be on a feature branch. + If `IS_PROTECTED` is `true`: warn — should be on a feature branch. If branching_strategy is `none`: offer to create a branch now. 4. **Remote configured?** diff --git a/src/config-loader.cts b/src/config-loader.cts index 4d191e07e..4a7b3b337 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -103,6 +103,28 @@ function _getNestedConfigDefault(section: string, field: string): unknown { return undefined; } +/** Shared flat-then-nested config lookup; exported for parity tests. */ +function _getConfigValue( + parsed: Record, + key: string, + nested?: { section: string; field: string }, +): unknown { + if (parsed[key] !== undefined) return parsed[key]; + if (nested && parsed[nested.section] && typeof parsed[nested.section] === 'object' && parsed[nested.section] !== null) { + return (parsed[nested.section] as Record)[nested.field]; + } + return undefined; +} + +/** Shared nested-only config lookup; exported for parity tests. */ +function _getConfigNested(parsed: Record, section: string, field: string): unknown { + const sec = parsed[section]; + if (sec !== null && typeof sec === 'object' && !Array.isArray(sec)) { + return (sec as Record)[field]; + } + return undefined; +} + const CONFIG_DEFAULTS = { model_profile: _getConfigDefault('model_profile'), commit_docs: _getConfigDefault('commit_docs'), @@ -638,8 +660,18 @@ function _warnUnusableConfig(fault: ConfigFault): void { * diagnostic names the file. Before this, a trailing comma in config.json was * byte-identical to the file not existing: builtin defaults, degraded:false, * and the user's entire configuration silently discarded. + * + * `options.persist: false` (#3648) suppresses the two normalize-then-write-back + * side effects below. Resolution is otherwise identical — same precedence, same + * returned object — the migrated shape simply stays in memory. Callers that only + * ASK the config something (a predicate, a status readout) pass it so that a read + * cannot dirty the working tree; the ~30 callers that omit it keep persisting, so + * a legacy config is still migrated exactly once by ordinary use. */ function loadConfigResolved(cwd: string, options: Record = {}): ConfigResolution { + // Opt-OUT, not opt-in: omitting the option must preserve the historical + // write-back for every existing caller. + const persist = options['persist'] !== false; // NOTE: loadConfigResolved resolves from cwd AS-IS (no walk-up). // Callers that need ancestor-anchoring (e.g. cmdAgentSkills) must do so // themselves via findProjectRoot() before calling this function. @@ -718,7 +750,9 @@ function loadConfigResolved(cwd: string, options: Record = {}): } } rootParsed = rootNormalized; - try { platformWriteSync(rootConfigPath, JSON.stringify(rootParsed, null, 2)); } catch { /* ignore */ } + if (persist) { + try { platformWriteSync(rootConfigPath, JSON.stringify(rootParsed, null, 2)); } catch { /* ignore */ } + } } else { rootParsed = rootNormalized; } @@ -783,7 +817,7 @@ function loadConfigResolved(cwd: string, options: Record = {}): } } - if (configDirty) { + if (configDirty && persist) { try { platformWriteSync(configPath, JSON.stringify(fileData, null, 2)); } catch { /* ignore */ } } @@ -832,16 +866,22 @@ function loadConfigResolved(cwd: string, options: Record = {}): _warnUnknownProfileOverrides(parsed, '.planning/config.json'); - const get = (key: string, nested?: { section: string; field: string }): unknown => { - if (parsed[key] !== undefined) return parsed[key]; - if (nested && parsed[nested.section] && typeof parsed[nested.section] === 'object' && parsed[nested.section] !== null) { - const sec = parsed[nested.section] as Record; - if (sec[nested.field] !== undefined) { - return sec[nested.field]; - } - } - return undefined; - }; + const get = (key: string, nested?: { section: string; field: string }): unknown => + _getConfigValue(parsed, key, nested); + + /** + * Nested-ONLY read — no top-level fallback (#3648). + * + * `get()`'s flat-then-nested order exists for keys that have a legacy flat + * spelling `normalizeLegacyKeys` migrates (`branching_strategy`, + * `base_branch`, …); for those, honouring the flat key is back-compat. A key + * introduced with no legacy form has nothing to be compatible WITH, so + * routing it through `get()` would invent an undocumented top-level alias + * that silently outranks the canonical nested key. Use this instead for new + * `
.` keys (round-4 external review). + */ + const getNested = (section: string, field: string): unknown => + _getConfigNested(parsed, section, field); const parallelization = (() => { const val = get('parallelization'); @@ -860,6 +900,8 @@ function loadConfigResolved(cwd: string, options: Record = {}): })(), search_gitignored: get('search_gitignored', { section: 'planning', field: 'search_gitignored' }) ?? defaults.search_gitignored, branching_strategy: get('branching_strategy', { section: 'git', field: 'branching_strategy' }) ?? defaults.branching_strategy, + base_branch: get('base_branch', { section: 'git', field: 'base_branch' }), + protected_branches: getNested('git', 'protected_branches'), phase_branch_template: get('phase_branch_template', { section: 'git', field: 'phase_branch_template' }) ?? defaults.phase_branch_template, milestone_branch_template: get('milestone_branch_template', { section: 'git', field: 'milestone_branch_template' }) ?? defaults.milestone_branch_template, quick_branch_template: get('quick_branch_template', { section: 'git', field: 'quick_branch_template' }) ?? defaults.quick_branch_template, @@ -979,8 +1021,15 @@ function loadConfigResolved(cwd: string, options: Record = {}): // Fix 2: Early intercept — workstream requested but ws config.json absent (or dir absent) // AND root config was loaded. Covers BOTH "dir exists, no config.json" AND "dir absent". // This delivers the #1366 acceptance criterion: nonexistent GSD_WORKSTREAM yields root, degraded. + // + // Both fallback recursions below forward `options` and override ONLY `workstream`. + // A bare `{ workstream: null }` silently dropped every other option, so a caller's + // `persist: false` was discarded on exactly this path and the root config was + // rewritten by a read (#3648, found by external review). The explicit + // `workstream: null` still wins the `hasOwnProperty` check at the top of this + // function, so spreading cannot let `workstreamContext` reintroduce a workstream. if (wsRequested && rootParsed) { - const fb = loadConfigResolved(cwd, { workstream: null }); + const fb = loadConfigResolved(cwd, { ...options, workstream: null }); return fallback({ config: fb.config, source: 'root', degraded: true }); } @@ -989,7 +1038,7 @@ function loadConfigResolved(cwd: string, options: Record = {}): if (rootParsed) { // Branch B: workstream requested but ws config.json absent; root config present. // (Only reached when wsRequested is false — e.g. ws='' with .planning/workstreams//config.json) - const fb = loadConfigResolved(cwd, { workstream: null }); + const fb = loadConfigResolved(cwd, { ...options, workstream: null }); return fallback({ config: fb.config, source: 'root', degraded: true }); } // Branch C: .planning/ exists but no config.json and no root config — federated/builtin defaults @@ -1088,6 +1137,8 @@ export = { CONFIG_DEFAULTS, _getConfigDefault, _getNestedConfigDefault, + _getConfigValue, + _getConfigNested, _deepMergeConfig, _warnedUnknownConfigKeys, _warnedShadowedGlobalKeys, diff --git a/src/config.cts b/src/config.cts index 94a75a81e..49ee4d12f 100644 --- a/src/config.cts +++ b/src/config.cts @@ -167,6 +167,34 @@ function validateKnownConfigKeyPath(keyPath: string): void { } } +/** + * Is `value` an acceptable `git.protected_branches` list (#3552)? + * + * A non-empty array whose every element is a string with non-whitespace + * content. Exported so a property test can pin this predicate against the + * resolver's own per-entry filter in `git-base-branch.cts` — `config-set` must + * only accept lists the resolver will honour in full, with nothing rejected. + * The two are deliberately different shapes (all-or-nothing here, per-entry + * there, because a direct file edit bypasses this check), so nothing keeps them + * agreeing except a test that asks both. + */ +function isValidProtectedBranches(value: unknown): boolean { + if (!Array.isArray(value) || value.length === 0) return false; + const entries = value as unknown[]; + // Index, do NOT use `.every()`. `.every()` SKIPS holes, so a sparse array + // (`["main", , "develop"]`) passed this check while the resolver's `for...of` + // — which yields `undefined` for a hole — rejected that element. The two + // surfaces then disagreed about the same value. JSON cannot express a hole, + // so neither surface meets one in production, but "unreachable" is not a + // reason to leave two definitions of the same predicate contradicting each + // other (round-4 external review). + for (let i = 0; i < entries.length; i += 1) { + const branch = entries[i]; + if (typeof branch !== 'string' || branch.trim().length === 0) return false; + } + return true; +} + function validateShipPrBodySections(value: unknown): void { if (!Array.isArray(value)) { error('Invalid ship.pr_body_sections value. Expected a JSON array of section objects.'); @@ -844,6 +872,12 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | } } + if (kp === 'git.protected_branches') { + if (!isValidProtectedBranches(parsedValue)) { + error(`Invalid git.protected_branches '${val}'. Must be a non-empty array of non-empty branch names.`); + } + } + if (kp === 'ship.pr_body_sections') { validateShipPrBodySections(parsedValue); } @@ -1280,4 +1314,5 @@ export = { // Exported for programmatic use by capability-writer and tests setConfigValue, setConfigValues, + isValidProtectedBranches, }; diff --git a/src/configuration.cts b/src/configuration.cts index 30651d25b..b75ddab88 100644 --- a/src/configuration.cts +++ b/src/configuration.cts @@ -309,6 +309,10 @@ function normalizeLegacyKeys(parsed: Record): NormalizeLegacyKe delete result['depth']; normalizations.push({ from: 'depth', to: 'granularity', value: mapped }); } + // 5. top-level base_branch → git.base_branch + if (Object.prototype.hasOwnProperty.call(result, 'base_branch')) { + hoistLegacyKey(result, normalizations, skipped, 'base_branch', 'git', 'base_branch'); + } return { parsed: result, normalizations, skipped }; } diff --git a/src/git-base-branch.cts b/src/git-base-branch.cts index 76771b745..7c22943d1 100644 --- a/src/git-base-branch.cts +++ b/src/git-base-branch.cts @@ -30,19 +30,35 @@ * tests can run without touching the real filesystem or spawning real git. */ -import fs from 'node:fs'; import path from 'node:path'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import configLoader = require('./config-loader.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports +import io = require('./io.cjs'); +import { normalizeLegacyKeys } from './configuration.cjs'; +import { sanitizeLabel } from './security.cjs'; import { execGit as execGitSeam } from './shell-command-projection.cjs'; +const { error, ERROR_REASON } = io; +const { loadConfig: loadConfigSeam } = configLoader; + // ─── Types ──────────────────────────────────────────────────────────────────── type ExecGitFn = typeof execGitSeam; +type LoadConfigFn = typeof loadConfigSeam; export interface BaseBranchDeps { /** Override the git runner (default: execGit from shell-command-projection) */ execGit?: ExecGitFn; - /** Override filesystem reads (default: fs.readFileSync / fs.existsSync) */ + /** + * Test-only low-level config-file seam for {@link readEffectiveGitConfig}. + * When supplied without `loadConfig`, reads only `/.planning/config.json`, + * applies legacy-key normalization in memory, and performs no merge, defaults, + * warning, or write-back. Production callers must use `loadConfig` instead. + */ readFile?: (p: string) => string | null; + /** Override effective configuration loading (default: config-loader.loadConfig) */ + loadConfig?: LoadConfigFn; /** Inject the write function used by cmdGitBaseBranch (default: process.stdout.write) */ write?: (s: string) => void; /** @@ -56,36 +72,121 @@ export interface BaseBranchDeps { // ─── Helpers ────────────────────────────────────────────────────────────────── -/** - * Safely look up `git.base_branch` from the project's config.json. - * Returns the configured value (a non-empty, non-null string) or null. - */ -export function readConfigBaseBranch( - planningDir: string, - deps?: Pick -): string | null { - const readFile: (p: string) => string | null = deps?.readFile ?? - ((p: string) => { try { return fs.readFileSync(p, 'utf8'); } catch { return null; } }); +interface EffectiveGitConfig { + baseBranch: string | null; + protectedBranches: string[]; + /** Rendered form of every entry rejected as unusable, for the caller to report. */ + rejectedProtectedBranches: string[]; +} - const configPath = path.join(planningDir, 'config.json'); - const raw = readFile(configPath); - if (!raw) return null; - - let cfg: unknown; - try { cfg = JSON.parse(raw); } catch { return null; } - if (!cfg || typeof cfg !== 'object' || Array.isArray(cfg)) return null; - - const top = cfg as Record; - // Support both "git.base_branch" (nested) and "base_branch" (flat legacy) - const gitSection = top.git; - if (gitSection && typeof gitSection === 'object' && !Array.isArray(gitSection)) { - const nested = (gitSection as Record).base_branch; - if (typeof nested === 'string' && nested.trim()) return nested.trim(); +/** Render a rejected config value for a diagnostic without throwing on exotic input. */ +function renderRejected(value: unknown): string { + try { + return sanitizeLabel(JSON.stringify(value) ?? String(value)); + } catch { + return sanitizeLabel(String(value)); } - const flat = top.base_branch; - if (typeof flat === 'string' && flat.trim()) return flat.trim(); +} - return null; +/** Nested-only read of `git.`, mirroring `loadConfigResolved`'s `getNested`. */ +export function _readGitNested(config: Record, field: string): unknown { + const git = config['git']; + if (git !== null && typeof git === 'object' && !Array.isArray(git)) { + return (git as Record)[field]; + } + return undefined; +} + +/** + * Flat-then-nested read, mirroring `loadConfigResolved`'s own `get()`. + * + * Used for `base_branch` ONLY, and only because it HAS a legacy flat spelling: + * `normalizeLegacyKeys` normally hoists it away, so a flat key that SURVIVED + * normalization is one the migration refused (a non-object `git` section, + * #3760) and is the user's last remaining expression of intent. + * + * `protected_branches` deliberately does NOT use this. It is new in #3552 with + * no legacy form, so honouring a top-level spelling would invent an + * undocumented alias that silently outranks the canonical nested key + * (round-4 external review). + */ +export function _readGitKey(config: Record, field: string): unknown { + if (config[field] !== undefined) return config[field]; + return _readGitNested(config, field); +} + +/** + * Read the effective root/workstream configuration once for branch policy. + * + * Production takes the `loadConfig` branch, with `persist: false` — this is a + * PREDICATE, invoked on every `execute-phase` and every `ship` run, and a + * question must not rewrite the file it is asking about. Without it, any project + * carrying a legacy flat key (`base_branch`, `branching_strategy`, `depth`, …) + * has `.planning/config.json` silently normalized and rewritten by a call whose + * entire contract is to answer a boolean (#3648 review Blocker 1). + * + * The `readFile` branch is a unit-test seam, NOT a second production path, and + * it is deliberately narrower than `loadConfig`. It covers exactly two of + * production's steps — `normalizeLegacyKeys`, then the flat-then-nested lookup + * — over a single `/.planning/config.json`. It does NOT apply + * root/workstream `_deepMergeConfig`, builtin or `~/.gsd/defaults.json` + * defaults, or `mergeFederatedConfig`. Tests that assert on any of those must + * drive `loadConfig` instead; the seam's own tests are scoped to normalization + * and shape validation, which is all it reproduces (#3648 review Major 3). + */ +function readEffectiveGitConfig( + cwd: string, + deps?: Pick +): EffectiveGitConfig { + let config: Record = {}; + + if (deps?.readFile && !deps.loadConfig) { + const raw = deps.readFile(path.join(cwd, '.planning', 'config.json')); + if (raw) { + try { + const parsed: unknown = JSON.parse(raw); + if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { + const { parsed: normalized } = normalizeLegacyKeys(parsed as Record); + config = { + base_branch: _readGitKey(normalized, 'base_branch'), + protected_branches: _readGitNested(normalized, 'protected_branches'), + }; + } + } catch { /* malformed direct edit contributes no policy values */ } + } + } else { + try { + config = (deps?.loadConfig ?? loadConfigSeam)(cwd, { persist: false }); + } catch { /* configuration loading is fail-soft for branch resolution */ } + } + + const rawBaseBranch = config.base_branch; + const baseBranch = typeof rawBaseBranch === 'string' && rawBaseBranch.trim() + ? rawBaseBranch.trim() + : null; + // A protection predicate must not fail OPEN. `config-set` validation is + // bypassable by a direct edit of .planning/config.json, so one bad element + // discarding the whole list would silently answer "not protected" for names + // the user believes are protected — the exact failure #3552 exists to close, + // reintroduced through a different door (#3648 review Blocker 3). Drop only + // the bad elements, and report every rejection so it cannot pass unnoticed. + const rawProtectedBranches = config.protected_branches; + const protectedBranches: string[] = []; + const rejectedProtectedBranches: string[] = []; + if (Array.isArray(rawProtectedBranches)) { + for (const branch of rawProtectedBranches) { + if (typeof branch === 'string' && branch.trim() !== '') { + protectedBranches.push(branch.trim()); + } else { + rejectedProtectedBranches.push(renderRejected(branch)); + } + } + } else if (rawProtectedBranches !== undefined && rawProtectedBranches !== null) { + // Not a list at all — contributes no names, but is still a misconfiguration. + rejectedProtectedBranches.push(renderRejected(rawProtectedBranches)); + } + + return { baseBranch, protectedBranches, rejectedProtectedBranches }; } /** @@ -202,9 +303,10 @@ export interface ResolvedBaseBranch { * Consults the full precedence ladder and always returns a non-empty string. * Never throws. */ -export function resolveBaseBranchDiagnostics( +function resolveBaseBranchDiagnosticsWithConfig( cwd: string, - deps?: BaseBranchDeps + configured: string | null, + deps?: BaseBranchDeps, ): ResolvedBaseBranch { const rawExecGit: ExecGitFn = deps?.execGit ?? execGitSeam; // A genuine execGit failure (timeout, or the call could not even spawn — @@ -221,11 +323,7 @@ export function resolveBaseBranchDiagnostics( return r; }; - // Derive .planning dir relative to cwd (mirrors planningDir() in planning-workspace.cjs) - const planningDir = path.join(cwd, '.planning'); - // 1. Config override - const configured = readConfigBaseBranch(planningDir, deps); if (configured) return { branch: configured, verified: true }; // 2. symbolic-ref (fast, no network) @@ -247,6 +345,14 @@ export function resolveBaseBranchDiagnostics( return { branch: 'main', verified: !anyGitFailure }; } +export function resolveBaseBranchDiagnostics( + cwd: string, + deps?: BaseBranchDeps +): ResolvedBaseBranch { + const { baseBranch } = readEffectiveGitConfig(cwd, deps); + return resolveBaseBranchDiagnosticsWithConfig(cwd, baseBranch, deps); +} + /** * Resolve the default/base branch for the repository at `cwd`. * @@ -261,6 +367,37 @@ export function resolveBaseBranch( return resolveBaseBranchDiagnostics(cwd, deps).branch; } +export interface ProtectedBranchStatus { + baseBranch: string; + protectedBranches: string[]; + /** Rendered `git.protected_branches` entries that were unusable and ignored. */ + rejectedProtectedBranches: string[]; + isProtected: boolean; + verified: boolean; +} + +/** Resolve the base branch plus configured protected-branch extensions. */ +export function resolveProtectedBranchStatus( + cwd: string, + currentBranch: string, + deps?: BaseBranchDeps +): ProtectedBranchStatus { + const effectiveConfig = readEffectiveGitConfig(cwd, deps); + const { branch: baseBranch, verified } = resolveBaseBranchDiagnosticsWithConfig( + cwd, + effectiveConfig.baseBranch, + deps, + ); + const protectedBranches = [...new Set([baseBranch, ...effectiveConfig.protectedBranches])]; + return { + baseBranch, + protectedBranches, + rejectedProtectedBranches: effectiveConfig.rejectedProtectedBranches, + isProtected: protectedBranches.includes(currentBranch), + verified, + }; +} + // ─── gitWorktreeInfoInternal (moved from core.cjs, ADR-857 T0 #1268) ───────── export interface GitWorktreeInfo { @@ -413,9 +550,60 @@ export function changedFilesSince( */ export function cmdGitBaseBranch( cwd: string, - _args: string[], + args: string[], deps?: BaseBranchDeps ): string { + if (args[0] === '--is-protected') { + if (args.length > 2) { + error('Usage: git base-branch --is-protected []', ERROR_REASON.USAGE); + } + const writeDiagnostic = deps?.writeDiagnostic ?? ((s: string) => process.stderr.write(s)); + // `git branch --show-current` prints nothing on a detached HEAD, so the + // call sites legitimately pass an explicit empty string: no protected + // branch is named '', the answer is false, and that is not a fault worth + // reporting. The flag with NO argument is a different thing — a caller bug + // that `args[1] ?? ''` used to collapse into the detached-HEAD case. Same + // handling, but said out loud so the two can be told apart. + // + // This diagnostic deliberately does NOT state the answer. The empty branch + // matches no protected name, but the fail-closed guard below still renders + // `true` when the base branch could not be verified — so promising "false" + // here would contradict what this same call prints on stdout (#3648 review). + if (args.length < 2) { + writeDiagnostic( + `⚠ git-base-branch: --is-protected was called without a branch argument; ` + + `treating it as an empty branch name. Pass the branch to test, ` + + `e.g. --is-protected "$CURRENT_BRANCH".\n` + ); + } + const status = resolveProtectedBranchStatus(cwd, args[1] ?? '', deps); + if (status.rejectedProtectedBranches.length > 0) { + writeDiagnostic( + `⚠ git-base-branch: ignoring ${status.rejectedProtectedBranches.length} unusable ` + + `git.protected_branches entr${status.rejectedProtectedBranches.length === 1 ? 'y' : 'ies'} ` + + `(${status.rejectedProtectedBranches.join(', ')}) — each must be a non-empty branch name. ` + + `The remaining names are still enforced. See #3552.\n` + ); + } + // A protection guard must fail closed: if the base branch could not be + // verified against this repository (a git query timed out or failed to + // run — #3057 B4), report "protected" rather than silently trusting an + // unverified guess that might happen to not match the current branch. + const rendered = String(status.verified ? status.isProtected : true); + if (!status.verified) { + writeDiagnostic( + `⚠ git-base-branch: --is-protected could not verify repository branch metadata; ` + + `defaulting to protected (fail-closed). See #3057.\n` + ); + } + const write = deps?.write ?? ((s: string) => process.stdout.write(s)); + write(rendered + '\n'); + return rendered; + } + if (args.length > 0) { + error(`Unknown flag for git.base-branch: ${args[0]}`, ERROR_REASON.USAGE); + } + const { branch, verified } = resolveBaseBranchDiagnostics(cwd, deps); if (!verified) { const writeDiagnostic = deps?.writeDiagnostic ?? ((s: string) => process.stderr.write(s)); diff --git a/tests/config-field-docs.test.cjs b/tests/config-field-docs.test.cjs index b2c74b700..c71ccf28a 100644 --- a/tests/config-field-docs.test.cjs +++ b/tests/config-field-docs.test.cjs @@ -17,6 +17,15 @@ const { splitTableRow } = require('../gsd-core/bin/lib/markdown-table.cjs'); const REFERENCE_PATH = path.join(__dirname, '..', 'gsd-core', 'references', 'planning-config.md'); const CORE_PATH = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'config-loader.cjs'); +const DOCS_CONFIG_PATH = path.join(__dirname, '..', 'docs', 'CONFIGURATION.md'); +const CONFIG_SCHEMA_MANIFEST_PATH = path.join( + __dirname, + '..', + 'gsd-core', + 'bin', + 'shared', + 'config-schema.manifest.json', +); /** Find the markdown table row whose first cell is `` `key` `` and return its cells. */ function tableRowForKey(content, key) { @@ -155,6 +164,46 @@ describe('config-field-docs', () => { ); }); + test('git.protected_branches canonical field has synchronized type and examples', () => { + const manifest = JSON.parse(fs.readFileSync(CONFIG_SCHEMA_MANIFEST_PATH, 'utf-8')); + assert.ok( + manifest.validKeys.includes('git.protected_branches'), + 'config schema manifest must register git.protected_branches', + ); + + const publicDocs = fs.readFileSync(DOCS_CONFIG_PATH, 'utf-8'); + const references = [ + ['docs/CONFIGURATION.md', publicDocs], + ['gsd-core/references/planning-config.md', content], + ]; + const example = /"protected_branches"\s*:\s*\[\s*"develop"\s*,\s*"staging"\s*\]/; + + for (const [name, reference] of references) { + const row = reference + .split(/\r?\n/) + .find((line) => line.startsWith('| `git.protected_branches` |')); + assert.ok(row, `${name} must document the canonical git.protected_branches key`); + assert.match(row, /array of non-empty strings/i, + `${name} must document the non-empty string-array contract`); + assert.match(row, /\| \(none\) \|/, + `${name} must document that the optional field has no persisted default`); + assert.match(reference, example, + `${name} must show the synchronized multi-branch JSON example`); + assert.match(reference, /extends the resolved base branch/i, + `${name} must state that configured names extend the resolved base`); + assert.match(reference, /execute-phase and ship/i, + `${name} must name both advisory warning boundaries`); + assert.match(reference, /does not\s+change\s+`git\.branching_strategy: "none"`/i, + `${name} must preserve branching_strategy none behavior`); + assert.match(reference, /exact branch name/i, + `${name} must state that matching is by exact name`); + assert.match(reference, /no glob or prefix/i, + `${name} must say globs and prefixes are unsupported, so git-flow layouts enumerate`); + assert.match(reference, /remaining names\s+still apply/i, + `${name} must state that an invalid entry drops only itself`); + } + }); + test('documents KNOWN_TOP_LEVEL internal fields not in CONFIG_DEFAULTS', () => { // These fields are in KNOWN_TOP_LEVEL (core.cjs) and read by loadConfig() // but not in CONFIG_DEFAULTS, so the CONFIG_DEFAULTS test doesn't cover them. diff --git a/tests/config-get-default.test.cjs b/tests/config-get-default.test.cjs index fc63c980d..2a3bd5abe 100644 --- a/tests/config-get-default.test.cjs +++ b/tests/config-get-default.test.cjs @@ -641,6 +641,15 @@ describe('config-get --default flag (#1893)', () => { { numRuns: 60 }, ); }); + + test('absent git.protected_branches has no schema default ((none) per docs)', () => { + fs.mkdirSync(planningDir, { recursive: true }); + fs.writeFileSync(path.join(planningDir, 'config.json'), '{}'); + const { reason } = runExpectError('config-get', 'git.protected_branches'); + assert.equal(reason, io.ERROR_REASON.CONFIG_KEY_NOT_FOUND); + const fallback = runRaw('config-get', 'git.protected_branches', '--default', '["main"]'); + assert.equal(fallback, '["main"]'); + }); }); } diff --git a/tests/config.test.cjs b/tests/config.test.cjs index 914374a79..b61febc24 100644 --- a/tests/config.test.cjs +++ b/tests/config.test.cjs @@ -436,6 +436,90 @@ describe('config-set command', () => { }); }); +// ─── config-set git.protected_branches (#3552) ────────────────────────────── + +describe('config-set git.protected_branches (#3552)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + runGsdTools('config-ensure-section', tmpDir); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('accepts and persists multiple non-empty branch names', () => { + const branches = ['develop', 'next']; + const result = runGsdTools( + ['config-set', 'git.protected_branches', JSON.stringify(branches)], + tmpDir, + ); + + assert.ok(result.success, `Command failed: ${result.error}`); + assert.deepStrictEqual(readConfig(tmpDir).git.protected_branches, branches); + }); + + test('rejects invalid shapes and preserves the previous list', () => { + const previous = ['develop', 'next']; + const seed = runGsdTools( + ['config-set', 'git.protected_branches', JSON.stringify(previous)], + tmpDir, + ); + assert.ok(seed.success, `Seed failed: ${seed.error}`); + + const invalidValues = [ + 'develop', + { develop: true }, + ['develop', 42], + [], + [''], + ['develop', ' '], + ]; + + for (const invalidValue of invalidValues) { + const result = runGsdTools( + ['config-set', 'git.protected_branches', JSON.stringify(invalidValue)], + tmpDir, + ); + assert.strictEqual( + result.success, + false, + `Expected rejection for ${JSON.stringify(invalidValue)}, got: ${result.output}`, + ); + assert.deepStrictEqual( + readConfig(tmpDir).git.protected_branches, + previous, + `Rejected value ${JSON.stringify(invalidValue)} must not change the prior list`, + ); + } + }); + + test('is absent by default and null removes the configured list', () => { + assert.ok( + !Object.prototype.hasOwnProperty.call(readConfig(tmpDir).git, 'protected_branches'), + 'config-ensure-section must not add a protected_branches default', + ); + + const setResult = runGsdTools( + ['config-set', 'git.protected_branches', '["develop","next"]'], + tmpDir, + ); + assert.ok(setResult.success, `Set failed: ${setResult.error}`); + + const unsetResult = runGsdTools( + ['config-set', 'git.protected_branches', 'null'], + tmpDir, + ); + assert.ok(unsetResult.success, `Unset failed: ${unsetResult.error}`); + assert.ok( + !Object.prototype.hasOwnProperty.call(readConfig(tmpDir).git, 'protected_branches'), + 'git.protected_branches must be absent after unset', + ); + }); +}); + // ─── config-get ────────────────────────────────────────────────────────────── describe('config-get command', () => { diff --git a/tests/configuration-normalize-legacy-keys.test.cjs b/tests/configuration-normalize-legacy-keys.test.cjs new file mode 100644 index 000000000..afcf117d1 --- /dev/null +++ b/tests/configuration-normalize-legacy-keys.test.cjs @@ -0,0 +1,159 @@ +'use strict'; + +/** + * Tests for `normalizeLegacyKeys` block 5 — top-level `base_branch` → + * `git.base_branch` (#3648). + * + * Block 5 is new in this PR and is a fifth trigger for the write-on-normalize + * path, so it must obey the same contract #3760 established for blocks 1-3: + * hoist into an absent or object section, and REFUSE — preserving the legacy + * key, pushing no `Normalization`, and reporting via `skipped` — when the + * destination section is present but is not an object. + * + * #3760's own suite (tests/configuration-migrate-config.test.cjs) locks that + * contract for blocks 1, 2 and 3. This file locks it for block 5, which did not + * exist when that suite was written, plus block 5's ordinary hoist semantics. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fc = require('./helpers/fast-check-setup.cjs'); +const { normalizeLegacyKeys } = require('../gsd-core/bin/lib/configuration.cjs'); + +/** Keys only a string/array spread can produce. */ +function numericKeys(obj) { + return Object.keys(obj).filter((k) => /^\d+$/.test(k)); +} + +describe('#3648 normalizeLegacyKeys block 5 — ordinary hoist semantics', () => { + test('an absent `git` section is created carrying the hoisted value', () => { + const { parsed, normalizations, skipped } = normalizeLegacyKeys({ base_branch: 'release' }); + + assert.deepStrictEqual(parsed.git, { base_branch: 'release' }); + assert.strictEqual(parsed.base_branch, undefined, 'the stale flat key must be dropped'); + assert.deepStrictEqual(skipped, []); + assert.deepStrictEqual( + normalizations.find((n) => n.from === 'base_branch'), + { from: 'base_branch', to: 'git.base_branch', value: 'release' }, + ); + }); + + test('an existing `git` section is merged into, not replaced', () => { + const { parsed } = normalizeLegacyKeys({ + git: { phase_branch_template: 'x' }, + base_branch: 'release', + }); + + assert.deepStrictEqual(parsed.git, { phase_branch_template: 'x', base_branch: 'release' }); + }); + + test('a null `git` section still means "absent" and is created', () => { + const { parsed, skipped } = normalizeLegacyKeys({ git: null, base_branch: 'release' }); + + assert.deepStrictEqual(parsed.git, { base_branch: 'release' }); + assert.deepStrictEqual(skipped, [], 'null is absence, not a malformed section'); + }); + + test('a canonical `git.base_branch` outranks the flat key, which is dropped', () => { + const { parsed, normalizations } = normalizeLegacyKeys({ + git: { base_branch: 'nested' }, + base_branch: 'flat', + }); + + assert.strictEqual(parsed.git.base_branch, 'nested', 'canonical value must win'); + assert.strictEqual(parsed.base_branch, undefined); + assert.ok(normalizations.find((n) => n.from === 'base_branch'), + 'an entry is still recorded so the stale flat key is written away'); + }); +}); + +describe('#3648 normalizeLegacyKeys block 5 — a non-object `git` section blocks the migration', () => { + // Same contract as #3760 blocks 1-3: preserve, refuse, report. Anything else + // is destructive, because config-loader persists whatever normalization + // produced whenever `normalizations` is non-empty. + + for (const [label, section] of [ + ['string', 'main'], + ['number', 42], + ['boolean', true], + ['array', ['main']], + ]) { + test(`a ${label} \`git\` section: value preserved, nothing normalized, refusal reported`, () => { + const input = { git: section, base_branch: 'release' }; + const { parsed, normalizations, skipped } = normalizeLegacyKeys(input); + + assert.deepStrictEqual(parsed.git, section, 'the section value must survive verbatim'); + assert.strictEqual(parsed.base_branch, 'release', 'the legacy key must NOT be consumed'); + assert.deepStrictEqual( + normalizations.filter((n) => n.from === 'base_branch'), [], + 'pushing a Normalization is what makes config-loader write the file back', + ); + assert.deepStrictEqual(skipped, [{ + from: 'base_branch', + to: 'git.base_branch', + section: 'git', + reason: 'non_object_section', + value: 'release', + sectionType: label, + }]); + }); + } + + test('negative control: the refusal is scoped to block 5, not to the whole call', () => { + // A non-object `git` must not stop an unrelated block from normalizing — + // otherwise the guard is a blanket bail-out rather than a per-section one. + const { parsed, normalizations } = normalizeLegacyKeys({ + git: 'main', + base_branch: 'release', + depth: 'quick', + }); + + assert.strictEqual(parsed.granularity, 'coarse', 'block 4 must still run'); + assert.ok(normalizations.find((n) => n.from === 'depth')); + }); +}); + +describe('#3648 normalizeLegacyKeys block 5 — property: hoist or refuse, never corrupt', () => { + test('across arbitrary `git` values the two outcomes are exhaustive and exclusive', () => { + fc.assert( + fc.property( + fc.oneof( + fc.string(), + fc.integer(), + fc.boolean(), + fc.constant(null), + fc.array(fc.string()), + fc.dictionary(fc.string(), fc.string()), + ), + fc.string({ minLength: 1 }), + (gitValue, branch) => { + const receivable = gitValue === null + || (typeof gitValue === 'object' && !Array.isArray(gitValue)); + const carriedIn = receivable && gitValue !== null ? numericKeys(gitValue) : []; + + const { parsed, normalizations, skipped } = + normalizeLegacyKeys({ git: gitValue, base_branch: branch }); + + const hoisted = normalizations.some((n) => n.from === 'base_branch'); + const refused = skipped.some((s) => s.from === 'base_branch'); + assert.notStrictEqual(hoisted, refused, + 'exactly one of the two outcomes must be reported for this key'); + assert.strictEqual(hoisted, receivable); + + if (receivable) { + assert.ok(parsed.git && typeof parsed.git === 'object' && !Array.isArray(parsed.git)); + assert.strictEqual(parsed.git.base_branch, branch); + assert.strictEqual(parsed.base_branch, undefined); + // No numeric key beyond whatever the input section already had: a + // spread of a non-object is what would manufacture them. + assert.deepStrictEqual(numericKeys(parsed.git).sort(), carriedIn.sort()); + } else { + assert.deepStrictEqual(parsed.git, gitValue); + assert.strictEqual(parsed.base_branch, branch); + } + }, + ), + { numRuns: 300 }, + ); + }); +}); diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index 2aa171cf8..8f19fbc31 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index 1e2846c75..f8cfb7028 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index 3676036a3..4b6039988 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index 9234e6329..b3dbf6e38 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/cline.json b/tests/fixtures/install-tree/cline.json index c2bd74306..987afd5b6 100644 --- a/tests/fixtures/install-tree/cline.json +++ b/tests/fixtures/install-tree/cline.json @@ -279,6 +279,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index ff233ec9b..43311ed72 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 5bb869a38..279c8acce 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -313,6 +313,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/copilot.json b/tests/fixtures/install-tree/copilot.json index f98192bab..e7d4ef5b4 100644 --- a/tests/fixtures/install-tree/copilot.json +++ b/tests/fixtures/install-tree/copilot.json @@ -278,6 +278,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index 8cb07da4c..86b63695c 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index 26dc283de..09cd73956 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 154e553de..635aa8da5 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 391b54599..e2ca89fb0 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -278,6 +278,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/kimi.json b/tests/fixtures/install-tree/kimi.json index 35c06483b..67b57e72b 100644 --- a/tests/fixtures/install-tree/kimi.json +++ b/tests/fixtures/install-tree/kimi.json @@ -314,6 +314,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index ca6fc5b74..04ab1c3ba 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index a68c00c88..4da5691df 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -244,6 +244,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 228ff0937..251cdbd27 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/trae.json b/tests/fixtures/install-tree/trae.json index 9c6bb1909..6a6c93d1e 100644 --- a/tests/fixtures/install-tree/trae.json +++ b/tests/fixtures/install-tree/trae.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index 0d029ce51..d07825a4e 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -277,6 +277,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/fixtures/install-tree/zcode.json b/tests/fixtures/install-tree/zcode.json index d6a63ba7c..851c1dce1 100644 --- a/tests/fixtures/install-tree/zcode.json +++ b/tests/fixtures/install-tree/zcode.json @@ -348,6 +348,7 @@ "gsd-core/workflows/execute-phase/steps/per-plan-executor-routing.md", "gsd-core/workflows/execute-phase/steps/per-plan-worktree-gate.md", "gsd-core/workflows/execute-phase/steps/post-merge-gate.md", + "gsd-core/workflows/execute-phase/steps/protected-branch.md", "gsd-core/workflows/execute-phase/steps/regression-gate-run.md", "gsd-core/workflows/execute-phase/steps/regression-gate.md", "gsd-core/workflows/execute-phase/steps/wave-post-gate-hooks.md", diff --git a/tests/git-base-branch.test.cjs b/tests/git-base-branch.test.cjs index 244e57733..46120320e 100644 --- a/tests/git-base-branch.test.cjs +++ b/tests/git-base-branch.test.cjs @@ -30,6 +30,7 @@ const os = require('node:os'); const path = require('node:path'); const { runGsdTools, cleanup, readFileNormalized } = require('./helpers.cjs'); +const { ExitError } = require('../gsd-core/bin/lib/cli-exit.cjs'); const { makeFaultyGit } = require('./helpers/faulty-deps.cjs'); const { gitOrThrow, throwIfFailed } = require('./helpers/git-fixture.cjs'); const { runHook } = require('./helpers/process-seam.cjs'); @@ -64,6 +65,21 @@ function addPlanning(dir) { fs.mkdirSync(path.join(dir, '.planning', 'phases'), { recursive: true }); } +/** Snapshot every file below .planning, including bytes and relative paths. */ +function snapshotPlanningTree(dir) { + const root = path.join(dir, '.planning'); + const snapshot = new Map(); + function visit(current) { + for (const entry of fs.readdirSync(current, { withFileTypes: true })) { + const absolute = path.join(current, entry.name); + if (entry.isDirectory()) visit(absolute); + else snapshot.set(path.relative(root, absolute), fs.readFileSync(absolute)); + } + } + visit(root); + return snapshot; +} + /** * Write a gsd config.json with git.base_branch set. */ @@ -234,6 +250,30 @@ describe('#1146: git.base-branch resolver', () => { `Expected flat config override 'release', got: '${branch}'`); }); + test('A3. #3648 precedence: both flat base_branch and nested git.base_branch set → nested (canonical) wins', (t) => { + // Regression for #3648 review Blocker 1: production config resolution + // (config-loader's `get()`) is flat-first, so a project that migrated to + // the namespaced `git.base_branch` but still carries a stale flat + // `base_branch` from before migration would silently get the old flat + // value back. `normalizeLegacyKeys` must hoist/resolve `base_branch` the + // same way it already does `branching_strategy` — canonical nested wins, + // stale flat top-level is dropped. + const dir = createGitRepo({ prefix: 'gsd-3648-a3-', defaultBranch: 'master' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + const cfgPath = require('node:path').join(dir, '.planning', 'config.json'); + require('node:fs').writeFileSync( + cfgPath, + JSON.stringify({ base_branch: 'stale-flat', git: { base_branch: 'canonical-nested' } }, null, 2) + '\n', + ); + + const result = runGsdTools(['query', 'git.base-branch'], dir); + assert.ok(result.success, `git.base-branch with both config keys failed:\n${result.error}`); + const branch = result.output.trim(); + assert.strictEqual(branch, 'canonical-nested', + `Expected namespaced 'git.base_branch' to win over stale flat 'base_branch', got: '${branch}'`); + }); + test('H. No remote, both "main" and "master" local branches exist → returns "main" (main wins tie-break)', (t) => { // Tier-4 tie-break: when both main and master exist locally and no remote info is available, // "main" wins (documented in tryLocalBranch JSDoc — modern default). @@ -337,7 +377,7 @@ describe('#3057 B4: resolveBaseBranchDiagnostics — verified vs unverified last const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3057-b4-fault-')); t.after(() => cleanup(dir)); // No .planning/config.json in this dir → the config-override tier is - // skipped naturally (readConfigBaseBranch's real-fs read misses cleanly). + // skipped naturally (loadConfig's real-fs read misses cleanly). const faultyGit = makeFaultyGit({ faults: [{ kind: 'timeout' }] }); const result = gitBaseBranch.resolveBaseBranchDiagnostics(dir, { execGit: faultyGit }); @@ -400,6 +440,1044 @@ describe('#3057 B4: resolveBaseBranchDiagnostics — verified vs unverified last assert.strictEqual(stdoutText, 'main\n'); assert.strictEqual(stderrText, '', 'a verified fallback must not write any diagnostic'); }); + + test('#3648 Major: --is-protected fails CLOSED (reports protected) when the base branch is unverified', (t) => { + // Regression for #3648 review's Major finding: `--is-protected` used to + // discard `verified` entirely, so a degraded git (timeout / spawn + // failure) silently answered `false` — the wrong failure direction for a + // protection guard. An unverified guess must not let a caller conclude + // "definitely not protected". + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3648-major-')); + t.after(() => cleanup(dir)); + + let stdoutText = ''; + let stderrText = ''; + gitBaseBranch.cmdGitBaseBranch(dir, ['--is-protected', 'some-topic-branch'], { + execGit: makeFaultyGit({ faults: [{ kind: 'timeout' }] }), + write: (s) => { stdoutText += s; }, + writeDiagnostic: (s) => { stderrText += s; }, + }); + assert.strictEqual(stdoutText, 'true\n', + 'an unverified answer must fail closed (report protected), not silently false'); + assert.match(stderrText, /could not verify repository branch metadata/); + + // Negative control: a verified resolution for the same non-matching + // branch must still cleanly report false — the fail-closed path must + // trigger on non-verification, not on every "not protected" answer. + stdoutText = ''; + stderrText = ''; + gitBaseBranch.cmdGitBaseBranch(dir, ['--is-protected', 'some-topic-branch'], { + execGit: makeFaultyGit(), + write: (s) => { stdoutText += s; }, + writeDiagnostic: (s) => { stderrText += s; }, + }); + assert.strictEqual(stdoutText, 'false\n'); + assert.strictEqual(stderrText, '', 'a verified non-match must not write any diagnostic'); + }); +}); + +// ─── #3552: protected-branch policy and execute-phase warning ──────────────── + +function extractProtectedBranchWarningBash(workflowFile, stepName) { + const content = readFileNormalized(path.join(WORKFLOW_DIR, workflowFile)); + const lines = content.split('\n'); + const blocks = []; + let inStep = false; + let inBash = false; + let buffer = []; + + for (const line of lines) { + if (!inStep && line === ``) { + inStep = true; + continue; + } + if (inStep && /^<\/step>\s*$/.test(line)) break; + if (inStep && !inBash && /^\s*```bash\s*$/.test(line)) { + inBash = true; + buffer = []; + continue; + } + if (inBash && /^\s*```\s*$/.test(line)) { + blocks.push(buffer.join('\n')); + inBash = false; + continue; + } + if (inBash) buffer.push(line); + } + + let block = blocks.find((candidate) => candidate.includes('--is-protected')); + if (!block) { + // #3648: the step may point at an extracted step file (e.g. execute-phase.md's + // ADR-857 byte-ceiling extraction) instead of carrying the bash block inline. + const stepBody = lines.slice(lines.indexOf(``) + 1); + const ref = stepBody.find((l) => /`execute-phase\/steps\/[\w-]+\.md`/.test(l)); + const refMatch = ref && ref.match(/`(execute-phase\/steps\/[\w-]+\.md)`/); + if (refMatch) { + const refLines = readFileNormalized(path.join(WORKFLOW_DIR, refMatch[1])).split('\n'); + const refBlocks = []; + let refInBash = false; + let refBuffer = []; + for (const line of refLines) { + if (!refInBash && /^\s*```bash\s*$/.test(line)) { + refInBash = true; + refBuffer = []; + continue; + } + if (refInBash && /^\s*```\s*$/.test(line)) { + refBlocks.push(refBuffer.join('\n')); + refInBash = false; + continue; + } + if (refInBash) refBuffer.push(line); + } + block = refBlocks.find((candidate) => candidate.includes('--is-protected')); + // #3648: sync-runtime-launcher.cjs adds the canonical gsd_run resolver + // preamble to this standalone step file (runtime-launcher-parity's + // one-preamble-per-file rule). That preamble defines its own gsd_run(), + // which would shadow this harness's injected mock and reach the real + // gsd-tools.cjs on the machine running the test. The preamble's own + // correctness is covered by tests/runtime-launcher-parity.test.cjs; this + // harness only needs the #3552 warning logic, so strip it here. + if (block) { + block = block.split('\n').filter((l) => !l.trimStart().startsWith('_GSD_SHIM_NAME=')).join('\n'); + } + } + } + if (!block) { + throw new Error(`${workflowFile} ${stepName} has no protected-branch warning bash block`); + } + return block; +} + +function writeProtectedBranchWarningScript(prefix, bash) { + const scriptDir = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const scriptPath = path.join(scriptDir, 'warning.sh'); + fs.writeFileSync(scriptPath, [ + '#!/usr/bin/env bash', + 'set -eu', + 'git() {', + ' if [ "$#" -ne 2 ] || [ "$1" != branch ] || [ "$2" != --show-current ]; then', + ' printf "unexpected git invocation\\n" >&2', + ' return 97', + ' fi', + ' printf "%s\\n" "$CURRENT_BRANCH_VALUE"', + '}', + 'gsd_run() {', + ' if [ "$#" -ne 4 ] || [ "$1" != query ] || [ "$2" != git.base-branch ] ||', + ' [ "$3" != --is-protected ] || [ "$4" != "$CURRENT_BRANCH_VALUE" ]; then', + ' printf "unexpected protected-branch query\\n"', + ' return 0', + ' fi', + // The real command writes its fail-closed and rejected-entry + // explanations to stderr. Emitting one here is what makes a swallowed + // `2>/dev/null` at the call site visible to a test. + ' if [ -n "${DIAGNOSTIC_TEXT:-}" ]; then printf "%s\\n" "$DIAGNOSTIC_TEXT" >&2; fi', + // QUERY_EXIT lets a test make the query FAIL (gsd-tools missing, a crash, a + // non-zero exit). Defaults to 0, so every pre-existing caller of this + // harness is unaffected. Nothing is printed on stdout in that case — + // matching a real failed command substitution. + ' if [ "${QUERY_EXIT:-0}" != 0 ]; then return "${QUERY_EXIT}"; fi', + ' printf "%s\\n" "$PROTECTED_RESULT"', + '}', + bash, + 'printf "IS_PROTECTED=%s\\n" "${IS_PROTECTED:-unbound}"', + 'printf "continued\\n"', + ].join('\n'), { mode: 0o755 }); + return { scriptDir, scriptPath }; +} + +describe('#3552: configured protected branches', () => { + const configuredLoad = (requestedCwd) => { + assert.strictEqual(requestedCwd, '/repo'); + return { + base_branch: 'main', + protected_branches: ['develop', 'next', 'develop'], + }; + }; + + test('#3552 configured match and unrelated branch produce opposite results', () => { + const match = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'develop', { + loadConfig: configuredLoad, + }); + const control = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'topic/3552', { + loadConfig: configuredLoad, + }); + + assert.deepStrictEqual(match, { + baseBranch: 'main', + protectedBranches: ['main', 'develop', 'next'], + rejectedProtectedBranches: [], + isProtected: true, + verified: true, + }); + assert.strictEqual(control.isProtected, false); + assert.notStrictEqual(match.isProtected, control.isProtected, + 'negative control must disagree with the configured protected-branch match'); + }); + + test('#3552 absent list retains resolved-base protection and unrelated control', () => { + const deps = { loadConfig: () => ({ base_branch: 'main' }) }; + const base = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'main', deps); + const control = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'topic/3552', deps); + + assert.deepStrictEqual(base.protectedBranches, ['main']); + assert.strictEqual(base.isProtected, true); + assert.strictEqual(control.isProtected, false); + assert.notStrictEqual(base.isProtected, control.isProtected, + 'negative control must disagree with the resolved-base match'); + }); + + test('#3552 a bad element drops only itself — valid names still protect', () => { + // A protection predicate must not fail OPEN. config-set validation is + // bypassable by a direct edit of .planning/config.json, so one bad element + // discarding the whole list is the exact failure #3552 exists to close, + // reintroduced through a different door (#3648 review Blocker 3). + for (const protectedBranches of [['develop', 42], ['develop', ' '], ['develop', null]]) { + const loadConfig = () => ({ base_branch: 'main', protected_branches: protectedBranches }); + const configuredName = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'develop', { loadConfig }); + + assert.strictEqual(configuredName.isProtected, true, + `'develop' must stay protected alongside a bad sibling: ${JSON.stringify(protectedBranches)}`); + assert.deepStrictEqual(configuredName.protectedBranches, ['main', 'develop']); + assert.strictEqual(configuredName.rejectedProtectedBranches.length, 1, + 'the bad element must be reported, not silently swallowed'); + } + }); + + test('#3552 a non-array value contributes no names and is reported', () => { + const loadConfig = () => ({ base_branch: 'main', protected_branches: 'develop' }); + const status = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'develop', { loadConfig }); + + assert.strictEqual(status.isProtected, false, + 'a bare string is not a list of branch names — it must not protect'); + assert.deepStrictEqual(status.protectedBranches, ['main']); + assert.deepStrictEqual(status.rejectedProtectedBranches, ['"develop"']); + }); + + test('#3552 negative control: a well-formed list reports nothing rejected', () => { + // Must disagree with every case above — otherwise the reject channel is + // reporting unconditionally and proves nothing. + const loadConfig = () => ({ base_branch: 'main', protected_branches: ['develop'] }); + const status = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'develop', { loadConfig }); + + assert.strictEqual(status.isProtected, true); + assert.deepStrictEqual(status.rejectedProtectedBranches, []); + }); + + test('#3552 an empty list is well-formed, not malformed', () => { + const loadConfig = () => ({ base_branch: 'main', protected_branches: [] }); + const status = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'main', { loadConfig }); + + assert.deepStrictEqual(status.protectedBranches, ['main']); + assert.deepStrictEqual(status.rejectedProtectedBranches, [], + 'declaring no extra protected branches is a valid choice, not an error'); + }); + + test('#3552 --is-protected writes a diagnostic naming the rejected elements', () => { + const diagnostics = []; + const out = []; + gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', 'develop'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop', 42] }), + write: (chunk) => out.push(chunk), + writeDiagnostic: (chunk) => diagnostics.push(chunk), + }); + + assert.strictEqual(out.join('').trim(), 'true'); + assert.strictEqual(diagnostics.length, 1, 'the rejection must be surfaced, not swallowed'); + assert.match(diagnostics.join(''), /protected_branches/); + assert.match(diagnostics.join(''), /42/); + }); + + test('#3552 negative control: a clean list writes no diagnostic', () => { + const diagnostics = []; + gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', 'develop'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop'] }), + write: () => {}, + writeDiagnostic: (chunk) => diagnostics.push(chunk), + }); + + assert.deepStrictEqual(diagnostics, [], + 'a well-formed list must be silent — otherwise the diagnostic carries no signal'); + }); + + test('#3648 Nit F-7: renderRejected sanitizes control and ANSI characters in diagnostics', () => { + const diagnostics = []; + gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', 'develop'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop', { malicious: 'bad\x1b[31m\nbranch' }] }), + write: () => {}, + writeDiagnostic: (chunk) => diagnostics.push(chunk), + }); + + assert.strictEqual(diagnostics.length, 1); + const diag = diagnostics.join(''); + assert.match(diag, /bad\\u001b\[31m/); + assert.strictEqual(diag.includes('\x1b'), false, 'diagnostic must not contain raw ESC control bytes'); + }); + + test('#3552 execute-phase handle_branching wires protected-branch step with Read and execute (not Skip.)', () => { + const content = readFileNormalized(path.join(WORKFLOW_DIR, 'execute-phase.md')); + const stepMatch = content.match(/([\s\S]*?)<\/step>/); + assert.ok(stepMatch, 'handle_branching step must exist in execute-phase.md'); + const stepBody = stepMatch[1]; + + assert.match( + stepBody, + /\*\*"none":\*\*\s+Read and execute `execute-phase\/steps\/protected-branch\.md`\./, + 'execute-phase handle_branching "none" arm must instruct executor to Read and execute the step file', + ); + assert.doesNotMatch( + stepBody, + /\*\*"none":\*\*\s*Skip\./, + 'execute-phase handle_branching "none" arm must not say "Skip."', + ); + + // Negative control: verify the assertion rejects an advisory "Skip." pointer + const fakeAdvisoryBody = stepBody.replace( + /\*\*"none":\*\*\s+Read and execute `execute-phase\/steps\/protected-branch\.md`\./, + '**"none":** Skip. See `execute-phase/steps/protected-branch.md`.', + ); + assert.doesNotMatch( + fakeAdvisoryBody, + /\*\*"none":\*\*\s+Read and execute `execute-phase\/steps\/protected-branch\.md`\./, + 'negative control: fake advisory pointer must fail the wiring assertion', + ); + }); + + test('#3552 active workstream CLI uses configured list and excludes root-only names', (t) => { + const dir = createGitRepo({ prefix: 'gsd-3552-cli-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + setGsdConfig(dir, 'git.protected_branches', ['root-only']); + fs.mkdirSync(path.join(dir, '.planning', 'workstreams', 'alpha'), { recursive: true }); + // Both HOME and USERPROFILE must be redirected — Node's os.homedir() (and + // anything relying on it) consults USERPROFILE first on Windows, where + // setting HOME alone leaves the real home directory in effect and makes + // this isolation silently vacuous on that platform. + const workstreamEnv = { GSD_WORKSTREAM: 'alpha', HOME: dir, USERPROFILE: dir }; + + const setResult = runGsdTools( + ['config-set', 'git.protected_branches', '["develop","next"]'], + dir, + workstreamEnv, + ); + assert.ok(setResult.success, setResult.error); + const workstreamConfig = JSON.parse(fs.readFileSync( + path.join(dir, '.planning', 'workstreams', 'alpha', 'config.json'), + 'utf8', + )); + assert.deepStrictEqual(workstreamConfig.git.protected_branches, ['develop', 'next']); + + const match = runGsdTools( + ['query', 'git.base-branch', '--is-protected', 'develop'], dir, workstreamEnv, + ); + const control = runGsdTools( + ['query', 'git.base-branch', '--is-protected', 'topic/3552'], dir, workstreamEnv, + ); + const rootOnly = runGsdTools( + ['query', 'git.base-branch', '--is-protected', 'root-only'], dir, workstreamEnv, + ); + + assert.ok(match.success, match.error); + assert.ok(control.success, control.error); + assert.ok(rootOnly.success, rootOnly.error); + assert.strictEqual(match.output, 'true'); + assert.strictEqual(control.output, 'false'); + assert.strictEqual(rootOnly.output, 'false'); + assert.notStrictEqual(match.output, control.output, + 'CLI negative control must disagree with the configured protected-branch match'); + assert.notStrictEqual(match.output, rootOnly.output, + 'workstream override must replace the root-only configured name'); + + let written = ''; + const returned = gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', 'develop'], { + loadConfig: configuredLoad, + write: (text) => { written += text; }, + }); + assert.strictEqual(returned, 'true'); + assert.strictEqual(written, 'true\n', 'direct command contract must remain newline-terminated'); + }); + + test('#3552 execute-phase warns on true, stays silent on false, and continues both', (t) => { + const bash = extractProtectedBranchWarningBash('execute-phase.md', 'handle_branching'); + const { scriptDir, scriptPath } = writeProtectedBranchWarningScript( + 'gsd-3552-execute-', + bash, + ); + t.after(() => cleanup(scriptDir)); + + const baseEnv = { ...process.env, CURRENT_BRANCH_VALUE: 'develop' }; + const match = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'true' }, + }); + const control = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'false' }, + }); + + assert.strictEqual(match.exitCode, 0, match.stderr); + assert.strictEqual(control.exitCode, 0, control.stderr); + assert.match(match.stdout, /continued\n$/); + assert.match(control.stdout, /continued\n$/); + assert.match(match.stderr, /protected branch/i); + assert.doesNotMatch(control.stderr, /protected branch/i); + assert.notStrictEqual(match.stderr, control.stderr, + 'warning negative control must disagree with the protected-branch match'); + }); + + test('#3552 ship warns on true, stays silent on false, and keeps the none-strategy offer', (t) => { + const bash = extractProtectedBranchWarningBash('ship.md', 'preflight_checks'); + const { scriptDir, scriptPath } = writeProtectedBranchWarningScript( + 'gsd-3552-ship-', + bash, + ); + t.after(() => cleanup(scriptDir)); + + const baseEnv = { ...process.env, CURRENT_BRANCH_VALUE: 'develop' }; + const match = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'true' }, + }); + const control = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'false' }, + }); + + assert.strictEqual(match.exitCode, 0, match.stderr); + assert.strictEqual(control.exitCode, 0, control.stderr); + assert.match(match.stdout, /continued\n$/); + assert.match(control.stdout, /continued\n$/); + assert.match(match.stderr, /protected branch/i); + assert.doesNotMatch(control.stderr, /protected branch/i); + assert.notStrictEqual(match.stderr, control.stderr, + 'ship warning negative control must disagree with the protected-branch match'); + + const ship = readFileNormalized(path.join(WORKFLOW_DIR, 'ship.md')); + const preflight = ship.slice( + ship.indexOf(''), + ship.indexOf('', ship.indexOf('')), + ); + assert.match(preflight, /branching_strategy is `none`: offer to create a branch now/i, + 'protected-branch warning must retain the none-strategy feature-branch offer'); + }); + + test('#3648 Nit: detached HEAD answers false; a missing argument is reported', () => { + // `git branch --show-current` prints nothing on a detached HEAD, so the + // call sites pass an explicit empty string. That must answer false — no + // protected branch is named '' — and must stay SILENT, because a detached + // HEAD is a normal state, not a misconfiguration. + const detachedDiagnostics = []; + const detachedOut = []; + gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', ''], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop'] }), + write: (chunk) => detachedOut.push(chunk), + writeDiagnostic: (chunk) => detachedDiagnostics.push(chunk), + }); + assert.strictEqual(detachedOut.join('').trim(), 'false'); + assert.deepStrictEqual(detachedDiagnostics, [], + 'a detached HEAD is not an error and must not warn'); + + // The flag with NO argument at all is a caller bug that `args[1] ?? ''` + // silently collapsed into the detached-HEAD case. Same answer, but said + // out loud so the two are distinguishable. + const missingDiagnostics = []; + const missingOut = []; + gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop'] }), + write: (chunk) => missingOut.push(chunk), + writeDiagnostic: (chunk) => missingDiagnostics.push(chunk), + }); + assert.strictEqual(missingOut.join('').trim(), 'false'); + assert.strictEqual(missingDiagnostics.length, 1, + 'a missing branch argument must be reported, not read as a detached HEAD'); + assert.match(missingDiagnostics.join(''), /--is-protected/); + + assert.notDeepStrictEqual(detachedDiagnostics, missingDiagnostics, + 'the two paths must be distinguishable — that is the whole point of the arm'); + + assert.throws( + () => gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protectd', 'develop'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop'] }), + }), + (err) => err instanceof ExitError && err.code === 1, + 'a misspelled predicate flag must be rejected with exit 1 via error()', + ); + + assert.throws( + () => gitBaseBranch.cmdGitBaseBranch('/repo', ['--is-protected', 'develop', 'extra'], { + loadConfig: () => ({ base_branch: 'main', protected_branches: ['develop'] }), + }), + (err) => err instanceof ExitError && err.code === 1, + 'surplus predicate arguments must be rejected with exit 1 via error()', + ); + }); + + test('#3648 cmdGitBaseBranch usage errors emit structured ERROR_REASON.USAGE and do not print stack trace', (t) => { + const dir = createGitRepo({ prefix: 'gsd-3648-usage-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + + const unknownFlag = runGsdTools(['query', 'git.base-branch', '--unknown-flag', '--json-errors'], dir); + assert.strictEqual(unknownFlag.success, false); + assert.strictEqual(unknownFlag.exitCode, 1); + assert.doesNotMatch(unknownFlag.error, /^\s*at\s+/m, 'usage errors must not print stack traces'); + let parsed = null; + try { parsed = JSON.parse(unknownFlag.error); } catch {} + assert.strictEqual(parsed?.reason, 'usage'); + + const surplusPositional = runGsdTools(['query', 'git.base-branch', '--is-protected', 'main', 'extra', '--json-errors'], dir); + assert.strictEqual(surplusPositional.success, false); + assert.strictEqual(surplusPositional.exitCode, 1); + assert.doesNotMatch(surplusPositional.error, /^\s*at\s+/m); + let parsedSurplus = null; + try { parsedSurplus = JSON.parse(surplusPositional.error); } catch {} + assert.strictEqual(parsedSurplus?.reason, 'usage'); + }); + + test('#3648 Minor F-1: nested and flat predicate reads stay aligned with the loader', () => { + const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); + const cases = [ + { git: { base_branch: 'nested', protected_branches: ['develop'] }, base_branch: 'flat', protected_branches: ['bogus'] }, + { git: { base_branch: 'nested' }, base_branch: 'flat' }, + { git: 'not-an-object', base_branch: 'flat', protected_branches: ['bogus'] }, + ]; + for (const fixture of cases) { + const seam = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'develop', { + readFile: () => JSON.stringify(fixture), + }); + assert.strictEqual( + gitBaseBranch._readGitKey(fixture, 'base_branch'), + configLoader._getConfigValue(fixture, 'base_branch', { section: 'git', field: 'base_branch' }), + ); + assert.strictEqual( + gitBaseBranch._readGitNested(fixture, 'protected_branches'), + configLoader._getConfigNested(fixture, 'git', 'protected_branches'), + ); + assert.ok(seam); + } + }); + + test('#3648 Blocker 4: the predicate diagnostic reaches the user at both call sites', (t) => { + // Both call sites piped the query's stderr to /dev/null, so the + // fail-closed explanation ("could not verify the base branch ... defaulting + // to protected") was discarded and the user saw a bare protected-branch + // warning on a branch that is not protected. The diagnostic was exercised + // only by its unit test — dead in production. + const cases = [ + ['execute-phase.md', 'handle_branching', 'gsd-3648-b4-execute-'], + ['ship.md', 'preflight_checks', 'gsd-3648-b4-ship-'], + ]; + + for (const [workflow, step, prefix] of cases) { + const bash = extractProtectedBranchWarningBash(workflow, step); + const { scriptDir, scriptPath } = writeProtectedBranchWarningScript(prefix, bash); + t.after(() => cleanup(scriptDir)); + + const baseEnv = { + ...process.env, + CURRENT_BRANCH_VALUE: 'develop', + PROTECTED_RESULT: 'true', + }; + const surfaced = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, DIAGNOSTIC_TEXT: 'could not verify the base branch' }, + }); + const control = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, DIAGNOSTIC_TEXT: '' }, + }); + + assert.strictEqual(surfaced.exitCode, 0, surfaced.stderr); + assert.match(surfaced.stderr, /could not verify the base branch/, + workflow + " must not discard the predicate's explanation"); + // Negative control: with nothing emitted that text must be absent, + // otherwise the assertion above could pass on unrelated output. + assert.doesNotMatch(control.stderr, /could not verify the base branch/); + assert.notStrictEqual(surfaced.stderr, control.stderr); + // The warning itself still fires in both, so the diagnostic is additive. + assert.match(surfaced.stderr, /protected branch/i); + assert.match(control.stderr, /protected branch/i); + } + }); + + test('#3648 round-4: a FAILED query degrades visibly, and does not abort under set -e', (t) => { + // Found by the round-4 external review. `IS_PROTECTED=$(gsd_run ...)` had + // two problems when the query itself failed (gsd-tools absent, a crash, any + // non-zero exit): the substitution yielded an empty string, so `[ "$X" = true ]` + // was simply false and the workflow continued with NO warning and no trace — + // a silent fail-open in the guard whose whole purpose is to warn; and the + // bare assignment is the last command in its own right, so a non-zero exit + // aborted the step under `set -e` before any branch ran. + // + // The contract is neither fail-open nor fail-closed: it degrades VISIBLY. + // Claiming "protected" on no evidence would warn on every branch whenever + // gsd-tools is unavailable; claiming "not protected" is the silent hole. + const cases = [ + ['execute-phase.md', 'handle_branching', 'gsd-3648-r4-execute-'], + ['ship.md', 'preflight_checks', 'gsd-3648-r4-ship-'], + ]; + + for (const [workflow, step, prefix] of cases) { + const bash = extractProtectedBranchWarningBash(workflow, step); + const { scriptDir, scriptPath } = writeProtectedBranchWarningScript(prefix, bash); + t.after(() => cleanup(scriptDir)); + + const baseEnv = { + ...process.env, + CURRENT_BRANCH_VALUE: 'topic/3648', + PROTECTED_RESULT: 'false', + }; + const failed = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, QUERY_EXIT: '3' }, + }); + // Control: the SAME script with a working query must stay silent on a + // non-protected branch. Without it, an assertion that the failure warns + // could pass against a script that warns unconditionally. + const working = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, QUERY_EXIT: '0' }, + }); + + // The harness runs under `set -eu`, so this also pins the set -e half. + assert.strictEqual(failed.exitCode, 0, + workflow + ' must survive a failed query under set -e: ' + failed.stderr); + assert.match(failed.stdout, /continued/, + workflow + ' must reach the end of the step'); + assert.match(failed.stderr, /Could not determine whether/, + workflow + ' must say the check did not run, rather than pass silently'); + assert.doesNotMatch(failed.stderr, /is a protected branch/, + 'and must NOT assert protectedness it never established'); + + assert.strictEqual(working.exitCode, 0, working.stderr); + assert.doesNotMatch(working.stderr, /Could not determine whether/, + 'a working query must not emit the degradation notice'); + assert.doesNotMatch(working.stderr, /is a protected branch/, + 'and must not warn about a branch that is not protected'); + assert.notStrictEqual(failed.stderr, working.stderr, + 'the two paths must be distinguishable'); + } + }); + + test('#3648 Minor 2: ship binds the predicate result instead of discarding it', (t) => { + // ship.md's prose branches on protectedness twice ("warn - should be on a + // feature branch", "if branching_strategy is none, offer to create a + // branch"). Echoing the warning without binding leaves the agent inferring + // state from warning text in tool output; the comparison it replaced was + // directly evaluable. + const bash = extractProtectedBranchWarningBash('ship.md', 'preflight_checks'); + const { scriptDir, scriptPath } = writeProtectedBranchWarningScript('gsd-3648-bind-', bash); + t.after(() => cleanup(scriptDir)); + + const baseEnv = { ...process.env, CURRENT_BRANCH_VALUE: 'develop' }; + const match = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'true' }, + }); + const control = runHook(scriptPath, [], { + interpreter: 'bash', + env: { ...baseEnv, PROTECTED_RESULT: 'false' }, + }); + + assert.match(match.stdout, /^IS_PROTECTED=true$/m, + 'ship must bind the predicate result to a variable the following prose can branch on'); + assert.match(control.stdout, /^IS_PROTECTED=false$/m, + 'the binding must track the predicate, not be hardcoded'); + assert.notStrictEqual(match.stdout, control.stdout); + }); +}); + +// ─── #3648 Major 1: negative space for readEffectiveGitConfig's readFile seam ─ +// +// The #3057 W3 suite that pinned readConfigBaseBranch's unusable-config arms +// was deleted with the function it targeted, but every arm it covered survives +// verbatim in readEffectiveGitConfig's readFile branch: the JSON.parse catch, +// the non-object guard, the git-section object guard, .trim(), and blank-string +// rejection. Deleting the tests left all of them unexercised — the four +// surviving readFile injections are positive-path only, and protected_branches +// was never driven through this seam at all. +// +// Reaching the seam requires readFile WITHOUT loadConfig. execGit is stubbed to +// answer cleanly-but-emptily so tiers 2-4 fall to a VERIFIED 'main', which +// makes 'main' the unambiguous signal that the config tier contributed nothing. +// +// WHAT THIS SUITE DOES AND DOES NOT PIN. Verified by mutating the built lib and +// re-running: +// +// .trim() on the resolved value -> KILLED by the positive controls +// the non-object guard on JSON.parse output -> SURVIVES +// the blank-string rejection -> SURVIVES +// +// The two survivors are unreachable through this entry point, for the same +// reason the deleted #3057 W3 suite recorded against its own equivalents: +// +// * A JSON-parsed value can never carry a `.git` or `.base_branch` own +// property unless it is already an object, so `null` / `42` / `"x"` / `[]` +// read as "no keys" whether or not the guard runs. +// * A blank base_branch is rejected a second time downstream — the resolver's +// `if (configured)` treats '' as falsy — so removing the guard here changes +// no observable output. +// +// Both remain defence-in-depth for a future non-JSON caller. They are recorded +// here as known-unkillable rather than left to look like coverage this suite +// does not provide. + +describe('#3648 property: config-set validation and resolver filtering must agree', () => { + // The two new validating surfaces this PR adds are deliberately different + // shapes: `config-set` is all-or-nothing (reject the whole write), while the + // resolver is per-entry (drop the bad names, keep the good ones, report), so + // that a direct edit of config.json cannot fail the predicate OPEN. Nothing + // structural keeps the two definitions of "usable branch name" in step — + // only this property, which asks both about the same values. + const fc = require('./helpers/fast-check-setup.cjs'); + const { isValidProtectedBranches } = require('../gsd-core/bin/lib/config.cjs'); + + /** A value generator weighted towards the boundary cases both surfaces care about. */ + const entry = () => fc.oneof( + fc.string(), + fc.constantFrom('', ' ', '\t\n', ' develop ', 'develop'), + fc.integer(), + fc.boolean(), + fc.constant(null), + fc.constant(undefined), + ); + + /** + * `fc.array` never produces a HOLE, and a hole is exactly where the two + * surfaces disagreed: `.every()` skips holes, `for...of` yields `undefined` + * for them. The property passed only because the generator could not reach + * the case (round-4 external review). Punching holes into a generated array + * is what makes this axis falsifiable. + */ + const withHoles = () => fc.tuple( + fc.array(entry(), { maxLength: 5 }), + fc.array(fc.nat({ max: 5 }), { maxLength: 3 }), + ).map(([values, holeIndices]) => { + const arr = values.slice(); + for (const i of holeIndices) { + if (i < arr.length) delete arr[i]; + } + return arr; + }); + + test('accepted by config-set ⟺ the resolver rejects nothing from a non-empty list', () => { + fc.assert( + fc.property( + fc.oneof(fc.array(entry(), { maxLength: 6 }), withHoles(), entry()), + (configured) => { + const status = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'topic/x', { + loadConfig: () => ({ base_branch: 'main', protected_branches: configured }), + }); + + const resolverKeptEverything = Array.isArray(configured) + && configured.length > 0 + && status.rejectedProtectedBranches.length === 0; + + assert.strictEqual(isValidProtectedBranches(configured), resolverKeptEverything, + `disagreement on ${JSON.stringify(configured)}`); + }, + ), + { numRuns: 400 }, + ); + }); + + test('a top-level `protected_branches` is NOT an alias for the canonical nested key', (t) => { + // Round-4 external review. `get(key, {section, field})` is flat-then-nested, + // which is back-compat for keys `normalizeLegacyKeys` migrates. Routing a + // BRAND-NEW key through it invents an undocumented top-level spelling that + // silently outranks `git.protected_branches`. `protected_branches` has no + // legacy form, so it resolves nested-only — in production and in the seam. + const dir = createGitRepo({ prefix: 'gsd-3648-flat-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ protected_branches: ['bogus'], git: { protected_branches: ['develop'] } }), + ); + + const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); + assert.deepStrictEqual( + configLoader.loadConfig(dir, { persist: false })['protected_branches'], ['develop'], + 'the canonical nested key must win outright'); + + // Control: `base_branch` DOES keep flat-then-nested, because it has a legacy + // flat spelling #3760's refusal path can leave behind. Without this, the + // assertion above could pass against a loader that lost flat support wholesale. + const seam = gitBaseBranch.resolveProtectedBranchStatus(dir, 'develop', { + readFile: () => JSON.stringify({ + git: 'not-an-object', base_branch: 'release', protected_branches: ['bogus'], + }), + }); + assert.strictEqual(seam.baseBranch, 'release', + 'a refused migration must still let the surviving flat base_branch through'); + assert.deepStrictEqual(seam.protectedBranches, ['release'], + 'while a top-level protected_branches contributes nothing'); + }); + + test('every surviving name is trimmed, non-empty, and de-duplicated against the base', () => { + fc.assert( + fc.property( + fc.array(entry(), { maxLength: 6 }), + (configured) => { + const status = gitBaseBranch.resolveProtectedBranchStatus('/repo', 'topic/x', { + loadConfig: () => ({ base_branch: 'main', protected_branches: configured }), + }); + + for (const name of status.protectedBranches) { + assert.strictEqual(typeof name, 'string'); + assert.strictEqual(name, name.trim(), 'names must be stored trimmed'); + assert.notStrictEqual(name, '', 'a blank name would silently match a detached HEAD'); + } + assert.deepStrictEqual( + status.protectedBranches, [...new Set(status.protectedBranches)], + 'duplicates (including one equal to the base branch) must collapse'); + assert.strictEqual(status.protectedBranches[0], 'main', + 'the resolved base branch is always protected and always leads'); + + // Nothing is lost silently: each input element is either kept + // (trimmed) or reported as rejected. + const kept = configured.filter((b) => typeof b === 'string' && b.trim() !== ''); + assert.strictEqual( + kept.length + status.rejectedProtectedBranches.length, configured.length); + }, + ), + { numRuns: 400 }, + ); + }); +}); + +describe('#3648 Blocker 1: --is-protected is a QUERY and must not rewrite config.json', () => { + // The predicate runs on every execute-phase and every ship. It resolves config + // through config-loader's `loadConfig`, which normalizes legacy keys and then + // WRITES the migrated shape back to .planning/config.json. A boolean question + // was therefore silently rewriting the user's checked-in config — and this PR + // widened the trigger by adding a fifth normalization block (top-level + // `base_branch` -> `git.base_branch`). The read now passes `persist: false`. + // + // Asserted on BYTES, not on parsed shape: the persisted rewrite reorders keys + // and reflows whitespace even when the resolved values are equivalent. + + /** A config whose ONLY interesting property is that it triggers a normalization. */ + function writeLegacyConfig(dir) { + fs.mkdirSync(path.join(dir, '.planning'), { recursive: true }); + const cfgPath = path.join(dir, '.planning', 'config.json'); + // Hand-written formatting deliberately unlike JSON.stringify(cfg, null, 2): + // if anything rewrites this file, the bytes cannot come back identical. + fs.writeFileSync(cfgPath, '{"base_branch": "develop", "granularity": "standard"}\n'); + return cfgPath; + } + + test('a legacy-key config survives --is-protected byte-for-byte, and is still honoured', (t) => { + const dir = createGitRepo({ prefix: 'gsd-3648-b1-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + const cfgPath = writeLegacyConfig(dir); + fs.appendFileSync(path.join(dir, '.git', 'info', 'exclude'), '\n.planning/\n'); + const before = fs.readFileSync(cfgPath); + const planningBefore = snapshotPlanningTree(dir); + + const match = runGsdTools(['query', 'git.base-branch', '--is-protected', 'develop'], dir); + const control = runGsdTools(['query', 'git.base-branch', '--is-protected', 'topic/3648'], dir); + + assert.ok(match.success, match.error); + assert.ok(control.success, control.error); + // Positive control: the flat legacy `base_branch` DID reach the predicate. + // Without this the byte assertion below would also pass for a query that + // ignored the config entirely. + assert.strictEqual(match.output, 'true'); + assert.strictEqual(control.output, 'false'); + + assert.deepStrictEqual( + fs.readFileSync(cfgPath), before, + 'a read-only predicate must leave .planning/config.json byte-identical', + ); + assert.deepStrictEqual( + snapshotPlanningTree(dir), planningBefore, + 'a read-only predicate must leave the entire .planning tree byte-identical', + ); + const status = gitOrThrow(['status', '--porcelain'], { cwd: dir }); + assert.strictEqual(status, '', 'read-only predicate must not leave a sibling write'); + }); + + test('negative control: an ordinary persisting load DOES rewrite the same fixture', (t) => { + // Without this the test above cannot fail for the right reason — a fixture + // that never triggered a normalization would keep its bytes no matter what + // `persist` did. This proves the fixture is live. + const dir = createGitRepo({ prefix: 'gsd-3648-b1-neg-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + const cfgPath = writeLegacyConfig(dir); + const before = fs.readFileSync(cfgPath); + + const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); + const persisted = configLoader.loadConfig(dir); + + assert.notDeepStrictEqual( + fs.readFileSync(cfgPath), before, + 'the default (omitted) option must keep migrating legacy configs on disk', + ); + assert.strictEqual(persisted['base_branch'], 'develop', + 'and it must resolve the same value the non-persisting read resolves'); + }); + + test('negative control: a planted .planning sibling changes the tree snapshot', (t) => { + const dir = createGitRepo({ prefix: 'gsd-3648-b1-stray-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + const before = snapshotPlanningTree(dir); + + fs.writeFileSync(path.join(dir, '.planning', 'sibling'), 'stray write\n'); + + assert.notDeepStrictEqual( + snapshotPlanningTree(dir), before, + 'the snapshot must detect a newly planted .planning sibling', + ); + }); + + test('persist:false changes only the side effect, not the resolved config', (t) => { + const dir = createGitRepo({ prefix: 'gsd-3648-b1-parity-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + const cfgPath = writeLegacyConfig(dir); + const before = fs.readFileSync(cfgPath); + + const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); + const quiet = configLoader.loadConfig(dir, { persist: false }); + assert.deepStrictEqual(fs.readFileSync(cfgPath), before, + 'persist:false must not write'); + + const loud = configLoader.loadConfig(dir); + assert.notDeepStrictEqual(fs.readFileSync(cfgPath), before, + 'the same directory, without the option, must write — proving the two differ'); + assert.deepStrictEqual(quiet, loud, + 'resolution must be identical; only the write is suppressed'); + }); + + test('persist survives the workstream fallback recursion', (t) => { + // `loadConfigResolved` re-enters ITSELF with `{ workstream: null }` when a + // workstream was requested but has no config.json of its own. That literal + // dropped every other option, so the recursive pass ran with the DEFAULT + // persistence and rewrote the root config — the predicate's `persist:false` + // was silently discarded for exactly the projects that use workstreams. + // Found by the round-4 external review. + const dir = createGitRepo({ prefix: 'gsd-3648-b1-ws-', defaultBranch: 'main' }); + t.after(() => cleanup(dir)); + addPlanning(dir); + // The workstream directory exists but carries no config.json — the shape + // that forces the fallback. Without it the recursion never runs and this + // test degenerates into the non-workstream case above. + fs.mkdirSync(path.join(dir, '.planning', 'workstreams', 'alpha'), { recursive: true }); + const cfgPath = writeLegacyConfig(dir); + const before = fs.readFileSync(cfgPath); + + const configLoader = require('../gsd-core/bin/lib/config-loader.cjs'); + const quiet = configLoader.loadConfig(dir, { persist: false, workstream: 'alpha' }); + + assert.strictEqual(quiet['base_branch'], 'develop', + 'positive control: the fallback really did resolve the root config'); + assert.deepStrictEqual(fs.readFileSync(cfgPath), before, + 'the recursive fallback pass must inherit persist:false'); + + const loud = configLoader.loadConfig(dir, { workstream: 'alpha' }); + assert.notDeepStrictEqual(fs.readFileSync(cfgPath), before, + 'negative control: the same fallback without the option must still write'); + assert.deepStrictEqual(quiet, loud, + 'and suppressing the write must not change what the fallback resolves'); + }); +}); + +describe('#3648 Major 1: readEffectiveGitConfig readFile seam — config present but unusable', () => { + const CWD = path.join(path.sep, 'gsd-3648-seam'); + + /** Drive the seam with raw config text, recording the paths requested. */ + function readWith(raw, seenPaths) { + return gitBaseBranch.resolveProtectedBranchStatus(CWD, 'some-topic-branch', { + execGit: makeFaultyGit(), + readFile: (requested) => { if (seenPaths) seenPaths.push(requested); return raw; }, + }); + } + + test('config.json exists but is not valid JSON → parse failure swallowed, base falls through', () => { + const seen = []; + assert.strictEqual(readWith('{ not json', seen).baseBranch, 'main'); + assert.deepStrictEqual(seen, [path.join(CWD, '.planning', 'config.json')], + 'the seam must look for config.json inside the planning dir under the cwd it was given'); + }); + + test('config.json parses to a non-object → ignored for null / string / number / array', () => { + assert.strictEqual(readWith('null').baseBranch, 'main', 'JSON null must not be treated as a config'); + assert.strictEqual(readWith('"master"').baseBranch, 'main', 'a bare JSON string must not be treated as a config'); + assert.strictEqual(readWith('42').baseBranch, 'main', 'a bare JSON number must not be treated as a config'); + assert.strictEqual(readWith('[]').baseBranch, 'main', 'a JSON array must not be treated as a config'); + }); + + test('"git" section present but base_branch missing / non-string / blank → no override', () => { + assert.strictEqual(readWith('{"git":{}}').baseBranch, 'main'); + assert.strictEqual(readWith('{"git":{"base_branch":42}}').baseBranch, 'main'); + assert.strictEqual(readWith('{"git":{"base_branch":null}}').baseBranch, 'main'); + assert.strictEqual(readWith('{"git":{"base_branch":""}}').baseBranch, 'main'); + assert.strictEqual(readWith('{"git":{"base_branch":" "}}').baseBranch, 'main', + 'a whitespace-only override must not win the precedence ladder'); + }); + + test('"git" key present but not a usable object → the flat legacy key is still honoured', () => { + // These also guard the hoist: a non-object "git" must not be spread into + // index keys on the way to carrying the flat value across. + assert.strictEqual(readWith('{"git":"main","base_branch":"release"}').baseBranch, 'release'); + assert.strictEqual(readWith('{"git":[],"base_branch":"release"}').baseBranch, 'release'); + assert.strictEqual(readWith('{"git":null,"base_branch":"release"}').baseBranch, 'release'); + }); + + test('flat base_branch present but non-string / blank → no override', () => { + assert.strictEqual(readWith('{"base_branch":true}').baseBranch, 'main'); + assert.strictEqual(readWith('{"base_branch":["main"]}').baseBranch, 'main'); + assert.strictEqual(readWith('{"base_branch":""}').baseBranch, 'main'); + assert.strictEqual(readWith('{"base_branch":" "}').baseBranch, 'main'); + }); + + test('config parses cleanly but carries neither key → distinct from an absent file', () => { + // The absent-file path returns before JSON.parse; these parse a real object + // and fall all the way through both lookups. + assert.strictEqual(readWith('{"other":1}').baseBranch, 'main'); + assert.strictEqual(readWith('{}').baseBranch, 'main'); + assert.strictEqual(readWith('').baseBranch, 'main', 'absent file (empty read) also yields no override'); + assert.strictEqual(readWith(null).baseBranch, 'main', 'a null read is an absent file'); + }); + + test('positive controls: values are trimmed and the nested key outranks the flat one', () => { + // Without these the whole describe could pass against a seam that ignored + // config entirely — every assertion above expects the fallback value. + assert.strictEqual(readWith('{"git":{"base_branch":" develop "}}').baseBranch, 'develop'); + assert.strictEqual(readWith('{"base_branch":" release\\n"}').baseBranch, 'release'); + assert.strictEqual(readWith('{"git":{"base_branch":"nested"},"base_branch":"flat"}').baseBranch, 'nested'); + }); + + test('protected_branches is honoured and validated through this seam too', () => { + const clean = readWith('{"git":{"base_branch":"main","protected_branches":["develop"," next "]}}'); + assert.deepStrictEqual(clean.protectedBranches, ['main', 'develop', 'next']); + assert.deepStrictEqual(clean.rejectedProtectedBranches, []); + + const partial = readWith('{"git":{"base_branch":"main","protected_branches":["develop",42]}}'); + assert.deepStrictEqual(partial.protectedBranches, ['main', 'develop'], + 'a bad element must drop only itself on this path as well'); + assert.deepStrictEqual(partial.rejectedProtectedBranches, ['42']); + + const notAList = readWith('{"git":{"base_branch":"main","protected_branches":"develop"}}'); + assert.deepStrictEqual(notAList.protectedBranches, ['main']); + assert.deepStrictEqual(notAList.rejectedProtectedBranches, ['"develop"']); + }); + + test('loadConfig wins when both seams are supplied — readFile is the test-only path', () => { + const status = gitBaseBranch.resolveProtectedBranchStatus(CWD, 'from-loader', { + execGit: makeFaultyGit(), + readFile: () => '{"git":{"base_branch":"from-readfile"}}', + loadConfig: () => ({ base_branch: 'from-loader' }), + }); + + assert.strictEqual(status.baseBranch, 'from-loader', + 'production resolution must not be displaced by the low-level seam'); + assert.strictEqual(status.isProtected, true); + }); }); // ─── #3057 W3: negative-space coverage for the resolver's failure arms ─────── @@ -441,86 +1519,6 @@ function throwingGit(message) { return () => { throw new Error(message); }; } -describe('#3057 W3: readConfigBaseBranch — config present but unusable', () => { - const PLANNING_DIR = path.join(path.sep, 'gsd-3057-w3', '.planning'); - - /** Read a config whose raw text is `raw`, recording the paths requested. */ - function readWith(raw, seenPaths) { - return gitBaseBranch.readConfigBaseBranch(PLANNING_DIR, { - readFile: (p) => { if (seenPaths) seenPaths.push(p); return raw; }, - }); - } - - test('config.json exists but is not valid JSON → null (parse failure swallowed)', () => { - const seen = []; - assert.strictEqual(readWith('{ not json', seen), null); - assert.deepStrictEqual(seen, [path.join(PLANNING_DIR, 'config.json')], - 'the resolver must look for config.json inside the planning dir it was given'); - }); - - test('config.json parses to a non-object → null for null / string / number / array', () => { - // NOTE on the `[]` case: this documents observed behaviour only. It does - // NOT pin the `Array.isArray(cfg)` guard in readConfigBaseBranch — that - // guard is unreachable (and therefore unkillable) through this readFile - // entry point. `cfg` is always the result of `JSON.parse(raw)` on a - // string, and a JSON array can never carry a `.git` or `.base_branch` - // own-property the way a hand-built JS array could; with the guard - // deleted entirely, `top.git`/`top.base_branch` on an array are still - // `undefined`, so the result is `null` either way. Verified by mutation: - // deleting `|| Array.isArray(cfg)` from the built lib does not change any - // output for any JSON-string input. The guard is real defense-in-depth - // for a future non-JSON-string caller, not something this suite can pin. - assert.strictEqual(readWith('null'), null, 'JSON null must not be treated as a config'); - assert.strictEqual(readWith('"master"'), null, 'a bare JSON string must not be treated as a config'); - assert.strictEqual(readWith('42'), null, 'a bare JSON number must not be treated as a config'); - assert.strictEqual(readWith('[]'), null, 'a JSON array must not be treated as a config'); - }); - - test('"git" section present but base_branch missing / non-string / blank → null', () => { - assert.strictEqual(readWith('{"git":{}}'), null); - assert.strictEqual(readWith('{"git":{"base_branch":42}}'), null); - assert.strictEqual(readWith('{"git":{"base_branch":null}}'), null); - assert.strictEqual(readWith('{"git":{"base_branch":""}}'), null); - assert.strictEqual(readWith('{"git":{"base_branch":" "}}'), null, - 'a whitespace-only override must not win the precedence ladder'); - }); - - test('"git" key present but not a usable object (string/array/null) → nested lookup finds nothing, flat legacy key still consulted', () => { - // NOTE on the `"git":[]` case: like the sibling note above, this does NOT - // pin `!Array.isArray(gitSection)`. `gitSection` here is a JSON-parsed - // array with no `.base_branch` own-property, so `gitSection.base_branch` - // is `undefined` whether or not the guard runs — the flat key is - // consulted either way. Verified by mutation: deleting - // `&& !Array.isArray(gitSection)` from the built lib does not change this - // output for any JSON-string input. - assert.strictEqual(readWith('{"git":"main","base_branch":"release"}'), 'release'); - assert.strictEqual(readWith('{"git":[],"base_branch":"release"}'), 'release'); - assert.strictEqual(readWith('{"git":null,"base_branch":"release"}'), 'release'); - }); - - test('flat base_branch present but non-string / blank → null', () => { - assert.strictEqual(readWith('{"base_branch":true}'), null); - assert.strictEqual(readWith('{"base_branch":["main"]}'), null); - assert.strictEqual(readWith('{"base_branch":""}'), null); - assert.strictEqual(readWith('{"base_branch":" "}'), null); - }); - - test('config parses cleanly but carries neither key → null (distinct from an absent file)', () => { - // The absent-file path returns null after reading an empty string and never - // reaches JSON.parse. This one parses a real object and falls all the way - // through both key lookups to the final return. - assert.strictEqual(readWith('{"other":1}'), null); - assert.strictEqual(readWith('{}'), null); - assert.strictEqual(readWith(''), null, 'absent file (empty read) also yields null'); - }); - - test('positive controls: values are trimmed, and the nested key outranks the flat one', () => { - assert.strictEqual(readWith('{"git":{"base_branch":" develop "}}'), 'develop'); - assert.strictEqual(readWith('{"base_branch":" release\\n"}'), 'release'); - assert.strictEqual(readWith('{"git":{"base_branch":"nested"},"base_branch":"flat"}'), 'nested'); - }); -}); - describe('#3057 W3: trySymbolicRef — tier-2 output that resolves to nothing', () => { test('stdout is exactly "origin/" → null (prefix strip leaves an empty name)', () => { assert.strictEqual(gitBaseBranch.trySymbolicRef('/x', constGit({ stdout: 'origin/' })), null);