enhance(#3552): warn on additional protected branches beyond the resolved base branch (#3648)

* 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Dennis Kim
2026-08-29 17:00:52 -04:00
committed by GitHub
parent 472f585f7c
commit 8487f0ed42
38 changed files with 1824 additions and 137 deletions

View File

@@ -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<string, unknown>,
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<string, unknown>)[nested.field];
}
return undefined;
}
/** Shared nested-only config lookup; exported for parity tests. */
function _getConfigNested(parsed: Record<string, unknown>, section: string, field: string): unknown {
const sec = parsed[section];
if (sec !== null && typeof sec === 'object' && !Array.isArray(sec)) {
return (sec as Record<string, unknown>)[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<string, unknown> = {}): 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<string, unknown> = {}):
}
}
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<string, unknown> = {}):
}
}
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<string, unknown> = {}):
_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<string, unknown>;
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
* `<section>.<field>` 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<string, unknown> = {}):
})(),
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<string, unknown> = {}):
// 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<string, unknown> = {}):
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,

View File

@@ -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,
};

View File

@@ -309,6 +309,10 @@ function normalizeLegacyKeys(parsed: Record<string, unknown>): 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 };
}

View File

@@ -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 `<cwd>/.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<BaseBranchDeps, 'readFile'>
): 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<string, unknown>;
// 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<string, unknown>).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.<field>`, mirroring `loadConfigResolved`'s `getNested`. */
export function _readGitNested(config: Record<string, unknown>, field: string): unknown {
const git = config['git'];
if (git !== null && typeof git === 'object' && !Array.isArray(git)) {
return (git as Record<string, unknown>)[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<string, unknown>, 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 `<cwd>/.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<BaseBranchDeps, 'loadConfig' | 'readFile'>
): EffectiveGitConfig {
let config: Record<string, unknown> = {};
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<string, unknown>);
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 [<branch>]', 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));