Skip to content

Commit fe120a1

Browse files
jlongsterjlongster
committed
fix(core): tidy subagent report review nits
User decisions: - Rebase onto the latest dev - Tell the model a silently launched subagent won't notify it - Fix the remaining small review nits
1 parent b08e37f commit fe120a1

7 files changed

Lines changed: 76 additions & 42 deletions

File tree

‎packages/core/src/session/subagent-completion.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ export const deliver = Effect.fnUntraced(function* <E>(
5757
input: Pick<Job.Info, "status" | "output" | "error" | "notificationID"> & {
5858
recovery: Extract<Job.Recovery, { kind: "subagent" }>
5959
resume?: boolean
60-
background?: ReadonlyArray<RunningBackgroundItem>
6160
},
6261
) {
6362
if (input.status === "running") return
@@ -73,7 +72,7 @@ export const deliver = Effect.fnUntraced(function* <E>(
7372
sessionID: recovery.parentSessionID,
7473
...(input.resume === false ? { resume: false } : {}),
7574
description: recovery.description,
76-
text: `<subagent sessionID="${recovery.childSessionID}" state="${input.status}" description="${recovery.description}">\n${formatBody(body, recovery.childSessionID, input.background)}\n</subagent>`,
75+
text: `<subagent sessionID="${recovery.childSessionID}" state="${input.status}" description="${recovery.description}">\n${body}\n</subagent>`,
7776
metadata: { source: "subagent", childID: recovery.childSessionID, agent: recovery.agent, state: input.status },
7877
})
7978
if (input.notificationID) yield* jobs.completeBackground(input.notificationID)

‎packages/core/src/session/subagent-job.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,7 @@ const make = Effect.gen(function* () {
5151
recovery,
5252
background: options?.background,
5353
run: Effect.gen(function* () {
54-
// A joining period already owns its boundary; otherwise the child was admitted but not
55-
// resumed, so the latest durable sequence precedes everything this period will write.
56-
const startedSeq =
57-
(yield* execution.periodStartSeq(recovery.childSessionID)) ??
58-
(yield* Bus.latestSequence(db, recovery.childSessionID))
59-
yield* execution.resume(recovery.childSessionID).pipe(
54+
const resume = execution.resume(recovery.childSessionID).pipe(
6055
// Only an explicit caller cancel releases the child's claim. A server shutdown
6156
// interrupts this job but must leave the child resumable on the next start.
6257
Effect.onInterrupt(() =>
@@ -69,6 +64,13 @@ const make = Effect.gen(function* () {
6964
),
7065
),
7166
)
67+
if (options?.background) return yield* resume.pipe(Effect.as(""))
68+
// A joining period already owns its boundary; otherwise the child was admitted but not
69+
// resumed, so the latest durable sequence precedes everything this period will write.
70+
const startedSeq =
71+
(yield* execution.periodStartSeq(recovery.childSessionID)) ??
72+
(yield* Bus.latestSequence(db, recovery.childSessionID))
73+
yield* resume
7274
const assistant = yield* store.lastAssistantAfter({
7375
sessionID: recovery.childSessionID,
7476
afterSeq: startedSeq,

‎packages/core/src/tool/plugin/subagent.ts‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,17 @@ import { SubagentMetadata } from "../../session/subagent-metadata.js"
1717

1818
export const name = "subagent"
1919

20-
const backgroundResult = (sessionID: SessionSchema.ID) => ({
20+
const backgroundResult = (sessionID: SessionSchema.ID, notify: boolean) => ({
2121
sessionID,
2222
status: "running" as const,
23-
output: [
24-
`The subagent is working in the background (sessionID: ${sessionID}). You will be notified automatically when it finishes.`,
25-
"DO NOT sleep, poll for progress, ask the subagent for status, or duplicate this subagent's work; avoid working with the same files or topics it is using.",
26-
"Work on non-overlapping tasks, or briefly tell the user what you launched and end your response.",
27-
"To send it follow-ups or change its instructions while it runs, call subagent again with this sessionID.",
28-
].join("\n"),
23+
output: notify
24+
? [
25+
`The subagent is working in the background (sessionID: ${sessionID}). You will be notified automatically when it finishes.`,
26+
"DO NOT sleep, poll for progress, ask the subagent for status, or duplicate this subagent's work; avoid working with the same files or topics it is using.",
27+
"Work on non-overlapping tasks, or briefly tell the user what you launched and end your response.",
28+
"To send it follow-ups or change its instructions while it runs, call subagent again with this sessionID.",
29+
].join("\n")
30+
: `The subagent is working in the background (sessionID: ${sessionID}) and will not notify you when it finishes. To check on it, call the subagent tool with this sessionID or read the session.`,
2931
})
3032

3133
export const Input = Schema.Struct({
@@ -208,6 +210,7 @@ export const Plugin = {
208210
))
209211

210212
const background = input.background === true
213+
const notify = input.notify ?? SubagentMetadata.notify(child.metadata)
211214
// A child that is not draining yet must be admitted without waking: the job's resume
212215
// owns the start, so the period cannot run before its boundary is registered.
213216
const running =
@@ -239,7 +242,7 @@ export const Plugin = {
239242
}
240243
yield* subagents.start(recovery, { background })
241244

242-
if (background) return backgroundResult(child.id)
245+
if (background) return backgroundResult(child.id, notify)
243246

244247
const result = yield* jobs.block({ id: child.id, sessionID: context.sessionID }).pipe(
245248
Effect.onInterrupt(() =>
@@ -248,7 +251,7 @@ export const Plugin = {
248251
}),
249252
),
250253
)
251-
if (result?.type === "backgrounded") return backgroundResult(child.id)
254+
if (result?.type === "backgrounded") return backgroundResult(child.id, notify)
252255
// Failure surfaces keep the sessionID visible so the model can continue the child.
253256
if (result?.info.status === "error")
254257
return yield* new ToolFailure({

‎packages/core/test/subagent-completion.test.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,10 +80,9 @@ describe("SubagentCompletion", () => {
8080

8181
yield* SubagentCompletion.deliver(sessions, jobs, {
8282
status: "completed",
83-
output: "All green",
83+
output: "All green\n\nBackground work still running:\n- shell (shell_1): bun test",
8484
notificationID,
8585
recovery,
86-
background: [{ kind: "shell", sessionID: recovery.childSessionID, shellID: "shell_1", command: "bun test" }],
8786
})
8887
yield* SubagentCompletion.deliver(sessions, jobs, { status: "error", error: "boom", recovery })
8988
yield* SubagentCompletion.deliver(sessions, jobs, { status: "cancelled", recovery })

‎packages/core/test/subagent-idle-e2e.test.ts‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ import { ShellSelect } from "@opencode/core/shell/select"
4242
import { makeGlobalNode, makeLocationNode } from "@opencode/util/effect/app-node"
4343
import { LayerNode } from "@opencode/util/effect/layer-node"
4444
import { Global } from "@opencode/util/global"
45-
import { Context, Deferred, Effect, Exit, Layer, Logger, Ref, Schema, Scope } from "effect"
45+
import { Context, Deferred, Effect, Exit, Layer, Logger, Ref, Schedule, Schema, Scope } from "effect"
4646
import { tempGlobalLayer } from "./fixture/global"
4747
import { offlineModels } from "./fixture/models"
4848
import { tmpdirScoped } from "./fixture/tmpdir"
@@ -297,20 +297,20 @@ const claims = (app: { readonly database: Database.Interface }) =>
297297
Effect.map((rows) => Object.fromEntries(rows.map((row) => [row.id, row.claimed !== null]))),
298298
)
299299

300-
const eventually = <A, E, R>(
301-
effect: Effect.Effect<A, E, R>,
302-
predicate: (value: A) => boolean,
303-
): Effect.Effect<A, E, R> =>
304-
Effect.suspend(() => effect).pipe(
305-
Effect.flatMap((value) =>
306-
predicate(value) ? Effect.succeed(value) : Effect.sleep(20).pipe(Effect.andThen(eventually(effect, predicate))),
307-
),
308-
)
309-
310-
const waitForReports = (app: { readonly sessions: Session.Interface }, sessionID: Session.ID, count: number) =>
311-
eventually(deliveredReports(app, sessionID), (value) => value.length >= count).pipe(
312-
Effect.andThen(app.sessions.wait(sessionID)),
300+
const waitForReports = Effect.fnUntraced(function* (
301+
app: { readonly sessions: Session.Interface },
302+
sessionID: Session.ID,
303+
count: number,
304+
) {
305+
const reports = yield* deliveredReports(app, sessionID).pipe(
306+
Effect.repeat({ until: (reports) => reports.length >= count, times: 500, schedule: Schedule.spaced(20) }),
313307
)
308+
expect(
309+
reports.length,
310+
`Session ${sessionID}: expected at least ${count} reports, got ${reports.length}`,
311+
).toBeGreaterThanOrEqual(count)
312+
yield* app.sessions.wait(sessionID)
313+
})
314314

315315
describe("subagent idle reports end to end", () => {
316316
e2e.live(
@@ -554,10 +554,7 @@ describe("subagent idle reports end to end", () => {
554554
const report = (yield* deliveredReports(h, childID))[0]!
555555
const grandchildID = Schema.decodeUnknownSync(Session.ID)(report.metadata?.childID)
556556
expect((yield* h.sessions.get(grandchildID)).metadata).toBeUndefined()
557-
yield* eventually(h.sessions.context(childID), (messages) =>
558-
messages.some((message) => messageText(message).includes("silent child saw grandchild")),
559-
)
560-
yield* h.sessions.wait(childID)
557+
expect((yield* h.sessions.context(childID)).map(messageText)).toContain("silent child saw grandchild")
561558

562559
// The grandchild reported to the silent child, and the silent child never reported up.
563560
expect(yield* deliveredReports(h, h.parent.id)).toEqual([])

‎packages/core/test/subagent-report.test.ts‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,8 @@ import { SessionProjector } from "@opencode/core/session/projector"
2525
import { SessionMessageTable, SessionTable } from "@opencode/core/session/sql"
2626
import { SessionStore } from "@opencode/core/session/store"
2727
import { LayerNode } from "@opencode/util/effect/layer-node"
28-
import { Context, DateTime, Deferred, Effect, Exit, Layer, LayerMap, Schema, Scope } from "effect"
29-
import { eq } from "drizzle-orm"
28+
import { Cause, Context, DateTime, Deferred, Effect, Exit, Layer, LayerMap, Schema, Scope } from "effect"
29+
import { eq, sql } from "drizzle-orm"
3030
import { testEffect } from "./lib/effect"
3131

3232
const it = testEffect(
@@ -564,6 +564,30 @@ describe("SubagentReport settlement", () => {
564564
})
565565

566566
describe("SubagentReport outbox", () => {
567+
it.effect("rolls back the idle projection and claim release when the report insert fails", () =>
568+
Effect.gen(function* () {
569+
const database = yield* Database.Service
570+
const parent = sessionID("ses_rollback_parent")
571+
const child = sessionID("ses_rollback_child")
572+
yield* seedSessions(database, [parent])
573+
yield* seedSessions(database, [child], { parent_id: parent })
574+
const context = yield* buildExecution(yield* Scope.Scope, () =>
575+
seedAssistant(database, child, 100, "Answer to report").pipe(Effect.asVoid),
576+
)
577+
const execution = Context.get(context, SessionExecution.Service)
578+
yield* Effect.acquireRelease(
579+
database.db.run(sql`CREATE TRIGGER fail_report BEFORE INSERT ON job BEGIN SELECT RAISE(ABORT, 'forced'); END`),
580+
() => database.db.run(sql`DROP TRIGGER fail_report`).pipe(Effect.orDie),
581+
)
582+
583+
const exit = yield* execution.resume(child).pipe(Effect.exit)
584+
expect(Exit.isFailure(exit) && Cause.pretty(exit.cause)).toContain("forced")
585+
const row = yield* database.db.select().from(SessionTable).where(eq(SessionTable.id, child)).get()
586+
expect(row).toMatchObject({ time_suspended: expect.any(Number), time_idle: null, idle_outcome: null })
587+
expect(yield* SessionInbox.list(database.db, parent)).toEqual([])
588+
}),
589+
)
590+
567591
it.effect("commits the report and release before delivery, then acknowledges and wakes the parent", () =>
568592
Effect.gen(function* () {
569593
const database = yield* Database.Service
@@ -765,7 +789,7 @@ function text(item: SessionInbox.Info | undefined) {
765789
}
766790

767791
/** Builds the local execution layer against the harness services and a stubbed runner. */
768-
function buildExecution(scope: Scope.Closeable, drain: Drain) {
792+
function buildExecution(scope: Scope.Scope, drain: Drain) {
769793
return Effect.gen(function* () {
770794
const database = yield* Database.Service
771795
const bus = yield* Bus.Service

‎packages/core/test/tool-subagent.test.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -997,19 +997,29 @@ describe("SubagentTool reporting", () => {
997997
const jobs = yield* Job.Service
998998
const spawned = yield* fixture.call("call-silent-spawn", { background: true, notify: false })
999999
const childID = outputSessionID(spawned.metadata)
1000+
expect(spawned.content).toEqual([
1001+
{
1002+
type: "text",
1003+
text: `The subagent is working in the background (sessionID: ${childID}) and will not notify you when it finishes. To check on it, call the subagent tool with this sessionID or read the session.`,
1004+
},
1005+
])
10001006
expect((yield* fixture.sessions.get(childID)).metadata).toEqual({ "subagent.notify": false })
10011007
yield* fixture.sessions.wait(childID)
10021008
yield* fixture.sessions.wait(fixture.parent.id)
10031009
expect(yield* fixture.notices).toEqual([])
10041010

10051011
// Omitted notify keeps the child silent across follow-ups.
1006-
yield* fixture.call("call-silent-follow", { sessionID: childID, background: true })
1012+
const silent = yield* fixture.call("call-silent-follow", { sessionID: childID, background: true })
1013+
expect(silent.content).toEqual(spawned.content)
10071014
yield* fixture.sessions.wait(childID)
10081015
yield* fixture.sessions.wait(fixture.parent.id)
10091016
expect((yield* fixture.sessions.get(childID)).metadata).toEqual({ "subagent.notify": false })
10101017
expect(yield* fixture.notices).toEqual([])
10111018

1012-
yield* fixture.call("call-loud-follow", { sessionID: childID, background: true, notify: true })
1019+
const loud = yield* fixture.call("call-loud-follow", { sessionID: childID, background: true, notify: true })
1020+
expect(loud.content).toEqual([
1021+
{ type: "text", text: expect.stringContaining("You will be notified automatically when it finishes.") },
1022+
])
10131023
yield* fixture.sessions.wait(childID)
10141024
yield* fixture.sessions.wait(fixture.parent.id)
10151025
expect((yield* fixture.sessions.get(childID)).metadata).toEqual({})

0 commit comments

Comments
 (0)