From 4b6eb86b843dfc0e6728b323f52f331dcc21f8cb Mon Sep 17 00:00:00 2001 From: Mikael Hugo Date: Sat, 2 May 2026 20:51:56 +0200 Subject: [PATCH] =?UTF-8?q?feat(sf):=20carry-forward=20injection=20?= =?UTF-8?q?=E2=80=94=20final=20piece=20of=20escalation=20feature=20(PDD)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the claimOverrideForInjection stub with a real race-safe implementation. With this commit, the full escalation loop is wired: agent escalates → user pauses → user resolves → next executor in the slice sees the user's choice as a hard constraint in its prompt. The buildExecuteTaskPrompt call site at auto-prompts.ts:2452-2469 already invoked claimOverrideForInjection (gated on phases.mid_execution_escalation). Before this commit it was a no-op because the function returned null unconditionally. Now it actually delivers the override block. PDD spec for this change: Purpose: complete the loop. Without carry-forward, the loop 'continues' but the next executor re-encounters the same ambiguity that triggered the escalation. Consumer: buildExecuteTaskPrompt in auto-prompts.ts (already wired). Contract: 1. No resolved-but-unapplied override in this slice → returns null. Existing behavior preserved when no escalation pending. Verified. 2. Pending escalation (no respondedAt) → returns null. Caller's pause-detection layer handles those. Verified. 3. Resolved escalation (respondedAt + userChoice set) → atomically marks escalation_override_applied=1 (race-safe via UPDATE … WHERE applied=0) and returns formatted markdown block with sourceTaskId. Verified. 4. Second claim on the same override → null (race loser or already-applied). Verified. 5. Missing/malformed artifact → logWarning + null without claiming (so the row isn't silently swallowed by an applied=1 flip). Failure boundary: - claimEscalationOverride is the atomic boundary. Either you claim it and it's yours forever, or someone else did and you skip. - Validation BEFORE claim — bad artifact never marks the row applied. - DB unavailable in claimEscalationOverride → returns false → caller treats as race-loser → null. Safe. Evidence: - Smoke test exercises 4 contract conditions: no-override → null pending-only → null resolved-then-claim → returns block + sets DB flag second-claim → null (idempotent) - Typecheck clean. - All 62 existing preferences tests still pass (no regression in the related plumbing). Non-goals: - reject-blocker carry-forward (gsd-2 has it; needs blocker_source DB column SF doesn't have). - Cross-slice override carry-forward (current scope: per-slice). - Override-applied audit event (gsd-2 emits one; can add later). Invariants: - Safety: applied flag is set BEFORE the prompt is built — so a crash mid-build never re-injects on retry. - Liveness: any task in the slice with a resolved override gets surfaced in sequence order (lowest sequence first via findUnappliedEscalationOverride's ORDER BY). - Race-safety: SQL UPDATE … WHERE applied=0 returns changes>0 only for the winner. Tested with sequential claims; both winners and losers behave correctly. DB schema: tasks.escalation_override_applied (INTEGER NOT NULL DEFAULT 0), migration v25. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/resources/extensions/sf/escalation.ts | 67 ++++++++++++++++- src/resources/extensions/sf/sf-db.ts | 88 +++++++++++++++++++++++ 2 files changed, 152 insertions(+), 3 deletions(-) diff --git a/src/resources/extensions/sf/escalation.ts b/src/resources/extensions/sf/escalation.ts index 6997481fe..e6c16babc 100644 --- a/src/resources/extensions/sf/escalation.ts +++ b/src/resources/extensions/sf/escalation.ts @@ -11,13 +11,16 @@ import { atomicWriteSync } from "./atomic-write.js"; import { resolveSlicePath } from "./paths.js"; import type { TaskRow } from "./sf-db.js"; import { + claimEscalationOverride, clearTaskEscalationFlags, + findUnappliedEscalationOverride, getTask, setTaskEscalationAwaitingReview, setTaskEscalationPending, } from "./sf-db.js"; import type { EscalationArtifact, EscalationOption } from "./types.js"; import { buildAuditEnvelope, emitUokAuditEvent } from "./uok/audit.js"; +import { logWarning } from "./workflow-logger.js"; // ─── Paths ──────────────────────────────────────────────────────────────── @@ -208,12 +211,70 @@ export function detectPendingEscalation( return null; } +/** ADR-011 P2 carry-forward injection: when a previous task in this slice + * had an escalation that the user resolved, atomically claim the override + * (race-safe via DB UPDATE) and return the markdown block to prepend to + * the next executor's prompt. Returns null when no unapplied override + * exists OR when another caller claimed it first. Idempotent: a successful + * claim flips override_applied 0→1 so the same override never injects twice. */ export function claimOverrideForInjection( _basePath: string, - _milestoneId: string, - _sliceId: string, + milestoneId: string, + sliceId: string, ): { injectionBlock: string; sourceTaskId: string } | null { - return null; + const unapplied = findUnappliedEscalationOverride(milestoneId, sliceId); + if (!unapplied) return null; + // Validate the artifact BEFORE claiming. A missing/malformed file would + // otherwise mark the row applied=1 and silently swallow the override. + const art = readEscalationArtifact(unapplied.artifactPath); + if (!art) { + logWarning( + "tool", + `escalation: artifact missing/malformed at ${unapplied.artifactPath} (task ${unapplied.taskId}); skipping without claim — operator should resolve or remove the row`, + ); + return null; + } + if (!art.respondedAt || !art.userChoice) return null; + const claimed = claimEscalationOverride( + milestoneId, + sliceId, + unapplied.taskId, + ); + if (!claimed) return null; // race loser + return { + injectionBlock: formatOverrideBlock(art), + sourceTaskId: unapplied.taskId, + }; +} + +/** Build the markdown block prepended to a downstream executor's prompt to + * carry forward a user's escalation resolution as a hard constraint. */ +function formatOverrideBlock(art: EscalationArtifact): string { + const isAccept = art.userChoice === "accept"; + const isOptionChoice = !!art.userChoice && !isAccept; + const choiceLabel = isAccept + ? `accepted recommendation (${art.recommendation})` + : isOptionChoice + ? `${art.options.find((o) => o.id === art.userChoice)?.label ?? art.userChoice} (id: ${art.userChoice})` + : (art.userChoice ?? "unknown"); + const tradeoffs = isOptionChoice + ? (art.options.find((o) => o.id === art.userChoice)?.tradeoffs ?? "") + : ""; + const rationale = art.userRationale + ? `\n\n**User rationale:** ${art.userRationale}` + : ""; + return [ + `## Escalation Override (from ${art.taskId})`, + "", + `During ${art.taskId} the executor escalated: **${art.question}**`, + "", + `The user's resolution: **${choiceLabel}**.${rationale}`, + tradeoffs ? `\n**Tradeoffs of this choice:** ${tradeoffs}` : "", + "", + "Apply this decision as a hard constraint for the current task. If it contradicts the task plan, surface the conflict in your summary rather than silently deviating.", + ] + .filter((line) => line !== undefined) + .join("\n"); } // ─── Resolution ──────────────────────────────────────────────────────────── diff --git a/src/resources/extensions/sf/sf-db.ts b/src/resources/extensions/sf/sf-db.ts index d2335f210..1b72df26e 100644 --- a/src/resources/extensions/sf/sf-db.ts +++ b/src/resources/extensions/sf/sf-db.ts @@ -369,6 +369,7 @@ function initSchema(db: DbAdapter, fileBacked: boolean): void { sequence INTEGER DEFAULT 0, -- Ordering hint: tools may set this to control execution order escalation_pending INTEGER NOT NULL DEFAULT 0, -- ADR-011 P2 (gsd-2): pause-on-escalation flag escalation_awaiting_review INTEGER NOT NULL DEFAULT 0, -- ADR-011 P2 (gsd-2): continueWithDefault=true marker (no pause) + escalation_override_applied INTEGER NOT NULL DEFAULT 0, -- ADR-011 P2: 1 once carry-forward injected into a downstream prompt escalation_artifact_path TEXT DEFAULT NULL, -- ADR-011 P2 (gsd-2): path to T##-ESCALATION.json PRIMARY KEY (milestone_id, slice_id, id), FOREIGN KEY (milestone_id, slice_id) REFERENCES slices(milestone_id, id) @@ -1485,6 +1486,28 @@ function migrateSchema(db: DbAdapter): void { }); } + if (currentVersion < 25) { + // ADR-011 P2 carry-forward: when an escalation is resolved, the user's + // choice should be visible to the next execute-task agent in the same + // slice. escalation_override_applied=0 marks "resolved but not yet + // injected into a downstream prompt"; the prompt builder calls + // claimEscalationOverride which atomically flips it to 1 (idempotent + // race-safe claim). Per-task granularity so multi-task slices can + // carry multiple resolved escalations forward independently. + ensureColumn( + db, + "tasks", + "escalation_override_applied", + `ALTER TABLE tasks ADD COLUMN escalation_override_applied INTEGER NOT NULL DEFAULT 0`, + ); + db.prepare( + "INSERT INTO schema_version (version, applied_at) VALUES (:version, :applied_at)", + ).run({ + ":version": 25, + ":applied_at": new Date().toISOString(), + }); + } + db.exec("COMMIT"); } catch (err) { db.exec("ROLLBACK"); @@ -2419,6 +2442,66 @@ export function clearTaskEscalationFlags( .run({ ":mid": milestoneId, ":sid": sliceId, ":tid": taskId }); } +/** ADR-011 P2 carry-forward: find a task in this slice that has a resolved + * escalation override that has NOT yet been injected into a downstream + * prompt. Returns the first match by sequence (lowest first), or null when + * no carry-forward is pending. + * + * The match criterion: escalation_artifact_path IS NOT NULL AND + * escalation_pending=0 AND escalation_awaiting_review=0 AND + * escalation_override_applied=0. The artifact's respondedAt is checked by + * the caller (claimOverrideForInjection in escalation.ts) — keeping artifact + * schema knowledge out of the DB layer. */ +export function findUnappliedEscalationOverride( + milestoneId: string, + sliceId: string, +): { taskId: string; artifactPath: string } | null { + if (!currentDb) return null; + const row = currentDb + .prepare( + `SELECT id, escalation_artifact_path + FROM tasks + WHERE milestone_id = :mid + AND slice_id = :sid + AND escalation_artifact_path IS NOT NULL + AND escalation_pending = 0 + AND escalation_awaiting_review = 0 + AND escalation_override_applied = 0 + ORDER BY sequence ASC, id ASC + LIMIT 1`, + ) + .get({ ":mid": milestoneId, ":sid": sliceId }) as + | { id: string; escalation_artifact_path: string | null } + | undefined; + if (!row || !row.escalation_artifact_path) return null; + return { taskId: row.id, artifactPath: row.escalation_artifact_path }; +} + +/** ADR-011 P2 carry-forward: atomically claim the override for injection. + * Returns true when this caller successfully flipped 0→1 (race winner) or + * false when another caller claimed it first (race loser). Use this to + * guarantee the override is injected exactly once. */ +export function claimEscalationOverride( + milestoneId: string, + sliceId: string, + taskId: string, +): boolean { + if (!currentDb) return false; + const result = currentDb + .prepare( + `UPDATE tasks + SET escalation_override_applied = 1 + WHERE milestone_id = :mid + AND slice_id = :sid + AND id = :tid + AND escalation_override_applied = 0`, + ) + .run({ ":mid": milestoneId, ":sid": sliceId, ":tid": taskId }) as + | { changes?: number } + | undefined; + return (result?.changes ?? 0) > 0; +} + export function setTaskBlockerDiscovered( milestoneId: string, sliceId: string, @@ -2641,6 +2724,9 @@ export interface TaskRow { escalation_pending?: number; /** ADR-011 P2: 1 = continueWithDefault=true marker — artifact recorded but loop not paused. */ escalation_awaiting_review?: number; + /** ADR-011 P2 carry-forward: 1 once the resolved override has been injected + * into a downstream prompt. Race-safe atomic claim via claimEscalationOverride. */ + escalation_override_applied?: number; /** ADR-011 P2: relative path to the T##-ESCALATION.json artifact next to T##-PLAN.md. */ escalation_artifact_path?: string | null; } @@ -2737,6 +2823,8 @@ function rowToTask(row: Record): TaskRow { escalation_pending: (row["escalation_pending"] as number) ?? 0, escalation_awaiting_review: (row["escalation_awaiting_review"] as number) ?? 0, + escalation_override_applied: + (row["escalation_override_applied"] as number) ?? 0, escalation_artifact_path: (row["escalation_artifact_path"] as string | null) ?? null, };