fix(3184): address pr-review-toolkit + codex review findings
- Export isPhaseUatPassed and supporting types/enums from SDK public surface - Remove /m flag from frontmatter regex to prevent mid-body strip - Implement reasonsHuman population with per-ReasonCode humanizer - Add test for multiple UAT files in same phase - Add test for INVALID_PHASE_NUM error path - Add test for workstream routing - Eliminate redundant stripMarkdownInjection call - Update stale cycle-number comments Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -343,6 +343,20 @@ export type { WSTransportOptions } from './ws-transport.js';
|
||||
// Query registry argv normalization (matches `gsd-sdk query` and `GSDTools` hot path)
|
||||
export { createRegistry, normalizeQueryCommand } from './query/index.js';
|
||||
|
||||
// Phase UAT predicate — programmatic API surface (#3184)
|
||||
export {
|
||||
isPhaseUatPassed,
|
||||
phaseUatPassed,
|
||||
REASON_CODE,
|
||||
ERROR_CODE,
|
||||
PhaseUatPassedError,
|
||||
} from './query/phase-uat-passed.js';
|
||||
export type {
|
||||
UatReason,
|
||||
ReasonCode,
|
||||
ErrorCode,
|
||||
} from './query/phase-uat-passed.js';
|
||||
|
||||
// Workstream utilities
|
||||
export { validateWorkstreamName, relPlanningPath } from './workstream-utils.js';
|
||||
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
/**
|
||||
* Unit tests for isPhaseUatPassed — walking skeleton (cycle 1 of ~15).
|
||||
* Tests for isPhaseUatPassed across UAT result classification paths.
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
@@ -438,6 +438,132 @@ result: PASS
|
||||
await rm(localTmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('preserves UAT body content when file contains an internal --- horizontal rule', async () => {
|
||||
// Regression: frontmatter regex with /m flag treated mid-body --- as a
|
||||
// second frontmatter block and stripped everything between them.
|
||||
const localTmp = await mkdtemp(join(tmpdir(), 'gsd-uat-hrule-'));
|
||||
try {
|
||||
const phaseDir = join(localTmp, '.planning', 'phases', '05-hrule');
|
||||
await mkdir(phaseDir, { recursive: true });
|
||||
const content = `---
|
||||
status: complete
|
||||
phase: 5
|
||||
---
|
||||
|
||||
### 1. Item above rule
|
||||
expected: works
|
||||
result: pass
|
||||
|
||||
---
|
||||
|
||||
### 2. Item below rule
|
||||
expected: also works
|
||||
result: pass
|
||||
`;
|
||||
await writeFile(join(phaseDir, '05-HUMAN-UAT.md'), content);
|
||||
|
||||
const result = await isPhaseUatPassed(localTmp, '5');
|
||||
expect(result.passed).toBe(true);
|
||||
expect(result.items.length).toBe(2);
|
||||
expect(result.items[0].name).toBe('Item above rule');
|
||||
expect(result.items[1].name).toBe('Item below rule');
|
||||
} finally {
|
||||
await rm(localTmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('aggregates items and reasons across multiple UAT files in the same phase', async () => {
|
||||
// Finding #4: multiple *-HUMAN-UAT.md files — all-pass + one failure
|
||||
const localTmp = await mkdtemp(join(tmpdir(), 'gsd-uat-multi-'));
|
||||
try {
|
||||
const phaseDir = join(localTmp, '.planning', 'phases', '05-multi');
|
||||
await mkdir(phaseDir, { recursive: true });
|
||||
|
||||
const passFile = `---
|
||||
status: complete
|
||||
phase: 5
|
||||
---
|
||||
|
||||
### 1. Passing check
|
||||
expected: it works
|
||||
result: pass
|
||||
`;
|
||||
const failFile = `---
|
||||
status: complete
|
||||
phase: 5
|
||||
---
|
||||
|
||||
### 1. Failing check
|
||||
expected: it works
|
||||
result: issue
|
||||
|
||||
### 2. Another pass
|
||||
expected: also works
|
||||
result: pass
|
||||
`;
|
||||
await writeFile(join(phaseDir, '05a-HUMAN-UAT.md'), passFile);
|
||||
await writeFile(join(phaseDir, '05b-HUMAN-UAT.md'), failFile);
|
||||
|
||||
const result = await isPhaseUatPassed(localTmp, '5');
|
||||
// Total items: 1 from first file + 2 from second file
|
||||
expect(result.items.length).toBe(3);
|
||||
// One NON_PASS_RESULT reason from the failing item
|
||||
expect(result.reasons.length).toBe(1);
|
||||
expect(result.reasons[0].code).toBe(REASON_CODE.NON_PASS_RESULT);
|
||||
expect(result.passed).toBe(false);
|
||||
// reasonsHuman must be non-empty
|
||||
expect(result.reasonsHuman.length).toBe(1);
|
||||
expect(typeof result.reasonsHuman[0]).toBe('string');
|
||||
expect(result.reasonsHuman[0].length).toBeGreaterThan(0);
|
||||
} finally {
|
||||
await rm(localTmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('throws PhaseUatPassedError with INVALID_PHASE_NUM when phase arg is empty', async () => {
|
||||
// Finding #5: INVALID_PHASE_NUM error path via phaseUatPassed handler
|
||||
const { phaseUatPassed, ERROR_CODE: EC } = await import('./phase-uat-passed.js');
|
||||
const localTmp = await mkdtemp(join(tmpdir(), 'gsd-uat-invalidphase-'));
|
||||
try {
|
||||
let thrown: unknown;
|
||||
try {
|
||||
await phaseUatPassed([], localTmp);
|
||||
} catch (e) {
|
||||
thrown = e;
|
||||
}
|
||||
expect(thrown).toBeInstanceOf(PhaseUatPassedError);
|
||||
expect((thrown as PhaseUatPassedError).code).toBe(EC.INVALID_PHASE_NUM);
|
||||
} finally {
|
||||
await rm(localTmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
it('resolves isPhaseUatPassed via workstream routing', async () => {
|
||||
// Finding #6: workstream-keyed project under .planning/workstreams/ws1/phases/
|
||||
const localTmp = await mkdtemp(join(tmpdir(), 'gsd-uat-ws-'));
|
||||
try {
|
||||
const wsPhaseDir = join(localTmp, '.planning', 'workstreams', 'ws1', 'phases', '05-ws-phase');
|
||||
await mkdir(wsPhaseDir, { recursive: true });
|
||||
const content = `---
|
||||
status: complete
|
||||
phase: 5
|
||||
---
|
||||
|
||||
### 1. WS item
|
||||
expected: works in workstream
|
||||
result: pass
|
||||
`;
|
||||
await writeFile(join(wsPhaseDir, '05-HUMAN-UAT.md'), content);
|
||||
|
||||
const result = await isPhaseUatPassed(localTmp, '5', 'ws1');
|
||||
expect(result.passed).toBe(true);
|
||||
expect(result.items.length).toBe(1);
|
||||
expect(result.items[0].name).toBe('WS item');
|
||||
} finally {
|
||||
await rm(localTmp, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('phase.uat-passed registry wire-up (cycle 16)', () => {
|
||||
|
||||
@@ -1,8 +1,6 @@
|
||||
/**
|
||||
* isPhaseUatPassed — SDK predicate answering "is phase N's UAT contract satisfied?"
|
||||
*
|
||||
* Cycle 2 of ~15: introduces REASON_CODE frozen enum and UatReason typed shape.
|
||||
* Non-pass items (result not literally 'pass') emit a typed NON_PASS_RESULT reason.
|
||||
* Implements the isPhaseUatPassed predicate per #3184.
|
||||
*/
|
||||
|
||||
import { readFile, readdir, stat } from 'node:fs/promises';
|
||||
@@ -64,8 +62,9 @@ interface UatItem {
|
||||
* Passes are applied in order; each returns a sanitised string.
|
||||
*/
|
||||
function stripMarkdownInjection(content: string): string {
|
||||
// Pass 1: strip YAML frontmatter region (---\n...\n---)
|
||||
let s = content.replace(/^---\r?\n[\s\S]*?\r?\n---/m, '');
|
||||
// Pass 1: strip YAML frontmatter region (---\n...\n---). No /m flag — ^ must
|
||||
// anchor to position 0 only, preventing mid-body --- separators from triggering.
|
||||
let s = content.replace(/^---\r?\n[\s\S]*?\r?\n---/, '');
|
||||
// Pass 2: strip fenced code blocks (``` ... ```)
|
||||
s = s.replace(/```[\s\S]*?```/g, '');
|
||||
// Pass 3: strip HTML comment regions (<!-- ... -->)
|
||||
@@ -75,8 +74,11 @@ function stripMarkdownInjection(content: string): string {
|
||||
return s;
|
||||
}
|
||||
|
||||
function parseAllUatItems(content: string): UatItem[] {
|
||||
const sanitised = stripMarkdownInjection(content);
|
||||
/**
|
||||
* Parse all UAT items from already-stripped content.
|
||||
* Callers must pass pre-stripped (sanitised) content — no internal strip is performed.
|
||||
*/
|
||||
function parseAllUatItems(sanitised: string): UatItem[] {
|
||||
const items: UatItem[] = [];
|
||||
UAT_ITEM_PATTERN.lastIndex = 0;
|
||||
let m: RegExpMatchArray | null;
|
||||
@@ -93,6 +95,30 @@ function parseAllUatItems(content: string): UatItem[] {
|
||||
return items;
|
||||
}
|
||||
|
||||
/** Build a human-readable string for a single UatReason. */
|
||||
function humanizeReason(r: UatReason): string {
|
||||
switch (r.code) {
|
||||
case REASON_CODE.CASE_MISMATCH:
|
||||
return `"${r.itemName ?? 'item'}" has a case-mismatched result ("${r.capturedValue ?? ''}" — expected "pass")`;
|
||||
case REASON_CODE.NON_PASS_RESULT:
|
||||
return `"${r.itemName ?? 'item'}" has result "${r.capturedValue ?? 'unknown'}" (expected "pass")`;
|
||||
case REASON_CODE.NO_ITEMS_EXTRACTED:
|
||||
return `${r.file ?? 'file'} contained no UAT items`;
|
||||
case REASON_CODE.NO_PHASE_DIR:
|
||||
return 'Phase directory not found';
|
||||
case REASON_CODE.NO_UAT_FILES:
|
||||
return 'No *-HUMAN-UAT.md files found in the phase directory';
|
||||
case REASON_CODE.HUMAN_VERIFICATION_NEEDED:
|
||||
return `"${r.itemName ?? 'item'}" requires manual human verification`;
|
||||
case REASON_CODE.BRACKETED_PLACEHOLDER:
|
||||
return `"${r.itemName ?? 'item'}" has an unfilled placeholder result: ${r.capturedValue ?? ''}`;
|
||||
case REASON_CODE.ORPHAN_ITEM_MISSING_RESULT:
|
||||
return `"${r.itemName ?? 'item'}" is missing a result field`;
|
||||
default:
|
||||
return `${r.code}: ${JSON.stringify(r)}`;
|
||||
}
|
||||
}
|
||||
|
||||
const HEADING_PATTERN = /###\s*(\d+)\.\s*([^\n]+)/g;
|
||||
|
||||
/**
|
||||
@@ -144,10 +170,11 @@ export async function isPhaseUatPassed(
|
||||
|
||||
const dir = await resolvePhaseDir(phase, projectDir, workstream);
|
||||
if (!dir) {
|
||||
const reasons: UatReason[] = [{ code: REASON_CODE.NO_PHASE_DIR }];
|
||||
return {
|
||||
passed: false,
|
||||
reasons: [{ code: REASON_CODE.NO_PHASE_DIR }],
|
||||
reasonsHuman: [],
|
||||
reasons,
|
||||
reasonsHuman: reasons.map(humanizeReason),
|
||||
items: [],
|
||||
};
|
||||
}
|
||||
@@ -156,10 +183,11 @@ export async function isPhaseUatPassed(
|
||||
const uatFiles = files.filter((f) => f.endsWith('-HUMAN-UAT.md'));
|
||||
|
||||
if (uatFiles.length === 0) {
|
||||
const reasons: UatReason[] = [{ code: REASON_CODE.NO_UAT_FILES }];
|
||||
return {
|
||||
passed: false,
|
||||
reasons: [{ code: REASON_CODE.NO_UAT_FILES }],
|
||||
reasonsHuman: [],
|
||||
reasons,
|
||||
reasonsHuman: reasons.map(humanizeReason),
|
||||
items: [],
|
||||
};
|
||||
}
|
||||
@@ -174,7 +202,7 @@ export async function isPhaseUatPassed(
|
||||
const strippedBody = stripMarkdownInjection(content);
|
||||
const itemsBeforeFile = items.length;
|
||||
const reasonsBeforeFile = reasons.length;
|
||||
const parsed = parseAllUatItems(content);
|
||||
const parsed = parseAllUatItems(strippedBody);
|
||||
for (const item of parsed) {
|
||||
items.push(item);
|
||||
if (item.result !== 'pass') {
|
||||
@@ -247,7 +275,7 @@ export async function isPhaseUatPassed(
|
||||
|
||||
const passed = items.length > 0 && reasons.length === 0;
|
||||
|
||||
return { passed, reasons, reasonsHuman: [], items };
|
||||
return { passed, reasons, reasonsHuman: reasons.map(humanizeReason), items };
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user