From debeabd5249c4448ea4f1604697d79e33cb423bc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 17 Aug 2026 21:59:40 -0400 Subject: [PATCH] enhance(#3587): add a per-phase commit_docs override (#3601) * feat(#3587): add a per-phase commit_docs override Delivers epic #2292's second user story: commit an architecture phase's artifacts while execution phases stay local. commit_docs was project-wide and binary, so the only choices were all phases or none. Shape is a config dynamic key phase_commit_docs., following the 14 existing dynamicKeyPatterns precedents rather than inventing a PLAN.md frontmatter spec -- which #2292 itself flags as becoming its own maintenance surface. Tier 1 resolves in cmdCommit, NOT in loadConfig: loadConfig has no phase context and is called by nearly every command, so threading one through it to serve a single caller would be a far larger blast radius for no gain. The phase comes from detectPhaseNumberFromFiles, which cmdCommit already computes for branch naming and which is already hardened against the #2539 project-code bug. Suppression by the per-phase tier returns its own reason rather than reusing skipped_commit_docs_false -- telling a user their project setting is false when it is true would be actively misleading. Additive; the two existing reason strings that agents/gsd-executor.md matches on are unchanged. The manifest's phase-id pattern is a hand-copy of PHASE_NUMBER_TOKEN_SOURCE because the manifest is hand-maintained JSON, so a behavioral parity test asserts both surfaces accept and reject the same token shapes. * fix(#3587): fold tests, close review findings, update reference docs Fold: the new tests were added as their own file, which required loosening a grandfathered lint-test-file-count bucket 5-to-6. A ratchet exists to go down only. commit-docs-bypass.test.cjs is the established commit_docs test home and already hosts two folded suites, so the tests fold there as a third block and the allowlist is reverted untouched. Standards review: CONTEXT.md and the test header both cited a phase-commit-docs-manifest-parity.test.cjs that never existed; a repo-wide sweep found a fourth stale cite in the schema manifest description. All four now name the real location. Spec review: the issue's Scope of changes named planning-config.md and git-planning-commit.md and neither was touched. Both now document the four-tier precedence and the new skip reason. Security review, minor and unproven: detectPhaseNumberFromFiles returns the FIRST matching path's phase, so a --files list spanning two phases resolves the override against whichever comes first. That helper is hardened and widely used, so it is not changed; the behavior is pinned by a named test and disclosed in the design and user docs. A pinned behavior is not a bug; an unpinned surprise is. * chore(#3587): backfill changeset pr number to 3601 --------- Co-authored-by: sim --- .changeset/rapid-orcas-tumble.md | 5 + CONTEXT.md | 4 +- docs/CONFIGURATION.md | 49 ++ docs/how-to/keep-planning-docs-private.md | 49 +- .../bin/shared/config-schema.manifest.json | 7 +- gsd-core/references/git-planning-commit.md | 3 +- gsd-core/references/planning-config.md | 4 +- src/commands.cts | 102 +++- src/config-loader.cts | 7 + src/config.cts | 2 +- tests/commit-docs-bypass.test.cjs | 503 ++++++++++++++++++ 11 files changed, 718 insertions(+), 17 deletions(-) create mode 100644 .changeset/rapid-orcas-tumble.md diff --git a/.changeset/rapid-orcas-tumble.md b/.changeset/rapid-orcas-tumble.md new file mode 100644 index 000000000..7e6ade412 --- /dev/null +++ b/.changeset/rapid-orcas-tumble.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3601 +--- +**Per-phase `commit_docs` override** — set `phase_commit_docs.` to commit one phase's `.planning/` artifacts (e.g. an architecture phase) while keeping other phases local, without flipping the project-wide `commit_docs` switch. (#3587) diff --git a/CONTEXT.md b/CONTEXT.md index 62f9769f8..da1c05b4d 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -222,7 +222,9 @@ Module owning agent-presence resolution and verification, extracted from the Cor Module owning project configuration loading: reads `.planning/config.json`, merges built-in defaults (`CONFIG_DEFAULTS`/`CANONICAL_CONFIG_DEFAULTS`), normalizes legacy keys, applies the active-workstream overlay, validates against the config schema, and warns on unknown keys/profile overrides. Primary interface: `loadConfigResolved(cwd, options) → ConfigResolution { config, source, degraded }` (provenance-aware, ADR-1411 P2 / #1415) — `source` ∈ `'workstream' | 'root' | 'builtin-defaults' | 'global-defaults'`; `degraded:true` when a workstream was requested but its config.json was absent (fell back to root config). `loadConfig(cwd, options) → Record` is the back-compat thin wrapper over `loadConfigResolved` (byte-identical result). Resolution is **caller-anchored, not loader-anchored**: `loadConfigResolved` resolves `cwd` as-is (no walk-up), so `loadConfig` stays byte-identical for its callers; callers that need cwd-drift tolerance (e.g. `cmdAgentSkills`) anchor to the project root via `findProjectRoot` (Project-Root Resolution Module) *before* calling `loadConfigResolved`. Helper exports: `_deepMergeConfig`, `isGitIgnored`, `_warnUnknownProfileOverrides`. Depends only on leaf modules (`configuration`, `config-schema`, `planning-workspace`, `shell-command-projection`, `core-utils`, `model-catalog`) — no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2e (#885) as the prerequisite for the model-resolver extraction (the resolvers call `loadConfig`); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/config-loader.cjs` (generated from `src/config-loader.cts`). ### Planning Commit Gate (`commit_docs`) -The seam deciding whether `.planning/` artifacts reach git. `commit_docs` resolves in the Config Loader Module (`loadConfigResolved`) through an ordered chain — an explicit value (top-level `commit_docs` or its `planning.commit_docs` alias) wins; absent that, `isGitIgnored(cwd, '.planning/')` auto-resolves it to `false`; otherwise the manifest default (`true`) applies. `cmdCommit` (Command Module) is the ONLY sanctioned writer: it returns the typed skip envelope `{ committed: false, skipped: true, hash: null, reason }` with `reason ∈ { 'skipped_commit_docs_false', 'skipped_gitignored' }` rather than erroring, and `skipped: true` is explicit so agent prompts match a first-class success signal instead of inferring a skip from a missing `committed` and improvising a raw-git fallback (#3678). Those `reason` strings are a de facto public contract — `agents/gsd-executor.md` pattern-matches on them — so they are additive-only. **The two reasons are ordered, not peers, and `skipped_gitignored` is near-unreachable in a real project** (measured #3585): `cmdCommit` tests resolved `commit_docs` FIRST, and whenever `.planning/config.json` exists — which it does in every initialized project — the loader's gitignore auto-detect has already resolved that value to `false`, so the first branch returns `skipped_commit_docs_false` and the `isGitIgnored` branch below it is never reached. `skipped_gitignored` fires only when `config.json` is absent entirely, so the loader falls back to the `true` default and `cmdCommit`'s own check is what fires. A gitignore-driven skip therefore reports the config-driven reason; both members are behaviorally pinned by `tests/commit-docs-bypass.test.cjs` (B1-B3 and G1) so a rename fails loudly, but the reason a user sees does not distinguish *which* input suppressed the commit. **The gate is bypassable only from OUTSIDE the code**: a workflow step that types `git add` into its own shell reaches the index without passing through `cmdCommit`, and no code change can intercept that. Two guards therefore enforce it as text rather than at runtime: `tests/commit-files-pathspec.test.cjs` (#2269) requires every shipped `commit` invocation to declare `--files`, and `tests/commit-docs-bypass.test.cjs` (#1783, made repo-wide by #3585) requires every shipped `git add` able to reach `.planning/` to sit inside an **executable** `commit_docs` check — a markdown prose conditional ("**If `commit_docs` is true:**") is not a guard, because the bash block below it runs regardless. Both consume one shell tokenizer (`tests/helpers/shipped-command-scan.cjs`) and one exemption marker (`# gsd-scan-ignore: #NNN`, reason must cite a tracking ref per ADR-456). Guard state does not cross a fenced-block boundary: each fenced block is its own shell, so a guard opened in one block does not protect a `git add` in the next. Known limit: `.gitignore` has no effect on files git already TRACKS, so a project that committed `.planning/` before ignoring it keeps staging those paths — see `gsd-core/references/planning-config.md` and #3586. +The seam deciding whether `.planning/` artifacts reach git. `commit_docs` resolves in the Config Loader Module (`loadConfigResolved`) through an ordered chain — an explicit value (top-level `commit_docs` or its `planning.commit_docs` alias) wins; absent that, `isGitIgnored(cwd, '.planning/')` auto-resolves it to `false`; otherwise the manifest default (`true`) applies. `cmdCommit` (Command Module) is the ONLY sanctioned writer: it returns the typed skip envelope `{ committed: false, skipped: true, hash: null, reason }` with `reason ∈ { 'skipped_commit_docs_false', 'skipped_gitignored', 'skipped_commit_docs_phase_false' }` rather than erroring, and `skipped: true` is explicit so agent prompts match a first-class success signal instead of inferring a skip from a missing `committed` and improvising a raw-git fallback (#3678). Those `reason` strings are a de facto public contract — `agents/gsd-executor.md` pattern-matches on them — so they are additive-only. + +**Per-phase override (#3587, epic #2292 Phase 3).** A NEW tier resolves ABOVE the chain above, entirely inside `cmdCommit` (`src/commands.cts`'s `resolveCommitDocsPolicy`/`resolvePhaseCommitDocsOverride`) — deliberately NOT inside `loadConfigResolved`, which has no phase context and is called by nearly every command. The dynamic config key `phase_commit_docs.` (a `{ "": boolean }` map, registered in `config-schema.manifest.json`'s `dynamicKeyPatterns` and threaded through `config-loader.cts`'s `_baseConfig` projection the same way `agent_skills` is — a dynamic key absent from that hand-maintained allowlist is silently dropped on read, the exact failure mode `features.` demonstrates today) lets a tech lead commit one phase's artifacts while the project-wide `commit_docs` stays `false` (or the reverse). The phase being committed is resolved via the PRE-EXISTING `detectPhaseNumberFromFiles(files)` (the same #2539-hardened, project-code-aware derivation `branching_strategy` already used one branch below) and normalized through `normalizePhaseName` on both sides of the comparison, so `3`/`03`/`PROJ-03` hit one entry and a value scoped to a DIFFERENT phase never leaks. A non-boolean stored value (`"true"`, `1`, `null`) is never coerced — it falls through to the pre-existing chain untouched. When this tier suppresses a commit, the envelope's reason is `skipped_commit_docs_phase_false` — deliberately distinct from `skipped_commit_docs_false`, so a per-phase suppression is never reported as "your project setting is false" when it is actually `true`. **AC4 (byte-identical when unset):** with no `phase_commit_docs` key, this tier is a no-op and the three-tier chain above resolves exactly as before — pinned by the `folded:phase-commit-docs` block's C1-C5 in `tests/commit-docs-bypass.test.cjs`. The `phase_commit_docs.` grammar is a hand-copy of the canonical `PHASE_NUMBER_TOKEN_SOURCE` (`src/phase-id.cts`, #2128) into the hand-maintained schema manifest — pinned against drift by that same block's describe 'E' in `tests/commit-docs-bypass.test.cjs` (behavioral, over a shared shape list, per CLAUDE.md's Generative Fix Divergence class). **The two reasons are ordered, not peers, and `skipped_gitignored` is near-unreachable in a real project** (measured #3585): `cmdCommit` tests resolved `commit_docs` FIRST, and whenever `.planning/config.json` exists — which it does in every initialized project — the loader's gitignore auto-detect has already resolved that value to `false`, so the first branch returns `skipped_commit_docs_false` and the `isGitIgnored` branch below it is never reached. `skipped_gitignored` fires only when `config.json` is absent entirely, so the loader falls back to the `true` default and `cmdCommit`'s own check is what fires. A gitignore-driven skip therefore reports the config-driven reason; both members are behaviorally pinned by `tests/commit-docs-bypass.test.cjs` (B1-B3 and G1) so a rename fails loudly, but the reason a user sees does not distinguish *which* input suppressed the commit. **The gate is bypassable only from OUTSIDE the code**: a workflow step that types `git add` into its own shell reaches the index without passing through `cmdCommit`, and no code change can intercept that. Two guards therefore enforce it as text rather than at runtime: `tests/commit-files-pathspec.test.cjs` (#2269) requires every shipped `commit` invocation to declare `--files`, and `tests/commit-docs-bypass.test.cjs` (#1783, made repo-wide by #3585) requires every shipped `git add` able to reach `.planning/` to sit inside an **executable** `commit_docs` check — a markdown prose conditional ("**If `commit_docs` is true:**") is not a guard, because the bash block below it runs regardless. Both consume one shell tokenizer (`tests/helpers/shipped-command-scan.cjs`) and one exemption marker (`# gsd-scan-ignore: #NNN`, reason must cite a tracking ref per ADR-456). Guard state does not cross a fenced-block boundary: each fenced block is its own shell, so a guard opened in one block does not protect a `git add` in the next. Known limit: `.gitignore` has no effect on files git already TRACKS, so a project that committed `.planning/` before ignoring it keeps staging those paths — see `gsd-core/references/planning-config.md` and #3586. ### Model Catalog Module Leaf module owning the **static** model tables and the closed vocabularies derived from them — the tier/runtime/provider enums (`VALID_TIERS`, `VALID_AGENT_TIERS`, `KNOWN_RUNTIMES`, `KNOWN_PROVIDERS`, `RUNTIMES_WITH_REASONING_EFFORT`, `RUNTIMES_WITH_FAST_MODE`, `ADAPTIVE_TIER_VALUES`), the alias and profile maps (`MODEL_ALIAS_MAP`, `RUNTIME_PROFILE_MAP`, `PROVIDER_PRESETS`), effort rendering (`renderEffortForRuntime`), and the agent→model projections (`getAgentToModelMapForProfile`, `formatAgentToModelMapAsTable`). A **genuine leaf**: it imports `node:path` and its own `model-catalog.json` and nothing else, which is what makes it the correct home for anything several unrelated surfaces must agree on. Model *ids* live in `model-catalog.json`, never inline — changing one means regenerating goldens (`UPDATE_GOLDEN`). Also owns the **Anthropic-flavored-model rule** (#3241, ADR-2313): `CLAUDE_AGENT_ALIASES` (the frozen four-alias set `opus`/`sonnet`/`haiku`/`fable`) and `isAnthropicFlavoredModel(model)`, which is true for a bare tier alias or for any `claude-*` id in any provider namespacing (`anthropic/claude-*`, `us.anthropic.claude-*`); no OpenAI/Codex model id contains "claude", so the case-insensitive substring test is safe and exhaustive. It was **moved down here from the Model Resolver Module** rather than shared from there, because the Codex posture check (`agent-install-check`, epic #2313 Phase 2) and the Codex `.toml` sync (`commands`, Phase 3) both need the rule and neither may take the `config-loader` dependency `model-resolver` would have brought; `model-resolver` re-exports it for back-compat and a parity test fails if the two ever fork. _Avoid_: "the model list" (ambiguous between the catalog JSON and the derived enums). See Model Resolver Module, ADR-2313, and ADR-0003. diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 5183845b6..c79f64d49 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -530,6 +530,55 @@ Note: a file deliberately force-added under an otherwise-ignored `.planning/` (` .planning/keep.md`) triggers this same warning — there is no reliable way to distinguish an intentional force-add from the accidental case above, so `W029` is expected in that situation too. +### Per-Phase Override (`phase_commit_docs`) + +`commit_docs` is a single project-wide switch by default, but a tech lead may want to commit one +phase's artifacts (e.g. an architecture or ADR phase) while keeping execution phases local. Set a +dynamic key of the form `phase_commit_docs.` to override `commit_docs` for that phase only: + +```bash +gsd-tools config-set phase_commit_docs.03 true +gsd-tools config-set phase_commit_docs.07 false +``` + +```json +{ + "commit_docs": false, + "phase_commit_docs": { + "03": true, + "07": false + } +} +``` + +The `` segment accepts the same phase-number shapes GSD uses elsewhere (`3`, `03`, +`12A`, `3.2` — a project-code prefix like `PROJ-03` is normalized to the bare phase number before +lookup), so `phase_commit_docs.3` and `phase_commit_docs.03` refer to the same entry. + +**Resolution order** (highest wins) when `gsd-tools commit` / `query commit` resolves the phase from +the committed `--files` paths: + +1. `phase_commit_docs.` for the phase being committed +2. explicit `commit_docs` / `planning.commit_docs` in config.json +3. `.gitignore` auto-detect (see [Auto-Detection](#auto-detection) above) +4. the manifest default (`true`) + +A per-phase value must be a real boolean — `"true"` (string), `1`, or `null` are never coerced and +fall through to the next tier. A value set for a different phase than the one being committed never +applies (no cross-phase leak). A commit that names no phase-scoped file (e.g. a project-wide +`ROADMAP.md`-only commit) has no phase to look up, so tier 1 is inapplicable and resolution starts +at tier 2 — unchanged from pre-#3587 behavior. + +When tier 1 suppresses a commit, the skip envelope's `reason` is +`skipped_commit_docs_phase_false` — distinct from the project-wide `skipped_commit_docs_false` — +so a caller is never told "commit_docs is false" when the project setting is actually `true`. + +A commit spanning multiple phases resolves the override against the first phase in the `--files` +list, so scope `--files` to one phase when using the override. + +See [Keep planning docs out of a shared repo](how-to/keep-planning-docs-private.md#per-phase-override) +for a worked example. + --- ## Hook Settings diff --git a/docs/how-to/keep-planning-docs-private.md b/docs/how-to/keep-planning-docs-private.md index 293dd28c8..c51f9474d 100644 --- a/docs/how-to/keep-planning-docs-private.md +++ b/docs/how-to/keep-planning-docs-private.md @@ -87,10 +87,55 @@ it — removing files from the index is destructive and the timing is yours. - **Teammates who have already pulled the tracked files** will see them deleted by your step-3 commit. That is the intended effect — the files leave the repo, not their working copies of your branch — but say so in the commit message or PR description so it is not a surprise. -- **Per-phase control** (committing docs for an architecture phase while keeping execution phases - local) is tracked separately; `commit_docs` is currently project-wide. + +## Per-phase override + +You just made the whole project local-only in step 1. If instead you want ONE phase's artifacts — +say, an architecture or ADR phase whose PLAN.md and REQUIREMENTS.md are worth sharing with the +team — committed while every other phase stays local, set a `phase_commit_docs` entry for that +phase's number instead of (or on top of) the project-wide switch: + +```bash +gsd-tools config-set planning.commit_docs false +gsd-tools config-set phase_commit_docs.03 true +``` + +```json +{ + "planning": { "commit_docs": false }, + "phase_commit_docs": { "03": true } +} +``` + +Now `gsd-tools query commit` commits phase 03's artifacts normally, and skips (with the +`skipped_commit_docs_phase_false`-or-`skipped_commit_docs_false` reason depending on which tier +decided) every other phase's — the project-wide setting from step 1 still governs everything +`phase_commit_docs` does not name. + +A few things worth knowing before you rely on this: + +- **The phase-id form doesn't matter.** `phase_commit_docs.3`, `phase_commit_docs.03`, and (if your + project uses a `project_code`) `phase_commit_docs.PROJ-03` all resolve to the same entry — GSD + normalizes the phase number before lookup, the same way it does everywhere else phase numbers are + compared. +- **It only applies to a commit that names a phase-scoped file.** `gsd-tools query commit` resolves + the phase from the `--files` paths you pass it (via the `.planning/phases//…` + segment). A commit that names no phase file — e.g. a bare `ROADMAP.md` update — has no phase to + look up, so `phase_commit_docs` never applies to it and the project-wide setting governs, same as + before this feature existed. +- **It's per phase, not per artifact.** You cannot commit a phase's ADR but suppress its SUMMARY + within the same commit; the override applies to the whole phase. +- **Reverse direction works too.** With the project-wide default (`commit_docs: true`), set + `phase_commit_docs. false` to suppress just one noisy execution phase while everything + else commits normally. + +Full precedence order (highest wins): `phase_commit_docs.` → explicit `commit_docs` / +`planning.commit_docs` → `.gitignore` auto-detect → the manifest default. See +[Configuration reference — per-phase override](../CONFIGURATION.md#per-phase-override-phase_commit_docs) +for the complete rules, including how a non-boolean value is handled. ## Related - [Configuration reference — `planning.commit_docs`](../CONFIGURATION.md#planning-settings) - [Configuration reference — auto-detection and the tracked-files caveat](../CONFIGURATION.md#auto-detection) +- [Configuration reference — per-phase override](../CONFIGURATION.md#per-phase-override-phase_commit_docs) diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index 0369a1e2b..f7c5e8727 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -181,7 +181,12 @@ { "topLevel": "model_policy", "source": "^model_policy\\.runtime_tiers\\.[a-zA-Z0-9_-]+\\.(opus|sonnet|haiku)$", - "description": "model_policy.runtime_tiers.." + "description": "model_policy.runtime_tiers.. (#3587)" + }, + { + "topLevel": "phase_commit_docs", + "source": "^phase_commit_docs\\.\\d+[A-Z]?(?:\\.\\d+)*$", + "description": "phase_commit_docs. — per-phase commit_docs override (#3587). The segment is a hand-copy of the canonical PHASE_NUMBER_TOKEN_SOURCE grammar owned by src/phase-id.cts (#2128); this manifest is hand-maintained JSON so it cannot import that constant. Pinned against drift by the behavioral parity test in tests/commit-docs-bypass.test.cjs (folded 'phase-commit-docs' block, describe block 'E', #3587) — do not hand-edit this pattern without updating that test." } ] } diff --git a/gsd-core/references/git-planning-commit.md b/gsd-core/references/git-planning-commit.md index 79e6cfa6b..97fdae8dd 100644 --- a/gsd-core/references/git-planning-commit.md +++ b/gsd-core/references/git-planning-commit.md @@ -12,7 +12,7 @@ Always use this for `.planning/` files — it handles `commit_docs` and gitignor gsd-tools query commit "docs({scope}): {description}" --files .planning/STATE.md .planning/ROADMAP.md ``` -The CLI will return `skipped` (with reason) if `commit_docs` is `false` or `.planning/` is gitignored. No manual conditional checks needed. +The CLI will return `skipped` (with reason) if `commit_docs` is `false`, `.planning/` is gitignored, or a per-phase `phase_commit_docs.` override resolves `false` for the phase being committed. No manual conditional checks needed. ## Amend previous commit @@ -37,4 +37,5 @@ gsd-tools query commit "" --files .planning/codebase/*.md --amend - `commit_docs: false` in config - `.planning/` is gitignored +- `phase_commit_docs.` resolves `false` for the phase being committed — overrides the project-wide `commit_docs` for that phase only (reason `skipped_commit_docs_phase_false`) - No changes to commit (check with `git status --porcelain .planning/`) diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index 8b48b3e0f..6be2a27e4 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -76,6 +76,8 @@ if [[ "$INIT" == @file:* ]]; then INIT=$(cat "${INIT#@file:}"); fi **Auto-detection:** If `.planning/` is gitignored, `commit_docs` is automatically `false` regardless of config.json. This prevents git errors when users have `.planning/` in `.gitignore`. +**Per-phase override:** `phase_commit_docs.` (e.g. `phase_commit_docs.03`) overrides `commit_docs` for one phase only, and wins over both the explicit config value and gitignore auto-detection — see `docs/CONFIGURATION.md#per-phase-override-phase_commit_docs` for the full precedence chain and examples. + **Commit via CLI (handles checks automatically):** ```bash @@ -380,7 +382,7 @@ These can be set at top level or nested under `planning.*` (e.g., `"planning": { Several config fields affect each other or trigger special behavior: -1. **`commit_docs` auto-detection** -- When no explicit value is set in config.json and `.planning/` is in `.gitignore`, `commit_docs` automatically resolves to `false`. An explicit `true` or `false` in config always overrides auto-detection. +1. **`commit_docs` resolution chain** -- Four tiers, highest wins: (1) `phase_commit_docs.` for the phase being committed, (2) an explicit `commit_docs` (or `planning.commit_docs`) value in config.json, (3) `.gitignore` auto-detection (`.planning/` in `.gitignore` resolves to `false`), (4) the manifest default (`true`). Precedence: per-phase → explicit config → gitignore auto-detect → default. 2. **`branching_strategy` controls branch templates** -- The `phase_branch_template` and `milestone_branch_template` fields are only used when `branching_strategy` is set to `"phase"` or `"milestone"` respectively. When `branching_strategy` is `"none"`, all template fields are ignored. diff --git a/src/commands.cts b/src/commands.cts index 18da38a91..5a8c180d1 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -1063,6 +1063,79 @@ function detectPhaseNumberFromFiles(files: string[] | undefined): string | null return null; } +type CommitDocsSource = 'phase' | 'config' | 'gitignore' | 'default'; +interface CommitDocsResolution { + resolved: boolean; + source: CommitDocsSource; +} + +/** + * #3587: resolve the `phase_commit_docs.` override for `phaseNum` + * against `config['phase_commit_docs']` (a `{ "": boolean }` map, the + * same shape `agent_skills`/`features` use for their dynamic key families). + * Returns `undefined` — "no override applies" — when: no phase is known (B7), + * the map carries no entry for THIS phase (B5: no cross-phase leak), or the + * entry exists but is not a boolean (B6: never silently coerced). Both sides of + * the comparison route through `normalizePhaseName` so `3`, `03`, and `PROJ-03` + * all resolve to the same entry (B4/B9), reusing the single-owner phase-id + * normalizer rather than a second, looser string-equality rule. + */ +function resolvePhaseCommitDocsOverride(config: Record, phaseNum: string | null): boolean | undefined { + if (!phaseNum) return undefined; + const overrides = config['phase_commit_docs']; + if (!overrides || typeof overrides !== 'object' || Array.isArray(overrides)) return undefined; + const target = normalizePhaseName(phaseNum); + for (const [key, value] of Object.entries(overrides as Record)) { + if (normalizePhaseName(key) === target) { + return typeof value === 'boolean' ? value : undefined; + } + } + return undefined; +} + +/** + * #3587: the four-tier `commit_docs` precedence chain for a single commit — + * `phase_commit_docs.` (tier 1, resolved HERE because this call site + * is the one place that knows the phase — see 40-design.md "Rejected" §1: NOT + * inside `loadConfig`, which has no phase context and is called by nearly every + * command), then the pre-existing explicit `commit_docs` (tier 2), `.gitignore` + * auto-detect (tier 3), and manifest default (tier 4). Tiers 2-4 are byte-for- + * behaviour identical to the pre-#3587 inline checks (epic #2292 AC4): when no + * phase override applies, `resolved` matches exactly what those checks computed + * and `source` merely labels which of the three decided it. + * + * `isPlanningGitIgnored` is a thunk, not a plain boolean, so the pre-existing + * short-circuit is preserved byte-for-behaviour: the original inline checks + * only ever ran `isGitIgnored` (a real `git check-ignore` subprocess) when + * `commit_docs` was truthy, and a phase override or an explicit `commit_docs: + * false` must keep skipping that call entirely, not just its result. Passing + * a thunk also keeps this function pure and directly property-testable + * (test matrix F1) without spawning git. + */ +function resolveCommitDocsPolicy( + config: Record, + phaseNum: string | null, + isPlanningGitIgnored: () => boolean, +): CommitDocsResolution { + const phaseOverride = resolvePhaseCommitDocsOverride(config, phaseNum); + if (phaseOverride !== undefined) return { resolved: phaseOverride, source: 'phase' }; + if (!config['commit_docs']) return { resolved: false, source: 'config' }; + if (isPlanningGitIgnored()) return { resolved: false, source: 'gitignore' }; + return { resolved: true, source: 'default' }; +} + +// Reason string per commit_docs-resolution source, for the tier-1/tier-2 skip +// envelope below. `phase` gets its OWN reason (`skipped_commit_docs_phase_false`) +// rather than reusing `skipped_commit_docs_false` — telling a user "commit_docs +// is false" when their project setting is actually `true` would be actively +// misleading (design "Rejected" §3). `config` keeps the pre-existing string +// unchanged: `agents/gsd-executor.md` pattern-matches on it (D2). +const COMMIT_DOCS_SKIP_REASON: Record, string> = { + phase: 'skipped_commit_docs_phase_false', + config: 'skipped_commit_docs_false', + gitignore: 'skipped_gitignored', +}; + function cmdCommit(cwd: string, message: string | undefined, files: string[] | undefined, raw: boolean, amend: boolean, noVerify: boolean): void { if (!message && !amend) { error('commit message required'); @@ -1079,19 +1152,24 @@ function cmdCommit(cwd: string, message: string | undefined, files: string[] | u const config = loadConfig(cwd); - // Check commit_docs config + // Check commit_docs config — #3587: resolved through the tier 1 + // (phase_commit_docs.) → tier 2 (commit_docs) → tier 3 (.gitignore) + // → tier 4 (default) precedence chain; see resolveCommitDocsPolicy above. // `skipped: true` is explicit so agent prompts can match on a first-class // success signal rather than inferring "skip" from "committed is missing" // and improvising raw git fallbacks (#3678). - if (!config['commit_docs']) { - const result = { committed: false, skipped: true, hash: null, reason: 'skipped_commit_docs_false' }; - output(result, raw, 'skipped'); - return; - } - - // Check if .planning is gitignored - if (isGitIgnored(cwd, '.planning')) { - const result = { committed: false, skipped: true, hash: null, reason: 'skipped_gitignored' }; + const commitDocsPolicy = resolveCommitDocsPolicy( + config, + detectPhaseNumberFromFiles(files), + () => isGitIgnored(cwd, '.planning'), + ); + if (!commitDocsPolicy.resolved) { + const result = { + committed: false, + skipped: true, + hash: null, + reason: COMMIT_DOCS_SKIP_REASON[commitDocsPolicy.source as Exclude], + }; output(result, raw, 'skipped'); return; } @@ -2395,6 +2473,10 @@ export = { cmdResolveGranularity, cmdResolveExecution, cmdEffortSync, + detectPhaseNumberFromFiles, + resolvePhaseCommitDocsOverride, + resolveCommitDocsPolicy, + COMMIT_DOCS_SKIP_REASON, cmdCommit, cmdCommitToSubrepo, cmdPrSubrepo, diff --git a/src/config-loader.cts b/src/config-loader.cts index aa7d59fe4..3ddc2981f 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -862,6 +862,13 @@ function loadConfigResolved(cwd: string, options: Record = {}): fast_mode: (parsed['fast_mode']) || null, agent_skills: (parsed['agent_skills']) || {}, agent_skills_security: (parsed['agent_skills_security']) || null, + // #3587: phase_commit_docs. — a dynamic-key family shaped like + // agent_skills above (`{ "": boolean }`). Must be threaded here + // explicitly: `_baseConfig` is a hand-maintained allowlist, so a key that + // is only in config-schema.manifest.json's dynamicKeyPatterns (and not + // projected here) is silently dropped on read — the exact `features`-key + // failure mode this module's own A3 test guards against. + phase_commit_docs: (parsed['phase_commit_docs']) || {}, manager: (parsed['manager']) || {}, response_language: get('response_language') || null, claude_md_path: get('claude_md_path') || null, diff --git a/src/config.cts b/src/config.cts index 01a9b2eb5..2bb0e0196 100644 --- a/src/config.cts +++ b/src/config.cts @@ -727,7 +727,7 @@ function cmdConfigSet(cwd: string, keyPath: string | undefined, value: string | validateKnownConfigKeyPath(kp); if (!isValidConfigKey(kp, cwd)) { - error(`Unknown config key: "${kp}". Valid keys: ${[...VALID_CONFIG_KEYS].sort().join(', ')}, agent_skills., features.`, ERROR_REASON.CONFIG_INVALID_KEY); + error(`Unknown config key: "${kp}". Valid keys: ${[...VALID_CONFIG_KEYS].sort().join(', ')}, agent_skills., features., phase_commit_docs.`, ERROR_REASON.CONFIG_INVALID_KEY); } // Parse value (handle booleans, numbers, and JSON arrays/objects) diff --git a/tests/commit-docs-bypass.test.cjs b/tests/commit-docs-bypass.test.cjs index 4aed70652..b420f2434 100644 --- a/tests/commit-docs-bypass.test.cjs +++ b/tests/commit-docs-bypass.test.cjs @@ -813,3 +813,506 @@ describe('bug #3678 — executor must respect commit_docs:false', () => { }); }); } + +// ──────────────────────────────────────────────────────────────────────── +// Folded from tests/phase-commit-docs.test.cjs — #3587 (epic #2292 Phase 3) +// ──────────────────────────────────────────────────────────────────────── +{ + const { describe: __foldDescribe } = require('node:test'); + __foldDescribe("folded:phase-commit-docs (#3587, epic #2292 Phase 3)", () => { +'use strict'; + +/** + * #3587 (epic #2292 Phase 3) — per-phase `commit_docs` override. + * + * A tech lead can mark a single phase's docs to be committed (or suppressed) + * independently of the project-wide `commit_docs` setting, via the dynamic + * config key `phase_commit_docs.`. See + * `.gsd/phase/feat-3587-per-phase-commit-docs/40-design.md` for the + * precedence chain (phase > config > gitignore > default) and + * `50-test-matrix.md` for the matrix this file implements (A/B/C/D/F — + * E lives in this file's own 'E' describe block below). + * + * All assertions are on STRUCTURED values (`resolved`, `source`, `reason`), + * never on rendered/prose text (CONTRIBUTING.md — Prohibited: Raw Text + * Matching on Test Outputs). + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const fc = require('./helpers/fast-check-setup.cjs'); +const { cleanup, createTempGitProject } = require('./helpers.cjs'); +const { seedPhase } = require('./fixtures/index.cjs'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { GIT_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); + +const commands = require('../gsd-core/bin/lib/commands.cjs'); +const configCli = require('../gsd-core/bin/lib/config.cjs'); +const { loadConfigResolved, CONFIG_DEFAULTS } = require('../gsd-core/bin/lib/config-loader.cjs'); +const io = require('../gsd-core/bin/lib/io.cjs'); +const { PHASE_NUMBER_TOKEN_SOURCE } = require('../gsd-core/bin/lib/phase-id.cjs'); +const { DYNAMIC_KEY_PATTERNS } = require('../gsd-core/bin/lib/config-schema.cjs'); + +const { + detectPhaseNumberFromFiles, + resolvePhaseCommitDocsOverride, + resolveCommitDocsPolicy, + COMMIT_DOCS_SKIP_REASON, + cmdCommit, +} = commands; + +function git(args, cwd) { + return gitOrThrow(args, { cwd, timeoutMs: GIT_TIMEOUT_MS }); +} + +function writeConfig(tmpDir, config) { + fs.mkdirSync(path.join(tmpDir, '.planning'), { recursive: true }); + fs.writeFileSync(path.join(tmpDir, '.planning', 'config.json'), JSON.stringify(config, null, 2)); +} + +/** + * bin/lib/io.cjs's output()/error() write directly to the raw fd via + * fs.writeSync (bypassing console), so console-capture helpers cannot see + * them. Monkeypatch fs.writeSync, save/restore in a finally — mirrors + * tests/config-get-default.test.cjs's captureFdWrite. + */ +function captureFdWrite(fd, fn) { + const orig = fs.writeSync; + let captured = Buffer.alloc(0); + fs.writeSync = (writeFd, ...rest) => { + if (writeFd !== fd) return orig.call(fs, writeFd, ...rest); + const [data, offset = 0, length] = rest; + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset, offset + (length ?? data.length - offset)) + : Buffer.from(String(data), 'utf8'); + captured = Buffer.concat([captured, chunk]); + return chunk.length; + }; + try { + fn(); + } finally { + fs.writeSync = orig; + } + return captured.toString('utf-8'); +} + +function runCommit(tmpDir, message, files) { + const out = captureFdWrite(1, () => { + cmdCommit(tmpDir, message, files, false, false, false); + }); + return JSON.parse(out); +} + +/** Drives cmdConfigSet in-process, sentinel-exit style (mirrors + * tests/config-get-default.test.cjs's runExpectError) for the one negative + * (A4) case that must exercise error()'s process.exit(1) path. */ +function runConfigSetExpectError(tmpDir, keyPath, value) { + const origExit = process.exit; + const origWriteSync = fs.writeSync; + io.setJsonErrorMode(true); + let stderr = ''; + fs.writeSync = (fd, ...rest) => { + if (fd !== 2) return origWriteSync.call(fs, fd, ...rest); + const [data, offset = 0, length] = rest; + const chunk = Buffer.isBuffer(data) + ? data.subarray(offset, offset + (length ?? data.length - offset)).toString('utf8') + : String(data); + stderr += chunk; + return Buffer.byteLength(chunk); + }; + class _ExitSignal extends Error {} + process.exit = () => { throw new _ExitSignal('exit'); }; + try { + configCli.cmdConfigSet(tmpDir, keyPath, value, true); + } catch (e) { + if (!(e instanceof _ExitSignal)) throw e; + } finally { + process.exit = origExit; + fs.writeSync = origWriteSync; + io.setJsonErrorMode(false); + } + const parts = stderr.split('\n').filter(Boolean); + let payload = {}; + try { payload = JSON.parse(parts[parts.length - 1]); } catch { /* leave {} */ } + return payload; +} + +describe('#3587 per-phase commit_docs override', () => { + let tmpDir; + + beforeEach(() => { tmpDir = createTempGitProject(); }); + afterEach(() => cleanup(tmpDir)); + + // ── A: registration — the key must actually exist ────────────────────── + describe('A: registration', () => { + test('A1: perPhaseKeyRoundTripsThroughCli — config-set phase_commit_docs.03 true persists to config.json', () => { + captureFdWrite(1, () => configCli.cmdConfigSet(tmpDir, 'phase_commit_docs.03', 'true', true)); + const onDisk = JSON.parse(fs.readFileSync(path.join(tmpDir, '.planning', 'config.json'), 'utf-8')); + assert.deepStrictEqual(onDisk.phase_commit_docs, { '03': true }); + }); + + test('A2: perPhaseKeyReadsBack — config-get phase_commit_docs.03 returns the value', () => { + writeConfig(tmpDir, { phase_commit_docs: { '03': true } }); + const out = captureFdWrite(1, () => configCli.cmdConfigGet(tmpDir, 'phase_commit_docs.03', true, undefined)); + assert.strictEqual(out.trim(), 'true'); + }); + + test('A3: perPhaseKeySurvivesLoadConfig — present in schema but absent from defaults manifest is NOT silently dropped', () => { + writeConfig(tmpDir, { phase_commit_docs: { '03': true, '07': false } }); + const { config } = loadConfigResolved(tmpDir, { workstream: null }); + assert.deepStrictEqual(config.phase_commit_docs, { '03': true, '07': false }); + }); + + test('A3b: with no phase_commit_docs key at all, loadConfig projects an empty object (never undefined)', () => { + writeConfig(tmpDir, { commit_docs: true }); + const { config } = loadConfigResolved(tmpDir, { workstream: null }); + assert.deepStrictEqual(config.phase_commit_docs, {}); + }); + + test('A4: malformedPhaseKeyIsRejectedNotFatal — phase_commit_docs. is rejected as unknown, not fatal', () => { + const payload = runConfigSetExpectError(tmpDir, 'phase_commit_docs.not-a-phase-number', 'true'); + assert.strictEqual(payload.reason, io.ERROR_REASON.CONFIG_INVALID_KEY); + // Non-fatal to the process itself: no exception escaped runConfigSetExpectError, + // i.e. error() was reached and returned control via the sentinel exit only. + }); + + test('A5: configDefaultsProjectionParity — the CONFIG_DEFAULTS flat projection needs no entry, matching the agent_skills/features dynamic-key precedent', () => { + assert.strictEqual( + Object.prototype.hasOwnProperty.call(CONFIG_DEFAULTS, 'phase_commit_docs'), + false, + 'phase_commit_docs is an object-shaped dynamic-key family (like agent_skills/features); ' + + 'the flat CONFIG_DEFAULTS projection in config-loader.cjs carries no entry for either ' + + 'precedent, and phase_commit_docs is read purely from parsed config, never from defaults.phase_commit_docs', + ); + }); + }); + + // ── B: resolution — the precedence chain (pure function, no I/O) ─────── + describe('B: resolution', () => { + const noGitIgnore = () => false; + const gitIgnored = () => true; + + test('B1: perPhaseBeatsConfig — P=true, C=false resolves true, source phase', () => { + const config = { commit_docs: false, phase_commit_docs: { '03': true } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'phase' }); + }); + + test('B2: perPhaseSuppressesAgainstConfig — P=false, C=true resolves false, source phase', () => { + const config = { commit_docs: true, phase_commit_docs: { '03': false } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: false, source: 'phase' }); + }); + + test('B3: perPhaseBeatsGitignoreAutoDetect — P=true, G=true resolves true, source phase', () => { + const config = { commit_docs: true, phase_commit_docs: { '03': true } }; + const r = resolveCommitDocsPolicy(config, '03', gitIgnored); + assert.deepStrictEqual(r, { resolved: true, source: 'phase' }); + }); + + test('B4: phaseIdNormalizesAcrossForms — P set as "3", phase committed is "03"', () => { + const config = { commit_docs: false, phase_commit_docs: { '3': true } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'phase' }); + }); + + test('B4b: phaseIdNormalizesAcrossForms — a project-code-prefixed committed phase ("PROJ-03") hits the bare "03" entry', () => { + const config = { commit_docs: false, phase_commit_docs: { '03': true } }; + const r = resolveCommitDocsPolicy(config, 'PROJ-03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'phase' }); + }); + + test('B5: perPhaseDoesNotLeakAcrossPhases — P set for phase 04, committing phase 03 falls to tier 2', () => { + const config = { commit_docs: true, phase_commit_docs: { '04': false } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'default' }); + }); + + test('B6: nonBooleanPerPhaseValueIsNotCoerced — string "true" is not coerced, falls through to config', () => { + const config = { commit_docs: false, phase_commit_docs: { '03': 'true' } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: false, source: 'config' }); + }); + + test('B6b: nonBooleanPerPhaseValueIsNotCoerced — numeric 1 is not coerced', () => { + const config = { commit_docs: true, phase_commit_docs: { '03': 1 } }; + const r = resolveCommitDocsPolicy(config, '03', gitIgnored); + assert.deepStrictEqual(r, { resolved: false, source: 'gitignore' }); + }); + + test('B6c: nonBooleanPerPhaseValueIsNotCoerced — null is not coerced', () => { + const config = { commit_docs: true, phase_commit_docs: { '03': null } }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'default' }); + }); + + test('B7: noPhaseFallsToProjectSetting — commit names no phase-scoped file, tier 1 inapplicable', () => { + const config = { commit_docs: false, phase_commit_docs: { '03': true } }; + const r = resolveCommitDocsPolicy(config, null, noGitIgnore); + assert.deepStrictEqual(r, { resolved: false, source: 'config' }); + }); + + test('B8: projectCodeDigitDoesNotMisresolvePhase — a project code ending in a digit resolves phase 07, not 2 (#2539 shape)', () => { + const files = ['.planning/phases/PROJECT_V2-07-widgets/07-PLAN.md']; + const phaseNum = detectPhaseNumberFromFiles(files); + assert.strictEqual(phaseNum, 'PROJECT_V2-07'); + const config = { commit_docs: false, phase_commit_docs: { '07': true } }; + const r = resolveCommitDocsPolicy(config, phaseNum, noGitIgnore); + assert.deepStrictEqual(r, { resolved: true, source: 'phase' }, + 'must resolve against phase 07 (the real phase), never phase 2 (the digit inside "V2-")'); + }); + + test('B9: an override map that is not an object is inert, not thrown', () => { + const config = { commit_docs: false, phase_commit_docs: 'not-an-object' }; + const r = resolveCommitDocsPolicy(config, '03', noGitIgnore); + assert.deepStrictEqual(r, { resolved: false, source: 'config' }); + }); + + test('resolvePhaseCommitDocsOverride returns undefined for a null phase', () => { + assert.strictEqual(resolvePhaseCommitDocsOverride({ phase_commit_docs: { '03': true } }, null), undefined); + }); + + // Pin, not a bug (#3587 F3 — security review): detectPhaseNumberFromFiles is + // pre-existing, hardened, widely-used logic that this phase deliberately does + // not touch. It returns the phase of the FIRST matching path in `--files`, so + // a commit whose `--files` spans two phase directories resolves the tier-1 + // override against whichever phase appears first, not the majority. A pinned, + // named behavior is not a bug; an unpinned surprise is. See "Known limits" in + // `.gsd/phase/feat-3587-per-phase-commit-docs/40-design.md` and + // `docs/CONFIGURATION.md`'s per-phase section. + test('multiPhaseFilesResolvesAgainstFirstPhase — --files spanning two phase directories with DIFFERENT phase_commit_docs values resolves against the FIRST phase, not the majority', () => { + const files = [ + '.planning/phases/03-widgets/03-PLAN.md', + '.planning/phases/04-gadgets/04-PLAN.md', + ]; + const phaseNum = detectPhaseNumberFromFiles(files); + assert.strictEqual(phaseNum, '03', 'detectPhaseNumberFromFiles returns the phase of the first matching path'); + + const config = { commit_docs: false, phase_commit_docs: { '03': true, '04': false } }; + const r = resolveCommitDocsPolicy(config, phaseNum, () => false); + assert.deepStrictEqual( + r, + { resolved: true, source: 'phase' }, + 'the override resolves against phase 03 (first in --files) even though phase 04 disagrees', + ); + }); + }); + + // ── C: AC4 — regression, the release blocker ──────────────────────────── + // C1-C3 assert the byte-identical-to-`next` behavior using ONLY pre-existing + // surfaces (cmdCommit's envelope, loadConfigResolved) — no #3587 API — so + // this exact test code is valid evidence run against the unmodified tree + // too (see the dispatch's VERIFY step 1: these were run against `next` + // first, before the tier-1 change landed, and passed identically). + describe('C: AC4 regression (must hold identically with no per-phase key set)', () => { + test('C1: unsetPerPhaseIsByteIdenticalConfigFalse — no per-phase key, C=false skips with the pre-existing reason', () => { + writeConfig(tmpDir, { commit_docs: false }); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, '# State\n'); + const envelope = runCommit(tmpDir, 'docs(test): noop', ['.planning/STATE.md']); + assert.strictEqual(envelope.committed, false); + assert.strictEqual(envelope.skipped, true); + assert.strictEqual(envelope.reason, 'skipped_commit_docs_false'); + }); + + test('C2: unsetPerPhaseIsByteIdenticalGitignored — no per-phase key, C unset, G=true skips with the pre-existing reason', () => { + fs.writeFileSync(path.join(tmpDir, '.gitignore'), '.planning/\n'); + git(['add', '.gitignore'], tmpDir); + git(['commit', '-m', 'chore: gitignore'], tmpDir); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, '# State\n'); + const envelope = runCommit(tmpDir, 'docs(test): noop', ['.planning/STATE.md']); + assert.strictEqual(envelope.committed, false); + assert.strictEqual(envelope.skipped, true); + assert.strictEqual(envelope.reason, 'skipped_gitignored'); + }); + + test('C3: unsetPerPhaseIsByteIdenticalDefault — no per-phase key, C unset, G=false commits as today', () => { + writeConfig(tmpDir, {}); + const statePath = path.join(tmpDir, '.planning', 'STATE.md'); + fs.writeFileSync(statePath, '# State\n'); + const headBefore = git(['rev-parse', 'HEAD'], tmpDir).trim(); + const envelope = runCommit(tmpDir, 'docs(test): noop', ['.planning/STATE.md']); + assert.strictEqual(envelope.committed, true); + const headAfter = git(['rev-parse', 'HEAD'], tmpDir).trim(); + assert.notStrictEqual(headAfter, headBefore, 'a real commit must have been made'); + }); + + test('C5: loadConfigUnchangedWithoutPerPhaseKey — loadConfigResolved output is unchanged in shape/values with no per-phase key', () => { + writeConfig(tmpDir, { commit_docs: true, model_profile: 'balanced' }); + const { config, source, degraded, reason } = loadConfigResolved(tmpDir, { workstream: null }); + assert.strictEqual(config.commit_docs, true); + assert.strictEqual(config.model_profile, 'balanced'); + assert.strictEqual(source, 'root'); + assert.strictEqual(degraded, false); + assert.strictEqual(reason, 'resolved'); + // The universal seam gained a harmless empty-object projection (A3b) — + // additive, never a behavior change for a caller that never reads the key. + assert.deepStrictEqual(config.phase_commit_docs, {}); + }); + }); + + // ── D: envelope contract ───────────────────────────────────────────────── + describe('D: envelope contract', () => { + test('D1: perPhaseSuppressionHasOwnReason — per-phase suppression gets a reason distinct from skipped_commit_docs_false', () => { + writeConfig(tmpDir, { commit_docs: true, phase_commit_docs: { '03': false } }); + seedPhase(tmpDir, '03-widgets', { '03-PLAN.md': '# Plan\n' }); + const envelope = runCommit(tmpDir, 'docs(test): noop', ['.planning/phases/03-widgets/03-PLAN.md']); + assert.strictEqual(envelope.committed, false); + assert.strictEqual(envelope.skipped, true); + assert.strictEqual(envelope.reason, COMMIT_DOCS_SKIP_REASON.phase); + assert.notStrictEqual(envelope.reason, 'skipped_commit_docs_false'); + }); + + test('D2: existingReasonStringsUnchanged — the two pre-existing reason strings are unchanged (agents/gsd-executor.md pattern-matches on them)', () => { + assert.strictEqual(COMMIT_DOCS_SKIP_REASON.config, 'skipped_commit_docs_false'); + assert.strictEqual(COMMIT_DOCS_SKIP_REASON.gitignore, 'skipped_gitignored'); + }); + + test('D3: perPhaseEnableActuallyCommits — per-phase override ENABLES a commit under project commit_docs:false; the file lands', () => { + writeConfig(tmpDir, { commit_docs: false, phase_commit_docs: { '03': true } }); + seedPhase(tmpDir, '03-widgets', { '03-PLAN.md': '# Plan\n' }); + const headBefore = git(['rev-parse', 'HEAD'], tmpDir).trim(); + const envelope = runCommit(tmpDir, 'docs(test): plan', ['.planning/phases/03-widgets/03-PLAN.md']); + assert.strictEqual(envelope.committed, true); + const headAfter = git(['rev-parse', 'HEAD'], tmpDir).trim(); + assert.notStrictEqual(headAfter, headBefore); + const committedFiles = git(['show', '--name-only', '--pretty=format:', 'HEAD'], tmpDir) + .split('\n').map((s) => s.trim()).filter(Boolean); + assert.ok( + committedFiles.includes('.planning/phases/03-widgets/03-PLAN.md'), + `expected the per-phase-enabled file to land in the commit; got: ${committedFiles.join(', ')}`, + ); + }); + }); + + // ── E: parity — the divergence the design named ───────────────────────── + // config-schema.manifest.json is hand-maintained JSON, so its + // phase_commit_docs. pattern is necessarily a SECOND, hand-copied + // statement of the canonical PHASE_NUMBER_TOKEN_SOURCE grammar + // (src/phase-id.cts, #2128) — this repo's "Generative Fix Divergence" class + // (CLAUDE.md). BEHAVIORAL over a shared shape list — not a source-grep of + // the regex text — because the point is that the two surfaces agree on + // INPUTS, not that they share characters. + describe('E: manifest/canonical-grammar parity for phase_commit_docs.', () => { + const manifestEntry = DYNAMIC_KEY_PATTERNS.find((p) => p.topLevel === 'phase_commit_docs'); + + // No 'i' flag: PHASE_NUMBER_TOKEN_SOURCE's own default reading is + // case-sensitive (uppercase-only `[A-Z]` letter suffix), and the + // manifest's regex (recompiled via `new RegExp(p.source)` in + // src/configuration.cts, no flags) is built from the same case-sensitive + // source string. Parity must hold at that same case sensitivity, or a + // divergence in flags would go undetected. + const canonicalPhaseIdRe = new RegExp(`^${PHASE_NUMBER_TOKEN_SOURCE}$`); + const canonicalAccepts = (shape) => canonicalPhaseIdRe.test(shape); + const manifestAccepts = (shape) => manifestEntry.test(`phase_commit_docs.${shape}`); + + test('the dynamicKeyPatterns entry for phase_commit_docs exists', () => { + assert.ok(manifestEntry, 'config-schema.manifest.json must carry a phase_commit_docs dynamicKeyPatterns entry'); + }); + + const acceptedShapes = [ + '0', '1', '01', '003', '12A', '1.2', '12.34', '1.2.3', '999', '12A.3', '0.0.0', + ]; + + describe('E1: manifestPhasePatternMatchesCanonicalGrammar', () => { + for (const shape of acceptedShapes) { + test(`accepted shape "${shape}" is accepted by both surfaces`, () => { + assert.strictEqual(canonicalAccepts(shape), true, `test fixture bug: canonical grammar must accept "${shape}"`); + assert.strictEqual(manifestAccepts(shape), true, `manifest pattern rejected canonical-accepted shape "${shape}"`); + }); + } + }); + + const rejectedShapes = [ + '', 'a', '1a', '01-a', '.1', '1.', '1..2', 'PROJ-01', '01 02', '1_2', '12AB', '1.a', '-1', '1-2', 'AB', + ]; + + describe('E2: manifestPhasePatternRejectsSameShapes', () => { + for (const shape of rejectedShapes) { + test(`rejected shape "${JSON.stringify(shape)}" is rejected by both surfaces`, () => { + assert.strictEqual(canonicalAccepts(shape), false, `test fixture bug: canonical grammar must reject "${shape}"`); + assert.strictEqual(manifestAccepts(shape), false, `manifest pattern accepted canonical-rejected shape "${shape}"`); + }); + } + }); + + // Bonus robustness: over a bounded, seeded fuzz of arbitrary short + // strings, the two surfaces must never disagree — catches a shape + // neither hand-picked list happened to cover. + test('property: manifest and canonical grammar agree on arbitrary short strings', () => { + fc.assert( + fc.property( + fc.string({ maxLength: 8 }), + (shape) => { + assert.strictEqual( + manifestAccepts(shape), + canonicalAccepts(shape), + `disagreement on shape ${JSON.stringify(shape)}`, + ); + }, + ), + ); + }); + }); + + // ── F: property — resolution is total, and source is honest ──────────── + describe('F: property', () => { + const phaseArb = fc.constantFrom(null, '03', '3', '04', 'PROJ-03', '12A', '3.2'); + const overrideValueArb = fc.oneof( + fc.boolean(), + fc.constant('true'), + fc.constant(1), + fc.constant(null), + fc.string({ maxLength: 5 }), + ); + const overridesArb = fc.dictionary( + fc.constantFrom('03', '3', '04', '12A', '3.2', 'not-a-phase'), + overrideValueArb, + { maxKeys: 4 }, + ); + const commitDocsArb = fc.boolean(); + const gitIgnoredArb = fc.boolean(); + + test('F1: resolutionIsTotalAndSourceIsHonest — resolution is total (boolean + a valid source), and the reported source names the tier that actually decided', () => { + fc.assert( + fc.property( + phaseArb, overridesArb, commitDocsArb, gitIgnoredArb, + (phaseNum, overrides, commitDocs, gitIgnored) => { + const config = { commit_docs: commitDocs, phase_commit_docs: overrides }; + const r = resolveCommitDocsPolicy(config, phaseNum, () => gitIgnored); + + // Totality: always a boolean resolution and a known source. + assert.strictEqual(typeof r.resolved, 'boolean'); + assert.ok(['phase', 'config', 'gitignore', 'default'].includes(r.source)); + + // Honesty: recompute independently what SHOULD have decided it, and + // require the reported source to match. + const override = resolvePhaseCommitDocsOverride(config, phaseNum); + if (override !== undefined) { + assert.strictEqual(r.source, 'phase'); + assert.strictEqual(r.resolved, override); + return; + } + if (!commitDocs) { + assert.strictEqual(r.source, 'config'); + assert.strictEqual(r.resolved, false); + return; + } + if (gitIgnored) { + assert.strictEqual(r.source, 'gitignore'); + assert.strictEqual(r.resolved, false); + return; + } + assert.strictEqual(r.source, 'default'); + assert.strictEqual(r.resolved, true); + }, + ), + ); + }); + }); +}); + }); +}