From 8487f0ed42930ba3394791f670f456a2dabfc9ee Mon Sep 17 00:00:00 2001 From: Dennis Kim Date: Sat, 29 Aug 2026 17:00:52 -0400 Subject: [PATCH] enhance(#3552): warn on additional protected branches beyond the resolved base branch (#3648) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(01-01): add failing protected-branch warning coverage - pin configured, absent, and malformed branch-list behavior - require opposite CLI and execute warning outcomes * feat(01-01): warn on configured protected branches - resolve the base branch union configured protected branch names - expose exact boolean CLI comparison output for workflow callers - keep execute-phase warning advisory and within its byte budget * test(01-01): add failing protected branch config coverage - cover valid list persistence and null unset - reject hostile shapes while preserving the prior value * feat(01-01): validate protected branch configuration - register git.protected_branches as a canonical config key - require a non-empty array of non-blank branch names * test(01-02): add failing ship protected-branch controls - Execute both workflow warning blocks with exact predicate arguments - Require true and false results to produce opposite warning outcomes - Preserve the none-strategy feature-branch offer contract * feat(01-02): warn at ship on protected branches - Reuse the typed protected-branch predicate in ship preflight - Keep raw base resolution for PR targeting and advisory branch creation - Prove execute and ship warning blocks with opposite-result controls * test(01-02): add failing protected-branch docs parity - Require the canonical schema key in both English config references - Pin the non-empty string-array type and absent default - Require synchronized multi-branch examples and advisory semantics * feat(01-02): publish protected branch configuration contract - Document the optional non-empty string-array field in both references - Explain resolved-base union and absent-field compatibility - Keep execute and ship warnings advisory under branching_strategy none * fix(01): CR-01 honor active workstream branch policy * fix(01): WR-01 assert protected config path selection * docs: add changeset fragment for #3648 Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_017CteVPJt4BkPmroMPGajYx * fix(#3648): resolve base_branch precedence inversion and round-1 findings Blocker 1/2: production config resolution was flat-first, so a project that migrated to git.base_branch but still carried a stale flat base_branch got the old value back. Add base_branch to normalizeLegacyKeys (mirrors the existing branching_strategy/sub_repos pattern: canonical nested wins) and route readEffectiveGitConfig's test seam through the same normalization so it can't silently diverge from production again. Adds a regression test with both keys set that fails without the fix. Blocker 3/4/5: restore the handle_branching case-selector prose and "none" contract sentence that #3389's tests anchor on, and revert the unrelated prose/comment compaction in the same step — both were drive-by edits outside #3552's scope. Also addresses review majors/minors: delete readConfigBaseBranch and readConfigProtectedBranches (dead in production, only self-tested); --is-protected now fails closed (reports protected) instead of silently answering false when the base branch can't be verified; trim configured protected-branch names; fix HOME-without-USERPROFILE vacuous isolation on Windows; correct the drift-ack's byte accounting. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01S44stkuQbhD3jTCtKzte5N * test(#3648): add failing legacy-key hoist safety coverage Round-2 review found normalizeLegacyKeys block 5 records a normalization carrying the DISCARDED flat value on the canonical-wins branch. Probing that turned up a second, unreported defect in the same helper shape: blocks 1, 2 and 5 all spread result['git'] / result['planning'] with no object guard, so a config whose section key holds a string is spread into index keys — {"git":"main","base_branch":"release"} -> {"git":{"0":"m","1":"a","2":"i","3":"n","base_branch":"release"}} The resolved value is accidentally still correct, so nothing fails and no diagnostic fires. But normalizations.length > 0 sets configDirty, and config-loader then serializes that shape back into the user's config.json — a read that silently corrupts config. The deleted #3057 W3 suite covered {"git":"main","base_branch":"release"} explicitly; this is the input it would have caught. Covers both defects across blocks 1 and 5, with object/array/null negative controls that must stay green in both phases, and a fast-check property over arbitrary `git` values. * test(#3648): pin fail-closed handling of malformed protected_branches Replaces the test that pinned the fail-OPEN behaviour. The old assertion — ['develop', 42] yields isProtected === false for 'develop' — locked in the exact failure #3552 exists to close: config-set validation is bypassable by a direct edit of .planning/config.json, so a user who believes 'develop' is protected got a silent false and no warning. It was also inconsistent with the fail-CLOSED direction twelve lines away, where an unverified base reports protected and writes a diagnostic. A protection predicate must not have two opposite failure directions depending on which input is bad (#3648 review Blocker 3). New coverage: a bad element drops only itself, a non-array contributes no names, an empty list is well-formed rather than malformed, and --is-protected surfaces the rejection. Both negative controls — a clean list reports nothing rejected and writes no diagnostic — must stay green in either phase, so the reject channel cannot fire unconditionally. * fix(#3648): drop only invalid protected_branches and report them Partition git.protected_branches instead of discarding the whole list on one bad element, and carry the rejections out through ProtectedBranchStatus so --is-protected can name them on stderr. Valid names keep protecting; the user finds out the rest were ignored. A non-array value still contributes no names — a bare string is not a list of branch names — but is now reported rather than swallowed. An empty array stays silent: declaring no extra protected branches is a valid choice, not a misconfiguration. writeDiagnostic is hoisted out of the unverified-base branch since both arms now use it. * test(#3648): prove the predicate diagnostic survives both call sites The workflow bash stub now emits a stderr diagnostic the way the real command does, which is what makes a swallowed `2>/dev/null` visible to a test — previously the stub was silent on stderr, so discarding it changed no observable behaviour and the call sites could drop the explanation undetected. Adds the Minor 2 binding check as well: ship must expose the predicate result as IS_PROTECTED rather than only echoing a warning, asserted by running the extracted bash and reading the bound value, not by grepping the workflow source. Both tests carry opposite-outcome controls — an empty diagnostic must leave the text absent, and a false predicate must bind false. * fix(#3648): surface the predicate diagnostic and bind ship's result Drop `2>/dev/null` from the --is-protected call at both call sites. The fail-closed explanation and the new rejected-entry warning both go to stderr, so discarding it left the user with a bare "protected branch" warning on a branch that is not protected and no way to tell a real match from a degraded-git guess. `git branch --show-current` keeps its own redirect — that one is genuine noise. ship.md binds IS_PROTECTED and its prose now branches on the variable, so the following steps have evaluable state instead of having to infer it from warning text in tool output. execute-phase.md byte accounting refreshed: 92326 -> 92645, net growth 319 bytes (was 331 before the redirect came out). Baseline re-verified against the current rebase base by blob id; the ceiling check passes with 755 bytes of margin. * test(#3648): restore negative space for the readFile config seam The #3057 W3 suite was deleted with readConfigBaseBranch, but every arm it pinned 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 — and the four surviving readFile injections were positive-path only. protected_branches was never driven through this seam at all. Restores nine cases against the seam, including protected_branches partitioning, plus a control proving loadConfig still wins when both seams are supplied. Records honestly what the suite pins. Mutating the built lib shows .trim() is KILLED, while the non-object guard and the blank-string rejection SURVIVE — both are unreachable through this entry point for the same reasons the deleted suite documented against its own equivalents: a JSON-parsed non-object carries no relevant own-property either way, and a blank value is rejected a second time downstream by the resolver's truthiness check. They stay as defence-in-depth and are labelled known-unkillable rather than left looking like coverage this suite does not provide. * test(#3648): distinguish detached HEAD from a missing branch argument `args[1] ?? ''` collapsed two different situations into one: a detached HEAD, where `git branch --show-current` legitimately prints nothing, and the flag being called with no argument at all. Both answered false, so the right outcome arrived by an unintentional path and a caller bug was indistinguishable from normal operation. Asserts the detached case stays silent and the missing-argument case reports, with a control that the two diagnostics differ. * fix(#3648): report a missing --is-protected branch argument Answer false either way, but say so when the flag arrives with no argument. A detached HEAD passes an explicit empty string and stays silent, since that is a normal state rather than a misconfiguration. * docs(#3648): state exact-name matching and per-entry rejection isProtected is exact string equality, so a git-flow project must enumerate every release/* and hotfix/* by name. #3552 only asked for an integration-branch field, so the implementation satisfies the letter of the issue while leaving its git-flow motivation partly unserved — say so where users will meet it rather than leaving them to discover it. Also documents the Blocker 3 behaviour change: an invalid entry is ignored with a warning naming it and the remaining names still apply. Both statements land in docs/CONFIGURATION.md and gsd-core/references/planning-config.md, and the config-field-docs parity test asserts each in both so the two cannot drift. * refactor(#3648): extract isValidProtectedBranches for cross-surface pinning The `git.protected_branches` check inside `cmdConfigSet` and the resolver's per-entry filter in `git-base-branch.cts` are deliberately different shapes — all-or-nothing on write, per-entry on read, so a hand-edited config.json cannot fail the guard open. Nothing structural keeps their two definitions of "usable branch name" in step. Lifting the write-side check into a named, exported predicate lets a property test ask both surfaces about the same value and assert they agree, which is the fast-check gap the round-2 review flagged. No behaviour change: the predicate is the same expression, called from the same place. * fix(#3648): stop --is-protected rewriting the config it is asking about `gsd_run query git.base-branch --is-protected` runs on every execute-phase and every ship. It resolved config through `loadConfig`, whose normalize-then-write path rewrites `.planning/config.json` whenever any legacy key normalizes — so a boolean question was silently editing the user's checked-in config. This PR had widened the trigger by adding a fifth normalization block (top-level `base_branch` -> `git.base_branch`), making it fire for exactly the projects the feature targets. `loadConfigResolved` gains `options.persist` (opt-OUT, default true): resolution is unchanged, only the two write-back side effects are suppressed. The predicate passes `persist: false`; the ~30 other callers are untouched, so a legacy config is still migrated by ordinary use. Asserted on BYTES rather than parsed shape, because the rewrite reorders keys and reflows whitespace even when the values are equivalent. Three tests, each with its own control: the end-to-end CLI leaves the file byte-identical while still answering `true` from the legacy key (proving the config WAS read); an ordinary persisting load of the same fixture DOES change the bytes (proving the fixture is live rather than inert); and `persist:false` vs default over one directory returns deep-equal config while differing on the write. Reverting the one-line `persist: false` fails the first of those and only that one. Also from the review: - `readEffectiveGitConfig`'s comment claimed the readFile branch routed "through the same precedence authority production uses". It does not, and cannot — it reproduces two of production's steps over a single file. The comment now names what the seam covers and what it does NOT (root/workstream deep merge, builtin and global defaults, federated merge), and the seam now applies production's flat-then-nested lookup so it stops disagreeing about a surviving flat key. - The missing-argument diagnostic promised "answering false", which the fail-closed guard on the same call can contradict by printing `true`. It now states what it did with the argument and leaves the answer to stdout. * test(#3648): re-pin block 5 on #3760's refusal contract #3767 landed on next while this PR was in review and fixed the non-object config-section defect properly: a present-but-non-object section now BLOCKS its own migration — value preserved, no Normalization pushed, refusal reported via `skipped[]` — rather than being rebuilt from a plain-object view. That supersedes this branch's round-2 `hoistLegacyKey`, which prevented the character-key spread but still dropped the section value silently, and which the round-3 review correctly called out as destruction in place of corruption. The rebase drops that commit and routes block 5 through the upstream helper. This file's tests asserted the superseded design, so they are rewritten to pin block 5 — `base_branch` -> `git.base_branch`, which did not exist when #3760's suite was written — against the contract that now governs it: ordinary hoist into an absent/null/object section, canonical-nested-wins, and refusal for each of string/number/boolean/array sections with the exact `skipped` entry. Two controls keep it from passing vacuously: the refusal must be scoped to block 5 (an unrelated block still normalizes in the same call), and a property over arbitrary `git` values asserts hoist and refusal are exhaustive AND mutually exclusive per key, that a refusal leaves both the section and the legacy key untouched, and that a hoist manufactures no index key the input did not carry. * docs(#3648): correct the Git Query and Config Loader module contracts CONTEXT.md's Git Query Module still described base-branch tier 1 as a direct `.planning/config.json` read. Since this PR it is the EFFECTIVE configuration resolved by the Config Loader — a materially different authority, carrying the root/workstream deep merge, flat-then-nested lookup and builtin/federated defaults. The `--is-protected` predicate, `git.protected_branches`, and the two invariants that distinguish the predicate from the plain query (fails closed on an unverified base; must not write) were undocumented entirely. The Config Loader entry now states that loading is not side-effect-free by default and documents `options.persist`. docs/INVENTORY.md's `git-base-branch.cjs` row carried the same stale ladder and no mention of the predicate. `node scripts/gen-inventory-manifest.cjs --write` was run and produced no diff: the manifest indexes roster NAMES, not row prose, so a description edit cannot move it. Also closes the global-defaults minor: `git.protected_branches` is inert in `~/.gsd/defaults.json`, but so is every other `git.*` key — no branch-policy key appears in `_globalBaseCfg` or `GLOBAL_DEFAULTS_RESOLUTION_KEYS`. That is section-wide and predates this PR, so the fix is to state the scope where users meet it rather than to quietly extend the resolution set for two new keys. * fix(#3648): close four defects found by the round-4 external review Two external reviewers (codex, antigravity/Gemini 3.1 Pro) were run adversarially against this branch. Four findings reproduced against source; each is fixed with a failing-first test and a control, and each fix was verified by reverting it and watching exactly the intended test fail. 1. `persist:false` was DROPPED by the workstream fallback (codex). Blocker 1 was only half closed. `loadConfigResolved` re-enters itself with a bare `{ workstream: null }` when a workstream has no config.json of its own, and that literal discarded every other option — so the recursive pass ran at the DEFAULT persistence and rewrote the ROOT config. Reproduced: with GSD_WORKSTREAM=alpha and a legacy flat `base_branch`, `--is-protected` rewrote `.planning/config.json` despite `persist:false`. Both recursions now forward `options` and override only `workstream`; the explicit override still wins the hasOwnProperty check, so spreading cannot let `workstreamContext` reintroduce a workstream. 2. Both workflow call sites failed OPEN, and aborted under `set -e` (both reviewers, independently). `IS_PROTECTED=$(gsd_run ...)` yields an empty string when the query fails, so `[ "$X" = true ]` was simply false: no warning, no trace — a silent hole in the guard whose only job is to warn. The bare assignment also aborted the step under `set -e`. Both sites now degrade VISIBLY: `|| IS_PROTECTED=""`, then an explicit empty-string arm that says the check did not run. Deliberately not fail-closed — claiming "protected" on no evidence would warn on every branch whenever gsd-tools is unavailable. 3. `isValidProtectedBranches` and the resolver disagreed on a sparse array (antigravity). `.every()` skips holes; the resolver's `for...of` yields `undefined` for them, so `["main", , "develop"]` was accepted by config-set and rejected by the resolver. The cross-surface property passed only because `fc.array` cannot generate a hole. The predicate now indexes, and the generator punches holes so that axis is actually falsifiable. JSON cannot express a hole, so this is unreachable in production — but two definitions of one predicate must not contradict each other. 4. A top-level `protected_branches` silently outranked `git.protected_branches` (antigravity). Routing the key through `get(key, {section, field})` gave it flat-then-nested precedence, which is back-compat for keys `normalizeLegacyKeys` migrates. `protected_branches` is new in #3552 and has no legacy form, so that invented an undocumented alias. It now resolves nested-only through a new `getNested`, in production and in the test seam. `base_branch` keeps flat-then-nested — it HAS a legacy spelling that #3760's refusal path can leave behind — and a control pins that distinction. Also narrows a CONTEXT.md claim this round introduced. The predicate fails closed only when a git query TIMED OUT or could not be spawned (#3057 B4's `verified`); a git command that runs and exits non-zero counts as a clean negative, so a cwd that is not a repository answers `false`, not `true`. Verified pre-existing on next @ 738f42f4, so the documentation was over-claiming rather than the code regressing — but an over-broad contract is exactly what the module docs must not carry. Both workflow byte figures re-derived after the call-site change: execute-phase.md 92356 -> 92865 (+509), ship.md 36784 -> 37227 (+443). * test(#3648): pin git config read parity * docs(#3648): document git query contracts * fix(#3648): expose protected branch default * test(#3648): snapshot planning tree for read-only query * test(#3648): pin planning snapshot stray-write detection * fix(#3648): resolve merge conflict from #3078's ack-fragment sweep next swept the fully-spent 2818/3003 ack fragments this branch had appended to (#3078, a84f7563). Rebased onto upstream/next and took the deletions on both, then moved the #3552 append into a new fragment of its own. Rebasing onto the current base also left execute-phase.md only 34 bytes under the frozen ADR-857 Phase 6 margin ceiling (93400 bytes) — intervening next PRs consumed the rest while this PR was in review. Extracted the "none" arm's protected-branch-warning bash block into gsd-core/workflows/execute-phase/steps/protected-branch.md (content unchanged, matching the existing steps/ extraction pattern used elsewhere in this file) so the inline growth is a one-line pointer instead of the full block. 93366 -> 93385 bytes (+19), 15 bytes inside the ceiling. * fix(#3648): drop stale ack entry for the new step file The extracted execute-phase/steps/protected-branch.md needed no acknowledgment of its own — the differential-attribution check flagged the entry as stale once the build ran, so removed it and kept the two growth entries (execute-phase.md, ship.md) that actually needed one. * fix(#3648): follow the step-file reference in the bash-extraction test helper extractProtectedBranchWarningBash() read the "none" arm's bash block directly out of execute-phase.md. That block now lives in execute-phase/steps/protected-branch.md (byte-ceiling extraction); the helper follows the step-file reference and extracts from there when no inline block is found, so the three execute-phase tests that execute this bash for real keep exercising the actual behavior. * fix(#3648): regenerate INVENTORY-MANIFEST.json and satisfy the CRLF-fragile lint rule - gen-inventory-manifest.cjs --write to pick up the new execute-phase/steps/protected-branch.md entry (already covered by docs/INVENTORY.md's generic workflow_steps wildcard row, so no INVENTORY.md edit is needed). - Reworked the step-file-reference lookup in extractProtectedBranchWarningBash() to avoid a bare-\n regex split on file content (local/no-crlf-fragile-split), using the same line-array scan the function already uses elsewhere. * fix(#3648): regenerate golden install-tree fixtures for the new step file npm run gen:install-tree, adding gsd-core/workflows/execute-phase/ steps/protected-branch.md to all 19 runtime install-tree fixtures. CI's tests/golden-install-tree.test.cjs caught this on push — I'd verified the differential-attribution and INVENTORY-MANIFEST checks but missed this separate golden-fixture check for the new file. * fix(#3648): add the canonical gsd_run preamble to the new step file CI's runtime-launcher-parity suite requires exactly one canonical resolver preamble in every workflow .md that calls gsd_run. The inline "none"-arm block never needed one (execute-phase.md already carried a preamble elsewhere in the same file), but the extracted execute-phase/steps/protected-branch.md is now its own file with no preamble of its own. Ran node scripts/sync-runtime-launcher.cjs to insert it (execute-phase.md itself is untouched — still 93385 bytes, inside the ADR-857 ceiling). That preamble defines its own gsd_run(), which shadows the mock tests/git-base-branch.test.cjs injects for the three #3648 tests that execute this bash for real — without stripping it, those tests reached the real gsd-tools.cjs on the machine running them instead of the test's fixture. Preamble correctness is already covered by tests/runtime-launcher-parity.test.cjs, so extractProtectedBranchWarningBash() now strips the preamble line before handing the block to the harness; it only needs to exercise the #3552 warning logic. * fix(#3552): address PR 3648 review feedback on protected branch warnings - Fix execute-phase handle_branching branching_strategy=none instruction to "Read and execute execute-phase/steps/protected-branch.md" - Use io.error(..., ERROR_REASON.USAGE) for cmdGitBaseBranch usage errors - Align git.protected_branches schema default to (none) without fallback [] - Relocate CONTEXT.md forward-referencing sentence into module body - Sanitize control and ANSI characters in renderRejected diagnostics - Clean up out-of-scope whitespace hunks in gsd-tools.cjs Emitted-Drift-Ack-Growth: execute-phase.md — #3552: execute-phase handle_branching adds a pointer to execute-phase/steps/protected-branch.md for branching_strategy=none so the protected-branch check executes while keeping execute-phase.md within the ADR-857 Phase 6 margin ceiling (93400 bytes). 93392 bytes, 8 bytes inside the ceiling. Emitted-Drift-Ack-Growth: ship.md — #3552: ship preflight step 3 now asks the same typed git.base-branch --is-protected predicate as execute-phase, binding IS_PROTECTED and warning without refusing execution or blocking the branching_strategy=none feature-branch offer; it degrades visibly (rather than silently reading an empty result as "not protected") when the query itself fails to run. 36841 bytes, well inside the XL cap (98304, tests/workflow-size-budget.test.cjs). --------- Co-authored-by: Claude Sonnet 5 Co-authored-by: Tom Boucher --- .changeset/lively-sloths-wave.md | 5 + CONTEXT.md | 4 +- docs/CONFIGURATION.md | 34 + docs/INVENTORY-MANIFEST.json | 1 + docs/INVENTORY.md | 2 +- .../bin/shared/config-schema.manifest.json | 1 + gsd-core/references/planning-config.md | 23 + gsd-core/workflows/execute-phase.md | 2 +- .../execute-phase/steps/protected-branch.md | 21 + gsd-core/workflows/ship.md | 10 +- src/config-loader.cts | 79 +- src/config.cts | 35 + src/configuration.cts | 4 + src/git-base-branch.cts | 260 +++- tests/config-field-docs.test.cjs | 49 + tests/config-get-default.test.cjs | 9 + tests/config.test.cjs | 84 ++ ...nfiguration-normalize-legacy-keys.test.cjs | 159 +++ tests/fixtures/install-tree/antigravity.json | 1 + tests/fixtures/install-tree/augment.json | 1 + tests/fixtures/install-tree/claude-local.json | 1 + tests/fixtures/install-tree/claude.json | 1 + tests/fixtures/install-tree/cline.json | 1 + tests/fixtures/install-tree/codebuddy.json | 1 + tests/fixtures/install-tree/codex.json | 1 + tests/fixtures/install-tree/copilot.json | 1 + tests/fixtures/install-tree/cursor.json | 1 + tests/fixtures/install-tree/hermes.json | 1 + tests/fixtures/install-tree/kilo.json | 1 + tests/fixtures/install-tree/kimi-code.json | 1 + tests/fixtures/install-tree/kimi.json | 1 + tests/fixtures/install-tree/opencode.json | 1 + tests/fixtures/install-tree/pi.json | 1 + tests/fixtures/install-tree/qwen.json | 1 + tests/fixtures/install-tree/trae.json | 1 + tests/fixtures/install-tree/windsurf.json | 1 + tests/fixtures/install-tree/zcode.json | 1 + tests/git-base-branch.test.cjs | 1160 +++++++++++++++-- 38 files changed, 1824 insertions(+), 137 deletions(-) create mode 100644 .changeset/lively-sloths-wave.md create mode 100644 gsd-core/workflows/execute-phase/steps/protected-branch.md create mode 100644 tests/configuration-normalize-legacy-keys.test.cjs 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);