From dc9b71296777c7aa82ade02a9ae6426e898a6db6 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 27 Apr 2026 09:03:36 -0400 Subject: [PATCH] refactor(state): drop unused args + lift currentPhase in cmdStateCompletePhase (#2761) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(state): drop unused args param and lift currentPhase in cmdStateCompletePhase Two cleanup items surfaced by CodeRabbit review of PR #2759: 1. cmdStateCompletePhase(cwd, args, raw) — args is never read inside the function. All sibling state subcommands use the leaner (cwd, raw) shape. Remove the unused parameter and update the dispatch call in gsd-tools.cjs. 2. output() at line 1754 called fs.readFileSync(statePath) after readModifyWriteStateMd had already released the lock, re-extracting Current Phase via an extra fs read. The closure already computed currentPhase at line 1704; lifting resolvedPhase into outer scope and capturing it in the callback eliminates the post-lock read and closes the small race window. Co-Authored-By: Claude Sonnet 4.6 * test(#2761): apply CodeRabbit nitpicks with regression tests Two CodeRabbit nitpicks from PR #2761 review, each landed with a regression test so a future refactor can't unwind them. 1. tests/dispatcher.test.cjs — pin the enumerated subcommand list: the 'state unknown subcommand errors' test now also asserts that the dispatcher's error string includes 'complete-phase'. Without this, a future reformat of the available-subcommands enumeration could silently drop entries and the existing 'Unknown state subcommand' substring check would still pass. 2. get-shit-done/bin/lib/state.cjs — tighten the Phase fallback in cmdStateCompletePhase: when STATE.md is missing the canonical '**Current Phase:**' field and the only phase signal is the decorated body line under '## Current Position' (e.g. 'Phase: 01 (Foo) — EXECUTING'), the previous fallback returned the entire decorated string, producing messy downstream output: Status: Phase 01 (Foo) — EXECUTING complete Phase: 01 (Foo) — EXECUTING — COMPLETE The fallback now strips everything past the leading numeric/decimal token via /^\\s*([\\w.-]+)/ so degraded inputs produce clean output identical to the canonical path. 3. tests/state.test.cjs — two new tests in a dedicated describe block: - decorated Phase line writes clean Phase identifier - canonical Current Phase wins over Current Position decoration Both run real `gsd state complete-phase` against synthetic STATE.md fixtures and assert on the rendered Status field. --------- Co-authored-by: Claude Sonnet 4.6 --- get-shit-done/bin/gsd-tools.cjs | 2 +- get-shit-done/bin/lib/state.cjs | 23 ++++++-- tests/dispatcher.test.cjs | 8 +++ tests/state.test.cjs | 99 +++++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+), 7 deletions(-) diff --git a/get-shit-done/bin/gsd-tools.cjs b/get-shit-done/bin/gsd-tools.cjs index 14f787584..c93bcea19 100755 --- a/get-shit-done/bin/gsd-tools.cjs +++ b/get-shit-done/bin/gsd-tools.cjs @@ -484,7 +484,7 @@ async function runCommand(command, args, cwd, raw, defaultValue) { const { 'keep-recent': keepRecent, 'dry-run': dryRun } = parseNamedArgs(args, ['keep-recent'], ['dry-run']); state.cmdStatePrune(cwd, { keepRecent: keepRecent || '3', dryRun: !!dryRun }, raw); } else if (subcommand === 'complete-phase') { - state.cmdStateCompletePhase(cwd, args, raw); + state.cmdStateCompletePhase(cwd, raw); } else if (subcommand === 'milestone-switch') { // Bug #2630: reset STATE.md frontmatter + Current Position for new milestone. // NB: the flag is `--milestone`, not `--version` — gsd-tools reserves diff --git a/get-shit-done/bin/lib/state.cjs b/get-shit-done/bin/lib/state.cjs index 0ec833df5..4d8d926f0 100644 --- a/get-shit-done/bin/lib/state.cjs +++ b/get-shit-done/bin/lib/state.cjs @@ -1689,7 +1689,7 @@ function cmdStatePrune(cwd, options, raw) { * that the phase execution is finished and the project is ready for the next phase. * Implements the `gsd state complete-phase` subcommand (issue #2735). */ -function cmdStateCompletePhase(cwd, args, raw) { +function cmdStateCompletePhase(cwd, raw) { const statePath = planningPaths(cwd).state; if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw); @@ -1698,12 +1698,23 @@ function cmdStateCompletePhase(cwd, args, raw) { const today = new Date().toISOString().split('T')[0]; const updated = []; + let resolvedPhase = '?'; readModifyWriteStateMd(statePath, (content) => { - // Read the current phase number for descriptive messages - const currentPhase = stateExtractField(content, 'Current Phase') || - stateExtractField(content, 'Phase') || - '?'; + // Read the current phase number for descriptive messages. + // + // The 'Phase' fallback can match the decorated body line under + // `## Current Position` (e.g. `Phase: 01 (Foo) — EXECUTING`), which would + // flow downstream into messy `Status: Phase 01 (Foo) — EXECUTING complete` + // output. Strip everything past the leading numeric/decimal token so the + // fallback path produces a clean phase identifier matching the canonical + // `Current Phase` field. CodeRabbit nitpick on PR #2761. + const rawPhase = stateExtractField(content, 'Current Phase') || + stateExtractField(content, 'Phase') || + ''; + const phaseToken = rawPhase.match(/^\s*([\w.-]+)/); + const currentPhase = phaseToken ? phaseToken[1] : '?'; + resolvedPhase = currentPhase; // Update Status field const statusValue = `Phase ${currentPhase} complete`; @@ -1752,7 +1763,7 @@ function cmdStateCompletePhase(cwd, args, raw) { }, cwd); output( - { updated, phase: stateExtractField(fs.readFileSync(planningPaths(cwd).state, 'utf-8'), 'Current Phase') || '?' }, + { updated, phase: resolvedPhase }, raw, updated.length > 0 ? 'true' : 'false', ); diff --git a/tests/dispatcher.test.cjs b/tests/dispatcher.test.cjs index 43e09e5a4..b3ded5e53 100644 --- a/tests/dispatcher.test.cjs +++ b/tests/dispatcher.test.cjs @@ -71,6 +71,14 @@ describe('dispatcher error paths', () => { const result = runGsdTools('state bogus', tmpDir); assert.strictEqual(result.success, false, 'Should exit non-zero'); assert.ok(result.error.includes('Unknown state subcommand'), `Expected "Unknown state subcommand" in stderr, got: ${result.error}`); + // Pin the enumerated subcommand list. If a future refactor reformats the + // error string and silently drops 'complete-phase' from the available list, + // this test fails loudly rather than passing on the substring above. + // CodeRabbit nitpick on PR #2761. + assert.ok( + result.error.includes('complete-phase'), + `Expected enumerated subcommands to include "complete-phase", got: ${result.error}`, + ); }); // Unknown subcommand: template diff --git a/tests/state.test.cjs b/tests/state.test.cjs index 92703bf3c..363003d4c 100644 --- a/tests/state.test.cjs +++ b/tests/state.test.cjs @@ -2370,6 +2370,105 @@ describe('stale phase dirs do not corrupt phase counts (bug #2445)', () => { }); }); +// ───────────────────────────────────────────────────────────────────────────── +// state complete-phase: Phase-fallback decoration handling (PR #2761 nitpick) +// ───────────────────────────────────────────────────────────────────────────── +// +// When STATE.md is missing the canonical `**Current Phase:**` field but +// includes a decorated `## Current Position` body line, the fallback path used +// to leak the decoration into downstream Status/Phase strings — producing +// `**Status:** Phase 01 (Foo) — EXECUTING complete` instead of the expected +// `**Status:** Phase 01 complete`. CodeRabbit flagged this on PR #2761 and the +// Phase fallback now strips everything past the leading numeric/decimal token. +describe('state complete-phase: decorated Phase fallback (#2761 nitpick)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = createTempProject(); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + test('writes clean Phase identifier when only Current Position decoration is present', () => { + // STATE.md without the canonical `**Current Phase:**` field — the only + // phase signal lives inside the `## Current Position` block as a decorated + // line. This is the regression fixture. + const stateMd = [ + '---', + 'milestone: v1.0', + '---', + '', + '# State', + '', + '**Status:** Executing', + '**Last Activity:** 2024-01-15', + '', + '## Current Position', + '', + 'Phase: 01 (Foo) — EXECUTING', + 'Plan: bootstrap', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateMd); + + const result = runGsdTools('state complete-phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const updated = fs.readFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + 'utf-8', + ); + + // Status should reference the bare phase identifier (`01`), not the + // decorated string. The negative assertion catches the regression + // shape directly. + assert.ok( + updated.includes('**Status:** Phase 01 complete'), + `Status should be "Phase 01 complete", got STATE.md:\n${updated}`, + ); + assert.ok( + !updated.includes('Phase 01 (Foo) — EXECUTING complete'), + `Status must not embed Current Position decoration: ${updated}`, + ); + }); + + test('canonical Current Phase field is preferred over Current Position decoration', () => { + // When both are present, Current Phase wins — same outcome as before, but + // pinned here so a future refactor that flips precedence is caught. + const stateMd = [ + '---', + 'milestone: v1.0', + '---', + '', + '# State', + '', + '**Status:** Executing', + '**Current Phase:** 03', + '**Last Activity:** 2024-01-15', + '', + '## Current Position', + '', + 'Phase: 01 (Foo) — EXECUTING', + '', + ].join('\n'); + fs.writeFileSync(path.join(tmpDir, '.planning', 'STATE.md'), stateMd); + + const result = runGsdTools('state complete-phase', tmpDir); + assert.ok(result.success, `Command failed: ${result.error}`); + + const updated = fs.readFileSync( + path.join(tmpDir, '.planning', 'STATE.md'), + 'utf-8', + ); + assert.ok( + updated.includes('**Status:** Phase 03 complete'), + `Status should reference canonical Current Phase (03), got: ${updated}`, + ); + }); +}); + // ───────────────────────────────────────────────────────────────────────────── // summary-extract command // ─────────────────────────────────────────────────────────────────────────────