diff --git a/.changeset/gallant-orcas-caper.md b/.changeset/gallant-orcas-caper.md new file mode 100644 index 000000000..1199be8b0 --- /dev/null +++ b/.changeset/gallant-orcas-caper.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3720 +--- +**`/gsd-pr-branch` gains a strict mode that keeps every planning artifact out of the PR branch** — set `planning.pr_strict: true` and the generated PR branch carries no `.planning/` path at all, structural files included, so a project can version its planning tree locally (keeping executor worktrees and `/gsd-undo` working) while publishing none of it. Defaults to `false`, which reproduces the previous classification and preservation exactly. (#2971) diff --git a/.changeset/gentle-otters-click.md b/.changeset/gentle-otters-click.md new file mode 100644 index 000000000..30f254ad1 --- /dev/null +++ b/.changeset/gentle-otters-click.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3720 +--- +**`/gsd-pr-branch` no longer deletes the base branch's planning files or silently drops commits** — the generated PR branch used to stage a deletion for any `.planning/` path the target branch already tracked, and a second commit touching the same planning file aborted the cherry-pick with "untracked working tree files would be overwritten", dropping that commit and every one after it. The filter now forces excluded paths back to what the target branch tracks in both the index and the working tree. Verification also asserts against the active filter mode instead of an unconditional zero, so a correct default-mode run that preserved `STATE.md` no longer reports itself as failed. (#2971) diff --git a/CONTEXT.md b/CONTEXT.md index be033eb23..1a9f4065b 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -233,6 +233,9 @@ Module owning agent-presence resolution and verification, extracted from the Cor ### Config Loader Module Module owning project configuration loading: reads `.planning/config.json`, merges built-in defaults (`CONFIG_DEFAULTS`/`CANONICAL_CONFIG_DEFAULTS`), normalizes legacy keys, applies the active-workstream overlay, validates against the config schema, and warns on unknown keys/profile overrides. Primary interface: `loadConfigResolved(cwd, options) → ConfigResolution { config, source, degraded }` (provenance-aware, ADR-1411 P2 / #1415) — `source` ∈ `'workstream' | 'root' | 'builtin-defaults' | 'global-defaults'`; `degraded:true` when a workstream was requested but its config.json was absent (fell back to root config). `loadConfig(cwd, options) → Record` is the back-compat thin wrapper over `loadConfigResolved` (byte-identical result). Resolution is **caller-anchored, not loader-anchored**: `loadConfigResolved` resolves `cwd` as-is (no walk-up), so `loadConfig` stays byte-identical for its callers; callers that need cwd-drift tolerance (e.g. `cmdAgentSkills`) anchor to the project root via `findProjectRoot` (Project-Root Resolution Module) *before* calling `loadConfigResolved`. Helper exports: `_deepMergeConfig`, `isGitIgnored`, `_warnUnknownProfileOverrides`. Depends only on leaf modules (`configuration`, `config-schema`, `planning-workspace`, `shell-command-projection`, `core-utils`, `model-catalog`) — no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2e (#885) as the prerequisite for the model-resolver extraction (the resolvers call `loadConfig`); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/config-loader.cjs` (generated from `src/config-loader.cts`). +### Planning Publication Gate (`planning.pr_strict`) +The seam deciding whether `.planning/` artifacts reach the REMOTE — distinct from the Planning Commit Gate below, which decides whether they reach git at all. `planning.pr_strict` (boolean, manifest default `false`) resolves through the same `loadConfigResolved` chain as its `planning.*` siblings (explicit top-level `pr_strict`, then the `planning.pr_strict` alias, then the manifest default), and is additionally registered in `SCHEMA_DEFAULTS` (`src/config.cts`) so `query config-get planning.pr_strict` answers `false` for an absent key rather than `Key not found` — `gsd-core/workflows/pr-branch.md` reads it with a plain `config-get` and must not special-case a missing key. It selects between two filter modes in that workflow: default preserves the five structural planning files plus `milestones/**` and drops nine transient subdirectories; strict drops every `.planning/` path and includes a commit only when it touches at least one file outside `.planning/`. The two path lists are declared ONCE in the workflow (`TRANSIENT_DIRS`, `STRUCTURAL_RE`) and both the un-stage step and the verification assertion are derived from them, because the prior shape declared them twice and the two steps disagreed by construction — `verify` asserted zero `.planning/` paths while `create_pr_branch` was specified to preserve five, so a correct run reported itself as failed on every phase (#2971). The two gates are independent but not orthogonal in effect: `pr_strict` is inert when `commit_docs` is `false`, since nothing is committed for the PR-branch filter to remove. Source of truth: `gsd-core/workflows/pr-branch.md`; key registered in `gsd-core/bin/shared/config-{defaults,schema}.manifest.json`. + ### 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', '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. diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 523671a7d..a6f6a72d2 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -1724,6 +1724,10 @@ Create a clean PR branch by filtering out `.planning/` commits. **Purpose:** Reviewers see only code changes, not GSD planning artifacts. +**Prerequisites:** Clean working tree — uncommitted changes are rejected before the PR branch is created. + +**Filter mode:** Set by [`planning.pr_strict`](CONFIGURATION.md#planning-settings). Default (`false`) keeps structural planning state — `STATE.md`, `ROADMAP.md`, `MILESTONES.md`, `PROJECT.md`, `REQUIREMENTS.md`, `milestones/**` — and drops the transient subdirectories. Strict (`true`) drops every `.planning/` path. The active mode is printed in the run header and in the verification summary. + ```bash /gsd-pr-branch # Filter against main /gsd-pr-branch develop # Filter against develop diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 1377d6edd..7bf9a2f56 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -509,6 +509,7 @@ The following combinations of `mode`, `granularity`, `model_profile`, and workfl | Setting | Type | Default | Description | |---------|------|---------|-------------| | `planning.commit_docs` | boolean | `true` | Whether `.planning/` files are committed to git | +| `planning.pr_strict` | boolean | `false` | Filter mode for [`/gsd-pr-branch`](COMMANDS.md#gsd-pr-branch). `false` — the generated PR branch keeps structural planning state (`STATE.md`, `ROADMAP.md`, `MILESTONES.md`, `PROJECT.md`, `REQUIREMENTS.md`, `milestones/**`) and drops the transient subdirectories. `true` — every `.planning/` path is dropped, structural files included, and a commit is carried over only when it touches at least one file outside `.planning/`. Applies to the root repository's PR branch only; `planning.sub_repos` companion branches are unaffected | | `planning.search_gitignored` | boolean | `false` | Add `--no-ignore` to broad searches to include `.planning/` | | `planning.sub_repos` | array of strings | `[]` | Paths of nested sub-repos relative to the project root. When set, GSD-aware tooling scopes phase-lookup, path-resolution, and commit operations per sub-repo instead of treating the outer repo as a monorepo | @@ -622,6 +623,25 @@ The prompt injection guard hook (`gsd-prompt-guard.js`) is always active and can When `planning.commit_docs` is `false` and `.planning/` is listed in `.gitignore`, GSD treats planning artifacts as local-only. `planning.search_gitignored: true` ensures broad searches still include the `.planning/` directory in this configuration. See [Keep planning docs out of a shared repo](how-to/keep-planning-docs-private.md) for the full setup, including untracking files git is already tracking. +#### Two ways to keep planning private, and what each costs + +"Private planning" is really two different questions — *is it in git at all?* and *does it reach the remote?* — and GSD answers them with two different keys. + +`planning.commit_docs: false` answers the first by keeping `.planning/` out of git entirely. That has a cost most projects discover late: **parallel executor worktrees stop working.** A worktree is checked out from a *commit*, so a `.planning/` directory that is untracked or ignored simply does not exist inside it, and the executor has no `PLAN.md` to read. Local-only planning and parallel execution are mutually exclusive under this setting. Untracked planning also has no git history, so `/gsd-undo` and revert paths have nothing to restore. + +`planning.pr_strict: true` answers the second instead, and leaves the first alone. Planning artifacts are committed normally on your working branch — so worktrees find them, and history exists — while `/gsd-pr-branch` guarantees that none of them reach the branch you publish. The guarantee is enforced by the command rather than by remembering never to push the working branch. + +Pick by which question you are actually asking: + +| You want | Setting | What you give up | +|---|---|---| +| Planning never enters git | `planning.commit_docs: false` | Parallel executor worktrees; planning git history | +| Planning is versioned locally but never published | `planning.commit_docs: true` + `planning.pr_strict: true` | Nothing — but the working branch itself must not be pushed; publish the generated `-pr` branch | + +The two are independent keys and can be set together, but `pr_strict` is inert when `commit_docs` is `false`: with nothing committed, there is nothing for the PR-branch filter to remove. + +See [Publish PRs without planning artifacts](how-to/publish-prs-without-planning-artifacts.md) for the setup. + ### `commit_docs` Pre-Commit Guard (opt-in) `planning.commit_docs: false` only stops GSD's own `gsd-tools commit`/`gsd-tools state` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 29dcdc7a3..f7ae84385 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -1330,6 +1330,14 @@ That third-party dependence is a real trade-off, held honestly rather than paper - REQ-PRBRANCH-01: System MUST identify commits that only modify `.planning/` files - REQ-PRBRANCH-02: System MUST create a new branch with planning commits filtered out - REQ-PRBRANCH-03: Code changes MUST be preserved exactly as committed +- REQ-PRBRANCH-04: System MUST NOT delete a `.planning/` path the target branch already tracks +- REQ-PRBRANCH-05: Verification MUST assert against the active filter mode's contract, not an unconditional zero + +**Filter modes.** `planning.pr_strict` selects what "filtered" means. The default mode treats `.planning/` as two populations: structural state that belongs in review (`STATE.md`, `ROADMAP.md`, `MILESTONES.md`, `PROJECT.md`, `REQUIREMENTS.md`, `milestones/**`) and transient per-phase artifacts that do not (`phases/`, `quick/`, `research/`, `threads/`, `todos/`, `debug/`, `seeds/`, `codebase/`, `ui-reviews/`). Strict mode collapses that distinction: nothing under `.planning/` reaches the PR branch, and a commit is carried over only when it touches at least one file outside `.planning/`. + +Strict mode exists because the two ways to keep planning private are not equivalent. Turning off `planning.commit_docs` keeps `.planning/` out of git, which also takes parallel executor worktrees with it — a worktree is checked out from a commit, so an untracked planning tree is simply absent inside it and the executor has no `PLAN.md` to read. Strict mode leaves planning committed, so worktrees and revert paths keep working, and moves the guarantee to the publication boundary instead. See [Publish PRs without planning artifacts](how-to/publish-prs-without-planning-artifacts.md). + +Both modes filter by forcing the excluded paths back to whatever the target branch already tracks, in the index *and* the working tree. Un-staging alone would record a deletion of any planning file the target branch carries, and would leave the picked file untracked on disk, where it makes a later cherry-pick of the same path abort. --- diff --git a/docs/README.md b/docs/README.md index 5d7b7db94..9b8540482 100644 --- a/docs/README.md +++ b/docs/README.md @@ -32,6 +32,7 @@ Language versions: [English](README.md) · [Português (pt-BR)](pt-BR/README.md) - [Read the statusline freshness marker](how-to/read-the-statusline-freshness-marker.md) — turn on `state ~N commits back`, and tell "STATE.md is fresh" apart from "freshness could not be established" - [Consume the planning snapshot](how-to/consume-the-planning-snapshot.md) — read `planning inspect` from a dashboard or harness, and tell "nothing to report" apart from "could not look" - [Keep planning docs out of a shared repo](how-to/keep-planning-docs-private.md) — make `.planning/` local-only, including untracking files git already tracks (the step `.gitignore` alone cannot do) +- [Publish PRs without planning artifacts](how-to/publish-prs-without-planning-artifacts.md) — keep `.planning/` committed locally, so worktrees and `/gsd-undo` keep working, while `planning.pr_strict` keeps every planning path out of the branch you push - [Plan a phase](how-to/plan-a-phase.md) — run research, decompose work, and verify plan quality - [Verify a dependency-compatibility claim](how-to/verify-a-dependency-compatibility-claim.md) — act on a compatibility claim the researcher left `[ASSUMED]`, and tell "nothing declared" apart from "a constraint is declared" and "the lookup failed" - [Execute a phase](how-to/execute-a-phase.md) — run plans in parallel waves with fresh-context subagents diff --git a/docs/how-to/keep-planning-docs-private.md b/docs/how-to/keep-planning-docs-private.md index ab9756d7d..c6fff12d3 100644 --- a/docs/how-to/keep-planning-docs-private.md +++ b/docs/how-to/keep-planning-docs-private.md @@ -180,8 +180,21 @@ Windows note: the hook runs under Git Bash, same as any other git hook. GSD's ow matrix is Linux-only, so this specific behavior is verified on Linux/macOS plus code review, not by an automated Windows run. +## If you need parallel executors, use the other posture instead + +Everything above keeps `.planning/` out of git, and that has one consequence worth knowing before +you commit to it: **parallel executor worktrees stop working.** A worktree is checked out from a +commit, so an untracked or ignored `.planning/` does not exist inside it and the executor has no +`PLAN.md` to read. Untracked planning also has no git history, so `/gsd-undo` and revert paths have +nothing to restore. + +If what you actually want is "planning is versioned locally, but never reaches the remote", leave +`commit_docs` on and set `planning.pr_strict: true` instead — see +[Publish PRs without planning artifacts](publish-prs-without-planning-artifacts.md). + ## Related +- [Publish PRs without planning artifacts](publish-prs-without-planning-artifacts.md) - [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/docs/how-to/publish-prs-without-planning-artifacts.md b/docs/how-to/publish-prs-without-planning-artifacts.md new file mode 100644 index 000000000..f72cf02e3 --- /dev/null +++ b/docs/how-to/publish-prs-without-planning-artifacts.md @@ -0,0 +1,113 @@ +# Publish PRs without planning artifacts + +You want GSD's `.planning/` tree versioned in git on your own branch — so parallel executors can +read their plans and `/gsd-undo` has something to restore — while the pull request your team +reviews contains none of it. + +This is the opposite trade to +[Keep planning docs out of a shared repo](keep-planning-docs-private.md), which removes planning +from git entirely and loses worktree isolation in the process. Use this guide when you want the +history and want the remote clean. + +Five steps, and the fourth is the one that decides whether the guarantee actually holds. + +## 1. Keep planning artifacts committed + +```bash +gsd-tools config-set planning.commit_docs true +``` + +This is already the default. Set it explicitly if someone previously turned it off, or if +`.planning/` is listed in `.gitignore` — a gitignored `.planning/` auto-resolves `commit_docs` to +`false` no matter what `config.json` says, and `planning.pr_strict` cannot filter what was never +committed. + +If `.planning/` is currently in `.gitignore`, remove that line before continuing. + +## 2. Turn on strict PR filtering + +```bash +gsd-tools config-set planning.pr_strict true +``` + +Confirm it took: + +```bash +gsd-tools query config-get planning.pr_strict --raw +``` + +`true` means every `.planning/` path will be filtered out of the generated PR branch — +`STATE.md` and `ROADMAP.md` included, not only the per-phase artifacts. + +## 3. Do the work normally + +Nothing about the phase loop changes. `/gsd-plan-phase` and `/gsd-execute-phase` commit planning +artifacts exactly as they always have, executor worktrees find their `PLAN.md`, and your working +branch carries the full planning history. + +## 4. Generate the PR branch — and push *that* branch + +```bash +/gsd-pr-branch +``` + +The command creates `-pr` from the target branch and rebuilds the history without any +`.planning/` path. **Push the generated `-pr` branch, never your working branch** — the working +branch is where the planning history lives, and it is the one thing that must not reach the remote. + +```bash +git push origin -pr +gh pr create --base main --head -pr +``` + +Requirements before it will run: + +- **A clean working tree.** Uncommitted changes are rejected up front. Commit or stash first. +- **Commits ahead of the target.** With none, the command exits without creating a branch. + +## 5. Read the verification summary + +The run ends with a summary. Two lines carry the guarantee: + +``` +Mode: strict +Planning paths in diff: 0 (allowed 0, forbidden 0 — must be 0) +``` + +`forbidden` is the number that matters. In strict mode it counts every `.planning/` path that +survived into the branch, and it must be `0`. `allowed` is always `0` in strict mode — there is no +allowed population. + +### What each outcome means + +| Summary line | Meaning | What to do | +|---|---|---| +| `Mode: strict` … `forbidden 0` | The guarantee held. | Push the `-pr` branch. | +| `Mode: default` | `planning.pr_strict` did not resolve to `true`. | Re-run step 2; the key is read from the project's `.planning/config.json`, so check you are in the right project root. | +| `forbidden` greater than `0` | The filter did not remove everything it promised. | Do **not** push. This is a bug — report it with the listed paths. | +| `Conflict outside the .planning/ filter` | A real content conflict between your commits and the target branch. | Resolve it on your working branch, then re-run `/gsd-pr-branch`. | +| `Working tree has uncommitted changes` | Step 4's precondition failed. | Commit or stash, then re-run. | +| `No commits ahead of ` | There is nothing to filter. | Not an error — you have not committed anything yet, or the target is wrong. | + +An `ℹ️` advisory listing `.planning/` paths appears only in **default** mode, naming paths that are +neither transient nor structural (`config.json`, `intel/`, `workstreams/`) and therefore kept. Strict +mode removes those too, so the advisory never appears — if you see it, you are not in strict mode. + +## Verify it for yourself + +Before trusting the setup on a real review, confirm the generated branch is clean: + +```bash +git diff --name-only main..-pr | grep '^\.planning/' +``` + +No output is the expected result. Any output means step 2 did not take effect, or the run reported +a non-zero `forbidden` count you should not push past. + +## Related + +- [Keep planning docs out of a shared repo](keep-planning-docs-private.md) — the other posture: + planning never enters git, at the cost of parallel executor worktrees +- [Planning settings reference](../CONFIGURATION.md#planning-settings) — `planning.pr_strict`, + `planning.commit_docs`, and the rest of the `planning.*` keys +- [`/gsd-pr-branch` reference](../COMMANDS.md#gsd-pr-branch) diff --git a/docs/ja-JP/CONFIGURATION.md b/docs/ja-JP/CONFIGURATION.md index cb6c3a416..e36639092 100644 --- a/docs/ja-JP/CONFIGURATION.md +++ b/docs/ja-JP/CONFIGURATION.md @@ -121,6 +121,7 @@ GSD はプロジェクト設定を `.planning/config.json` に保存します。 | 設定 | 型 | デフォルト | 説明 | |------|-----|-----------|------| | `planning.commit_docs` | boolean | `true` | `.planning/` ファイルを git にコミットするかどうか | +| `planning.pr_strict` | boolean | `false` | `/gsd-pr-branch` のフィルタモード。`false` は STATE.md、ROADMAP.md、MILESTONES.md、PROJECT.md、REQUIREMENTS.md、milestones/ などの構造的なプランニング状態を PR ブランチに残す。`true` はすべての `.planning/` パスを削除する | | `planning.search_gitignored` | boolean | `false` | `.planning/` を含めるために広範な検索に `--no-ignore` を追加 | ### 自動検出 diff --git a/docs/ko-KR/CONFIGURATION.md b/docs/ko-KR/CONFIGURATION.md index 152d647f8..ce3930032 100644 --- a/docs/ko-KR/CONFIGURATION.md +++ b/docs/ko-KR/CONFIGURATION.md @@ -121,6 +121,7 @@ GSD는 프로젝트 설정을 `.planning/config.json`에 저장합니다. `/gsd- | 설정 | 타입 | 기본값 | 설명 | |------|------|--------|------| | `planning.commit_docs` | boolean | `true` | `.planning/` 파일을 git에 커밋할지 여부 | +| `planning.pr_strict` | boolean | `false` | `/gsd-pr-branch`의 필터 모드. `false`는 STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, REQUIREMENTS.md, milestones/ 등 구조적 플래닝 상태를 PR 브랜치에 유지하고, `true`는 모든 `.planning/` 경로를 제거함 | | `planning.search_gitignored` | boolean | `false` | 광범위한 검색에 `--no-ignore`를 추가하여 `.planning/`을 포함 | ### 자동 감지 diff --git a/docs/pt-BR/CONFIGURATION.md b/docs/pt-BR/CONFIGURATION.md index 4bcddad32..8fa86b8ad 100644 --- a/docs/pt-BR/CONFIGURATION.md +++ b/docs/pt-BR/CONFIGURATION.md @@ -352,6 +352,7 @@ As seguintes combinações de `mode`, `granularity`, `model_profile` e controles | Configuração | Tipo | Padrão | Descrição | |---------|------|---------|-------------| | `planning.commit_docs` | boolean | `true` | Define se os arquivos de `.planning/` são commitados no git | +| `planning.pr_strict` | boolean | `false` | Modo de filtro para `/gsd-pr-branch`. `false` mantém o estado estrutural de planejamento (STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, REQUIREMENTS.md, milestones/) no branch de PR; `true` remove todos os caminhos de `.planning/` | | `planning.search_gitignored` | boolean | `false` | Adiciona `--no-ignore` em buscas amplas para incluir `.planning/` | | `planning.sub_repos` | array de strings | `[]` | Caminhos de sub-repositórios aninhados relativos à raiz do projeto. Quando definido, as ferramentas com reconhecimento de GSD limitam a busca de fase, resolução de caminho e operações de commit por sub-repo em vez de tratar o repositório externo como um monorepo | diff --git a/docs/zh-CN/CONFIGURATION.md b/docs/zh-CN/CONFIGURATION.md index 57efc8ab5..6f3cfb918 100644 --- a/docs/zh-CN/CONFIGURATION.md +++ b/docs/zh-CN/CONFIGURATION.md @@ -352,6 +352,7 @@ API 密钥字段接受字符串值（密钥本身）。也可以设置为哨兵 | 设置 | 类型 | 默认值 | 描述 | |---------|------|---------|-------------| | `planning.commit_docs` | boolean | `true` | `.planning/` 文件是否提交到 git | +| `planning.pr_strict` | boolean | `false` | `/gsd-pr-branch` 的过滤模式。`false` 会在 PR 分支中保留 STATE.md、ROADMAP.md、MILESTONES.md、PROJECT.md、REQUIREMENTS.md、milestones/ 等结构性规划状态；`true` 会删除所有 `.planning/` 路径 | | `planning.search_gitignored` | boolean | `false` | 向大范围搜索添加 `--no-ignore` 以包含 `.planning/` | | `planning.sub_repos` | string 数组 | `[]` | 相对于项目根目录的嵌套子仓库路径。设置后，GSD 感知工具按子仓库划定阶段查找、路径解析和提交操作的范围，而非将外层仓库视为 monorepo | diff --git a/gsd-core/bin/shared/config-defaults.manifest.json b/gsd-core/bin/shared/config-defaults.manifest.json index 26bbb0b8c..b276e2573 100644 --- a/gsd-core/bin/shared/config-defaults.manifest.json +++ b/gsd-core/bin/shared/config-defaults.manifest.json @@ -61,6 +61,7 @@ }, "planning": { "commit_docs": true, + "pr_strict": false, "search_gitignored": false, "sub_repos": [], "granularity": "standard" diff --git a/gsd-core/bin/shared/config-schema.manifest.json b/gsd-core/bin/shared/config-schema.manifest.json index e2939aae1..bc4588411 100644 --- a/gsd-core/bin/shared/config-schema.manifest.json +++ b/gsd-core/bin/shared/config-schema.manifest.json @@ -49,6 +49,7 @@ "git.milestone_branch_template", "git.quick_branch_template", "planning.commit_docs", + "planning.pr_strict", "planning.search_gitignored", "planning.sub_repos", "review.default_reviewers", diff --git a/gsd-core/references/planning-config.md b/gsd-core/references/planning-config.md index 5362777dc..5644834c5 100644 --- a/gsd-core/references/planning-config.md +++ b/gsd-core/references/planning-config.md @@ -6,6 +6,7 @@ Configuration options for `.planning/` directory behavior. ```json "planning": { "commit_docs": true, + "pr_strict": false, "search_gitignored": false }, "git": { @@ -27,6 +28,7 @@ Configuration options for `.planning/` directory behavior. | Option | Default | Description | |--------|---------|-------------| | `commit_docs` | `true` | Whether to commit planning artifacts to git | +| `pr_strict` | `false` | Filter mode for `/gsd:pr-branch`. `false` keeps structural planning state (STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, REQUIREMENTS.md, milestones/) in the PR branch; `true` drops every `.planning/` path | | `search_gitignored` | `false` | Add `--no-ignore` to broad rg searches | | `git.branching_strategy` | `"none"` | Git branching approach: `"none"`, `"phase"`, or `"milestone"` | | `git.base_branch` | `null` (auto-detect) | Target branch for PRs and merges (e.g. `"master"`, `"develop"`). When `null`, auto-detects from `git symbolic-ref refs/remotes/origin/HEAD`, falling back to `"main"`. | diff --git a/gsd-core/workflows/pr-branch.md b/gsd-core/workflows/pr-branch.md index 623881d84..bc5163114 100644 --- a/gsd-core/workflows/pr-branch.md +++ b/gsd-core/workflows/pr-branch.md @@ -1,8 +1,17 @@ -Create a clean branch for pull requests by filtering out transient .planning/ commits. -The PR branch contains only code changes and structural planning state — reviewers -don't see GSD transient artifacts (PLAN.md, SUMMARY.md, CONTEXT.md, RESEARCH.md, etc.) -but milestone archives, STATE.md, ROADMAP.md, and PROJECT.md changes are preserved. +Create a clean branch for pull requests by filtering .planning/ paths out of the +cherry-picked history. Two modes, selected by the `planning.pr_strict` config key: + +- **default** (`planning.pr_strict: false`) — the PR branch contains code changes and + structural planning state. Reviewers don't see GSD transient artifacts (PLAN.md, + SUMMARY.md, CONTEXT.md, RESEARCH.md, etc.), but milestone archives, STATE.md, + ROADMAP.md, and PROJECT.md changes are preserved. +- **strict** (`planning.pr_strict: true`) — *every* .planning/ path is filtered out, + structural files included. This is what makes `planning.commit_docs: true` safe for a + project that versions its planning tree locally but publishes none of it: planning state + keeps real git history (so `/gsd:undo` and revert paths have something to restore) and + executor worktrees still find their PLAN.md, while the public PR carries nothing from + `.planning/`. Uses git cherry-pick with path filtering to rebuild a clean history. @@ -22,6 +31,7 @@ TARGET=${1:-$(gsd_run query git.base-branch)} Check preconditions: - Must be on a feature branch (not main/master) - Must have commits ahead of target +- Working tree must be clean ```bash AHEAD=$(git rev-list --count "$TARGET".."$CURRENT_BRANCH" 2>/dev/null) @@ -29,6 +39,26 @@ if [ "$AHEAD" = "0" ]; then echo "No commits ahead of $TARGET — nothing to filter." exit 0 fi + +# The filter below removes files from the index AND the working tree before each +# commit lands, and this command switches branches underneath the user's own +# checkout. An uncommitted edit to a tracked file would be destroyed by that, and +# git cherry-pick refuses to run against a dirty tree anyway — so fail here, where +# the message is legible, rather than midway through the cherry-pick loop. +DIRTY=$(git status --porcelain --untracked-files=no) +if [ -n "$DIRTY" ]; then + echo "Working tree has uncommitted changes — commit or stash them first:" >&2 + echo "$DIRTY" >&2 + exit 1 +fi +``` + +Resolve the filter mode from config. A non-zero exit or an unset key means the default +mode; only the literal string `true` selects strict. + +```bash +PR_STRICT=$(gsd_run query config-get planning.pr_strict --raw 2>/dev/null) +if [ "$PR_STRICT" = "true" ]; then PR_MODE="strict"; else PR_MODE="default"; PR_STRICT="false"; fi ``` Display: @@ -40,6 +70,7 @@ Display: Branch: {CURRENT_BRANCH} Target: {TARGET} Commits: {AHEAD} ahead +Mode: {PR_MODE} (planning.pr_strict={PR_STRICT}) ``` @@ -207,47 +238,62 @@ Classify commits: git log --oneline "$TARGET".."$CURRENT_BRANCH" --no-merges ``` -**Structural planning files** — always preserved (repository planning state): -- `.planning/STATE.md` -- `.planning/ROADMAP.md` -- `.planning/MILESTONES.md` -- `.planning/PROJECT.md` -- `.planning/REQUIREMENTS.md` -- `.planning/milestones/**` +**Canonical path declarations.** These two lines are the single source of truth for the +whole command. `create_pr_branch` derives *which paths it removes* from them, and `verify` +derives *which paths must not appear* from the same two lines — so the two steps cannot +disagree about what the filter promised. Declare them exactly once; do not restate either +list anywhere else in this file. -**Transient planning files** — excluded from PR branch (reviewer noise): -- `.planning/phases/**` (PLAN.md, SUMMARY.md, CONTEXT.md, RESEARCH.md, etc.) -- `.planning/quick/**` -- `.planning/research/**` -- `.planning/threads/**` -- `.planning/todos/**` -- `.planning/debug/**` -- `.planning/seeds/**` -- `.planning/codebase/**` -- `.planning/ui-reviews/**` +```bash +# Transient planning subdirectories — reviewer noise (PLAN.md, SUMMARY.md, CONTEXT.md, +# RESEARCH.md, and friends). Filtered out in BOTH modes. +TRANSIENT_DIRS="phases quick research threads todos debug seeds codebase ui-reviews" + +# Structural planning files — repository planning state. Preserved in default mode, +# filtered out in strict mode. Anchored on both alternatives so `.planning/STATEX.md` +# and `.planning/STATE.md.bak` are NOT treated as structural. +STRUCTURAL_RE="^\.planning/(STATE|ROADMAP|MILESTONES|PROJECT|REQUIREMENTS)\.md$|^\.planning/milestones/" +``` + +Derive the mode's two projections — `FILTER_PATHS` (what `create_pr_branch` removes from +each cherry-picked commit) and `FORBIDDEN_RE` (what `verify` asserts is absent): + +```bash +if [ "$PR_STRICT" = "true" ]; then + FILTER_PATHS=".planning/" + FORBIDDEN_RE="^\.planning/" +else + FILTER_PATHS=$(for d in $TRANSIENT_DIRS; do printf '.planning/%s/ ' "$d"; done) + FORBIDDEN_RE="^\.planning/($(echo "$TRANSIENT_DIRS" | tr ' ' '|'))/" +fi +``` For each commit, check what it touches: ```bash # For each commit hash FILES=$(git diff-tree --no-commit-id --name-only -r $HASH) -NON_PLANNING=$(echo "$FILES" | grep -v "^\.planning/" | wc -l) -STRUCTURAL=$(echo "$FILES" | grep -E "^\.planning/(STATE|ROADMAP|MILESTONES|PROJECT|REQUIREMENTS)\.md|^\.planning/milestones/" | wc -l) -TRANSIENT_ONLY=$(echo "$FILES" | grep "^\.planning/" | grep -vE "^\.planning/(STATE|ROADMAP|MILESTONES|PROJECT|REQUIREMENTS)\.md|^\.planning/milestones/" | wc -l) +NON_PLANNING=$(echo "$FILES" | grep -c -v "^\.planning/" || true) +STRUCTURAL=$(echo "$FILES" | grep -Ec "$STRUCTURAL_RE" || true) ``` Classify: -- **Code commits**: Touch at least one non-.planning/ file → INCLUDE -- **Structural planning commits**: Touch only structural .planning/ files (STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, REQUIREMENTS.md, milestones/**) → INCLUDE -- **Transient planning commits**: Touch only transient .planning/ files (phases/, quick/, research/, etc.) → EXCLUDE -- **Mixed commits**: Touch code + any planning files → INCLUDE (transient planning changes come along; acceptable in mixed context) +- **Code commits**: touch at least one non-`.planning/` file → INCLUDE (both modes) +- **Mixed commits**: touch code + any planning files → INCLUDE (both modes; the planning + paths are filtered out by `create_pr_branch`, not the commit) +- **Structural planning commits**: touch only structural `.planning/` files → INCLUDE in + **default** mode; **EXCLUDE** in strict mode, which has no structural carve-out +- **Transient planning commits**: touch only `.planning/` paths that are not structural → + EXCLUDE (both modes) + +In strict mode this collapses to a single rule: `NON_PLANNING > 0` → INCLUDE, else EXCLUDE. Display analysis: ``` -Commits to include: {N} (code changes + structural planning) -Commits to exclude: {N} (transient planning-only) -Mixed commits: {N} (code + planning — included) -Structural planning commits: {N} (STATE/ROADMAP/milestone updates — included) +Commits to include: {N} (code changes{, + structural planning — default mode only}) +Commits to exclude: {N} (planning-only) +Mixed commits: {N} (code + planning — included, planning paths filtered) +Structural planning commits: {N} ({included|excluded — strict mode}) ``` @@ -259,18 +305,69 @@ PR_BRANCH="${CURRENT_BRANCH}-pr" git checkout -b "$PR_BRANCH" "$TARGET" ``` -Cherry-pick code commits and structural planning commits (in order): +Cherry-pick the included commits, in order, filtering `$FILTER_PATHS` out of each one. + +The filter forces every filtered path back to **exactly what the PR branch's HEAD already +has**, in both the index and the working tree. That is stricter than simply un-staging, and +both halves matter: + +- `git rm -r -f --ignore-unmatch` clears the index entry (including an unmerged one) and + removes the file the pick just wrote. It only ever touches paths that are in the index, so + a genuinely untracked planning file of the user's is never harmed. +- `git checkout HEAD --` then restores whatever the target branch legitimately tracks at + those paths. **Without this, un-staging a path the target branch already tracks records a + DELETION** — the generated PR would remove the base branch's planning files. In strict mode + that would be the base's entire `.planning/` tree. + +Leaving the filtered file behind in the working tree is not an option either: a later commit +touching the same planning path makes `git cherry-pick` abort with *"untracked working tree +files would be overwritten by merge"*, and every remaining commit is silently dropped. ```bash -for HASH in $CODE_AND_STRUCTURAL_COMMITS; do - git cherry-pick "$HASH" --no-commit - # Remove only transient .planning/ subdirectories that came along in mixed commits. - # DO NOT remove structural files (STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, - # REQUIREMENTS.md, milestones/) — these must survive into the PR branch. - for dir in phases quick research threads todos debug seeds codebase ui-reviews; do - git rm -r --cached ".planning/$dir/" 2>/dev/null || true +for HASH in $INCLUDED_COMMITS; do + # A modify/delete conflict on a filtered path is EXPECTED and is resolved below — the + # filtered path is absent from HEAD by construction. Do not treat it as a failure here. + git cherry-pick --no-commit "$HASH" || true + + for P in $FILTER_PATHS; do + git rm -r -f -q --ignore-unmatch -- "$P" 2>/dev/null || true + git checkout HEAD -- "$P" 2>/dev/null || true done - git commit -C "$HASH" + + # Anything still unmerged is a REAL conflict, outside the filter. Halt — do not + # improvise a resolution and do not continue, which would drop the rest of the queue. + # Unwind first: this loop runs in the user's own checkout, so exiting mid-sequence + # would strand them on a half-built branch with cherry-pick state still live. + if [ -n "$(git diff --name-only --diff-filter=U)" ]; then + echo "Conflict outside the .planning/ filter while picking $HASH:" >&2 + git diff --name-only --diff-filter=U >&2 + # Order matters. `--quit` drops the sequencer state but leaves the unmerged index + # in place, and an unmerged index makes `git checkout` refuse — so reset first. + # $PR_BRANCH is disposable and every commit on it was cherry-picked, and the + # clean-tree precondition guarantees the user had nothing uncommitted, so a hard + # reset here cannot destroy anything of theirs. + git cherry-pick --quit 2>/dev/null || true + git reset -q --hard HEAD + if git checkout -q "$CURRENT_BRANCH"; then + git branch -q -D "$PR_BRANCH" 2>/dev/null || true + echo "Restored $CURRENT_BRANCH and removed the partial $PR_BRANCH." >&2 + else + # Never claim a restore that did not happen — say exactly where they are. + echo "Could not return to $CURRENT_BRANCH; you are still on $PR_BRANCH." >&2 + echo "Run: git checkout $CURRENT_BRANCH && git branch -D $PR_BRANCH" >&2 + fi + echo "Resolve the conflict against $TARGET, then re-run /gsd:pr-branch." >&2 + exit 1 + fi + + # Nothing left after filtering (possible when a pick's only surviving content was + # planning state): clear the sequencer rather than failing on an empty commit. + if git diff --cached --quiet; then + git cherry-pick --quit 2>/dev/null || true + continue + fi + + git commit -q -C "$HASH" done ``` @@ -281,20 +378,36 @@ git checkout "$CURRENT_BRANCH" +Assert against the **active mode's** contract — `$FORBIDDEN_RE`, the same declaration +`create_pr_branch` filtered on. Counting every `.planning/` path unconditionally would +contradict default mode, which is specified to preserve structural files: a correct run +would report itself as failed on every phase that touched STATE.md, which is every phase. + ```bash -# Verify no .planning/ files in PR branch -PLANNING_FILES=$(git diff --name-only "$TARGET".."$PR_BRANCH" | grep "^\.planning/" | wc -l) -TOTAL_FILES=$(git diff --name-only "$TARGET".."$PR_BRANCH" | wc -l) +DIFF_PATHS=$(git diff --name-only "$TARGET".."$PR_BRANCH") +FORBIDDEN=$(echo "$DIFF_PATHS" | grep -Ec "$FORBIDDEN_RE" || true) +PLANNING_TOTAL=$(echo "$DIFF_PATHS" | grep -c "^\.planning/" || true) +ALLOWED=$((PLANNING_TOTAL - FORBIDDEN)) +TOTAL_FILES=$(echo "$DIFF_PATHS" | grep -c . || true) PR_COMMITS=$(git rev-list --count "$TARGET".."$PR_BRANCH") + +# Default mode preserves anything under .planning/ that is neither transient nor +# structural — config.json, intel/, workstreams/. That is deliberate and unchanged, but it +# must not be silent: report it so the user can choose strict mode knowingly. +OTHER=$(echo "$DIFF_PATHS" | grep "^\.planning/" | grep -Ev "$FORBIDDEN_RE" | grep -Ev "$STRUCTURAL_RE" || true) ``` +`$FORBIDDEN` is the pass/fail number — it must be `0`. A non-zero value means the filter +did not do what this mode promised; report it and do not tell the user to push. + Display results: ``` ✅ PR branch created: {PR_BRANCH} Original: {AHEAD} commits, {ORIGINAL_FILES} files PR branch: {PR_COMMITS} commits, {TOTAL_FILES} files -Planning files: {PLANNING_FILES} (should be 0) +Mode: {PR_MODE} +Planning paths in diff: {PLANNING_TOTAL} (allowed {ALLOWED}, forbidden {FORBIDDEN} — must be 0) Next steps: git push origin {PR_BRANCH} @@ -302,14 +415,25 @@ Next steps: Or use /gsd:ship to create the PR automatically. ``` + +When `$OTHER` is non-empty (default mode only — strict forbids all of it), append: +``` +ℹ️ These .planning/ paths are neither transient nor structural, so default mode keeps them: +{OTHER} + Set `planning.pr_strict: true` to keep every .planning/ path out of the PR branch. +``` +- [ ] Working tree was clean before the PR branch was created - [ ] PR branch created from target - [ ] Planning-only commits excluded -- [ ] No .planning/ files in PR branch diff +- [ ] Zero paths matching the active mode's `$FORBIDDEN_RE` in the PR branch diff — + strict: no `.planning/` path at all; default: none from `$TRANSIENT_DIRS` +- [ ] No `.planning/` path the target branch already tracked was deleted +- [ ] Every included commit landed — none dropped by a failed cherry-pick - [ ] Commit messages preserved from original - [ ] User shown next steps diff --git a/src/config-loader.cts b/src/config-loader.cts index 3ddc2981f..83c2daae7 100644 --- a/src/config-loader.cts +++ b/src/config-loader.cts @@ -79,6 +79,7 @@ function _resetFederatedRegistryForTests(): void { * - git.* → flat git keys (branching_strategy, templates) * - workflow.* → flat names (research, verifier, …) * - planning.sub_repos → sub_repos + * - planning.pr_strict → pr_strict * - planning.commit_docs / search_gitignored → top-level flat keys */ @@ -115,6 +116,7 @@ const CONFIG_DEFAULTS = { exa_search: _getConfigDefault('exa_search'), text_mode: _getNestedConfigDefault('workflow', 'text_mode'), sub_repos: _getNestedConfigDefault('planning', 'sub_repos'), + pr_strict: _getNestedConfigDefault('planning', 'pr_strict'), resolve_model_ids: _getConfigDefault('resolve_model_ids'), context_window: _getConfigDefault('context_window'), phase_naming: _getConfigDefault('phase_naming'), @@ -844,6 +846,7 @@ function loadConfigResolved(cwd: string, options: Record = {}): _auto_chain_active: get('_auto_chain_active', { section: 'workflow', field: '_auto_chain_active' }) ?? false, mode: get('mode') ?? 'interactive', sub_repos: get('sub_repos', { section: 'planning', field: 'sub_repos' }) ?? defaults.sub_repos, + pr_strict: get('pr_strict', { section: 'planning', field: 'pr_strict' }) ?? defaults.pr_strict, resolve_model_ids: get('resolve_model_ids') ?? defaults.resolve_model_ids, context_window: get('context_window') ?? defaults.context_window, phase_naming: get('phase_naming') ?? defaults.phase_naming, diff --git a/src/config.cts b/src/config.cts index 2bb0e0196..639c66f05 100644 --- a/src/config.cts +++ b/src/config.cts @@ -97,6 +97,10 @@ const SCHEMA_DEFAULTS: Record = { // Derived from the defaults manifest rather than restated, so the manifest // stays the single source of truth for the smart-zone budget (#2630). 'workflow.smart_zone_tokens': CONFIG_DEFAULTS.smart_zone_tokens, + // #2971: /gsd:pr-branch reads this key directly; an absent key must resolve to the + // manifest default rather than "Key not found". Derived from the defaults manifest so + // the manifest stays the single source of truth. + 'planning.pr_strict': CONFIG_DEFAULTS.pr_strict, }; /** diff --git a/tests/emitted-drift-acks/2971-pr-branch-strict-planning-filter.json b/tests/emitted-drift-acks/2971-pr-branch-strict-planning-filter.json new file mode 100644 index 000000000..4364c266f --- /dev/null +++ b/tests/emitted-drift-acks/2971-pr-branch-strict-planning-filter.json @@ -0,0 +1,8 @@ +{ + "version": 1, + "paths": { + "pr-branch.md": { + "reason": "#2971: +5729 bytes vs next. Adds the planning.pr_strict filter mode and repairs two verified defects in the same cherry-pick loop. The growth is four things, none of them prose padding: (1) the canonical TRANSIENT_DIRS/STRUCTURAL_RE declarations plus their per-mode FILTER_PATHS/FORBIDDEN_RE projections, which replace two duplicated hardcoded lists so create_pr_branch and verify can no longer disagree about what the filter promised; (2) a rewritten create_pr_branch loop — the old `git rm -r --cached` staged a DELETION of any .planning/ path the target branch already tracked, and left the picked file untracked on disk so a later commit touching that path aborted with \"untracked working tree files would be overwritten\" and every remaining commit was dropped, both reproduced against real git before the fix; the replacement forces filtered paths back to HEAD in index and worktree, halts on a conflict outside the filter instead of improvising, and skips a commit left empty by filtering; (3) a mode-derived verify step plus the advisory line naming the .planning/ paths default mode deliberately keeps, so correcting the assertion does not trade a permanently-wrong signal for silence; (4) a clean-working-tree precondition, required because the corrected filter now removes files from the working tree. Comments carry the why for each, because every one of them is a place a future edit would otherwise reintroduce the defect." + } + } +} diff --git a/tests/helpers/pr-branch-filter.cjs b/tests/helpers/pr-branch-filter.cjs new file mode 100644 index 000000000..ddf707aef --- /dev/null +++ b/tests/helpers/pr-branch-filter.cjs @@ -0,0 +1,191 @@ +'use strict'; + +/** + * Parsing seam for `tests/pr-branch-planning-filter.test.cjs` against + * `/gsd-pr-branch`'s planning-path filter. + * + * The workflow (`gsd-core/workflows/pr-branch.md`) is the single source of + * truth for which `.planning/` subdirectories are "transient" (excluded from + * a PR branch in non-strict mode), for the "structural" regex that carves + * out an exception even in that non-strict filter, AND for the + * `create_pr_branch` cherry-pick/filter recipe itself. Rather than hardcoding + * a second copy of that path list / regex / recipe here — which could + * silently drift from the shipped workflow the moment any of them changes — + * this module PARSES the workflow's own `TRANSIENT_DIRS="..."` and + * `STRUCTURAL_RE="..."` shell declarations, and EXTRACTS its + * `for HASH in $INCLUDED_COMMITS` cherry-pick loop, out of its markdown text. + * The path/regex parsing mirrors, in JS, the same derivation the workflow's + * shell performs on them; the loop extraction returns the shell body + * verbatim so callers execute the actual shipped recipe rather than a + * hand-copied mirror of it. If the workflow's declarations or recipe move, + * get renamed, are duplicated, or go missing, this module throws rather than + * silently falling back to a stale/guessed value — the workflow is the + * single source of truth for behavior as well as for data. + * + * ## Known limits + * + * This is a targeted regex extraction of two specific `NAME="..."` shell + * assignments, not a shell parser: it assumes each declaration appears + * verbatim, unquoted-value-free (no embedded `"` — shell wouldn't allow that + * unescaped inside a double-quoted assignment either), and on its own line. + * It does not evaluate shell variable expansion, comments, or conditionals + * around the declaration — only the literal assigned string. + */ + +const fs = require('fs'); +const path = require('path'); +const { escapeRegex: escapeRe } = require('../../gsd-core/bin/lib/pattern.cjs'); + +const WORKFLOW_PATH = path.join(__dirname, '..', '..', 'gsd-core', 'workflows', 'pr-branch.md'); + +const TRANSIENT_DIRS_RE = /^\s*TRANSIENT_DIRS="([^"]*)"/gm; +const STRUCTURAL_RE_RE = /^\s*STRUCTURAL_RE="([^"]*)"/gm; + +// Collects every match of `re` (a global regex) against `text`, returning +// the captured group-1 values in order. `re.lastIndex` is reset first so +// repeated calls against the same shared regex object are safe. +const collectMatches = (re, text) => { + re.lastIndex = 0; + const out = []; + let m; + while ((m = re.exec(text)) !== null) { + out.push(m[1]); + } + return out; +}; + +const parseWorkflow = (text) => { + if (typeof text !== 'string') { + throw new Error('parseWorkflow: expected the workflow text as a string'); + } + + const transientMatches = collectMatches(TRANSIENT_DIRS_RE, text); + if (transientMatches.length === 0) { + throw new Error('pr-branch.md: no TRANSIENT_DIRS declaration'); + } + if (transientMatches.length > 1) { + throw new Error(`pr-branch.md: TRANSIENT_DIRS declared ${transientMatches.length} times — the filter must have exactly one canonical declaration`); + } + + const structuralMatches = collectMatches(STRUCTURAL_RE_RE, text); + if (structuralMatches.length === 0) { + throw new Error('pr-branch.md: no STRUCTURAL_RE declaration'); + } + if (structuralMatches.length > 1) { + throw new Error(`pr-branch.md: STRUCTURAL_RE declared ${structuralMatches.length} times — the filter must have exactly one canonical declaration`); + } + + const transientDirs = transientMatches[0].split(/\s+/).filter((s) => s.length > 0); + const structuralRe = structuralMatches[0]; + + return { transientDirs, structuralRe }; +}; + +const readWorkflow = () => parseWorkflow(fs.readFileSync(WORKFLOW_PATH, 'utf-8')); + +const BASH_FENCE_OPEN_RE = /^```bash\s*$/; +const BASH_FENCE_CLOSE_RE = /^```\s*$/; +const PICK_LOOP_MARKER = 'for HASH in $INCLUDED_COMMITS'; + +// Scans `text` for fenced ```bash blocks and returns the verbatim body +// (fence markers stripped, lines rejoined with '\n') of the single block +// that contains `create_pr_branch`'s cherry-pick loop. Throws if the marker +// is found in zero or more-than-one bash block, since the recipe must have +// exactly one canonical form. +const extractPickLoop = (text) => { + if (typeof text !== 'string') { + throw new Error('extractPickLoop: expected the workflow text as a string'); + } + + const lines = text.split('\n'); + const matches = []; + let i = 0; + while (i < lines.length) { + if (BASH_FENCE_OPEN_RE.test(lines[i])) { + const bodyLines = []; + let j = i + 1; + while (j < lines.length && !BASH_FENCE_CLOSE_RE.test(lines[j])) { + bodyLines.push(lines[j]); + j += 1; + } + const body = bodyLines.join('\n'); + if (body.includes(PICK_LOOP_MARKER)) { + matches.push(body); + } + i = j + 1; + } else { + i += 1; + } + } + + if (matches.length === 0) { + throw new Error('pr-branch.md: no create_pr_branch cherry-pick loop found (expected a bash block containing "for HASH in $INCLUDED_COMMITS")'); + } + if (matches.length > 1) { + throw new Error(`pr-branch.md: cherry-pick loop found in ${matches.length} bash blocks — the recipe must have exactly one canonical form`); + } + + return matches[0]; +}; + +const normalizePaths = (input) => { + const raw = typeof input === 'string' ? input.split('\n') : input; + return raw.map((s) => s.replace(/\r$/, '')).filter((s) => s.length > 0); +}; + +const forbiddenRegex = ({ strict, transientDirs }) => { + if (strict === true) { + return /^\.planning\//; + } + if (!transientDirs || transientDirs.length === 0) { + return /(?!)/; + } + const alt = transientDirs.map((d) => escapeRe(d)).join('|'); + return new RegExp(`^\\.planning/(${alt})/`); +}; + +const forbiddenPaths = (files, opts) => { + const re = forbiddenRegex(opts); + return normalizePaths(files).filter((p) => re.test(p)); +}; + +const structuralPaths = (files, { structuralRe }) => { + const re = new RegExp(structuralRe); + return normalizePaths(files).filter((p) => re.test(p)); +}; + +const classifyCommit = (files, opts) => { + const paths = normalizePaths(files); + if (paths.length === 0) return 'exclude'; + + const nonPlanning = paths.filter((p) => !/^\.planning\//.test(p)); + if (nonPlanning.length > 0) return 'include'; + + if (opts.strict) return 'exclude'; + + if (structuralPaths(paths, opts).length > 0) return 'include'; + + return 'exclude'; +}; + +const otherPlanningPaths = (files, opts) => { + const paths = normalizePaths(files); + const forbidden = new Set(forbiddenPaths(paths, opts)); + const structural = new Set(structuralPaths(paths, opts)); + return paths.filter( + (p) => /^\.planning\//.test(p) && !forbidden.has(p) && !structural.has(p), + ); +}; + +module.exports = { + WORKFLOW_PATH, + parseWorkflow, + readWorkflow, + extractPickLoop, + normalizePaths, + forbiddenRegex, + forbiddenPaths, + structuralPaths, + classifyCommit, + otherPlanningPaths, +}; diff --git a/tests/pr-branch-planning-filter.test.cjs b/tests/pr-branch-planning-filter.test.cjs new file mode 100644 index 000000000..587b422dd --- /dev/null +++ b/tests/pr-branch-planning-filter.test.cjs @@ -0,0 +1,843 @@ +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +/** + * Failing-first suite for issue #2971 — `/gsd-pr-branch`'s `.planning/` path + * filter grows a `planning.pr_strict` config switch and its `verify` step + * stops contradicting the `create_pr_branch` step it is supposed to validate. + * + * `tests/helpers/pr-branch-filter.cjs` is the seam this file drives: it + * PARSES the two shell declarations (`TRANSIENT_DIRS="..."` and + * `STRUCTURAL_RE="..."`) straight out of `gsd-core/workflows/pr-branch.md`, + * so the shipped workflow — not a second, hand-copied list in this test file + * — is the single source of truth for which `.planning/` subdirectories are + * "transient" and which structural files are carved out of the default + * filter. Nothing below hardcodes a transient-dir or structural-file list; + * every layer either builds an inline fixture text and feeds it through + * `parseWorkflow`, or reads the real shipped file through `readWorkflow`. + * + * Six layers, in order: + * L1 — pure predicates over an inline workflow-text fixture (no disk, no + * subprocess): `classifyCommit` / `forbiddenPaths` / `structuralPaths` + * / `otherPlanningPaths` against hand-picked and boundary path lists. + * L2 — the real `create_pr_branch` cherry-pick-and-filter recipe, executed + * against real git fixtures via `sh -c`. Pins two defects reproduced + * empirically against today's shipped recipe (accidental deletion of + * untouched base `.planning/` content; a second commit silently + * dropped by an "untracked working tree files would be overwritten" + * cherry-pick abort) and proves both filter modes end-to-end. + * L3 — `planning.pr_strict` registration through the real CLI/config + * surfaces: `config-get`/`config-set`, the schema manifest, the + * defaults manifest, and `loadConfig`'s flat-root-alias behavior. + * L4 — the issue's load-bearing worktree claim, EXECUTED against a real + * `git worktree add`, not merely asserted. + * L5 — fast-check properties over the pure predicates, seeded and bounded. + * L6 — a drift guard over the shipped workflow's prose: this is the one + * layer allowed to source-grep, because the `.md` text IS the runtime + * contract GSD loads (CONTRIBUTING.md's source-text-is-the-product + * exception; the `local/no-source-grep` ESLint rule only covers + * `.cjs`/`.js`/`.ts`, never `.md`). + * + * Per CLAUDE.md's failing-first regression protocol, most of L1-L6 is + * EXPECTED to fail until the #2971 implementation lands `planning.pr_strict` + * in the config manifests/loader and rewrites `pr-branch.md`'s + * `create_pr_branch`/`verify` steps. No assertion here is softened, + * try/caught, or skipped to paper over that — a thrown `parseWorkflow` / + * `readWorkflow` error inside an individual `test()` body is a truthful, + * individually-attributable failure, not a suite crash, because every call + * to `readWorkflow()` happens inside a `test()` body rather than at + * `describe()`-collection time. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); +const { execFileSync } = require('child_process'); +const fc = require('./helpers/fast-check-setup.cjs'); + +const { + cleanup, createTempProject, runGsdTools, readFileNormalized, +} = require('./helpers.cjs'); +const { gitOrThrow, GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/git-fixture.cjs'); +const { + WORKFLOW_PATH, + parseWorkflow, + readWorkflow, + extractPickLoop, + forbiddenRegex, + forbiddenPaths, + structuralPaths, + classifyCommit, + otherPlanningPaths, +} = require('./helpers/pr-branch-filter.cjs'); +const { loadConfig } = require('../gsd-core/bin/lib/config-loader.cjs'); + +const REPO_ROOT = path.resolve(__dirname, '..'); +const CONFIG_DEFAULTS_MANIFEST_PATH = path.join( + REPO_ROOT, 'gsd-core', 'bin', 'shared', 'config-defaults.manifest.json', +); +const CONFIG_SCHEMA_MANIFEST_PATH = path.join( + REPO_ROOT, 'gsd-core', 'bin', 'shared', 'config-schema.manifest.json', +); + +// ── Shared git-fixture primitives (L2 + L4) ──────────────────────────────── + +function git(args, cwd) { + return gitOrThrow(args, { cwd, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +function initRepo(dir) { + fs.mkdirSync(dir, { recursive: true }); + git(['-c', 'init.defaultBranch=main', 'init', '-q'], dir); + git(['config', 'user.email', 'test@example.com'], dir); + git(['config', 'user.name', 'Test User'], dir); + git(['config', 'commit.gpgsign', 'false'], dir); +} + +function writeFile(dir, relPath, content) { + const full = path.join(dir, relPath); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); +} + +function commitAll(dir, message) { + git(['add', '-A'], dir); + git(['commit', '-q', '-m', message], dir); +} + +function uniqueTmpPath(prefix) { + return path.join(os.tmpdir(), `${prefix}-${Date.now()}-${Math.random().toString(36).slice(2)}`); +} + +describe('#2971 — pr-branch.md planning.pr_strict filter (failing-first)', () => { + // ── L1: pure predicates (inline text fixtures, no disk) ───────────────── + describe('L1: pure predicates over an inline workflow-text fixture', () => { + const FIXTURE_TEXT = [ + 'TRANSIENT_DIRS="phases quick research threads todos debug seeds codebase ui-reviews"', + 'STRUCTURAL_RE="^\\.planning/(STATE|ROADMAP|MILESTONES|PROJECT|REQUIREMENTS)\\.md$|^\\.planning/milestones/"', + ].join('\n'); + const fixture = parseWorkflow(FIXTURE_TEXT); + const strictOpts = { strict: true, transientDirs: fixture.transientDirs, structuralRe: fixture.structuralRe }; + const defaultOpts = { strict: false, transientDirs: fixture.transientDirs, structuralRe: fixture.structuralRe }; + + test('1: [src/a.ts] includes in both modes', () => { + assert.strictEqual(classifyCommit(['src/a.ts'], strictOpts), 'include'); + assert.strictEqual(classifyCommit(['src/a.ts'], defaultOpts), 'include'); + }); + + test('2: [src/a.ts, .planning/phases/PLAN.md] includes in both modes', () => { + const files = ['src/a.ts', '.planning/phases/PLAN.md']; + assert.strictEqual(classifyCommit(files, strictOpts), 'include'); + assert.strictEqual(classifyCommit(files, defaultOpts), 'include'); + }); + + test('3: [.planning/phases/PLAN.md] excludes in both modes', () => { + const files = ['.planning/phases/PLAN.md']; + assert.strictEqual(classifyCommit(files, strictOpts), 'exclude'); + assert.strictEqual(classifyCommit(files, defaultOpts), 'exclude'); + }); + + test('4: [.planning/STATE.md] includes default, excludes strict', () => { + const files = ['.planning/STATE.md']; + assert.strictEqual(classifyCommit(files, defaultOpts), 'include'); + assert.strictEqual(classifyCommit(files, strictOpts), 'exclude'); + }); + + test('5: [.planning/milestones/m1/x.md] includes default, excludes strict', () => { + const files = ['.planning/milestones/m1/x.md']; + assert.strictEqual(classifyCommit(files, defaultOpts), 'include'); + assert.strictEqual(classifyCommit(files, strictOpts), 'exclude'); + }); + + test('6: [.planning/config.json] excludes in both modes (third bucket)', () => { + const files = ['.planning/config.json']; + assert.strictEqual(classifyCommit(files, defaultOpts), 'exclude'); + assert.strictEqual(classifyCommit(files, strictOpts), 'exclude'); + }); + + test('7: [] excludes in both modes', () => { + assert.strictEqual(classifyCommit([], defaultOpts), 'exclude'); + assert.strictEqual(classifyCommit([], strictOpts), 'exclude'); + }); + + test('8: [.planning/STATE.md, src/a.ts] — forbiddenPaths [] default, [.planning/STATE.md] strict', () => { + const files = ['.planning/STATE.md', 'src/a.ts']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), ['.planning/STATE.md']); + }); + + test('9: LOOKALIKE [src/planning-inspect.cts] — forbiddenPaths [] in both modes', () => { + const files = ['src/planning-inspect.cts']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), []); + }); + + test('10: LOOKALIKE [.planning-notes/x.md] — forbiddenPaths [] in both modes', () => { + const files = ['.planning-notes/x.md']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), []); + }); + + test('11: LOOKALIKE [.planningX/x.md] — forbiddenPaths [] in both modes', () => { + const files = ['.planningX/x.md']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), []); + }); + + test('12: BOUNDARY [.planning/phases.md] (file, stem equals a transient dir) — not forbidden default, forbidden strict', () => { + const files = ['.planning/phases.md']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), []); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), ['.planning/phases.md']); + }); + + test('13: BOUNDARY [.planning/phases/x.md] forbidden in both modes', () => { + const files = ['.planning/phases/x.md']; + assert.deepStrictEqual(forbiddenPaths(files, defaultOpts), ['.planning/phases/x.md']); + assert.deepStrictEqual(forbiddenPaths(files, strictOpts), ['.planning/phases/x.md']); + }); + + test('14: every parsed transient dir forbids .planning//f.md in default mode', () => { + for (const d of fixture.transientDirs) { + const p = `.planning/${d}/f.md`; + assert.deepStrictEqual( + forbiddenPaths([p], defaultOpts), [p], + `expected ${p} to be forbidden in default mode`, + ); + } + }); + + test('15: every structural file is not forbidden default, forbidden strict', () => { + const structuralRe = new RegExp(fixture.structuralRe); + for (const name of ['STATE', 'ROADMAP', 'MILESTONES', 'PROJECT', 'REQUIREMENTS']) { + const p = `.planning/${name}.md`; + assert.ok(structuralRe.test(p), `fixture bug: STRUCTURAL_RE must accept ${p}`); + assert.deepStrictEqual(forbiddenPaths([p], defaultOpts), [], `${p} must not be forbidden in default mode`); + assert.deepStrictEqual(forbiddenPaths([p], strictOpts), [p], `${p} must be forbidden in strict mode`); + } + }); + + test('16: LOOKALIKE structural names — structuralPaths [] (anchored), land in otherPlanningPaths default', () => { + const files = ['.planning/STATEX.md', '.planning/STATE.md.bak']; + assert.deepStrictEqual(structuralPaths(files, defaultOpts), []); + assert.deepStrictEqual( + otherPlanningPaths(files, defaultOpts).sort(), + [...files].sort(), + ); + }); + + test('17: CRLF string form classifies/filters identically to the LF array form', () => { + const crlf = '.planning/phases/a.md\r\nsrc/b.ts\r\n'; + const arr = ['.planning/phases/a.md', 'src/b.ts']; + for (const opts of [defaultOpts, strictOpts]) { + assert.strictEqual(classifyCommit(crlf, opts), classifyCommit(arr, opts)); + assert.deepStrictEqual(forbiddenPaths(crlf, opts), forbiddenPaths(arr, opts)); + } + }); + + test('18: parseWorkflow throws on an absent declaration, and names the count on a duplicate', () => { + assert.throws( + () => parseWorkflow('no declarations here\n'), + /no TRANSIENT_DIRS declaration/, + ); + assert.throws( + () => parseWorkflow('TRANSIENT_DIRS="a b"\n'), + /no STRUCTURAL_RE declaration/, + ); + const dupTransient = [ + 'TRANSIENT_DIRS="a b"', + 'TRANSIENT_DIRS="a b"', + 'STRUCTURAL_RE="^\\.planning/STATE\\.md$"', + ].join('\n'); + assert.throws( + () => parseWorkflow(dupTransient), + /TRANSIENT_DIRS declared 2 times/, + ); + const dupStructural = [ + 'TRANSIENT_DIRS="a b"', + 'STRUCTURAL_RE="^\\.planning/STATE\\.md$"', + 'STRUCTURAL_RE="^\\.planning/STATE\\.md$"', + ].join('\n'); + assert.throws( + () => parseWorkflow(dupStructural), + /STRUCTURAL_RE declared 2 times/, + ); + }); + }); + + // ── L2: the real create_pr_branch recipe, executed against real git ───── + describe('L2: create_pr_branch recipe (real git)', () => { + const activeDirs = []; + + function trackDir(dir) { + activeDirs.push(dir); + return dir; + } + + function teardown() { + while (activeDirs.length) cleanup(activeDirs.pop()); + } + + function currentTransientDirs() { + return readWorkflow().transientDirs; + } + + /** + * Builds a fixture repo: + * main: code.txt, (unless noPlanning) .planning/STATE.md, + * .planning/phases/old.md + * feature: c1 modifies code.txt + .planning/STATE.md, ADDS + * .planning/phases/new.md; c2 modifies code.txt AND the same + * .planning/phases/new.md (unless planningOnlySecondCommit, + * in which case c2 touches ONLY .planning/phases/new.md). + * prbranch: checked out from main (or from main + a conflicting extra + * commit on main, when conflict is true). + */ + function buildFixture({ noPlanning = false, conflict = false, planningOnlySecondCommit = false } = {}) { + const dir = trackDir(fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-prbranch-'))); + initRepo(dir); + writeFile(dir, 'code.txt', 'line1\n'); + if (!noPlanning) { + writeFile(dir, '.planning/STATE.md', 'state v1\n'); + writeFile(dir, '.planning/phases/old.md', 'old plan\n'); + } + commitAll(dir, 'chore: base'); + git(['branch', 'feature'], dir); + + git(['checkout', '-q', 'feature'], dir); + writeFile(dir, 'code.txt', 'line1-c1\n'); + writeFile(dir, '.planning/STATE.md', 'state v2\n'); + writeFile(dir, '.planning/phases/new.md', 'new plan v1\n'); + commitAll(dir, 'feat: c1'); + + if (planningOnlySecondCommit) { + writeFile(dir, '.planning/phases/new.md', 'new plan v2\n'); + commitAll(dir, 'docs: c2 planning-only'); + } else { + writeFile(dir, 'code.txt', 'line1-c1\nline2\n'); + writeFile(dir, '.planning/phases/new.md', 'new plan v2\n'); + commitAll(dir, 'feat: c2'); + } + + git(['checkout', '-q', 'main'], dir); + if (conflict) { + writeFile(dir, 'code.txt', 'line1-main\n'); + commitAll(dir, 'chore: main conflicting edit'); + } + git(['checkout', '-q', '-b', 'prbranch', 'main'], dir); + return dir; + } + + function readWorkflowText() { + return fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + } + + // Builds the exact fixture script L2 executes: fixed shell vars plus the + // REAL create_pr_branch cherry-pick loop extracted verbatim from + // pr-branch.md — not a hand-written mirror of it. NOTE: INCLUDED_COMMITS + // here is deliberately ALL commits `main..feature`; this layer is only + // proving the filter recipe (rm/checkout/conflict-halt/empty-skip), not + // `analyze_commits`' include/exclude classification, which L1 covers. + function buildRecipeScript(filterPaths) { + return [ + 'set -u', + 'CURRENT_BRANCH=feature', + 'PR_BRANCH=prbranch', + 'TARGET=main', + 'INCLUDED_COMMITS=$(git rev-list --reverse main..feature)', + `FILTER_PATHS="${filterPaths.join(' ')}"`, + extractPickLoop(readWorkflowText()), + ].join('\n'); + } + + function runFilterLoop(repoDir, { strict, transientDirs }) { + const filterPaths = strict ? ['.planning/'] : transientDirs.map((d) => `.planning/${d}/`); + const script = buildRecipeScript(filterPaths); + try { + const stdout = execFileSync('sh', ['-c', script], { cwd: repoDir, encoding: 'utf8', timeout: 30000 }); + return { status: 0, stdout, stderr: '' }; + } catch (err) { + return { + status: typeof err.status === 'number' ? err.status : 1, + stdout: err.stdout || '', + stderr: err.stderr || '', + }; + } + } + + test('19: REGRESSION default — old.md is never staged as deleted', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + const result = runFilterLoop(dir, { strict: false, transientDirs }); + try { + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const lines = git(['diff', '--name-status', 'main..prbranch'], dir) + .split('\n').map((s) => s.trim()).filter(Boolean); + const deletedOld = lines.some((l) => l.startsWith('D') && l.includes('.planning/phases/old.md')); + assert.strictEqual(deletedOld, false, `old.md must never be deleted; diff lines: ${lines.join(' | ')}`); + } finally { + teardown(); + } + }); + + test('20: REGRESSION strict — base planning tree is never deleted', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + try { + const result = runFilterLoop(dir, { strict: true, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const tree = git(['ls-tree', '-r', '--name-only', 'prbranch', '--', '.planning'], dir) + .split('\n').map((s) => s.trim()).filter(Boolean); + assert.ok(tree.includes('.planning/STATE.md'), `expected .planning/STATE.md in ${tree.join(', ')}`); + assert.ok(tree.includes('.planning/phases/old.md'), `expected .planning/phases/old.md in ${tree.join(', ')}`); + } finally { + teardown(); + } + }); + + test('21: REGRESSION default — both c1 and c2 land', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + try { + const result = runFilterLoop(dir, { strict: false, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const count = parseInt(git(['rev-list', '--count', 'main..prbranch'], dir).trim(), 10); + assert.strictEqual(count, 2, 'both c1 and c2 must land as commits on prbranch'); + const codeContent = fs.readFileSync(path.join(dir, 'code.txt'), 'utf-8'); + assert.ok(codeContent.includes('line2'), `expected c2's line in code.txt, got: ${codeContent}`); + } finally { + teardown(); + } + }); + + test('22: REGRESSION strict — both c1 and c2 land', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + try { + const result = runFilterLoop(dir, { strict: true, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const count = parseInt(git(['rev-list', '--count', 'main..prbranch'], dir).trim(), 10); + assert.strictEqual(count, 2, 'both c1 and c2 must land as commits on prbranch'); + const codeContent = fs.readFileSync(path.join(dir, 'code.txt'), 'utf-8'); + assert.ok(codeContent.includes('line2'), `expected c2's line in code.txt, got: ${codeContent}`); + } finally { + teardown(); + } + }); + + test('23: default end-to-end — diff carries code + STATE.md, nothing forbidden-default', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + try { + const result = runFilterLoop(dir, { strict: false, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const files = git(['diff', '--name-only', 'main..prbranch'], dir) + .split('\n').map((s) => s.trim()).filter(Boolean); + assert.ok(files.includes('code.txt'), `expected code.txt in ${files.join(', ')}`); + assert.ok(files.includes('.planning/STATE.md'), `expected .planning/STATE.md in ${files.join(', ')}`); + const re = forbiddenRegex({ strict: false, transientDirs }); + assert.deepStrictEqual(files.filter((f) => re.test(f)), []); + } finally { + teardown(); + } + }); + + test('24: strict end-to-end — diff carries code, nothing under .planning/', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture(); + try { + const result = runFilterLoop(dir, { strict: true, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const files = git(['diff', '--name-only', 'main..prbranch'], dir) + .split('\n').map((s) => s.trim()).filter(Boolean); + assert.ok(files.includes('code.txt'), `expected code.txt in ${files.join(', ')}`); + assert.deepStrictEqual(files.filter((f) => f.startsWith('.planning/')), []); + } finally { + teardown(); + } + }); + + test('25: NEGATIVE — a real code conflict is not swallowed by the filter, and the halt path fully unwinds the partial prbranch', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture({ conflict: true }); + try { + const result = runFilterLoop(dir, { strict: false, transientDirs }); + assert.notStrictEqual(result.status, 0, 'a genuine code conflict must not exit 0'); + assert.ok( + result.stderr.includes('Conflict outside the .planning/ filter'), + `expected the real halt message in stderr, got: ${result.stderr}`, + ); + const currentBranch = git(['rev-parse', '--abbrev-ref', 'HEAD'], dir).trim(); + assert.strictEqual( + currentBranch, 'feature', + `expected the halt path to restore CURRENT_BRANCH (feature), got: ${currentBranch}`, + ); + assert.throws( + () => git(['rev-parse', '--verify', 'prbranch'], dir), + /.*/, + 'expected the partial prbranch to be deleted by the halt path, not left stranded mid cherry-pick', + ); + } finally { + teardown(); + } + }); + + test('26: BOUNDARY — main tracks no .planning/ at all; loop still exits 0, both commits land', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture({ noPlanning: true }); + try { + const result = runFilterLoop(dir, { strict: false, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const count = parseInt(git(['rev-list', '--count', 'main..prbranch'], dir).trim(), 10); + assert.strictEqual(count, 2); + } finally { + teardown(); + } + }); + + test('27: BOUNDARY strict — a planning-only c2 is excluded, only c1 lands', () => { + const transientDirs = currentTransientDirs(); + const dir = buildFixture({ planningOnlySecondCommit: true }); + try { + const result = runFilterLoop(dir, { strict: true, transientDirs }); + assert.strictEqual(result.status, 0, `filter loop failed: ${result.stderr}`); + const count = parseInt(git(['rev-list', '--count', 'main..prbranch'], dir).trim(), 10); + assert.strictEqual(count, 1, 'a commit touching only .planning/ must not land under strict mode'); + } finally { + teardown(); + } + }); + }); + + // ── L3: planning.pr_strict registration through the real CLI/config ───── + describe('L3: planning.pr_strict config key registration', () => { + const activeDirs = []; + function trackDir(dir) { + activeDirs.push(dir); + return dir; + } + function teardown() { + while (activeDirs.length) cleanup(activeDirs.pop()); + } + + test('28: no config.json at all -> config-get planning.pr_strict --raw is false, not "Key not found"', () => { + const dir = trackDir(createTempProject()); + try { + const res = runGsdTools(['query', 'config-get', 'planning.pr_strict', '--raw'], dir); + assert.ok(res.success, `expected success, got: ${res.error}`); + assert.strictEqual(res.output, 'false'); + } finally { + teardown(); + } + }); + + test('29: {"planning":{"pr_strict":true}} -> prints true', () => { + const dir = trackDir(createTempProject()); + try { + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ planning: { pr_strict: true } }), + ); + const res = runGsdTools(['query', 'config-get', 'planning.pr_strict', '--raw'], dir); + assert.ok(res.success, `expected success, got: ${res.error}`); + assert.strictEqual(res.output, 'true'); + } finally { + teardown(); + } + }); + + test('30: {"planning":{"pr_strict":false}} -> prints false', () => { + const dir = trackDir(createTempProject()); + try { + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ planning: { pr_strict: false } }), + ); + const res = runGsdTools(['query', 'config-get', 'planning.pr_strict', '--raw'], dir); + assert.ok(res.success, `expected success, got: ${res.error}`); + assert.strictEqual(res.output, 'false'); + } finally { + teardown(); + } + }); + + test('31: config-set planning.pr_strict true exits 0 and round-trips through config-get', () => { + const dir = trackDir(createTempProject()); + try { + const setRes = runGsdTools(['config-set', 'planning.pr_strict', 'true'], dir); + assert.ok(setRes.success, `config-set failed: ${setRes.error}`); + const getRes = runGsdTools(['query', 'config-get', 'planning.pr_strict', '--raw'], dir); + assert.ok(getRes.success, `config-get failed: ${getRes.error}`); + assert.strictEqual(getRes.output, 'true'); + } finally { + teardown(); + } + }); + + test('32: config-set planning.pr_stric true (typo) is rejected with a non-zero exit', () => { + const dir = trackDir(createTempProject()); + try { + const res = runGsdTools(['config-set', 'planning.pr_stric', 'true'], dir); + assert.ok(!res.success, 'a typo\'d key must not be accepted'); + assert.notStrictEqual(res.exitCode, 0); + } finally { + teardown(); + } + }); + + test('33: config-defaults.manifest.json parses and planning.pr_strict is strictly false', () => { + const manifest = JSON.parse(fs.readFileSync(CONFIG_DEFAULTS_MANIFEST_PATH, 'utf-8')); + assert.strictEqual( + manifest.planning && manifest.planning.pr_strict, + false, + 'planning.pr_strict must be present and strictly false (not truthy, not absent)', + ); + }); + + test('34: config-schema.manifest.json validKeys includes planning.pr_strict', () => { + const schema = JSON.parse(fs.readFileSync(CONFIG_SCHEMA_MANIFEST_PATH, 'utf-8')); + assert.ok( + Array.isArray(schema.validKeys) && schema.validKeys.includes('planning.pr_strict'), + 'config-schema.manifest.json validKeys must include "planning.pr_strict"', + ); + }); + + test('35: loadConfig resolves the flat root alias {"pr_strict":true} -> config.pr_strict === true', () => { + const dir = trackDir(createTempProject()); + try { + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ pr_strict: true }), + ); + const config = loadConfig(dir); + assert.strictEqual(config.pr_strict, true); + } finally { + teardown(); + } + }); + + test('36: loadConfig on {} yields pr_strict === false', () => { + const dir = trackDir(createTempProject()); + try { + fs.writeFileSync(path.join(dir, '.planning', 'config.json'), JSON.stringify({})); + const config = loadConfig(dir); + assert.strictEqual(config.pr_strict, false); + } finally { + teardown(); + } + }); + }); + + // ── L4: the issue's load-bearing worktree claim, EXECUTED not asserted ── + describe('L4: worktree materialization (issue #2971 load-bearing claim)', () => { + const activeDirs = []; + const activeWorktrees = []; + + function teardown() { + while (activeWorktrees.length) { + const { repoDir, worktreeDir } = activeWorktrees.pop(); + try { + git(['worktree', 'remove', '--force', worktreeDir], repoDir); + } catch (_) { + // best-effort; cleanup() below still removes the directory. + } + cleanup(worktreeDir); + } + while (activeDirs.length) cleanup(activeDirs.pop()); + } + + test('37: gitignored .planning/ never reaches a linked worktree (commit_docs:false is broken for parallel executors)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-prbranch-wt-ignored-')); + activeDirs.push(repoDir); + try { + initRepo(repoDir); + writeFile(repoDir, '.gitignore', '.planning/\n'); + writeFile(repoDir, '.planning/STATE.md', 'state\n'); + writeFile(repoDir, 'README.md', '# repo\n'); + commitAll(repoDir, 'chore: base (planning gitignored)'); + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + + const worktreeDir = uniqueTmpPath('gsd-prbranch-wt-linked'); + git(['worktree', 'add', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + + assert.strictEqual( + fs.existsSync(path.join(worktreeDir, '.planning')), + false, + 'a worktree built from a commit whose .planning/ was gitignored (never tracked) must not contain .planning/', + ); + } finally { + teardown(); + } + }); + + test('38: committed .planning/PLAN.md exists in a linked worktree (commit_docs:true + planning.pr_strict is the working combination)', () => { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-prbranch-wt-committed-')); + activeDirs.push(repoDir); + try { + initRepo(repoDir); + writeFile(repoDir, '.planning/PLAN.md', '# Plan\n'); + writeFile(repoDir, 'README.md', '# repo\n'); + commitAll(repoDir, 'chore: base (planning committed)'); + const sha = git(['rev-parse', 'HEAD'], repoDir).trim(); + + const worktreeDir = uniqueTmpPath('gsd-prbranch-wt-committed-linked'); + git(['worktree', 'add', worktreeDir, sha], repoDir); + activeWorktrees.push({ repoDir, worktreeDir }); + + assert.ok( + fs.existsSync(path.join(worktreeDir, '.planning', 'PLAN.md')), + '.planning/PLAN.md must exist inside a worktree built from a commit that tracked it', + ); + } finally { + teardown(); + } + }); + }); + + // ── L5: fast-check properties ───────────────────────────────────────── + describe('L5: fast-check properties', () => { + function buildPool(transientDirs) { + const transientPaths = transientDirs.flatMap((d) => [`.planning/${d}/f.md`, `.planning/${d}/sub/g.md`]); + const structuralPathsPool = [ + '.planning/STATE.md', + '.planning/ROADMAP.md', + '.planning/MILESTONES.md', + '.planning/PROJECT.md', + '.planning/REQUIREMENTS.md', + '.planning/milestones/m1/x.md', + ]; + const thirdBucket = ['.planning/config.json', '.planning/notes.md']; + const nonPlanning = ['src/a.ts', 'docs/readme.md', 'package.json', 'src/planning-inspect.cts']; + return fc.constantFrom(...transientPaths, ...structuralPathsPool, ...thirdBucket, ...nonPlanning); + } + + test('39: forbiddenPaths(strict) is a superset of forbiddenPaths(default)', () => { + const { transientDirs, structuralRe } = readWorkflow(); + const pool = buildPool(transientDirs); + fc.assert( + fc.property(fc.array(pool, { maxLength: 12 }), (files) => { + const strictSet = new Set(forbiddenPaths(files, { strict: true, transientDirs, structuralRe })); + const defaultSet = new Set(forbiddenPaths(files, { strict: false, transientDirs, structuralRe })); + for (const f of defaultSet) { + assert.ok(strictSet.has(f), `${f} forbidden in default mode but not in strict mode`); + } + }), + { seed: 2971, numRuns: 300 }, + ); + }); + + test('40: forbiddenPaths(strict) equals exactly the members matching /^\\.planning\\//', () => { + const { transientDirs, structuralRe } = readWorkflow(); + const pool = buildPool(transientDirs); + fc.assert( + fc.property(fc.array(pool, { maxLength: 12 }), (files) => { + const actual = forbiddenPaths(files, { strict: true, transientDirs, structuralRe }).sort(); + const expected = files.filter((f) => /^\.planning\//.test(f)).sort(); + assert.deepStrictEqual(actual, expected); + }), + { seed: 2971, numRuns: 300 }, + ); + }); + + test('41: no path outside .planning/ is ever in forbiddenPaths, either mode', () => { + const { transientDirs, structuralRe } = readWorkflow(); + const pool = buildPool(transientDirs); + fc.assert( + fc.property(fc.array(pool, { maxLength: 12 }), (files) => { + for (const strict of [true, false]) { + const result = forbiddenPaths(files, { strict, transientDirs, structuralRe }); + for (const f of result) { + assert.ok(f.startsWith('.planning/'), `${f} is outside .planning/ but was forbidden (strict=${strict})`); + } + } + }), + { seed: 2971, numRuns: 300 }, + ); + }); + + test('42: strict mode classifyCommit is include iff a member is outside .planning/', () => { + const { transientDirs, structuralRe } = readWorkflow(); + const pool = buildPool(transientDirs); + fc.assert( + fc.property(fc.array(pool, { maxLength: 12 }), (files) => { + const actual = classifyCommit(files, { strict: true, transientDirs, structuralRe }); + const expected = files.some((f) => !/^\.planning\//.test(f)) ? 'include' : 'exclude'; + assert.strictEqual(actual, expected); + }), + { seed: 2971, numRuns: 300 }, + ); + }); + }); + + // ── L6: drift guard over the shipped workflow (prose) ──────────────── + describe('L6: drift guard over the shipped workflow (prose)', () => { + test('43: workflow declares TRANSIENT_DIRS/STRUCTURAL_RE exactly once; 9 dirs incl. phases + ui-reviews', () => { + const { transientDirs } = readWorkflow(); + assert.strictEqual(transientDirs.length, 9, `expected 9 transient dirs, got: ${transientDirs.join(', ')}`); + assert.ok(transientDirs.includes('phases')); + assert.ok(transientDirs.includes('ui-reviews')); + }); + + test('44: verify step derives from FORBIDDEN_RE, not the old unconditional wc -l count', () => { + const text = readFileNormalized(WORKFLOW_PATH); + const occurrences = (text.match(/FORBIDDEN_RE/g) || []).length; + assert.ok( + occurrences >= 2, + `expected FORBIDDEN_RE to appear at least twice (declaration + use), found ${occurrences}`, + ); + assert.ok( + !text.includes('grep "^\\.planning/" | wc -l'), + 'the old unconditional `grep "^\\.planning/" | wc -l` count must be gone', + ); + }); + + test('45: create_pr_branch recipe carries every command shape L2 proved correct, in the load-bearing order', () => { + const text = readFileNormalized(WORKFLOW_PATH); + const required = [ + 'git rm -r -f -q --ignore-unmatch --', + 'git checkout HEAD --', + '--diff-filter=U', + 'git cherry-pick --quit', + ]; + for (const needle of required) { + assert.ok(text.includes(needle), `workflow is missing required recipe fragment: ${needle}`); + } + + const loop = extractPickLoop(text); + assert.ok(loop.length > 0, 'extractPickLoop must find exactly one canonical cherry-pick loop'); + + const rmIndex = loop.indexOf('git rm -r -f -q --ignore-unmatch --'); + const checkoutIndex = loop.indexOf('git checkout HEAD --'); + assert.ok(rmIndex >= 0 && checkoutIndex >= 0, 'both rm and checkout forms must appear inside the extracted loop'); + assert.ok( + rmIndex < checkoutIndex, + 'order is load-bearing: `git rm -r -f -q --ignore-unmatch --` must run BEFORE `git checkout HEAD --` — ' + + 'reversing it would restore the target branch\'s file and then immediately delete it, corrupting the PR branch', + ); + }); + + test('46: the old un-stage form is gone', () => { + const text = readFileNormalized(WORKFLOW_PATH); + assert.ok( + !text.includes('git rm -r --cached ".planning/'), + 'the old unconditional `git rm -r --cached ".planning/` form must be removed', + ); + }); + + test('47: the workflow reads the config key', () => { + const text = readFileNormalized(WORKFLOW_PATH); + assert.ok(text.includes('config-get planning.pr_strict'), 'workflow must call config-get planning.pr_strict'); + }); + + test('48: success criteria no longer claim an unconditional zero', () => { + const text = readFileNormalized(WORKFLOW_PATH); + assert.ok( + !text.includes('- [ ] No .planning/ files in PR branch diff'), + 'the unconditional "No .planning/ files in PR branch diff" success line must be removed/replaced', + ); + }); + }); +});