From da4f339dd6578e17626c83c4f5406f66d8c481d0 Mon Sep 17 00:00:00 2001 From: Bailey Dixon Date: Mon, 6 Jul 2026 18:28:34 -0400 Subject: [PATCH 01/21] fix(orchestration): gate cascade on plan status, guard verdicts, wall-time from startedAt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-1 safety fixes for the goal/execution-plan loop (from the agent-runtime audit): - cascadeReadiness / transitionStepToReady now early-return unless the plan is RUNNING. A BLOCKED (budget/wall-time) or CANCELED (abandon/replan) plan no longer cascades finishing steps into fresh dispatches — closing the hole where a blocked plan kept spending the exact budget the operator was asked to approve. - recordVerdict rejects a verdict on a settled step (DONE/BLOCKED/CANCELED) with CONFLICT. A stale/duplicate reviewer webhook can no longer move a DONE step back to TODO and re-dispatch, un-completing finished work. - ExecutionPlan.startedAt (migration 0091) stamped in activatePlan; wall-time budget measured from it, not createdAt, so planning + approval-wait isn't charged against the execution clock. - Add @@index([status,lastEventAt]) + @@index([assignmentEventId]) on AgentRun so the cross-tenant 5s/60s worker sweeps stop sequential-scanning. 4 new regression tests; all 24 orchestration tests green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../migration.sql | 12 ++ prisma/schema.prisma | 16 ++ .../services/__tests__/orchestration.test.ts | 142 ++++++++++++++++++ src/server/services/orchestration-service.ts | 45 +++++- 4 files changed, 212 insertions(+), 3 deletions(-) create mode 100644 prisma/migrations/0091_plan_startedat_and_agentrun_sweep_indexes/migration.sql diff --git a/prisma/migrations/0091_plan_startedat_and_agentrun_sweep_indexes/migration.sql b/prisma/migrations/0091_plan_startedat_and_agentrun_sweep_indexes/migration.sql new file mode 100644 index 0000000..1a14cf8 --- /dev/null +++ b/prisma/migrations/0091_plan_startedat_and_agentrun_sweep_indexes/migration.sql @@ -0,0 +1,12 @@ +-- AlterTable: wall-time budget clock starts at execution (activatePlan), not +-- decompose, so planning + approval-wait isn't charged against the run clock. +ALTER TABLE "ExecutionPlan" ADD COLUMN "startedAt" TIMESTAMP(3); + +-- CreateIndex: status-leading index for the cross-tenant worker sweeps +-- (pollActiveRuns / ensureSubscriptions / stale watchdog) that filter on +-- status with no workspaceId and previously seq-scanned every 5s/60s. +CREATE INDEX "AgentRun_status_lastEventAt_idx" ON "AgentRun"("status", "lastEventAt"); + +-- CreateIndex: dedup lookup on the dispatch path (has this AGENT_ASSIGNED +-- event already opened a run?). +CREATE INDEX "AgentRun_assignmentEventId_idx" ON "AgentRun"("assignmentEventId"); diff --git a/prisma/schema.prisma b/prisma/schema.prisma index ac2b01d..5c9445a 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -2594,6 +2594,15 @@ model AgentRun { /// Collapse stacked runs: list heads with `supersededByRunId IS NULL`, /// expand a thread by following the chain. @@index([supersededByRunId]) + /// Cross-tenant worker sweeps (pollActiveRuns / ensureSubscriptions / + /// stale watchdog) filter on `status` with NO workspaceId, so the + /// workspaceId-leading indexes above can't serve them and they seq-scan + /// every 5s/60s. A status-leading index keeps those hot sweeps off a + /// sequential scan as AgentRun history grows. + @@index([status, lastEventAt]) + /// Dedup lookup on the dispatch path: has this AGENT_ASSIGNED event + /// already opened a run for the agent? + @@index([assignmentEventId]) } /// Append-only timeline for an AgentRun. Drives the live pulse strip and @@ -3118,6 +3127,13 @@ model ExecutionPlan { maxTotalCostUsd Float? maxWallTimeMinutes Int? totalCostUsd Float @default(0) + /// When execution actually began — activatePlan flips DRAFT/APPROVED → + /// RUNNING and stamps this. Wall-time budget is measured from here, NOT + /// createdAt, so planning + approval-wait time isn't charged against the + /// execution clock (a plan approved days after decompose shouldn't BLOCK + /// on its first cost record). Null until first activation; legacy rows + /// fall back to createdAt. + startedAt DateTime? /// Marks the live decompose attempt for a goal. Prior attempts are /// flipped false when a new decompose runs. isActiveAttempt Boolean @default(true) diff --git a/src/server/services/__tests__/orchestration.test.ts b/src/server/services/__tests__/orchestration.test.ts index 7a3aeba..9695fd9 100644 --- a/src/server/services/__tests__/orchestration.test.ts +++ b/src/server/services/__tests__/orchestration.test.ts @@ -921,3 +921,145 @@ describe("orchestration: attach hand-authored plan to goal", () => { ).rejects.toThrow(/different goal/); }); }); + +describe("orchestration: audit phase-1 safety guards", () => { + // Build a 2-step plan (child depends on root), activated so root is READY. + async function build2StepRunning() { + const { fixture, prisma } = await setup(); + const worker = await makeAgent(fixture.workspace.id, "worker"); + const crew = await createAgentCrew(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + name: "Crew", + members: [{ agentId: worker.id, role: "WORKER" }], + }); + const goal = await createGoal(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + title: "Goal", + crewId: crew.id, + }); + const { planId } = await decomposeGoal(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + goalId: goal.id, + }); + const { stepIds } = await addStepsToPlan(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + planId, + steps: [{ title: "root" }, { title: "child", dependsOnStepIndexes: [0] }], + }); + await prisma.$transaction((tx) => + activatePlan(tx, { workspaceId: fixture.workspace.id, actorId: fixture.user.id, planId }), + ); + return { fixture, prisma, planId, rootId: stepIds[0], childId: stepIds[1] }; + } + + it("P1.1 — a BLOCKED plan does not cascade dependents into dispatch", async () => { + const { fixture, prisma, planId, rootId, childId } = await build2StepRunning(); + // Simulate the budget watchdog blocking the plan mid-flight. + await prisma.executionPlan.update({ + where: { id: planId }, + data: { status: ExecutionPlanStatus.BLOCKED }, + }); + // Root finishes; a cascade must NOT ready the child while BLOCKED. + await prisma.executionStep.update({ + where: { id: rootId }, + data: { status: ExecutionStepStatus.DONE }, + }); + const flipped = await prisma.$transaction((tx) => + cascadeReadiness(tx, { workspaceId: fixture.workspace.id, planId, actorId: fixture.user.id }), + ); + expect(flipped).toEqual([]); + let child = await prisma.executionStep.findUniqueOrThrow({ where: { id: childId } }); + expect(child.status).toBe(ExecutionStepStatus.TODO); + + // Resuming the plan lets the cascade proceed — proves the gate was the + // only thing holding dispatch, not a stuck dependency. + await prisma.executionPlan.update({ + where: { id: planId }, + data: { status: ExecutionPlanStatus.RUNNING }, + }); + await prisma.$transaction((tx) => + cascadeReadiness(tx, { workspaceId: fixture.workspace.id, planId, actorId: fixture.user.id }), + ); + child = await prisma.executionStep.findUniqueOrThrow({ where: { id: childId } }); + expect(child.status).toBe(ExecutionStepStatus.READY); + }); + + it("P1.1 — a CANCELED plan does not re-open work via a late retry verdict", async () => { + const { fixture, prisma, planId, rootId } = await build2StepRunning(); + // Cancel the plan (as abandonGoal would), while root is still in flight. + await prisma.executionPlan.update({ + where: { id: planId }, + data: { status: ExecutionPlanStatus.CANCELED }, + }); + // A late FAIL verdict arrives on the still-READY root. It should record + // (root is not settled) but the RETRY re-dispatch must be a no-op on a + // CANCELED plan — the step lands TODO and is NOT re-readied. + const res = await recordVerdict(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + stepId: rootId, + verdict: "FAIL", + feedback: "late", + }); + expect(res.outcome).toBe("RETRY"); + const root = await prisma.executionStep.findUniqueOrThrow({ where: { id: rootId } }); + expect(root.status).toBe(ExecutionStepStatus.TODO); // not re-READY + }); + + it("P1.2 — a stale verdict on a settled (DONE) step is rejected, keeping it DONE", async () => { + const { fixture, prisma, rootId } = await build2StepRunning(); + // First verdict completes the step. + const first = await recordVerdict(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + stepId: rootId, + verdict: "PASS", + feedback: "ok", + }); + expect(first.outcome).toBe("DONE"); + // A stale/duplicate FAIL webhook must not reopen the finished step. + await expect( + recordVerdict(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + stepId: rootId, + verdict: "FAIL", + feedback: "stale", + }), + ).rejects.toThrow(/settled/); + const step = await prisma.executionStep.findUniqueOrThrow({ where: { id: rootId } }); + expect(step.status).toBe(ExecutionStepStatus.DONE); + expect(step.retryCount).toBe(0); + }); + + it("P1.7 — activatePlan stamps startedAt for the wall-time clock", async () => { + const { fixture, prisma } = await setup(); + const goal = await createGoal(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + title: "Goal", + }); + const { planId } = await decomposeGoal(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + goalId: goal.id, + }); + await addStepsToPlan(prisma, { + workspaceId: fixture.workspace.id, + actorId: fixture.user.id, + planId, + steps: [{ title: "only" }], + }); + const before = await prisma.executionPlan.findUniqueOrThrow({ where: { id: planId } }); + expect(before.startedAt).toBeNull(); + await prisma.$transaction((tx) => + activatePlan(tx, { workspaceId: fixture.workspace.id, actorId: fixture.user.id, planId }), + ); + const after = await prisma.executionPlan.findUniqueOrThrow({ where: { id: planId } }); + expect(after.startedAt).not.toBeNull(); + }); +}); diff --git a/src/server/services/orchestration-service.ts b/src/server/services/orchestration-service.ts index dabf3db..e0d5c7e 100644 --- a/src/server/services/orchestration-service.ts +++ b/src/server/services/orchestration-service.ts @@ -1301,6 +1301,18 @@ export async function cascadeReadiness( tx: Tx, params: { workspaceId: string; planId: string; actorId: string | null }, ): Promise { + // A plan only dispatches while RUNNING. Once it is BLOCKED (budget / + // wall-time watchdog) or CANCELED (goal abandoned or superseded by a new + // decompose attempt), a step that finishes must NOT cascade its dependents + // into fresh dispatches — that would spend exactly the budget the operator + // was asked to approve, and un-pause a plan the operator deliberately + // stopped. DRAFT/APPROVED never dispatch either (activatePlan flips RUNNING + // first, then cascades). + const plan = await tx.executionPlan.findUnique({ + where: { id: params.planId }, + select: { status: true }, + }); + if (!plan || plan.status !== ExecutionPlanStatus.RUNNING) return []; const steps = await tx.executionStep.findMany({ where: { planId: params.planId }, select: { id: true, status: true, dependsOnStepIds: true }, @@ -1347,11 +1359,16 @@ async function transitionStepToReady( status: true, issueId: true, plan: { - select: { id: true, crewId: true, contextSetId: true, issueId: true }, + select: { id: true, status: true, crewId: true, contextSetId: true, issueId: true }, }, }, }); if (!step) return; + // Defence in depth for callers other than cascadeReadiness (e.g. the + // recordVerdict RETRY path, which re-readies a step directly): never + // dispatch a step whose plan is no longer RUNNING. A late verdict on a + // step whose plan was CANCELED/BLOCKED mid-flight must not re-open work. + if (step.plan.status !== ExecutionPlanStatus.RUNNING) return; // Resolve the worker: explicit assignee > crew WORKER. let workerAgentId = step.assignedAgentId; @@ -1770,6 +1787,22 @@ export async function recordVerdict( if (!step) { throw new TRPCError({ code: "NOT_FOUND", message: "Execution step not found." }); } + // A verdict may only land on an in-flight step (READY / RUNNING / REVIEW). + // Reject it on an already-settled step (DONE / BLOCKED / CANCELED): a stale + // or duplicated reviewer webhook posting FAIL on a DONE step would otherwise + // move it back to TODO, bump retryCount, and re-dispatch a worker — + // un-completing finished work and desyncing downstream steps. Also stops an + // auto-judge double-fire from double-recording. + if ( + step.status === ExecutionStepStatus.DONE || + step.status === ExecutionStepStatus.BLOCKED || + step.status === ExecutionStepStatus.CANCELED + ) { + throw new TRPCError({ + code: "CONFLICT", + message: `Step is ${step.status} — a verdict can't be recorded on a settled step.`, + }); + } const verdictJson: JudgeVerdictJson = { verdict: params.verdict, @@ -2066,7 +2099,10 @@ export async function activatePlan( } await tx.executionPlan.update({ where: { id: plan.id }, - data: { status: ExecutionPlanStatus.RUNNING }, + // Stamp startedAt so the wall-time budget clock starts at execution, not + // at decompose. activatePlan only runs from DRAFT/APPROVED, so startedAt + // is null here on the first (and only) activation. + data: { status: ExecutionPlanStatus.RUNNING, startedAt: new Date() }, }); await recordChange(tx, { workspaceId: params.workspaceId, @@ -2173,6 +2209,7 @@ export async function applyRunCostToPlan( crewId: true, goalId: true, createdAt: true, + startedAt: true, }, }); if (plan.goalId) { @@ -2195,6 +2232,7 @@ interface PlanBudgetRow { crewId: string | null; goalId: string | null; createdAt: Date; + startedAt: Date | null; } /** @@ -2211,9 +2249,10 @@ export async function checkAndBlockBudget( if (plan.status !== ExecutionPlanStatus.RUNNING) return false; const overCost = plan.maxTotalCostUsd != null && plan.totalCostUsd > plan.maxTotalCostUsd; + const wallClockStart = plan.startedAt ?? plan.createdAt; const overTime = plan.maxWallTimeMinutes != null && - Date.now() - plan.createdAt.getTime() > plan.maxWallTimeMinutes * 60_000; + Date.now() - wallClockStart.getTime() > plan.maxWallTimeMinutes * 60_000; if (!overCost && !overTime) return false; await tx.executionPlan.update({ From 53735c38adccdd9fa2d65c15d20b4657a56b8d6f Mon Sep 17 00:00:00 2001 From: Bailey Dixon Date: Mon, 6 Jul 2026 18:28:58 -0400 Subject: [PATCH 02/21] fix(runtime): unknown status non-terminal, keep-alive quiet runs, propagate approve/stop failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-1 runtime-backend safety fixes (agent-runtime audit): - pollActiveRuns treats getStatus 'unknown' as NON-terminal (leaves the run ACTIVE, lets the stale watchdog arbitrate) instead of false-STALLing. This is the worker-restart / deploy false-STALL for Codex, and any momentary blip. - hermes-runs mapStatus defaults an unrecognized non-empty lifecycle word ('initializing', 'preparing', …) to 'running', not 'unknown'. - On a live 'running' poll always bump lastEventAt, even when the step label is unchanged, so a quiet-but-alive run (mid long tool call) isn't watchdog-STALLED. - hermes-runs approve/stop now inspect res.ok and throw on failure; the agent-run approve/reject mutation surfaces it as an error instead of clearing the block and returning ok — no more false 'approved' while the provider run stays blocked. - runtime.register now calls assertEndpointTransport like create/update, so a plaintext public endpoint can't slip in via the register path. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/server/routers/agent-run.ts | 29 ++++++++- src/server/routers/runtime.ts | 4 ++ src/server/services/dispatch/hermes-runs.ts | 55 ++++++++++++---- .../services/dispatch/run-dispatcher.ts | 62 +++++++++++++------ 4 files changed, 114 insertions(+), 36 deletions(-) diff --git a/src/server/routers/agent-run.ts b/src/server/routers/agent-run.ts index e9947b2..d31febc 100644 --- a/src/server/routers/agent-run.ts +++ b/src/server/routers/agent-run.ts @@ -1243,7 +1243,19 @@ export const agentRunRouter = router({ } if (input.decision === "approve") { - await connector.approve?.(run.externalRunId, input.scope); + // A failed approval POST must NOT clear the block — surface it so the + // operator can retry instead of seeing a false success while the run + // stays blocked at the provider. + try { + await connector.approve?.(run.externalRunId, input.scope); + } catch (err) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Approval didn't reach the runtime — the run is still blocked. ${ + err instanceof Error ? err.message : "" + }`.trim(), + }); + } await ctx.db.$transaction(async (tx) => { await tx.agentRun.update({ where: { id: run.id }, @@ -1267,8 +1279,19 @@ export const agentRunRouter = router({ return { ok: true as const, decision: "approve" as const }; } - // Reject → stop the live run, then close the AgentRun. - await connector.stop?.(run.externalRunId); + // Reject → stop the live run, then close the AgentRun. If the stop POST + // fails, do NOT close the run locally (that would orphan a still-live + // provider run) — surface the failure so the operator can retry. + try { + await connector.stop?.(run.externalRunId); + } catch (err) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: `Stop didn't reach the runtime — the run may still be active. ${ + err instanceof Error ? err.message : "" + }`.trim(), + }); + } await ctx.db.$transaction(async (tx) => { await tx.agentRun.update({ where: { id: run.id }, diff --git a/src/server/routers/runtime.ts b/src/server/routers/runtime.ts index 18fb2f1..6ee8cba 100644 --- a/src/server/routers/runtime.ts +++ b/src/server/routers/runtime.ts @@ -233,6 +233,10 @@ export const runtimeRouter = router({ register: workspaceProcedure .input(z.object(baseFields)) .mutation(async ({ ctx, input }) => { + // Same transport guard as `create` / `update`: a REMOTE_HTTP endpoint + // on a public host must use TLS. `register` previously skipped this, so + // a plaintext public endpoint could slip in through this path. + assertEndpointTransport(input.endpoint || null); // For LOCAL_DAEMON the daemon registers itself and owns the row; // for REMOTE_HTTP an admin typically registers it but the same // attribution holds. ownerId is set from the calling user. diff --git a/src/server/services/dispatch/hermes-runs.ts b/src/server/services/dispatch/hermes-runs.ts index 6104c78..d33eaf8 100644 --- a/src/server/services/dispatch/hermes-runs.ts +++ b/src/server/services/dispatch/hermes-runs.ts @@ -15,7 +15,12 @@ function mapStatus(raw: string): RunStatus["state"] { if (s.includes("fail") || s.includes("error")) return "failed"; if (s.includes("approval")) return "waiting_for_approval"; if (s.includes("run") || s.includes("pending") || s.includes("queue")) return "running"; - return "unknown"; + // A non-empty but unrecognized lifecycle word ("initializing", "preparing", + // "starting", …) means the run is alive in a phase we don't have an explicit + // label for. Treat it as running, NOT unknown, so the poll loop keeps it + // ACTIVE instead of false-STALLing a run that hasn't even started emitting. + // Only a truly empty status is genuinely unknown. + return s.trim() ? "running" : "unknown"; } /** @@ -98,14 +103,27 @@ export function makeHermesRunsConnector(opts?: { return token ? { authorization: `Bearer ${token}` } : {}; }; + // Throws on network failure OR a non-2xx response so the caller can surface + // "approval failed, retry" instead of silently showing success while the + // provider run stays blocked. Internal fire-and-forget callers (auto-yolo) + // must attach their own .catch. const approveRun = async (externalRunId: string, choice: string): Promise => { - await fetch(`${base()}/runs/${externalRunId}/approval`, { - method: "POST", - headers: { "content-type": "application/json", ...authHeaders() }, - body: JSON.stringify({ choice }), - }).catch((err) => { + let res: Response; + try { + res = await fetch(`${base()}/runs/${externalRunId}/approval`, { + method: "POST", + headers: { "content-type": "application/json", ...authHeaders() }, + body: JSON.stringify({ choice }), + }); + } catch (err) { logger.warn({ err, externalRunId }, "hermes-runs: approval post failed"); - }); + throw err instanceof Error ? err : new Error("Hermes approval post failed"); + } + if (!res.ok) { + const text = await res.text().catch(() => ""); + logger.warn({ externalRunId, status: res.status }, "hermes-runs: approval rejected"); + throw new Error(text || `Hermes approval failed (${res.status})`); + } }; return { @@ -232,7 +250,9 @@ export function makeHermesRunsConnector(opts?: { break; case "approval.request": if (runYoloOverrides.get(externalRunId) ?? runtimeYoloMode) { - void approveRun(externalRunId, "once"); + void approveRun(externalRunId, "once").catch((err) => + logger.warn({ err, externalRunId }, "hermes-runs: auto-approve failed"), + ); onEvent({ type: "approval_resolved", choice: "once" }); break; } @@ -345,12 +365,21 @@ export function makeHermesRunsConnector(opts?: { }, async stop(externalRunId: string): Promise { - await fetch(`${base()}/runs/${externalRunId}/stop`, { - method: "POST", - headers: authHeaders(), - }).catch((err) => { + let res: Response; + try { + res = await fetch(`${base()}/runs/${externalRunId}/stop`, { + method: "POST", + headers: authHeaders(), + }); + } catch (err) { logger.warn({ err, externalRunId }, "hermes-runs: stop post failed"); - }); + throw err instanceof Error ? err : new Error("Hermes stop post failed"); + } + if (!res.ok) { + const text = await res.text().catch(() => ""); + logger.warn({ externalRunId, status: res.status }, "hermes-runs: stop rejected"); + throw new Error(text || `Hermes stop failed (${res.status})`); + } }, }; } diff --git a/src/server/services/dispatch/run-dispatcher.ts b/src/server/services/dispatch/run-dispatcher.ts index 5066294..ee8f0b9 100644 --- a/src/server/services/dispatch/run-dispatcher.ts +++ b/src/server/services/dispatch/run-dispatcher.ts @@ -782,23 +782,18 @@ async function pollActiveRuns(): Promise { if (verdict.state === "breach") continue; const step = status.lastEvent ?? "running"; - // Clear a prior approval block (operator approved → agent resumed) - // or just advance the step label. - if (run.awaitingApprovalAt || step !== run.currentStep) { - const hasLiveEventStream = Boolean(connector.subscribe); - await db - .$transaction(async (tx) => { - if (hasLiveEventStream) { - await tx.agentRun.update({ - where: { id: run.id }, - data: { - lastEventAt: new Date(), - currentStep: step, - ...(run.awaitingApprovalAt ? { awaitingApprovalAt: null } : {}), - }, - }); - return; - } + const stepChanged = step !== run.currentStep; + const hasLiveEventStream = Boolean(connector.subscribe); + const clearApproval = Boolean(run.awaitingApprovalAt); + // Always touch the run on a live "running" poll — even when the step + // label is unchanged — so the stale watchdog never STALLs a + // quiet-but-alive run (an agent mid-long-tool-call emits no step change + // for a while, but it is very much alive). + await db + .$transaction(async (tx) => { + if (!hasLiveEventStream && stepChanged) { + // No SSE enrichment layer + the step advanced → append a discrete + // STEP event (which itself bumps lastEventAt + currentStep). await appendRunEvent(tx, { runId: run.id, workspaceId: run.workspaceId, @@ -808,12 +803,39 @@ async function pollActiveRuns(): Promise { currentStep: step, payload: { lastEvent: status.lastEvent ?? null, currentStep: step }, }); - }) - .catch((err) => logger.warn({ err, runId: run.id }, "runs-dispatch: step update failed")); - } + if (clearApproval) { + await tx.agentRun.update({ + where: { id: run.id }, + data: { awaitingApprovalAt: null }, + }); + } + return; + } + // Live SSE layer (granular events land there) or unchanged step: a + // cheap keep-alive bump, plus currentStep / approval-clear when they + // actually changed. + await tx.agentRun.update({ + where: { id: run.id }, + data: { + lastEventAt: new Date(), + ...(stepChanged ? { currentStep: step } : {}), + ...(clearApproval ? { awaitingApprovalAt: null } : {}), + }, + }); + }) + .catch((err) => logger.warn({ err, runId: run.id }, "runs-dispatch: step update failed")); continue; } + // A connector that can't currently determine the run's state ("unknown") + // is NOT a terminal failure: a worker restart (Codex keeps its run state in + // process memory), a momentary provider blip, or an as-yet-unrecognized + // lifecycle word all surface as unknown. Terminalizing here false-STALLs + // live work on every deploy. Leave the run ACTIVE and deliberately do NOT + // bump lastEventAt — the stale watchdog is the single arbiter of death and + // will STALL it only if it stays unknown/idle past the threshold. + if (status.state === "unknown") continue; + // Terminal — finish the run and mirror usage. Provider-side // "completed" is not enough to complete a Forge issue run; agents must // close through runs.complete so mode-specific contracts are validated. From dae3cf91b2839ffbd6de0494ed5150622e729c18 Mon Sep 17 00:00:00 2001 From: Bailey Dixon Date: Mon, 6 Jul 2026 18:28:58 -0400 Subject: [PATCH 03/21] fix(cli): O_EXCL pid lock + keep last-known agent linkage on heartbeat blip Phase-1 daemon reliability fixes (agent-runtime audit): - acquirePidLock() writes the daemon pid via an atomic O_CREAT|O_EXCL open BEFORE the SSE stream opens, closing the TOCTOU where two racing 'forge daemon start' invocations both passed the liveness check and both dispatched every event. A loser that finds a live holder refuses; a stale holder is reclaimed. - refreshLinkedAgent distinguishes a transient failure (undefined) from a genuine unlink (null); the heartbeat loop only overwrites linkedAgent on a definitive answer, so one soft agents.me blip no longer nulls the linkage and silently drops all chat + assignment dispatch for up to 60s. Co-Authored-By: Claude Opus 4.8 (1M context) --- tools/forge-cli/src/daemon.ts | 79 +++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 9 deletions(-) diff --git a/tools/forge-cli/src/daemon.ts b/tools/forge-cli/src/daemon.ts index c01ba04..4e810fb 100644 --- a/tools/forge-cli/src/daemon.ts +++ b/tools/forge-cli/src/daemon.ts @@ -148,9 +148,18 @@ async function registerOrReuseRuntime( return reg.data.id; } -async function refreshLinkedAgent(auth: AuthFile): Promise { +/** + * Resolve the agent linked to this key. Distinguishes two null-ish cases so + * the heartbeat loop can react correctly: + * - `undefined` → the call FAILED (transient rate-limit / network blip). The + * caller should KEEP its last-known linkage rather than dropping dispatch. + * - `null` → the call succeeded but no agent is linked (a real unlink). + * - `AgentMe` → the current linkage. + */ +async function refreshLinkedAgent(auth: AuthFile): Promise { const me = await callTool(auth, "agents.me", {}); - if (me.isError || !me.data) return null; + if (me.isError) return undefined; + if (!me.data) return null; return me.data; } @@ -212,6 +221,47 @@ class SeenEvents { } } +/** + * Acquire the daemon pid file as an atomic exclusive lock (O_CREAT|O_EXCL via + * the "wx" flag). This closes the TOCTOU in `startDaemon`, whose liveness + * check and the pid write were separated by the whole spawn, so two racing + * `forge daemon start` invocations could both pass the check and both open an + * SSE stream — double-dispatching every event. With an exclusive create, only + * one child wins the lock; a loser that finds a *live* holder refuses, and a + * *stale* holder is reclaimed. + */ +async function acquirePidLock(): Promise { + for (let attempt = 0; attempt < 3; attempt++) { + let handle: Awaited> | undefined; + try { + handle = await fs.open(pidPath(), "wx", 0o600); // wx = O_CREAT|O_EXCL|O_WRONLY + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== "EEXIST") throw err; + // Someone holds (or held) the lock. + const existing = await readPid(); + if (existing && isPidAlive(existing)) { + console.error( + chalk.red( + `daemon already running (pid=${existing}). Run \`forge daemon stop\` first.`, + ), + ); + process.exit(1); + } + // Dead or unreadable holder → reclaim and retry the exclusive create. + await fs.unlink(pidPath()).catch(() => {}); + continue; + } + try { + await handle.writeFile(String(process.pid) + "\n"); + } finally { + await handle.close(); + } + return; + } + console.error(chalk.red("daemon: could not acquire pid lock (racing start?).")); + process.exit(1); +} + async function runDaemon(opts: { foreground: boolean }) { const auth = await requireAuth(); const hostname = os.hostname(); @@ -227,7 +277,7 @@ async function runDaemon(opts: { foreground: boolean }) { const runtimeId = await registerOrReuseRuntime(auth, hostname, providers); console.log(chalk.green(` ✓ runtime ${runtimeId} registered`)); - let linkedAgent = await refreshLinkedAgent(auth); + let linkedAgent: AgentMe | null = (await refreshLinkedAgent(auth)) ?? null; if (linkedAgent) { console.log( chalk.gray( @@ -244,9 +294,10 @@ async function runDaemon(opts: { foreground: boolean }) { const seenEvents = new SeenEvents(); - // PID file (foreground only writes if requested too — `stop` works - // either way). - await fs.writeFile(pidPath(), String(process.pid) + "\n", { mode: 0o600 }); + // PID file as an atomic exclusive lock, acquired BEFORE the SSE stream + // opens so a second racing daemon can't also start dispatching. `stop` / + // `status` read this file either way (foreground or backgrounded). + await acquirePidLock(); let stopping = false; const stop = async () => { @@ -267,9 +318,19 @@ async function runDaemon(opts: { foreground: boolean }) { const heartbeatTimer = setInterval(async () => { try { await callTool(auth, "runtimes.heartbeat", { runtimeId }); - // Refresh agent linkage opportunistically — admins can swap the - // key's linkedAgentId without restarting the daemon. - linkedAgent = await refreshLinkedAgent(auth); + // Refresh agent linkage opportunistically — admins can swap the key's + // linkedAgentId without restarting the daemon. But only overwrite on a + // definitive answer: a transient agents.me failure returns `undefined`, + // and clobbering `linkedAgent` to null there would silently drop ALL + // chat + assignment dispatch until the next successful heartbeat. + const refreshed = await refreshLinkedAgent(auth); + if (refreshed !== undefined) { + linkedAgent = refreshed; + } else if (linkedAgent) { + console.error( + chalk.yellow("heartbeat: agent refresh failed (transient); keeping last-known linkage"), + ); + } } catch (err) { console.error(chalk.yellow(`heartbeat failed:`), err); } From bb4e041646f794c0e964e0890f5d2bc2b8f18ee7 Mon Sep 17 00:00:00 2001 From: Bailey Dixon Date: Mon, 6 Jul 2026 18:28:58 -0400 Subject: [PATCH 04/21] fix(ui): distinguish load errors from empty states; fix runtime settings dead-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase-1 runtime/orchestration UX fixes (agent-runtime audit): - Goals list/detail and Plans list/detail now render a themed 'couldn't load … Retry' state on a fetch error, instead of masking it as 'No goals yet' / 'Goal not found — may have been abandoned', which read as though work was deleted. - global.runtimes returns each runtime's home workspace; the global Runtimes settings page routes the settings gear via workspacesInUse[0] ?? homeWorkspace, so a freshly registered LOCAL_DAEMON (no workspacesInUse yet) is no longer a read-only dead-end you can't self-test or add secrets to. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/app/(app)/settings/runtimes/page.tsx | 6 ++++- .../(app)/w/[slug]/goals/[goalId]/page.tsx | 22 ++++++++++++++++ src/app/(app)/w/[slug]/goals/page.tsx | 16 ++++++++++++ .../(app)/w/[slug]/plans/[planId]/page.tsx | 26 ++++++++++++++++++- src/app/(app)/w/[slug]/plans/page.tsx | 14 +++++++++- .../orchestration-ui/use-goal-trpc.ts | 9 ++++++- src/server/routers/global.ts | 6 +++++ 7 files changed, 95 insertions(+), 4 deletions(-) diff --git a/src/app/(app)/settings/runtimes/page.tsx b/src/app/(app)/settings/runtimes/page.tsx index b0e7de9..f490250 100644 --- a/src/app/(app)/settings/runtimes/page.tsx +++ b/src/app/(app)/settings/runtimes/page.tsx @@ -122,7 +122,11 @@ export default function RuntimesPage() { : r.health.tone === "warning" ? "border-warning/30 bg-warning/5" : "border-border bg-card/40"; - const settingsWorkspace = r.workspacesInUse[0]; + // Prefer a workspace the runtime is actively used in, but + // always fall back to its home workspace so the settings link + // is never a dead-end (a freshly registered daemon has no + // workspacesInUse yet but still needs self-test / secrets). + const settingsWorkspace = r.workspacesInUse[0] ?? r.homeWorkspace; return (
diff --git a/src/app/(app)/w/[slug]/goals/[goalId]/page.tsx b/src/app/(app)/w/[slug]/goals/[goalId]/page.tsx index a13252e..092ba33 100644 --- a/src/app/(app)/w/[slug]/goals/[goalId]/page.tsx +++ b/src/app/(app)/w/[slug]/goals/[goalId]/page.tsx @@ -72,6 +72,8 @@ export default function GoalDetailPage() { const { data: crewList } = trpc.agentCrew.list.useQuery({}); const goal = query?.data; const isLoading = available ? (query?.isLoading ?? true) : false; + // A fetch failure must not read as "Goal not found — abandoned or removed". + const isError = available ? (query?.isError ?? false) : false; // Reactivity: any orchestration event invalidates the goal so the // page repaints with fresh plan/step/budget state. The `goal` router @@ -193,6 +195,26 @@ export default function GoalDetailPage() { ); } + if (isError) { + return ( + <> + +
+
+ + +
+
+ + ); + } + if (!goal) { return ( <> diff --git a/src/app/(app)/w/[slug]/goals/page.tsx b/src/app/(app)/w/[slug]/goals/page.tsx index cb06b89..326a173 100644 --- a/src/app/(app)/w/[slug]/goals/page.tsx +++ b/src/app/(app)/w/[slug]/goals/page.tsx @@ -53,6 +53,10 @@ export default function GoalsPage() { const available = Boolean(goalRouter?.list); const data = listQuery?.data; const isLoading = available ? (listQuery?.isLoading ?? true) : false; + // Distinguish a fetch FAILURE from a genuinely empty workspace — otherwise a + // transient network/server error renders "No goals yet" and reads as though + // the operator's goals were deleted. + const isError = available ? (listQuery?.isError ?? false) : false; const invalidateGoalList = () => { const goalUtils = (utils as unknown as { goal?: { list?: { invalidate?: () => void } } }).goal; @@ -155,6 +159,18 @@ export default function GoalsPage() { /> ) : isLoading ? ( + ) : isError ? ( +
+ } + title="Couldn't load goals" + description="Something went wrong fetching your goals — this is a load error, not an empty workspace. Your goals are safe." + /> + +
) : items.length === 0 ? (
); } + if (isError) { + return ( + <> + +
+
+ + +
+
+ + ); + } if (!plan) { return ( <> diff --git a/src/app/(app)/w/[slug]/plans/page.tsx b/src/app/(app)/w/[slug]/plans/page.tsx index c0de898..0ade5ea 100644 --- a/src/app/(app)/w/[slug]/plans/page.tsx +++ b/src/app/(app)/w/[slug]/plans/page.tsx @@ -227,7 +227,7 @@ export default function PlansPage() { const router = useRouter(); const utils = trpc.useUtils(); const [tab, setTab] = useState("active"); - const { data, isLoading } = trpc.executionPlan.list.useQuery( + const { data, isLoading, isError, refetch } = trpc.executionPlan.list.useQuery( tab === "archived" ? { archivedOnly: true } : { includeArchived: false }, @@ -391,6 +391,18 @@ export default function PlansPage() { {isLoading ? ( + ) : isError ? ( +
+ } + title="Couldn't load plans" + description="Something went wrong fetching plans — this is a load error, not an empty list. Nothing was deleted." + /> + +
) : items.length === 0 ? ( tab === "archived" ? ( = { useQuery: ( input: TInput, opts?: { enabled?: boolean; staleTime?: number }, - ) => { data: TOutput | undefined; isLoading: boolean }; + ) => { + data: TOutput | undefined; + isLoading: boolean; + // Surfaced so callers can distinguish a fetch FAILURE from an empty result + // (a transient error must not render as "no goals" / "goal not found"). + isError: boolean; + refetch: () => void; + }; }; type MutationHook = { diff --git a/src/server/routers/global.ts b/src/server/routers/global.ts index b314bb3..8456474 100644 --- a/src/server/routers/global.ts +++ b/src/server/routers/global.ts @@ -183,6 +183,11 @@ export const globalRouter = router({ disabledAt: true, archivedAt: true, createdAt: true, + // The runtime's own home workspace (where it was registered). Always + // present, unlike `workspacesInUse` (derived from bound agents), so the + // settings UI can always link to the per-runtime detail page — even for + // a freshly registered daemon with no agents yet. + workspace: { select: { id: true, slug: true, name: true, key: true } }, agents: { where: { archivedAt: null }, select: { @@ -222,6 +227,7 @@ export const globalRouter = router({ online: health.kind === "online", disabled: !!r.disabledAt, boundAgents: r.agents.map((a) => ({ id: a.id, profileKey: a.profileKey, name: a.name })), + homeWorkspace: r.workspace, workspacesInUse: [...workspaces.values()], }; }); From c57e512ef2faaed3d3ffc1cf394de681e8d8b92f Mon Sep 17 00:00:00 2001 From: Bailey Dixon Date: Mon, 6 Jul 2026 18:30:33 -0400 Subject: [PATCH 05/21] docs: DEVLOG + CHANGELOG for agent-runtime audit Phase 1 Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 6 ++++++ DEVLOG.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 013e824..a107e70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,12 @@ date, grouped by `Added` / `Changed` / `Fixed` / `Removed`. - **The Coach stops posting junk or duplicate comments.** It now discards empty or meta AI responses (e.g. "Posted the diagnostic comment…") instead of posting them, and won't comment on the same issue more than once in 24 hours — so a stuck issue gets one useful diagnostic, not an hourly pile-up. - **AI triage works again — and tells you what's wrong when it can't.** The "AI triage suggestion" card on an issue could get stuck showing a bare *"AI triage unavailable."* with only a Retry, because some AI providers reply in plain text instead of the structured format Forge expected — so every suggestion was thrown away. Forge now reads those plain-text replies too, so triage produces a real priority / label / agent suggestion. When triage genuinely can't run, the card now explains why and links straight to **Settings → Workspace → AI** to fix it, instead of leaving you guessing. +- **A goal or plan that hits its budget actually stops spending.** When a plan reaches its cost or time cap (or you abandon its goal), its remaining steps stop dispatching immediately — previously a blocked plan could keep launching agents and spend the very budget you were asked to approve. +- **Finished work stays finished.** A late or duplicate reviewer verdict can no longer knock a completed step back into "to-do" and re-run it. +- **Agent runs aren't falsely marked "stalled" after a deploy or restart.** A run whose runtime briefly can't report status — or that's quietly mid-work on a long step — now stays running, and is only marked stalled when it's genuinely idle. +- **Approving or stopping an agent run tells you if it didn't go through.** A failed Approve or Stop now surfaces an error instead of showing success while the run stays blocked at the runtime. +- **Goals and Plans tell a load error apart from an empty list.** A network hiccup now shows "couldn't load … Retry" instead of "No goals yet" / "Goal not found — may have been abandoned", which looked like your work had been deleted. +- **A freshly registered runtime is no longer a dead-end.** You can open a just-registered local daemon's settings — run its self-test, add credentials, bind a repo — straight from the global **Runtimes** page, without waiting for an agent to use it first. ### Changed diff --git a/DEVLOG.md b/DEVLOG.md index 17f9ec5..f063fa8 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -2,6 +2,55 @@ > Append-only session log. Read at session start. Update at session end. +## 2026-07-06 — Agent-runtime audit: Phase 1 safety fixes + +Acted on the 2026-07-06 agent-runtime audit (73 findings; report artifact + +[[agent-runtime-audit-2026-07]] memory). Phase 1 = the small, confirmed +safety/lifecycle/false-terminal fixes. Landed as four commits on branch +`worktree-audit-fixes`. + +- **Orchestration loop (`orchestration-service.ts`).** `cascadeReadiness` and + `transitionStepToReady` now load `plan.status` and early-return unless RUNNING + — a BLOCKED/CANCELED plan stops cascading finishing steps into fresh dispatch + (was: kept spending past the budget block / after abandon). `recordVerdict` + throws CONFLICT on a settled step (DONE/BLOCKED/CANCELED) so a stale/dup verdict + can't reopen finished work. `ExecutionPlan.startedAt` (migration **0091**) + stamped in `activatePlan`; `checkAndBlockBudget` measures wall-time from + `startedAt ?? createdAt` so planning + approval-wait isn't charged to execution. +- **Migration 0091** also adds `AgentRun @@index([status,lastEventAt])` + + `@@index([assignmentEventId])` — the cross-tenant 5s/60s sweeps + (`pollActiveRuns`/`ensureSubscriptions`/stale watchdog) previously seq-scanned + (every AgentRun index led with `workspaceId`). Left the pre-existing + `ExternalResource` rename-index drift out of the migration (unrelated). +- **Run dispatch (`run-dispatcher.ts` / `hermes-runs.ts`).** `getStatus` + `'unknown'` is now NON-terminal (leave ACTIVE, let the stale watchdog arbitrate) + — fixes the Codex worker-restart / deploy false-STALL and momentary blips. + `mapStatus` maps an unrecognized non-empty status → `running` (not `unknown`). + On a live `running` poll we always bump `lastEventAt` even when the step label + is unchanged, so a quiet-but-alive run isn't watchdog-STALLED. Hermes + `approve`/`stop` now inspect `res.ok` and throw; `agent-run.approve` (reject too) + surfaces the failure as a TRPCError instead of clearing the block + returning ok. + `runtime.register` now calls `assertEndpointTransport` like create/update. +- **CLI daemon (`daemon.ts`).** `acquirePidLock()` writes the pid via + `fs.open(..., "wx")` (O_CREAT|O_EXCL) before SSE opens → closes the + double-daemon TOCTOU. `refreshLinkedAgent` returns `undefined` on a transient + `agents.me` failure vs `null` on genuine unlink; the heartbeat only overwrites + `linkedAgent` on a definitive answer (was: one blip nulled linkage, dropping all + dispatch ~60s). +- **UI.** Goals/Plans list+detail get an `isError` branch (themed "couldn't load … + Retry") so a fetch failure isn't rendered as "No goals yet"/"Goal not found". + `global.runtimes` returns each runtime's `homeWorkspace`; the global Runtimes + settings gear routes via `workspacesInUse[0] ?? homeWorkspace` so a fresh + LOCAL_DAEMON isn't a read-only dead-end. Widened the `useGoalRouter` query shim + type with `isError`/`refetch`. + +Verification: `pnpm typecheck` clean; `pnpm lint` 0 errors (only pre-existing +native-`