fix(workflow): preserve structural planning commits in gsd-pr-branch (#2031)

The previous implementation filtered ALL .planning/-only commits, including milestone archive commits, STATE.md, ROADMAP.md, and PROJECT.md updates. Merging the PR branch then left the target with inconsistent planning state. Fixes by distinguishing two categories of .planning/ commits: - Structural (STATE.md, ROADMAP.md, MILESTONES.md, PROJECT.md, REQUIREMENTS.md, milestones/**): INCLUDED in PR branch - Transient (phases/, quick/, research/, threads/, todos/, debug/, seeds/, codebase/, ui-reviews/): EXCLUDED from PR branch The git rm in create_pr_branch is now scoped to transient subdirectories only, so structural files survive cherry-pick into the PR branch. Adds regression test asserting structural file handling is documented. Closes #2004 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-04-10 11:25:55 -04:00
parent 0a4ae79b7b
commit 14c3ef5b1f
2 changed files with 129 additions and 13 deletions
--- a/get-shit-done/workflows/pr-branch.md
+++ b/get-shit-done/workflows/pr-branch.md
@@ -1,7 +1,8 @@
 <purpose>
-Create a clean branch for pull requests by filtering out .planning/ commits.
-The PR branch contains only code changes — reviewers don't see GSD artifacts
-(PLAN.md, SUMMARY.md, STATE.md, CONTEXT.md, etc.).
+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.

 Uses git cherry-pick with path filtering to rebuild a clean history.
 </purpose>
@@ -48,24 +49,47 @@ Classify commits:
 git log --oneline "$TARGET".."$CURRENT_BRANCH" --no-merges
 ```

-For each commit, check if it ONLY touches .planning/ files:
+**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/**`
+
+**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/**`
+
+For each commit, check what it touches:

 ```bash
 # For each commit hash
 FILES=$(git diff-tree --no-commit-id --name-only -r $HASH)
-ALL_PLANNING=$(echo "$FILES" | grep -v "^\.planning/" | wc -l)
+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)
 ```

 Classify:
 - **Code commits**: Touch at least one non-.planning/ file → INCLUDE
- **Planning-only commits**: Touch only .planning/ files → EXCLUDE
- **Mixed commits**: Touch both → INCLUDE (planning changes come along)
+- **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)

 Display analysis:
 ```
-Commits to include: {N} (code changes)
-Commits to exclude: {N} (planning-only)
+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)
 ```
 </step>

@@ -77,13 +101,17 @@ PR_BRANCH="${CURRENT_BRANCH}-pr"
 git checkout -b "$PR_BRANCH" "$TARGET"
 ```

-Cherry-pick only code commits (in order):
+Cherry-pick code commits and structural planning commits (in order):

 ```bash
-for HASH in $CODE_COMMITS; do
+for HASH in $CODE_AND_STRUCTURAL_COMMITS; do
  git cherry-pick "$HASH" --no-commit
-  # Remove any .planning/ files that came along in mixed commits
-  git rm -r --cached .planning/ 2>/dev/null || true
+  # 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
+  done
  git commit -C "$HASH"
 done
 ```
--- a/tests/bug-2004-pr-branch-milestone.test.cjs
+++ b/tests/bug-2004-pr-branch-milestone.test.cjs
@@ -0,0 +1,88 @@
+/**
+ * Regression tests for bug #2004
+ *
+ * /gsd-pr-branch must not exclude milestone archive and structural planning
+ * commits. The previous implementation filtered ALL .planning/-only commits,
+ * including STATE.md, ROADMAP.md, MILESTONES.md, and milestones/** updates
+ * that are needed to preserve repository planning state after a merge.
+ *
+ * Fixed: pr-branch.md now distinguishes:
+ *   - Transient planning commits (phase plans, summaries, research, context) → EXCLUDE
+ *   - Structural planning commits (STATE.md, ROADMAP.md, MILESTONES.md,
+ *     PROJECT.md, milestones/**) → INCLUDE
+ *   - Code commits (any non-.planning/ file) → INCLUDE
+ *   - Mixed commits (code + planning) → INCLUDE
+ */
+
+'use strict';
+
+const { describe, test } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+
+const workflowPath = path.resolve(
+  __dirname, '..', 'get-shit-done', 'workflows', 'pr-branch.md'
+);
+
+describe('bug #2004: pr-branch preserves structural planning commits', () => {
+  let content;
+
+  test('setup: pr-branch workflow is readable', () => {
+    content = fs.readFileSync(workflowPath, 'utf-8');
+    assert.ok(content.length > 0, 'pr-branch.md must not be empty');
+  });
+
+  test('workflow distinguishes structural vs transient planning commits', () => {
+    content = content || fs.readFileSync(workflowPath, 'utf-8');
+    // Must contain language distinguishing structural from transient/phase planning files
+    assert.ok(
+      /structural|milestone.*archive|STATE\.md.*INCLUDE|preserve.*milestone|milestone.*preserve/i.test(content),
+      'pr-branch.md must distinguish structural planning commits from transient ones'
+    );
+  });
+
+  test('workflow lists STATE.md and ROADMAP.md as structural files to preserve', () => {
+    content = content || fs.readFileSync(workflowPath, 'utf-8');
+    assert.ok(
+      content.includes('STATE.md'),
+      'pr-branch.md must reference STATE.md as a structural file to preserve'
+    );
+    assert.ok(
+      content.includes('ROADMAP.md'),
+      'pr-branch.md must reference ROADMAP.md as a structural file to preserve'
+    );
+  });
+
+  test('workflow lists MILESTONES.md or milestones/ as structural files to preserve', () => {
+    content = content || fs.readFileSync(workflowPath, 'utf-8');
+    assert.ok(
+      content.includes('MILESTONES.md') || content.includes('milestones/'),
+      'pr-branch.md must reference MILESTONES.md or milestones/ as structural files to preserve'
+    );
+  });
+
+  test('workflow has four commit categories (code, planning-only, mixed, structural)', () => {
+    content = content || fs.readFileSync(workflowPath, 'utf-8');
+    // Must have at least a "structural" or "milestone" category beyond the original three
+    assert.ok(
+      /structural.*commit|milestone.*commit|commit.*structural|commit.*milestone/i.test(content) ||
+      /INCLUDE.*STATE\.md|STATE\.md.*INCLUDE/i.test(content),
+      'pr-branch.md must classify structural planning commits as INCLUDE'
+    );
+  });
+
+  test('create_pr_branch step does not rm -r --cached all of .planning/', () => {
+    content = content || fs.readFileSync(workflowPath, 'utf-8');
+    // The original bug: `git rm -r --cached .planning/` nuked structural files.
+    // The fix must either remove this wholesale rm or scope it to transient dirs.
+    // Acceptable: narrowed rm targeting only phase/, quick/, research/, etc.
+    // Not acceptable: `git rm -r --cached .planning/` with no scoping.
+    const hasUnscoped = /git rm -r --cached \.planning\/(?!\*)?(?!phases|quick|research|threads|todos|debug|seeds|ui-reviews|codebase)/
+      .test(content);
+    assert.ok(
+      !hasUnscoped,
+      'create_pr_branch must not use unscoped "git rm -r --cached .planning/" — scope to transient subdirectories only'
+    );
+  });
+});