enhance(#3957): a no-op reports the real condition and the values it already computed (#4157)

* test(#3957): add failing-first coverage for no-op decline reporting (epic #3473 B9) * fix(#3957): a no-op reports the real condition and the values it already computed (epic #3473 B9) * test(#3957): correct stale assertions and a withheld-arm fixture after rebase (epic #3473 B9) * docs(#3957): add Fixed changeset fragment for no-op decline reporting (epic #3473 B9) * docs(#3957): backfill changeset PR number to #4157 --------- Co-authored-by: sim <sim@local>
2026-09-01 21:40:07 -04:00
parent bf4485ada2
commit ddb877fa0a
7 changed files with 874 additions and 40 deletions
--- a/src/io.cts
+++ b/src/io.cts
@@ -214,6 +214,47 @@ function output(result: unknown, raw: boolean, rawValue?: unknown): void {
  writeAllSync(1, data);
 }

+/**
+ * Single owner of the "a no-op decline reports the real condition" pairing:
+ * a `[gsd-tools] WARNING:` stderr disclosure plus the matching
+ * `{ [flagKey]: false, ...computed, reason }` JSON payload (#3957, epic
+ * #3473 B9). Before this helper, each CLI command's no-op arm hand-wrote
+ * both halves independently, and #3957's own sweep found sites where one
+ * half drifted: a decline that discarded values it had already computed, a
+ * `reason` string naming a condition that hadn't actually fired, or no
+ * stderr disclosure at all (missing the `[gsd-tools] WARNING:` convention
+ * #3217, ADR-3180 §7.6 rule 4, established for exactly this situation —
+ * a JSON `reason` field alone is easy for a caller piping stdout through
+ * `--json` to never read). Routing every no-op decline through one call
+ * site makes the convention structural — a call site, not a habit a fifth
+ * arm can independently forget — rather than leaving it to per-site
+ * discipline.
+ *
+ * `disclosure` is written to stderr VERBATIM. Like `error()`'s `message`
+ * argument (see that function's own doc comment above `formatDiagnosticToken`),
+ * this function stays a dumb, faithful writer and does not sanitize it — a
+ * caller that interpolates an UNTRUSTED substring (a caller-supplied
+ * blocker/session-field string, an argv token) into `disclosure` MUST pass
+ * that substring through `formatDiagnosticToken` first. Skipping that step
+ * lets an embedded `\n` forge a second, attacker-authored
+ * `[gsd-tools] WARNING:` line — the same risk `error()`'s doc comment
+ * documents for its own stderr write.
+ */
+function declineNoOp(
+  raw: boolean,
+  flagKey: string,
+  reason: string,
+  disclosure: string,
+  computed: Record<string, unknown> = {},
+): void {
+  // `process.stderr.write`, matching the `[gsd-tools] WARNING:` call sites
+  // this helper replaces (state.cts's stateReplaceFieldWithFallback and the
+  // cmdStateUpdateProgress decline arms) — not the `writeAllSync(2, …)`
+  // primitive `error()` uses, which is reserved for the fatal-exit path.
+  process.stderr.write(`[gsd-tools] WARNING: ${disclosure}\n`);
+  output({ [flagKey]: false, ...computed, reason }, raw, 'false');
+}
+
 /**
 * Frozen enum of typed reason codes used by error() for structured errors.
 * Each subcommand contributes its own codes; the enum exists so tests can
@@ -412,6 +453,7 @@ export = {
  ensureGsdTempDir,
  reapStaleTempFiles,
  output,
+  declineNoOp,
  serializeForOutput,
  ERROR_REASON,
  setJsonErrorMode,
--- a/src/roadmap.cts
+++ b/src/roadmap.cts
@@ -13,7 +13,7 @@ import { escapeRegex } from './pattern.cjs';
 import { splitLines, detectEol, joinLines } from './text-lines.cjs';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import ioMod = require('./io.cjs');
-const { output, error, formatDiagnosticToken } = ioMod;
+const { output, error, formatDiagnosticToken, declineNoOp } = ioMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import phaseIdMod = require('./phase-id.cjs');
 const { normalizePhaseName, phaseMarkdownRegexSource, matchPhaseDirs, stripProjectCodePrefix, OPTIONAL_PHASE_TAG_SOURCE, roadmapPhaseLookupSources, phaseHeadingPrefixSrcFor, PHASE_HEADING_BASELINE, isSentinelPhaseId, scopeToPhase, bracketQualifiedKey, foldBracketId } = phaseIdMod;
@@ -943,7 +943,13 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und
  const summaryCount = countMatchedSummaries(phaseInfo!.plans, phaseInfo!.summaries);

  if (planCount === 0) {
-    output({ updated: false, reason: 'No plans found', plan_count: 0, summary_count: 0 }, raw, 'no plans');
+    declineNoOp(
+      raw,
+      'updated',
+      'No plans found',
+      'roadmap update-plan-progress skipped — no plans found for this phase. ROADMAP.md was left unchanged.',
+      { plan_count: 0, summary_count: 0 },
+    );
    return;
  }

@@ -987,13 +993,26 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und
  const today = realClock.localToday();

  if (!fs.existsSync(roadmapPath)) {
-    output({ updated: false, reason: 'ROADMAP.md not found', plan_count: planCount, summary_count: summaryCount }, raw, 'no roadmap');
+    declineNoOp(
+      raw,
+      'updated',
+      'ROADMAP.md not found',
+      'roadmap update-plan-progress skipped — ROADMAP.md not found.',
+      { plan_count: planCount, summary_count: summaryCount },
+    );
    return;
  }

  // Wrap entire read-modify-write in lock to prevent concurrent corruption
+  let updated = false;
  withPlanningLock(cwd, () => {
-    let roadmapContent = fs.readFileSync(roadmapPath, 'utf-8');
+    // #3957 (B9.4): captured BEFORE any transform runs, so the write/report
+    // decision below reflects whether the transforms actually changed
+    // anything — not just that they ran. Every transform below still runs
+    // unconditionally exactly as before; only the final write-and-report
+    // step becomes conditional on `roadmapContent !== originalContent`.
+    const originalContent = fs.readFileSync(roadmapPath, 'utf-8');
+    let roadmapContent = originalContent;
    const phasePattern = phaseMarkdownRegexSource(phaseNum);

    // Progress table row: update Plans Complete/Status/Completed columns BY
@@ -1217,17 +1236,36 @@ function cmdRoadmapUpdatePlanProgress(cwd: string, phaseNum: string | null | und
      }
    }

-    platformWriteSync(roadmapPath, roadmapContent);
+    // #3957 (B9.4): write and report an update only when the transforms
+    // above actually produced different bytes — mirroring the sibling
+    // `cmdRoadmapAnnotateDependencies`'s existing `nextContent !== content`
+    // gate. Previously this wrote and reported `updated: true`
+    // unconditionally, even on an idempotent re-run that changed nothing.
+    if (roadmapContent !== originalContent) {
+      platformWriteSync(roadmapPath, roadmapContent);
+      updated = true;
+    }
  });
-  output({
-    updated: true,
+
+  const computed = {
    phase: phaseNum,
    plan_count: planCount,
    summary_count: summaryCount,
    status,
    complete: isComplete,
    verification_stale_check_indeterminate: verificationStaleCheckIndeterminate,
-  }, raw, `${summaryCount}/${planCount} ${status}`);
+  };
+  if (updated) {
+    output({ updated: true, ...computed }, raw, `${summaryCount}/${planCount} ${status}`);
+  } else {
+    declineNoOp(
+      raw,
+      'updated',
+      "no changes were needed — ROADMAP.md already reflects this phase's plan/summary counts and status",
+      "roadmap update-plan-progress skipped — no changes were needed; ROADMAP.md already reflects this phase's plan/summary counts and status.",
+      computed,
+    );
+  }
 }

 // ─── cmdRoadmapAnnotateDependencies ───────────────────────────────────────────
@@ -1252,13 +1290,33 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u

  const roadmapPath = planningPaths(cwd).roadmap;
  if (!fs.existsSync(roadmapPath)) {
-    output({ updated: false, reason: 'ROADMAP.md not found' }, raw, 'no roadmap');
+    declineNoOp(raw, 'updated', 'ROADMAP.md not found', 'roadmap annotate-dependencies skipped — ROADMAP.md not found.');
    return;
  }

  const phaseInfo = findPhaseInternal(cwd, phaseNum);
-  if (!phaseInfo || phaseInfo.plans.length === 0) {
-    output({ updated: false, reason: 'no plans found for phase', phase: phaseNum }, raw, 'no plans');
+  // #3957 (B9.1): distinguish "phase does not resolve at all" from "phase
+  // resolves but has zero plans" — previously both collapsed into the same
+  // 'no plans found for phase' reason, which is simply false for the first
+  // case (there IS no such phase to have plans).
+  if (!phaseInfo) {
+    declineNoOp(
+      raw,
+      'updated',
+      `phase ${phaseNum} not found`,
+      `roadmap annotate-dependencies skipped — phase ${formatDiagnosticToken(String(phaseNum))} not found.`,
+      { phase: phaseNum },
+    );
+    return;
+  }
+  if (phaseInfo.plans.length === 0) {
+    declineNoOp(
+      raw,
+      'updated',
+      `phase ${phaseNum} has no plans`,
+      `roadmap annotate-dependencies skipped — phase ${formatDiagnosticToken(String(phaseNum))} has no plans.`,
+      { phase: phaseNum },
+    );
    return;
  }

@@ -1277,7 +1335,12 @@ function cmdRoadmapAnnotateDependencies(cwd: string, phaseNum: string | null | u
  }

  if (planData.length === 0) {
-    output({ updated: false, reason: 'could not read plan frontmatter' }, raw, 'no frontmatter');
+    declineNoOp(
+      raw,
+      'updated',
+      'could not read plan frontmatter',
+      'roadmap annotate-dependencies skipped — could not read plan frontmatter for any plan in this phase.',
+    );
    return;
  }

--- a/src/state.cts
+++ b/src/state.cts
@@ -11,7 +11,7 @@ import path from 'node:path';
 import { escapeRegex } from './pattern.cjs';
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import ioMod = require('./io.cjs');
-const { output, error } = ioMod;
+const { output, error, declineNoOp, formatDiagnosticToken } = ioMod;
 // eslint-disable-next-line @typescript-eslint/no-require-imports
 import cliExitModule = require('./cli-exit.cjs');
 const { ExitError } = cliExitModule;
@@ -1268,12 +1268,15 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
    // never read, and STATE.md's Progress field goes stale with no
    // user-visible signal beyond it. Mirrors the established
    // `[gsd-tools] WARNING:` stderr convention this file already uses
-    // (stateReplaceFieldWithFallback above) for a comparable silent no-op.
-    process.stderr.write(
-      `[gsd-tools] WARNING: state update-progress skipped — phase scope is ${phaseScope}, not complete. ` +
-      `STATE.md's Progress field was left unchanged.\n`
+    // (stateReplaceFieldWithFallback above) for a comparable silent no-op —
+    // now routed through the shared `declineNoOp` helper (#3957) so the
+    // pairing is structural rather than hand-written per arm.
+    declineNoOp(
+      raw,
+      'updated',
+      `phase scope is ${phaseScope}, not complete`,
+      `state update-progress skipped — phase scope is ${phaseScope}, not complete. STATE.md's Progress field was left unchanged.`,
    );
-    output({ updated: false, reason: `phase scope is ${phaseScope}, not complete` }, raw, 'false');
    return;
  }

@@ -1286,14 +1289,11 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
  // ("nothing to measure" ≠ "0% done"). The legitimate 0% case (plans exist,
  // none summarized → clampPercent(0, N>0) = 0) is unaffected: totalPlans > 0.
  if (totalPlans === 0) {
-    process.stderr.write(
-      `[gsd-tools] WARNING: state update-progress skipped — no plans found in current-milestone phases (0 plans). ` +
-      `STATE.md's Progress field was left unchanged (milestone archived?).\n`
-    );
-    output(
-      { updated: false, reason: 'no plans found in current-milestone phases — STATE.md left unchanged (milestone archived?)' },
+    declineNoOp(
      raw,
-      'false',
+      'updated',
+      'no plans found in current-milestone phases — STATE.md left unchanged (milestone archived?)',
+      `state update-progress skipped — no plans found in current-milestone phases (0 plans). STATE.md's Progress field was left unchanged (milestone archived?).`,
    );
    return;
  }
@@ -1306,8 +1306,7 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
  // disagrees with its own completed/total.
  const preview = computeUpdateProgressPreview(statePath, cwd);
  if (preview.withheld) {
-    process.stderr.write(`[gsd-tools] WARNING: state update-progress skipped — ${preview.reason}\n`);
-    output({ updated: false, reason: preview.reason }, raw, 'false');
+    declineNoOp(raw, 'updated', preview.reason, `state update-progress skipped — ${preview.reason}`);
    return;
  }
  const { percent, completedPlans: fmCompletedPlans, totalPlans: fmTotalPlans } = preview;
@@ -1351,7 +1350,19 @@ function cmdStateUpdateProgress(cwd: string, raw: boolean): void {
  if (updated) {
    output({ updated: true, percent, completed: fmCompletedPlans, total: fmTotalPlans, bar: progressStr }, raw, progressStr);
  } else {
-    output({ updated: false, reason: 'Progress field not found in STATE.md' }, raw, 'false');
+    // #3957: the frontmatter progress data was already confirmed present a
+    // few lines above (computeUpdateProgressPreview didn't withhold) — what's
+    // actually missing here is the BODY `Progress:`/`**Progress:**` line
+    // itself. The prior 'Progress field not found in STATE.md' reason named
+    // the wrong layer and silently discarded percent/completed/total, which
+    // the sibling success arm above reports from the same preview.
+    declineNoOp(
+      raw,
+      'updated',
+      'no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected)',
+      'state update-progress skipped — no Progress: line found in STATE.md body to update (frontmatter progress data is unaffected).',
+      { percent, completed: fmCompletedPlans, total: fmTotalPlans },
+    );
  }
 }

@@ -1647,7 +1658,15 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void {
  if (!fs.existsSync(statePath)) { output({ error: 'STATE.md not found' }, raw, undefined); return; }
  if (!text) { output({ error: 'text required' }, raw, undefined); return; }

-  let resolved = false;
+  // #3957: track section-found and bullet-matched SEPARATELY. Previously
+  // `resolved` was set unconditionally as soon as the heading was located —
+  // before checking whether any bullet line actually matched `text` — so a
+  // call naming a non-existent blocker reported `resolved: true` (a false
+  // success). Only a real bullet match makes `resolved` true and the
+  // rewrite happen; otherwise the transform returns `content` unchanged
+  // (this repo's established no-op-return idiom).
+  let sectionFound = false;
+  let matched = false;

  readModifyWriteStateMd(statePath, (content) => {
    // ADR-1372 T6: find Blockers/Concerns section via tokenizeHeadings; stop at level 2 or 3.
@@ -1656,6 +1675,7 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void {
    const i = hs.findIndex(h => (h.level === 2 || h.level === 3) && /^(?:Blockers|Blockers\/Concerns|Concerns)$/i.test(h.text));
    if (i === -1) return content;

+    sectionFound = true;
    const h = hs[i];
    const ls = content.split('\n');
    const hl = ls[h.line - 1];
@@ -1668,23 +1688,43 @@ function cmdStateResolveBlocker(cwd: string, text: string, raw: boolean): void {
    const lines = sectionBody.split('\n');
    const filtered = lines.filter(line => {
      if (!line.startsWith('- ')) return true;
-      return !line.toLowerCase().includes(text.toLowerCase());
+      // Case-insensitive substring match — unchanged from before the fix;
+      // only whether a match occurred is now tracked accurately.
+      const isMatch = line.toLowerCase().includes(text.toLowerCase());
+      if (isMatch) matched = true;
+      return !isMatch;
    });

+    if (!matched) return content;
+
    let newBody = filtered.join('\n');
    // If section is now empty, add placeholder
    if (!newBody.trim() || !newBody.includes('- ')) {
      newBody = 'None\n';
    }

-    resolved = true;
    return content.slice(0, bs) + newBody + content.slice(se);
  }, cwd);

-  if (resolved) {
+  if (matched) {
    output({ resolved: true, blocker: text }, raw, 'true');
+  } else if (!sectionFound) {
+    declineNoOp(
+      raw,
+      'resolved',
+      'no Blockers/Concerns section found in STATE.md',
+      'state resolve-blocker skipped — no Blockers/Concerns section found in STATE.md.',
+    );
  } else {
-    output({ resolved: false, reason: 'Blockers section not found in STATE.md' }, raw, 'false');
+    // `formatDiagnosticToken` only guards the STDERR disclosure — the JSON
+    // `reason` field can embed `text` raw since output()'s own
+    // JSON.stringify serialization already escapes it correctly.
+    declineNoOp(
+      raw,
+      'resolved',
+      `no blocker matching ${text} found in the Blockers section`,
+      `state resolve-blocker skipped — no blocker matching ${formatDiagnosticToken(text)} found in the Blockers section.`,
+    );
  }
 }

@@ -1917,8 +1957,31 @@ function cmdStateRecordSession(cwd: string, options: StateRecordSessionOptions,
    const result: Record<string, unknown> = { recorded: true, updated: reconciledUpdated };
    if (sessionCreated) result['created'] = true;
    output(result, raw, 'true');
+  } else if (updated.length === 0) {
+    // Nothing was ever attempted — no --stopped-at/--resume-file supplied
+    // and no existing Last session/Last Date/Stopped At/Resume File labels
+    // to touch.
+    declineNoOp(
+      raw,
+      'recorded',
+      'no session fields found in STATE.md to update',
+      'state record-session skipped — no session fields found in STATE.md to update.',
+    );
  } else {
-    output({ recorded: false, reason: 'No session fields found in STATE.md' }, raw, 'false');
+    // #3957: `updated` (pre-reconciliation) was non-empty — a rewrite
+    // matched a session field and reported it as changed — but
+    // `reconcileReportedFields` found the persisted bytes byte-identical to
+    // what was already on disk (the matched field's supplied value equals
+    // its already-recorded value), so nothing actually changed. Distinct
+    // from the "nothing was ever attempted" case above: the prior single
+    // reason collapsed both into 'No session fields found in STATE.md',
+    // which was simply wrong for this case.
+    declineNoOp(
+      raw,
+      'recorded',
+      'the matched session field(s) already held the reported value — no bytes changed',
+      'state record-session skipped — the matched session field(s) already held the reported value; no bytes changed.',
+    );
  }
 }