From 349bd9c809ad0bb9c50d37bfc08b078cfdc13c9a Mon Sep 17 00:00:00 2001 From: Lex Date: Tue, 11 Aug 2026 15:07:28 +0800 Subject: [PATCH 01/11] fix(opencode): serialize goal automation state --- packages/core/src/database/migration.gen.ts | 1 + .../migration/20260811060000_goal_outcome.ts | 21 + packages/core/src/database/schema.gen.ts | 9 + packages/core/src/goal/sql.ts | 11 + packages/opencode/src/dag/runtime/loop.ts | 83 ++- packages/opencode/src/goal/CONTEXT.md | 37 ++ .../adr/0001-goal-transition-authority.md | 40 ++ packages/opencode/src/goal/goal.ts | 509 +++++++++--------- packages/opencode/src/goal/judge.ts | 12 +- packages/opencode/src/goal/loop.ts | 84 +-- packages/opencode/src/goal/prompts.ts | 11 +- packages/opencode/src/goal/state.ts | 29 +- .../opencode/src/session/automation-lease.ts | 124 +++++ packages/opencode/test/goal/e2e-loop.test.ts | 167 +++++- packages/opencode/test/goal/goal.test.ts | 141 ++++- packages/opencode/test/goal/judge.test.ts | 11 + .../test/session/automation-lease.test.ts | 45 ++ 17 files changed, 995 insertions(+), 340 deletions(-) create mode 100644 packages/core/src/database/migration/20260811060000_goal_outcome.ts create mode 100644 packages/opencode/src/goal/CONTEXT.md create mode 100644 packages/opencode/src/goal/docs/adr/0001-goal-transition-authority.md create mode 100644 packages/opencode/src/session/automation-lease.ts create mode 100644 packages/opencode/test/session/automation-lease.test.ts diff --git a/packages/core/src/database/migration.gen.ts b/packages/core/src/database/migration.gen.ts index 1c21bf23b1..aaf56d9868 100644 --- a/packages/core/src/database/migration.gen.ts +++ b/packages/core/src/database/migration.gen.ts @@ -51,5 +51,6 @@ export const migrations = ( import("./migration/20260803083938_restore_goal_state"), import("./migration/20260805094941_workflow_node_timeout_extensions"), import("./migration/20260805094942_workflow_node_escalation_pending"), + import("./migration/20260811060000_goal_outcome"), ]) ).map((module) => module.default) satisfies DatabaseMigration.Migration[] diff --git a/packages/core/src/database/migration/20260811060000_goal_outcome.ts b/packages/core/src/database/migration/20260811060000_goal_outcome.ts new file mode 100644 index 0000000000..6cc45b5b17 --- /dev/null +++ b/packages/core/src/database/migration/20260811060000_goal_outcome.ts @@ -0,0 +1,21 @@ +import { Effect } from "effect" +import type { DatabaseMigration } from "../migration" + +export default { + id: "20260811060000_goal_outcome", + up(tx) { + return Effect.gen(function* () { + yield* tx.run(` + CREATE TABLE goal_outcome ( + goal_id text PRIMARY KEY, + session_id text NOT NULL, + payload text NOT NULL, + completed_at integer NOT NULL + ); + `) + yield* tx.run( + `CREATE INDEX goal_outcome_session_completed_idx ON goal_outcome (session_id, completed_at);`, + ) + }) + }, +} satisfies DatabaseMigration.Migration diff --git a/packages/core/src/database/schema.gen.ts b/packages/core/src/database/schema.gen.ts index ac75ddb53a..224a2f04e8 100644 --- a/packages/core/src/database/schema.gen.ts +++ b/packages/core/src/database/schema.gen.ts @@ -156,6 +156,14 @@ export default { \`updated_at\` integer NOT NULL ); `) + yield* tx.run(` + CREATE TABLE \`goal_outcome\` ( + \`goal_id\` text PRIMARY KEY, + \`session_id\` text NOT NULL, + \`payload\` text NOT NULL, + \`completed_at\` integer NOT NULL + ); + `) yield* tx.run(` CREATE TABLE \`permission\` ( \`id\` text PRIMARY KEY, @@ -324,6 +332,7 @@ export default { yield* tx.run(`CREATE UNIQUE INDEX \`event_aggregate_seq_idx\` ON \`event\` (\`aggregate_id\`,\`seq\`);`) yield* tx.run(`CREATE INDEX \`event_aggregate_type_seq_idx\` ON \`event\` (\`aggregate_id\`,\`type\`,\`seq\`);`) yield* tx.run(`CREATE INDEX \`goal_state_updated_at_idx\` ON \`goal_state\` (\`updated_at\`);`) + yield* tx.run(`CREATE INDEX \`goal_outcome_session_completed_idx\` ON \`goal_outcome\` (\`session_id\`, \`completed_at\`);`) yield* tx.run( `CREATE UNIQUE INDEX \`permission_project_action_resource_idx\` ON \`permission\` (\`project_id\`,\`action\`,\`resource\`);`, ) diff --git a/packages/core/src/goal/sql.ts b/packages/core/src/goal/sql.ts index fb33057cb1..55b8a8962b 100644 --- a/packages/core/src/goal/sql.ts +++ b/packages/core/src/goal/sql.ts @@ -9,3 +9,14 @@ export const GoalStateTable = sqliteTable( }, (t) => [index("goal_state_updated_at_idx").on(t.updated_at)], ) + +export const GoalOutcomeTable = sqliteTable( + "goal_outcome", + { + goal_id: text().primaryKey(), + session_id: text().notNull(), + payload: text().notNull(), + completed_at: integer().notNull(), + }, + (t) => [index("goal_outcome_session_completed_idx").on(t.session_id, t.completed_at)], +) diff --git a/packages/opencode/src/dag/runtime/loop.ts b/packages/opencode/src/dag/runtime/loop.ts index c5bcdfd294..20def0c963 100644 --- a/packages/opencode/src/dag/runtime/loop.ts +++ b/packages/opencode/src/dag/runtime/loop.ts @@ -23,6 +23,7 @@ import { Session } from "@/session/session" import { SessionPrompt } from "@/session/prompt" import { SessionID } from "@/session/schema" import { SessionStatus } from "@/session/status" +import { SessionAutomationLease } from "@/session/automation-lease" import { renderTemplate } from "../templates/resolve" import { sanitizeInput } from "../templates/sanitize" import { DagConfig } from "../config" @@ -46,7 +47,7 @@ interface WorkflowEntry { watchers: Map> } -export const layer = Layer.effect( +const serviceLayer = Layer.effect( Service, Effect.gen(function* () { const events = yield* EventV2Bridge.Service @@ -56,6 +57,7 @@ export const layer = Layer.effect( const sessionSvc = yield* Session.Service const promptSvc = yield* SessionPrompt.Service const statusSvc = yield* SessionStatus.Service + const automation = yield* SessionAutomationLease.Service const state = yield* InstanceState.make( Effect.fn("DagLoop.state")(function* (ctx) { @@ -391,6 +393,7 @@ export const layer = Layer.effect( if (isStepping) runtime.setStepMode(true) const entry: WorkflowEntry = { runtime, semaphore, evalLock: Semaphore.makeUnsafe(1), parentSessionID: wf.sessionId, config, fibers: new Map(), watchers: new Map() } runtimes.set(dagID, entry) + yield* automation.register(SessionID.make(wf.sessionId), { kind: "dag", id: dagID }) // Reconciliation settles every persisted running attempt before the // runtime is rebuilt. Recovery never adopts or restarts provider work; // a new execution attempt must come from explicit workflow control. @@ -483,6 +486,7 @@ export const layer = Layer.effect( const semaphore = Semaphore.makeUnsafe(maxConcurrency) const entry: WorkflowEntry = { runtime, semaphore, evalLock: Semaphore.makeUnsafe(1), parentSessionID: wf.sessionId, config, fibers: new Map(), watchers: new Map() } runtimes.set(dagID, entry) + yield* automation.register(SessionID.make(wf.sessionId), { kind: "dag", id: dagID }) yield* entry.evalLock.withPermits(1)( Effect.gen(function* () { yield* spawnReady(dagID) @@ -1159,6 +1163,18 @@ export const layer = Layer.effect( : []), ].join("\n\n") + const wakeWorkflowIDs = new Set([ + ...batch.nodes.map((node) => node.workflowId), + ...batch.workflows.map((workflow) => workflow.id), + ]) + for (const workflowID of wakeWorkflowIDs) { + yield* automation.register(SessionID.make(sessionID), { kind: "dag", id: workflowID }) + } + const wakeLease = Option.getOrUndefined( + yield* automation.claim(SessionID.make(sessionID), { kind: "dag" }), + ) + if (!wakeLease) return + // Persist wake_reported AFTER successful delivery only. // A failure stays durable for a later idle event or restart scan; // it must not spin synchronously on the same row. @@ -1166,27 +1182,46 @@ export const layer = Layer.effect( // receives the node result and can act) but NOT rendered as a user // message in the TUI chat — DAG data surfaces via the sidebar panel // and Inspector, keeping the chat conversation clean. - const didDeliver = yield* promptSvc.promptIfIdle({ - sessionID: SessionID.make(sessionID), - parts: [{ type: "text", text: summary, synthetic: true }], - }).pipe( - Effect.flatMap(Option.match({ - onNone: () => Effect.succeed(false), - onSome: () => - store.markWakeBatchReported(batch).pipe( - Effect.tap(() => - Effect.sync(() => { - plan.unresponsiveDagIDs.forEach((workflowID) => - deliveredUnresponsiveDagIDs.add(workflowID), - ) - }), - ), - Effect.as(true), + const didDeliver = Option.getOrElse( + yield* automation.use( + wakeLease, + promptSvc.promptIfIdle({ + sessionID: SessionID.make(sessionID), + parts: [{ type: "text", text: summary, synthetic: true }], + }).pipe( + Effect.flatMap(Option.match({ + onNone: () => Effect.succeed(false), + onSome: () => + store.markWakeBatchReported(batch).pipe( + Effect.tap(() => + Effect.forEach( + batch.workflows.filter((workflow) => + isWorkflowTerminalStatus(workflow.status as never), + ), + (workflow) => + automation.unregister(SessionID.make(sessionID), { + kind: "dag", + id: workflow.id, + }), + { discard: true }, + ), + ), + Effect.tap(() => + Effect.sync(() => { + plan.unresponsiveDagIDs.forEach((workflowID) => + deliveredUnresponsiveDagIDs.add(workflowID), + ) + }), + ), + Effect.as(true), + ), + })), + Effect.catchCause(() => + Effect.logWarning("DAG wake delivery failed", { sessionID }).pipe(Effect.as(false)), ), - })), - Effect.catchCause(() => - Effect.logWarning("DAG wake delivery failed", { sessionID }).pipe(Effect.as(false)), + ), ), + () => false, ) if (!didDeliver) return } @@ -1260,6 +1295,12 @@ export const layer = Layer.effect( ), ) if (!snapshot.workflows.some((wf) => wf.projectId === ctx.project.id)) continue + yield* Effect.forEach( + snapshot.workflows, + (workflow) => + automation.register(SessionID.make(sessionID), { kind: "dag", id: workflow.id }), + { discard: true }, + ) yield* tryDeliverWake(sessionID).pipe(Effect.forkScoped) } @@ -1275,6 +1316,8 @@ export const layer = Layer.effect( }), ) +export const layer = serviceLayer.pipe(Layer.provide(SessionAutomationLease.defaultLayer)) + export const defaultLayer = layer.pipe( Layer.provide(EventV2Bridge.defaultLayer), Layer.provide(DagStore.defaultLayer), diff --git a/packages/opencode/src/goal/CONTEXT.md b/packages/opencode/src/goal/CONTEXT.md new file mode 100644 index 0000000000..449914813a --- /dev/null +++ b/packages/opencode/src/goal/CONTEXT.md @@ -0,0 +1,37 @@ +# Standing Goal Context + +Standing Goal keeps one durable autonomous objective for a Session and advances it only when that Session becomes idle. + +## Glossary + +| Term | Meaning | +| --- | --- | +| Goal Instance | One objective generation, identified by `goal_id`; clearing and creating a new objective creates a different instance. | +| Goal Revision | The monotonic version of one Goal Instance. Loop decisions carry the revision they observed. | +| Goal Transition | One serialized read, decision, and save/delete operation over a Goal row. | +| Goal Outcome | The durable terminal snapshot written in the same transaction that removes the current Goal row. | +| Judge Verdict | `done`, `continue`, or `blocked`; blocked is a recoverable pause, never successful completion. | +| Session Automation Lease | The process-local right to admit an autonomous prompt while a Session is idle. | + +## Invariants + +- `transition` in `goal.ts` is the only durable Goal mutation seam. +- A Goal transition reads and writes or deletes inside one immediate database transaction. +- A delayed loop decision applies only to the same `goal_id` and revision it observed. +- Terminal completion writes `goal_outcome` and deletes the current row in one transition; a durable `done` row is never an intermediate cleanup obligation. +- `blocked` pauses the Goal and remains distinguishable from `done` in state, events, transcript text, and judge prompts. +- `SessionAutomationLease` elects one automation owner per Session. DAG owns the Session while any registered workflow remains; Goal is eligible only after the final DAG owner releases it. +- Goal and DAG effects revalidate the claimed generation immediately before mutation or prompt admission. `SessionPrompt.promptIfIdle` remains the final idle-state guard. +- The current Session runner is process-local, so the automation lease is process-local. Clustered execution requires a separate durable lease design. + +## Boundaries + +- `Goal` owns durable state transitions and Goal lifecycle events. +- `GoalJudge` owns verdict parsing and transport-failure fallback. +- `GoalLoop` observes idle Sessions, asks the judge, submits version-bound transitions, and requests the shared Session automation lease. +- `SessionAutomationLease` owns Goal/DAG arbitration; `SessionPrompt` and `SessionRunState` own final prompt admission and runner idleness. +- `DagLoop` and `GoalLoop` may both observe one Session, but neither may mutate from an unverified automation claim or bypass `promptIfIdle` for autonomous driving. + +## Decisions + +- [ADR-0001: Serialized Goal transitions and shared Session automation admission](docs/adr/0001-goal-transition-authority.md) diff --git a/packages/opencode/src/goal/docs/adr/0001-goal-transition-authority.md b/packages/opencode/src/goal/docs/adr/0001-goal-transition-authority.md new file mode 100644 index 0000000000..6f29f22716 --- /dev/null +++ b/packages/opencode/src/goal/docs/adr/0001-goal-transition-authority.md @@ -0,0 +1,40 @@ +# ADR-0001: Serialized Goal transitions and shared Session automation admission + +- Status: Accepted +- Date: 2026-08-11 + +## Context + +Goal commands and GoalLoop previously loaded a row and later performed an unconditional upsert. A legal `pause` or `clear` racing a delayed judge result could therefore be overwritten or resurrected. Judge completion also persisted `done` and deleted it in a second operation, so a process failure between them left a terminal row that no loop would process. + +GoalLoop admitted continuation with `SessionPrompt.prompt`, while DagLoop admitted parent wakes with `promptIfIdle`. Both could observe the same idle Session and independently start automation. + +The judge also encoded blocked or unachievable work as successful completion, so presentation reported an achieved Goal without a deliverable. + +## Decision + +All durable Goal mutations go through one `transition` function. It uses an immediate database transaction to read the current row, decide from that row, and save or delete before releasing the write lock. Goal instances carry `goal_id` and `revision`; delayed judge work supplies both values and becomes a no-op if either changed. + +A `done` verdict writes an immutable `goal_outcome` snapshot and deletes the current row in the same transaction. It returns that snapshot for the `goal.updated(done)` followed by `goal.cleared` presentation contract. No durable done cleanup phase remains, while completion remains queryable after a process failure. + +Judge output is tri-state: `done`, `continue`, or `blocked`. `blocked` writes a paused Goal with the blocker as its reason. + +`SessionAutomationLease` is the process-local authority for Goal/DAG ownership. Goal and DAG register their active identities; DAG has priority while any workflow is registered. A claim carries a generation that is revalidated immediately before a state transition or autonomous prompt. Registration changes invalidate older claims. After that ownership check, `SessionPrompt.promptIfIdle` remains the final atomic idle-state admission guard. Failure at either boundary admits no Goal prompt and leaves the durable Goal available for a later idle event. + +## Consequences + +- Pause and clear cannot be overwritten by a stale judge decision. +- A judge result from a cleared Goal cannot mutate a replacement Goal in the same Session. +- Completion cannot strand a durable done row. +- Completion leaves one durable terminal outcome even though the current Goal view is empty. +- Blocked work is visible and resumable without being reported as achieved. +- Goal and DAG automation cannot concurrently admit two turns into one process-local Session. +- Clustered Session execution will need a durable lease before Session drains stop being process-local. + +## Alternatives Considered + +- Compare timestamps before unconditional upsert: rejected because it leaves read/write split and depends on clock uniqueness. +- Add only an in-memory Goal mutex: rejected because separate processes can still update the same database. +- Keep boolean judge output and infer blocked from reason text: rejected because state semantics would depend on unstructured language. +- Rely on the prompt mutex alone: rejected because the judge can mutate Goal state before prompt admission and because prompt serialization does not elect a Goal/DAG owner. +- Add a second Goal-specific prompt mutex: rejected because it would not coordinate with DagLoop. diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index 36eda34c7a..6dd1b9bc36 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -1,19 +1,26 @@ export * as Goal from "./goal" import { Effect, Layer, Context, Schema, Fiber } from "effect" -import { eq } from "drizzle-orm" +import { desc, eq } from "drizzle-orm" import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { Database } from "@opencode-ai/core/database/database" import { EventV2Bridge } from "@/event-v2-bridge" import { GoalState } from "./state" -import { GoalStateTable } from "@opencode-ai/core/goal/sql" +import { GoalOutcomeTable, GoalStateTable } from "@opencode-ai/core/goal/sql" import { GoalEvent } from "./events" import { GoalPrompts } from "./prompts" import { SessionID } from "@/session/schema" import { SessionStatus } from "@/session/status" +import { SessionAutomationLease } from "@/session/automation-lease" + +export type RemoveSubgoalResult = + | { tag: "ok"; removed: string; state: GoalState.Info } + | { tag: "noState" } + | { tag: "outOfBounds"; size: number } export interface Interface { readonly load: (sessionID: SessionID) => Effect.Effect + readonly lastOutcome: (sessionID: SessionID) => Effect.Effect readonly set: (sessionID: SessionID, goal: string, maxTurns?: number) => Effect.Effect readonly pause: (sessionID: SessionID, reason: string) => Effect.Effect readonly resume: (sessionID: SessionID) => Effect.Effect @@ -24,11 +31,7 @@ export interface Interface { sessionID: SessionID, /** 1-based index of the subgoal to remove (1 = first subgoal). */ index: number, - ) => Effect.Effect< - | { tag: "ok"; removed: string; state: GoalState.Info } - | { tag: "noState" } - | { tag: "outOfBounds"; size: number } - > + ) => Effect.Effect readonly clearSubgoals: (sessionID: SessionID) => Effect.Effect readonly statusLine: (sessionID: SessionID) => Effect.Effect readonly dispatch: (sessionID: SessionID, args: string) => Effect.Effect<{ @@ -42,9 +45,10 @@ export interface Interface { }> readonly updateAfterJudge: ( sessionID: SessionID, - verdict: "done" | "continue", + verdict: GoalState.Verdict, reason: string, parseFailed: boolean, + expected?: { readonly goalID: string; readonly revision: number }, ) => Effect.Effect< | { state: GoalState.Info @@ -99,12 +103,13 @@ export interface Interface { export class Service extends Context.Service()("@opencode/Goal") {} -export const layer = Layer.effect( +const serviceLayer = Layer.effect( Service, Effect.gen(function* () { const events = yield* EventV2Bridge.Service const { db } = yield* Database.Service const sessionStatus = yield* SessionStatus.Service + const automation = yield* SessionAutomationLease.Service // Unified event publisher — every state change publishes goal.updated // with the full snapshot, identical to Todo's todo.updated pattern. @@ -154,49 +159,6 @@ export const layer = Layer.effect( } }) - // Terminal cleanup for "done" transitions. Loads current state (if any), - // constructs a transient snapshot with status="done" + the given reason, - // emits goal.updated(done), deletes the row, then emits goal.cleared. - // - // Does NOT touch the fiber map. This is the key safety property: - // - markDone (user-initiated from slash command or goal.complete - // tool) calls clearFiber FIRST, then deleteAndPublishDone — the - // loop fiber is already stopped when this runs. - // - loop.ts done branch calls deleteAndPublishDone DIRECTLY from - // inside the loop fiber — so it must not self-interrupt. - // - // Without this separation, calling goal.clear() from within afterIdle - // would interrupt ourselves before goal.cleared was published (the - // event bus would miss the terminal event, and TUI/SSE consumers - // polling state would never see the transition). - // - // The whole terminal sequence (load → publish(done) → delete → - // publish(cleared)) runs inside Effect.uninterruptible. This is - // defense-in-depth (F1): even if a future caller arranges for the loop - // fiber to be interrupted mid-call, the terminal event contract still - // completes atomically — goal.cleared cannot be skipped by an interrupt - // landing between publish(done) and publish(cleared). The operations are - // short synchronous DB + event publishes, so there is no deadlock risk. - const deleteAndPublishDone = Effect.fnUntraced(function* (sessionID: SessionID, reason: string) { - return yield* Effect.uninterruptible( - Effect.gen(function* () { - const state = yield* loadState(sessionID) - if (state) { - const doneState = new GoalState.Info({ - ...state, - status: "done", - last_verdict: "done", - last_reason: reason, - }) - yield* publishGoal(sessionID, doneState) - } - yield* deleteState(sessionID) - yield* events.publish(GoalEvent.Cleared, { sessionID }) - return state - }), - ) - }) - function loadState(sessionID: SessionID) { return db .select() @@ -212,34 +174,135 @@ export const layer = Layer.effect( ) } - function saveState(sessionID: SessionID, state: GoalState.Info) { - const payload = JSON.stringify(Schema.encodeSync(GoalState.Info)(state)) - return db - .insert(GoalStateTable) - .values({ session_id: sessionID, payload, updated_at: Date.now() }) - .onConflictDoUpdate({ - target: GoalStateTable.session_id, - set: { payload, updated_at: Date.now() }, - }) - .run() - .pipe(Effect.orDie) - } + type Transition = + | { readonly tag: "noop"; readonly value: A } + | { readonly tag: "save"; readonly state: GoalState.Info; readonly value: A } + | { + readonly tag: "delete" + readonly terminal?: GoalState.Info + readonly value: A + } - function deleteState(sessionID: SessionID) { - return db - .delete(GoalStateTable) - .where(eq(GoalStateTable.session_id, sessionID)) - .run() - .pipe(Effect.orDie) - } + // The only durable Goal mutation seam. The immediate transaction makes the + // read + decision + write/delete one serializable state transition, so a + // stale loop result cannot overwrite a concurrent pause or resurrect a row + // deleted by clear. Events are emitted after commit but inside the same + // uninterruptible region; durable state always leads presentation state. + const transition = ( + sessionID: SessionID, + decide: (state: GoalState.Info | undefined) => Transition, + ) => + Effect.uninterruptible( + Effect.gen(function* () { + const result = yield* db + .transaction( + (tx) => + Effect.gen(function* () { + const row = yield* tx + .select() + .from(GoalStateTable) + .where(eq(GoalStateTable.session_id, sessionID)) + .get() + const current = row + ? Schema.decodeUnknownSync(GoalState.Info)(JSON.parse(row.payload)) + : undefined + const next = decide(current) + if (next.tag === "save") { + const payload = JSON.stringify(Schema.encodeSync(GoalState.Info)(next.state)) + if (row) { + yield* tx + .update(GoalStateTable) + .set({ payload, updated_at: Math.max(Date.now(), row.updated_at + 1) }) + .where(eq(GoalStateTable.session_id, sessionID)) + .run() + } else { + yield* tx + .insert(GoalStateTable) + .values({ session_id: sessionID, payload, updated_at: Date.now() }) + .run() + } + } + if (next.tag === "delete" && row) { + if (next.terminal) { + const payload = JSON.stringify(Schema.encodeSync(GoalState.Info)(next.terminal)) + const goalID = + next.terminal.goal_id && next.terminal.goal_id !== "legacy" + ? next.terminal.goal_id + : `${sessionID}:legacy:${next.terminal.created_at}` + yield* tx + .insert(GoalOutcomeTable) + .values({ + goal_id: goalID, + session_id: sessionID, + payload, + completed_at: Date.now(), + }) + .onConflictDoUpdate({ + target: GoalOutcomeTable.goal_id, + set: { payload, completed_at: Date.now() }, + }) + .run() + } + yield* tx + .delete(GoalStateTable) + .where(eq(GoalStateTable.session_id, sessionID)) + .run() + } + return next + }), + { behavior: "immediate" }, + ) + .pipe(Effect.orDie) + if (result.tag === "save") yield* publishGoal(sessionID, result.state) + if (result.tag === "delete") { + if (result.terminal) yield* publishGoal(sessionID, result.terminal) + yield* events.publish(GoalEvent.Cleared, { sessionID }) + } + return result.value + }), + ) + + const matchesExpected = ( + state: GoalState.Info, + expected?: { readonly goalID: string; readonly revision: number }, + ) => + !expected || + ((state.goal_id ?? "legacy") === expected.goalID && (state.revision ?? 0) === expected.revision) + + const deleteAndPublishDone = Effect.fnUntraced(function* (sessionID: SessionID, reason: string) { + return yield* transition(sessionID, (state) => { + if (!state) return { tag: "noop", value: undefined } + const doneState = GoalState.advance(state, { + status: "done", + last_verdict: "done", + last_reason: reason, + }) + return { tag: "delete", terminal: doneState, value: state } + }) + }) const load = Effect.fn("Goal.load")(function* (sessionID: SessionID) { return yield* loadState(sessionID) }) + const lastOutcome = Effect.fn("Goal.lastOutcome")(function* (sessionID: SessionID) { + const row = yield* db + .select() + .from(GoalOutcomeTable) + .where(eq(GoalOutcomeTable.session_id, sessionID)) + .orderBy(desc(GoalOutcomeTable.completed_at)) + .limit(1) + .get() + .pipe(Effect.orDie) + if (!row) return undefined + return Schema.decodeUnknownSync(GoalState.Info)(JSON.parse(row.payload)) + }) + const set = Effect.fn("Goal.set")(function* (sessionID: SessionID, goal: string, maxTurns?: number) { const now = Date.now() const state = new GoalState.Info({ + goal_id: Bun.randomUUIDv7(), + revision: GoalState.nni(0), goal, status: "active", turns_used: GoalState.nni(0), @@ -249,23 +312,24 @@ export const layer = Layer.effect( consecutive_parse_failures: GoalState.nni(0), subgoals: [], }) - yield* saveState(sessionID, state) - yield* publishGoal(sessionID, state) - return state + const result = yield* transition(sessionID, () => ({ tag: "save", state, value: state })) + yield* automation.register(sessionID, { kind: "goal", id: result.goal_id ?? "legacy" }) + return result }) const pause = Effect.fn("Goal.pause")(function* (sessionID: SessionID, reason: string) { - const state = yield* loadState(sessionID) - if (!state || state.status !== "active") return undefined - const updated = new GoalState.Info({ - ...state, - status: "paused", - paused_reason: reason, - last_turn_at: Date.now(), + const updated = yield* transition(sessionID, (state) => { + if (!state || state.status !== "active") return { tag: "noop", value: undefined } + const next = GoalState.advance(state, { + status: "paused", + paused_reason: reason, + last_turn_at: Date.now(), + }) + return { tag: "save", state: next, value: next } }) - yield* saveState(sessionID, updated) + if (!updated) return undefined + yield* automation.unregister(sessionID, { kind: "goal", id: updated.goal_id ?? "legacy" }) yield* clearFiber(sessionID) - yield* publishGoal(sessionID, updated) return updated }) @@ -279,47 +343,38 @@ export const layer = Layer.effect( // and publishing goal.updated(paused) can never leave a paused DB row // with no corresponding event on the bus. const pauseAndPublish = Effect.fnUntraced(function* (sessionID: SessionID, reason: string) { - return yield* Effect.uninterruptible( - Effect.gen(function* () { - const state = yield* loadState(sessionID) - if (!state || state.status !== "active") return undefined - const updated = new GoalState.Info({ - ...state, - status: "paused", - paused_reason: reason, - last_turn_at: Date.now(), - }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return updated - }), - ) + return yield* transition(sessionID, (state) => { + if (!state || state.status !== "active") return { tag: "noop", value: undefined } + const updated = GoalState.advance(state, { + status: "paused", + paused_reason: reason, + last_turn_at: Date.now(), + }) + return { tag: "save", state: updated, value: updated } + }) }) const resume = Effect.fn("Goal.resume")(function* (sessionID: SessionID) { - const state = yield* loadState(sessionID) - if (!state || state.status !== "paused") return undefined - // Preserve turns_used so the original max_turns budget is respected. - // Resetting to 0 would silently grant another full budget, defeating - // `max_turns` as a runaway guard — a paused goal that exhausted its - // budget would immediately re-exhaust the new budget on resume. - // Users wanting a fresh budget should /goal clear and /goal . - const updated = new GoalState.Info({ - ...state, - status: "active", - consecutive_parse_failures: GoalState.nni(0), - paused_reason: undefined, - last_turn_at: Date.now(), + const updated = yield* transition(sessionID, (state) => { + if (!state || state.status !== "paused") return { tag: "noop", value: undefined } + const updated = GoalState.advance(state, { + status: "active", + consecutive_parse_failures: GoalState.nni(0), + paused_reason: undefined, + last_turn_at: Date.now(), + }) + return { tag: "save", state: updated, value: updated } }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) + if (updated) + yield* automation.register(sessionID, { kind: "goal", id: updated.goal_id ?? "legacy" }) return updated }) const clear = Effect.fn("Goal.clear")(function* (sessionID: SessionID) { - yield* deleteState(sessionID) + const cleared = yield* transition(sessionID, (state) => ({ tag: "delete", value: state })) + if (cleared) + yield* automation.unregister(sessionID, { kind: "goal", id: cleared.goal_id ?? "legacy" }) yield* clearFiber(sessionID) - yield* events.publish(GoalEvent.Cleared, { sessionID }) }) const markDone = Effect.fn("Goal.markDone")(function* (sessionID: SessionID, reason: string) { @@ -331,50 +386,48 @@ export const layer = Layer.effect( // row (preserving whatever turns_used a prior continue dispatch set) and // re-renders the done snapshot from it. yield* clearFiber(sessionID) - return yield* deleteAndPublishDone(sessionID, reason) + const completed = yield* deleteAndPublishDone(sessionID, reason) + if (completed) + yield* automation.unregister(sessionID, { kind: "goal", id: completed.goal_id ?? "legacy" }) + return completed }) const addSubgoal = Effect.fn("Goal.addSubgoal")(function* (sessionID: SessionID, subgoal: string) { - const state = yield* loadState(sessionID) - if (!state) return undefined - const updated = new GoalState.Info({ - ...state, - subgoals: [...(state.subgoals ?? []), subgoal], - last_turn_at: Date.now(), + return yield* transition(sessionID, (state) => { + if (!state) return { tag: "noop", value: undefined } + const updated = GoalState.advance(state, { + subgoals: [...(state.subgoals ?? []), subgoal], + last_turn_at: Date.now(), + }) + return { tag: "save", state: updated, value: updated } }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return updated }) const removeSubgoal = Effect.fn("Goal.removeSubgoal")(function* (sessionID: SessionID, index: number) { - const state = yield* loadState(sessionID) - if (!state) return { tag: "noState" as const } - const subgoals = state.subgoals ?? [] - const idx = index - 1 - if (idx < 0 || idx >= subgoals.length) return { tag: "outOfBounds" as const, size: subgoals.length } - const removed = subgoals[idx] - const updated = new GoalState.Info({ - ...state, - subgoals: subgoals.filter((_, i) => i !== idx), - last_turn_at: Date.now(), + return yield* transition(sessionID, (state) => { + if (!state) return { tag: "noop", value: { tag: "noState" as const } } + const subgoals = state.subgoals ?? [] + const idx = index - 1 + if (idx < 0 || idx >= subgoals.length) + return { tag: "noop", value: { tag: "outOfBounds" as const, size: subgoals.length } } + const removed = subgoals[idx] + const updated = GoalState.advance(state, { + subgoals: subgoals.filter((_, i) => i !== idx), + last_turn_at: Date.now(), + }) + return { tag: "save", state: updated, value: { tag: "ok" as const, removed, state: updated } } }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return { tag: "ok" as const, removed, state: updated } }) const clearSubgoals = Effect.fn("Goal.clearSubgoals")(function* (sessionID: SessionID) { - const state = yield* loadState(sessionID) - if (!state) return undefined - const updated = new GoalState.Info({ - ...state, - subgoals: [], - last_turn_at: Date.now(), + return yield* transition(sessionID, (state) => { + if (!state) return { tag: "noop", value: undefined } + const updated = GoalState.advance(state, { + subgoals: [], + last_turn_at: Date.now(), + }) + return { tag: "save", state: updated, value: updated } }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return updated }) const statusLine = Effect.fn("Goal.statusLine")(function* (sessionID: SessionID) { @@ -395,82 +448,65 @@ export const layer = Layer.effect( const updateAfterJudge = Effect.fn("Goal.updateAfterJudge")(function* ( sessionID: SessionID, - verdict: "done" | "continue", + verdict: GoalState.Verdict, reason: string, parseFailed: boolean, + expected?: { readonly goalID: string; readonly revision: number }, ) { - const state = yield* loadState(sessionID) - if (!state || state.status !== "active") return undefined - - const now = Date.now() - const newParseFailures = parseFailed ? state.consecutive_parse_failures + 1 : 0 - - if (verdict === "done") { - const updated = new GoalState.Info({ - ...state, - status: "done", - // State transitions are budget-neutral — a `done` verdict drives no - // continuation dispatch, so it must NOT consume budget. turns_used - // reflects only continuation dispatches (see spec: - // turn-budget-counts-continuation-dispatches-only). - turns_used: state.turns_used, - last_turn_at: now, - last_verdict: "done", - last_reason: reason, - consecutive_parse_failures: GoalState.nni(newParseFailures), - }) - yield* saveState(sessionID, updated) - // Do NOT publish goal.updated here. deleteAndPublishDone is the SOLE - // owner of the terminal event sequence (goal.updated(done) → delete → - // goal.cleared); publishing here would double-fire goal.updated(done) - // on every judge-declared completion (see spec: - // terminal-event-contract-publishes-exactly-once). We still saveState - // so deleteAndPublishDone can load the done row and re-render the - // snapshot. loop.ts invokes deleteAndPublishDone after this returns. - return { - state: updated, - shouldContinue: false, - message: `✓ 目标已达成：${reason}`, + return yield* transition(sessionID, (state) => { + if (!state || state.status !== "active" || !matchesExpected(state, expected)) + return { tag: "noop", value: undefined } + + const now = Date.now() + const newParseFailures = parseFailed ? state.consecutive_parse_failures + 1 : 0 + if (verdict === "done") { + const updated = GoalState.advance(state, { + status: "done", + last_turn_at: now, + last_verdict: "done", + last_reason: reason, + consecutive_parse_failures: GoalState.nni(newParseFailures), + }) + return { + tag: "delete", + terminal: updated, + value: { + state: updated, + shouldContinue: false, + message: `✓ 目标已达成：${reason}`, + }, + } } - } - - const turnsUsed = GoalState.nni(state.turns_used + 1) - if (newParseFailures >= GoalPrompts.MAX_CONSECUTIVE_PARSE_FAILURES) { - const pauseReason = - "judge 模型未返回有效 JSON 判定。请检查模型配置或换用更可靠的模型，然后 /goal resume。" - const updated = new GoalState.Info({ - ...state, - status: "paused", - turns_used: turnsUsed, - last_turn_at: now, - last_verdict: "continue", - last_reason: reason, - paused_reason: pauseReason, - consecutive_parse_failures: GoalState.nni(newParseFailures), - }) - // Do NOT call clearFiber here. updateAfterJudge is inlined into - // GoalLoop.afterIdle (loop.ts:122), so the fiber running this code - // IS the one registered in the fibers map — clearFiber would - // self-interrupt before publishGoal reaches the event bus, leaving - // the pause invisible to SSE/TUI and aborting the rest of afterIdle. - // The fiber naturally terminates when afterIdle returns; no explicit - // interrupt is needed (same rationale as pauseAndPublish / - // deleteAndPublishDone). - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return { - state: updated, - shouldContinue: false, - message: `⏸ 目标已暂停 — ${pauseReason}`, + if (verdict === "blocked") { + const updated = GoalState.advance(state, { + status: "paused", + last_turn_at: now, + last_verdict: "blocked", + last_reason: reason, + paused_reason: reason, + consecutive_parse_failures: GoalState.nni(newParseFailures), + }) + return { + tag: "save", + state: updated, + value: { + state: updated, + shouldContinue: false, + message: `⏸ 目标已阻塞 — ${reason}`, + }, + } } - } - if (turnsUsed >= state.max_turns) { - const pauseReason = `已用 ${turnsUsed}/${state.max_turns} 轮。使用 /goal resume 继续，或 /goal clear 停止。` - const updated = new GoalState.Info({ - ...state, - status: "paused", + const turnsUsed = GoalState.nni(state.turns_used + 1) + const pauseReason = + newParseFailures >= GoalPrompts.MAX_CONSECUTIVE_PARSE_FAILURES + ? "judge 模型未返回有效 JSON 判定。请检查模型配置或换用更可靠的模型，然后 /goal resume。" + : turnsUsed >= state.max_turns + ? `已用 ${turnsUsed}/${state.max_turns} 轮。使用 /goal resume 继续，或 /goal clear 停止。` + : undefined + const updated = GoalState.advance(state, { + status: pauseReason ? "paused" : "active", turns_used: turnsUsed, last_turn_at: now, last_verdict: "continue", @@ -478,34 +514,18 @@ export const layer = Layer.effect( paused_reason: pauseReason, consecutive_parse_failures: GoalState.nni(newParseFailures), }) - // Same self-interrupt hazard as the parse-failure branch above: we - // are running inside the afterIdle loop fiber, so clearFiber would - // interrupt ourselves before publishGoal(paused) fires. - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) return { + tag: "save", state: updated, - shouldContinue: false, - message: `⏸ 目标已暂停 — ${pauseReason}`, + value: { + state: updated, + shouldContinue: !pauseReason, + message: pauseReason + ? `⏸ 目标已暂停 — ${pauseReason}` + : `↻ 继续推进目标（${updated.turns_used}/${updated.max_turns}）：${reason}`, + }, } - } - - const updated = new GoalState.Info({ - ...state, - status: "active", - turns_used: turnsUsed, - last_turn_at: now, - last_verdict: "continue", - last_reason: reason, - consecutive_parse_failures: GoalState.nni(newParseFailures), }) - yield* saveState(sessionID, updated) - yield* publishGoal(sessionID, updated) - return { - state: updated, - shouldContinue: true, - message: `↻ 继续推进目标（${updated.turns_used}/${updated.max_turns}）：${reason}`, - } }) const dispatch = Effect.fn("Goal.dispatch")(function* (sessionID: SessionID, args: string) { @@ -678,6 +698,7 @@ export const layer = Layer.effect( return Service.of({ load, + lastOutcome, set, pause, resume, @@ -699,10 +720,16 @@ export const layer = Layer.effect( }), ) +export const layer = serviceLayer.pipe(Layer.provide(SessionAutomationLease.defaultLayer)) + export const defaultLayer = layer.pipe( Layer.provide(SessionStatus.defaultLayer), Layer.provide(EventV2Bridge.defaultLayer), Layer.provide(Database.defaultLayer), ) -export const node = LayerNode.make(layer, [EventV2Bridge.node, Database.node, SessionStatus.node]) +export const node = LayerNode.make(layer, [ + EventV2Bridge.node, + Database.node, + SessionStatus.node, +]) diff --git a/packages/opencode/src/goal/judge.ts b/packages/opencode/src/goal/judge.ts index 0f2bedf712..16846e986d 100644 --- a/packages/opencode/src/goal/judge.ts +++ b/packages/opencode/src/goal/judge.ts @@ -4,7 +4,7 @@ import { Effect } from "effect" import { GoalPrompts } from "./prompts" export interface JudgeResult { - readonly verdict: "done" | "continue" + readonly verdict: "done" | "continue" | "blocked" readonly reason: string readonly parseFailed: boolean } @@ -16,6 +16,11 @@ export function parseJudgeResponse(raw: string): JudgeResult { // Step 2: try JSON.parse whole string try { const obj = JSON.parse(stripped) + if ( + (obj.verdict === "done" || obj.verdict === "continue" || obj.verdict === "blocked") && + typeof obj.reason === "string" + ) + return { verdict: obj.verdict, reason: obj.reason, parseFailed: false } if (typeof obj.done === "boolean" && typeof obj.reason === "string") return { verdict: obj.done ? "done" : "continue", reason: obj.reason, parseFailed: false } } catch {} @@ -25,6 +30,11 @@ export function parseJudgeResponse(raw: string): JudgeResult { if (match) { try { const obj = JSON.parse(match[0]) + if ( + (obj.verdict === "done" || obj.verdict === "continue" || obj.verdict === "blocked") && + typeof obj.reason === "string" + ) + return { verdict: obj.verdict, reason: obj.reason, parseFailed: false } if (typeof obj.done === "boolean" && typeof obj.reason === "string") return { verdict: obj.done ? "done" : "continue", reason: obj.reason, parseFailed: false } } catch {} diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index 31b748cb3c..122e824624 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -13,6 +13,7 @@ import { GoalJudge } from "./judge" import { GoalPrompts } from "./prompts" import { generateText } from "ai" import { SessionID } from "@/session/schema" +import { SessionAutomationLease } from "@/session/automation-lease" export interface Interface { readonly init: () => Effect.Effect @@ -97,7 +98,7 @@ export function isStaleZombie( ) } -export const layer = Layer.effect( +const serviceLayer = Layer.effect( Service, Effect.gen(function* () { const events = yield* EventV2Bridge.Service @@ -106,6 +107,14 @@ export const layer = Layer.effect( const provider = yield* Provider.Service const goal = yield* Goal.Service const status = yield* SessionStatus.Service + const automation = yield* SessionAutomationLease.Service + + const pauseGoal = Effect.fnUntraced(function* (sessionID: SessionID, reason: string) { + const paused = yield* goal.pauseAndPublish(sessionID, reason) + if (paused) + yield* automation.unregister(sessionID, { kind: "goal", id: paused.goal_id ?? "legacy" }) + return paused + }) const state = yield* InstanceState.make( Effect.fn("GoalLoop.state")(function* (_ctx) { @@ -150,6 +159,10 @@ export const layer = Layer.effect( const afterIdle = Effect.fn("GoalLoop.afterIdle")(function* (sessionID: SessionID) { const goalState = yield* goal.load(sessionID) if (!goalState || goalState.status !== "active") return + const goalOwner = { kind: "goal" as const, id: goalState.goal_id ?? "legacy" } + yield* automation.register(sessionID, goalOwner) + const observedLease = Option.getOrUndefined(yield* automation.claim(sessionID, goalOwner)) + if (!observedLease) return // Zombie-goal freshness guard (D6). If the goal is active but has run // zero continuations and is older than FRESHNESS_THRESHOLD, the initial @@ -172,12 +185,10 @@ export const layer = Layer.effect( const probeMsgs = yield* sessions.messages({ sessionID, limit: 1 }) const hasAssistant = probeMsgs.some((m) => m.info.role === "assistant") if (isStaleZombie(goalState, hasAssistant)) { - yield* goal - .pauseAndPublish( + yield* pauseGoal( sessionID, `initial kick produced no assistant response within ${GoalPrompts.FRESHNESS_THRESHOLD / 1000}s — likely provider error or model refusal. Use /goal resume to retry.`, - ) - .pipe(Effect.ignore) + ).pipe(Effect.ignore) return } } @@ -189,7 +200,7 @@ export const layer = Layer.effect( // been compacted or the initial kick failed after the stale-zombie // guard window. Pause visibly instead of silently stalling. const pauseMsg = "近期消息中无 assistant 回复，目标已暂停。使用 /goal resume 重试。" - yield* goal.pauseAndPublish(sessionID, pauseMsg).pipe(Effect.ignore) + yield* pauseGoal(sessionID, pauseMsg).pipe(Effect.ignore) yield* promptSvc.prompt({ sessionID, noReply: true, parts: [{ type: "text", text: `⏸ 目标已暂停 — ${pauseMsg}` }] }).pipe(Effect.ignore) return } @@ -240,30 +251,26 @@ export const layer = Layer.effect( ) : { verdict: "continue" as const, reason: "上一轮无文本输出（纯工具调用），跳过判定直接继续", parseFailed: false } - const updateResult = yield* goal.updateAfterJudge(sessionID, verdict.verdict, verdict.reason, verdict.parseFailed) + const updateResult = Option.getOrUndefined( + yield* automation.use( + observedLease, + goal.updateAfterJudge( + sessionID, + verdict.verdict, + verdict.reason, + verdict.parseFailed, + { + goalID: goalState.goal_id ?? "legacy", + revision: goalState.revision ?? 0, + }, + ), + ), + ) if (!updateResult) return if (!updateResult.shouldContinue) { - // Inject visible completion message when goal is achieved, then - // auto-clear the goal state. `updateAfterJudge` already persisted - // a done snapshot and published goal.updated — that snapshot is - // only kept long enough to emit the completion message, then the - // row is removed so done is a transient visual-only state (mirrors - // how /goal clear behaves). This is what makes goal completion - // not require a manual /goal clear afterwards. + yield* automation.unregister(sessionID, goalOwner) if (verdict.verdict === "done") { - // Run the terminal event sequence FIRST (F1): publish(done) → - // delete → publish(cleared) is the contract SSE/TUI consumers - // rely on, so it must complete before any other effect that could - // race the loop fiber. deleteAndPublishDone is uninterruptible and - // fiber-safe (no clearFiber), so this ordering is pure - // defense-in-depth — the completion message text is computed from - // updateResult.message (pre-deletion state) and is unaffected by - // running after the delete. The noReply path returns before any - // status transition today, but completing the terminal sequence - // first makes the contract structurally enforced rather than - // dependent on that noReply implementation detail. - yield* goal.deleteAndPublishDone(sessionID, verdict.reason).pipe(Effect.ignore) yield* promptSvc.prompt({ sessionID, noReply: true, @@ -295,7 +302,7 @@ export const layer = Layer.effect( // "active" with no continuation. Pause with a visible reason so the // user knows the loop was interrupted by a status change. const pauseMsg = `judge 期间会话状态变化（${currentStatus.type}），目标已暂停` - yield* goal.pauseAndPublish(sessionID, pauseMsg).pipe(Effect.ignore) + yield* pauseGoal(sessionID, pauseMsg).pipe(Effect.ignore) yield* promptSvc.prompt({ sessionID, noReply: true, parts: [{ type: "text", text: `⏸ 目标已暂停 — ${pauseMsg}` }] }).pipe(Effect.ignore) return } @@ -311,7 +318,7 @@ export const layer = Layer.effect( // publishGoal(paused) reaches the event bus. Use pauseAndPublish // which skips fiber management — the fiber naturally terminates // when this function returns. - yield* goal.pauseAndPublish(sessionID, "当前轮被中断").pipe(Effect.ignore) // user preempted + yield* pauseGoal(sessionID, "当前轮被中断").pipe(Effect.ignore) // user preempted return } @@ -345,12 +352,14 @@ export const layer = Layer.effect( // cause (recoverable failures + defects) and transition to a recoverable // paused state via the fiber-safe pauseAndPublish (goal.pause would // clearFiber — us — mid-publish; see the preempt branches above). - yield* promptSvc - .prompt({ + const continuationLease = Option.getOrUndefined(yield* automation.claim(sessionID, goalOwner)) + if (!continuationLease) return + yield* automation.use( + continuationLease, + promptSvc.promptIfIdle({ sessionID, parts: [{ type: "text", text: continuationText }], - }) - .pipe( + }).pipe( Effect.catchCause((cause) => Effect.gen(function* () { // F1: Only pause for non-interrupt causes. An interrupt (user @@ -372,15 +381,20 @@ export const layer = Layer.effect( // misclassified as a dispatch failure and spuriously paused here. if (Cause.hasInterrupts(cause)) { yield* Effect.logInfo("goal continuation interrupted (likely user ESC) — not pausing; shouldPreempt handles next cycle") - return + return Option.none() } const errMsg = `continuation dispatch failed: ${Cause.pretty(cause)}` yield* Effect.logWarning("goal continuation dispatch failed", { error: Cause.pretty(cause) }) yield* goal.pauseAndPublish(sessionID, errMsg).pipe(Effect.ignore) yield* promptSvc.prompt({ sessionID, noReply: true, parts: [{ type: "text", text: `⏸ 目标已暂停 — ${errMsg}` }] }).pipe(Effect.ignore) + return Option.none() }), ), - ) + ), + ) + const afterDispatch = yield* goal.load(sessionID) + if (!afterDispatch || afterDispatch.status !== "active") + yield* automation.unregister(sessionID, goalOwner) // NOTE: We deliberately DO NOT call goal.clearLoopFiber here. The // promptSvc.prompt above triggers a fresh agent loop, which when it @@ -400,6 +414,8 @@ export const layer = Layer.effect( }), ) +export const layer = serviceLayer.pipe(Layer.provide(SessionAutomationLease.defaultLayer)) + // GoalLoop.defaultLayer self-provides its construction deps. Because // Layer.provideMerge(self, layer) requires `layer` (GoalLoop) to be // self-contained — self's context is NOT fed into layer — every dep in the diff --git a/packages/opencode/src/goal/prompts.ts b/packages/opencode/src/goal/prompts.ts index 152e17fc7b..188d44b977 100644 --- a/packages/opencode/src/goal/prompts.ts +++ b/packages/opencode/src/goal/prompts.ts @@ -25,16 +25,17 @@ You will receive: 2. The agent's most recent response. Return ONLY a JSON object (no markdown, no explanation): -{"done": true/false, "reason": "one sentence explanation"} +{"verdict": "done" | "continue" | "blocked", "reason": "one sentence explanation"} -"done" = true means ONE of: +"verdict" = "done" means ONE of: - The agent explicitly confirmed the goal is complete with evidence. - The goal produced a clear, verifiable deliverable (file created, test passed, etc.). - - The goal is unachievable or blocked and the agent said so. -"done" = false means the agent is still making progress or has more steps. +"verdict" = "blocked" means the agent cannot make progress without user input or an external-state change. -Be conservative: if in doubt, return "done": false.` +"verdict" = "continue" means the agent is still making progress or has more steps. + +Be conservative: if in doubt, return "verdict": "continue".` export const JUDGE_USER_PROMPT_TEMPLATE = `Goal: {goal} diff --git a/packages/opencode/src/goal/state.ts b/packages/opencode/src/goal/state.ts index ce7fd42225..21326c2f07 100644 --- a/packages/opencode/src/goal/state.ts +++ b/packages/opencode/src/goal/state.ts @@ -7,10 +7,18 @@ export const Status = Schema.Literals(["active", "paused", "done"]) export type Status = Schema.Schema.Type // `skipped` was a dead enum value with no production write path — removed. -export const Verdict = Schema.Literals(["done", "continue"]) +export const Verdict = Schema.Literals(["done", "continue", "blocked"]) export type Verdict = Schema.Schema.Type export class Info extends Schema.Class("GoalState")({ + goal_id: Schema.String.pipe( + Schema.optional, + Schema.withDecodingDefault(Effect.succeed("legacy")), + ), + revision: NonNegativeInt.pipe( + Schema.optional, + Schema.withDecodingDefault(Effect.succeed(0 as Schema.Schema.Type)), + ), goal: Schema.String, status: Status, turns_used: NonNegativeInt, @@ -32,3 +40,22 @@ export class Info extends Schema.Class("GoalState")({ * site instead of `as any` scattered across goal.ts. */ export const nni = (value: number): Schema.Schema.Type => value + +export function advance(state: Info, patch: Partial>) { + return new Info({ + goal_id: state.goal_id, + revision: nni((state.revision ?? 0) + 1), + goal: state.goal, + status: state.status, + turns_used: state.turns_used, + max_turns: state.max_turns, + created_at: state.created_at, + last_turn_at: state.last_turn_at, + last_verdict: state.last_verdict, + last_reason: state.last_reason, + paused_reason: state.paused_reason, + consecutive_parse_failures: state.consecutive_parse_failures, + subgoals: state.subgoals, + ...patch, + }) +} diff --git a/packages/opencode/src/session/automation-lease.ts b/packages/opencode/src/session/automation-lease.ts new file mode 100644 index 0000000000..138fddecbf --- /dev/null +++ b/packages/opencode/src/session/automation-lease.ts @@ -0,0 +1,124 @@ +export * as SessionAutomationLease from "./automation-lease" + +import { Context, Effect, Layer, Option } from "effect" +import { LayerNode } from "@opencode-ai/core/effect/layer-node" +import { KeyedMutex } from "@opencode-ai/core/effect/keyed-mutex" +import { SessionID } from "./schema" + +export type Owner = + | { readonly kind: "goal"; readonly id: string } + | { readonly kind: "dag"; readonly id: string } + +export interface Token { + readonly sessionID: SessionID + readonly owner: Owner + readonly generation: number +} + +type Request = + | { readonly kind: "goal"; readonly id: string } + | { readonly kind: "dag" } + +export interface Interface { + readonly register: (sessionID: SessionID, owner: Owner) => Effect.Effect + readonly unregister: (sessionID: SessionID, owner: Owner) => Effect.Effect + readonly claim: (sessionID: SessionID, request: Request) => Effect.Effect> + readonly use: (token: Token, effect: Effect.Effect) => Effect.Effect, E, R> +} + +export class Service extends Context.Service()("@opencode/SessionAutomationLease") {} + +export const layer = Layer.sync(Service, () => { + const locks = KeyedMutex.makeUnsafe() + const registrations = new Map< + SessionID, + { readonly goals: Set; readonly dags: Set; generation: number } + >() + + const entry = (sessionID: SessionID) => { + const current = registrations.get(sessionID) + if (current) return current + const created = { goals: new Set(), dags: new Set(), generation: 0 } + registrations.set(sessionID, created) + return created + } + + const owner = (sessionID: SessionID): Owner | undefined => { + const current = registrations.get(sessionID) + const dag = current?.dags.values().next().value + if (dag) return { kind: "dag", id: dag } + const goal = current?.goals.values().next().value + if (goal) return { kind: "goal", id: goal } + return undefined + } + + const register = Effect.fn("SessionAutomationLease.register")(function* ( + sessionID: SessionID, + value: Owner, + ) { + yield* locks.withLock(sessionID)( + Effect.sync(() => { + const current = entry(sessionID) + const values = value.kind === "dag" ? current.dags : current.goals + if (values.has(value.id)) return + values.add(value.id) + current.generation += 1 + }), + ) + }) + + const unregister = Effect.fn("SessionAutomationLease.unregister")(function* ( + sessionID: SessionID, + value: Owner, + ) { + yield* locks.withLock(sessionID)( + Effect.sync(() => { + const current = registrations.get(sessionID) + if (!current) return + const values = value.kind === "dag" ? current.dags : current.goals + if (!values.delete(value.id)) return + current.generation += 1 + if (current.goals.size === 0 && current.dags.size === 0) registrations.delete(sessionID) + }), + ) + }) + + const claim = Effect.fn("SessionAutomationLease.claim")(function* ( + sessionID: SessionID, + request: Request, + ) { + return yield* locks.withLock(sessionID)( + Effect.sync(() => { + const current = registrations.get(sessionID) + const selected = owner(sessionID) + if (!current || !selected) return Option.none() + if (request.kind === "goal" && (selected.kind !== "goal" || selected.id !== request.id)) + return Option.none() + if (request.kind === "dag" && selected.kind !== "dag") return Option.none() + return Option.some({ sessionID, owner: selected, generation: current.generation }) + }), + ) + }) + + const use: Interface["use"] = Effect.fn("SessionAutomationLease.use")(function* (token, effect) { + const valid = yield* locks.withLock(token.sessionID)( + Effect.sync(() => { + const current = registrations.get(token.sessionID) + const selected = owner(token.sessionID) + return !( + !current || + current.generation !== token.generation || + selected?.kind !== token.owner.kind || + selected.id !== token.owner.id + ) + }), + ) + if (!valid) return Option.none() + return Option.some(yield* effect) + }) + + return Service.of({ register, unregister, claim, use }) +}) + +export const defaultLayer = layer +export const node = LayerNode.make(layer, []) diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index 679c2e012b..4b37f00334 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -1,5 +1,5 @@ import { describe, expect } from "bun:test" -import { Cause, Effect, Layer } from "effect" +import { Cause, Effect, Layer, Option } from "effect" import { GoalLoop, GoalLoopJudgeLLM } from "@/goal/loop" import { Goal } from "@/goal/goal" import { GoalEvent } from "@/goal/events" @@ -9,6 +9,7 @@ import { Session } from "@/session/session" import { SessionPrompt } from "@/session/prompt" import { Provider } from "@/provider/provider" import { SessionID } from "@/session/schema" +import { SessionAutomationLease } from "@/session/automation-lease" import { testEffect, pollWithTimeout } from "../lib/effect" // P2b: full-cycle Goal regression (D5). Drives set → idle → judge(continue) → @@ -66,16 +67,21 @@ const mkAssistantTools = () => // assertions. Resolves void — these tests never drive a real agent turn from // the mock; the goal state and event captures are the observable contract. const recordingPrompt = (sink: { noReply?: boolean; text: string }[]) => - Layer.succeed(SessionPrompt.Service, { - prompt: (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => + Layer.succeed(SessionPrompt.Service, (() => { + const record = (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => Effect.sync(() => { sink.push({ noReply: input.noReply, text: input.parts?.map((p) => p.text).join("\n") ?? "", }) return undefined as never - }), - } as never) + }) + return { + prompt: record, + promptIfIdle: (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => + record(input).pipe(Effect.map(Option.some)), + } as never + })()) describe("GoalLoop end-to-end — continue → done lifecycle (P2b)", () => { // Per-test mutable mock state (each it.instance runs in its own scope, but @@ -92,16 +98,21 @@ describe("GoalLoop end-to-end — continue → done lifecycle (P2b)", () => { const sessionMock = Layer.succeed(Session.Service, { messages: () => Effect.succeed([mkAssistant()]), } as never) - const promptMock = Layer.succeed(SessionPrompt.Service, { - prompt: (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => + const promptMock = Layer.succeed(SessionPrompt.Service, (() => { + const record = (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => Effect.sync(() => { promptCalls.push({ noReply: input.noReply, text: input.parts?.map((p) => p.text).join("\n") ?? "", }) return undefined as never - }), - } as never) + }) + return { + prompt: record, + promptIfIdle: (input: { noReply?: boolean; parts?: Array<{ type: string; text: string }> }) => + record(input).pipe(Effect.map(Option.some)), + } as never + })()) const providerMock = Layer.succeed(Provider.Service, {} as never) const judgeMock = Layer.succeed( GoalLoopJudgeLLM, @@ -177,6 +188,142 @@ describe("GoalLoop end-to-end — continue → done lifecycle (P2b)", () => { ) }) +describe("GoalLoop — shared Session automation lease", () => { + let leaseAttempts = 0 + let directPromptAttempts = 0 + const sessionMock = Layer.succeed(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + } as never) + const promptMock = Layer.succeed(SessionPrompt.Service, { + prompt: () => + Effect.sync(() => { + directPromptAttempts += 1 + return undefined as never + }), + promptIfIdle: () => + Effect.sync(() => { + leaseAttempts += 1 + return Option.none() + }), + } as never) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => Effect.succeed(JSON.stringify({ verdict: "continue", reason: "more work" })), + }), + ) + const leaseLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.succeed(Provider.Service, {} as never)), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + Layer.provide(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + ) + const it = testEffect(leaseLayer) + + it.instance("a busy Session lease rejects Goal continuation without direct prompt admission", () => + Effect.gen(function* () { + leaseAttempts = 0 + directPromptAttempts = 0 + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const events = yield* EventV2Bridge.Service + yield* loop.init() + const sessionID = SessionID.descending() + yield* goal.set(sessionID, "ship the feature", 10) + yield* Effect.yieldNow + + yield* events.publish(SessionStatus.Event.Status, { + sessionID, + status: { type: "idle" }, + }) + yield* pollWithTimeout( + Effect.sync(() => (leaseAttempts > 0 ? true : undefined)), + "GoalLoop never attempted the shared Session automation lease", + "5 seconds", + ) + + expect(directPromptAttempts).toBe(0) + expect((yield* goal.load(sessionID))?.status).toBe("active") + }), + ) +}) + +describe("GoalLoop + DAG owner arbitration", () => { + let judgeCalls = 0 + let continuationCalls = 0 + const sessionMock = Layer.succeed(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + } as never) + const promptMock = Layer.succeed(SessionPrompt.Service, { + prompt: () => Effect.succeed(undefined as never), + promptIfIdle: () => + Effect.sync(() => { + continuationCalls += 1 + return Option.some(undefined as never) + }), + } as never) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => + Effect.sync(() => { + judgeCalls += 1 + return JSON.stringify({ verdict: "continue", reason: "more work" }) + }), + }), + ) + const arbitrationLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.succeed(Provider.Service, {} as never)), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + Layer.provide(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + ) + const it = testEffect(arbitrationLayer) + + it.instance("a live DAG owns the Session; Goal resumes after the DAG releases it", () => + Effect.gen(function* () { + judgeCalls = 0 + continuationCalls = 0 + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const events = yield* EventV2Bridge.Service + const automation = yield* SessionAutomationLease.Service + yield* loop.init() + const sessionID = SessionID.descending() + yield* goal.set(sessionID, "ship the feature", 10) + yield* automation.register(sessionID, { kind: "dag", id: "dag-executor" }) + yield* Effect.yieldNow + + yield* events.publish(SessionStatus.Event.Status, { + sessionID, + status: { type: "idle" }, + }) + yield* Effect.sleep("50 millis") + expect(judgeCalls).toBe(0) + expect(continuationCalls).toBe(0) + + yield* automation.unregister(sessionID, { kind: "dag", id: "dag-executor" }) + yield* events.publish(SessionStatus.Event.Status, { + sessionID, + status: { type: "idle" }, + }) + yield* pollWithTimeout( + Effect.sync(() => (continuationCalls === 1 ? true : undefined)), + "Goal did not resume after the DAG released the Session lease", + "5 seconds", + ) + expect(judgeCalls).toBe(1) + }), + ) +}) + // D1 (hooks-goal-completeness): a continuation dispatch failure must surface as a // recoverable paused state, not a silent stall. Reuses the e2e harness with a // prompt mock that always fails — the only prompt in this flow is the @@ -194,6 +341,7 @@ describe("GoalLoop — continuation dispatch failure → recoverable pause (D1)" // Always-failing prompt — simulates provider fault / session write error. const promptFailMock = Layer.succeed(SessionPrompt.Service, { prompt: () => Effect.fail(new Error("continuation provider down")), + promptIfIdle: () => Effect.fail(new Error("continuation provider down")), } as never) const providerMock = Layer.succeed(Provider.Service, {} as never) const judgeMock = Layer.succeed( @@ -520,6 +668,7 @@ describe("GoalLoop — continuation interrupted → no pause, goal stays active let interruptCause: Cause.Cause = Cause.interrupt(0) const promptInterruptMock = Layer.succeed(SessionPrompt.Service, { prompt: () => Effect.failCause(interruptCause), + promptIfIdle: () => Effect.failCause(interruptCause), } as never) const providerMock = Layer.succeed(Provider.Service, {} as never) const judgeMock = Layer.succeed( diff --git a/packages/opencode/test/goal/goal.test.ts b/packages/opencode/test/goal/goal.test.ts index 1b8aa9aeef..739f08e13b 100644 --- a/packages/opencode/test/goal/goal.test.ts +++ b/packages/opencode/test/goal/goal.test.ts @@ -117,7 +117,7 @@ describe("Goal.updateAfterJudge — continue branch", () => { ) }) -describe("Goal.updateAfterJudge — done branch (turn budget)", () => { +describe("Goal.updateAfterJudge — atomic done transition", () => { // §2.2 — done is a STATE TRANSITION, not a continuation dispatch, so it must // NOT consume budget. Pre-fix this fails (code does +1); post-§3 it passes. it.live("done verdict does not increment turns_used (state transitions are budget-neutral)", () => @@ -128,20 +128,20 @@ describe("Goal.updateAfterJudge — done branch (turn budget)", () => { const before = yield* goal.load(sessionID) const n = Number(before?.turns_used) - yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) + const result = yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) - const after = yield* goal.load(sessionID) - expect(after?.status).toBe("done") - expect(Number(after?.turns_used)).toBe(n) + expect(result?.state.status).toBe("done") + expect(Number(result?.state.turns_used)).toBe(n) + expect(yield* goal.load(sessionID)).toBeUndefined() + const outcome = yield* goal.lastOutcome(sessionID) + expect(outcome?.status).toBe("done") + expect(outcome?.last_reason).toBe("delivered") }), ) }) describe("Goal.updateAfterJudge — done branch (terminal event contract)", () => { - // §2.3 — updateAfterJudge's done branch must NOT publish goal.updated; only - // deleteAndPublishDone owns the terminal sequence. Pre-fix this fails (code - // publishes); post-§4 it passes. - it.live("done verdict does not publish goal.updated (single-owner: deleteAndPublishDone)", () => + it.live("done verdict atomically removes the row and publishes the terminal sequence", () => Effect.gen(function* () { const goal = yield* Goal.Service const events = yield* EventV2Bridge.Service @@ -153,36 +153,122 @@ describe("Goal.updateAfterJudge — done branch (terminal event contract)", () = yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) - const updates = seen.filter((e) => e.type === GoalEvent.Updated.type) - expect(updates.length).toBe(0) + const types = seen.map((e) => e.type) + expect(types).toEqual([GoalEvent.Updated.type, GoalEvent.Cleared.type]) + doneUpdated(seen) + const cleared = seen.filter((e) => e.type === GoalEvent.Cleared.type) + expect(cleared.length).toBe(1) + + // row is gone after the terminal sequence + const loaded = yield* goal.load(sessionID) + expect(loaded).toBeUndefined() }), ) +}) - // §4.3 — full judge-done flow: updateAfterJudge persists the done row WITHOUT - // publishing, then deleteAndPublishDone publishes the terminal sequence - // exactly once: goal.updated(done) → goal.cleared, no duplicate updated. - it.live("full judge-done flow publishes goal.updated(done) -> goal.cleared exactly once", () => +describe("Goal.updateAfterJudge — blocked branch", () => { + it.live("blocked pauses the goal and never emits a successful done state", () => Effect.gen(function* () { const goal = yield* Goal.Service const events = yield* EventV2Bridge.Service const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "ship feature X", 10) + yield* goal.set(sessionID, "deploy production", 10) seen.length = 0 - yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) - yield* goal.deleteAndPublishDone(sessionID, "delivered") + const result = yield* goal.updateAfterJudge( + sessionID, + "blocked", + "missing production credentials", + false, + ) - const types = seen.map((e) => e.type) - expect(types).toEqual([GoalEvent.Updated.type, GoalEvent.Cleared.type]) - doneUpdated(seen) - const cleared = seen.filter((e) => e.type === GoalEvent.Cleared.type) - expect(cleared.length).toBe(1) + expect(result?.state.status).toBe("paused") + expect(result?.state.last_verdict).toBe("blocked") + expect(result?.message).toContain("已阻塞") + expect(seen.some((event) => event.status === "done")).toBe(false) + }), + ) +}) - // row is gone after the terminal sequence - const loaded = yield* goal.load(sessionID) - expect(loaded).toBeUndefined() +describe("Goal transition authority — stale loop decisions", () => { + it.live("concurrent pause and judge update always settle paused", () => + Effect.gen(function* () { + const goal = yield* Goal.Service + const sessionID = SessionID.descending() + yield* goal.set(sessionID, "ship feature X", 10) + + yield* Effect.all( + [ + goal.pause(sessionID, "user-paused"), + goal.updateAfterJudge(sessionID, "continue", "racing judge result", false), + ], + { concurrency: 2 }, + ) + + expect((yield* goal.load(sessionID))?.status).toBe("paused") + }), + ) + + it.live("concurrent clear and judge update never leave a resurrected row", () => + Effect.gen(function* () { + const goal = yield* Goal.Service + const sessionID = SessionID.descending() + yield* goal.set(sessionID, "ship feature X", 10) + + yield* Effect.all( + [ + goal.clear(sessionID), + goal.updateAfterJudge(sessionID, "continue", "racing judge result", false), + ], + { concurrency: 2 }, + ) + + expect(yield* goal.load(sessionID)).toBeUndefined() + }), + ) + + it.live("a judge result read before pause cannot reactivate the paused goal", () => + Effect.gen(function* () { + const goal = yield* Goal.Service + const sessionID = SessionID.descending() + const before = yield* goal.set(sessionID, "ship feature X", 10) + + yield* goal.pause(sessionID, "user-paused") + const stale = yield* goal.updateAfterJudge( + sessionID, + "continue", + "stale judge result", + false, + { goalID: before.goal_id ?? "legacy", revision: before.revision ?? 0 }, + ) + + expect(stale).toBeUndefined() + expect((yield* goal.load(sessionID))?.status).toBe("paused") + }), + ) + + it.live("a judge result from a cleared goal cannot mutate its replacement", () => + Effect.gen(function* () { + const goal = yield* Goal.Service + const sessionID = SessionID.descending() + const before = yield* goal.set(sessionID, "old goal", 10) + + yield* goal.clear(sessionID) + const replacement = yield* goal.set(sessionID, "new goal", 10) + const stale = yield* goal.updateAfterJudge( + sessionID, + "continue", + "old result", + false, + { goalID: before.goal_id ?? "legacy", revision: before.revision ?? 0 }, + ) + + expect(stale).toBeUndefined() + const current = yield* goal.load(sessionID) + expect(current?.goal_id).toBe(replacement.goal_id) + expect(current?.turns_used).toBe(0) }), ) }) @@ -650,9 +736,6 @@ describe("Goal.deleteAndPublishDone — terminal sequence is uninterruptible (F1 const sessionID = SessionID.descending() yield* goal.set(sessionID, "ship feature X", 10) - // Persist a done row WITHOUT publishing — mirrors what loop.ts does - // (updateAfterJudge) before invoking deleteAndPublishDone. - yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) seen.length = 0 const fiber = yield* goal.deleteAndPublishDone(sessionID, "delivered").pipe(Effect.forkScoped) diff --git a/packages/opencode/test/goal/judge.test.ts b/packages/opencode/test/goal/judge.test.ts index 99b3b814bd..9a87432eb1 100644 --- a/packages/opencode/test/goal/judge.test.ts +++ b/packages/opencode/test/goal/judge.test.ts @@ -14,6 +14,17 @@ describe("parseJudgeResponse", () => { expect(result).toEqual({ verdict: "continue", reason: "still working", parseFailed: false }) }) + test("blocked verdict stays distinct from successful completion", () => { + const result = GoalJudge.parseJudgeResponse( + '{"verdict":"blocked","reason":"missing production credentials"}', + ) + expect(result).toEqual({ + verdict: "blocked", + reason: "missing production credentials", + parseFailed: false, + }) + }) + // §1.3 — markdown-fenced JSON strips fences (step 1) test("markdown-fenced JSON strips fences and parses", () => { const raw = "```json\n{\"done\": false, \"reason\": \"more steps remain\"}\n```" diff --git a/packages/opencode/test/session/automation-lease.test.ts b/packages/opencode/test/session/automation-lease.test.ts new file mode 100644 index 0000000000..59a080268c --- /dev/null +++ b/packages/opencode/test/session/automation-lease.test.ts @@ -0,0 +1,45 @@ +import { describe, expect } from "bun:test" +import { Effect, Option } from "effect" +import { SessionAutomationLease } from "@/session/automation-lease" +import { SessionID } from "@/session/schema" +import { testEffect } from "../lib/effect" + +const it = testEffect(SessionAutomationLease.defaultLayer) + +describe("SessionAutomationLease", () => { + it.instance("DAG registration preempts Goal and invalidates its generation", () => + Effect.gen(function* () { + const lease = yield* SessionAutomationLease.Service + const sessionID = SessionID.descending() + const goal = { kind: "goal" as const, id: "goal-1" } + const dag = { kind: "dag" as const, id: "dag-1" } + + yield* lease.register(sessionID, goal) + const goalToken = Option.getOrThrow(yield* lease.claim(sessionID, goal)) + yield* lease.register(sessionID, dag) + + expect(Option.isNone(yield* lease.use(goalToken, Effect.succeed("goal")))).toBe(true) + const dagToken = Option.getOrThrow(yield* lease.claim(sessionID, { kind: "dag" })) + expect(Option.getOrThrow(yield* lease.use(dagToken, Effect.succeed("dag")))).toBe("dag") + }), + ) + + it.instance("Goal becomes owner again after the final DAG unregisters", () => + Effect.gen(function* () { + const lease = yield* SessionAutomationLease.Service + const sessionID = SessionID.descending() + const goal = { kind: "goal" as const, id: "goal-1" } + const first = { kind: "dag" as const, id: "dag-1" } + const second = { kind: "dag" as const, id: "dag-2" } + + yield* lease.register(sessionID, goal) + yield* lease.register(sessionID, first) + yield* lease.register(sessionID, second) + yield* lease.unregister(sessionID, first) + expect(Option.isSome(yield* lease.claim(sessionID, { kind: "dag" }))).toBe(true) + + yield* lease.unregister(sessionID, second) + expect(Option.isSome(yield* lease.claim(sessionID, goal))).toBe(true) + }), + ) +}) From a05a8fa58e3416046c13a9f8f6f41a2754cb59b2 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 02:47:44 +0800 Subject: [PATCH 02/11] fix(goal): bind dag lease registration lifetime to workflow terminal state (GOAL-FP-01-01/-03) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The DAG automation-lease registration lifecycle was bound to WAKE DELIVERY instead of workflow state, leaking dag registrations that permanently block the session's goal (owner() prefers dag, so the goal can never claim). - Startup wake sweep registered every workflow in the wake snapshot, including terminal workflows with wake_reported=true, which are never in the wake batch and therefore never unregistered (-01). - Terminal event handlers (WorkflowCompleted/Failed/Cancelled) never unregistered, so a workflow terminalizing without a successful wake delivery kept its registration indefinitely (-03). Fix: - Sweep: register only non-terminal workflows. Verified safe for terminal-but-unreported workflows: tryDeliverWake registers every workflow in its batch itself right before claiming the wake lease, so redelivery does not depend on the sweep. - Terminal handlers: unregister the dag registration on workflow terminalization. Identity verified: the projector writes WorkflowTable.id = event dagID, so the unregister key { kind: "dag", id: evt.data.dagID } matches every registration key (adoption, recovery, sweep, delivery). TDD evidence (test/dag/dag-lease-lifecycle.test.ts, real DagLoop init over in-memory DB + real SessionAutomationLease + real Goal/store): - Red (current code): -01 "goal claimable after restart" failed with claim(goal) = none (dag leaked by the sweep); -03 "dag lease released on terminal event without wake delivery" timed out (registration persisted). - Green after fix: 2/2 pass. - Mutation 1 (revert sweep filter): -01 goes Red. Restored. - Mutation 2 (remove handler unregister): -03 goes Red. Restored. Verification: bun test test/goal test/session/automation-lease.test.ts test/dag → 564 pass / 0 fail; bun typecheck clean; bun lint → 4852 warnings (ratchet unchanged, 0 new). Co-Authored-By: Claude --- packages/opencode/src/dag/runtime/loop.ts | 20 +- .../test/dag/dag-lease-lifecycle.test.ts | 366 ++++++++++++++++++ 2 files changed, 385 insertions(+), 1 deletion(-) create mode 100644 packages/opencode/test/dag/dag-lease-lifecycle.test.ts diff --git a/packages/opencode/src/dag/runtime/loop.ts b/packages/opencode/src/dag/runtime/loop.ts index 20def0c963..d5c3b46c4e 100644 --- a/packages/opencode/src/dag/runtime/loop.ts +++ b/packages/opencode/src/dag/runtime/loop.ts @@ -949,6 +949,15 @@ const serviceLayer = Layer.effect( // P1-6: trigger wake on workflow terminal so the parent // learns the final outcome even if no idle event fires. if (parentSessionID) { + // GOAL-FP-01-03: the dag registration lifetime is bound to + // workflow state, not to wake delivery. A workflow that + // terminalizes without a successful wake delivery must not + // keep its registration (and the Session's ownership) + // indefinitely. The key matches the registration key + // (WorkflowTable.id === event dagID, per the projector); + // tryDeliverWake re-registers its batch itself before + // claiming the wake lease when a delivery is attempted. + yield* automation.unregister(SessionID.make(parentSessionID), { kind: "dag", id: dagID }) yield* tryDeliverWake(parentSessionID).pipe(Effect.ignore, Effect.forkScoped) } }).pipe(guarded("WorkflowTerminal")), @@ -1295,8 +1304,17 @@ const serviceLayer = Layer.effect( ), ) if (!snapshot.workflows.some((wf) => wf.projectId === ctx.project.id)) continue + // GOAL-FP-01-01: register only NON-terminal workflows. Terminal + // workflows with wake_reported=true would otherwise be re-registered + // on every restart and never unregistered (the only unregister for + // them lives in the wake-delivery SUCCESS path, whose batch only + // carries unreported workflows) — permanently leaking a dag + // registration and blocking the goal. Terminal-but-unreported + // workflows are safe to skip here too: tryDeliverWake registers + // every workflow in its batch itself right before claiming the wake + // lease, so wake redelivery does not depend on this sweep. yield* Effect.forEach( - snapshot.workflows, + snapshot.workflows.filter((workflow) => !isWorkflowTerminalStatus(workflow.status as never)), (workflow) => automation.register(SessionID.make(sessionID), { kind: "dag", id: workflow.id }), { discard: true }, diff --git a/packages/opencode/test/dag/dag-lease-lifecycle.test.ts b/packages/opencode/test/dag/dag-lease-lifecycle.test.ts new file mode 100644 index 0000000000..f572290ab7 --- /dev/null +++ b/packages/opencode/test/dag/dag-lease-lifecycle.test.ts @@ -0,0 +1,366 @@ +import { describe, expect, it } from "bun:test" +import { Deferred, Effect, Layer, Option, Queue } from "effect" +import type { SessionV1 } from "@opencode-ai/core/v1/session" +import { Database } from "@opencode-ai/core/database/database" +import { DagProjector } from "@opencode-ai/core/dag/projector" +import { WorkflowTable } from "@opencode-ai/core/dag/sql" +import { DagStore } from "@opencode-ai/core/dag/store" +import { EventV2 } from "@opencode-ai/core/event" +import { Project } from "@opencode-ai/core/project" +import { ProjectTable } from "@opencode-ai/core/project/sql" +import { AbsolutePath } from "@opencode-ai/core/schema" +import { SessionTable } from "@opencode-ai/core/session/sql" +import { Model } from "@opencode-ai/schema/model" +import { Provider } from "@opencode-ai/schema/provider" +import { Agent } from "@/agent/agent" +import { Dag, type NodeConfig } from "@/dag/dag" +import { DagLoop } from "@/dag/runtime/loop" +import { InstanceRef } from "@/effect/instance-ref" +import { EventV2Bridge } from "@/event-v2-bridge" +import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "@/session/automation-lease" +import { MessageID, PartID, SessionID } from "@/session/schema" +import { SessionPrompt } from "@/session/prompt" +import { Session } from "@/session/session" +import { SessionStatus } from "@/session/status" +import { pollWithTimeout } from "../lib/effect" + +// GOAL-FP-01-01 / GOAL-FP-01-03: the DAG automation-lease registration lifetime +// must be bound to WORKFLOW STATE, not to wake delivery. +// +// -01: after a restart, a session whose snapshot contains only terminal +// workflows (one already wake-reported) must not get a dag registration +// from the startup wake sweep — the active goal must remain claimable. +// -03: a workflow that terminalizes without a successful wake delivery must +// release its dag registration from the terminal event handler. +// +// Real DagLoop startup sweep + real SessionAutomationLease + real DagStore / +// Projector / EventV2 over an in-memory database; Session / SessionPrompt / +// Agent are mocked exactly like the wake-integration harness. + +interface ChildPromptGate { + readonly title: string + readonly release: Deferred.Deferred +} + +const PARENT_SESSION = "ses_parent" +const PROJECT_ID = "project-1" + +function node(id: string, dependsOn: string[] = []): NodeConfig { + return { + id, + name: id, + worker_type: "build", + depends_on: dependsOn, + required: true, + prompt_template: { inline: id }, + report_to_parent: true, + } +} + +function reply(sessionID: string, text: string): SessionV1.WithParts { + const id = MessageID.ascending() + return { + info: { + id, + role: "assistant", + parentID: MessageID.ascending(), + sessionID: SessionID.make(sessionID), + mode: "build", + agent: "build", + cost: 0, + path: { cwd: process.cwd(), root: process.cwd() }, + tokens: { input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, + modelID: Model.ID.make("test-model"), + providerID: Provider.ID.make("test"), + time: { created: Date.now() }, + finish: "stop", + }, + parts: text ? [{ id: PartID.ascending(), sessionID: SessionID.make(sessionID), messageID: id, type: "text", text }] : [], + } +} + +function takeWithin(queue: Queue.Queue, message: string) { + return Queue.take(queue).pipe( + Effect.timeoutOption("1 second"), + Effect.flatMap( + Option.match({ + onNone: () => Effect.fail(new Error(message)), + onSome: Effect.succeed, + }), + ), + ) +} + +function leaseLifecycleLayer(input: { childPrompts: Queue.Queue }) { + const database = Database.layerFromPath(":memory:") + const events = EventV2.layer.pipe(Layer.provide(database)) + const bridge = EventV2Bridge.layer.pipe(Layer.provide(events)) + const store = DagStore.layer.pipe(Layer.provide(database)) + const status = SessionStatus.layer.pipe(Layer.provide(bridge)) + const projector = DagProjector.layer.pipe( + Layer.provide(events), + Layer.provide(database), + ) + const dag = Dag.layer.pipe( + Layer.provide(bridge), + Layer.provide(store), + ) + const goal = Goal.layer.pipe( + Layer.provide(bridge), + Layer.provide(database), + Layer.provide(status), + ) + const base = Layer.mergeAll(database, events, bridge, store, projector, dag, goal, status) + const childTitles = new Map() + const created: string[] = [] + const session = Layer.mock(Session.Service, { + get: () => + Effect.succeed({ + id: SessionID.make(PARENT_SESSION), + slug: "parent", + projectID: Project.ID.make(PROJECT_ID), + directory: process.cwd(), + title: "Parent", + version: "test", + time: { created: 0, updated: 0 }, + permission: [], + agent: "build", + }), + create: (value) => + Effect.sync(() => { + const id = `ses_child_${created.length + 1}` + created.push(id) + childTitles.set(id, (value?.title ?? id).replace(" (DAG node)", "")) + return { + id: SessionID.make(id), + slug: "child", + projectID: Project.ID.make(PROJECT_ID), + directory: process.cwd(), + title: value?.title ?? id, + version: "test", + time: { created: 0, updated: 0 }, + } + }), + messages: () => Effect.succeed([]), + }) + const deliver = Effect.fn("test.dagLease.SessionPrompt.deliver")(function* (value: SessionPrompt.PromptInput) { + const sessionID = value.sessionID as string + if (sessionID === PARENT_SESSION) { + // Both scenarios require the parent wake delivery to FAIL (or never be + // attempted): the release path under test is workflow state, not + // delivery. Die loudly — if a parent wake is actually delivered here, + // the test premise is broken and the failure must not be silent. + return yield* Effect.die(new Error("parent wake delivery must not succeed in lease-lifecycle scenarios")) + } + const release = yield* Deferred.make() + yield* Queue.offer(input.childPrompts, { title: childTitles.get(sessionID) ?? sessionID, release }) + return reply(sessionID, yield* Deferred.await(release)) + }) + const prompt = Layer.mock(SessionPrompt.Service, { + cancel: () => Effect.void, + prompt: deliver, + promptIfIdle: (value) => deliver(value).pipe(Effect.map(Option.some)), + }) + const agent = Layer.mock(Agent.Service, { + get: () => + Effect.succeed({ + name: "build", + mode: "all", + permission: [], + options: {}, + description: "", + prompt: "", + model: { providerID: Provider.ID.make("test"), modelID: Model.ID.make("test-model") }, + tools: {}, + hooks: {}, + }), + }) + const loop = DagLoop.layer.pipe( + Layer.provide(base), + Layer.provide(session), + Layer.provide(prompt), + Layer.provide(agent), + ) + // DagLoop.layer consumes the lease internally (its Layer.provide does not + // re-expose it). Merge the SAME module-level layer at the top so the test + // body can observe the lease; Layer.build memoization dedups the shared + // layer reference, so it is the very instance DagLoop and Goal use. + return Layer.mergeAll(base, loop, SessionAutomationLease.defaultLayer) +} + +function runLeaseTest( + test: (services: { + readonly dag: Dag.Interface + readonly loop: DagLoop.Interface + readonly store: DagStore.Interface + readonly goal: Goal.Interface + readonly status: SessionStatus.Interface + readonly automation: SessionAutomationLease.Interface + readonly database: Database.Interface + readonly childPrompts: Queue.Queue + }) => Effect.Effect, +) { + return Effect.gen(function* () { + const childPrompts = yield* Queue.unbounded() + return yield* Effect.gen(function* () { + const dag = yield* Dag.Service + const loop = yield* DagLoop.Service + const store = yield* DagStore.Service + const goal = yield* Goal.Service + const status = yield* SessionStatus.Service + const automation = yield* SessionAutomationLease.Service + const database = yield* Database.Service + yield* database.db + .insert(ProjectTable) + .values({ + id: Project.ID.make(PROJECT_ID), + worktree: AbsolutePath.make(process.cwd()), + sandboxes: [], + }) + .run() + .pipe(Effect.orDie) + yield* database.db + .insert(SessionTable) + .values({ + id: SessionID.make(PARENT_SESSION), + project_id: Project.ID.make(PROJECT_ID), + slug: "parent", + directory: AbsolutePath.make(process.cwd()), + title: "Parent", + version: "test", + }) + .run() + .pipe(Effect.orDie) + return yield* test({ dag, loop, store, goal, status, automation, database, childPrompts }) + }).pipe( + Effect.provide(leaseLifecycleLayer({ childPrompts })), + Effect.provideService(InstanceRef, { + directory: process.cwd(), + worktree: process.cwd(), + project: { + id: Project.ID.make(PROJECT_ID), + worktree: process.cwd(), + time: { created: 0, updated: 0 }, + sandboxes: [], + }, + }), + Effect.scoped, + ) + }) +} + +describe("DagLoop lease lifecycle — startup wake sweep (GOAL-FP-01-01)", () => { + it("a restarted session whose snapshot holds only terminal workflows leaves the goal claimable", async () => { + await Effect.runPromise( + runLeaseTest(({ loop, goal, status, automation, database }) => + Effect.gen(function* () { + const sid = SessionID.make(PARENT_SESSION) + + // Historical crash snapshot: two terminal workflows under the same + // session. dag-wf-done was already wake-reported before the crash; + // dag-wf-undone terminalized without a delivered wake (it is what + // makes the session visible to the startup wake sweep). + yield* database.db + .insert(WorkflowTable) + .values({ + id: "dag-wf-done", + project_id: Project.ID.make(PROJECT_ID), + session_id: SessionID.make(PARENT_SESSION), + title: "already reported", + status: "completed", + config: "", + seq: 1, + wake_reported: true, + }) + .run() + .pipe(Effect.orDie) + yield* database.db + .insert(WorkflowTable) + .values({ + id: "dag-wf-undone", + project_id: Project.ID.make(PROJECT_ID), + session_id: SessionID.make(PARENT_SESSION), + title: "terminal before delivery", + status: "failed", + config: "", + seq: 2, + wake_reported: false, + }) + .run() + .pipe(Effect.orDie) + + // An active goal survived the restart (Goal.set registers the goal + // owner with the shared Session automation lease, like GoalLoop). + const goalState = yield* goal.set(sid, "ship the feature", 10) + const goalOwner = { kind: "goal" as const, id: goalState.goal_id ?? "legacy" } + + // The session is NOT idle when DagLoop boots, so the forked wake + // redelivery aborts before it can register or deliver anything: + // the sweep's own registration decision is the only dag-lease input. + yield* status.set(sid, { type: "busy" }) + + // Restart: DagLoop.init runs the startup wake sweep synchronously. + yield* loop.init() + + // Public contract: the goal must be claimable (owner() is goal, not + // a leaked dag registration from a terminal workflow). + expect(Option.isSome(yield* automation.claim(sid, goalOwner))).toBe(true) + expect(Option.isNone(yield* automation.claim(sid, { kind: "dag" }))).toBe(true) + }), + ), + ) + }) +}) + +describe("DagLoop lease lifecycle — terminal event release (GOAL-FP-01-03)", () => { + it("a workflow that terminalizes without a successful wake delivery releases its dag lease", async () => { + await Effect.runPromise( + runLeaseTest(({ dag, loop, store, status, automation, childPrompts }) => + Effect.gen(function* () { + const sid = SessionID.make(PARENT_SESSION) + + // The parent never goes idle: the wake redelivery aborts before + // registering/delivering, so the terminal event handler is the only + // possible release path for the dag registration. + yield* status.set(sid, { type: "busy" }) + yield* loop.init() + + const dagID = yield* dag.create({ + projectID: PROJECT_ID, + sessionID: PARENT_SESSION, + title: "lease release", + config: { name: "lease-release", nodes: [node("implement")] }, + }) + + // Adoption (WorkflowStarted) registered the dag lease for the parent. + const child = yield* takeWithin(childPrompts, "implement did not start") + expect(Option.isSome(yield* automation.claim(sid, { kind: "dag" }))).toBe(true) + + // Complete the node → workflow terminalizes → terminal event handler. + yield* Deferred.succeed(child.release, "done") + yield* pollWithTimeout( + store.getWorkflow(dagID).pipe( + Effect.map((workflow) => (workflow?.status === "completed" ? workflow : undefined)), + ), + "workflow did not complete", + ) + + // Public contract: the terminal event handler must release the dag + // lease even though no wake delivery ever succeeded. + yield* pollWithTimeout( + automation.claim(sid, { kind: "dag" }).pipe( + Effect.map((token) => (Option.isNone(token) ? true : undefined)), + ), + "dag lease was not released after workflow terminalization without wake delivery", + ) + + // And the goal can now be admitted. + const goalOwner = { kind: "goal" as const, id: "goal-1" } + yield* automation.register(sid, goalOwner) + expect(Option.isSome(yield* automation.claim(sid, goalOwner))).toBe(true) + }), + ), + ) + }) +}) From 1cdc2fdf94f8ae3cdf73c002ebafe7f7f66cfad7 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 03:19:50 +0800 Subject: [PATCH 03/11] fix(goal): release dag lease on terminalization without a runtime entry (GOAL-FP-01-03 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2-A residual on the -01/-03 seam: the terminal-handler unregister was gated by Stream.filter(runtimes.has(dagID)), so a workflow registered by the startup wake sweep but never adopted into a runtime entry (recoverWorkflow aborted at startup, e.g. an unreadable persisted row) could only ever be unregistered by a successful wake delivery — a control-op terminalization left a permanent dag registration and the goal permanently blocked. Fix (same seam, loop.ts only): - Terminal handlers no longer filter on runtimes.has. The handler remains a no-op for events not concerning this instance: the evalLock cleanup and the wake fork stay gated on the runtime entry, and the new no-entry release is scoped by the durable row's project (the same cross-instance guard every adoption path uses). - When the terminal event has no runtime entry, the handler releases the registration from the durable row: store.getWorkflow(dagID) → WorkflowRow.sessionId (verified: DagStore.Interface.getWorkflow returns WorkflowRow with sessionId — no store changes needed), then automation.unregister(SessionID.make(wf.sessionId), { kind: "dag", id: dagID }) with the project guard. TDD evidence (test/dag/dag-lease-lifecycle.test.ts, same real-DagLoop harness): - Red: new test "releases a swept registration when a workflow with no runtime entry is terminalized by a control op" timed out — the dag lease survived WorkflowCancelled (the recovery failure is simulated as a session-store defect that aborts reconcileWorkflow, leaving a non-terminal row with no runtime entry; sweep registers it; dag.cancel terminalizes it). - Green after fix: 3/3 in the file. - Mutation (remove the no-entry unregister branch): the new test goes Red (timeout). Restored. Verification: bun test test/dag test/session/automation-lease.test.ts test/goal → 565 pass / 0 fail; bun typecheck clean; bun lint → 4852 warnings (ratchet unchanged, 0 new). Co-Authored-By: Claude --- packages/opencode/src/dag/runtime/loop.ts | 22 ++- .../test/dag/dag-lease-lifecycle.test.ts | 130 ++++++++++++++++-- 2 files changed, 138 insertions(+), 14 deletions(-) diff --git a/packages/opencode/src/dag/runtime/loop.ts b/packages/opencode/src/dag/runtime/loop.ts index d5c3b46c4e..6a62852fcc 100644 --- a/packages/opencode/src/dag/runtime/loop.ts +++ b/packages/opencode/src/dag/runtime/loop.ts @@ -923,8 +923,16 @@ const serviceLayer = Layer.effect( ) for (const def of [DagEvent.WorkflowCompleted, DagEvent.WorkflowFailed, DagEvent.WorkflowCancelled]) { + // Deliberately NO runtimes.has filter: a workflow terminalized by a + // control op after a failed startup recovery (e.g. recoverWorkflow + // aborted on an unreadable persisted row) has no runtime entry but + // may still hold a dag registration from the startup wake sweep. + // The handler stays a no-op for events not concerning this + // instance: the evalLock cleanup and wake fork remain gated on + // `entry`, and the no-entry release below is scoped by the durable + // row's project — the same cross-instance guard every adoption + // path uses. yield* events.subscribe(def).pipe( - Stream.filter((e) => runtimes.has(e.data.dagID as string)), Stream.runForEach((evt) => Effect.gen(function* () { const dagID = evt.data.dagID as string @@ -959,6 +967,18 @@ const serviceLayer = Layer.effect( // claiming the wake lease when a delivery is attempted. yield* automation.unregister(SessionID.make(parentSessionID), { kind: "dag", id: dagID }) yield* tryDeliverWake(parentSessionID).pipe(Effect.ignore, Effect.forkScoped) + } else { + // GOAL-FP-01-03 follow-up (P2-A): no runtime entry, but the + // startup wake sweep may have registered this non-terminal + // row before its recovery failed. Release from the durable + // row (session_id + project) so a control-op + // terminalization cannot leave a permanent registration + // with no runtime to ever clean it. Foreign-project events + // are a no-op here. + const wf = yield* store.getWorkflow(dagID) + if (wf && wf.projectId === ctx.project.id) { + yield* automation.unregister(SessionID.make(wf.sessionId), { kind: "dag", id: dagID }) + } } }).pipe(guarded("WorkflowTerminal")), ), diff --git a/packages/opencode/test/dag/dag-lease-lifecycle.test.ts b/packages/opencode/test/dag/dag-lease-lifecycle.test.ts index f572290ab7..c8fc4e1aaf 100644 --- a/packages/opencode/test/dag/dag-lease-lifecycle.test.ts +++ b/packages/opencode/test/dag/dag-lease-lifecycle.test.ts @@ -3,7 +3,7 @@ import { Deferred, Effect, Layer, Option, Queue } from "effect" import type { SessionV1 } from "@opencode-ai/core/v1/session" import { Database } from "@opencode-ai/core/database/database" import { DagProjector } from "@opencode-ai/core/dag/projector" -import { WorkflowTable } from "@opencode-ai/core/dag/sql" +import { WorkflowNodeTable, WorkflowTable } from "@opencode-ai/core/dag/sql" import { DagStore } from "@opencode-ai/core/dag/store" import { EventV2 } from "@opencode-ai/core/event" import { Project } from "@opencode-ai/core/project" @@ -115,18 +115,25 @@ function leaseLifecycleLayer(input: { childPrompts: Queue.Queue const childTitles = new Map() const created: string[] = [] const session = Layer.mock(Session.Service, { - get: () => - Effect.succeed({ - id: SessionID.make(PARENT_SESSION), - slug: "parent", - projectID: Project.ID.make(PROJECT_ID), - directory: process.cwd(), - title: "Parent", - version: "test", - time: { created: 0, updated: 0 }, - permission: [], - agent: "build", - }), + get: (sessionID) => + sessionID === "ses_child_ghost" + ? // Simulated session-store DEFECT: a die passes through the checker's + // catchTag("NotFoundError") (recovery.ts: "any other failure must + // propagate"), so reconcileWorkflow aborts recoverWorkflow for the + // ghost workflow — leaving its row non-terminal with NO runtime + // entry, the P2-A registration-leak precondition. + Effect.die("simulated session store defect (ghost child)") + : Effect.succeed({ + id: SessionID.make(PARENT_SESSION), + slug: "parent", + projectID: Project.ID.make(PROJECT_ID), + directory: process.cwd(), + title: "Parent", + version: "test", + time: { created: 0, updated: 0 }, + permission: [], + agent: "build", + }), create: (value) => Effect.sync(() => { const id = `ses_child_${created.length + 1}` @@ -364,3 +371,100 @@ describe("DagLoop lease lifecycle — terminal event release (GOAL-FP-01-03)", ( ) }) }) + +describe("DagLoop lease lifecycle — runtime-less terminal release (GOAL-FP-01-03 follow-up)", () => { + it("releases a swept registration when a workflow with no runtime entry is terminalized by a control op", async () => { + await Effect.runPromise( + runLeaseTest(({ loop, dag, store, status, automation, database }) => + Effect.gen(function* () { + const sid = SessionID.make(PARENT_SESSION) + + // A workflow whose recovery FAILS at startup: its running node + // references a child session the session store cannot read, so + // reconcileWorkflow's checker failure aborts recoverWorkflow + // BEFORE the runtime entry is created. The row stays non-terminal + // with no runtime entry — the P2-A precondition. + yield* database.db + .insert(WorkflowTable) + .values({ + id: "dag-wf-ghost", + project_id: Project.ID.make(PROJECT_ID), + session_id: SessionID.make(PARENT_SESSION), + title: "unrecoverable", + status: "running", + config: "", + seq: 1, + wake_reported: true, + }) + .run() + .pipe(Effect.orDie) + yield* database.db + .insert(WorkflowNodeTable) + .values({ + id: "n1", + workflow_id: "dag-wf-ghost", + name: "n1", + worker_type: "build", + status: "running", + required: true, + depends_on: [], + child_session_id: "ses_child_ghost", + seq: 1, + }) + .run() + .pipe(Effect.orDie) + + // An unreported terminal workflow makes the session visible to the + // startup wake sweep — which registers the non-terminal ghost. + yield* database.db + .insert(WorkflowTable) + .values({ + id: "dag-wf-undone", + project_id: Project.ID.make(PROJECT_ID), + session_id: SessionID.make(PARENT_SESSION), + title: "terminal before delivery", + status: "failed", + config: "", + seq: 2, + wake_reported: false, + }) + .run() + .pipe(Effect.orDie) + + // The session is NOT idle when DagLoop boots, so the forked wake + // redelivery aborts — no delivery-side register/unregister. + yield* status.set(sid, { type: "busy" }) + yield* loop.init() + + // The sweep registered the ghost (non-terminal) even though its + // recovery failed and no runtime entry exists. + expect(Option.isSome(yield* automation.claim(sid, { kind: "dag" }))).toBe(true) + expect((yield* store.getWorkflow("dag-wf-ghost"))?.status).toBe("running") + + // A control op terminalizes it — a real WorkflowCancelled event + // that no runtime entry backs. + yield* dag.cancel("dag-wf-ghost") + yield* pollWithTimeout( + store.getWorkflow("dag-wf-ghost").pipe( + Effect.map((wf) => (wf?.status === "cancelled" ? wf : undefined)), + ), + "runtime-less workflow did not cancel", + ) + + // Public contract: the terminal event must release the swept + // registration even though the workflow has no runtime entry. + yield* pollWithTimeout( + automation.claim(sid, { kind: "dag" }).pipe( + Effect.map((token) => (Option.isNone(token) ? true : undefined)), + ), + "dag lease was not released when a runtime-less workflow terminalized", + ) + + const goalOwner = { kind: "goal" as const, id: "goal-1" } + yield* automation.register(sid, goalOwner) + expect(Option.isSome(yield* automation.claim(sid, goalOwner))).toBe(true) + }), + ), + ) + }) +}) From 4ee892218056c7a7e446619385c474b026cc15c6 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 03:46:16 +0800 Subject: [PATCH 04/11] fix(goal): re-trigger goal evaluation when the dag owner releases (GOAL-FP-01-02) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The final DAG lease unregister (U2) lands AFTER the last wake turn's idle event: the runner emits the session idle status before completing its awaiter, so GoalLoop's idle-driven claim still sees the dag registration and yields; after U2 lands there is no second idle and the active goal silently stalls until the next external idle. SessionAutomationLease.unregister now detects the dag -> goal/none owner transition (before/after compare under the per-session KeyedMutex, generation bump semantics preserved) and re-triggers the goal evaluation by reusing the EXISTING idle status event mechanism (SessionStatus.set idle) — no new event or GoalLoop consumer. The publish runs after the lock (unconditional fire-and-forget enqueue, cannot lose or duplicate; Set.delete is idempotent and only the last dag removal flips the owner). A busy-session gate avoids spurious judge calls mid-turn: a busy turn always re-emits idle on completion, which re-drives the claim with the dag already released. This is also the GOAL-FP-01-11 mitigation surface: a claim that lost the ownership race gets another chance once the owner actually transfers. TDD evidence: - Red: test/dag/dag-goal-wake-retrigger.test.ts fails on pre-fix code with "goal was not re-evaluated after the dag lease release (GOAL-FP-01-02)" after the workflow completes and the wake is reported, no further idle events published (saved /tmp/red-goal-fp-01-02.txt). - Green: real DagLoop wake delivery end-to-end (U2 fires in the delivery tap) + real GoalLoop on the shared bus; goal claimed, judge runs, turns_used advances, continuation dispatched. - Mutation: reverting the unregister re-trigger makes the test Red again (saved /tmp/mutation-red-goal-fp-01-02.txt); restored to Green. - e2e-loop "DAG owner arbitration" updated to the new contract: the dag release alone re-drives the goal (manual second idle publish removed); its SessionStatus wiring switched to provideMerge so the lease re-trigger is visible from the test body context. Verification: bun test test/dag test/goal test/session/automation-lease.test.ts = 566 pass / 0 fail; bun typecheck (tsgo --noEmit) clean; bun lint = 4852 warnings (at the ratchet threshold, 0 errors). Co-Authored-By: Claude --- .../opencode/src/session/automation-lease.ts | 45 ++- .../test/dag/dag-goal-wake-retrigger.test.ts | 364 ++++++++++++++++++ packages/opencode/test/goal/e2e-loop.test.ts | 16 +- 3 files changed, 416 insertions(+), 9 deletions(-) create mode 100644 packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts diff --git a/packages/opencode/src/session/automation-lease.ts b/packages/opencode/src/session/automation-lease.ts index 138fddecbf..b44537cf93 100644 --- a/packages/opencode/src/session/automation-lease.ts +++ b/packages/opencode/src/session/automation-lease.ts @@ -4,6 +4,7 @@ import { Context, Effect, Layer, Option } from "effect" import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { KeyedMutex } from "@opencode-ai/core/effect/keyed-mutex" import { SessionID } from "./schema" +import { SessionStatus } from "./status" export type Owner = | { readonly kind: "goal"; readonly id: string } @@ -71,16 +72,54 @@ export const layer = Layer.sync(Service, () => { sessionID: SessionID, value: Owner, ) { - yield* locks.withLock(sessionID)( + // GOAL-FP-01-02: when the dag ownership actually disappears (owner + // transitions dag → goal/none), re-trigger the goal evaluation through + // the EXISTING idle status event mechanism so a goal that yielded to the + // dag on the last idle event gets a fresh evaluation. The final dag + // unregister of a wake delivery (U2 in dag/runtime/loop.ts) lands AFTER + // the wake turn's idle event — without this re-trigger the goal silently + // stalls until the next external idle. This is also the GOAL-FP-01-11 + // mitigation surface: a claim that lost the ownership race gets another + // chance once the owner actually transfers. + // + // The dag-release decision is computed atomically under the per-session + // lock (compare owner before/after the Set removal, accounting for the + // generation bump); the idle publish itself runs AFTER the lock. The + // publish is an unconditional fire-and-forget bus enqueue — no interleave + // can suppress it — and subscribers process it in their own fibers + // (GoalLoop / DagLoop fork their work before touching the lease lock), so + // no deadlock is possible. Set.delete is idempotent and only the removal + // of the LAST dag flips the owner, so the emit cannot duplicate. + const dagOwnershipReleased = yield* locks.withLock(sessionID)( Effect.sync(() => { const current = registrations.get(sessionID) - if (!current) return + if (!current) return false + const before = owner(sessionID) const values = value.kind === "dag" ? current.dags : current.goals - if (!values.delete(value.id)) return + if (!values.delete(value.id)) return false current.generation += 1 if (current.goals.size === 0 && current.dags.size === 0) registrations.delete(sessionID) + const after = owner(sessionID) + return before?.kind === "dag" && after?.kind !== "dag" }), ) + if (!dagOwnershipReleased) return + // SessionStatus is resolved optionally: automation-lease is deliberately + // dependency-free (consumers wire it standalone, e.g. + // test/session/automation-lease.test.ts), and every entry point that runs + // the lease (AppLayer, DagLoop, GoalLoop) provides SessionStatus. Without + // it the re-trigger degrades to the pre-fix behavior (the caller's next + // idle event still drives the goal — claim re-evaluation is never + // load-bearing for correctness of the lease itself). + const status = yield* Effect.serviceOption(SessionStatus.Service) + if (Option.isNone(status)) return + // Only re-trigger when the session is actually idle: a busy session's + // turn ALWAYS re-emits idle when it finishes (runner onIdle → + // SessionStatus.set), which re-drives the goal claim with the dag already + // released. Emitting here mid-turn would waste a judge call and transiently + // drop the busy entry from the status map. + if ((yield* status.value.get(sessionID)).type !== "idle") return + yield* status.value.set(sessionID, { type: "idle" }) }) const claim = Effect.fn("SessionAutomationLease.claim")(function* ( diff --git a/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts new file mode 100644 index 0000000000..30e46087d3 --- /dev/null +++ b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts @@ -0,0 +1,364 @@ +import { describe, expect, it } from "bun:test" +import { Deferred, Effect, Layer, Option, Queue } from "effect" +import type { SessionV1 } from "@opencode-ai/core/v1/session" +import { Database } from "@opencode-ai/core/database/database" +import { DagProjector } from "@opencode-ai/core/dag/projector" +import { DagStore } from "@opencode-ai/core/dag/store" +import { EventV2 } from "@opencode-ai/core/event" +import { Project } from "@opencode-ai/core/project" +import { ProjectTable } from "@opencode-ai/core/project/sql" +import { AbsolutePath } from "@opencode-ai/core/schema" +import { SessionTable } from "@opencode-ai/core/session/sql" +import { Model } from "@opencode-ai/schema/model" +import { Provider as ProviderSchema } from "@opencode-ai/schema/provider" +import { Provider as ProviderService } from "@/provider/provider" +import { Agent } from "@/agent/agent" +import { Dag, type NodeConfig } from "@/dag/dag" +import { DagLoop } from "@/dag/runtime/loop" +import { InstanceRef } from "@/effect/instance-ref" +import { EventV2Bridge } from "@/event-v2-bridge" +import { Goal } from "@/goal/goal" +import { GoalLoop, GoalLoopJudgeLLM } from "@/goal/loop" +import { SessionAutomationLease } from "@/session/automation-lease" +import { MessageID, PartID, SessionID } from "@/session/schema" +import { SessionPrompt } from "@/session/prompt" +import { Session } from "@/session/session" +import { SessionStatus } from "@/session/status" +import { pollWithTimeout } from "../lib/effect" + +// GOAL-FP-01-02: the final DAG lease unregister (U2) lands AFTER the wake +// turn's idle event. GoalLoop's claim runs on idle while the dag registration +// still exists and yields; no further idle event follows, so an active goal +// silently stalls. Contract under test: when the dag owner disappears, +// unregister itself must re-trigger the goal evaluation through the existing +// idle status event mechanism — with NO further external idle events. +// +// Real DagLoop (adoption, terminal handler, wake delivery end-to-end so U2 +// fires inside the delivery tap) + real GoalLoop (idle subscription on the +// real event bus, judge scripted via GoalLoopJudgeLLM) + real +// SessionAutomationLease / SessionStatus / Goal / DagStore over one in-memory +// database. Session / SessionPrompt / Agent / Provider are mocked exactly +// like the wake-integration harness. + +interface ChildPromptGate { + readonly title: string + readonly release: Deferred.Deferred +} + +const PARENT_SESSION = "ses_parent" +const PROJECT_ID = "project-1" + +// Scripted assistant response — afterIdle extracts its text as the judge input. +const mkAssistant = (): SessionV1.WithParts => reply("ses_any", "I have made progress on the feature.") + +function node(id: string, dependsOn: string[] = []): NodeConfig { + return { + id, + name: id, + worker_type: "build", + depends_on: dependsOn, + required: true, + prompt_template: { inline: id }, + report_to_parent: true, + } +} + +function reply(sessionID: string, text: string): SessionV1.WithParts { + const id = MessageID.ascending() + return { + info: { + id, + role: "assistant", + parentID: MessageID.ascending(), + sessionID: SessionID.make(sessionID), + mode: "build", + agent: "build", + cost: 0, + path: { cwd: process.cwd(), root: process.cwd() }, + tokens: { input: 0, output: 0, reasoning: 0, cache: { read: 0, write: 0 } }, + modelID: Model.ID.make("test-model"), + providerID: ProviderSchema.ID.make("test"), + time: { created: Date.now() }, + finish: "stop", + }, + parts: text ? [{ id: PartID.ascending(), sessionID: SessionID.make(sessionID), messageID: id, type: "text", text }] : [], + } +} + +function takeWithin(queue: Queue.Queue, message: string) { + return Queue.take(queue).pipe( + Effect.timeoutOption("1 second"), + Effect.flatMap( + Option.match({ + onNone: () => Effect.fail(new Error(message)), + onSome: Effect.succeed, + }), + ), + ) +} + +// Mutable observation state shared by the layer mocks and the test body. +let judgeCalls = 0 +let promptCalls: { noReply?: boolean; text: string }[] = [] +const reset = () => { + judgeCalls = 0 + promptCalls = [] +} + +function goalWakeLayer(input: { childPrompts: Queue.Queue }) { + const database = Database.layerFromPath(":memory:") + const events = EventV2.layer.pipe(Layer.provide(database)) + const bridge = EventV2Bridge.layer.pipe(Layer.provide(events)) + const store = DagStore.layer.pipe(Layer.provide(database)) + const status = SessionStatus.layer.pipe(Layer.provide(bridge)) + const projector = DagProjector.layer.pipe( + Layer.provide(events), + Layer.provide(database), + ) + const dag = Dag.layer.pipe( + Layer.provide(bridge), + Layer.provide(store), + ) + const goal = Goal.layer.pipe( + Layer.provide(bridge), + Layer.provide(database), + Layer.provide(status), + ) + const base = Layer.mergeAll(database, events, bridge, store, projector, dag, goal, status) + const childTitles = new Map() + const created: string[] = [] + const session = Layer.mock(Session.Service, { + get: (_sessionID) => + Effect.succeed({ + id: SessionID.make(PARENT_SESSION), + slug: "parent", + projectID: Project.ID.make(PROJECT_ID), + directory: process.cwd(), + title: "Parent", + version: "test", + time: { created: 0, updated: 0 }, + }), + create: (value) => + Effect.sync(() => { + const id = `ses_child_${created.length + 1}` + created.push(id) + childTitles.set(id, (value?.title ?? id).replace(" (DAG node)", "")) + return { + id: SessionID.make(id), + slug: "child", + projectID: Project.ID.make(PROJECT_ID), + directory: process.cwd(), + title: value?.title ?? id, + version: "test", + time: { created: 0, updated: 0 }, + } + }), + // GoalLoop.afterIdle reads the last-20 message window: an assistant + // message must exist so the judge is reached (no stale-zombie / no-assistant + // early pauses). + messages: () => Effect.succeed([mkAssistant()]), + }) + const deliver = Effect.fn("test.goalWake.SessionPrompt.deliver")(function* (value: SessionPrompt.PromptInput) { + const sessionID = value.sessionID as string + if (sessionID === PARENT_SESSION) { + // Parent prompts: the dag wake delivery AND the goal continuation must + // both succeed. Record the call so the test can tell them apart. + yield* Effect.sync(() => { + promptCalls.push({ + noReply: value.noReply, + text: value.parts?.map((p) => (p.type === "text" ? p.text : "")).join("\n") ?? "", + }) + }) + return reply(sessionID, "parent turn") + } + const release = yield* Deferred.make() + yield* Queue.offer(input.childPrompts, { title: childTitles.get(sessionID) ?? sessionID, release }) + return reply(sessionID, yield* Deferred.await(release)) + }) + const prompt = Layer.mock(SessionPrompt.Service, { + cancel: () => Effect.void, + prompt: deliver, + promptIfIdle: (value) => deliver(value).pipe(Effect.map(Option.some)), + }) + const agent = Layer.mock(Agent.Service, { + get: () => + Effect.succeed({ + name: "build", + mode: "all", + permission: [], + options: {}, + description: "", + prompt: "", + model: { providerID: ProviderSchema.ID.make("test"), modelID: Model.ID.make("test-model") }, + tools: {}, + hooks: {}, + }), + }) + const loop = DagLoop.layer.pipe( + Layer.provide(base), + Layer.provide(session), + Layer.provide(prompt), + Layer.provide(agent), + ) + // Real GoalLoop over the same shared bus/status/goal/lease instances. + const goalLoop = GoalLoop.layer.pipe( + Layer.provide(session), + Layer.provide(prompt), + Layer.provide(Layer.mock(ProviderService.Service, {})), + Layer.provide( + Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => + Effect.sync(() => { + judgeCalls += 1 + return JSON.stringify({ done: false, reason: "more steps needed" }) + }), + }), + ), + ), + Layer.provide(goal), + Layer.provide(status), + Layer.provide(bridge), + ) + // DagLoop.layer / GoalLoop.layer / Goal.layer consume the lease internally. + // Merge the SAME module-level layer at the top so the test body observes the + // very instance DagLoop and GoalLoop use (Layer.build memoization dedups the + // shared layer reference). + return Layer.mergeAll(base, loop, goalLoop, SessionAutomationLease.defaultLayer) +} + +function runGoalWakeTest( + test: (services: { + readonly dag: Dag.Interface + readonly loop: DagLoop.Interface + readonly goalLoop: GoalLoop.Interface + readonly store: DagStore.Interface + readonly goal: Goal.Interface + readonly automation: SessionAutomationLease.Interface + readonly database: Database.Interface + readonly childPrompts: Queue.Queue + }) => Effect.Effect, +) { + return Effect.gen(function* () { + const childPrompts = yield* Queue.unbounded() + return yield* Effect.gen(function* () { + const dag = yield* Dag.Service + const loop = yield* DagLoop.Service + const goalLoop = yield* GoalLoop.Service + const store = yield* DagStore.Service + const goal = yield* Goal.Service + const automation = yield* SessionAutomationLease.Service + const database = yield* Database.Service + yield* database.db + .insert(ProjectTable) + .values({ + id: Project.ID.make(PROJECT_ID), + worktree: AbsolutePath.make(process.cwd()), + sandboxes: [], + }) + .run() + .pipe(Effect.orDie) + yield* database.db + .insert(SessionTable) + .values({ + id: SessionID.make(PARENT_SESSION), + project_id: Project.ID.make(PROJECT_ID), + slug: "parent", + directory: AbsolutePath.make(process.cwd()), + title: "Parent", + version: "test", + }) + .run() + .pipe(Effect.orDie) + return yield* test({ dag, loop, goalLoop, store, goal, automation, database, childPrompts }) + }).pipe( + Effect.provide(goalWakeLayer({ childPrompts })), + Effect.provideService(InstanceRef, { + directory: process.cwd(), + worktree: process.cwd(), + project: { + id: Project.ID.make(PROJECT_ID), + worktree: process.cwd(), + time: { created: 0, updated: 0 }, + sandboxes: [], + }, + }), + Effect.scoped, + ) + }) +} + +describe("DagLoop final wake delivery re-triggers the goal (GOAL-FP-01-02)", () => { + it("an active goal is claimed and progresses after the dag lease release, with no further idle events", async () => { + await Effect.runPromise( + runGoalWakeTest(({ dag, loop, goalLoop, store, goal, automation, childPrompts }) => + Effect.gen(function* () { + reset() + const sid = SessionID.make(PARENT_SESSION) + + yield* loop.init() + yield* goalLoop.init() + // Give the forkScoped idle subscriptions one scheduler turn to + // acquire their PubSub subscriptions. + yield* Effect.yieldNow + + // An active goal in the same session. No idle event is ever + // published by the test body from here on. + const goalState = yield* goal.set(sid, "ship the feature", 10) + const goalOwner = { kind: "goal" as const, id: goalState.goal_id ?? "legacy" } + yield* Effect.yieldNow + + const dagID = yield* dag.create({ + projectID: PROJECT_ID, + sessionID: PARENT_SESSION, + title: "wake retrigger", + config: { name: "wake-retrigger", nodes: [node("implement")] }, + }) + + // Adoption (WorkflowStarted) registered the dag lease for the parent. + const child = yield* takeWithin(childPrompts, "implement did not start") + expect(Option.isSome(yield* automation.claim(sid, { kind: "dag" }))).toBe(true) + + // Complete the node → workflow terminalizes → the terminal handler + // releases the dag registration and forks the wake delivery. + yield* Deferred.succeed(child.release, "done") + yield* pollWithTimeout( + store.getWorkflow(dagID).pipe( + Effect.map((workflow) => (workflow?.status === "completed" ? workflow : undefined)), + ), + "workflow did not complete", + ) + + // The final wake delivery succeeded and reported (U2 unregistered the + // terminal workflow inside the delivery tap). + yield* pollWithTimeout( + store.getWorkflow(dagID).pipe( + Effect.map((workflow) => (workflow?.wakeReported ? workflow : undefined)), + ), + "wake was never reported", + ) + + // Public contract: with NO further idle events, the dag release must + // itself re-trigger the goal evaluation. judgeCalls > 0 proves + // GoalLoop.afterIdle ran a full cycle (lease claimed → judge → + // updateAfterJudge → continuation dispatch). + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "goal was not re-evaluated after the dag lease release (GOAL-FP-01-02)", + "5 seconds", + ) + + const g = yield* goal.load(sid) + expect(g?.status).toBe("active") + expect(Number(g?.turns_used)).toBeGreaterThanOrEqual(1) + // The continuation prompt (not a noReply pause line) carries the goal. + expect(promptCalls.some((p) => !p.noReply && p.text.includes("ship the feature"))).toBe(true) + // Ownership transferred: the dag lease is gone, the goal owns the session. + expect(Option.isNone(yield* automation.claim(sid, { kind: "dag" }))).toBe(true) + expect(Option.isSome(yield* automation.claim(sid, goalOwner))).toBe(true) + }), + ), + ) + }) +}) diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index 4b37f00334..e79a9990b4 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -281,13 +281,17 @@ describe("GoalLoop + DAG owner arbitration", () => { Layer.provide(Layer.succeed(Provider.Service, {} as never)), Layer.provide(judgeMock), Layer.provideMerge(Goal.defaultLayer), - Layer.provide(SessionStatus.defaultLayer), + // provideMerge (not provide): the lease's GOAL-FP-01-02 re-trigger runs + // in the test body's context when unregister is called from the body, so + // SessionStatus must be part of the output context (branch 3 documents + // the same pattern). + Layer.provideMerge(SessionStatus.defaultLayer), Layer.provideMerge(EventV2Bridge.defaultLayer), Layer.provideMerge(SessionAutomationLease.defaultLayer), ) const it = testEffect(arbitrationLayer) - it.instance("a live DAG owns the Session; Goal resumes after the DAG releases it", () => + it.instance("a live DAG owns the Session; Goal resumes when the DAG releases it", () => Effect.gen(function* () { judgeCalls = 0 continuationCalls = 0 @@ -309,11 +313,11 @@ describe("GoalLoop + DAG owner arbitration", () => { expect(judgeCalls).toBe(0) expect(continuationCalls).toBe(0) + // GOAL-FP-01-02: the dag release itself re-triggers the goal evaluation + // through the idle status event mechanism — no follow-up idle event is + // needed. This is exactly the stall that previously required the manual + // second idle publish below. yield* automation.unregister(sessionID, { kind: "dag", id: "dag-executor" }) - yield* events.publish(SessionStatus.Event.Status, { - sessionID, - status: { type: "idle" }, - }) yield* pollWithTimeout( Effect.sync(() => (continuationCalls === 1 ? true : undefined)), "Goal did not resume after the DAG released the Session lease", From e75d09996151f6c73ac7f8c4ce3672c4f6907748 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 04:45:20 +0800 Subject: [PATCH 05/11] fix(goal): serialize afterIdle evaluation across re-trigger races (GOAL-FP-01-02 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit R1: the GOAL-FP-01-02 unregister re-trigger publishes a duplicate idle for every dag release, so the turn-idle fiber B (whose claim landed after U2) and the retry fiber D both hold valid same-generation goal tokens. The harmful interleavings on the synthetic no-text verdict path: 4a — B commits and is interrupted by D's registerLoopFiber between commit and continuation dispatch, D's stale-revision commit noops, goal silently stalls; 4b — D double-commits (turns inflation) or spurious-pauses on the busy status check. Candidate analysis: (a) per-session serialization of afterIdle alone still lets the second fiber commit again after the first dispatched (4b survives); (c) generation bump on goal re-register invalidates only the OTHER fiber's token — the revision guard still admits D's fresh-load commit (inflation) and does not stop the interrupt from killing B post-commit (4a survives); skip-if-alive on the fiber map races the fiber's unwinding window (branch-4 contract). Chosen fix (b): a per-session blocked-claim flag in the lease. Mechanism: claim records "a goal claim was rejected by the dag owner" (blockedGoalClaims); a successful (or non-dag-rejected) goal claim clears it; unregister CONSUMES it (Set.delete) inside the same per-session KeyedMutex critical section as the owner-transition decision, so the re-trigger fires exactly once per blocked claim, atomically with claim serialization. The blocked claim's evaluation fiber yields at the claim itself, so the retry it spawns is the only evaluation in flight. Unconstructibility arguments: - 4a: D is forked only if the flag was set, i.e. only after an evaluation's claim was rejected and that fiber yielded at the claim. B in flight post-commit implies B's claim succeeded, which cleared the flag under the same lock before U2's consume — no publish, no D, no interrupt. The commit→dispatch tail of the sole evaluation can no longer be raced. - 4b: D implies the flag was set and not cleared since, so no evaluation committed in between; D loads fresh state and commits once. A turn-boundary fiber whose claim succeeds clears the flag before any release decision, so one commit per boundary. The busy→pause path is unreachable for D (no turn is in flight when D runs). No loss: the retry obligation is only dropped by a successful claim (the evaluation then happened) or by the busy-gate consume — whose session re-emits idle on turn completion and re-drives the claim (runner onIdle → SessionStatus.set idle). TDD evidence: - Red: new e2e-loop test "an unblocked goal is evaluated exactly once when the dag releases before the boundary idle" fails deterministically on the unfixed re-trigger with turns_used 2 for one real boundary (Expected: 1, Received: 2), pinned by a second-dispatch gate — saved /tmp/red-goal-r1.txt. - Green: real GoalLoop + real lease + synthetic no-text verdict; the dag release stays silent when no claim was ever blocked, the boundary evaluation commits exactly once. - Mutation: reverting the blocked-claim gate to the unconditional publish makes the test Red again (Expected: 1, Received: 2) — saved /tmp/mutation-red-goal-r1.txt — then restored. - The GOAL-FP-01-02 dag wake test now reproduces the faithful production sequence: the prompt mock emits the wake turn's idle event (as the real runner does before its awaiter resolves), the blocked claim arms the re-trigger, and U2's retry drives the goal with no idle after U2. Verification: bun test test/dag test/goal test/session/automation-lease.test.ts = 567 pass / 0 fail; bun typecheck (tsgo --noEmit) clean; bun lint = 4852 warnings (at the ratchet threshold, 0 errors). Co-Authored-By: Claude --- .../opencode/src/session/automation-lease.ts | 32 ++++- .../test/dag/dag-goal-wake-retrigger.test.ts | 28 +++- packages/opencode/test/goal/e2e-loop.test.ts | 133 +++++++++++++++++- 3 files changed, 187 insertions(+), 6 deletions(-) diff --git a/packages/opencode/src/session/automation-lease.ts b/packages/opencode/src/session/automation-lease.ts index b44537cf93..90762a102c 100644 --- a/packages/opencode/src/session/automation-lease.ts +++ b/packages/opencode/src/session/automation-lease.ts @@ -35,6 +35,12 @@ export const layer = Layer.sync(Service, () => { SessionID, { readonly goals: Set; readonly dags: Set; generation: number } >() + // Sessions whose goal claim was rejected because a dag owns the automation + // lease. Set by claim, cleared by a successful (or non-dag-rejected) goal + // claim, and CONSUMED by the unregister re-trigger below — all under the + // per-session lock, so the re-trigger decision is atomic with claim + // serialization (GOAL-FP-01-02 follow-up / R1). + const blockedGoalClaims = new Set() const entry = (sessionID: SessionID) => { const current = registrations.get(sessionID) @@ -90,7 +96,16 @@ export const layer = Layer.sync(Service, () => { // (GoalLoop / DagLoop fork their work before touching the lease lock), so // no deadlock is possible. Set.delete is idempotent and only the removal // of the LAST dag flips the owner, so the emit cannot duplicate. - const dagOwnershipReleased = yield* locks.withLock(sessionID)( + // + // R1 (GOAL-FP-01-02 follow-up): the re-trigger fires ONLY when a goal + // claim was actually rejected by the dag (blockedGoalClaims). A rejected + // claim's evaluation fiber yields at the claim itself, so the retry + // evaluation it spawns is the only evaluation in flight — the duplicate + // evaluation that raced the turn-idle fiber (double commit / interrupt + // between commit and dispatch) is unconstructible. A successful goal + // claim clears the flag (under the same lock), so a release that a + // boundary evaluation already picked up does not double-fire. + const goalRetryDue = yield* locks.withLock(sessionID)( Effect.sync(() => { const current = registrations.get(sessionID) if (!current) return false @@ -100,10 +115,13 @@ export const layer = Layer.sync(Service, () => { current.generation += 1 if (current.goals.size === 0 && current.dags.size === 0) registrations.delete(sessionID) const after = owner(sessionID) - return before?.kind === "dag" && after?.kind !== "dag" + if (before?.kind !== "dag" || after?.kind === "dag") return false + // Consume the retry obligation: exactly one re-trigger per blocked + // claim, even when several dags release back-to-back. + return blockedGoalClaims.delete(sessionID) }), ) - if (!dagOwnershipReleased) return + if (!goalRetryDue) return // SessionStatus is resolved optionally: automation-lease is deliberately // dependency-free (consumers wire it standalone, e.g. // test/session/automation-lease.test.ts), and every entry point that runs @@ -130,6 +148,14 @@ export const layer = Layer.sync(Service, () => { Effect.sync(() => { const current = registrations.get(sessionID) const selected = owner(sessionID) + if (request.kind === "goal") { + // Track dag-blocked goal claims: the unregister re-trigger only + // fires for sessions whose goal evaluation was actually rejected by + // a dag owner. Any other outcome (success, or a rejection that is + // not dag-blocking) clears the obligation. + if (selected?.kind === "dag") blockedGoalClaims.add(sessionID) + else blockedGoalClaims.delete(sessionID) + } if (!current || !selected) return Option.none() if (request.kind === "goal" && (selected.kind !== "goal" || selected.id !== request.id)) return Option.none() diff --git a/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts index 30e46087d3..60fb088287 100644 --- a/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts +++ b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts @@ -31,7 +31,9 @@ import { pollWithTimeout } from "../lib/effect" // still exists and yields; no further idle event follows, so an active goal // silently stalls. Contract under test: when the dag owner disappears, // unregister itself must re-trigger the goal evaluation through the existing -// idle status event mechanism — with NO further external idle events. +// idle status event mechanism — with NO idle events AFTER U2 (the wake turn's +// own idle, which the real runner emits and the prompt mock reproduces here, +// is what blocks the claim in the first place and arms the re-trigger). // // Real DagLoop (adoption, terminal handler, wake delivery end-to-end so U2 // fires inside the delivery tap) + real GoalLoop (idle subscription on the @@ -100,9 +102,11 @@ function takeWithin(queue: Queue.Queue, message: string) { // Mutable observation state shared by the layer mocks and the test body. let judgeCalls = 0 let promptCalls: { noReply?: boolean; text: string }[] = [] +let parentPromptCalls = 0 const reset = () => { judgeCalls = 0 promptCalls = [] + parentPromptCalls = 0 } function goalWakeLayer(input: { childPrompts: Queue.Queue }) { @@ -169,6 +173,26 @@ function goalWakeLayer(input: { childPrompts: Queue.Queue }) { text: value.parts?.map((p) => (p.type === "text" ? p.text : "")).join("\n") ?? "", }) }) + // The FIRST parent prompt is the wake delivery. Mirror the real runner: + // a completed wake turn emits the session idle event before its awaiter + // resolves — i.e., before the delivery tap's U2. That idle event drives + // GoalLoop's evaluation, whose claim is rejected by the still-registered + // dag — the blocked claim the unregister re-trigger exists to retry + // (GOAL-FP-01-02 / R1). Later parent prompts are goal continuations and + // must not re-emit (the mock has no real runner turn). + if (parentPromptCalls === 0) { + parentPromptCalls += 1 + yield* Effect.serviceOption(EventV2Bridge.Service).pipe( + Effect.flatMap((bridge) => + Option.isSome(bridge) + ? bridge.value.publish(SessionStatus.Event.Status, { + sessionID: SessionID.make(sessionID), + status: { type: "idle" }, + }) + : Effect.void, + ), + ) + } return reply(sessionID, "parent turn") } const release = yield* Deferred.make() @@ -178,7 +202,7 @@ function goalWakeLayer(input: { childPrompts: Queue.Queue }) { const prompt = Layer.mock(SessionPrompt.Service, { cancel: () => Effect.void, prompt: deliver, - promptIfIdle: (value) => deliver(value).pipe(Effect.map(Option.some)), + promptIfIdle: (value: SessionPrompt.PromptInput) => deliver(value).pipe(Effect.map(Option.some)), }) const agent = Layer.mock(Agent.Service, { get: () => diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index e79a9990b4..75c37f68af 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -1,5 +1,5 @@ import { describe, expect } from "bun:test" -import { Cause, Effect, Layer, Option } from "effect" +import { Cause, Deferred, Effect, Exit, Layer, Option } from "effect" import { GoalLoop, GoalLoopJudgeLLM } from "@/goal/loop" import { Goal } from "@/goal/goal" import { GoalEvent } from "@/goal/events" @@ -328,6 +328,137 @@ describe("GoalLoop + DAG owner arbitration", () => { ) }) +// GOAL-FP-01-02 follow-up (R1): the dag-release re-trigger must NOT publish a +// duplicate idle when no goal evaluation was ever blocked by the dag. The +// unfixed re-trigger forks a full evaluation (D) whose commit consumes the +// turn boundary; the real turn-idle fiber (B) then commits AGAIN for the same +// boundary — turns inflation (and, with a live runner, the busy→pause path). +// +// Deterministic construction through the public seam: the dag releases while +// the session is idle, THEN the turn-boundary idle event is published. The +// re-trigger's evaluation (if any) completes before the boundary evaluation +// forks, so the boundary fiber always double-commits under the unfixed +// re-trigger. The second continuation dispatch is parked on a gate so the +// test observes the settled double-commit state instead of a transient. +// +// The judge is scripted out of the picture entirely: the assistant message +// carries no text, so afterIdle takes the synthetic "continue" verdict path +// (loop.ts branch 2) and the judge mock must never be reached. +describe("GoalLoop — dag release must not double-evaluate a boundary (GOAL-FP-01-02 follow-up)", () => { + let continuationCalls = 0 + let gateHit = false + let gateRelease = Deferred.makeUnsafe() + const reset = () => { + continuationCalls = 0 + gateHit = false + gateRelease = Deferred.makeUnsafe() + } + + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistantTools()]), + }) + // Second continuation dispatch parks on a gate: under the unfixed + // re-trigger the boundary fiber commits (turns 1 → 2) and reaches the gate; + // the test then observes the settled double-commit state. + const promptMock = Layer.mock(SessionPrompt.Service, { + prompt: () => Effect.die("the direct prompt path is not exercised in this scenario"), + promptIfIdle: () => + Effect.sync(() => { + continuationCalls += 1 + }).pipe( + Effect.flatMap(() => { + if (continuationCalls === 2) { + gateHit = true + return Deferred.await(gateRelease) + } + return Effect.void + }), + Effect.map(() => Option.none()), + ), + }) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => Effect.die("the synthetic no-text verdict path must never reach the judge"), + }), + ) + const raceLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.mock(Provider.Service, {})), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + // provideMerge (not provide): unregister runs in the test body context and + // the lease's re-trigger resolves SessionStatus from it (see the + // arbitration describe above). + Layer.provideMerge(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + ) + const it = testEffect(raceLayer) + + it.instance("an unblocked goal is evaluated exactly once when the dag releases before the boundary idle", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const events = yield* EventV2Bridge.Service + const automation = yield* SessionAutomationLease.Service + yield* loop.init() + const sessionID = SessionID.descending() + yield* goal.set(sessionID, "ship the feature", 10) + yield* automation.register(sessionID, { kind: "dag", id: "dag-executor" }) + yield* Effect.yieldNow + + // The dag releases while the session is idle and NO evaluation was ever + // blocked by it. The re-trigger must stay silent here. + yield* automation.unregister(sessionID, { kind: "dag", id: "dag-executor" }) + + // Under the unfixed re-trigger an evaluation (D) was already forked by + // the unregister's idle publish. Wait for it to settle so the boundary + // fiber below cannot interrupt it mid-flight. + const spuriousEvaluation = yield* pollWithTimeout( + Effect.sync(() => (continuationCalls >= 1 ? true : undefined)), + "unfixed re-trigger evaluation never dispatched", + "500 millis", + ) + .pipe(Effect.exit) + .pipe(Effect.map(Exit.isSuccess)) + + // The real turn-boundary idle event (the runner's idle emit). + yield* events.publish(SessionStatus.Event.Status, { + sessionID, + status: { type: "idle" }, + }) + + // Under the unfixed re-trigger the boundary fiber commits a SECOND time + // (turns inflation) and parks at the second-dispatch gate. + const doubleCommit = yield* pollWithTimeout( + Effect.sync(() => (gateHit ? true : undefined)), + "boundary fiber never reached the second dispatch (no double evaluation)", + "500 millis", + ) + .pipe(Effect.exit) + .pipe(Effect.map(Exit.isSuccess)) + + // Let the parked boundary fiber finish (no-op when it was never parked). + yield* Deferred.succeed(gateRelease, undefined) + yield* pollWithTimeout( + Effect.sync(() => (continuationCalls >= (spuriousEvaluation ? 2 : 1) ? true : undefined)), + "boundary evaluation never dispatched its continuation", + "5 seconds", + ) + + const g = yield* goal.load(sessionID) + expect(g?.status).toBe("active") + // The R1 harm: the boundary's single real evaluation must account for + // exactly one turn — not two. + expect(Number(g?.turns_used)).toBe(1) + expect(doubleCommit).toBe(false) + }), + ) +}) + // D1 (hooks-goal-completeness): a continuation dispatch failure must surface as a // recoverable paused state, not a silent stall. Reuses the e2e harness with a // prompt mock that always fails — the only prompt in this flow is the From 9cff41dde4f8b1c1a778c299860fd6842b07b6d4 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 05:26:16 +0800 Subject: [PATCH 06/11] fix(goal): clean up goal state, outcomes, and dag leases on session delete (GOAL-FP-01-05/-06/-16) TDD: red test first (test/session/session-remove-cleanup.test.ts, 3 fail on current code), minimal green, mutation (revert Session.defaultLayer cleanup provides -> 3 fail), restore -> green. Wiring diagnosis (-05): Session.remove resolved Goal via Effect.serviceOption(Goal.Service) captured at layer construction. In the production AppLayer (effect/app-runtime.ts) Goal.defaultLayer and Session.defaultLayer are Layer.mergeAll siblings; mergeAll builds members concurrently against the parent context only, so Goal was never in Session's build context and the cleanup silently no-op'd - `opencode session delete` orphaned the goal_state row. Fixed by making Goal, SessionAutomationLease and Dag hard requirements of Session.layer: Session.defaultLayer self-provides all three (each is self-contained, requirements=never), Session.node lists their nodes, and tsgo now enforces the wiring at every composition site (4 raw-layer test harnesses updated). No layer cycle: Goal -> SessionStatus/Lease, Dag -> DagStore/DagProjector, none depends on Session. Cleanup (-06): Session.remove now (1) purges goal rows via Goal.purgeSession, (2) cancels owned non-terminal workflows via the existing Dag.cancel authority (durable terminalization; the DagLoop terminal handler aborts child sessions and releases the dag lease - no second runtime authority), and (3) purges the session's lease registrations via the new SessionAutomationLease.purgeSession (under the per-session KeyedMutex). Each step catches its cause and logs a warning; deletion itself still cannot fail. Ordering + crash window: cleanup runs BEFORE the Deleted publish (the SessionProjector deletes the session row inside that transaction; FK cascade then wipes workflow rows). A crash mid-way leaves a live session with no goal/workflows (consistent, recoverable) - never orphan goal rows or re-adoptable workflows under a deleted session. No shared transaction exists (three separate aggregates: goal tables, workflow events, lease map); each step is individually atomic. -16: goal_outcome rows now deleted in the same durable transition transaction as the goal_state row (transition seam gained a deleteOutcomes flag; Goal.purgeSession sets it, Goal.clear keeps outcome history). Verification: bun test test/session test/goal test/dag -> 964 pass, 0 fail; bun typecheck clean; bun lint 4852 (ratchet). Co-Authored-By: Claude --- packages/opencode/src/goal/goal.ts | 27 +++ .../opencode/src/session/automation-lease.ts | 19 ++- packages/opencode/src/session/session.ts | 93 ++++++++++- .../opencode/test/hook/event-wiring.test.ts | 6 + .../opencode/test/server/session-list.test.ts | 6 + .../opencode/test/session/fork-batch.test.ts | 6 + .../session/session-remove-cleanup.test.ts | 156 ++++++++++++++++++ .../opencode/test/session/session.test.ts | 6 + 8 files changed, 309 insertions(+), 10 deletions(-) create mode 100644 packages/opencode/test/session/session-remove-cleanup.test.ts diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index 6dd1b9bc36..62ce376d01 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -25,6 +25,8 @@ export interface Interface { readonly pause: (sessionID: SessionID, reason: string) => Effect.Effect readonly resume: (sessionID: SessionID) => Effect.Effect readonly clear: (sessionID: SessionID) => Effect.Effect + /** Session-deletion cleanup: remove goal_state AND all goal_outcome rows. */ + readonly purgeSession: (sessionID: SessionID) => Effect.Effect readonly markDone: (sessionID: SessionID, reason: string) => Effect.Effect readonly addSubgoal: (sessionID: SessionID, subgoal: string) => Effect.Effect readonly removeSubgoal: ( @@ -180,6 +182,9 @@ const serviceLayer = Layer.effect( | { readonly tag: "delete" readonly terminal?: GoalState.Info + /** GOAL-FP-01-16: also delete every goal_outcome row for the session + * in the same transaction (session deletion, not a plain clear). */ + readonly deleteOutcomes?: boolean readonly value: A } @@ -248,6 +253,12 @@ const serviceLayer = Layer.effect( .where(eq(GoalStateTable.session_id, sessionID)) .run() } + if (next.tag === "delete" && next.deleteOutcomes) { + yield* tx + .delete(GoalOutcomeTable) + .where(eq(GoalOutcomeTable.session_id, sessionID)) + .run() + } return next }), { behavior: "immediate" }, @@ -377,6 +388,21 @@ const serviceLayer = Layer.effect( yield* clearFiber(sessionID) }) + // GOAL-FP-01-05/-16: session-deletion cleanup. `clear` keeps the + // goal_outcome history (lastOutcome readers), but a deleted session has no + // readers — its outcome rows are garbage and must go in the SAME durable + // transition as the goal_state row so the pair cannot be split by a crash. + const purgeSession = Effect.fn("Goal.purgeSession")(function* (sessionID: SessionID) { + const cleared = yield* transition(sessionID, (state) => ({ + tag: "delete", + deleteOutcomes: true, + value: state, + })) + if (cleared) + yield* automation.unregister(sessionID, { kind: "goal", id: cleared.goal_id ?? "legacy" }) + yield* clearFiber(sessionID) + }) + const markDone = Effect.fn("Goal.markDone")(function* (sessionID: SessionID, reason: string) { // User/tool-initiated completion: stop the running loop fiber, then // perform terminal cleanup (publish done-updated → delete → publish cleared). @@ -703,6 +729,7 @@ const serviceLayer = Layer.effect( pause, resume, clear, + purgeSession, markDone, addSubgoal, removeSubgoal, diff --git a/packages/opencode/src/session/automation-lease.ts b/packages/opencode/src/session/automation-lease.ts index 90762a102c..ac0fd00012 100644 --- a/packages/opencode/src/session/automation-lease.ts +++ b/packages/opencode/src/session/automation-lease.ts @@ -25,6 +25,8 @@ export interface Interface { readonly unregister: (sessionID: SessionID, owner: Owner) => Effect.Effect readonly claim: (sessionID: SessionID, request: Request) => Effect.Effect> readonly use: (token: Token, effect: Effect.Effect) => Effect.Effect, E, R> + /** Drop every registration and retry obligation for a session (session deletion). */ + readonly purgeSession: (sessionID: SessionID) => Effect.Effect } export class Service extends Context.Service()("@opencode/SessionAutomationLease") {} @@ -182,7 +184,22 @@ export const layer = Layer.sync(Service, () => { return Option.some(yield* effect) }) - return Service.of({ register, unregister, claim, use }) + // GOAL-FP-01-06: session deletion must drop every registration the session + // holds (goal, dag, and any wake-sweep registration) so the automation + // ownership map cannot keep a deleted session's claim alive until process + // exit. Runs under the per-session lock, same as every other mutation, and + // deliberately does NOT emit the unregister goal re-trigger — the session is + // being deleted, so a goal re-evaluation would be work on a dead session. + const purgeSession = Effect.fn("SessionAutomationLease.purgeSession")(function* (sessionID: SessionID) { + yield* locks.withLock(sessionID)( + Effect.sync(() => { + registrations.delete(sessionID) + blockedGoalClaims.delete(sessionID) + }), + ) + }) + + return Service.of({ register, unregister, claim, use, purgeSession }) }) export const defaultLayer = layer diff --git a/packages/opencode/src/session/session.ts b/packages/opencode/src/session/session.ts index b3c65515f4..985a22e517 100644 --- a/packages/opencode/src/session/session.ts +++ b/packages/opencode/src/session/session.ts @@ -45,6 +45,9 @@ import { ProviderV2 } from "@opencode-ai/core/provider" import { ModelV2 } from "@opencode-ai/core/model" import { SessionMessageID } from "@opencode-ai/schema/session-message-id" import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "./automation-lease" +import { Dag } from "@/dag/dag" +import { isWorkflowTerminalStatus } from "@opencode-ai/core/dag/core/types" import { landSystemMessages } from "@/hook/trigger-result" const runtime = makeRuntime(Database.Service, Database.defaultLayer) @@ -486,7 +489,13 @@ export type Patch = Omit, "time" | "share" | "summary" | "revert" export const layer: Layer.Layer< Service, never, - BackgroundJob.Service | RuntimeFlags.Service | Database.Service | EventV2Bridge.Service + | BackgroundJob.Service + | RuntimeFlags.Service + | Database.Service + | EventV2Bridge.Service + | Goal.Service + | SessionAutomationLease.Service + | Dag.Service > = Layer.effect( Service, Effect.gen(function* () { @@ -507,10 +516,21 @@ export const layer: Layer.Layer< // deferred import resolves to the cached module instantly. const { SettingsHook } = yield* Effect.promise(() => import("@/hook/settings")) const settingsHook = Option.getOrUndefined(yield* Effect.serviceOption(SettingsHook.Service)) - // Goal cleanup is optional — Session must not require Goal at construction - // (that would force every Session.defaultLayer consumer to provide Goal's - // transitive deps). Resolved lazily via serviceOption. - const goalOpt = yield* Effect.serviceOption(Goal.Service) + // GOAL-FP-01-05: goal/dag/lease cleanup used to resolve via + // Effect.serviceOption(Goal.Service), which yields None in the production + // AppLayer — Goal.defaultLayer and Session.defaultLayer are + // Layer.mergeAll siblings (effect/app-runtime.ts) and mergeAll does not + // cross-provide, so Session's layer context never contained Goal and + // `opencode session delete` silently skipped the cleanup. The cleanup is + // NOT optional (delete integrity), so these are now hard layer + // requirements: typecheck enforces that every composition of Session's + // layer provides them (defaultLayer self-provides all three below; the + // node graph lists them in Session.node). No layer cycle exists — Goal, + // Dag and SessionAutomationLease defaultLayers are all self-contained and + // none of them depends on Session. + const goal = yield* Goal.Service + const dag = yield* Dag.Service + const automation = yield* SessionAutomationLease.Service const createNext = Effect.fn("Session.createNext")(function* (input: { id?: SessionID @@ -644,10 +664,48 @@ export const layer: Layer.Layer< .pipe(Effect.catch(() => Effect.succeed({ additionalContexts: [], systemMessages: [] }))) yield* landSystemMessages(seResult, { sessionID }) } - // Cleanup goal state (only when Goal service is available in context) - if (goalOpt._tag === "Some") { - yield* goalOpt.value.clear(sessionID).pipe(Effect.catchCause(() => Effect.void)) + // Cleanup durable automation state BEFORE the Deleted publish: the + // SessionProjector deletes the session row (and FK cascades wipe the + // workflow rows) inside the Deleted publish transaction, so running + // cleanup first means a crash mid-way can only leave a live session + // with no goal/workflows (consistent, recoverable) — never orphan + // goal rows or re-adoptable workflows under a deleted session. The + // three steps live in separate aggregates (goal_state/goal_outcome, + // workflow events, the lease map) so no shared transaction is + // available; each step is individually atomic. + yield* goal.purgeSession(sessionID).pipe( + Effect.catchCause((cause) => + Effect.logWarning("goal purge failed during session remove", { sessionID, cause }), + ), + ) + // GOAL-FP-01-06: cancel workflows owned by this session so the running + // DagLoop runtime stops (aborts child sessions, releases the dag + // lease) and a restart recovery scan can never re-adopt them. + // Terminal rows are already inert; pending rows are terminalized by + // the startup orphan-pending sweep (cancel is not a valid transition + // from pending). + const workflows = yield* dag.store.listBySession(sessionID).pipe(Effect.orDie) + for (const workflow of workflows) { + // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- WorkflowRow.status is a plain string column whose values are the WorkflowStatus literals (only the projector writes it, via validated transitions). + if (isWorkflowTerminalStatus(workflow.status as never)) continue + yield* dag.cancel(workflow.id).pipe( + Effect.catchCause((cause) => + Effect.logWarning("workflow cancellation failed during session remove", { + dagID: workflow.id, + sessionID, + cause, + }), + ), + ) } + // Belt-and-braces lease sweep: drops goal registrations, wake-sweep + // registrations, and any dag registration whose workflow did not + // reach the terminalization handler above (e.g. cancel rejected). + yield* automation.purgeSession(sessionID).pipe( + Effect.catchCause((cause) => + Effect.logWarning("automation lease purge failed during session remove", { sessionID, cause }), + ), + ) yield* events.remove(sessionID) } catch (error) { yield* Effect.logError("failed to remove session", { sessionID, error }) @@ -973,6 +1031,15 @@ export const defaultLayer = layer.pipe( Layer.provide(SessionExecution.noopLayer), Layer.provide(SessionV2.defaultLayer), Layer.provide(RuntimeFlags.defaultLayer), + // GOAL-FP-01-05/-06: self-provide the remove() cleanup dependencies so the + // cleanup runs in EVERY composition of Session.defaultLayer (AppLayer + // mergeAll siblings, DagLoop, workspace, share, MoveSession, …). All three + // defaultLayers are self-contained (never requirements) and none depends on + // Session, so this cannot introduce a layer cycle; memoization shares the + // instances with the other group-1 siblings. + Layer.provide(Goal.defaultLayer), + Layer.provide(SessionAutomationLease.defaultLayer), + Layer.provide(Dag.defaultLayer), ) const cancelBackgroundJobs = Effect.fn("Session.cancelBackgroundJobs")(function* ( @@ -1117,6 +1184,14 @@ export function* listGlobal(input?: { } } -export const node = LayerNode.make(layer, [BackgroundJob.node, RuntimeFlags.node, Database.node, EventV2Bridge.node, Goal.node]) +export const node = LayerNode.make(layer, [ + BackgroundJob.node, + RuntimeFlags.node, + Database.node, + EventV2Bridge.node, + Goal.node, + SessionAutomationLease.node, + Dag.node, +]) export * as Session from "./session" diff --git a/packages/opencode/test/hook/event-wiring.test.ts b/packages/opencode/test/hook/event-wiring.test.ts index e823cc03c9..6447054472 100644 --- a/packages/opencode/test/hook/event-wiring.test.ts +++ b/packages/opencode/test/hook/event-wiring.test.ts @@ -8,6 +8,9 @@ import { BackgroundJob } from "@/background/job" import { EventV2Bridge } from "@/event-v2-bridge" import { RuntimeFlags } from "@/effect/runtime-flags" import { Session } from "@/session/session" +import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "@/session/automation-lease" +import { Dag } from "@/dag/dag" import { SessionID } from "@/session/schema" import { Permission } from "@/permission" import { Notification } from "@/notification" @@ -64,6 +67,9 @@ const sessionEnv = Layer.mergeAll( Layer.provide(SessionProjector.defaultLayer), Layer.provide(RuntimeFlags.defaultLayer), Layer.provide(BackgroundJob.defaultLayer), + Layer.provide(Goal.defaultLayer), + Layer.provide(SessionAutomationLease.defaultLayer), + Layer.provide(Dag.defaultLayer), ), Database.defaultLayer, ) diff --git a/packages/opencode/test/server/session-list.test.ts b/packages/opencode/test/server/session-list.test.ts index 213e3cdce3..cd275e3e90 100644 --- a/packages/opencode/test/server/session-list.test.ts +++ b/packages/opencode/test/server/session-list.test.ts @@ -3,6 +3,9 @@ import { Effect, Layer } from "effect" import { Database } from "@opencode-ai/core/database/database" import { SessionProjector } from "@opencode-ai/core/session/projector" import { Session as SessionNs } from "@/session/session" +import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "@/session/automation-lease" +import { Dag } from "@/dag/dag" import { disposeAllInstances, provideInstance, TestInstance } from "../fixture/fixture" import { mkdir } from "fs/promises" import path from "path" @@ -25,6 +28,9 @@ const layer = (experimentalWorkspaces: boolean) => Layer.provide(SessionProjector.defaultLayer), Layer.provide(RuntimeFlags.layer({ experimentalWorkspaces })), Layer.provide(BackgroundJob.defaultLayer), + Layer.provide(Goal.defaultLayer), + Layer.provide(SessionAutomationLease.defaultLayer), + Layer.provide(Dag.defaultLayer), ), ) const it = testEffect(layer(false)) diff --git a/packages/opencode/test/session/fork-batch.test.ts b/packages/opencode/test/session/fork-batch.test.ts index ab7225d4a9..52c7f19167 100644 --- a/packages/opencode/test/session/fork-batch.test.ts +++ b/packages/opencode/test/session/fork-batch.test.ts @@ -14,6 +14,9 @@ import * as Statement from "effect/unstable/sql/Statement" import * as Reactivity from "effect/unstable/reactivity/Reactivity" import { eq, sql } from "drizzle-orm" import { Session as SessionNs } from "@/session/session" +import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "@/session/automation-lease" +import { Dag } from "@/dag/dag" import { MessageID, PartID } from "../../src/session/schema" import { testInstanceStoreLayer } from "../fixture/fixture" import { testEffect } from "../lib/effect" @@ -148,6 +151,9 @@ const it = testEffect( Layer.provide(projectorLayer), Layer.provide(RuntimeFlags.layer({ experimentalWorkspaces: false })), Layer.provide(BackgroundJob.defaultLayer), + Layer.provide(Goal.defaultLayer), + Layer.provide(SessionAutomationLease.defaultLayer), + Layer.provide(Dag.defaultLayer), ), CrossSpawnSpawner.defaultLayer, testInstanceStoreLayer, diff --git a/packages/opencode/test/session/session-remove-cleanup.test.ts b/packages/opencode/test/session/session-remove-cleanup.test.ts new file mode 100644 index 0000000000..deea0486e6 --- /dev/null +++ b/packages/opencode/test/session/session-remove-cleanup.test.ts @@ -0,0 +1,156 @@ +import { describe, expect } from "bun:test" +import { Effect, Layer, Option } from "effect" +import { and, eq } from "drizzle-orm" +import { Database } from "@opencode-ai/core/database/database" +import { EventTable } from "@opencode-ai/core/event/sql" +import { EventV2 } from "@opencode-ai/core/event" +import { DagEvent } from "@opencode-ai/schema/dag-event" +import { GoalOutcomeTable, GoalStateTable } from "@opencode-ai/core/goal/sql" +import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" +import { Session as SessionNs } from "@/session/session" +import { SessionAutomationLease } from "@/session/automation-lease" +import { Goal } from "@/goal/goal" +import { Dag } from "@/dag/dag" +import { testEffect } from "../lib/effect" +import { testInstanceStoreLayer } from "../fixture/fixture" + +// GOAL-FP-01-05/-06/-16: `Session.remove` must be the single cleanup point for +// durable session-scoped state — goal_state + goal_outcome rows, the dag +// automation lease registrations, and owned workflows. +// +// The layer mirrors the production AppLayer (effect/app-runtime.ts) group-1 +// composition: Session, Goal and Dag are `Layer.mergeAll` SIBLINGS. mergeAll +// builds every member concurrently against the parent context only, so +// siblings cannot see each other's outputs. In production that made +// `Effect.serviceOption(Goal.Service)` inside Session's layer yield None and +// the cleanup silently no-op. This test builds the same sibling shape, so it +// fails against that wiring and passes once Session.defaultLayer self-provides +// its cleanup dependencies. +const testLayer = Layer.mergeAll( + SessionNs.defaultLayer, + Goal.defaultLayer, + Dag.defaultLayer, + SessionAutomationLease.defaultLayer, + Database.defaultLayer, + testInstanceStoreLayer, + CrossSpawnSpawner.defaultLayer, +) + +const it = testEffect(testLayer) + +describe("Session.remove goal cleanup (GOAL-FP-01-05/-16)", () => { + it.instance("deletes goal_state and goal_outcome rows for the removed session", () => + Effect.gen(function* () { + const session = yield* SessionNs.Service + const goal = yield* Goal.Service + const { db } = yield* Database.Service + + const info = yield* session.create({}) + const sessionID = info.id + // markDone terminalizes the active goal into a durable goal_outcome row. + yield* goal.set(sessionID, "first goal", 10) + yield* goal.markDone(sessionID, "done for cleanup test") + // A fresh active goal leaves a goal_state row behind at remove time. + yield* goal.set(sessionID, "second goal", 10) + + const outcomeBefore = yield* db + .select() + .from(GoalOutcomeTable) + .where(eq(GoalOutcomeTable.session_id, sessionID)) + .get() + .pipe(Effect.orDie) + expect(outcomeBefore).not.toBeNull() + + yield* session.remove(sessionID) + + const stateRow = yield* db + .select() + .from(GoalStateTable) + .where(eq(GoalStateTable.session_id, sessionID)) + .get() + .pipe(Effect.orDie) + expect(stateRow).toBeUndefined() + + const outcomeRow = yield* db + .select() + .from(GoalOutcomeTable) + .where(eq(GoalOutcomeTable.session_id, sessionID)) + .get() + .pipe(Effect.orDie) + expect(outcomeRow).toBeUndefined() + }), + ) +}) + +describe("Session.remove dag lease cleanup (GOAL-FP-01-06)", () => { + it.instance("purges dag automation lease registrations for the removed session", () => + Effect.gen(function* () { + const session = yield* SessionNs.Service + const lease = yield* SessionAutomationLease.Service + + const info = yield* session.create({}) + const sessionID = info.id + yield* lease.register(sessionID, { kind: "dag", id: "wf-lease-test" }) + expect(Option.isSome(yield* lease.claim(sessionID, { kind: "dag" }))).toBe(true) + + yield* session.remove(sessionID) + + expect(Option.isNone(yield* lease.claim(sessionID, { kind: "dag" }))).toBe(true) + }), + ) + + it.instance("cancels workflows owned by the removed session", () => + Effect.gen(function* () { + const session = yield* SessionNs.Service + const dag = yield* Dag.Service + const { db } = yield* Database.Service + + const info = yield* session.create({}) + const sessionID = info.id + const dagID = yield* dag.create({ + projectID: info.projectID, + sessionID, + title: "session-remove-cleanup-test", + config: { + name: "session-remove-cleanup-test", + nodes: [ + { + id: "n1", + name: "n1", + worker_type: "build", + depends_on: [], + required: true, + prompt_template: { inline: "do work" }, + }, + ], + }, + }) + expect((yield* dag.store.getWorkflow(dagID).pipe(Effect.orDie))?.status).toBe("running") + + yield* session.remove(sessionID) + + // The workflow READ row is FK-cascaded away with the session row, so + // the cancellation contract observable here is the durable + // dag.workflow.cancelled event — the terminalization that stops the + // running DagLoop runtime (aborting child sessions and releasing the + // dag lease) and keeps the workflow out of the restart recovery scan. + const cancelledEvent = yield* db + .select() + .from(EventTable) + .where( + and( + eq(EventTable.aggregate_id, dagID), + eq(EventTable.type, EventV2.versionedType(DagEvent.WorkflowCancelled.type, 1)), + ), + ) + .get() + .pipe(Effect.orDie) + expect(cancelledEvent).not.toBeNull() + + // Recovery scan contract (dag/runtime/loop.ts adopts only + // running/paused/stepping rows): the workflow must not be re-adoptable. + const adoptable = yield* dag.store.listByStatus("running").pipe(Effect.orDie) + expect(adoptable.map((wf) => wf.id)).not.toContain(dagID) + }), + ) +}) diff --git a/packages/opencode/test/session/session.test.ts b/packages/opencode/test/session/session.test.ts index c82f713d2b..4323703b07 100644 --- a/packages/opencode/test/session/session.test.ts +++ b/packages/opencode/test/session/session.test.ts @@ -5,6 +5,9 @@ import { EventV2 } from "@opencode-ai/core/event" import { SessionProjector } from "@opencode-ai/core/session/projector" import { Deferred, Effect, Exit, Layer } from "effect" import { Session as SessionNs } from "@/session/session" +import { Goal } from "@/goal/goal" +import { SessionAutomationLease } from "@/session/automation-lease" +import { Dag } from "@/dag/dag" import { MessageV2 } from "../../src/session/message-v2" import { MessageID, PartID, type SessionID } from "../../src/session/schema" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" @@ -25,6 +28,9 @@ const it = testEffect( Layer.provide(SessionProjector.defaultLayer), Layer.provide(RuntimeFlags.layer({ experimentalWorkspaces: false })), Layer.provide(BackgroundJob.defaultLayer), + Layer.provide(Goal.defaultLayer), + Layer.provide(SessionAutomationLease.defaultLayer), + Layer.provide(Dag.defaultLayer), ), CrossSpawnSpawner.defaultLayer, testInstanceStoreLayer, From 0f59df4006a33f75e416d246c86f065bc5bb3678 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 06:11:35 +0800 Subject: [PATCH 07/11] fix(goal): publish session deletion after automation cleanup (GOAL-FP-01-05 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P2-A (ordering inversion): Session.remove published the Deleted event BEFORE the cleanup block, contradicting the block's own comment. Inside the publish transaction the SessionProjector deletes the session row and the workflow FK cascade wipes the workflow rows, so dag.store.listBySession in the cleanup always returned [] — the cancel loop was dead code, the WorkflowCancelled event never fired, the DagLoop terminal handler never aborted running DAG child sessions, and a crash between publish and cleanup orphaned goal_state/goal_outcome rows. Fix: reordered remove() to goal purge -> workflow cancel -> lease purge -> Deleted publish -> event-log removal. The SettingsHook SessionEnd trigger now runs BEFORE the destructive steps (its documented contract is to observe the session before removal; the event-wiring test asserts trigger contents only, no Deleted-vs-hook ordering, so no consumer conflict). P2-B (vacuous assertion): the cancellation test asserted expect(cancelledEvent).not.toBeNull(), which passes vacuously — drizzle .get() returns undefined for a missing row. Changed to toBeDefined(). TDD evidence: - Red (vacuity proof): toBeDefined() on the publish-first code fails with Received: undefined — the cancel event was indeed absent (2 pass / 1 fail). - Green: after the reorder, the event is actually present (3 pass / 0 fail). - Mutation: moved the publish back before the cleanup -> 1 fail (event absent). Restored -> green. Verification: bun test test/session test/goal test/dag -> 964 pass, 0 fail; bun typecheck clean; bun lint 4852 (ratchet). Co-Authored-By: Claude --- packages/opencode/src/session/session.ts | 21 +++++++++++++------ .../session/session-remove-cleanup.test.ts | 5 ++++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/packages/opencode/src/session/session.ts b/packages/opencode/src/session/session.ts index 985a22e517..eeb747f578 100644 --- a/packages/opencode/src/session/session.ts +++ b/packages/opencode/src/session/session.ts @@ -655,9 +655,10 @@ export const layer: Layer.Layer< yield* remove(child.id) } - yield* events.publish(SessionV1.Event.Deleted, { sessionID, info: session }) // SettingsHook: SessionEnd fires before session-scoped hook state is cleared // (the trigger implementation clears seen-cache and session hooks after execution). + // It deliberately runs BEFORE the destructive steps below so the hook + // observes the session and its workflows while they still fully exist. if (settingsHook) { const seResult = yield* settingsHook .trigger({ event: "SessionEnd", reason: "delete" }, { sessionID, transcriptPath: "" }) @@ -666,11 +667,16 @@ export const layer: Layer.Layer< } // Cleanup durable automation state BEFORE the Deleted publish: the // SessionProjector deletes the session row (and FK cascades wipe the - // workflow rows) inside the Deleted publish transaction, so running - // cleanup first means a crash mid-way can only leave a live session - // with no goal/workflows (consistent, recoverable) — never orphan - // goal rows or re-adoptable workflows under a deleted session. The - // three steps live in separate aggregates (goal_state/goal_outcome, + // workflow rows) inside the Deleted publish transaction. Publishing + // first would make `dag.store.listBySession` below return [] (the + // cascade already removed the rows), turning the cancel loop into + // dead code — the WorkflowCancelled event would never fire, the + // DagLoop terminal handler would never abort running child sessions, + // and a crash between publish and cleanup would orphan goal rows. + // Running cleanup first means a crash mid-way can only leave a live + // session with no goal/workflows (consistent, recoverable) — never + // orphan goal rows or re-adoptable workflows under a deleted session. + // The three steps live in separate aggregates (goal_state/goal_outcome, // workflow events, the lease map) so no shared transaction is // available; each step is individually atomic. yield* goal.purgeSession(sessionID).pipe( @@ -706,6 +712,9 @@ export const layer: Layer.Layer< Effect.logWarning("automation lease purge failed during session remove", { sessionID, cause }), ), ) + // Session-row deletion (projector, inside this publish's transaction) + // comes LAST, after every cleanup step above. + yield* events.publish(SessionV1.Event.Deleted, { sessionID, info: session }) yield* events.remove(sessionID) } catch (error) { yield* Effect.logError("failed to remove session", { sessionID, error }) diff --git a/packages/opencode/test/session/session-remove-cleanup.test.ts b/packages/opencode/test/session/session-remove-cleanup.test.ts index deea0486e6..41e7488475 100644 --- a/packages/opencode/test/session/session-remove-cleanup.test.ts +++ b/packages/opencode/test/session/session-remove-cleanup.test.ts @@ -145,7 +145,10 @@ describe("Session.remove dag lease cleanup (GOAL-FP-01-06)", () => { ) .get() .pipe(Effect.orDie) - expect(cancelledEvent).not.toBeNull() + // P2-B: toBeNull() was vacuous — drizzle .get() returns undefined for a + // missing row and `expect(undefined).not.toBeNull()` always passes. + // toBeDefined() actually pins the durable dag.workflow.cancelled event. + expect(cancelledEvent).toBeDefined() // Recovery scan contract (dag/runtime/loop.ts adopts only // running/paused/stepping rows): the workflow must not be re-adoptable. From 9c68e5a25e8155b07aee4fdf0ca54a4fb60f6615 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 07:18:09 +0800 Subject: [PATCH 08/11] fix(goal): resume active goals after restart (GOAL-FP-01-04) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GoalLoop was purely event-driven: the idle-status subscription was the only driver, and no component emits idle for sessions that already existed at startup. An active goal that survived a crash slept until the next user interaction; with turns_used > 0 the D6 zombie guard also never fired (it runs inside afterIdle). The automation obligation — an active goal keeps advancing — was lost across restart. Add a startup scan to GoalLoop.init: - The durable snapshot is captured at instance boot inside the InstanceState builder (Goal.listActiveSessions — new accessor returning session ids whose goal_state row is "active", plus the goal revision), then the per-session triggers are forkScoped after the idle subscription is armed. Building from init's caller context would not work: evaluation fibers resolve services from their ambient runtime context, and the builder runs under the ScopedCache layer-build environment — the same context the idle subscription sees (this is why the test-injected GoalLoopJudgeLLM is visible). - The scan reuses the EXISTING evaluation path verbatim — the idle handler body was extracted into triggerEvaluation (active pre-check, fork afterIdle, registerLoopFiber, identity-scoped self-clean) and is now shared by both drivers. No new evaluation logic. - Mutual exclusion stays with the lease claim: a dag-owned session is rejected inside afterIdle exactly as on a real idle, and the GOAL-FP-01-02 blocked-claim re-trigger re-evaluates it once the dag releases (harmless + self-healing; covered by a test). - Busy sessions are gated via SessionStatus exactly like the idle path (the automation-lease re-trigger gate), plus afterIdle's post-judge status check and promptIfIdle; covered by a test. - Crash window between snapshot and trigger: terminal changes are absorbed by the active-status re-check; non-terminal changes (the scan fiber scheduled late, after the session's own idle event already evaluated the boundary) are absorbed by the expectedRevision gate — revision bumps on every durable transition, so a stale trigger cannot double-commit turns (the R1 turns-inflation harm, caught by the existing dag-release test before the gate existed). - The scan runs once, forkScoped; query and per-session failures are logged and swallowed, never fatal to init. TDD: Red — seeded a goal in the durable store before boot, published ZERO idle/status events, polled 5s for the judge/continuation: 3 tests failed with "startup scan never evaluated … (5s timeout)", goal stayed dormant. Green — added the scan; all 3 pass. Mutation — removed the scan trigger: same 3 tests go Red; restored → green. Verified: bun test test/goal test/dag test/session/automation-lease.test.ts (570 pass, 0 fail), bun typecheck (packages/opencode) clean, bun lint 4852 warnings (≤ 4852). Co-Authored-By: Claude --- packages/opencode/src/goal/goal.ts | 38 +++++ packages/opencode/src/goal/loop.ts | 162 ++++++++++++++---- packages/opencode/test/goal/e2e-loop.test.ts | 164 +++++++++++++++++++ 3 files changed, 334 insertions(+), 30 deletions(-) diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index 62ce376d01..7719c68afb 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -20,6 +20,17 @@ export type RemoveSubgoalResult = export interface Interface { readonly load: (sessionID: SessionID) => Effect.Effect + /** + * GOAL-FP-01-04: durable sessions whose goal_state row is still "active" — + * the startup-resume scan input for GoalLoop.init. Returns the session id + * plus the goal's current revision so the scan can detect goals that were + * touched after its boot-time snapshot (revision bumps on every durable + * transition). Best-effort: rows whose payload fails to decode are skipped, + * not fatal. + */ + readonly listActiveSessions: () => Effect.Effect< + ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }> + > readonly lastOutcome: (sessionID: SessionID) => Effect.Effect readonly set: (sessionID: SessionID, goal: string, maxTurns?: number) => Effect.Effect readonly pause: (sessionID: SessionID, reason: string) => Effect.Effect @@ -296,6 +307,32 @@ const serviceLayer = Layer.effect( return yield* loadState(sessionID) }) + // GOAL-FP-01-04: startup-resume scan accessor. GoalLoop is event-driven; + // after a restart nothing emits idle for sessions whose goal was active + // when the process died, so GoalLoop.init queries this durable set and + // re-triggers its existing idle evaluation path. Only "active" rows are + // returned — paused rows are user-visible and terminal rows are deleted + // by transition. Each entry carries the goal revision so the scan can + // skip goals that were touched after its boot-time snapshot. A row whose + // payload cannot be decoded is skipped defensively: the scan is + // best-effort, and the session's own idle event or /goal resume remains + // available as the recovery path. + const listActiveSessions = Effect.fn("Goal.listActiveSessions")(function* () { + const rows = yield* db.select().from(GoalStateTable).all().pipe(Effect.orDie) + const active: Array<{ sessionID: SessionID; revision: number }> = [] + for (const row of rows) { + let state: GoalState.Info + try { + state = Schema.decodeUnknownSync(GoalState.Info)(JSON.parse(row.payload)) + } catch { + continue + } + if (state.status === "active") + active.push({ sessionID: SessionID.make(row.session_id), revision: state.revision ?? 0 }) + } + return active + }) + const lastOutcome = Effect.fn("Goal.lastOutcome")(function* (sessionID: SessionID) { const row = yield* db .select() @@ -724,6 +761,7 @@ const serviceLayer = Layer.effect( return Service.of({ load, + listActiveSessions, lastOutcome, set, pause, diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index 122e824624..e7f33a58d6 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -117,38 +117,49 @@ const serviceLayer = Layer.effect( }) const state = yield* InstanceState.make( - Effect.fn("GoalLoop.state")(function* (_ctx) { - const scope = yield* Scope.Scope + Effect.fn("GoalLoop.state")(function* () { yield* events.subscribe(SessionStatus.Event.Status).pipe( Stream.filter((evt) => evt.data.status.type === "idle"), - Stream.runForEach((evt) => - Effect.gen(function* () { - const sid = evt.data.sessionID - // D4 (fiber lifecycle): do NOT fork or register a fiber for - // sessions without an active goal. Without this pre-check the - // fibers Map grows once per idle event for every session that - // ever went idle — including ones that never set a goal. afterIdle - // re-checks goal state internally too; that internal check stays - // as a TOCTOU guard (goal could be cleared between this load and - // the fork). v1.17.11: idle has no cause field; afterIdle handles - // abort detection via shouldPreempt (user message after cancel). - const goalState = yield* goal.load(sid) - if (!goalState || goalState.status !== "active") return - const fiber = yield* afterIdle(sid).pipe(Effect.ignore, Effect.forkIn(scope)) - yield* goal.registerLoopFiber(sid, fiber) - // D4 self-clean: when this afterIdle fiber completes naturally, - // remove it from the fibers Map IF it is still the registered one. - // A newer idle event may have already registered a fresh fiber - // (registerLoopFiber interrupts + overwrites the old one); - // clearLoopFiberIf's identity check avoids evicting the new fiber. - // The watcher never interrupts and completes right after its - // target, so it does not accumulate across idle events. - yield* Fiber.await(fiber).pipe( - Effect.flatMap(() => goal.clearLoopFiberIf(sid, fiber)), - Effect.ignore, - Effect.forkIn(scope), - ) - }).pipe(Effect.ignore), + // D4 (fiber lifecycle): triggerEvaluation below carries the full + // discipline (active-goal pre-check, fork, fiber registration, + // identity-scoped self-clean), shared verbatim with the + // GOAL-FP-01-04 startup scan so both drivers use one path. + Stream.runForEach((evt) => triggerEvaluation(evt.data.sessionID).pipe(Effect.ignore)), + Effect.forkScoped, + ) + // GOAL-FP-01-04: the startup resume scan. Its durable snapshot is + // captured HERE — at instance boot, inside the builder — not inside + // the forked scan fiber: a fiber delayed by scheduling could query + // AFTER this process already evaluated a goal (the session's own + // idle event), and re-evaluating that same turn boundary would + // double-commit turns (the R1 turns-inflation harm). Querying at + // boot means only goals that were active BEFORE this process started + // are ever scanned, and the per-session trigger re-checks the + // snapshot revision (bumped by every durable transition) to absorb + // the query→trigger window. + // + // The builder context matters too: service resolution inside the + // scan's evaluation fibers happens against the fiber's ambient + // runtime context, and the builder runs under the ScopedCache + // environment captured at layer build — the same context the + // idle-event subscription above sees. Forking the scan from init's + // caller context would inherit a context that lacks build-scope + // services (e.g. the test-injected GoalLoopJudgeLLM, or the + // Provider in slim callers) — the scan would silently no-op or + // crash. The per-session triggers are forked after the subscription + // is armed and into the same scope; failures are logged, never fatal + // to init. + const bootSnapshot = yield* goal.listActiveSessions().pipe( + Effect.tapError((error) => + Effect.logWarning("goal startup scan query failed", { error: String(error) }), + ), + Effect.orElseSucceed( + (): ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }> => [], + ), + ) + yield* scanForActiveGoals(bootSnapshot).pipe( + Effect.catchCause((cause) => + Effect.logWarning("goal startup scan failed", { cause: Cause.pretty(cause) }), ), Effect.forkScoped, ) @@ -406,6 +417,97 @@ const serviceLayer = Layer.effect( // stalling the goal loop. }) + // Shared evaluation trigger for BOTH the idle-event subscription above + // and the GOAL-FP-01-04 startup scan below — no second evaluation path. + // + // D4 (fiber lifecycle): do NOT fork or register a fiber for sessions + // without an active goal. Without this pre-check the fibers Map grows + // once per idle event for every session that ever went idle — including + // ones that never set a goal. afterIdle re-checks goal state internally + // too; that internal check stays as a TOCTOU guard (goal could be cleared + // between this load and the fork). v1.17.11: idle has no cause field; + // afterIdle handles abort detection via shouldPreempt (user message after + // cancel). + // + // D4 self-clean: when the afterIdle fiber completes naturally, remove it + // from the fibers Map IF it is still the registered one. A newer idle + // event may have already registered a fresh fiber (registerLoopFiber + // interrupts + overwrites the old one); clearLoopFiberIf's identity check + // avoids evicting the new fiber. The watcher never interrupts and + // completes right after its target, so it does not accumulate. + const triggerEvaluation = Effect.fnUntraced(function* ( + sessionID: SessionID, + scanExpected?: { readonly expectedRevision: number }, + ) { + const scope = yield* Scope.Scope + const goalState = yield* goal.load(sessionID) + if (!goalState || goalState.status !== "active") return + // GOAL-FP-01-04 crash window (query → trigger): the boot snapshot may + // go stale before the forked scan fiber triggers. A TERMINAL change is + // absorbed by the active-status re-check above (row deleted or paused + // → return). A NON-terminal change — the goal is still active but was + // touched by this process after the snapshot, e.g. the session's own + // idle event already evaluated this turn boundary — is absorbed here: + // revision bumps on EVERY durable transition (set, pause, resume, + // judge update, subgoal edits). Firing on a stale revision would + // double-commit turns for the same boundary (the R1 turns-inflation + // harm). The idle subscription never passes scanExpected, so this gate + // only narrows the scan. + if (scanExpected && (goalState.revision ?? 0) !== scanExpected.expectedRevision) return + const fiber = yield* afterIdle(sessionID).pipe(Effect.ignore, Effect.forkIn(scope)) + yield* goal.registerLoopFiber(sessionID, fiber) + yield* Fiber.await(fiber).pipe( + Effect.flatMap(() => goal.clearLoopFiberIf(sessionID, fiber)), + Effect.ignore, + Effect.forkIn(scope), + ) + }) + + // GOAL-FP-01-04: startup resume scan. GoalLoop is purely event-driven — + // the idle subscription above is the only driver, and no component emits + // idle for sessions that already existed at startup. An active goal that + // survived a crash therefore sleeps until the next user interaction (the + // D6 zombie guard also never fires: it runs inside afterIdle). The scan + // restores the automation obligation: after the subscription is armed, + // query the durable store for sessions with an active goal and trigger + // the EXISTING idle evaluation path for each. + // + // - Mutual exclusion: the lease claim is the sole authority. A session + // whose owner is dag is rejected by claim inside afterIdle exactly as + // on a real idle, and the blocked-claim re-trigger (GOAL-FP-01-02) + // re-evaluates it once the dag releases — the rejected trigger is + // harmless and self-healing. + // - Busy sessions: the SessionStatus gate below mirrors the + // automation-lease re-trigger gate; a session mid-turn is skipped and + // will be driven by its own turn-end idle event. At startup the status + // map is empty (get defaults to idle), so this only filters sessions + // that genuinely flipped busy between bootstrap and the scan. + // - Crash window (query → trigger): a goal may go terminal between the + // boot snapshot and triggerEvaluation. The existing guards absorb it — + // triggerEvaluation re-loads the goal and returns when it is no longer + // active, and updateAfterJudge re-checks goalID+revision under the + // lease token. The NON-terminal window (goal still active but touched + // by this process after the snapshot) is absorbed by the + // expectedRevision gate in triggerEvaluation (see there). + // - The scan runs once, forkScoped, and never fails init: per-session + // and query failures are logged and swallowed. + const scanForActiveGoals = Effect.fnUntraced(function* ( + snapshot: ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }>, + ) { + for (const { sessionID, revision } of snapshot) { + const current = yield* status.get(sessionID) + if (current.type !== "idle") continue + yield* triggerEvaluation(sessionID, { expectedRevision: revision }).pipe( + Effect.catchCause((cause) => + Effect.logWarning("goal startup scan failed for session", { + sessionID, + cause: Cause.pretty(cause), + }), + ), + ) + } + }) + const init = Effect.fn("GoalLoop.init")(function* () { yield* InstanceState.get(state) }) diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index 75c37f68af..92d0f97a83 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -910,3 +910,167 @@ describe("GoalLoop — continuation interrupted → no pause, goal stays active }), ) }) + +// GOAL-FP-01-04: GoalLoop is purely event-driven — the idle subscription is +// the only driver, and nothing re-emits idle for sessions that already +// existed at startup. An active goal that survived a crash therefore sleeps +// until the next user interaction (and the D6 zombie guard cannot fire +// without an idle event). The startup scan in GoalLoop.init must resume it: +// seed the goal in the durable store BEFORE boot, publish ZERO idle/status +// events, and the goal must still get evaluated (judge + continuation). +// +// The shared scanLayer mirrors the e2e harness: Goal / SessionStatus / +// EventV2Bridge / the lease are real; Session / SessionPrompt / Provider and +// the judge LLM are mocked. provideMerge exposes SessionStatus and the lease +// to the test body so pre-boot setup (busy / dag owner) shares the SAME +// instances the scan reads. +describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04)", () => { + let judgeCalls = 0 + let continuationCalls = 0 + const reset = () => { + judgeCalls = 0 + continuationCalls = 0 + } + + // Layer.mock (not Layer.succeed(… as never)) — the R1 describe above shows + // the warning-free pattern; `as never` would add lint-ratchet warnings. + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + }) + const promptMock = Layer.mock(SessionPrompt.Service, { + prompt: () => Effect.die("the direct prompt path is not exercised in this scenario"), + promptIfIdle: () => + Effect.sync(() => { + continuationCalls += 1 + return Option.none() + }), + }) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => + Effect.sync(() => { + judgeCalls += 1 + return JSON.stringify({ done: false, reason: "more steps needed" }) + }), + }), + ) + const scanLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.mock(Provider.Service, {})), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + // provideMerge (not provide): the test body seeds pre-boot busy / dag + // owner through SessionStatus and the lease, and the scan must read the + // SAME instances (see the arbitration describe above). + Layer.provideMerge(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + ) + const it = testEffect(scanLayer) + + it.instance("a goal active before boot is evaluated with ZERO idle events", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + // Seed the durable store BEFORE GoalLoop boots — models a goal_state + // row surviving a crash-restart. No idle/status event is published. + const sid = SessionID.descending() + yield* goal.set(sid, "ship the feature", 10) + yield* loop.init() + + // The only driver available is the startup scan: assert the full + // claim+judge+continuation flow ran within the poll window. + yield* pollWithTimeout( + Effect.sync(() => (continuationCalls >= 1 ? true : undefined)), + "startup scan never evaluated the pre-boot active goal", + "5 seconds", + ) + expect(judgeCalls).toBe(1) + const g = yield* goal.load(sid) + expect(g?.status).toBe("active") + expect(Number(g?.turns_used)).toBe(1) + }), + ) + + it.instance("a dag-owned session yields to the startup scan and resumes when the dag releases", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const automation = yield* SessionAutomationLease.Service + + // Session B is a plain active goal — its evaluation is the positive + // signal that the scan RAN (judgeCalls 0→1). Session A is dag-owned: + // the scan's claim must be rejected exactly like a real idle, so A + // contributes no judge call and stays untouched. + const sidA = SessionID.descending() + const sidB = SessionID.descending() + yield* goal.set(sidA, "goal owned by dag", 10) + yield* goal.set(sidB, "goal evaluated by scan", 10) + yield* automation.register(sidA, { kind: "dag", id: "dag-executor" }) + yield* loop.init() + + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "startup scan never evaluated the unblocked goal", + "5 seconds", + ) + const a = yield* goal.load(sidA) + expect(a?.status).toBe("active") + expect(Number(a?.turns_used)).toBe(0) // claim rejected — trigger harmless + expect(judgeCalls).toBe(1) // only B was evaluated + + // GOAL-FP-01-02: releasing the dag re-triggers the blocked goal + // evaluation through the idle mechanism — no manual idle event needed. + yield* automation.unregister(sidA, { kind: "dag", id: "dag-executor" }) + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 2 ? true : undefined)), + "goal did not resume after the dag released the session", + "5 seconds", + ) + const a2 = yield* goal.load(sidA) + expect(Number(a2?.turns_used)).toBe(1) + }), + ) + + it.instance("a busy session is not force-evaluated by the scan; its own idle event drives it", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const status = yield* SessionStatus.Service + + const sidA = SessionID.descending() + const sidB = SessionID.descending() + yield* goal.set(sidA, "goal on busy session", 10) + yield* goal.set(sidB, "goal on idle session", 10) + yield* status.set(sidA, { type: "busy" }) + yield* loop.init() + + // B's evaluation proves the scan ran; A must have been skipped by the + // SessionStatus gate — no force-evaluation mid-turn. + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "startup scan never evaluated the idle-session goal", + "5 seconds", + ) + expect(judgeCalls).toBe(1) + const a = yield* goal.load(sidA) + expect(a?.status).toBe("active") + expect(Number(a?.turns_used)).toBe(0) + + // When the busy session finishes, its own idle event drives the goal. + yield* status.set(sidA, { type: "idle" }) + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 2 ? true : undefined)), + "busy session's goal was not driven by its own idle event", + "5 seconds", + ) + const a2 = yield* goal.load(sidA) + expect(Number(a2?.turns_used)).toBe(1) + }), + ) +}) From ead30ca959b1abe1a391bc45561d43ec25fd9a66 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 08:17:07 +0800 Subject: [PATCH 09/11] fix(goal): scope the startup scan to the instance directory and harden failure handling (GOAL-FP-01-04 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Domain review of the GOAL-FP-01-04 startup scan: one P1 (D-1) and three P2s (D-2/D-3/D-4), plus one registered residual. D-1 (P1): the scan was not scoped to the instance. goal_state has no directory column and the Database is the shared global opencode.db, so any instance boot evaluated/committed/paused/drove the active goals of EVERY project — judge budget burn, pause prompts injected into foreign sessions, cross-project agent turns with the wrong cwd. Fix: the scan query (Goal.listActiveSessions) now inner-joins goal_state.session_id → session.id and filters session.directory = the instance directory. The session table (core session sql, directory column) is the single directory authority; no schema change was needed. The directory is resolved in GoalLoop.init from the caller context (InstanceRef, which instance boot provides) and handed to the instance-state builder via a ref set before the first InstanceState.get — the builder runs under the ScopedCache layer-build environment, which does NOT include InstanceRef in production (reading InstanceState.directory there would die). D-2 (P2): "query failures logged and swallowed, never fatal" was false. tapError/orElseSucceed only handle Cause.Fail, so a boot-time DB defect killed the state builder, closing the ScopedCache entry scope and taking the idle subscription down with it until restart. Fix: the query is wrapped in Effect.catchCause, which in this effect version catches Fail AND Defect (there is no catchAllCause) — any failure degrades to no-scan + a log. Per-session triggers keep their catchCause guards. D-3 (P2): undecodable goal_state rows were skipped silently. The skip now logs a warning with the session id and the decode error, so the dormancy is visible (asserted via TestConsole). D-4 (P2): the boot-snapshot revision gate was not airtight: if an idle evaluation committed between the scan's gate load and its afterIdle entry load, the scan's evaluation would commit again (matchesExpected passes on the re-loaded revision) — double-commit of the same boundary. Fix: replaced the snapshot-revision comparison with a per-process evaluatedRevisions map — afterIdle records the committed revision on every successful updateAfterJudge; the scan path (triggerEvaluation gate + afterIdle entry gate, flagged by scanResume) skips when the recorded revision equals the current revision. The idle path never consults the gate, so it keeps re-evaluating the same revision across new turn boundaries. This also fixes the D-5 cross-process false negative: a revision bumped by a touch-without-evaluation (incl. by another process before this boot) no longer suppresses the resume. The map is overwritten by every commit and deleted at the same terminal points where afterIdle unregisters the goal automation. D-4 testability: the A-commits/B-scan interleaving is not deterministically constructible through the public seam — the scan's gate load and afterIdle's entry load are adjacent in the same fiber with no injectable pause between them, and the fiber map's interrupt-on-replace kills any earlier evaluation a test could park. The committed D-4 test instead deterministically parks the scan's evaluation at the judge (Deferred, not sleep), races an idle evaluation into the same boundary, and asserts exactly one commit — the tightest public-seam construction of the race. The record gate's exact interleaving is argued above rather than exercised. Registered residual (not fixable in-process): the cross-process mirror of D-4 — two live GoalLoop instances in the same process group could both evaluate the same boundary (each has its own record map). Trigger conditions: two instances booted against the same session/goal_state simultaneously. Rare; would need a cross-instance lease or a directory-level claim, out of scope for this slice. TDD: D-1 Red — a foreign-directory goal got evaluated (foreignJudgeCalls 1, turns 1, expected 0); D-2 Red — dropping goal_state killed init (test body died); D-3 Red — no skip log captured. D-4 regression guard green on HEAD. Green after the fix: all four pass. Mutations: removed the directory filter → D-1 Red; restored tapError/orElseSucceed + orDie (pre-fix defect channel) → D-2 Red. Restored → green. Verified: bun test test/goal test/dag test/session/automation-lease.test.ts (574 pass, 0 fail, 3x stable e2e-loop reruns), bun typecheck (packages/opencode) clean, bun lint 4852 warnings (≤ 4852). Co-Authored-By: Claude --- packages/opencode/src/goal/goal.ts | 58 +++-- packages/opencode/src/goal/loop.ts | 160 +++++++----- packages/opencode/test/goal/e2e-loop.test.ts | 246 ++++++++++++++++++- 3 files changed, 376 insertions(+), 88 deletions(-) diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index 7719c68afb..b3a0aadcd4 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -1,7 +1,8 @@ export * as Goal from "./goal" import { Effect, Layer, Context, Schema, Fiber } from "effect" -import { desc, eq } from "drizzle-orm" +import { desc, eq, sql } from "drizzle-orm" +import { SessionTable } from "@opencode-ai/core/session/sql" import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { Database } from "@opencode-ai/core/database/database" import { EventV2Bridge } from "@/event-v2-bridge" @@ -21,16 +22,14 @@ export type RemoveSubgoalResult = export interface Interface { readonly load: (sessionID: SessionID) => Effect.Effect /** - * GOAL-FP-01-04: durable sessions whose goal_state row is still "active" — - * the startup-resume scan input for GoalLoop.init. Returns the session id - * plus the goal's current revision so the scan can detect goals that were - * touched after its boot-time snapshot (revision bumps on every durable - * transition). Best-effort: rows whose payload fails to decode are skipped, - * not fatal. + * GOAL-FP-01-04: durable session ids whose goal_state row is still "active" + * AND whose session belongs to `directory` — the startup-resume scan input + * for GoalLoop.init. The session table is the directory authority + * (goal_state has no directory column), so one instance's scan can never + * drive another instance's goals. Best-effort: rows whose payload fails to + * decode are skipped with a logged warning, not fatal. */ - readonly listActiveSessions: () => Effect.Effect< - ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }> - > + readonly listActiveSessions: (directory: string) => Effect.Effect, Error> readonly lastOutcome: (sessionID: SessionID) => Effect.Effect readonly set: (sessionID: SessionID, goal: string, maxTurns?: number) => Effect.Effect readonly pause: (sessionID: SessionID, reason: string) => Effect.Effect @@ -312,23 +311,40 @@ const serviceLayer = Layer.effect( // when the process died, so GoalLoop.init queries this durable set and // re-triggers its existing idle evaluation path. Only "active" rows are // returned — paused rows are user-visible and terminal rows are deleted - // by transition. Each entry carries the goal revision so the scan can - // skip goals that were touched after its boot-time snapshot. A row whose - // payload cannot be decoded is skipped defensively: the scan is - // best-effort, and the session's own idle event or /goal resume remains - // available as the recovery path. - const listActiveSessions = Effect.fn("Goal.listActiveSessions")(function* () { - const rows = yield* db.select().from(GoalStateTable).all().pipe(Effect.orDie) - const active: Array<{ sessionID: SessionID; revision: number }> = [] + // by transition. + // + // D-1: scoped to the instance's own directory. goal_state has no + // directory column; the session table is the directory authority, so the + // query joins goal_state.session_id → session.id and filters on + // session.directory — the scan can never evaluate, commit, pause, or + // drive another instance's sessions. Goal rows whose session row is + // missing are dropped by the inner join (invisible to the scan, same as + // other instances' rows). + // + // D-3: a row whose payload cannot be decoded is skipped defensively (the + // scan is best-effort; the session's own idle event or /goal resume + // remains available) but the skip is LOGGED with the session id and the + // decode error — a silently-dormant goal is not diagnosable. + const listActiveSessions = Effect.fn("Goal.listActiveSessions")(function* (directory: string) { + const rows = yield* db + .select({ session_id: GoalStateTable.session_id, payload: GoalStateTable.payload }) + .from(GoalStateTable) + .innerJoin(SessionTable, sql`${GoalStateTable.session_id} = ${SessionTable.id}`) + .where(eq(SessionTable.directory, directory)) + .all() + const active: SessionID[] = [] for (const row of rows) { let state: GoalState.Info try { state = Schema.decodeUnknownSync(GoalState.Info)(JSON.parse(row.payload)) - } catch { + } catch (error) { + yield* Effect.logWarning( + `goal startup scan skipped undecodable goal_state row for ${row.session_id}`, + { error: String(error) }, + ) continue } - if (state.status === "active") - active.push({ sessionID: SessionID.make(row.session_id), revision: state.revision ?? 0 }) + if (state.status === "active") active.push(SessionID.make(row.session_id)) } return active }) diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index e7f33a58d6..2306354bcf 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -116,6 +116,16 @@ const serviceLayer = Layer.effect( return paused }) + // GOAL-FP-01-04 (D-1): the instance directory the scan scopes to. The + // state builder runs under the ScopedCache layer-build environment, + // which does NOT include InstanceRef in production — resolving + // InstanceState.directory inside the builder would die with "InstanceRef + // not provided". init resolves it from its CALLER's context (instance + // boot provides InstanceRef) and hands it to the builder through this + // ref, set BEFORE the first InstanceState.get so the builder always + // reads a populated value. + const scanDirectoryRef: { current: string } = { current: "" } + const state = yield* InstanceState.make( Effect.fn("GoalLoop.state")(function* () { yield* events.subscribe(SessionStatus.Event.Status).pipe( @@ -127,37 +137,35 @@ const serviceLayer = Layer.effect( Stream.runForEach((evt) => triggerEvaluation(evt.data.sessionID).pipe(Effect.ignore)), Effect.forkScoped, ) - // GOAL-FP-01-04: the startup resume scan. Its durable snapshot is - // captured HERE — at instance boot, inside the builder — not inside - // the forked scan fiber: a fiber delayed by scheduling could query - // AFTER this process already evaluated a goal (the session's own - // idle event), and re-evaluating that same turn boundary would - // double-commit turns (the R1 turns-inflation harm). Querying at - // boot means only goals that were active BEFORE this process started - // are ever scanned, and the per-session trigger re-checks the - // snapshot revision (bumped by every durable transition) to absorb - // the query→trigger window. + // GOAL-FP-01-04: the startup resume scan. The durable snapshot is + // captured HERE — at instance boot (the builder runs at the first + // InstanceState.get, i.e. init), awaited — not inside the forked + // scan fiber: a fiber delayed by scheduling could query AFTER this + // process already evaluated a goal (the session's own idle event), + // and re-evaluating that same turn boundary would double-commit + // turns (the R1 turns-inflation harm). Querying at boot means only + // goals that were active BEFORE this process started are ever + // scanned. The builder context is also the layer-build context — the + // one the idle subscription above sees — so the scan's evaluation + // fibers resolve the same services. // - // The builder context matters too: service resolution inside the - // scan's evaluation fibers happens against the fiber's ambient - // runtime context, and the builder runs under the ScopedCache - // environment captured at layer build — the same context the - // idle-event subscription above sees. Forking the scan from init's - // caller context would inherit a context that lacks build-scope - // services (e.g. the test-injected GoalLoopJudgeLLM, or the - // Provider in slim callers) — the scan would silently no-op or - // crash. The per-session triggers are forked after the subscription - // is armed and into the same scope; failures are logged, never fatal - // to init. - const bootSnapshot = yield* goal.listActiveSessions().pipe( - Effect.tapError((error) => - Effect.logWarning("goal startup scan query failed", { error: String(error) }), - ), - Effect.orElseSucceed( - (): ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }> => [], - ), + // D-2: catchCause (unlike tapError/orElseSucceed) catches Fail AND + // Defect, so ANY query failure degrades to no-scan + a log and can + // never kill the builder — which would close the ScopedCache entry + // scope and take the idle subscription down with it. + const snapshot = yield* goal.listActiveSessions(scanDirectoryRef.current).pipe( + Effect.catchCause((cause) => { + const empty: ReadonlyArray = [] + return Effect.logWarning("goal startup scan query failed", { + directory: scanDirectoryRef.current, + cause: Cause.pretty(cause), + }).pipe(Effect.as(empty)) + }), ) - yield* scanForActiveGoals(bootSnapshot).pipe( + // The per-session triggers are forked after the subscription is + // armed and into the same scope; failures are logged, never fatal to + // init. + yield* scanForActiveGoals(snapshot).pipe( Effect.catchCause((cause) => Effect.logWarning("goal startup scan failed", { cause: Cause.pretty(cause) }), ), @@ -167,9 +175,27 @@ const serviceLayer = Layer.effect( }), ) - const afterIdle = Effect.fn("GoalLoop.afterIdle")(function* (sessionID: SessionID) { + // D-4 (GOAL-FP-01-04 follow-up): per-process record of which goal + // revision this process already evaluated. Written by afterIdle on every + // successful updateAfterJudge commit; consulted ONLY by the startup-scan + // path (scanResume) — the idle path must keep re-evaluating the same + // revision across new turn boundaries, so the gate never applies to it. + // Lifecycle mirrors the fibers map: overwritten by every commit, deleted + // at the same terminal points where afterIdle unregisters the goal + // automation. + const evaluatedRevisions = new Map() + + const afterIdle = Effect.fn("GoalLoop.afterIdle")(function* (sessionID: SessionID, scanResume?: boolean) { const goalState = yield* goal.load(sessionID) if (!goalState || goalState.status !== "active") return + // D-4 entry gate (scan path only): the boot snapshot may have gone + // stale between triggerEvaluation's load and this entry load — an idle + // evaluation could have committed a new revision in between, and this + // scan evaluation would then double-commit the SAME boundary + // (matchesExpected passes because this entry load already sees the + // newer revision). If this process already evaluated the CURRENT + // revision, the scan trigger is stale — skip. + if (scanResume && evaluatedRevisions.get(sessionID) === (goalState.revision ?? 0)) return const goalOwner = { kind: "goal" as const, id: goalState.goal_id ?? "legacy" } yield* automation.register(sessionID, goalOwner) const observedLease = Option.getOrUndefined(yield* automation.claim(sessionID, goalOwner)) @@ -279,8 +305,14 @@ const serviceLayer = Layer.effect( ) if (!updateResult) return + // D-4: record the committed revision as evaluated-by-this-process + // (every verdict — continue, done, blocked — is a completed + // evaluation of the pre-commit state). + evaluatedRevisions.set(sessionID, updateResult.state.revision ?? 0) + if (!updateResult.shouldContinue) { yield* automation.unregister(sessionID, goalOwner) + evaluatedRevisions.delete(sessionID) if (verdict.verdict === "done") { yield* promptSvc.prompt({ sessionID, @@ -404,8 +436,10 @@ const serviceLayer = Layer.effect( ), ) const afterDispatch = yield* goal.load(sessionID) - if (!afterDispatch || afterDispatch.status !== "active") + if (!afterDispatch || afterDispatch.status !== "active") { yield* automation.unregister(sessionID, goalOwner) + evaluatedRevisions.delete(sessionID) + } // NOTE: We deliberately DO NOT call goal.clearLoopFiber here. The // promptSvc.prompt above triggers a fresh agent loop, which when it @@ -435,26 +469,23 @@ const serviceLayer = Layer.effect( // interrupts + overwrites the old one); clearLoopFiberIf's identity check // avoids evicting the new fiber. The watcher never interrupts and // completes right after its target, so it does not accumulate. - const triggerEvaluation = Effect.fnUntraced(function* ( - sessionID: SessionID, - scanExpected?: { readonly expectedRevision: number }, - ) { + const triggerEvaluation = Effect.fnUntraced(function* (sessionID: SessionID, scanResume?: boolean) { const scope = yield* Scope.Scope const goalState = yield* goal.load(sessionID) if (!goalState || goalState.status !== "active") return - // GOAL-FP-01-04 crash window (query → trigger): the boot snapshot may - // go stale before the forked scan fiber triggers. A TERMINAL change is - // absorbed by the active-status re-check above (row deleted or paused - // → return). A NON-terminal change — the goal is still active but was - // touched by this process after the snapshot, e.g. the session's own - // idle event already evaluated this turn boundary — is absorbed here: - // revision bumps on EVERY durable transition (set, pause, resume, - // judge update, subgoal edits). Firing on a stale revision would - // double-commit turns for the same boundary (the R1 turns-inflation - // harm). The idle subscription never passes scanExpected, so this gate - // only narrows the scan. - if (scanExpected && (goalState.revision ?? 0) !== scanExpected.expectedRevision) return - const fiber = yield* afterIdle(sessionID).pipe(Effect.ignore, Effect.forkIn(scope)) + // D-4 gate (scan path only): skip when this process already evaluated + // the CURRENT revision — the boot snapshot went stale after a + // legitimate evaluation (e.g. the session's own idle event ran before + // the scan fiber). This replaces the boot-snapshot revision + // comparison: unlike that gate, a revision bumped by a non-evaluation + // touch (pause/resume/subgoal edit — including one made by another + // process before this boot) does NOT suppress the resume, which is + // correct — the goal still awaits its evaluation. The idle + // subscription never passes scanResume, so this only narrows the scan. + // afterIdle re-checks at its own entry load (see there) to close the + // window between this load and the fork. + if (scanResume && evaluatedRevisions.get(sessionID) === (goalState.revision ?? 0)) return + const fiber = yield* afterIdle(sessionID, scanResume).pipe(Effect.ignore, Effect.forkIn(scope)) yield* goal.registerLoopFiber(sessionID, fiber) yield* Fiber.await(fiber).pipe( Effect.flatMap(() => goal.clearLoopFiberIf(sessionID, fiber)), @@ -468,9 +499,8 @@ const serviceLayer = Layer.effect( // idle for sessions that already existed at startup. An active goal that // survived a crash therefore sleeps until the next user interaction (the // D6 zombie guard also never fires: it runs inside afterIdle). The scan - // restores the automation obligation: after the subscription is armed, - // query the durable store for sessions with an active goal and trigger - // the EXISTING idle evaluation path for each. + // restores the automation obligation: for each session in the boot + // snapshot, trigger the EXISTING idle evaluation path. // // - Mutual exclusion: the lease claim is the sole authority. A session // whose owner is dag is rejected by claim inside afterIdle exactly as @@ -482,22 +512,17 @@ const serviceLayer = Layer.effect( // will be driven by its own turn-end idle event. At startup the status // map is empty (get defaults to idle), so this only filters sessions // that genuinely flipped busy between bootstrap and the scan. - // - Crash window (query → trigger): a goal may go terminal between the - // boot snapshot and triggerEvaluation. The existing guards absorb it — - // triggerEvaluation re-loads the goal and returns when it is no longer - // active, and updateAfterJudge re-checks goalID+revision under the - // lease token. The NON-terminal window (goal still active but touched - // by this process after the snapshot) is absorbed by the - // expectedRevision gate in triggerEvaluation (see there). - // - The scan runs once, forkScoped, and never fails init: per-session - // and query failures are logged and swallowed. - const scanForActiveGoals = Effect.fnUntraced(function* ( - snapshot: ReadonlyArray<{ readonly sessionID: SessionID; readonly revision: number }>, - ) { - for (const { sessionID, revision } of snapshot) { + // - Crash window (query → trigger): terminal changes are absorbed by the + // active-status re-check; non-terminal changes (already evaluated in + // this process) by the D-4 record gate in triggerEvaluation/afterIdle. + // - Failures: per-session catchCause (covers Fail AND Defect) so one bad + // session never kills the rest of the scan; the whole scan is forked, + // so a failure can never kill init. + const scanForActiveGoals = Effect.fnUntraced(function* (snapshot: ReadonlyArray) { + for (const sessionID of snapshot) { const current = yield* status.get(sessionID) if (current.type !== "idle") continue - yield* triggerEvaluation(sessionID, { expectedRevision: revision }).pipe( + yield* triggerEvaluation(sessionID, true).pipe( Effect.catchCause((cause) => Effect.logWarning("goal startup scan failed for session", { sessionID, @@ -509,6 +534,11 @@ const serviceLayer = Layer.effect( }) const init = Effect.fn("GoalLoop.init")(function* () { + // Resolve the scan's directory scope BEFORE the first state get — the + // builder (which runs inside that get) reads it from the ref. This + // context carries InstanceRef (instance boot provides it); the + // builder's does not. + scanDirectoryRef.current = yield* InstanceState.directory yield* InstanceState.get(state) }) diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index 92d0f97a83..0b24da1b51 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -10,6 +10,14 @@ import { SessionPrompt } from "@/session/prompt" import { Provider } from "@/provider/provider" import { SessionID } from "@/session/schema" import { SessionAutomationLease } from "@/session/automation-lease" +import { Database } from "@opencode-ai/core/database/database" +import { SessionTable } from "@opencode-ai/core/session/sql" +import { GoalStateTable } from "@opencode-ai/core/goal/sql" +import { ProjectTable } from "@opencode-ai/core/project/sql" +import { ProjectSchema } from "@opencode-ai/core/project/schema" +import { AbsolutePath } from "@opencode-ai/core/schema" +import { TestInstance } from "../fixture/fixture" +import { logLines } from "effect/testing/TestConsole" import { testEffect, pollWithTimeout } from "../lib/effect" // P2b: full-cycle Goal regression (D5). Drives set → idle → judge(continue) → @@ -962,22 +970,48 @@ describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04 Layer.provide(judgeMock), Layer.provideMerge(Goal.defaultLayer), // provideMerge (not provide): the test body seeds pre-boot busy / dag - // owner through SessionStatus and the lease, and the scan must read the - // SAME instances (see the arbitration describe above). + // owner through SessionStatus, the lease, and the DB, and the scan must + // read the SAME instances (see the arbitration describe above). Layer.provideMerge(SessionStatus.defaultLayer), Layer.provideMerge(EventV2Bridge.defaultLayer), Layer.provideMerge(SessionAutomationLease.defaultLayer), + Layer.provideMerge(Database.defaultLayer), ) const it = testEffect(scanLayer) + // Seeds a durable session row (+ its project row, FK-required) so the + // D-1 directory join can attribute the goal_state row to an instance. + const seedSessionRow = (sessionID: SessionID, directory: string) => + Effect.gen(function* () { + const { db } = yield* Database.Service + const projectID = ProjectSchema.ID.make(Bun.randomUUIDv7()) + yield* db.insert(ProjectTable).values({ + id: projectID, + worktree: AbsolutePath.make(directory), + sandboxes: [AbsolutePath.make(directory)], + }) + yield* db.insert(SessionTable).values({ + id: sessionID, + project_id: projectID, + slug: "test-session", + directory, + title: "test session", + version: "1", + time_created: Date.now(), + time_updated: Date.now(), + }) + }) + it.instance("a goal active before boot is evaluated with ZERO idle events", () => Effect.gen(function* () { reset() const loop = yield* GoalLoop.Service const goal = yield* Goal.Service + const directory = (yield* TestInstance).directory // Seed the durable store BEFORE GoalLoop boots — models a goal_state // row surviving a crash-restart. No idle/status event is published. const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) yield* goal.set(sid, "ship the feature", 10) yield* loop.init() @@ -1001,6 +1035,7 @@ describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04 const loop = yield* GoalLoop.Service const goal = yield* Goal.Service const automation = yield* SessionAutomationLease.Service + const directory = (yield* TestInstance).directory // Session B is a plain active goal — its evaluation is the positive // signal that the scan RAN (judgeCalls 0→1). Session A is dag-owned: @@ -1008,6 +1043,8 @@ describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04 // contributes no judge call and stays untouched. const sidA = SessionID.descending() const sidB = SessionID.descending() + yield* seedSessionRow(sidA, directory) + yield* seedSessionRow(sidB, directory) yield* goal.set(sidA, "goal owned by dag", 10) yield* goal.set(sidB, "goal evaluated by scan", 10) yield* automation.register(sidA, { kind: "dag", id: "dag-executor" }) @@ -1042,9 +1079,12 @@ describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04 const loop = yield* GoalLoop.Service const goal = yield* Goal.Service const status = yield* SessionStatus.Service + const directory = (yield* TestInstance).directory const sidA = SessionID.descending() const sidB = SessionID.descending() + yield* seedSessionRow(sidA, directory) + yield* seedSessionRow(sidB, directory) yield* goal.set(sidA, "goal on busy session", 10) yield* goal.set(sidB, "goal on idle session", 10) yield* status.set(sidA, { type: "busy" }) @@ -1074,3 +1114,205 @@ describe("GoalLoop — startup scan resumes pre-boot active goals (GOAL-FP-01-04 }), ) }) + +// GOAL-FP-01-04 follow-up (D-1..D-4): scoping and hardening of the startup +// scan. D-1: the scan must be scoped to the instance's own directory (join +// goal_state → session.directory). D-2: a defective scan query must degrade +// to no-scan + a log, never kill init or the idle path. D-3: undecodable +// rows must be skipped with a visible log, not silently. D-4: a scan +// evaluation racing an idle evaluation must commit exactly once. +describe("GoalLoop — startup scan scoping and hardening (GOAL-FP-01-04 follow-up)", () => { + let judgeCalls = 0 + let foreignJudgeCalls = 0 + let parkFirstJudge = false + let judgeRelease = Deferred.makeUnsafe() + const reset = () => { + judgeCalls = 0 + foreignJudgeCalls = 0 + parkFirstJudge = false + judgeRelease = Deferred.makeUnsafe() + } + + // The judge LLM prompt carries the goal text verbatim, so the mock can + // attribute calls to the foreign-directory goal via a marker string. + const FOREIGN_GOAL = "FOREIGN-MARKER ship the feature" + + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + }) + const promptMock = Layer.mock(SessionPrompt.Service, { + prompt: () => Effect.die("the direct prompt path is not exercised in this scenario"), + promptIfIdle: () => Effect.sync(() => Option.none()), + }) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: (opts: { user: string }) => + Effect.gen(function* () { + judgeCalls += 1 + if (opts.user.includes("FOREIGN-MARKER")) foreignJudgeCalls += 1 + // D-4 hook: park the first judge call so the scan and idle + // evaluations race deterministically. + if (parkFirstJudge && judgeCalls === 1) yield* Deferred.await(judgeRelease) + return JSON.stringify({ done: false, reason: "more steps needed" }) + }), + }), + ) + const hardenLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.mock(Provider.Service, {})), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + Layer.provideMerge(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + Layer.provideMerge(Database.defaultLayer), + ) + const it = testEffect(hardenLayer) + + const seedSessionRow = (sessionID: SessionID, directory: string) => + Effect.gen(function* () { + const { db } = yield* Database.Service + const projectID = ProjectSchema.ID.make(Bun.randomUUIDv7()) + yield* db.insert(ProjectTable).values({ + id: projectID, + worktree: AbsolutePath.make(directory), + sandboxes: [AbsolutePath.make(directory)], + }) + yield* db.insert(SessionTable).values({ + id: sessionID, + project_id: projectID, + slug: "test-session", + directory, + title: "test session", + version: "1", + time_created: Date.now(), + time_updated: Date.now(), + }) + }) + + it.instance("D-1: a foreign-directory active goal is not evaluated; the same-directory goal is", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const directory = (yield* TestInstance).directory + + const sidForeign = SessionID.descending() + const sidSame = SessionID.descending() + yield* seedSessionRow(sidForeign, directory + "-foreign") + yield* seedSessionRow(sidSame, directory) + yield* goal.set(sidForeign, FOREIGN_GOAL, 10) + yield* goal.set(sidSame, "ship the feature", 10) + yield* loop.init() + + // Positive control: the same-directory goal IS evaluated by the scan. + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "startup scan never evaluated the same-directory goal", + "5 seconds", + ) + // Let any (buggy) foreign evaluation settle before asserting. + yield* Effect.sleep("300 millis") + const same = yield* goal.load(sidSame) + const foreign = yield* goal.load(sidForeign) + expect(Number(same?.turns_used)).toBe(1) + expect(foreignJudgeCalls).toBe(0) + expect(foreign?.status).toBe("active") + expect(Number(foreign?.turns_used)).toBe(0) + }), + ) + + it.instance("D-2: a defective scan query never kills init; the idle path still works", () => + Effect.gen(function* () { + reset() + const { db } = yield* Database.Service + // Corrupt DB state: the scan query hits a missing table. + yield* db.run("DROP TABLE goal_state") + const loop = yield* GoalLoop.Service + yield* loop.init() // must not die + yield* db.run( + "CREATE TABLE goal_state (session_id TEXT PRIMARY KEY NOT NULL, payload TEXT NOT NULL, updated_at INTEGER NOT NULL)", + ) + const goal = yield* Goal.Service + const events = yield* EventV2Bridge.Service + const directory = (yield* TestInstance).directory + const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) + yield* goal.set(sid, "ship the feature", 10) + yield* Effect.yieldNow + + // The idle subscription (armed before the scan) must still drive the + // goal — the defective scan degraded, it did not kill the loop. + yield* events.publish(SessionStatus.Event.Status, { sessionID: sid, status: { type: "idle" } }) + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "idle path dead after a defective scan", + "5 seconds", + ) + expect(Number((yield* goal.load(sid))?.turns_used)).toBe(1) + }), + ) + + it.instance("D-3: an undecodable goal_state row is skipped with a visible warning log", () => + Effect.gen(function* () { + reset() + const { db } = yield* Database.Service + const directory = (yield* TestInstance).directory + const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) + yield* db + .insert(GoalStateTable) + .values({ session_id: sid, payload: "{corrupt", updated_at: Date.now() }) + const loop = yield* GoalLoop.Service + yield* loop.init() + const logs = JSON.stringify(yield* logLines) + expect(logs).toContain("goal startup scan skipped undecodable goal_state row") + expect(logs).toContain(String(sid)) + }), + ) + + it.instance("D-4: a scan evaluation racing an idle evaluation commits exactly once", () => + Effect.gen(function* () { + reset() + parkFirstJudge = true + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const events = yield* EventV2Bridge.Service + const directory = (yield* TestInstance).directory + const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) + yield* goal.set(sid, "ship the feature", 10) + yield* loop.init() + + // Wait for the scan's evaluation to reach the judge, where it parks. + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "the scan's evaluation never reached the judge", + "5 seconds", + ) + // Now a second evaluation races it: the idle event drives an + // independent trigger for the SAME turn boundary. The fiber map's + // interrupt-on-replace kills the parked scan evaluation, and exactly + // ONE commit for the boundary must land. + yield* events.publish(SessionStatus.Event.Status, { sessionID: sid, status: { type: "idle" } }) + + yield* pollWithTimeout( + Effect.gen(function* () { + const g = yield* goal.load(sid) + return Number(g?.turns_used) >= 1 ? true : undefined + }), + "no racing evaluation committed", + "5 seconds", + ) + yield* Effect.sleep("50 millis") + yield* Deferred.succeed(judgeRelease, undefined) + const g = yield* goal.load(sid) + expect(g?.status).toBe("active") + // The single-writer commit point (matchesExpected + record gate) must + // yield exactly ONE commit for the boundary — not two. + expect(Number(g?.turns_used)).toBe(1) + }), + ) +}) From 1da14396d0fa7d3ac93889d6ef7f42428772ee82 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 09:17:31 +0800 Subject: [PATCH 10/11] fix(goal): close P3 hygiene findings (GOAL-FP-01-07/-08/-09/-12/-13/-14/-15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - GOAL-FP-01-07: updateAfterJudge `expected` (goalID+revision) is now a required parameter — the stale-judge protection is the contract, not a caller convention. matchesExpected's optional short-circuit is gone; typecheck enforces every caller passes the pre-judge identity. - GOAL-FP-01-08: Goal.set now unregisters the previous goal id from the automation lease atomically with the overwrite, so a replaced goal can no longer leave a double id in the registration set (owner() returned the stale first id and silently starved the new goal's claim). - GOAL-FP-01-09: the goal tool's `complete` no longer shows "✓ 目标已达成" when markDone no-ops (clear/complete race) — it reports the no-op instead of presenting a goal that no longer exists as achieved. - GOAL-FP-01-12: the dispatch-failure path now pauses via pauseGoal (pauseAndPublish + inline lease unregister), symmetric with every other pause site instead of depending on the trailing afterDispatch load. - GOAL-FP-01-13 (test): one integration test drives the goal continuation through the REAL SessionRunState.startIfIdle admission gate — real busy flip, real admission rejection, and the REAL Runner onIdle re-driving the loop to done with no manual idle events. Remains mocked: SessionPrompt admitPrompt/runLoop (full app layer — disproportionate), Session, Provider, judge LLM. - GOAL-FP-01-14: wake delivery dedupes on retry — a summary whose transcript part was already written is only re-marked, never re-prompted (in-process; the crash-between-write-and-mark residual on the restart sweep is registered — a durable delivering-marker would need a schema change). The delivery failure log now carries the cause. - GOAL-FP-01-15: the done confirmation prompt failure is logged instead of silently swallowed (no retry — a retried line could re-inject after a new goal is set; the crash-window transcript loss is inherent to the durable-leads-presentation invariant and the event stream still notifies consumers). Tests: red-first pinning tests for -08 (lease), -09 (tool API), -12 (lease after a defecting trailing load), -14 (wake retry dedupe); -13's test is the artifact. Mutation-verified for -08/-09/-12/-14. Co-Authored-By: Claude --- packages/opencode/src/dag/runtime/loop.ts | 89 ++++-- packages/opencode/src/goal/goal.ts | 32 ++- packages/opencode/src/goal/loop.ts | 25 +- packages/opencode/src/tool/goal.ts | 26 +- .../test/dag/dag-goal-wake-retrigger.test.ts | 88 +++++- packages/opencode/test/goal/e2e-loop.test.ts | 261 ++++++++++++++++++ packages/opencode/test/goal/goal.test.ts | 139 ++++++++-- packages/opencode/test/tool/goal-tool.test.ts | 22 ++ 8 files changed, 603 insertions(+), 79 deletions(-) diff --git a/packages/opencode/src/dag/runtime/loop.ts b/packages/opencode/src/dag/runtime/loop.ts index 6a62852fcc..06f09f9af1 100644 --- a/packages/opencode/src/dag/runtime/loop.ts +++ b/packages/opencode/src/dag/runtime/loop.ts @@ -69,6 +69,17 @@ const serviceLayer = Layer.effect( const recovering = new Set() const wakeInFlight = new Set() const wakePending = new Set() + // GOAL-FP-01-14: per-session record of the last wake summary whose + // transcript part was written. The durable mark runs AFTER the write + // (at-least-once delivery: a mark failure keeps the batch unreported + // for a retry), so a retry of an already-written summary would + // re-inject the same digest into the transcript. The retry dedupes on + // this map and only re-marks. In-process only — a crash between write + // and mark still duplicates on the restart sweep (a durable + // delivering-marker would need a schema change; registered, see + // GOAL-FP-01-14). Capped: evicting entries degrades to the pre-fix + // duplicate visibility, never to a lost wake. + const deliveredWakeSummaries = new Map() // Seed the commented global dag.jsonc once per instance init — the // per-round DagConfig.load below stays a pure read so the spawn @@ -1211,42 +1222,60 @@ const serviceLayer = Layer.effect( // receives the node result and can act) but NOT rendered as a user // message in the TUI chat — DAG data surfaces via the sidebar panel // and Inspector, keeping the chat conversation clean. + // + // GOAL-FP-01-14: the transcript part is written BEFORE the + // durable mark. A mark failure (or a crash between the two) + // leaves the batch unreported and the retry would re-inject the + // SAME summary. When this session already had this exact summary + // written, skip the prompt and only re-mark — the write is + // idempotent in effect because an identical digest adds no + // information. A differing summary (new results committed + // between attempts) always prompts. + if (deliveredWakeSummaries.size > 1024) deliveredWakeSummaries.clear() const didDeliver = Option.getOrElse( yield* automation.use( wakeLease, - promptSvc.promptIfIdle({ - sessionID: SessionID.make(sessionID), - parts: [{ type: "text", text: summary, synthetic: true }], - }).pipe( - Effect.flatMap(Option.match({ - onNone: () => Effect.succeed(false), - onSome: () => - store.markWakeBatchReported(batch).pipe( - Effect.tap(() => - Effect.forEach( - batch.workflows.filter((workflow) => - isWorkflowTerminalStatus(workflow.status as never), - ), - (workflow) => - automation.unregister(SessionID.make(sessionID), { - kind: "dag", - id: workflow.id, - }), - { discard: true }, - ), + Effect.gen(function* () { + if (deliveredWakeSummaries.get(sessionID) !== summary) { + const delivered = yield* promptSvc.promptIfIdle({ + sessionID: SessionID.make(sessionID), + parts: [{ type: "text", text: summary, synthetic: true }], + }) + if (Option.isNone(delivered)) return false + // Record BEFORE the mark: the transcript part was + // already written (the prompt just succeeded), so the + // retry must skip the prompt even when the mark below + // fails again. + deliveredWakeSummaries.set(sessionID, summary) + } + yield* store.markWakeBatchReported(batch).pipe( + Effect.tap(() => + Effect.forEach( + batch.workflows.filter((workflow) => + isWorkflowTerminalStatus(workflow.status as never), ), - Effect.tap(() => - Effect.sync(() => { - plan.unresponsiveDagIDs.forEach((workflowID) => - deliveredUnresponsiveDagIDs.add(workflowID), - ) + (workflow) => + automation.unregister(SessionID.make(sessionID), { + kind: "dag", + id: workflow.id, }), - ), - Effect.as(true), + { discard: true }, ), - })), - Effect.catchCause(() => - Effect.logWarning("DAG wake delivery failed", { sessionID }).pipe(Effect.as(false)), + ), + Effect.tap(() => + Effect.sync(() => { + plan.unresponsiveDagIDs.forEach((workflowID) => + deliveredUnresponsiveDagIDs.add(workflowID), + ) + }), + ), + ) + return true + }).pipe( + Effect.catchCause((cause) => + Effect.logWarning("DAG wake delivery failed", { sessionID, cause: Cause.pretty(cause) }).pipe( + Effect.as(false), + ), ), ), ), diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index b3a0aadcd4..c7fcb0af81 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -60,7 +60,11 @@ export interface Interface { verdict: GoalState.Verdict, reason: string, parseFailed: boolean, - expected?: { readonly goalID: string; readonly revision: number }, + /** GOAL-FP-01-07: the pre-judge state identity is part of the contract — + * a judge result is only applied when the durable row still carries this + * goalID+revision pair. Required (not optional): an omitted expected would + * let a stale loop result mutate whatever goal replaced the judged one. */ + expected: { readonly goalID: string; readonly revision: number }, ) => Effect.Effect< | { state: GoalState.Info @@ -285,10 +289,8 @@ const serviceLayer = Layer.effect( const matchesExpected = ( state: GoalState.Info, - expected?: { readonly goalID: string; readonly revision: number }, - ) => - !expected || - ((state.goal_id ?? "legacy") === expected.goalID && (state.revision ?? 0) === expected.revision) + expected: { readonly goalID: string; readonly revision: number }, + ) => (state.goal_id ?? "legacy") === expected.goalID && (state.revision ?? 0) === expected.revision const deleteAndPublishDone = Effect.fnUntraced(function* (sessionID: SessionID, reason: string) { return yield* transition(sessionID, (state) => { @@ -376,9 +378,21 @@ const serviceLayer = Layer.effect( consecutive_parse_failures: GoalState.nni(0), subgoals: [], }) - const result = yield* transition(sessionID, () => ({ tag: "save", state, value: state })) - yield* automation.register(sessionID, { kind: "goal", id: result.goal_id ?? "legacy" }) - return result + // GOAL-FP-01-08: the overwrite must stay consistent with the lease. The + // previous id is captured from the SAME seam read that decides the + // overwrite, and unregistered before the new id is registered — a stale + // id left in the registration set would be returned by owner() and + // reject the new goal's claim (loop silently starved until /goal clear). + const result = yield* transition(sessionID, (previous) => ({ + tag: "save", + state, + value: { state, previousGoalID: previous?.goal_id ?? "legacy" }, + })) + if (result.previousGoalID !== (result.state.goal_id ?? "legacy")) { + yield* automation.unregister(sessionID, { kind: "goal", id: result.previousGoalID }) + } + yield* automation.register(sessionID, { kind: "goal", id: result.state.goal_id ?? "legacy" }) + return result.state }) const pause = Effect.fn("Goal.pause")(function* (sessionID: SessionID, reason: string) { @@ -530,7 +544,7 @@ const serviceLayer = Layer.effect( verdict: GoalState.Verdict, reason: string, parseFailed: boolean, - expected?: { readonly goalID: string; readonly revision: number }, + expected: { readonly goalID: string; readonly revision: number }, ) { return yield* transition(sessionID, (state) => { if (!state || state.status !== "active" || !matchesExpected(state, expected)) diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index 2306354bcf..b43ebd294c 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -314,11 +314,25 @@ const serviceLayer = Layer.effect( yield* automation.unregister(sessionID, goalOwner) evaluatedRevisions.delete(sessionID) if (verdict.verdict === "done") { + // GOAL-FP-01-15: the done transition has already committed when this + // prompt runs (durable state leads presentation — the row is gone + // and goal.updated(done)/goal.cleared are published), so a failure + // here loses only the transcript line, never the state. Never + // swallow it silently — log it so a lost confirmation is + // diagnosable. No retry: a retried prompt could re-inject a "done" + // line after the goal was re-created. yield* promptSvc.prompt({ sessionID, noReply: true, parts: [{ type: "text", text: updateResult.message }], - }).pipe(Effect.ignore) + }).pipe( + Effect.catchCause((cause) => + Effect.logWarning("goal done message delivery failed", { + sessionID, + cause: Cause.pretty(cause), + }), + ), + ) } else { // Auto-pause branch: updateAfterJudge paused the goal due to // judge-parse-failure or budget exhaustion (verdict.verdict is @@ -428,7 +442,14 @@ const serviceLayer = Layer.effect( } const errMsg = `continuation dispatch failed: ${Cause.pretty(cause)}` yield* Effect.logWarning("goal continuation dispatch failed", { error: Cause.pretty(cause) }) - yield* goal.pauseAndPublish(sessionID, errMsg).pipe(Effect.ignore) + // GOAL-FP-01-12: symmetric with every other pause site — the + // unregister must be part of the failure transition, not + // deferred to the trailing afterDispatch load (which a defect or + // a concurrent replacement can skip, leaking the registration + // until /goal clear). pauseGoal keeps the fiber-safe + // pauseAndPublish (goal.pause would clearFiber — us — + // mid-publish) and releases the lease registration inline. + yield* pauseGoal(sessionID, errMsg).pipe(Effect.ignore) yield* promptSvc.prompt({ sessionID, noReply: true, parts: [{ type: "text", text: `⏸ 目标已暂停 — ${errMsg}` }] }).pipe(Effect.ignore) return Option.none() }), diff --git a/packages/opencode/src/tool/goal.ts b/packages/opencode/src/tool/goal.ts index 8ad1243c67..55c3197c89 100644 --- a/packages/opencode/src/tool/goal.ts +++ b/packages/opencode/src/tool/goal.ts @@ -127,18 +127,30 @@ export const GoalTool = Tool.define( // could be missed in the "N turns" count shown to the user. const finalState = yield* goal.markDone(ctx.sessionID, params.reason.trim()) - const displayState = finalState ?? state - const completionMsg = `✓ 目标已达成（${displayState.turns_used}/${displayState.max_turns} 轮）：${displayState.goal}\nReason: ${params.reason.trim()}` + // GOAL-FP-01-09: markDone returns undefined when the transition did + // not happen (the goal was cleared or completed between the `load` + // above and the markDone transition). Presenting the pre-call state + // as completed would claim an achievement for a goal that no longer + // exists — report the no-op instead. + if (!finalState) { + return { + title: "goal no longer active", + output: + "Cannot complete goal: the goal is no longer active (it may have been cleared or completed concurrently). No state transition was applied.", + metadata: { goal: null }, + } + } + const completionMsg = `✓ 目标已达成（${finalState.turns_used}/${finalState.max_turns} 轮）：${finalState.goal}\nReason: ${params.reason.trim()}` return { - title: `goal completed (${displayState.turns_used}/${displayState.max_turns})`, + title: `goal completed (${finalState.turns_used}/${finalState.max_turns})`, output: completionMsg, metadata: { goal: { - text: displayState.goal, + text: finalState.goal, status: "done" as const, - turnsUsed: displayState.turns_used, - maxTurns: displayState.max_turns, - subgoals: displayState.subgoals ?? [], + turnsUsed: finalState.turns_used, + maxTurns: finalState.max_turns, + subgoals: finalState.subgoals ?? [], }, }, } diff --git a/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts index 60fb088287..45d62f4080 100644 --- a/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts +++ b/packages/opencode/test/dag/dag-goal-wake-retrigger.test.ts @@ -103,17 +103,39 @@ function takeWithin(queue: Queue.Queue, message: string) { let judgeCalls = 0 let promptCalls: { noReply?: boolean; text: string }[] = [] let parentPromptCalls = 0 +let markReportCalls = 0 const reset = () => { judgeCalls = 0 promptCalls = [] parentPromptCalls = 0 + markReportCalls = 0 } -function goalWakeLayer(input: { childPrompts: Queue.Queue }) { +function goalWakeLayer(input: { childPrompts: Queue.Queue; failFirstMarkReport?: boolean }) { const database = Database.layerFromPath(":memory:") const events = EventV2.layer.pipe(Layer.provide(database)) const bridge = EventV2Bridge.layer.pipe(Layer.provide(events)) - const store = DagStore.layer.pipe(Layer.provide(database)) + const store = input.failFirstMarkReport + ? // GOAL-FP-01-14 harness: the real store with ONE injected failure on the + // first markWakeBatchReported call — the wake transcript part has + // already been written when that mark fails, and the retry must not + // re-inject the summary. + Layer.effect( + DagStore.Service, + Effect.gen(function* () { + const real = yield* DagStore.Service + return DagStore.Service.of({ + ...real, + markWakeBatchReported: (batch: DagStore.WakeBatch) => + Effect.gen(function* () { + markReportCalls += 1 + if (markReportCalls === 1) return yield* Effect.die("injected markWakeBatchReported failure") + return yield* real.markWakeBatchReported(batch) + }), + }) + }), + ).pipe(Layer.provide(DagStore.layer.pipe(Layer.provide(database)))) + : DagStore.layer.pipe(Layer.provide(database)) const status = SessionStatus.layer.pipe(Layer.provide(bridge)) const projector = DagProjector.layer.pipe( Layer.provide(events), @@ -263,6 +285,7 @@ function runGoalWakeTest( readonly database: Database.Interface readonly childPrompts: Queue.Queue }) => Effect.Effect, + layerInput: { failFirstMarkReport?: boolean } = {}, ) { return Effect.gen(function* () { const childPrompts = yield* Queue.unbounded() @@ -297,7 +320,7 @@ function runGoalWakeTest( .pipe(Effect.orDie) return yield* test({ dag, loop, goalLoop, store, goal, automation, database, childPrompts }) }).pipe( - Effect.provide(goalWakeLayer({ childPrompts })), + Effect.provide(goalWakeLayer({ childPrompts, ...layerInput })), Effect.provideService(InstanceRef, { directory: process.cwd(), worktree: process.cwd(), @@ -386,3 +409,62 @@ describe("DagLoop final wake delivery re-triggers the goal (GOAL-FP-01-02)", () ) }) }) + +// GOAL-FP-01-14: the wake transcript part is written BEFORE the durable +// markWakeBatchReported, so a mark failure (or a crash between the two) leaves +// the batch unreported — and the retry re-injects the SAME summary into the +// transcript (duplicate visibility). The delivery must dedupe on retry: when +// the summary was already written, the retry only re-marks, it must not +// re-prompt. The retry here is armed by the wake turn's own idle event (the +// prompt mock mirrors the real runner's end-of-turn idle). +describe("DagLoop wake delivery — a mark failure retry must not re-inject the summary (GOAL-FP-01-14)", () => { + it("the wake summary reaches the transcript exactly once when the first mark fails", async () => { + await Effect.runPromise( + runGoalWakeTest( + ({ dag, loop, store, childPrompts }) => + Effect.gen(function* () { + reset() + yield* loop.init() + yield* Effect.yieldNow + + const dagID = yield* dag.create({ + projectID: PROJECT_ID, + sessionID: PARENT_SESSION, + title: "mark failure retry", + config: { name: "mark-fail", nodes: [node("implement")] }, + }) + + const child = yield* takeWithin(childPrompts, "implement did not start") + yield* Deferred.succeed(child.release, "done") + yield* pollWithTimeout( + store.getWorkflow(dagID).pipe( + Effect.map((workflow) => (workflow?.status === "completed" ? workflow : undefined)), + ), + "workflow did not complete", + ) + + // First delivery attempt writes the transcript part, then the + // injected mark failure leaves the batch unreported. The retry + // (armed by the wake turn's own idle event) must re-mark only. + yield* pollWithTimeout( + Effect.sync(() => (markReportCalls >= 2 ? true : undefined)), + "wake delivery never retried after the injected mark failure", + "5 seconds", + ) + yield* pollWithTimeout( + store.getWorkflow(dagID).pipe( + Effect.map((workflow) => (workflow?.wakeReported ? workflow : undefined)), + ), + "wake was never reported", + ) + + // Pre-fix: the retry re-prompted the identical summary — the + // transcript would show the wake digest twice. + const wakeSummaries = promptCalls.filter((p) => p.text.includes("[DAG Workflow completed]")) + expect(wakeSummaries.length).toBe(1) + }), + { failFirstMarkReport: true }, + ), + ) + }) +}) diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index 0b24da1b51..ec5d68fc15 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -7,6 +7,7 @@ import { EventV2Bridge } from "@/event-v2-bridge" import { SessionStatus } from "@/session/status" import { Session } from "@/session/session" import { SessionPrompt } from "@/session/prompt" +import { SessionRunState } from "@/session/run-state" import { Provider } from "@/provider/provider" import { SessionID } from "@/session/schema" import { SessionAutomationLease } from "@/session/automation-lease" @@ -576,6 +577,266 @@ describe("GoalLoop — continuation dispatch failure → recoverable pause (D1)" ) }) +// ── GOAL-FP-01-12: dispatch-failure unregister must not depend on the +// trailing load ───────────────────────────────────────────────────────── +// +// The failure path pauses the goal and the loop releases the lease +// registration afterwards. That release must be SYMMETRIC with the pause +// (pauseAndPublish + unregister in the same handler) — it must not depend on +// the afterDispatch load that follows the dispatch attempt. To make the +// dependency observable, the failure path's visible-pause prompt parks on a +// gate; the test body then drops the goal_state table and releases the gate, +// so the trailing load dies with a defect: only an inline unregister can +// release the lease. +describe("GoalLoop — dispatch failure releases the lease without the trailing load (GOAL-FP-01-12)", () => { + let judgeCalls = 0 + let promptGate = Deferred.makeUnsafe() + const reset = () => { + judgeCalls = 0 + promptGate = Deferred.makeUnsafe() + } + + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + }) + // Continuation dispatch fails (promptIfIdle). The failure handler's + // visible-pause prompt parks on a gate — the sync point where the test body + // drops the goal_state table — so afterDispatch's goal.load defects: the + // lease release must NOT depend on that trailing load. The die after the + // gate is swallowed by the handler's Effect.ignore. + const promptFailAndParkMock = Layer.mock(SessionPrompt.Service, { + prompt: () => + Effect.gen(function* () { + yield* Deferred.await(promptGate) + return yield* Effect.die("failure-path prompt is the last stop before the trailing load") + }), + promptIfIdle: () => Effect.die(new Error("continuation provider down")), + }) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => + Effect.sync(() => { + judgeCalls += 1 + return JSON.stringify({ done: false, reason: "more steps needed" }) + }), + }), + ) + const failLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptFailAndParkMock), + Layer.provide(Layer.mock(Provider.Service, {})), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + Layer.provideMerge(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + Layer.provideMerge(Database.defaultLayer), + ) + const it = testEffect(failLayer) + + it.instance("the lease registration is gone after the failure pause even when the post-dispatch load dies", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const automation = yield* SessionAutomationLease.Service + const { db } = yield* Database.Service + const events = yield* EventV2Bridge.Service + const seen = yield* captureEvents(events) + yield* loop.init() + const sid = SessionID.descending() + const goalState = yield* goal.set(sid, "ship the feature", 10) + const goalOwner = { kind: "goal" as const, id: goalState.goal_id ?? "legacy" } + yield* Effect.yieldNow + + yield* events.publish(SessionStatus.Event.Status, { sessionID: sid, status: { type: "idle" } }) + // The pause committed and published BEFORE the handler reaches the + // parked prompt — the parked handler is the deterministic sync point. + yield* pollWithTimeout( + Effect.sync(() => + seen.some((e) => e.type === GoalEvent.Updated.type && e.status === "paused") ? true : undefined, + ), + "failure path never paused the goal", + "5 seconds", + ) + // Kill the trailing load: the handler is parked on the prompt gate, so + // dropping the table here guarantees afterDispatch's goal.load defects. + yield* db.run("DROP TABLE goal_state") + yield* Deferred.succeed(promptGate, undefined) + // Give the (defecting) trailing load a scheduler turn to run. + yield* Effect.sleep("100 millis") + expect(judgeCalls).toBeGreaterThanOrEqual(1) + + // The registration must already be released — pre-fix it leaks until + // /goal clear because the trailing load (the only unregister) died. + expect(Option.isNone(yield* automation.claim(sid, goalOwner))).toBe(true) + }), + ) +}) + +// ── GOAL-FP-01-13: real admission seam for the goal continuation ─────── +// +// Every other GoalLoop harness mocks SessionPrompt with a flat +// `promptIfIdle: () => Option.some(...)` — the real admission gate +// (SessionRunState.startIfIdle: Runner state machine, busy flip, onIdle → +// real SessionStatus.set → real idle event) is never exercised, so the +// lease-claim + promptIfIdle atomicity has no regression coverage. +// +// The REAL SessionPrompt layer pulls in the whole app (Permission, MCP, LSP, +// ToolRegistry, Config, Plugin, …) — disproportionate for this suite. The +// tightest feasible real seam: the REAL SessionRunState.defaultLayer, with a +// SessionPrompt mock that delegates promptIfIdle admission to the real gate +// exactly like the real implementation's core. Remains mocked (reported): +// SessionPrompt.admitPrompt (transcript write) + runLoop (provider turn), +// Session.messages, Provider, judge LLM. +describe("GoalLoop — real SessionRunState admission seam (GOAL-FP-01-13)", () => { + let judgeCalls = 0 + let admissions = 0 + let rejectedAdmissions = 0 + let firstAdmissionParked = false + let admissionRelease = Deferred.makeUnsafe() + const reset = () => { + judgeCalls = 0 + admissions = 0 + rejectedAdmissions = 0 + firstAdmissionParked = false + admissionRelease = Deferred.makeUnsafe() + } + + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistant()]), + }) + // Effect.fn-wrapped like the wake-integration harness's `deliver` mock. The + // real SessionRunState is resolved via serviceOption (R-free, same pattern + // the harness uses for the bridge) so the implementation stays assignable to + // the SessionPrompt Interface while still hitting the REAL admission gate. + // + // The scripted "run" completes with an interrupt (typed never, no cast): + // the Runner's finishRun still emits the real onIdle (status.set(idle) → + // real event) before completing the handle, so the loop re-drive chain is + // real. The mock returns Option.none() even on admission — afterIdle + // discards the promptIfIdle result (only its failure matters), and + // admission is observable through the real status flip and the counters. + const promptIfIdle = Effect.fn("test.goalSeam.SessionPrompt.promptIfIdle")(function* ( + input: SessionPrompt.PromptInput, + ) { + const runState = yield* Effect.serviceOption(SessionRunState.Service) + if (Option.isNone(runState)) return yield* Effect.die("SessionRunState not provided to the seam mock") + const admitted = yield* runState.value.startIfIdle( + input.sessionID, + Effect.die("onInterrupt is not exercised in this scenario"), + Effect.gen(function* () { + admissions += 1 + if (admissions === 1) { + firstAdmissionParked = true + yield* Deferred.await(admissionRelease) + } + return yield* Effect.interrupt + }), + ) + if (Option.isNone(admitted)) { + rejectedAdmissions += 1 + return Option.none() + } + // Await the run's completion (the Cancelled exit is captured) so the + // mock's promptIfIdle stays faithful to the real one's waiting behavior. + yield* admitted.value.pipe(Effect.exit, Effect.asVoid) + return Option.none() + }) + const promptMock = Layer.mock(SessionPrompt.Service, { + prompt: () => Effect.die("the direct prompt path is not exercised in this scenario"), + promptIfIdle, + }) + const judgeMock = Layer.succeed( + GoalLoopJudgeLLM, + GoalLoopJudgeLLM.of({ + call: () => + Effect.sync(() => { + judgeCalls += 1 + // Calls 1-2 continue (drive admissions 1-2); call 3 ends the goal. + return judgeCalls <= 2 + ? JSON.stringify({ done: false, reason: "more steps needed" }) + : JSON.stringify({ done: true, reason: "feature shipped" }) + }), + }), + ) + const seamLayer = GoalLoop.layer.pipe( + Layer.provide(sessionMock), + Layer.provide(promptMock), + Layer.provide(Layer.mock(Provider.Service, {})), + Layer.provide(judgeMock), + Layer.provideMerge(Goal.defaultLayer), + Layer.provideMerge(SessionStatus.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + Layer.provideMerge(SessionRunState.defaultLayer), + ) + const it = testEffect(seamLayer) + + it.instance("a continuation admitted by the real gate flips the session busy, blocks concurrent admission, and the real idle re-drives the loop to done", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const status = yield* SessionStatus.Service + const runState = yield* SessionRunState.Service + const events = yield* EventV2Bridge.Service + const seen = yield* captureEvents(events) + yield* loop.init() + const sid = SessionID.descending() + yield* goal.set(sid, "ship the feature", 10) + yield* Effect.yieldNow + + // Turn 1: idle → judge(continue) → continuation admitted through the + // REAL admission gate; the scripted run parks and the session is BUSY. + yield* events.publish(SessionStatus.Event.Status, { sessionID: sid, status: { type: "idle" } }) + yield* pollWithTimeout( + Effect.sync(() => (firstAdmissionParked ? true : undefined)), + "the continuation was never admitted through the real gate", + "5 seconds", + ) + // Real seam proof: admission itself flipped the session status to busy. + expect((yield* status.get(sid)).type).toBe("busy") + // The real gate rejects concurrent admission while the goal run holds it + // (the probe work is Effect.never — a rejection never forks it). + const probe = yield* runState.startIfIdle( + sid, + Effect.die("probe onInterrupt is not exercised"), + Effect.never, + ) + expect(Option.isNone(probe)).toBe(true) + expect(admissions).toBe(1) + + // Release the run: the REAL Runner onIdle publishes the REAL idle + // status event, which re-drives GoalLoop with NO manual idle publish — + // the next continuation and the judge(done) terminal transition both + // ride the real chain. + yield* Deferred.succeed(admissionRelease, undefined) + yield* pollWithTimeout( + Effect.sync(() => (admissions >= 2 ? true : undefined)), + "the real onIdle event never re-drove the goal loop", + "5 seconds", + ) + expect(rejectedAdmissions).toBe(0) + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 3 ? true : undefined)), + "the loop never reached the terminal judge call", + "5 seconds", + ) + + // Terminal contract through the real chain: exactly one done update, + // the cleared event, and the row is gone. + const doneUpdates = seen.filter((e) => e.type === GoalEvent.Updated.type && e.status === "done") + expect(doneUpdates.length).toBe(1) + expect(seen.some((e) => e.type === GoalEvent.Cleared.type)).toBe(true) + expect(yield* goal.load(sid)).toBeUndefined() + expect(admissions).toBe(2) + expect(judgeCalls).toBe(3) + }), + ) +}) + // ── Stall-prevention branch coverage ─────────────────────────────────── // // afterIdle has four historically-silent stall paths that now surface as diff --git a/packages/opencode/test/goal/goal.test.ts b/packages/opencode/test/goal/goal.test.ts index 739f08e13b..ef65f9babc 100644 --- a/packages/opencode/test/goal/goal.test.ts +++ b/packages/opencode/test/goal/goal.test.ts @@ -1,10 +1,11 @@ import { describe, expect } from "bun:test" -import { Deferred, Effect, Fiber, Layer } from "effect" +import { Deferred, Effect, Fiber, Layer, Option } from "effect" import { Goal } from "@/goal/goal" import { GoalEvent } from "@/goal/events" import { GoalPrompts } from "@/goal/prompts" import { EventV2Bridge } from "@/event-v2-bridge" import { SessionStatus } from "@/session/status" +import { SessionAutomationLease } from "@/session/automation-lease" import { Database } from "@opencode-ai/core/database/database" import { SessionID } from "@/session/schema" import { pollWithTimeout, testEffect } from "../lib/effect" @@ -99,10 +100,13 @@ describe("Goal.updateAfterJudge — continue branch", () => { const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "build feature X", 10) + const state = yield* goal.set(sessionID, "build feature X", 10) seen.length = 0 // drop the set() goal.updated(active) - const result = yield* goal.updateAfterJudge(sessionID, "continue", "more steps", false) + const result = yield* goal.updateAfterJudge(sessionID, "continue", "more steps", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) expect(result?.shouldContinue).toBe(true) const loaded = yield* goal.load(sessionID) @@ -128,7 +132,10 @@ describe("Goal.updateAfterJudge — atomic done transition", () => { const before = yield* goal.load(sessionID) const n = Number(before?.turns_used) - const result = yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) + const result = yield* goal.updateAfterJudge(sessionID, "done", "delivered", false, { + goalID: before?.goal_id ?? "legacy", + revision: before?.revision ?? 0, + }) expect(result?.state.status).toBe("done") expect(Number(result?.state.turns_used)).toBe(n) @@ -148,10 +155,13 @@ describe("Goal.updateAfterJudge — done branch (terminal event contract)", () = const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "ship feature X", 10) + const state = yield* goal.set(sessionID, "ship feature X", 10) seen.length = 0 - yield* goal.updateAfterJudge(sessionID, "done", "delivered", false) + yield* goal.updateAfterJudge(sessionID, "done", "delivered", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) const types = seen.map((e) => e.type) expect(types).toEqual([GoalEvent.Updated.type, GoalEvent.Cleared.type]) @@ -174,7 +184,7 @@ describe("Goal.updateAfterJudge — blocked branch", () => { const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "deploy production", 10) + const state = yield* goal.set(sessionID, "deploy production", 10) seen.length = 0 const result = yield* goal.updateAfterJudge( @@ -182,6 +192,7 @@ describe("Goal.updateAfterJudge — blocked branch", () => { "blocked", "missing production credentials", false, + { goalID: state.goal_id ?? "legacy", revision: state.revision ?? 0 }, ) expect(result?.state.status).toBe("paused") @@ -197,12 +208,15 @@ describe("Goal transition authority — stale loop decisions", () => { Effect.gen(function* () { const goal = yield* Goal.Service const sessionID = SessionID.descending() - yield* goal.set(sessionID, "ship feature X", 10) + const state = yield* goal.set(sessionID, "ship feature X", 10) yield* Effect.all( [ goal.pause(sessionID, "user-paused"), - goal.updateAfterJudge(sessionID, "continue", "racing judge result", false), + goal.updateAfterJudge(sessionID, "continue", "racing judge result", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }), ], { concurrency: 2 }, ) @@ -215,12 +229,15 @@ describe("Goal transition authority — stale loop decisions", () => { Effect.gen(function* () { const goal = yield* Goal.Service const sessionID = SessionID.descending() - yield* goal.set(sessionID, "ship feature X", 10) + const state = yield* goal.set(sessionID, "ship feature X", 10) yield* Effect.all( [ goal.clear(sessionID), - goal.updateAfterJudge(sessionID, "continue", "racing judge result", false), + goal.updateAfterJudge(sessionID, "continue", "racing judge result", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }), ], { concurrency: 2 }, ) @@ -309,9 +326,12 @@ describe("Goal.markDone — turns_used is budget-neutral", () => { const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "ship feature X", 10) + const state = yield* goal.set(sessionID, "ship feature X", 10) // Simulate one continuation dispatch (the budget-consuming event). - yield* goal.updateAfterJudge(sessionID, "continue", "more steps", false) + yield* goal.updateAfterJudge(sessionID, "continue", "more steps", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) const continued = yield* goal.load(sessionID) seen.length = 0 @@ -324,6 +344,39 @@ describe("Goal.markDone — turns_used is budget-neutral", () => { ) }) +// --------------------------------------------------------------------------- +// GOAL-FP-01-08: Goal.set must not leave the previous goal's id in the lease. +// The lease's owner() returns the FIRST id in the registration set, so a stale +// entry makes the new goal's claim be rejected (selected.id !== request.id) +// and the loop silently starves. Replacing a goal must unregister the previous +// id atomically with the new registration. +// --------------------------------------------------------------------------- + +const setLeaseLayer = Goal.layer.pipe( + Layer.provide(SessionStatus.defaultLayer), + Layer.provideMerge(SessionAutomationLease.defaultLayer), + Layer.provideMerge(Database.defaultLayer), + Layer.provideMerge(EventV2Bridge.defaultLayer), +) + +describe("Goal.set — replacing a goal unregisters the previous lease id (GOAL-FP-01-08)", () => { + testEffect(setLeaseLayer).live("set on an existing goal leaves exactly the new goal id claimable", () => + Effect.gen(function* () { + const goal = yield* Goal.Service + const lease = yield* SessionAutomationLease.Service + const sessionID = SessionID.descending() + + const first = yield* goal.set(sessionID, "goal A", 10) + const second = yield* goal.set(sessionID, "goal B", 10) + + // Pre-fix: the lease still holds BOTH ids (register never removes the + // previous one), so the stale id is claimable and the fresh one is not. + expect(Option.isNone(yield* lease.claim(sessionID, { kind: "goal", id: first.goal_id ?? "legacy" }))).toBe(true) + expect(Option.isSome(yield* lease.claim(sessionID, { kind: "goal", id: second.goal_id ?? "legacy" }))).toBe(true) + }), + ) +}) + // --------------------------------------------------------------------------- // §5 — Expand state-machine coverage (lock the contract). All PASS against // current post-bug-fix behavior; they exist to catch regressions when §6-§10 @@ -406,9 +459,12 @@ describe("Goal.resume — preserves turns_used (no fresh budget), resets parse f const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "build feature X", 10) + const state = yield* goal.set(sessionID, "build feature X", 10) // One continuation dispatch with a parse failure → turns_used=1, cpf=1 - yield* goal.updateAfterJudge(sessionID, "continue", "more steps", true) + yield* goal.updateAfterJudge(sessionID, "continue", "more steps", true, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) const beforePause = yield* goal.load(sessionID) expect(Number(beforePause?.turns_used)).toBe(1) expect(Number(beforePause?.consecutive_parse_failures)).toBe(1) @@ -445,9 +501,15 @@ describe("Goal.resume — preserves turns_used (no fresh budget), resets parse f const sessionID = SessionID.descending() // max_turns=2: a second continue verdict trips the budget-pause branch - yield* goal.set(sessionID, "build feature X", 2) - yield* goal.updateAfterJudge(sessionID, "continue", "step 1", false) // turns_used 1 - yield* goal.updateAfterJudge(sessionID, "continue", "step 2", false) // turns_used 2 >= max → paused + const state = yield* goal.set(sessionID, "build feature X", 2) + const step1 = yield* goal.updateAfterJudge(sessionID, "continue", "step 1", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) // turns_used 1 + yield* goal.updateAfterJudge(sessionID, "continue", "step 2", false, { + goalID: step1?.state.goal_id ?? "legacy", + revision: step1?.state.revision ?? 0, + }) // turns_used 2 >= max → paused const paused = yield* goal.load(sessionID) expect(paused?.status).toBe("paused") @@ -587,12 +649,18 @@ describe("Goal.updateAfterJudge — transport failures trigger auto-pause (D5)", const seen = yield* captureEvents(events) const sessionID = SessionID.descending() - yield* goal.set(sessionID, "build feature X", 10) + const state = yield* goal.set(sessionID, "build feature X", 10) seen.length = 0 // Two transport failures — still active, counter climbing 1 → 2 - const r1 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 1", true) - const r2 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 2", true) + const r1 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 1", true, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) + const r2 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 2", true, { + goalID: r1?.state.goal_id ?? "legacy", + revision: r1?.state.revision ?? 0, + }) expect(r1?.shouldContinue).toBe(true) expect(r2?.shouldContinue).toBe(true) @@ -601,7 +669,10 @@ describe("Goal.updateAfterJudge — transport failures trigger auto-pause (D5)", expect(Number(midState?.consecutive_parse_failures)).toBe(2) // Third transport failure — counter reaches 3 → auto-pause - const r3 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 3", true) + const r3 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error 3", true, { + goalID: r2?.state.goal_id ?? "legacy", + revision: r2?.state.revision ?? 0, + }) expect(r3?.shouldContinue).toBe(false) const finalState = yield* goal.load(sessionID) @@ -625,22 +696,31 @@ describe("Goal.updateAfterJudge — transport failures trigger auto-pause (D5)", const goal = yield* Goal.Service const sessionID = SessionID.descending() - yield* goal.set(sessionID, "build feature X", 10) + const seeded = yield* goal.set(sessionID, "build feature X", 10) // transport-fail (parseFailed: true) → counter 1 - yield* goal.updateAfterJudge(sessionID, "continue", "transport error", true) + const first = yield* goal.updateAfterJudge(sessionID, "continue", "transport error", true, { + goalID: seeded.goal_id ?? "legacy", + revision: seeded.revision ?? 0, + }) let state = yield* goal.load(sessionID) expect(Number(state?.consecutive_parse_failures)).toBe(1) expect(state?.status).toBe("active") // parse-fail (parseFailed: true) → counter 2 - yield* goal.updateAfterJudge(sessionID, "continue", "无法解析", true) + yield* goal.updateAfterJudge(sessionID, "continue", "无法解析", true, { + goalID: first?.state.goal_id ?? "legacy", + revision: first?.state.revision ?? 0, + }) state = yield* goal.load(sessionID) expect(Number(state?.consecutive_parse_failures)).toBe(2) expect(state?.status).toBe("active") // transport-fail (parseFailed: true) → counter 3 → PAUSE - const r3 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error", true) + const r3 = yield* goal.updateAfterJudge(sessionID, "continue", "transport error", true, { + goalID: state?.goal_id ?? "legacy", + revision: state?.revision ?? 0, + }) expect(r3?.shouldContinue).toBe(false) state = yield* goal.load(sessionID) @@ -877,8 +957,11 @@ describe("Goal.dispatch resume — busy guard (D5)", () => { const goal = yield* Goal.Service const sessionID = SessionID.descending() // max_turns 1 → one continue exhausts the budget and auto-pauses. - yield* goal.set(sessionID, "ship feature X", 1) - yield* goal.updateAfterJudge(sessionID, "continue", "more", false) + const state = yield* goal.set(sessionID, "ship feature X", 1) + yield* goal.updateAfterJudge(sessionID, "continue", "more", false, { + goalID: state.goal_id ?? "legacy", + revision: state.revision ?? 0, + }) const paused = yield* goal.load(sessionID) expect(paused?.status).toBe("paused") diff --git a/packages/opencode/test/tool/goal-tool.test.ts b/packages/opencode/test/tool/goal-tool.test.ts index 3ba125b036..577977417e 100644 --- a/packages/opencode/test/tool/goal-tool.test.ts +++ b/packages/opencode/test/tool/goal-tool.test.ts @@ -120,6 +120,28 @@ describe("tool.goal — service resolution phase", () => { }), ) + // GOAL-FP-01-09: markDone re-loads the current row and can no-op when the + // goal was cleared or completed between the tool's `load` and the transition. + // The tool must NOT present that no-op as an achievement — the stale + // "✓ 目标已达成" line would claim a transition that never happened. + it.instance("complete does not claim achievement when markDone did not transition (GOAL-FP-01-09)", () => + Effect.gen(function* () { + const info = yield* GoalTool + const tool = yield* info.init() + const goalLayer = Layer.mock(Goal.Service, { + load: () => Effect.succeed(activeGoal), + markDone: () => Effect.succeed(undefined), + }) + + const result = yield* tool.execute({ action: "complete", reason: "docs read" }, ctx()).pipe( + Effect.provide(goalLayer), + ) + + expect(result.output).not.toContain("目标已达成") + expect(result.output).toContain("Cannot complete goal") + }), + ) + it.instance("status degrades gracefully when Goal.Service is absent (headless)", () => Effect.gen(function* () { const info = yield* GoalTool From 3c7ae2833bf0810d50dbc356f4f10865509b9249 Mon Sep 17 00:00:00 2001 From: Lex Date: Thu, 13 Aug 2026 10:20:03 +0800 Subject: [PATCH 11/11] =?UTF-8?q?fix(goal):=20harden=20the=20startup-scan?= =?UTF-8?q?=20seam=20=E2=80=94=20session.directory=20index,=20defensive=20?= =?UTF-8?q?scan=20ref,=20lease=20SessionStatus=20requirement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Standards deep review follow-ups on the GOAL-FP-01-04 startup scan and the GOAL-FP-01-02 dag-release re-trigger (S-1..S-3, all P2). S-1: missing session.directory index. The boot scan joins goal_state → session filtered on session.directory, so every instance boot linearly scanned the whole channel-global session table. Added the inline index("session_directory_idx").on(table.directory) to the session table definition and generated the migration + snapshot via the sanctioned generator (packages/core/script/migration.ts): new migration file 20260813020344_bored_skaar, schema.json / schema.gen.ts / migration.gen.ts regenerated; `bun run script/migration.ts --check` is clean. The regeneration also reconciled pre-existing snapshot drift: the hand-written 20260811060000_goal_outcome migration had never been baked into schema.json/schema.gen (the check was already red at HEAD); the generator's duplicate of it was discarded so existing installs never re-run the DDL. S-2: scanDirectoryRef fragile-by-construction. The unset-ref invariant lives only in the init→builder call order. The builder now reads the ref defensively: if it is unset at build time, log an ERROR and skip the scan (loud no-op) instead of querying with an empty directory that silently matches no session. The alternative (threading the directory through the ScopedCache key or InstanceState.make input) would require modifying shared instance-state.ts beyond the listed files; the defensive read is the accepted fallback. Not covered by a test: the unset path is unreachable through the public seam — init sets the ref before the only call site of InstanceState.get — so no injectable unset-ref path exists without exposing internals. S-3: serviceOption(SessionStatus) unsanctioned in the lease. The dag-release re-trigger silently degraded to a dropped re-trigger when SessionStatus was absent. SessionStatus.Service is now a HARD requirement of the lease layer (Layer.sync → Layer.effect; the serviceOption/None branch is gone); defaultLayer self-provides SessionStatus.defaultLayer, and the node lists SessionStatus.node (added to Session's node list — the documented "missing wire fails silently" invariant). All production and test consumers already build via defaultLayer, so only the standalone lease test needed wiring; it now also gains a re-trigger test asserting the blocked goal claim is re-driven through the real SessionStatus idle publish (typed via the event definition's data schema, no unsafe assertions). Verified: bun test test/goal test/dag test/session/automation-lease.test.ts (579 pass, 0 fail) and bun test test/session (408 pass, 0 fail), bun typecheck clean, bun lint 4852 warnings (≤ 4852). Co-Authored-By: Claude --- packages/core/schema.json | 89 ++++++++++++++++++- packages/core/src/database/migration.gen.ts | 1 + .../migration/20260813020344_bored_skaar.ts | 11 +++ packages/core/src/database/schema.gen.ts | 19 ++-- packages/core/src/session/sql.ts | 3 + packages/opencode/src/goal/loop.ts | 14 +++ .../opencode/src/session/automation-lease.ts | 38 ++++---- packages/opencode/src/session/session.ts | 5 ++ .../test/session/automation-lease.test.ts | 46 +++++++++- 9 files changed, 195 insertions(+), 31 deletions(-) create mode 100644 packages/core/src/database/migration/20260813020344_bored_skaar.ts diff --git a/packages/core/schema.json b/packages/core/schema.json index 126f187051..cd735b190a 100644 --- a/packages/core/schema.json +++ b/packages/core/schema.json @@ -1,9 +1,9 @@ { "version": "7", "dialect": "sqlite", - "id": "abdf5c23-7f2e-4ca3-b08b-012db47b5aa5", + "id": "7e8e00e9-7bbb-443e-996b-f646ec030c2b", "prevIds": [ - "442cdbd5-86a8-41a9-86d6-5361dbac90e0" + "cce2163c-da01-4239-86fa-776d48a58d89" ], "ddl": [ { @@ -50,6 +50,10 @@ "name": "event", "entityType": "tables" }, + { + "name": "goal_outcome", + "entityType": "tables" + }, { "name": "goal_state", "entityType": "tables" @@ -1018,6 +1022,46 @@ "entityType": "columns", "table": "event" }, + { + "type": "text", + "notNull": false, + "autoincrement": false, + "default": null, + "generated": null, + "name": "goal_id", + "entityType": "columns", + "table": "goal_outcome" + }, + { + "type": "text", + "notNull": true, + "autoincrement": false, + "default": null, + "generated": null, + "name": "session_id", + "entityType": "columns", + "table": "goal_outcome" + }, + { + "type": "text", + "notNull": true, + "autoincrement": false, + "default": null, + "generated": null, + "name": "payload", + "entityType": "columns", + "table": "goal_outcome" + }, + { + "type": "integer", + "notNull": true, + "autoincrement": false, + "default": null, + "generated": null, + "name": "completed_at", + "entityType": "columns", + "table": "goal_outcome" + }, { "type": "text", "notNull": false, @@ -2364,6 +2408,15 @@ "table": "event", "entityType": "pks" }, + { + "columns": [ + "goal_id" + ], + "nameExplicit": false, + "name": "goal_outcome_pk", + "table": "goal_outcome", + "entityType": "pks" + }, { "columns": [ "session_id" @@ -2640,6 +2693,24 @@ "entityType": "indexes", "table": "event" }, + { + "columns": [ + { + "value": "session_id", + "isExpression": false + }, + { + "value": "completed_at", + "isExpression": false + } + ], + "isUnique": false, + "where": null, + "origin": "manual", + "name": "goal_outcome_session_completed_idx", + "entityType": "indexes", + "table": "goal_outcome" + }, { "columns": [ { @@ -2910,6 +2981,20 @@ "entityType": "indexes", "table": "session" }, + { + "columns": [ + { + "value": "directory", + "isExpression": false + } + ], + "isUnique": false, + "where": null, + "origin": "manual", + "name": "session_directory_idx", + "entityType": "indexes", + "table": "session" + }, { "columns": [ { diff --git a/packages/core/src/database/migration.gen.ts b/packages/core/src/database/migration.gen.ts index aaf56d9868..ddffdb838b 100644 --- a/packages/core/src/database/migration.gen.ts +++ b/packages/core/src/database/migration.gen.ts @@ -52,5 +52,6 @@ export const migrations = ( import("./migration/20260805094941_workflow_node_timeout_extensions"), import("./migration/20260805094942_workflow_node_escalation_pending"), import("./migration/20260811060000_goal_outcome"), + import("./migration/20260813020344_bored_skaar"), ]) ).map((module) => module.default) satisfies DatabaseMigration.Migration[] diff --git a/packages/core/src/database/migration/20260813020344_bored_skaar.ts b/packages/core/src/database/migration/20260813020344_bored_skaar.ts new file mode 100644 index 0000000000..a20fb7ab1b --- /dev/null +++ b/packages/core/src/database/migration/20260813020344_bored_skaar.ts @@ -0,0 +1,11 @@ +import { Effect } from "effect" +import type { DatabaseMigration } from "../migration" + +export default { + id: "20260813020344_bored_skaar", + up(tx) { + return Effect.gen(function* () { + yield* tx.run(`CREATE INDEX \`session_directory_idx\` ON \`session\` (\`directory\`);`) + }) + }, +} satisfies DatabaseMigration.Migration diff --git a/packages/core/src/database/schema.gen.ts b/packages/core/src/database/schema.gen.ts index 224a2f04e8..8cc88289be 100644 --- a/packages/core/src/database/schema.gen.ts +++ b/packages/core/src/database/schema.gen.ts @@ -149,13 +149,6 @@ export default { CONSTRAINT \`fk_event_aggregate_id_event_sequence_aggregate_id_fk\` FOREIGN KEY (\`aggregate_id\`) REFERENCES \`event_sequence\`(\`aggregate_id\`) ON DELETE CASCADE ); `) - yield* tx.run(` - CREATE TABLE \`goal_state\` ( - \`session_id\` text PRIMARY KEY, - \`payload\` text NOT NULL, - \`updated_at\` integer NOT NULL - ); - `) yield* tx.run(` CREATE TABLE \`goal_outcome\` ( \`goal_id\` text PRIMARY KEY, @@ -164,6 +157,13 @@ export default { \`completed_at\` integer NOT NULL ); `) + yield* tx.run(` + CREATE TABLE \`goal_state\` ( + \`session_id\` text PRIMARY KEY, + \`payload\` text NOT NULL, + \`updated_at\` integer NOT NULL + ); + `) yield* tx.run(` CREATE TABLE \`permission\` ( \`id\` text PRIMARY KEY, @@ -331,8 +331,10 @@ export default { ) yield* tx.run(`CREATE UNIQUE INDEX \`event_aggregate_seq_idx\` ON \`event\` (\`aggregate_id\`,\`seq\`);`) yield* tx.run(`CREATE INDEX \`event_aggregate_type_seq_idx\` ON \`event\` (\`aggregate_id\`,\`type\`,\`seq\`);`) + yield* tx.run( + `CREATE INDEX \`goal_outcome_session_completed_idx\` ON \`goal_outcome\` (\`session_id\`,\`completed_at\`);`, + ) yield* tx.run(`CREATE INDEX \`goal_state_updated_at_idx\` ON \`goal_state\` (\`updated_at\`);`) - yield* tx.run(`CREATE INDEX \`goal_outcome_session_completed_idx\` ON \`goal_outcome\` (\`session_id\`, \`completed_at\`);`) yield* tx.run( `CREATE UNIQUE INDEX \`permission_project_action_resource_idx\` ON \`permission\` (\`project_id\`,\`action\`,\`resource\`);`, ) @@ -363,6 +365,7 @@ export default { yield* tx.run(`CREATE INDEX \`session_project_idx\` ON \`session\` (\`project_id\`);`) yield* tx.run(`CREATE INDEX \`session_workspace_idx\` ON \`session\` (\`workspace_id\`);`) yield* tx.run(`CREATE INDEX \`session_parent_idx\` ON \`session\` (\`parent_id\`);`) + yield* tx.run(`CREATE INDEX \`session_directory_idx\` ON \`session\` (\`directory\`);`) yield* tx.run(`CREATE INDEX \`todo_session_idx\` ON \`todo\` (\`session_id\`);`) }) }, diff --git a/packages/core/src/session/sql.ts b/packages/core/src/session/sql.ts index 264a1d2cca..a7ce8df496 100644 --- a/packages/core/src/session/sql.ts +++ b/packages/core/src/session/sql.ts @@ -62,6 +62,9 @@ export const SessionTable = sqliteTable( index("session_project_idx").on(table.project_id), index("session_workspace_idx").on(table.workspace_id), index("session_parent_idx").on(table.parent_id), + // GOAL-FP-01-04 (S-1): the GoalLoop startup scan joins goal_state → + // session and filters on session.directory on every instance boot. + index("session_directory_idx").on(table.directory), ], ) diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index b43ebd294c..792d130d75 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -153,6 +153,20 @@ const serviceLayer = Layer.effect( // Defect, so ANY query failure degrades to no-scan + a log and can // never kill the builder — which would close the ScopedCache entry // scope and take the idle subscription down with it. + // + // S-2: defensive read. The ref being set before the first state get + // is an invariant of the init→builder chain, not of the type system — + // if any future path builds this state without init setting the ref + // first, scanning with the empty value would silently match no + // session (a quiet no-op that looks healthy). Fail LOUD instead: + // log an error and skip the scan. The idle subscription above stays + // armed either way, so the event-driven path is unaffected. + if (!scanDirectoryRef.current) { + yield* Effect.logError( + "goal startup scan skipped: instance directory not resolved before state build", + ) + return {} + } const snapshot = yield* goal.listActiveSessions(scanDirectoryRef.current).pipe( Effect.catchCause((cause) => { const empty: ReadonlyArray = [] diff --git a/packages/opencode/src/session/automation-lease.ts b/packages/opencode/src/session/automation-lease.ts index ac0fd00012..ce839fcef3 100644 --- a/packages/opencode/src/session/automation-lease.ts +++ b/packages/opencode/src/session/automation-lease.ts @@ -31,8 +31,16 @@ export interface Interface { export class Service extends Context.Service()("@opencode/SessionAutomationLease") {} -export const layer = Layer.sync(Service, () => { - const locks = KeyedMutex.makeUnsafe() +// S-3: SessionStatus is a HARD requirement of the lease layer. The +// dag-release re-trigger (GOAL-FP-01-02) must never silently degrade — a +// busy session's turn always re-emits idle when it finishes, so the +// re-trigger needs the real status map to gate and emit. SessionStatus is +// lightweight and dependency-free, so this adds no cycle. +export const layer = Layer.effect( + Service, + Effect.gen(function* () { + const sessionStatus = yield* SessionStatus.Service + const locks = KeyedMutex.makeUnsafe() const registrations = new Map< SessionID, { readonly goals: Set; readonly dags: Set; generation: number } @@ -124,22 +132,15 @@ export const layer = Layer.sync(Service, () => { }), ) if (!goalRetryDue) return - // SessionStatus is resolved optionally: automation-lease is deliberately - // dependency-free (consumers wire it standalone, e.g. - // test/session/automation-lease.test.ts), and every entry point that runs - // the lease (AppLayer, DagLoop, GoalLoop) provides SessionStatus. Without - // it the re-trigger degrades to the pre-fix behavior (the caller's next - // idle event still drives the goal — claim re-evaluation is never - // load-bearing for correctness of the lease itself). - const status = yield* Effect.serviceOption(SessionStatus.Service) - if (Option.isNone(status)) return // Only re-trigger when the session is actually idle: a busy session's // turn ALWAYS re-emits idle when it finishes (runner onIdle → // SessionStatus.set), which re-drives the goal claim with the dag already - // released. Emitting here mid-turn would waste a judge call and transiently - // drop the busy entry from the status map. - if ((yield* status.value.get(sessionID)).type !== "idle") return - yield* status.value.set(sessionID, { type: "idle" }) + // released. Emitting here mid-turn would waste a judge call and + // transiently drop the busy entry from the status map. SessionStatus is + // a hard requirement of the lease layer (S-3), so this gate can never + // silently degrade to a dropped re-trigger. + if ((yield* sessionStatus.get(sessionID)).type !== "idle") return + yield* sessionStatus.set(sessionID, { type: "idle" }) }) const claim = Effect.fn("SessionAutomationLease.claim")(function* ( @@ -200,7 +201,8 @@ export const layer = Layer.sync(Service, () => { }) return Service.of({ register, unregister, claim, use, purgeSession }) -}) + }), +) -export const defaultLayer = layer -export const node = LayerNode.make(layer, []) +export const defaultLayer = layer.pipe(Layer.provide(SessionStatus.defaultLayer)) +export const node = LayerNode.make(layer, [SessionStatus.node]) diff --git a/packages/opencode/src/session/session.ts b/packages/opencode/src/session/session.ts index eeb747f578..27789bc6bb 100644 --- a/packages/opencode/src/session/session.ts +++ b/packages/opencode/src/session/session.ts @@ -46,6 +46,7 @@ import { ModelV2 } from "@opencode-ai/core/model" import { SessionMessageID } from "@opencode-ai/schema/session-message-id" import { Goal } from "@/goal/goal" import { SessionAutomationLease } from "./automation-lease" +import { SessionStatus } from "./status" import { Dag } from "@/dag/dag" import { isWorkflowTerminalStatus } from "@opencode-ai/core/dag/core/types" import { landSystemMessages } from "@/hook/trigger-result" @@ -1200,6 +1201,10 @@ export const node = LayerNode.make(layer, [ EventV2Bridge.node, Goal.node, SessionAutomationLease.node, + // S-3: the lease node now requires SessionStatus (hard requirement for + // the dag-release re-trigger); consumers listing the lease node must + // provide it or the wiring fails silently. + SessionStatus.node, Dag.node, ]) diff --git a/packages/opencode/test/session/automation-lease.test.ts b/packages/opencode/test/session/automation-lease.test.ts index 59a080268c..a40595d766 100644 --- a/packages/opencode/test/session/automation-lease.test.ts +++ b/packages/opencode/test/session/automation-lease.test.ts @@ -1,10 +1,17 @@ import { describe, expect } from "bun:test" -import { Effect, Option } from "effect" +import { Effect, Layer, Option, Schema } from "effect" import { SessionAutomationLease } from "@/session/automation-lease" import { SessionID } from "@/session/schema" -import { testEffect } from "../lib/effect" +import { SessionStatus } from "@/session/status" +import { EventV2Bridge } from "@/event-v2-bridge" +import { testEffect, pollWithTimeout } from "../lib/effect" -const it = testEffect(SessionAutomationLease.defaultLayer) +// S-3: the lease's dag-release re-trigger requires the real SessionStatus — +// the defaultLayer self-provides it, and the merged EventV2Bridge shares the +// memoized instance so the test can observe the re-triggered idle event. +const it = testEffect( + SessionAutomationLease.defaultLayer.pipe(Layer.provideMerge(EventV2Bridge.defaultLayer)), +) describe("SessionAutomationLease", () => { it.instance("DAG registration preempts Goal and invalidates its generation", () => @@ -42,4 +49,37 @@ describe("SessionAutomationLease", () => { expect(Option.isSome(yield* lease.claim(sessionID, goal))).toBe(true) }), ) + + // S-3: the dag-release re-trigger must reach the real SessionStatus and + // emit the idle status event — the re-trigger can never silently degrade + // now that SessionStatus is a hard requirement of the lease layer. + it.instance("S-3: a blocked goal claim is re-triggered through SessionStatus when the dag releases", () => + Effect.gen(function* () { + const lease = yield* SessionAutomationLease.Service + const events = yield* EventV2Bridge.Service + const idleSessions: string[] = [] + const unsubscribe = yield* events.listen((event) => + Effect.sync(() => { + // event.data is untyped on the bus — decode it with the event + // definition's data schema instead of asserting on it. + if (event.type !== SessionStatus.Event.Status.type) return + const payload = Schema.decodeUnknownSync(SessionStatus.Event.Status.data)(event.data) + if (payload.status.type === "idle") idleSessions.push(String(payload.sessionID)) + }), + ) + yield* Effect.addFinalizer(() => unsubscribe) + + const sessionID = SessionID.descending() + yield* lease.register(sessionID, { kind: "dag", id: "dag-1" }) + // A goal claim rejected by the dag records the blocked obligation. + expect(Option.isNone(yield* lease.claim(sessionID, { kind: "goal", id: "goal-1" }))).toBe(true) + + yield* lease.unregister(sessionID, { kind: "dag", id: "dag-1" }) + yield* pollWithTimeout( + Effect.sync(() => (idleSessions.includes(String(sessionID)) ? true : undefined)), + "dag release never re-triggered the idle status event", + "5 seconds", + ) + }), + ) })