From 14c3ef5b1fe22cf895db447e1296dc86034cc1f9 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 10 Apr 2026 11:25:55 -0400 Subject: [PATCH] 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 --- get-shit-done/workflows/pr-branch.md | 54 ++++++++++--- tests/bug-2004-pr-branch-milestone.test.cjs | 88 +++++++++++++++++++++ 2 files changed, 129 insertions(+), 13 deletions(-) create mode 100644 tests/bug-2004-pr-branch-milestone.test.cjs diff --git a/get-shit-done/workflows/pr-branch.md b/get-shit-done/workflows/pr-branch.md index 60e6e54ad..9bf74d52c 100644 --- a/get-shit-done/workflows/pr-branch.md +++ b/get-shit-done/workflows/pr-branch.md @@ -1,7 +1,8 @@ -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. @@ -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) ``` @@ -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 ``` diff --git a/tests/bug-2004-pr-branch-milestone.test.cjs b/tests/bug-2004-pr-branch-milestone.test.cjs new file mode 100644 index 000000000..1b10e0257 --- /dev/null +++ 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' + ); + }); +});