feat: add gates ensuring discuss-phase decisions are translated to plans and verified (closes #2492) (#2611)

* feat(#2492): add gates ensuring discuss-phase decisions are translated and verified Two gates close the loop between CONTEXT.md `<decisions>` and downstream work, fixing #2492: - Plan-phase **translation gate** (BLOCKING). After requirements coverage, refuses to mark a phase planned when a trackable decision is not cited (by id `D-NN` or by 6+-word phrase) in any plan's `must_haves`, `truths`, or body. Failure message names each missed decision with id, category, text, and remediation paths. - Verify-phase **validation gate** (NON-BLOCKING). Searches plans, SUMMARY.md, files modified, and recent commit subjects for each trackable decision. Misses are written to VERIFICATION.md as a warning section but do not change verification status. Asymmetry is deliberate — fuzzy-match miss should not fail an otherwise green phase. Shared helper `parseDecisions()` lives in `sdk/src/query/decisions.ts` so #2493 can consume the same parser. Decisions opt out of both gates via `### Claude's Discretion` heading or `[informational]` / `[folded]` / `[deferred]` tags. Both gates skip silently when `workflow.context_coverage_gate=false` (default `true`). Closes #2492 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#2492): make plan-phase decision gate actually block (review F1, F8, F9, F10, F15) - F1: replace `${context_path}` with `${CONTEXT_PATH}` in the plan-phase gate snippet so the BLOCKING gate receives a non-empty path. The variable was defined in Step 4 (`CONTEXT_PATH=$(_gsd_field "$INIT" ...)`) and the gate snippet referenced the lowercase form, leaving the gate to run with an empty path argument and silently skip. - F15: wrap the SDK call with `jq -e '.data.passed == true' || exit 1` so failure halts the workflow instead of being printed and ignored. The verify-phase counterpart deliberately keeps no exit-1 (non-blocking by design) and now carries an inline note documenting the asymmetry. - F10: tag the JSON example fence as `json` and the options-list fence as `text` (MD040). - F8/F9: anchor the heading-presence test regexes to `^## 13[a-z]?\\.` so prose substrings like "Requirements Coverage Gate" mentioned in body text cannot satisfy the assertion. Added two new regression tests (variable-name match, exit-1 guard) so a future revert is caught. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#2492): tighten decision-coverage gates against false positives and config drift (review F3,F4,F5,F6,F7,F16,F18,F19) - F3: forward `workstream` arg through both gate handlers so workstream-scoped `workflow.context_coverage_gate=false` actually skips. Added negative test that creates a workstream config disabling the gate while the root config has it enabled and asserts the workstream call is skipped. - F4: restrict the plan-phase haystack to designated sections — front-matter `must_haves` / `truths` / `objective` plus body sections under headings matching `must_haves|truths|tasks|objective`. HTML comments and fenced code blocks are stripped before extraction so a commented-out citation or a literal example never counts as coverage. Verify-phase keeps the broader artifact-wide haystack by design (non-blocking). - F5: reject decisions with fewer than 6 normalized words from soft-matching (previously only rejected when the resulting phrase was under 12 chars AFTER slicing — too lenient). Short decisions now require an explicit `D-NN` citation, with regression tests for the boundary. - F6: walk every `*-SUMMARY.md` independently and use `matchAll` with the `/g` flag so multiple `files_modified:` blocks across multiple summaries are all aggregated. Previously only the first block in the concatenated string was parsed, silently dropping later plans' files. - F7: validate every `files_modified` path stays inside `projectDir` after resolution (rejects absolute paths, `../` traversal). Cap each file read at 256 KB. Skipped paths emit a stderr warning naming the entry. - F16: validate `workflow.context_coverage_gate` is boolean in `loadGateConfig`; warn loudly on numeric or other-shaped values and default to ON. Mirrors the schema-vs-loadConfig validation gap from #2609. - F18: bump verify-phase `git log -n` cap from 50 to 200 so longer-running phases are not undercounted. Documented as a precision-vs-recall tradeoff appropriate for a non-blocking gate. - F19: tighten `QueryResult` / `QueryHandler` to be parameterized (`<T = unknown>`). Drops the `as unknown as Record<string, unknown>` casts in the gate handlers and surfaces shape mismatches at compile time for callers that pass a typed `data` value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#2492): harden decisions parser and verify-phase glob (review F11,F12,F13,F14,F17,F20) - F11: strip fenced code blocks from CONTEXT.md before searching for `<decisions>` so an example block inside ``` ``` is not mis-parsed. - F12: accept tab-indented continuation lines (previously required a leading space) so decisions split with `\t` continue cleanly. - F13: parse EVERY `<decisions>` block in the file via `matchAll`, not just the first. CONTEXT.md may legitimately carry more than one block. - F14: `decisions.parse` handler now resolves a relative path against `projectDir` — symmetric with the gate handlers — and still accepts absolute paths. - F17: replace `ls "${PHASE_DIR}"/*-CONTEXT.md | head -1` in verify-phase.md with a glob loop (ShellCheck SC2012 fix). Also avoids spawning an extra subprocess and survives filenames with whitespace. - F20: extend the unicode quote-stripping in the discretion-heading match to cover U+2018/2019/201A/201B and the U+201C-F double-quote variants plus backtick, so any rendering of "Claude's Discretion" collapses to the same key. Each fix has a regression test in `decisions.test.ts`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-23 00:26:53 -04:00
parent 1a3d953767
commit f30da8326a
16 changed files with 1900 additions and 4 deletions
--- a/sdk/src/query/decisions.test.ts
+++ b/sdk/src/query/decisions.test.ts
@@ -0,0 +1,215 @@
+/**
+ * Unit tests for CONTEXT.md `<decisions>` parser.
+ *
+ * Decision format (from `discuss-phase.md` lines 1035–1048):
+ *
+ *   <decisions>
+ *   ## Implementation Decisions
+ *
+ *   ### Category A
+ *   - **D-01:** First decision text
+ *   - **D-02 [folded]:** Second decision text
+ *
+ *   ### Claude's Discretion
+ *   - free-form, never tracked
+ *
+ *   ### Folded Todos
+ *   - **D-03 [folded]:** ...
+ *   </decisions>
+ *
+ * Issue #2492.
+ */
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { parseDecisions } from './decisions.js';
+
+const MINIMAL = `# Phase 17 Context
+
+<decisions>
+## Implementation Decisions
+
+### API Surface
+- **D-01:** Use bit offsets, not byte offsets
+- **D-02:** Display TArray element type alongside count
+
+### Storage
+- **D-03 [informational]:** Backing store is on disk
+- **D-04:** Persist via SQLite WAL mode
+
+### Claude's Discretion
+- Naming of internal helpers is up to the implementer
+- **D-99:** This should be ignored — it lives under Discretion
+
+### Folded Todos
+- **D-05 [folded]:** Add a CLI flag for verbose mode
+</decisions>
+`;
+
+describe('parseDecisions (#2492)', () => {
+  it('extracts D-NN decisions with id, text, and category', () => {
+    const decisions = parseDecisions(MINIMAL);
+    const ids = decisions.map((d) => d.id);
+    expect(ids).toContain('D-01');
+    expect(ids).toContain('D-02');
+    expect(ids).toContain('D-04');
+    const d01 = decisions.find((d) => d.id === 'D-01');
+    expect(d01?.text).toBe('Use bit offsets, not byte offsets');
+    expect(d01?.category).toBe('API Surface');
+  });
+
+  it('captures bracketed tags', () => {
+    const decisions = parseDecisions(MINIMAL);
+    const d05 = decisions.find((d) => d.id === 'D-05');
+    expect(d05?.tags).toContain('folded');
+    const d03 = decisions.find((d) => d.id === 'D-03');
+    expect(d03?.tags).toContain('informational');
+  });
+
+  it('marks Claude\'s Discretion entries as non-trackable', () => {
+    const decisions = parseDecisions(MINIMAL);
+    const d99 = decisions.find((d) => d.id === 'D-99');
+    expect(d99).toBeDefined();
+    expect(d99?.trackable).toBe(false);
+    // And it must NOT appear in the trackable filter
+    const trackableIds = decisions.filter((d) => d.trackable).map((d) => d.id);
+    expect(trackableIds).not.toContain('D-99');
+  });
+
+  it('marks [informational] entries as opt-out (excluded from trackable by default)', () => {
+    const trackable = parseDecisions(MINIMAL).filter((d) => d.trackable);
+    const ids = trackable.map((d) => d.id);
+    expect(ids).toContain('D-01');
+    expect(ids).toContain('D-02');
+    expect(ids).toContain('D-04');
+    expect(ids).not.toContain('D-03'); // [informational] tag
+    expect(ids).not.toContain('D-05'); // [folded] tag — not user-facing decision
+  });
+
+  it('returns empty array when CONTEXT.md has no <decisions> block', () => {
+    expect(parseDecisions('# Phase 1\n\nNo decisions here.\n')).toEqual([]);
+  });
+
+  it('returns empty array when content is empty', () => {
+    expect(parseDecisions('')).toEqual([]);
+  });
+
+  it('returns empty array when <decisions> block is empty', () => {
+    expect(parseDecisions('<decisions>\n</decisions>')).toEqual([]);
+  });
+
+  it('does not crash on malformed bullet lines', () => {
+    const malformed = `<decisions>
+- not a decision (no D-NN)
+- **D-bogus:** wrong id format
+- **D-7:** single digit allowed
+- **D-10:** ten
+</decisions>`;
+    const decisions = parseDecisions(malformed);
+    const ids = decisions.map((d) => d.id);
+    expect(ids).toContain('D-7');
+    expect(ids).toContain('D-10');
+    expect(ids).not.toContain('D-bogus');
+  });
+
+  it('preserves multi-line decision text continuations', () => {
+    const multi = `<decisions>
+### Cat
+- **D-01:** First line
+  continues here
+- **D-02:** Second
+</decisions>`;
+    const decisions = parseDecisions(multi);
+    const d01 = decisions.find((d) => d.id === 'D-01');
+    expect(d01?.text).toMatch(/First line/);
+  });
+
+  // ─── Adversarial-review regressions ────────────────────────────────────
+
+  it('ignores `<decisions>` blocks inside fenced code (review F11)', () => {
+    const content = `# Doc
+
+\`\`\`
+<decisions>
+### Example
+- **D-99:** Should not be parsed
+</decisions>
+\`\`\`
+
+<decisions>
+### Real
+- **D-01:** Real decision text long enough to soft match
+</decisions>`;
+    const decisions = parseDecisions(content);
+    const ids = decisions.map((d) => d.id);
+    expect(ids).toContain('D-01');
+    expect(ids).not.toContain('D-99');
+  });
+
+  it('captures continuation lines indented with TABS (review F12)', () => {
+    const content = '<decisions>\n### Cat\n- **D-07:** First line\n\tcontinued via tab\n</decisions>';
+    const decisions = parseDecisions(content);
+    const d07 = decisions.find((d) => d.id === 'D-07');
+    expect(d07?.text).toMatch(/continued via tab/);
+  });
+
+  it('parses ALL `<decisions>` blocks, not just the first (review F13)', () => {
+    const content = `<decisions>
+### One
+- **D-01:** First batch
+</decisions>
+
+Some prose.
+
+<decisions>
+### Two
+- **D-02:** Second batch
+</decisions>`;
+    const ids = parseDecisions(content).map((d) => d.id);
+    expect(ids).toContain('D-01');
+    expect(ids).toContain('D-02');
+  });
+
+  it('treats curly-quote variants of "Claude\u2019s Discretion" as non-trackable (review F20)', () => {
+    // U+201B (single high-reversed-9 quotation mark) — uncommon but legal unicode.
+    const content =
+      '<decisions>\n### Claude\u201Bs Discretion\n- **D-50:** Should be non-trackable\n</decisions>';
+    const decisions = parseDecisions(content);
+    const d50 = decisions.find((d) => d.id === 'D-50');
+    expect(d50?.trackable).toBe(false);
+  });
+});
+
+// ─── decisions.parse query handler ────────────────────────────────────────
+
+import { decisionsParse } from './decisions.js';
+import { mkdtemp, writeFile, rm, mkdir } from 'node:fs/promises';
+import { join } from 'node:path';
+import { tmpdir } from 'node:os';
+
+describe('decisionsParse handler (review F14 — accepts relative path via projectDir)', () => {
+  let tmp: string;
+  beforeEach(async () => {
+    tmp = await mkdtemp(join(tmpdir(), 'gsd-decparse-'));
+  });
+  afterEach(async () => {
+    await rm(tmp, { recursive: true, force: true });
+  });
+
+  it('resolves a relative file path against projectDir', async () => {
+    await mkdir(join(tmp, '.planning', 'phases', '17'), { recursive: true });
+    await writeFile(
+      join(tmp, '.planning', 'phases', '17', '17-CONTEXT.md'),
+      '<decisions>\n### Cat\n- **D-01:** Hello\n</decisions>',
+      'utf-8',
+    );
+    const result = await decisionsParse(['.planning/phases/17/17-CONTEXT.md'], tmp);
+    expect((result.data as { trackable: number }).trackable).toBe(1);
+    expect((result.data as { missing: boolean }).missing).toBe(false);
+  });
+
+  it('still accepts an absolute path', async () => {
+    const abs = join(tmp, 'CONTEXT.md');
+    await writeFile(abs, '<decisions>\n### Cat\n- **D-02:** Bye\n</decisions>', 'utf-8');
+    const result = await decisionsParse([abs], tmp);
+    expect((result.data as { trackable: number }).trackable).toBe(1);
+  });
+});