fix(#2796): roadmap-update-plan-progress accepts --phase flag form (#2815)

* fix(#2796): roadmap update-plan-progress accepts --phase flag form

roadmap-update-plan-progress used positional-only arg parsing: args[0].
When execute-phase.md:228 calls it with --phase <N>, args[0] was the
literal string "--phase", which findPhase received as the phase number.
findPhase returned found:false, causing updated:false with no write.
ROADMAP.md plan checkboxes silently never advanced.

Fix: check for --phase <value> first; fall back to the first non-flag
positional argument for backward-compatible direct calls.

Regression test: tests/bug-2796-arg-parsing-regression.test.cjs

* fix(#2796): address CodeRabbit — guard --phase against flag-like values, bash fence

sdk/src/query/roadmap-update-plan-progress.ts

@@ -15,7 +15,23 @@ import { GSDError, ErrorClassification } from '../errors.js';
 import type { QueryHandler } from './utils.js';
 
 export const roadmapUpdatePlanProgress: QueryHandler = async (args, projectDir, workstream) => {
-  const phaseNum = args[0];
+  // Support --phase <N> flag form in addition to positional (fixes #2796).
+  // execute-phase.md:228 passes --phase so positional-only parsing silently
+  // took the literal string "--phase" as the phase value.
+  const phaseIdx = args.indexOf('--phase');
+  let phaseNum: string;
+  const phaseFlagValue = phaseIdx !== -1 ? args[phaseIdx + 1] : undefined;
+  if (
+    phaseIdx !== -1 &&
+    phaseFlagValue !== undefined &&
+    !String(phaseFlagValue).startsWith('--')
+  ) {
+    phaseNum = String(phaseFlagValue);
+  } else {
+    // Positional: skip any leading flag tokens in case of mixed invocations.
+    const positional = args.filter((a) => !String(a).startsWith('--'));
+    phaseNum = positional[0] ? String(positional[0]) : '';
+  }
   if (!phaseNum) {
     throw new GSDError('phase number required for roadmap update-plan-progress', ErrorClassification.Validation);
   }

tests/bug-2796-arg-parsing-regression.test.cjs

@@ -0,0 +1,153 @@
+/**
+ * Regression test for bug #2796
+ *
+ * roadmap.update-plan-progress used positional-only arg destructuring:
+ * `const phaseNum = args[0]`. When called with the flag form documented in
+ * execute-phase.md:228 (`--phase "TEST" --plan "01" --status "complete"`),
+ * args[0] was the literal string "--phase", which was passed to findPhase().
+ * findPhase found no phase named "--phase" and returned `updated: false` with
+ * `reason: "no matching checkbox found"`, silently no-oping. ROADMAP.md plan
+ * checkboxes never advanced.
+ *
+ * The stateBeginPhase handler already uses parseNamedArgs and is NOT affected.
+ *
+ * Fix: roadmap-update-plan-progress.ts now checks for --phase <value> before
+ * falling back to positional arg[0] (filtering out flag tokens).
+ */
+
+'use strict';
+
+const { describe, test, beforeEach, afterEach } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const { execFileSync } = require('node:child_process');
+const { createTempGitProject, cleanup } = require('./helpers.cjs');
+
+const REPO_ROOT = path.join(__dirname, '..');
+const SDK_CLI = path.join(REPO_ROOT, 'sdk', 'dist', 'cli.js');
+
+function runSdkQuery(subcommand, args, projectDir) {
+  const argv = ['query', subcommand, ...args, '--project-dir', projectDir];
+  let stdout = '';
+  let stderr = '';
+  let exitCode = 0;
+  try {
+    stdout = execFileSync(process.execPath, [SDK_CLI, ...argv], {
+      encoding: 'utf-8',
+      stdio: ['pipe', 'pipe', 'pipe'],
+      env: { ...process.env, GSD_SESSION_KEY: '' },
+    });
+  } catch (err) {
+    exitCode = err.status ?? 1;
+    stdout = err.stdout?.toString() ?? '';
+    stderr = err.stderr?.toString() ?? '';
+  }
+  let json = null;
+  try { json = JSON.parse(stdout.trim()); } catch { /* ok */ }
+  return { exitCode, stdout: stdout.trim(), stderr: stderr.trim(), json };
+}
+
+/** Create a minimal ROADMAP.md with a phase checkbox */
+function createRoadmap(projectDir, phaseNum, planLabel) {
+  const planningDir = path.join(projectDir, '.planning');
+  fs.mkdirSync(path.join(planningDir, 'phases'), { recursive: true });
+
+  const roadmap = [
+    '# My Project Roadmap',
+    '',
+    '## v1.0 — MVP',
+    '',
+    `### Phase ${phaseNum}: Test Phase`,
+    '',
+    `**Plans:** 1/1 plans complete`,
+    '',
+    `| # | Phase | Plans | Status | Date |`,
+    `|---|-------|-------|--------|------|`,
+    `| ${phaseNum} | Test Phase | 0/1 | Planned | |`,
+    '',
+    `- [ ] ${planLabel}: Do the thing`,
+    '',
+  ].join('\n');
+
+  fs.writeFileSync(path.join(planningDir, 'ROADMAP.md'), roadmap);
+
+  // Create the phase directory so findPhase finds it
+  const phaseDir = path.join(
+    planningDir, 'phases',
+    `${String(phaseNum).padStart(2, '0')}-test-phase`
+  );
+  fs.mkdirSync(phaseDir, { recursive: true });
+
+  // Create a plan file and a summary so progress = 1/1
+  fs.writeFileSync(path.join(phaseDir, `${planLabel}-PLAN.md`), '# Plan\n');
+  fs.writeFileSync(path.join(phaseDir, `${planLabel}-SUMMARY.md`), '# Summary\n');
+
+  return phaseDir;
+}
+
+describe('bug-2796: roadmap update-plan-progress accepts --phase flag', () => {
+  let tmpDir;
+
+  beforeEach(() => {
+    tmpDir = createTempGitProject('gsd-test-2796-');
+  });
+
+  afterEach(() => {
+    cleanup(tmpDir);
+  });
+
+  test('flag form --phase <N> resolves the correct phase (not literal "--phase")', () => {
+    createRoadmap(tmpDir, '9', '01');
+
+    // Flag form: this is the form execute-phase.md:228 uses
+    const result = runSdkQuery(
+      'roadmap.update-plan-progress',
+      ['--phase', '9'],
+      tmpDir
+    );
+
+    // Before fix: exitCode=1 with "phase --phase not found" or updated:false
+    // After fix: should succeed with phase="9" and updated:true
+    assert.strictEqual(result.exitCode, 0, `should exit 0; stderr: ${result.stderr}`);
+    assert.ok(result.json !== null, 'should emit JSON');
+    assert.ok(result.json.updated === true, 'updated should be true');
+    assert.strictEqual(String(result.json.phase), '9', 'phase should be "9", not "--phase"');
+  });
+
+  test('positional form still works (backward compat)', () => {
+    createRoadmap(tmpDir, '9', '01');
+
+    const result = runSdkQuery(
+      'roadmap.update-plan-progress',
+      ['9'],
+      tmpDir
+    );
+
+    assert.strictEqual(result.exitCode, 0, `should exit 0; stderr: ${result.stderr}`);
+    assert.ok(result.json?.updated === true, 'updated should be true');
+    assert.strictEqual(String(result.json.phase), '9', 'phase should be "9"');
+  });
+
+  test('flag form does not pass "--phase" as the phase value to findPhase', () => {
+    // Before fix: findPhase("--phase") returned found:false, causing updated:false
+    // This test confirms the phase value is not the flag name itself.
+    createRoadmap(tmpDir, '5', '01');
+
+    const result = runSdkQuery(
+      'roadmap.update-plan-progress',
+      ['--phase', '5'],
+      tmpDir
+    );
+
+    // If the phase was "--phase", we'd get an error like "Phase --phase not found"
+    assert.ok(
+      !result.stderr.includes('--phase not found'),
+      'stderr must not say "--phase not found"'
+    );
+    assert.ok(
+      !result.stderr.includes('"--phase"'),
+      'stderr must not treat "--phase" as the phase value'
+    );
+  });
+});