diff --git a/src/resources/extensions/gsd/auto-dispatch.ts b/src/resources/extensions/gsd/auto-dispatch.ts index 981706481..4ad59530b 100644 --- a/src/resources/extensions/gsd/auto-dispatch.ts +++ b/src/resources/extensions/gsd/auto-dispatch.ts @@ -57,6 +57,8 @@ import { atomicWriteSync, removeProjectionFileSync } from "./atomic-write.js"; import { logWarning, logError } from "./workflow-logger.js"; import { dirname, join, sep } from "node:path"; import { hasImplementationArtifacts } from "./milestone-implementation-evidence.js"; +import { isFlatPhaseMigrationInFlight } from "./flat-phase-migration.js"; +import { composeToolAffordanceReminder } from "./unit-context-composer.js"; import { buildDiscussMilestonePrompt, buildDiscussProjectPrompt, @@ -853,6 +855,15 @@ export const DISPATCH_RULES: DispatchRule[] = [ // "never discussed" and re-dispatches discuss-milestone after task // closeout instead of continuing execution (#1317). const artifactBasePath = resolveArtifactBasePath(basePath, mid, session); + // The layout migration moves .gsd/milestones/ aside before rendering + // .gsd/phases/, so mid-flight the slice plans exist in neither layout. + // A dispatch landing in that window reads "never discussed" and re-plans a + // milestone that is already fully planned in the DB, discarding the plan. + // Startup dispatch races the migration on every run, so decline to decide + // while the layout is converting; the next iteration sees a settled tree. + // Scoped to a migration actually in flight — a settled legacy project must + // still reach this rule (#4671). + if (isFlatPhaseMigrationInFlight(artifactBasePath)) return null; if (hasMilestonePassedDiscuss(artifactBasePath, mid)) return null; // Align with the plan-v2 gate's lookup semantics: whitespace-only counts // as missing, and an auto worktree may fall back to GSD_PROJECT_ROOT. @@ -2074,6 +2085,22 @@ function applyLanguageDirectiveToDispatch( return { ...action, prompt: `${directive}\n\n${action.prompt}` }; } +/** + * Repeat the unit's allowed tool tokens at the very end of the dispatched prompt. + * + * `## Tool Surface` already carries this, but it lands ~4% into a 14K-character + * prompt (measured on validate-milestone: offset 627, 13,731 characters after it) + * and units kept reaching for `bash`, `gsd_uat_exec`, and subagent `"review"` + * regardless. Applied at the dispatch seam so every unit gets it from one place, + * rather than editing each prompt builder's tail. + */ +function appendToolAffordanceToDispatch(action: DispatchAction): DispatchAction { + if (action.action !== "dispatch" || !action.prompt || !action.unitType) return action; + const reminder = composeToolAffordanceReminder(action.unitType); + if (!reminder || action.prompt.trimEnd().endsWith(reminder)) return action; + return { ...action, prompt: `${action.prompt.trimEnd()}\n\n${reminder}` }; +} + // ─── Resolver ───────────────────────────────────────────────────────────── /** @@ -2170,7 +2197,7 @@ export async function resolveDispatch( level: "error", }; } - return applyLanguageDirectiveToDispatch(action, ctx.prefs); + return applyLanguageDirectiveToDispatch(appendToolAffordanceToDispatch(action), ctx.prefs); } for (const rule of DISPATCH_RULES) { @@ -2189,7 +2216,7 @@ export async function resolveDispatch( matchedRule: rule.name, }; } - return applyLanguageDirectiveToDispatch(action, ctx.prefs); + return applyLanguageDirectiveToDispatch(appendToolAffordanceToDispatch(action), ctx.prefs); } } diff --git a/src/resources/extensions/gsd/auto/loop.ts b/src/resources/extensions/gsd/auto/loop.ts index fe4c38253..44b4abe8d 100644 --- a/src/resources/extensions/gsd/auto/loop.ts +++ b/src/resources/extensions/gsd/auto/loop.ts @@ -33,6 +33,7 @@ import { } from "./unit-phase.js"; import { STUCK_WINDOW_SIZE } from "./dispatch-history.js"; import { debugLog } from "../debug-logger.js"; +import { markBlockedStopReason } from "../stop-notice.js"; import { isInfrastructureError, isTransientCooldownError, getCooldownRetryAfterMs, COOLDOWN_FALLBACK_WAIT_MS, MAX_COOLDOWN_RETRIES } from "./infra-errors.js"; import { ModelPolicyDispatchBlockedError } from "../auto-model-selection.js"; import { resolveEngine } from "../engine-resolver.js"; @@ -1194,7 +1195,10 @@ export async function autoLoop( }); finishTurn("paused", "manual-attention", "orchestration-blocked"); } else { - await deps.stopAuto(ctx, pi, blockMessage); + // Carry the blocked marker: the headless host picks its exit code + // from the reason string, so an unmarked blocked stop exits 0 and + // reports success over a milestone that never closed. + await deps.stopAuto(ctx, pi, markBlockedStopReason(blockMessage)); finishTurn("stopped", "manual-attention", "orchestration-blocked"); } finishIncompleteIteration({ diff --git a/src/resources/extensions/gsd/browser-evidence.ts b/src/resources/extensions/gsd/browser-evidence.ts index c58626e04..b8f796fd6 100644 --- a/src/resources/extensions/gsd/browser-evidence.ts +++ b/src/resources/extensions/gsd/browser-evidence.ts @@ -24,6 +24,13 @@ export const BROWSER_ACTION_RE = /\b(?:open(?:ed)?|navigate(?:d)?|click(?:ed)?|t export const BROWSER_ASSERTION_RE = /\b(?:assert(?:ed|ion)?|observed|confirmed|verified|expected|visible|text|count|label|strikethrough|localstorage|screenshot|snapshot|passed)\b/i; const NON_REQUIREMENT_BROWSER_HEADING_RE = /^(?:not\s+proven|not\s+covered|out\s+of\s+scope|deferred|follow-?ups?|known\s+limitations|notes\s+for\s+tester)\b/i; const NON_REQUIREMENT_BROWSER_LINE_RE = /\b(?:deferred|not\s+proven|not\s+covered|out\s+of\s+scope|future\s+slice|follow-?up|no\s+(?:live\s+)?browser|without\s+(?:a\s+)?browser|not\s+(?:a\s+)?browser)\b/i; +// The negations above only fire when the negator sits directly beside "browser". +// A negated *list* — "no runtime behavior, server, UI, or browser interaction is +// involved" — separates them, so the line fell through and matched +// `browser interaction` as a requirement. That escalated a slice whose UAT said +// browsers were irrelevant into one demanding browser verification. Bounded to a +// single clause so a genuine requirement in the next sentence still counts. +const NEGATED_BROWSER_CLAUSE_RE = /\b(?:no|without|not)\b[^.;:!?]{0,60}\bbrowser\b/i; export function compactTextParts(parts: Array): string { return parts.flatMap((part) => Array.isArray(part) ? part : [part]) @@ -51,7 +58,11 @@ export function hasBrowserRequiredText(text: string): boolean { if (!inNonRequirementSection && BROWSER_REQUIREMENT_RE.test(title)) return true; continue; } - if (inNonRequirementSection || NON_REQUIREMENT_BROWSER_LINE_RE.test(line)) continue; + if ( + inNonRequirementSection || + NON_REQUIREMENT_BROWSER_LINE_RE.test(line) || + NEGATED_BROWSER_CLAUSE_RE.test(line) + ) continue; if (BROWSER_REQUIREMENT_RE.test(line)) return true; } return false; diff --git a/src/resources/extensions/gsd/closeout-consistency-gate.ts b/src/resources/extensions/gsd/closeout-consistency-gate.ts index 26e258063..308bd2126 100644 --- a/src/resources/extensions/gsd/closeout-consistency-gate.ts +++ b/src/resources/extensions/gsd/closeout-consistency-gate.ts @@ -33,6 +33,7 @@ import { } from "./db/milestone-closeout-readiness.js"; import { loadEffectiveGSDPreferences } from "./preferences.js"; import { captureMilestoneVerificationSourceRevision } from "./verification-source-integrity.js"; +import { resolveCanonicalMilestoneRoot } from "./worktree-manager.js"; import { atomicWriteSync, removeProjectionFileSync } from "./atomic-write.js"; export const CLOSEOUT_CONSISTENCY_BLOCKED_REASON = "closeout-consistency-blocked"; @@ -214,7 +215,16 @@ export function checkCloseoutConsistencyGate( } let canonicalAuthorization = null; if (adoptedMilestone) { - const artifactBasePath = options.artifactBasePath ?? artifactBasePathFromDb(); + // Resolve the tree the milestone actually executed in. Under worktree + // isolation validation runs inside .gsd-worktrees/ and records that + // tree's revision; computing the current revision from the project root + // compares two different working trees, so the authorization could never + // match and the merge blocked with "validation authorization is not + // current" on a milestone the DB had already marked complete. + const artifactBasePath = resolveCanonicalMilestoneRoot( + options.artifactBasePath ?? artifactBasePathFromDb() ?? "", + milestoneId, + ) || options.artifactBasePath || artifactBasePathFromDb(); if (!artifactBasePath) { return blocked( "validation-not-pass", diff --git a/src/resources/extensions/gsd/flat-phase-migration.ts b/src/resources/extensions/gsd/flat-phase-migration.ts index 82e45856c..f761e8421 100644 --- a/src/resources/extensions/gsd/flat-phase-migration.ts +++ b/src/resources/extensions/gsd/flat-phase-migration.ts @@ -14,6 +14,7 @@ import { getMilestoneSlices, getSliceTasks, } from "./gsd-db.js"; +import { withFileLock } from "./file-lock.js"; import { countDbHierarchy, scanMarkdownHierarchy } from "./migration-auto-check.js"; import { logWarning } from "./workflow-logger.js"; import { LAYOUT_SEGMENTS } from "./layout-policy.js"; @@ -306,6 +307,19 @@ export function needsFlatPhaseMigration(basePath: string): boolean { return hasLegacyMilestoneSubdirs(legacyMigratingPath(basePath)); } +/** + * Is a flat-phase migration mid-flight right now? + * + * True only while the legacy tree sits at `.gsd/milestones.migrating` — the + * window between moving it aside and rendering `.gsd/phases/`, during which the + * slice plans exist in neither layout. Distinct from `needsFlatPhaseMigration`, + * which is also true for a settled legacy project that has not started + * migrating: callers that must not misread a transient gap want this one. + */ +export function isFlatPhaseMigrationInFlight(basePath: string): boolean { + return hasLegacyMilestoneSubdirs(legacyMigratingPath(basePath)); +} + /** Retention window before flat-phase migration backups are auto-pruned. */ export const FLAT_PHASE_BACKUP_RETENTION_MS = 30 * 24 * 60 * 60 * 1000; @@ -366,6 +380,41 @@ export function pruneStaleFlatPhaseBackups(basePath: string): number { export async function migrateToFlatPhase(basePath: string): Promise { if (!needsFlatPhaseMigration(basePath)) return; + // Headless runs the gsd extension in two processes (host + RPC child) and both + // fire session_start, so two migrations race over the same tree: one moves + // milestones/ aside or clears phases/ while the other is mid-render, and the + // loser dies on ENOENT. A whole-migration cross-process lock is the only + // ordering that helps — the steps are not individually idempotent. + // Stale window is well above file-lock's 10s default: migration renders the + // entire projection and can legitimately run for minutes on a large project. + try { + await withFileLock( + join(basePath, ".gsd"), + async () => { + // Re-check under the lock: the winner may have completed the migration + // while this process was waiting, which makes the loser a clean no-op + // rather than a second destructive pass. + if (!needsFlatPhaseMigration(basePath)) return; + await migrateToFlatPhaseLocked(basePath); + }, + // Migration is mostly synchronous fs work, so it blocks the event loop and + // proper-lockfile's mtime keepalive timer cannot fire while it runs. The + // stale window therefore has to exceed the whole migration, not the gap + // between keepalives, or a waiting process declares the lock dead and + // steals it mid-render. + { retries: 30, stale: 600_000 }, + ); + } catch (err) { + // If the lock was stolen anyway, the holder's release() throws even though + // its migration finished. Outcome decides: a completed migration makes that + // release error noise, an incomplete one is a real failure. + const message = (err as Error)?.message ?? ""; + if (!/already released|compromised/i.test(message) || needsFlatPhaseMigration(basePath)) throw err; + logWarning("migration", `flat-phase migration lock released early but migration completed: ${message}`); + } +} + +async function migrateToFlatPhaseLocked(basePath: string): Promise { const milestonesPath = join(basePath, ".gsd", "milestones"); const migratingPath = legacyMigratingPath(basePath); const phasesPath = join(basePath, ".gsd", LAYOUT_SEGMENTS.level1); diff --git a/src/resources/extensions/gsd/markdown-renderer.ts b/src/resources/extensions/gsd/markdown-renderer.ts index 71bcbc957..31e01b1b0 100644 --- a/src/resources/extensions/gsd/markdown-renderer.ts +++ b/src/resources/extensions/gsd/markdown-renderer.ts @@ -425,6 +425,13 @@ function renderSlicePlanMarkdown(slice: SliceRow, tasks: TaskRow[], gates: GateR lines.push(`# ${slice.id}: ${slice.title || slice.id}`); lines.push(""); + // State the DB identities explicitly. Under flat-phase layout this file lives + // at .gsd/phases/01-m001/01-01-PLAN.md, and an agent with only the path to go + // on infers milestoneId "01-m001"/sliceId "01-01" — which the tool contract + // rejects, since it demands the DB identities M001/S01. + lines.push(`**Milestone:** ${slice.milestone_id}`); + lines.push(`**Slice:** ${slice.id}`); + lines.push(""); lines.push(`**Goal:** ${slice.goal}`); lines.push(`**Demo:** ${slice.demo}`); lines.push(""); diff --git a/src/resources/extensions/gsd/stop-notice.ts b/src/resources/extensions/gsd/stop-notice.ts index ae2bcd2ee..10d9c8160 100644 --- a/src/resources/extensions/gsd/stop-notice.ts +++ b/src/resources/extensions/gsd/stop-notice.ts @@ -13,6 +13,17 @@ export function isBlockedStopReason(reason?: string | null): boolean { return /^Blocked:\s*/i.test(reason ?? ""); } +/** + * Mark a reason as a blocked stop. + * + * The headless host derives its exit code from this marker alone, so a stop the + * orchestrator classified as `kind: "blocked"` must carry it — otherwise the run + * reports 0 (complete) over work that never finished. Idempotent. + */ +export function markBlockedStopReason(reason: string): string { + return isBlockedStopReason(reason) ? reason : `Blocked: ${reason}`; +} + /** Strip the "Blocked: " marker for display. */ export function stopNoticeDisplayReason(reason?: string | null): string { return (reason ?? "").replace(/^Blocked:\s*/i, "").trim(); @@ -44,9 +55,16 @@ export function formatVerdictRejectedNotice(message: string): string { export const PAUSED_NOTICE_PREFIXES = ["auto-mode paused", "step-mode paused"] as const; +/** Prefixes formatStopNoticePrefix produces for a blocked stop. */ +export const BLOCKED_NOTICE_PREFIXES = ["auto-mode blocked", "step-mode blocked"] as const; + export const TERMINAL_NOTICE_PREFIXES = [ "auto-mode stopped", "step-mode stopped", + // A blocked stop ends the run exactly like a plain stop — it just carries a + // different exit code. Omitting these left the host without a terminal signal + // for the one outcome that most needs to be reported. + ...BLOCKED_NOTICE_PREFIXES, "auto-mode complete", "no active milestone", "auto-mode idle", @@ -95,6 +113,11 @@ export function isInteractiveMenuUnavailableNotice(message: string): boolean { export function isBlockedNoticeMessage(message: string): boolean { return ( message.includes("blocked:") || + // formatStopNoticePrefix emits "Auto-mode blocked — reason" (em-dash, no + // colon), so the "blocked:" test above never matched the very notice this + // module's own formatter produces. A blocked stop then read as an ordinary + // stop and headless exited 0 over an unfinished milestone. + BLOCKED_NOTICE_PREFIXES.some((prefix) => message.startsWith(prefix)) || message.startsWith("verdict rejected") || (isPauseNotice(message) && !isNonBlockingPauseNotice(message)) || isManualResolutionNotice(message) || diff --git a/src/resources/extensions/gsd/tests/browser-evidence.test.ts b/src/resources/extensions/gsd/tests/browser-evidence.test.ts index 73e33428b..55c9a9347 100644 --- a/src/resources/extensions/gsd/tests/browser-evidence.test.ts +++ b/src/resources/extensions/gsd/tests/browser-evidence.test.ts @@ -140,3 +140,37 @@ describe('hasBrowserRequiredText', () => { ); }); }); + +describe('hasBrowserRequiredText — negated browser mentions', () => { + // Acceptance run 7: a slice that writes two text files declared + // "UAT mode: artifact-driven" and explained why. complete-slice rejected it with + // "UAT requires browser verification". The rationale line read + // "...no runtime behavior, server, UI, or browser interaction is involved" — the + // negator sits several list items away from "browser", so the adjacency-based + // negation guard missed it and `browser interaction` matched as a requirement. + test('a negated list mentioning browser is not a browser requirement', () => { + const text = [ + '## UAT Type', + '', + '- UAT mode: artifact-driven', + '- Why this mode is sufficient: slice deliverables are static text files with exact', + ' required content; no runtime behavior, server, UI, or browser interaction is involved.', + ].join('\n'); + assert.ok(!hasBrowserRequiredText(text), 'negated browser mention must not escalate'); + }); + + test('adjacent negations still pass', () => { + assert.ok(!hasBrowserRequiredText('- No browser interaction is required.')); + assert.ok(!hasBrowserRequiredText('- Verified without a browser session.')); + }); + + test('a real requirement in a later clause still counts', () => { + // The negation guard is clause-bounded, so it must not swallow the sentence after it. + const text = [ + '## Test Cases', + '', + '1. No seeded data is needed. Open the page at localhost:3000 and screenshot it.', + ].join('\n'); + assert.ok(hasBrowserRequiredText(text), 'a genuine browser step must still be detected'); + }); +}); diff --git a/src/resources/extensions/gsd/tests/dispatch-during-migration.test.ts b/src/resources/extensions/gsd/tests/dispatch-during-migration.test.ts new file mode 100644 index 000000000..d6a977cfa --- /dev/null +++ b/src/resources/extensions/gsd/tests/dispatch-during-migration.test.ts @@ -0,0 +1,109 @@ +// gsd-pi — Dispatch must not conclude "never discussed" while the layout migration +// is mid-flight. +// +// `migrateToFlatPhase` moves .gsd/milestones/ aside before rendering .gsd/phases/, +// so there is a window where slice plans exist in neither layout. A dispatch landing +// in that window sees no plans, `hasMilestonePassedDiscuss` returns false, and the +// `execution-entry phase (no context) → discuss-milestone` rule re-plans a milestone +// that was already fully planned — discarding the plan while the DB still holds it. +// +// Observed on every acceptance run (3/3) before the guard: auto mode re-planned the +// seeded milestone from scratch on startup. + +import { test, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { randomUUID } from "node:crypto"; + +import { + _setFlatPhaseMigrationBoundaryForTest, + migrateToFlatPhase, +} from "../flat-phase-migration.ts"; +import { resolveDispatch, DISPATCH_RULES, type DispatchContext } from "../auto-dispatch.ts"; +import { openDatabase, closeDatabase, insertMilestone, insertSlice, insertTask } from "../gsd-db.ts"; +import { convertDispatchRules, initRegistry, getRegistry, resetRegistry } from "../rule-registry.ts"; +import type { GSDState } from "../types.ts"; + +const tmpDirs: string[] = []; + +afterEach(() => { + _setFlatPhaseMigrationBoundaryForTest(null); + closeDatabase(); + for (const d of tmpDirs) { try { rmSync(d, { recursive: true, force: true }); } catch { /* */ } } + tmpDirs.length = 0; +}); + +function seedLegacyProject(): string { + const base = mkdtempSync(join(tmpdir(), `gsd-dispatch-mig-${randomUUID()}`)); + tmpDirs.push(base); + mkdirSync(join(base, ".gsd", "milestones", "M001", "slices", "S01", "tasks"), { recursive: true }); + openDatabase(join(base, ".gsd", "gsd.db")); + insertMilestone({ id: "M001", title: "Planned", status: "active" }); + insertSlice({ + milestoneId: "M001", id: "S01", title: "Slice", status: "active", + risk: "low", depends: [], demo: "demo", sequence: 1, + }); + insertTask({ milestoneId: "M001", sliceId: "S01", id: "T01", title: "Task", status: "pending", sequence: 1 }); + return base; +} + +function executingState(): GSDState { + return { + activeMilestone: { id: "M001", title: "Planned" }, + activeSlice: { id: "S01", title: "Slice" }, + activeTask: null, + phase: "executing", + recentDecisions: [], + blockers: [], + nextAction: "", + registry: [], + } as unknown as GSDState; +} + +test("dispatch inside the migration window does not re-dispatch discuss-milestone", async () => { + const base = seedLegacyProject(); + + let previousExists = false; + try { getRegistry(); previousExists = true; } catch { previousExists = false; } + initRegistry(convertDispatchRules(DISPATCH_RULES)); + + let dispatchedUnit: string | undefined; + let dispatchError: string | undefined; + + // Fire a dispatch at the exact moment the legacy tree has been moved aside and + // phases/ has not yet been rendered — the window a real startup dispatch races. + _setFlatPhaseMigrationBoundaryForTest((stage) => { + if (stage !== "after-move" || dispatchedUnit !== undefined) return; + dispatchedUnit = "(pending)"; + const ctx: DispatchContext = { + basePath: base, + mid: "M001", + midTitle: "Planned", + state: executingState(), + prefs: undefined, + }; + // The boundary hook is synchronous; capture the promise result out of band. + void resolveDispatch(ctx).then( + (r) => { dispatchedUnit = (r as { unitType?: string }).unitType ?? `(${r.action})`; }, + (e) => { dispatchError = e instanceof Error ? e.message : String(e); }, + ); + }); + + await migrateToFlatPhase(base); + await new Promise((r) => setTimeout(r, 50)); // let the out-of-band dispatch settle + + try { + assert.equal(dispatchError, undefined, `dispatch threw: ${dispatchError}`); + assert.notEqual( + dispatchedUnit, + "discuss-milestone", + "a milestone planned in the DB must not be re-discussed because the migration " + + "temporarily moved its plan files", + ); + } finally { + initRegistry(convertDispatchRules(DISPATCH_RULES)); + if (!previousExists) resetRegistry(); + } +}); diff --git a/src/resources/extensions/gsd/tests/flat-phase-migration.test.ts b/src/resources/extensions/gsd/tests/flat-phase-migration.test.ts index de4fb6b95..26a5a3fb7 100644 --- a/src/resources/extensions/gsd/tests/flat-phase-migration.test.ts +++ b/src/resources/extensions/gsd/tests/flat-phase-migration.test.ts @@ -6,6 +6,7 @@ import { mkdtempSync, mkdirSync, renameSync, rmSync, writeFileSync, existsSync, import { join } from "node:path"; import { tmpdir } from "node:os"; import { randomUUID } from "node:crypto"; +import { spawn } from "node:child_process"; import { _setFlatPhaseMigrationBoundaryForTest, @@ -15,6 +16,7 @@ import { } from "../flat-phase-migration.ts"; import { openDatabase, closeDatabase, insertArtifact, insertMilestone, insertSlice, insertTask, getAllMilestones, getMilestoneSlices, getSliceTasks, _getAdapter } from "../gsd-db.ts"; import { writeCompatMarker } from "../compat/compat-marker.ts"; +import { resolveMilestonePath } from "../paths.ts"; const tmpDirs: string[] = []; function makeTmp(options: { withTask?: boolean } = {}): string { @@ -47,6 +49,61 @@ afterEach(() => { tmpDirs.length = 0; }); +test("milestone resolution follows the layout across migration", async () => { + // Guards the invariant that a writer depends on: after the layout is rewritten + // (milestones/ → phases/), resolveMilestonePath must point at the new dir. A + // writer that resolves null here synthesizes a canonical name of its own, and + // the projection ends up split across two phase directories for one milestone + // — which is how S02's slice summary went missing and closeout stalled. + const base = makeTmp(); + + // Warm the cache against the legacy layout. + const before = resolveMilestonePath(base, "M001"); + assert.ok(before?.includes("milestones"), `expected legacy dir, got ${before}`); + + await migrateToFlatPhase(base); + + const after = resolveMilestonePath(base, "M001"); + assert.ok( + after?.includes("phases"), + `after migration the milestone must resolve into phases/, got ${after}`, + ); + assert.equal(existsSync(after!), true, "the resolved phase dir must exist on disk"); +}); + +test("concurrent migrations in separate processes do not corrupt each other", async () => { + // Headless runs the gsd extension in two processes (host + RPC child) and both + // fire session_start. Before the cross-process lock, the second migration moved + // milestones/ aside or cleared phases/ mid-render and the loser died on ENOENT. + // Only a real child process exercises the lock — an in-process test cannot. + const base = makeTmp(); + closeDatabase(); // children own the DB handle + + const migrationUrl = new URL("../flat-phase-migration.js", import.meta.url).href; + const dbUrl = new URL("../gsd-db.js", import.meta.url).href; + const worker = ` + const db = await import(${JSON.stringify(dbUrl)}); + const mig = await import(${JSON.stringify(migrationUrl)}); + db.openDatabase(${JSON.stringify(join(base, ".gsd", "gsd.db"))}); + try { await mig.migrateToFlatPhase(${JSON.stringify(base)}); } + finally { if (db.isDbAvailable()) db.closeDatabase(); } + `; + + const exits = await Promise.all( + [0, 1].map(() => new Promise((resolve) => { + const child = spawn(process.execPath, ["--input-type=module", "-e", worker], { stdio: "pipe" }); + let stderr = ""; + child.stderr.on("data", (chunk) => { stderr += String(chunk); }); + child.on("exit", (code) => resolve(code === 0 ? 0 : (assert.fail(`migration worker failed: ${stderr}`), 1))); + })), + ); + + assert.deepEqual(exits, [0, 0], "both concurrent migrations must succeed"); + assert.equal(needsFlatPhaseMigration(base), false, "migration must be complete after the race"); + assert.equal(existsSync(join(base, ".gsd", "phases")), true, "flat-phase layout must exist"); + assert.equal(existsSync(join(base, ".gsd", "milestones")), false, "legacy layout must be gone"); +}); + test("needsFlatPhaseMigration returns true when .gsd/milestones/ exists", () => { const base = makeTmp(); assert.equal(needsFlatPhaseMigration(base), true); diff --git a/src/resources/extensions/gsd/tests/stop-notice.test.ts b/src/resources/extensions/gsd/tests/stop-notice.test.ts index 51fe22888..6fa040d72 100644 --- a/src/resources/extensions/gsd/tests/stop-notice.test.ts +++ b/src/resources/extensions/gsd/tests/stop-notice.test.ts @@ -10,6 +10,7 @@ import { formatVerdictRecordedNotice, formatVerdictRejectedNotice, isBlockedStopReason, + markBlockedStopReason, stopNoticeDisplayReason, stopNoticeKind, isTerminalNotice, @@ -105,3 +106,52 @@ describe("emitter↔detector round-trip", () => { } }); }); + +describe("blocked stop marking", () => { + // Run 5 of the auto-mode acceptance harness stopped with + // "state did not advance after finalized complete-slice M001/S02" and exited 0 + // — reporting success while the milestone was still open. The orchestrator had + // classified it kind:"blocked", but the reason reached the headless host + // unmarked, and the exit code is derived from the marker alone. + const orchestratorReason = "state did not advance after finalized complete-slice M001/S02"; + + test("an unmarked reason does not classify as blocked", () => { + assert.equal(isBlockedStopReason(orchestratorReason), false); + assert.equal(stopNoticeKind(orchestratorReason), "stopped"); + }); + + test("marking makes it classify as blocked and survive the round-trip", () => { + const marked = markBlockedStopReason(orchestratorReason); + assert.equal(isBlockedStopReason(marked), true); + assert.equal(stopNoticeKind(marked), "blocked"); + assert.equal(stopNoticeDisplayReason(marked), orchestratorReason); + assert.equal(formatStopNoticePrefix(marked), `Auto-mode blocked — ${orchestratorReason}`); + }); + + test("marking is idempotent", () => { + const once = markBlockedStopReason(orchestratorReason); + assert.equal(markBlockedStopReason(once), once); + }); +}); + +describe("blocked stop notices reach the host as blocked+terminal", () => { + // The full emitter→detector path for the run-5 stall: orchestrator marks the + // reason, stopAuto formats the notice, the headless host classifies it. + const notice = formatStopNoticePrefix( + markBlockedStopReason("state did not advance after finalized complete-slice M001/S02"), + ).toLowerCase(); + + test("classifies as blocked (drives the blocked exit code, not 0)", () => { + assert.ok(isBlockedNoticeMessage(notice), notice); + }); + + test("classifies as terminal (ends the run rather than hanging)", () => { + assert.ok(isTerminalNotice(notice), notice); + }); + + test("a plain stop stays non-blocking", () => { + const plain = formatStopNoticePrefix("all milestones complete").toLowerCase(); + assert.ok(isTerminalNotice(plain)); + assert.equal(isBlockedNoticeMessage(plain), false); + }); +}); diff --git a/src/resources/extensions/gsd/tests/tool-surface-affordance.test.ts b/src/resources/extensions/gsd/tests/tool-surface-affordance.test.ts new file mode 100644 index 000000000..b9e8bd8a2 --- /dev/null +++ b/src/resources/extensions/gsd/tests/tool-surface-affordance.test.ts @@ -0,0 +1,128 @@ +// Project/App: gsd-pi +// File Purpose: The Tool Surface must advertise the tokens the contract enforces. +// +// Every HARD BLOCK observed in the auto-mode acceptance runs was a near-miss on a +// token the agent could not see until it was rejected — `gsd_uat_exec` for +// `gsd_exec`, subagent `"review"` for `"reviewer"`, `bash` for `gsd_exec`. The +// contract was enforced but not advertised: 15 of the 24 units with an enforced +// `allowedGsdTools` list had no tool guidance at all. + +import { describe, test } from "node:test"; +import assert from "node:assert/strict"; + +import { composeToolAffordanceReminder, composeToolSurfaceInstructions } from "../unit-context-composer.ts"; +import { UNIT_TOOL_CONTRACTS } from "../unit-tool-contracts.ts"; +import { resolveManifest } from "../unit-context-manifest.ts"; + +const UNITS = Object.keys(UNIT_TOOL_CONTRACTS); + +/** Budget for the derived affordance line. It rides in every unit prompt of every run. */ +const DERIVED_LINE_BUDGET_CHARS = 400; + +describe("tool surface advertises the enforced contract", () => { + test("every unit with a tool contract names each allowed GSD tool", () => { + const gaps: string[] = []; + for (const unit of UNITS) { + const surface = composeToolSurfaceInstructions(unit, { renderMode: "standalone" }); + const missing = UNIT_TOOL_CONTRACTS[unit]!.allowedGsdTools.filter( + (tool) => !surface.includes(`\`${tool}\``), + ); + if (missing.length > 0) gaps.push(`${unit}: ${missing.join(", ")}`); + } + assert.deepEqual(gaps, [], `units not advertising their allowed GSD tools:\n${gaps.join("\n")}`); + }); + + test("allowed subagent tokens are named verbatim where the policy declares them", () => { + const gaps: string[] = []; + for (const unit of UNITS) { + const policy = resolveManifest(unit)?.tools; + const allowed = policy && "allowedSubagents" in policy ? policy.allowedSubagents ?? [] : []; + if (allowed.length === 0) continue; + const surface = composeToolSurfaceInstructions(unit, { renderMode: "standalone" }); + const missing = allowed.filter((agent) => !surface.includes(`\`${agent}\``)); + if (missing.length > 0) gaps.push(`${unit}: ${missing.join(", ")}`); + } + assert.deepEqual(gaps, [], `units not advertising their allowed subagents:\n${gaps.join("\n")}`); + }); + + test("the derived affordance line stays within its token budget", () => { + const over: string[] = []; + for (const unit of UNITS) { + const surface = composeToolSurfaceInstructions(unit, { renderMode: "standalone" }); + const derived = surface + .split("\n") + .find((line) => line.startsWith("GSD lifecycle tools available here:")) ?? ""; + if (derived.length > DERIVED_LINE_BUDGET_CHARS) over.push(`${unit}: ${derived.length} chars`); + } + assert.deepEqual(over, [], `derived tool line over ${DERIVED_LINE_BUDGET_CHARS} chars:\n${over.join("\n")}`); + }); + + test("the surface does not claim the list is exhaustive", () => { + // Generic tools (read, grep) are unaffected by allowedGsdTools; wording that + // implies otherwise would suppress legitimate tool use. + for (const unit of UNITS) { + const surface = composeToolSurfaceInstructions(unit, { renderMode: "standalone" }); + assert.equal( + /only tools you may use|the only tools/i.test(surface), + false, + `${unit}: surface implies the GSD list is the complete tool set`, + ); + } + }); +}); + +describe("guidance prose agrees with the enforced contract", () => { + // Pins the property I had to verify by hand twice while diagnosing: a tool the + // prose tells a unit to *use* must actually be allowed. Prohibitions ("do not + // call X") legitimately name forbidden tools, so only positive mentions count. + test("tools named positively in guidance are in the unit's allow-list", () => { + const drift: string[] = []; + for (const unit of UNITS) { + const allowed = new Set(UNIT_TOOL_CONTRACTS[unit]!.allowedGsdTools as readonly string[]); + const surface = composeToolSurfaceInstructions(unit, { renderMode: "standalone" }); + for (const sentence of surface.split(/(?<=[.!?])\s+/)) { + if (/\bdo not\b|\bnot available\b|\bunavailable\b|\bbelongs to\b|\bis not\b/i.test(sentence)) continue; + if (sentence.startsWith("GSD lifecycle tools available here:")) continue; + for (const tool of sentence.match(/gsd_[a-z_]+/g) ?? []) { + if (!allowed.has(tool)) drift.push(`${unit}: "${tool}" recommended but not allowed`); + } + } + } + assert.deepEqual(drift, [], `guidance recommends forbidden tools:\n${drift.join("\n")}`); + }); +}); + +describe("tail reminder puts the affordance where the model acts", () => { + // Run 9 proved the `## Tool Surface` section alone is not enough: it lands at + // char 627 of validate-milestone's 14,358-char prompt — 4% in, with 13,731 + // characters after it — and the unit still reached for `bash`, `gsd_uat_exec`, + // and subagent "review". These assertions pin the position, not just presence. + test("reminder names the allowed tokens for a unit with a contract", () => { + const reminder = composeToolAffordanceReminder("validate-milestone"); + for (const tool of UNIT_TOOL_CONTRACTS["validate-milestone"]!.allowedGsdTools) { + assert.ok(reminder.includes(`\`${tool}\``), `reminder omits ${tool}`); + } + assert.ok(reminder.includes("`reviewer`"), "reminder omits allowed subagent tokens"); + assert.ok(/near-miss/i.test(reminder), "reminder should warn that variants are rejected"); + // Run 13 regression: the reminder is a name-reference, not a menu. Phrasing it + // as "available here" invited validate-milestone to call gsd_reassess_roadmap + // and grow a completed milestone a new slice, and the run stopped terminating. + assert.ok( + /does not mean this unit needs it/i.test(reminder), + "reminder must not read as an invitation to use every listed tool", + ); + }); + + test("reminder is a single compact line — the prompt tail is expensive", () => { + for (const unit of UNITS) { + const reminder = composeToolAffordanceReminder(unit); + if (!reminder) continue; + assert.equal(reminder.includes("\n"), false, `${unit}: reminder must stay one line`); + assert.ok(reminder.length <= 640, `${unit}: reminder is ${reminder.length} chars`); + } + }); + + test("units without a tool contract get no reminder", () => { + assert.equal(composeToolAffordanceReminder("definitely-not-a-unit"), ""); + }); +}); diff --git a/src/resources/extensions/gsd/tests/verification-gate.test.ts b/src/resources/extensions/gsd/tests/verification-gate.test.ts index da5297797..c0866fe56 100644 --- a/src/resources/extensions/gsd/tests/verification-gate.test.ts +++ b/src/resources/extensions/gsd/tests/verification-gate.test.ts @@ -821,6 +821,32 @@ test("isLikelyCommand: prose descriptions are rejected", () => { assert.equal(isLikelyCommand("Build succeeds without errors or warnings"), false); }); +test("isLikelyCommand: lowercase prose is rejected, including a leading file path", () => { + // Every prose case above announces itself with a capital letter or a comma. + // Lowercase prose fell through to "command" and got executed: the gate ran + // `greet/hello.txt exists and contains "hello"`, which tried to execute the + // .txt file and failed with exit 126 "Permission denied" — failing the gate + // for a task that had actually succeeded. + assert.equal(isLikelyCommand('greet/hello.txt exists and contains "hello"'), false); + assert.equal(isLikelyCommand("./out/report.txt exists and contains the summary"), false); + assert.equal(isLikelyCommand("the migration is complete and the table exists"), false); + + // Real commands with a path-like or bare first token still pass. + assert.equal(isLikelyCommand("./scripts/verify.sh"), true); + assert.equal(isLikelyCommand("./scripts/check.sh --strict --quiet"), true); + assert.equal(isLikelyCommand("mytool build release"), true); +}); + +test("discoverCommands: a prose verify field is not run as a shell command", () => { + const dir = makeTempDir("gsd-verify-prose"); + const result = discoverCommands({ + cwd: dir, + taskPlanVerify: 'greet/hello.txt exists and contains "hello"', + }); + assert.deepEqual(result.commands, [], "prose must not become a runnable check"); + assert.notEqual(result.source, "task-plan"); +}); + test("isLikelyCommand: known command word followed by English prose is rejected (issue #1567)", () => { assert.equal(isLikelyCommand("git log shows the scaffold commit on branch x"), false); assert.equal(isLikelyCommand("make builds the firmware without errors at repo root"), false); diff --git a/src/resources/extensions/gsd/unit-context-composer.ts b/src/resources/extensions/gsd/unit-context-composer.ts index f8c4f1e81..36e4f5ba3 100644 --- a/src/resources/extensions/gsd/unit-context-composer.ts +++ b/src/resources/extensions/gsd/unit-context-composer.ts @@ -223,7 +223,7 @@ const TOOL_SURFACE_GUIDANCE_BY_UNIT: Record = { "execute-task": "Complete only this task via `gsd_task_complete`. Do not call `gsd_slice_complete`, `gsd_validate_milestone`, or `gsd_complete_milestone` — the orchestrator owns phase transitions.", "validate-milestone": - "Dispatch reviewer subagents in parallel, then persist the verdict via `gsd_validate_milestone`. Do not query `.gsd/gsd.db` directly — use `gsd_milestone_status` and inlined context.", + "Run shell verification commands through `gsd_exec` — `bash` is not available in this unit, and `gsd_uat_exec` belongs to run-uat. Dispatch `reviewer` subagents in parallel, then persist the verdict via `gsd_validate_milestone`. Do not query `.gsd/gsd.db` directly — use `gsd_milestone_status` and inlined context. Validation reports on the milestone as planned; it does not extend it. Call `gsd_reassess_roadmap` only when validation FAILS and the roadmap must change to fix it — never to add slices or scope to a milestone whose planned work is complete.", "complete-milestone": "Persist completion only through `gsd_complete_milestone` after verification passes. Do not query `.gsd/gsd.db` directly. Do not write `.gsd/PROJECT.md` or `.gsd/REQUIREMENTS.md` by hand — use `gsd_summary_save` and `gsd_requirement_update`.", "replan-slice": @@ -298,30 +298,96 @@ function formatForbiddenWorkflowToolsLine( * unrestricted (`tools.mode: "all"`) units omit the block unless they have * unit-specific closeout guidance registered above. */ +/** + * Advertise the tokens the contract actually enforces. + * + * Read from `getUnitToolSurfaceContract` — the same table `AUTO_UNIT_SCOPED_TOOLS` + * and the enforcer derive from — so the instruction cannot drift from the rule. + * Every observed HARD BLOCK in the acceptance runs was a near-miss on a token the + * agent could not see until it was rejected (`gsd_uat_exec` for `gsd_exec`, + * subagent `"review"` for `"reviewer"`), so these are rendered verbatim and + * backticked rather than described in prose. + * + * Deliberately worded as "available here", not "the only tools you may use": + * generic tools like read/grep are unaffected by this list. + */ +function formatAllowedToolsLine( + unitType: string, + policy: ToolsPolicy | null, + style: "surface" | "reminder" = "surface", +): string | null { + const allowedGsdTools = getUnitToolSurfaceContract(unitType)?.allowedGsdTools ?? []; + const subagents = policy && "allowedSubagents" in policy ? policy.allowedSubagents ?? [] : []; + const toolLabel = style === "surface" ? "GSD lifecycle tools available here" : "GSD lifecycle"; + const agentLabel = style === "surface" ? "Subagent types available here" : "Subagents"; + const parts: string[] = []; + if (allowedGsdTools.length > 0) { + parts.push(`${toolLabel}: ${allowedGsdTools.map((t) => `\`${t}\``).join(", ")}.`); + } + if (subagents.length > 0) { + parts.push(`${agentLabel}: ${subagents.map((a) => `\`${a}\``).join(", ")}.`); + } + return parts.length > 0 ? parts.join(" ") : null; +} + +/** + * Compact tail reminder of the tokens this unit's contract enforces. + * + * The same affordance also appears in `## Tool Surface`, but measurement showed + * that section lands ~4% into a 14K-character prompt with 13.7K characters after + * it — and the units kept reaching for `bash`, `gsd_uat_exec`, and subagent + * `"review"` anyway. Repeating it at the very end puts it where the model acts. + * Kept to one line: the tail is prime real estate and every unit prompt pays for it. + */ +export function composeToolAffordanceReminder(unitType: string, basePath?: string): string { + const manifest = resolveManifest(unitType); + const policy = manifest + ? resolveEffectivePlanningToolsPolicy(unitType, manifest.tools, basePath) ?? manifest.tools + : null; + const line = formatAllowedToolsLine(unitType, policy, "reminder"); + if (!line) return ""; + // Phrased as a name-reference, not a menu. Wording it as "available here" read + // as an invitation: acceptance run 13 saw validate-milestone reach for + // `gsd_reassess_roadmap` — permitted, and in its required set — to add a slice + // to a milestone whose planned work was already complete, and the run stopped + // terminating. Listing a token must not imply this unit should use it. + return `Reminder — if you call a GSD lifecycle tool or subagent here, it must be one of these exact names (near-miss variants are rejected): ${line} Listing a tool does not mean this unit needs it.`; +} + export function composeToolSurfaceInstructions( unitType: string, opts: ComposeToolSurfaceInstructionOptions, ): string { + // The manifest is optional here: three units (discuss-slice, replan-task, + // execute-task-simple) carry an enforced tool contract without one, and + // returning early for them left their allowed set unadvertised. const manifest = resolveManifest(unitType); - if (!manifest) return ""; - - const effectiveTools = resolveEffectivePlanningToolsPolicy(unitType, manifest.tools, opts.basePath) ?? manifest.tools; - const unitGuidance = guidanceForUnitToolsPolicy(unitType, effectiveTools) ?? TOOL_SURFACE_GUIDANCE_BY_UNIT[unitType]; - const policyGuidance = unitGuidance ? null : guidanceForToolsPolicy(effectiveTools); + const effectiveTools = manifest + ? resolveEffectivePlanningToolsPolicy(unitType, manifest.tools, opts.basePath) ?? manifest.tools + : null; + const allowedLine = formatAllowedToolsLine(unitType, effectiveTools); + const unitGuidance = effectiveTools + ? guidanceForUnitToolsPolicy(unitType, effectiveTools) ?? TOOL_SURFACE_GUIDANCE_BY_UNIT[unitType] + : TOOL_SURFACE_GUIDANCE_BY_UNIT[unitType]; + const policyGuidance = unitGuidance || !effectiveTools ? null : guidanceForToolsPolicy(effectiveTools); const forbiddenLine = formatForbiddenWorkflowToolsLine(unitType, unitGuidance); const parts = [unitGuidance, policyGuidance, forbiddenLine].filter( (part): part is string => typeof part === "string" && part.length > 0, ); - if (parts.length === 0) return ""; + if (parts.length === 0 && !allowedLine) return ""; - const body = parts.join(" "); + // The derived affordance gets its own line rather than being folded into the + // prose: it is a token list, and the failure mode it addresses is the model + // mis-typing a token it skim-read out of a paragraph. + const prose = parts.join(" "); if (opts.renderMode === "nested") { - return `Tool surface: ${body}`; + return `Tool surface: ${[allowedLine, prose].filter(Boolean).join(" ")}`; } - - return ["## Tool Surface", "", body].join("\n"); + const sections = [allowedLine, prose].filter((part): part is string => !!part).join("\n\n"); + return `## Tool Surface\n\n${sections}`; } + // ─── v2 surface (#4924) ─────────────────────────────────────────────────── /** diff --git a/src/resources/extensions/gsd/verification-gate.ts b/src/resources/extensions/gsd/verification-gate.ts index b711f8fc0..49b1cc7ef 100644 --- a/src/resources/extensions/gsd/verification-gate.ts +++ b/src/resources/extensions/gsd/verification-gate.ts @@ -553,8 +553,12 @@ export function isLikelyCommand(cmd: string): boolean { return !readsAsProseAfterCommandWord(effectiveTokens); } - // Path-like first token → command - if (effectiveFirstToken.startsWith("/") || effectiveFirstToken.startsWith("./") || effectiveFirstToken.startsWith("../")) return true; + // Path-like first token → command, unless the rest reads as English prose. + // "./out/report.txt exists and contains the summary" is a description of a + // file, not an invocation of it. + if (effectiveFirstToken.startsWith("/") || effectiveFirstToken.startsWith("./") || effectiveFirstToken.startsWith("../")) { + return !readsAsProseAfterCommandWord(effectiveTokens); + } // Has flag-like tokens → command if (effectiveTokens.some(t => t.startsWith("-"))) return true; @@ -571,7 +575,12 @@ export function isLikelyCommand(cmd: string): boolean { // Non-ASCII prose with multiple words should not be executed as a command. if (!/[A-Za-z0-9]/.test(effectiveFirstToken) && effectiveTokens.length >= 4) return false; - return true; + // Everything above only rejects prose that announces itself with a capital + // letter or comma. Lowercase prose fell through to "command" and got executed + // — `greet/hello.txt exists and contains "hello"` ran the .txt file as a + // program and failed with exit 126 "Permission denied", failing the gate for + // a task that had in fact succeeded. English function words are the tell. + return !readsAsProseAfterCommandWord(effectiveTokens); } /** diff --git a/src/tests/fixtures/prompt-golden-fixtures.ts b/src/tests/fixtures/prompt-golden-fixtures.ts index 0189fc69b..1bf64533b 100644 --- a/src/tests/fixtures/prompt-golden-fixtures.ts +++ b/src/tests/fixtures/prompt-golden-fixtures.ts @@ -34,16 +34,21 @@ export const promptGoldenUnits = [ // explicit terminal-handoff stop rules for gsd_task_reopen/gsd_replan_slice // per issue #846) have grown this prompt; the baseline is adjusted so the // gate still tracks shrinkage from the original oversized prompts while - // allowing today's ~9454-char fixture. + // allowing today's ~9487-char fixture. // T025 re-baseline (15400 -> 15900): rendered fixture measured 9418 chars // at 04f3ba14e (the last adjustment — its "~9154" note was a short-tmp-path // Linux measurement; the gate was already over cap there) and 9454 chars at // HEAD c6935a65b. The +36-char net growth since is deliberate content // (DB-authoritative milestone lifecycle #1476, terminal-handoff stop-rule // tightening), mostly offset by #1475 prompt compression — not accidental - // bloat. floor(15900 * 0.6) = 9540 leaves the same ~86-char headroom the - // #846 adjustment used. - phase2StartChars: 15900, + // bloat. + // Re-baseline (15900 -> 16000): the Tool Surface block now advertises the + // exact allowed GSD lifecycle tool and subagent tokens derived from the + // unit's tool contract, which removed the near-miss HARD BLOCKs that + // stalled auto-mode runs. Measured 9487 chars here; floor(16000 * 0.6) = + // 9600 restores roughly the headroom the #846 adjustment used, which + // 15900's 9540 cap had shrunk to 53 chars. + phase2StartChars: 16000, requiredMarkers: [ "UNIT: Complete Slice S01", "Tool Surface",