From fabee483f7a20c700c14bdb6dccc8ba78c22bc0e Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 16 Aug 2026 06:19:02 +0800 Subject: [PATCH 1/2] fix(hook): apply prompt-hook timeout, surface dropped HookCommand fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue #286 — HookCommand schema accepted fields that every executor dropped: - promptHandler now applies entry.timeout (same idiom as command/mcp/http; the header doc promised timeout for every hook type but prompt was the only executor never enforcing it — expiry degrades to the existing non-blocking warn) - detectUnsupportedFields now flags allowedEnvVars / statusMessage / per-command once (zero consumers; only entry-level _sessionEntry?.once is read) so configs warn instead of silently no-op; timeout stays unflagged (now fully honored) Test: red-first in test/hook/warn-unsupported.test.ts (flagging), 146 hook tests green. --- packages/opencode/src/hook/settings.ts | 21 ++++++++++++++++++- .../test/hook/warn-unsupported.test.ts | 12 +++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/hook/settings.ts b/packages/opencode/src/hook/settings.ts index 44b67c0983..89dd6c587e 100644 --- a/packages/opencode/src/hook/settings.ts +++ b/packages/opencode/src/hook/settings.ts @@ -968,6 +968,16 @@ export function detectUnsupportedFields( for (const m of matchers) { for (const h of m.hooks ?? []) { if (h.shell !== undefined) unsupported.push({ field: "shell", value: h.shell, eventName }) + // issue #286 — schema-accepted but executor-dropped fields. Surfaced + // here instead of silently swallowed: allowedEnvVars/statusMessage have + // zero consumers anywhere; per-command `once` is never read (only the + // entry-level _sessionEntry?.once is consumed). `timeout` is NOT + // flagged — every handler type applies it (incl. prompt). + if (h.allowedEnvVars !== undefined) + unsupported.push({ field: "allowedEnvVars", value: h.allowedEnvVars, eventName }) + if (h.statusMessage !== undefined) + unsupported.push({ field: "statusMessage", value: h.statusMessage, eventName }) + if (h.once !== undefined) unsupported.push({ field: "once", value: h.once, eventName }) // `if` is implemented (condition-filter); async/asyncRewake implemented. // All 5 known types now have handlers; type-level unsupported set is empty by design. } @@ -1537,10 +1547,19 @@ const promptHandler: HookHandler = { schema: HookJSONOutputZodSchema, } satisfies Parameters[0] + // issue #286 — the header doc promises `timeout` for every hook type, + // but promptHandler was the only executor never applying it. Honor it + // with the same idiom as command/mcp/http; on expiry the TimeoutException + // is captured by Effect.exit below and degrades to the non-blocking warn. + const timeoutMs = entry.timeout ? entry.timeout * 1000 : DEFAULT_TIMEOUT_MS + const llmExit = yield* Effect.tryPromise({ try: () => generateObject(params).then((r) => r.object), catch: (e) => e, - }).pipe(Effect.exit) + }).pipe( + Effect.timeout(timeoutMs), + Effect.exit, + ) if (llmExit._tag === "Failure") { log.warn("prompt hook failed (non-blocking)", { error: String(llmExit.cause) }) diff --git a/packages/opencode/test/hook/warn-unsupported.test.ts b/packages/opencode/test/hook/warn-unsupported.test.ts index f258cf6682..23ea587871 100644 --- a/packages/opencode/test/hook/warn-unsupported.test.ts +++ b/packages/opencode/test/hook/warn-unsupported.test.ts @@ -38,4 +38,16 @@ describe("detectUnsupportedFields", () => { expect(detectUnsupportedFields(undefined)).toEqual([]) expect(detectUnsupportedFields({})).toEqual([]) }) + + // GOAL-FP/issue #286: HookCommand fields accepted by the schema but dropped + // by every executor must be surfaced, not silently swallowed. `timeout` for + // type "prompt" is implemented (excluded here); allowedEnvVars/statusMessage + // have zero consumers anywhere, and per-command `once` is never read (only + // the entry-level _sessionEntry?.once is consumed). + test("allowedEnvVars / statusMessage / per-command once are flagged (dropped by executors)", () => { + const unsupported = detectUnsupportedFields( + hooks({ allowedEnvVars: ["FOO"], statusMessage: "hi", once: true }), + ) + expect(unsupported.map((u) => u.field).sort()).toEqual(["allowedEnvVars", "once", "statusMessage"]) + }) }) From 11597a05dd7153a807ad11d776cdf4ec827163e2 Mon Sep 17 00:00:00 2001 From: Lex Date: Sun, 16 Aug 2026 06:33:10 +0800 Subject: [PATCH 2/2] fix(goal): durable boundary gate stops crash-recovery turn inflation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit issue #285 — after a crash, the boot scan could re-judge the exact boundary the crashed process already judged and committed: evaluatedRevisions is process-local, so both dedup gates let the stale boundary through, and updateAfterJudge inflated turns_used and dispatched a duplicate continuation. - GoalState.Info gains last_judged_msg; updateAfterJudge(judged) persists the judged assistant message id on every continue commit (same transition, atomic) - afterIdle scan path: while the session window still ends on last_judged_msg, no new progress has landed — skip re-evaluation. Live idle events are never gated: each dispatched continuation produces a fresh assistant message, so the live path always judges a new boundary; resume/subgoal edits re-drive through live turns, not the scan. - crash between continue commit and continuation dispatch: the gate skips (avoids inflation + duplicate); recovering the lost dispatch remains the separate explicit design noted in goal CONTEXT.md. Tests (red-first): unchanged-boundary scan commits nothing (turns stay 1, judge 0 calls); advanced-boundary scan proceeds (turns 2, continuation dispatched). --- packages/opencode/src/goal/goal.ts | 8 + packages/opencode/src/goal/loop.ts | 21 +++ packages/opencode/src/goal/state.ts | 6 + packages/opencode/test/goal/e2e-loop.test.ts | 145 ++++++++++++++++++- 4 files changed, 178 insertions(+), 2 deletions(-) diff --git a/packages/opencode/src/goal/goal.ts b/packages/opencode/src/goal/goal.ts index dc3fbe8fb4..1fd6c07f96 100644 --- a/packages/opencode/src/goal/goal.ts +++ b/packages/opencode/src/goal/goal.ts @@ -88,6 +88,11 @@ export interface Interface { * 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 }, + /** issue #285: the assistant message ID judged for this evaluation. + * Persisted on continue commits as the DURABLE crash-recovery gate — the + * boot scan skips a window still ending on this boundary (the + * process-local evaluatedRevisions map cannot survive a crash). */ + judged?: string, ) => Effect.Effect< | { state: GoalState.Info @@ -678,6 +683,7 @@ const serviceLayer = Layer.effect( reason: string, parseFailed: boolean, expected: { readonly goalID: string; readonly revision: number }, + judged?: string, ) { return yield* transition(sessionID, (state) => { if (!state || state.status !== "active" || !matchesExpected(state, expected)) @@ -739,6 +745,8 @@ const serviceLayer = Layer.effect( last_reason: reason, paused_reason: pauseReason, consecutive_parse_failures: GoalState.nni(newParseFailures), + // issue #285: record the judged boundary for the durable scan gate. + ...(judged !== undefined ? { last_judged_msg: judged } : {}), }) return { tag: "save", diff --git a/packages/opencode/src/goal/loop.ts b/packages/opencode/src/goal/loop.ts index 18ff08f635..f6a55ecd70 100644 --- a/packages/opencode/src/goal/loop.ts +++ b/packages/opencode/src/goal/loop.ts @@ -242,6 +242,26 @@ const serviceLayer = Layer.effect( // 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 + // issue #285 — durable boundary gate (scan path only). The + // evaluatedRevisions map above is process-local and dies with the + // process; the goal row's last_judged_msg is the crash-surviving record + // of which boundary was already judged and committed. While the session + // window still ends on that same message, no new progress has landed — + // re-judging would inflate turns_used and dispatch a duplicate + // continuation. Live idle events are never gated here: every dispatched + // continuation produces a fresh assistant message, so the live path + // always judges a new boundary. + if (scanResume && goalState.last_judged_msg) { + const win = yield* sessions + .messages({ sessionID, limit: 20 }) + .pipe( + Effect.catchIf((e) => NotFoundError.isInstance(e), () => + Effect.succeed([] as SessionV1.WithParts[]), + ), + ) + const lastSeen = [...win].reverse().find((m) => m.info.role === "assistant") + if (lastSeen && lastSeen.info.id === goalState.last_judged_msg) 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)) @@ -363,6 +383,7 @@ const serviceLayer = Layer.effect( goalID: goalState.goal_id ?? "legacy", revision: goalState.revision ?? 0, }, + lastAssistant.info.id, ), ), ) diff --git a/packages/opencode/src/goal/state.ts b/packages/opencode/src/goal/state.ts index 21326c2f07..7734c104fe 100644 --- a/packages/opencode/src/goal/state.ts +++ b/packages/opencode/src/goal/state.ts @@ -28,6 +28,11 @@ export class Info extends Schema.Class("GoalState")({ last_verdict: Schema.optional(Verdict), last_reason: Schema.optional(Schema.String), paused_reason: Schema.optional(Schema.String), + // issue #285 — the assistant message ID of the boundary judged by the last + // committed continue evaluation. Durable crash-recovery gate: the boot scan + // must not re-judge a window that still ends on this message (the + // process-local evaluatedRevisions map dies with the process). + last_judged_msg: Schema.optional(Schema.String), consecutive_parse_failures: NonNegativeInt, subgoals: Schema.Array(Schema.String).pipe(Schema.optional, Schema.withDecodingDefault(Effect.succeed([] as ReadonlyArray))), }) {} @@ -54,6 +59,7 @@ export function advance(state: Info, patch: Partial>) { last_verdict: state.last_verdict, last_reason: state.last_reason, paused_reason: state.paused_reason, + last_judged_msg: state.last_judged_msg, consecutive_parse_failures: state.consecutive_parse_failures, subgoals: state.subgoals, ...patch, diff --git a/packages/opencode/test/goal/e2e-loop.test.ts b/packages/opencode/test/goal/e2e-loop.test.ts index f9cc5cfc65..0ba2cf7eb9 100644 --- a/packages/opencode/test/goal/e2e-loop.test.ts +++ b/packages/opencode/test/goal/e2e-loop.test.ts @@ -51,9 +51,9 @@ const assistantText = "I have made progress on the feature." // Each scenario yields one scheduler turn before its first idle publish so that // fiber can acquire the PubSub subscription. No business event exists yet, so // outcome completion remains separately observed through public state/events. -const mkAssistant = () => +const mkAssistant = (id?: string) => ({ - info: { role: "assistant", time: { created: Date.now() } }, + info: { id, role: "assistant", time: { created: Date.now() } }, parts: [{ type: "text", text: assistantText }], }) as never @@ -1793,3 +1793,144 @@ describe("GoalLoop — judge-chain defect degrades into the parse budget (GOAL-F }), ) }) + +// issue #285 / GOAL-FP-01-21: the boot scan must not re-judge a boundary the +// crashed process already judged and committed. The process-local +// evaluatedRevisions map dies with the process, so the DURABLE gate is the +// goal row's last_judged_msg: updateAfterJudge records the judged assistant +// message ID on every continue commit, and the scan path skips evaluation +// while the session window still ends on that same message. Live idle events +// are never gated (each dispatched continuation produces a fresh assistant +// message, so the live path always sees a new boundary). +describe("GoalLoop — boot scan must not re-evaluate an already-judged boundary (issue #285)", () => { + let judgeCalls = 0 + let continuationCalls = 0 + let boundaryID = "msg_boundary_a" + const reset = () => { + judgeCalls = 0 + continuationCalls = 0 + boundaryID = "msg_boundary_a" + } + + const sessionMock = Layer.mock(Session.Service, { + messages: () => Effect.succeed([mkAssistant(boundaryID)]), + }) + const promptMock = Layer.mock(SessionPrompt.Service, withIdleAdmission({ + 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({ verdict: "continue", reason: "more work" }) + }), + }), + ) + + const boundaryLayer = 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(boundaryLayer) + + 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(), + }) + }) + + // Commits one continue evaluation ahead of the (re)boot — models a process + // that crashed right after the commit, before the continuation produced an + // assistant message. + const commitPriorBoundary = (sid: SessionID) => + Effect.gen(function* () { + const goal = yield* Goal.Service + const before = yield* goal.load(sid) + const result = yield* goal.updateAfterJudge( + sid, + "continue", + "pre-crash commit", + false, + { goalID: before!.goal_id ?? "legacy", revision: before!.revision ?? 0 }, + boundaryID, + ) + expect(result?.state.turns_used).toBe(1) + return result?.state + }) + + it.instance("scan with an unchanged boundary skips re-evaluation (no inflation)", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const directory = (yield* TestInstance).directory + const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) + yield* goal.set(sid, "boundary guard", 5) + yield* commitPriorBoundary(sid) + + yield* loop.init() + // Negative assertion: the scan runs in a forked fiber with no readiness + // signal on the skip path, so a bounded wait stands in for polling. + yield* Effect.sleep("300 millis") + expect(judgeCalls).toBe(0) + expect(continuationCalls).toBe(0) + const g = yield* goal.load(sid) + expect(g?.turns_used).toBe(1) + }), + ) + + it.instance("scan proceeds once the window advanced past the boundary", () => + Effect.gen(function* () { + reset() + const loop = yield* GoalLoop.Service + const goal = yield* Goal.Service + const directory = (yield* TestInstance).directory + const sid = SessionID.descending() + yield* seedSessionRow(sid, directory) + yield* goal.set(sid, "boundary guard", 5) + yield* commitPriorBoundary(sid) + // The continuation finished and produced a NEW assistant message. + boundaryID = "msg_boundary_b" + + yield* loop.init() + yield* pollWithTimeout( + Effect.sync(() => (judgeCalls >= 1 ? true : undefined)), + "scan never re-evaluated the advanced boundary", + "5 seconds", + ) + const g = yield* goal.load(sid) + expect(g?.turns_used).toBe(2) + expect(continuationCalls).toBe(1) + }), + ) +})