golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor(state): drop unused args + lift currentPhase in cmdStateCompletePhase (#2761)

Browse Source 
* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-04-27 09:03:36 -04:00
committed by GitHub
parent 9472f343db
commit dc9b712967
4 changed files with 125 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
get-shit-done/bin/gsd-tools.cjs
View File

@@ -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

23
get-shit-done/bin/lib/state.cjs
View File

@@ -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',
);

8
tests/dispatcher.test.cjs
View File

@@ -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

99
tests/state.test.cjs
View File

@@ -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
// ─────────────────────────────────────────────────────────────────────────────