Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
I couldn't confirm the goal loop works as described: in unit-test scenarios it doesn't start from /goal, gets stuck after a human message, and the round limit can be bypassed. Type checks and the existing tests pass. I didn't run it end to end, because the loop needs a real model reply. I ran the scenarios through the driver and goal service in a throwaway test built on your test setup.
Public API impact: session.usage.recorded gets a new source value, "goal-verify", in the schema and the generated client types. The model gets two new built-in tools, goal and todo. Both are additions. Code that switches over source exhaustively will need a new case.
Concerns, most important first:
- A human message stops the goal for good, and
/goal resumecan't restart it. When a user message arrives, the goal is disarmed. The comment says it picks up again "at the next idle boundary", butonSucceededreturns early for a disarmed goal, so it never does. The goal staysactive(also after a failure, a compaction failure or a restart), andresumeonly acceptspaused. In my test,resumefailed withGoal action "resume" is not allowed in phase "active". The user has to pause and then resume. - Nothing starts the first round for a user-set goal.
/goal <objective>andPOST /goalonly arm the goal; rounds are only scheduled after a successful run. The user's next message disarms the goal before that run starts, so in practice only a goal the model sets during its own turn ever runs. - The round limit can be bypassed.
roundsisn't reset when youseta new goal: a second goal in my test started atrounds: 3, so it inherits the old goal's budget. Meanwhileclearfollowed bysetdeletes the row and gives a fresh budget of 8. Because the model'sgoaltool offerssetandclear, which arm the goal, the model can start a new goal or restart its own loop without limit. So "the model cannot resume" doesn't hold in practice. Consider whethersetandclearbelong in the model's tool at all. - An empty todo list lets the verifier complete the goal (
[].every(...)is true) and stops the driver from scheduling a round (see inline). - The verifier call can stall goal handling for every session. All driver events go through one sequential stream, so one verifier request blocks goal events for every other session until it finishes.
Also, an autonomous /goal command was merged in #45379 and fully reverted in #49866 at a maintainer's request. It would be good to confirm with maintainers that they want this direction before going further.
Related: #27167
| Effect.gen(function* () { | ||
| const activation = yield* SessionGoalActivation.Service | ||
| if (event.type === "session.execution.succeeded") return yield* onSucceeded(event.data.sessionID, context) | ||
| if (event.type === "session.inbox.enqueued") { |
There was a problem hiding this comment.
This disarms the goal on any human message, but nothing re-arms it. onSucceeded returns early when the goal isn't armed, so it doesn't pick up "at the next idle boundary". The durable phase stays active, and resume rejects active, so the user has to pause and then resume. The same happens after a failure, a compaction failure or a restart. It also means a /goal set by the user is disarmed by the very next message, before any round runs. Either re-check at idle without disarming, or let resume accept an active goal that isn't armed.
| yield* verify.verify(sessionID, context) | ||
| const state = yield* goal.readState(sessionID) | ||
| if (state.goal?.phase !== "active") return | ||
| if (state.todo.every(isSettled)) return |
There was a problem hiding this comment.
[].every(isSettled) is true, so a goal with an empty todo list never gets a round and stays active forever. On the verifier side, items.every(isSettled) (goal-verify.ts line 170) lets a complete verdict finish a goal that has no items. An empty list should probably mean 'not done' in both places.
| * resumes a paused goal, so the model cannot construct the request at all. | ||
| */ | ||
| export const Input = Schema.Struct({ | ||
| action: Schema.Literals(["set", "edit", "pause", "clear"]), |
There was a problem hiding this comment.
Removing resume doesn't stop the model from restarting its own loop. set creates a new active goal and arms it, and clear followed by set deletes the goal row, which resets rounds to 0 and gives a fresh maxGoalRounds budget. (A plain set keeps the old rounds, because projectUpdated never resets it, so a new goal also inherits the previous goal's count.) Should the model be able to set or clear goals at all, or only edit and pause?
| .subscribe() | ||
| .pipe( | ||
| Stream.filter(SessionGoalDriver.isDriverEvent), | ||
| Stream.runForEach((event) => |
There was a problem hiding this comment.
Every driver event for every session goes through this one sequential runForEach, and onSucceeded waits on a full verifier request. While that request runs, goal events for other sessions (including the disarm ones) wait behind it. Consider handling each event in its own fiber, or queuing per session.
|
All four addressed in 1. Re-arm. The human-yield is now a skip, not a permanent disarm. 2. Empty todo list. Both places now require 3. Model tool. The action set is now 4. Per-session events. Tests: six files, 68 pass; the driver file is stable over |
f80a056 to
2def7fe
Compare
|
One adjustment after rebasing onto current dev: dev now has the The distinction is worth keeping: |
There was a problem hiding this comment.
Your new commits fix all four earlier points: re-arming after a human message (and resume on an active goal that isn't armed), empty todo lists, the round budget and the model's set/clear, and handling events per session. I checked each with throwaway tests on your driver test setup. The goal, todo and command tests pass (59 of 59), and the type check in packages/core passes. I didn't run the loop end to end, because it needs a real model reply.
Public API impact (all additions): goal-verify is a new source value for session.usage.recorded, and the model gets two new built-in tools, goal (edit/pause) and goal_todo. The existing todowrite tool is unchanged. Code that switches over source exhaustively will need a new case.
New concerns:
- The round limit depends on the verifier recording a result (see inline). If verification is skipped, the goal runs rounds without limit.
goalandgoal_todoare offered in every session, even when no goal is set. That makes every model request a little larger and invites the model to call them when there is nothing to work on. Ondev,todowriteis only registered whenexperimental.todois on. It would be good to gate these the same way, or only add them while the session has a goal.
packages/core/src/session/goal-driver.ts:80: Therounds >= maxGoalRoundscheck below only works ifroundsgoes up, and onlyprojectTodoVerifiedincrements it. Butverifyreturns without publishingsession.todo.verifiedwhen the session or goal lookup misses, whenselectTitlereturns nothing (for example, notitleagent or no usable model), or whenpreparefails (orElseSucceed(() => undefined)). In any of those cases this function goes on to schedule another round, and the limit never triggers. With a verifier stub that publishes nothing, 12onSucceededcalls scheduled 12 rounds, and the count stayed at 0 of 8 with the goal stillactive. Count the round in the driver when it schedules one, or haveverifyalways publish anincompleteverdict (or fail, so the driver disarms).
balabommablock-cpu
left a comment
There was a problem hiding this comment.
Ran the goal tests at 2f195bf (session-goal*.test.ts, tool-goal.test.ts, plugin/command.test.ts): 57 pass.
One product question on stopping, since the bot reviews covered the mechanics. With the re-arm change, pressing Esc during a round means "stop this round", not "stop the goal":
- Esc gives
execution.interruptedwithreason: "user", and the goal stays armed (the "deliberate interruption leaves the goal armed" test). - The user types "wait, don't touch the migrations". That disarms for the turn.
- When that turn finishes,
onSucceededre-arms and schedules round N+1 on its own.
I checked that sequence with a throwaway test on your driver setup: one round is scheduled after step 3.
That may well be the intent (Esc to steer, /goal pause to stop). But people usually press Esc because something is going wrong, so the agent starting again by itself after they reply may be a surprise. Either Esc pausing the goal, or a one-line notice after Esc ("Goal still active, continues after your next message. /goal pause to stop"), would make the rule visible. The notice is probably the smaller change and fits the indicator in #56417.
|
Thanks for testing it at You have also found the exact place where the design's intent and a user's instinct diverge. Esc is meant to stop this round, not the goal, because the goal is still what the user asked for; I would take the smaller of your two: keep Esc as stop-the-round, and say so. A one-line notice after a user interruption — "Goal still active; it continues after your next message. Making Esc pause the goal is also defensible, but it takes the one gesture people reach for when something looks wrong and makes it a durable state change, which is harder to undo than a notice. I will add the notice to the indicator work rather than this PR, so this one stays about the driver. |
There was a problem hiding this comment.
This push is a rebase onto dev. The goal driver, verifier and tools are unchanged, and the goal, command and projector tests in packages/core pass (74 of 74). The earlier points on re-arming, empty todo lists, the round budget and goal actions, and per-session event handling were already fixed in 3c9f7adb5 and are still fixed.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo.
Still open from my last review: the round limit only advances when the verifier records a result (session.todo.verified). If verify returns early (no title agent or model, or request preparation fails), rounds keep being scheduled without a limit.
New from the rebase: the regenerated packages/client/src/promise/generated/types.ts no longer declares McpClientTokenInput/McpClientTokenOutput and DebugHeapDumpInput/DebugHeapDumpOutput, but generated/client.ts still imports them, so the client package doesn't type-check (TS2305). The types were dropped in the endpoints commit d5dbb2c40 (from #55744). Re-running bun run generate after the rebase should fix it.
|
Both fixed in 1. Round budget. That makes the budget real when verification goes silent. Your throwaway case now stops: twelve 2. Tool surface. Both have tests that fail on the previous commit: the budget case expected |
There was a problem hiding this comment.
The unlimited-rounds problem is fixed. The driver now counts a round when it schedules one, so a verifier that records nothing can no longer keep the loop going. With the same throwaway test as last time, the loop stopped at 8 of 8 and moved to blocked. goal and goal_todo are now only offered to the model when the session has a goal. The goal, command and projector tests in packages/core pass (77 of 77).
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, now only while a goal exists.
One new concern: the round count is now written straight to the database instead of through an event (see inline).
| * is counted, so a goal that settled while the round was being admitted keeps its | ||
| * final count. | ||
| */ | ||
| export const recordRound = Effect.fn("SessionGoal.recordRound")(function* ( |
There was a problem hiding this comment.
Moving the count to the driver fixes the round limit. But recordRound updates session_goal.rounds directly, while everything else in session_goal and session_todo is updated by projecting durable events (Goal.Updated, Todo.Verified, ...). So the count isn't in the event log: rebuilding or replaying the tables from events loses it, and clients that follow events never see it change. The round's synthetic inbox row already carries metadata: { source: "goal", round }, so the session.inbox.enqueued projection could increment rounds in the same transaction. Alternatively, publish a small durable event for each scheduled round. Either keeps the count tied to the event log and still doesn't depend on the verifier.
There was a problem hiding this comment.
The only new commit on this branch is test-only (c6962c887). The rest of the diff came in with the rebase onto #54799 and #55744 (for example, the revision check in projectTodoUpdated), and the round driver, verifier and tools are unchanged. The goal, command and projector tests in packages/core pass (79 of 79), and nothing new needs flagging.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
This push brings back the missing generated client types, so packages/client compiles again. The goal, command and projector tests in packages/core pass (79 of 79). Thread 5 (the round count written straight to the database) is still open.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
One new problem, from the stacked #54799 commit cb6a13f2f rather than this pull request's own commits. In packages/schema/src/session-event.ts, .annotate({ identifier: "Session.Event.Durable" }) now comes after .pipe(Schema.toTaggedUnion("type")). annotate returns a plain union, so SessionEvent.Durable.cases is undefined when the code runs, and Session.log (Object.hasOwn(SessionEvent.Durable.cases, …) in packages/core/src/session.ts:272) throws on every streamed event. This is also the TS2339 error that bun run typecheck:tsc reports. test/session-log.test.ts fails 3 of 5 on this head; with only the old order restored (.annotate(...) before .pipe(Schema.toTaggedUnion("type"))), all 5 pass. Putting the old order back, probably in #54799, fixes it.
There was a problem hiding this comment.
This push only merges dev into the branch; the goal code is unchanged. The goal, command and projector tests in packages/core pass (79 of 79). The points from my last review still stand, including the Session.Event.Durable annotate order that makes Session.log fail.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
|
Fixed, and the cause was not where it looked.
The commit is gone from the branch, along with the dead Verified at the new head: On your round-count thread: the fix from the previous push stands — |
There was a problem hiding this comment.
The Session.Event.Durable annotate order is fixed: SessionEvent.Durable.cases is defined again, test/session-log.test.ts passes, and bun run typecheck:tsc in packages/core is clean. The goal, command, projector and session-log tests pass (84 of 84). Thread 5 (the round count written straight to the database) is still open.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
This push only rebases the branch onto a newer dev; the pull request's own changes are unchanged. The goal, command, projector and session-log tests in packages/core pass (84 of 84), and bun run typecheck:tsc is clean. Thread 5 (the round count written straight to the database) is still open.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
Thread 5 is addressed: the round count is now incremented when the session.inbox.enqueued event for the driver's round message is processed, in the same transaction, instead of by a direct database write. That ties the count to the event log, and it still doesn't depend on the verifier. The goal, command, projector and session-log tests in packages/core pass (86 of 86), and bun run typecheck:tsc is clean. Nothing new to raise.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
|
Done the first way you suggested: the count is now incremented by the The projection checks Two things this buys, both now tested:
On the other four threads, in case they are still counted as open: they are addressed in this branch — the idle boundary re-arms a disarmed goal unless a human message is waiting ( |
There was a problem hiding this comment.
This push changes no files: the commits were rewritten, and the code is identical to the previous head. The goal, command, projector and session-log tests in packages/core pass (86 of 86). Nothing new to raise.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
This push is a rebase onto a newer dev. The goal code and tests are unchanged; only the regenerated client types, the schema snapshot and migration.gen.ts differ, because dev added the session_allowance migration. The goal migration now comes before it in the list. That is safe for existing installs, because applied migrations are tracked by id, not by position. The goal, command, projector, session-log and database migration tests in packages/core pass (119 of 119), and bun run typecheck:tsc is clean. Nothing new to raise.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
This push only puts packages/core/schema.json back in the generator's formatting. Its diff against dev is now just the goal tables and the snapshot id; everything else is identical to the previous head. The goal, driver and database migration tests in packages/core pass (62 of 62). Nothing new to raise.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
There was a problem hiding this comment.
The import switch to @opencode/ai/effect is fine. However, the rebase left the schema snapshot out of sync with dev, so the database migration check now fails: test/database-migration.test.ts ("declared schema has no ungenerated migrations") fails, and bun script/migration.ts --check reports "Core schema has ungenerated database migrations". The other 105 goal, command, projector, session-log and migration tests in packages/core pass. All earlier threads remain addressed.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
| "autoincrement": false, | ||
| "default": "0", | ||
| "generated": null, | ||
| "name": "allowance", |
There was a problem hiding this comment.
This snapshot still has the session_v2.allowance column. On the new base, dev drops it in 20261024175301_drop_session_allowance, and the previous snapshot (76109fbf…), which this one chains from, has no allowance entry. As a result, bun script/migration.ts --check reports "Core schema has ungenerated database migrations", and DatabaseMigration > declared schema has no ungenerated migrations fails. Regenerating the goal snapshot on the new base with bun script/migration.ts, probably in the #54799 commit where it is introduced, should fix it.
The durable half of a session goal: a schema contract, session_goal / session_todo tables, four durable events, their projections and the read model. No endpoints, no UI, no round driver. The projections trust the event log's order: an update upserts the row wholesale, and a different goal id clears the previous todo list instead of being skipped. A clear deletes by (session_id, goal_id). The todo list is replaced wholesale in one transaction, and a verification applies verdicts by position, rejecting a verdict whose revision moved since it was taken. Round counting and the revision compare-and-set belong to the endpoints and the driver that use them.
A session.todo.updated event carries the goal revision a verification is judged against, so an event that does not advance it could rewind the goal onto a number an already-stale verdict carries; an event with no goal row wrote a list no verdict could ever match. Both are dropped now. Also drops the unrelated httpapi-codegen change (stripBrand and the Import artifact filter). The generated client stays as committed; regenerating from a clean install is left to the maintainer.
Add the session goal and todo endpoints, the /goal command, and the user's cancel authority. pause commits the durable phase and the process-local disarm separately, then interrupts the running turn with reason user. The round driver, verifier and model tools land in a follow-up.
Re-arm at the idle boundary instead of staying disarmed after a human message, and let resume accept an active goal that is not armed. Treat an empty todo list as not done in both the driver and the verifier. Restrict the model's goal tool to edit and pause, and reset rounds when a new goal is set. Handle driver events per session so a verifier request cannot block another session's disarm.
There was a problem hiding this comment.
The schema snapshot is fixed. After the rebase onto dev at v2.2.0, packages/core/schema.json no longer has the session_v2.allowance column, and the database migration test (including "declared schema has no ungenerated migrations") passes. The goal, command, projector, session-log and migration tests in packages/core pass (116 of 116), and bun run typecheck:tsc is clean in packages/core and packages/client. Nothing new to raise.
Public API impact is the same as before, all additions: goal-verify is a new session.usage.recorded source value, and the model gets two new built-in tools, goal and goal_todo, only while a goal exists.
|
Closing: #59437 landed a |
Issue for this PR
Part of the goal/todo series. Stacked on #55744 (the endpoints) and #54799 (the durable contracts) — review those first.
Type of change
What does this PR do?
Adds the goal round driver, the independent verification round, and the model-facing tools.
No new loop. A round is one synthetic inbox row plus one
SessionExecution.wake. The only place a round is scheduled is thesession.execution.succeededsubscription, and it re-checks before scheduling: the process-local arm, the durable phase, the verifier's verdict, the todo gate,rounds < maxGoalRounds, and no human item waiting. The upper bound ismaxGoalRounds(default 8).The verifier is a second, independent LLM request — the shape of
SessionTitle.generate: no conversation history, its own prompt entry, its ownUsageRecorded{source:"goal-verify"}. Its input is only three kinds of evidence: the session's file diff, the results of finished tool calls (exit code and output), and the todo rows'acceptancetext. The assistant's prose never enters. The output is decoded from onellm.stream; anything that does not decode isincomplete(fail-closed).completedis the verifier's alone. The model's todo tool cannot writecompleted, and the gate is in code:todo.every(s => s === "completed" || s === "cancelled").The model cannot resume. The goal tool's
actionset isset|edit|pause|clear—resumeis not constructible.Fail-closed disarm list: execution failure, shutdown interruption, a verifier throw, session move/fork, compaction failure.
armedis process-local and never persisted, so a restart is always disarmed.Notes
SessionGoalVerifyis a global node withverify(sessionID, context)parameter injection — puttingSessionContext.nodein its requirements makesInstance.Servicesself-referential.Session.inbox()rather thannextPromotable, becausenextPromotableonly returns the head and would miss a user row queued behind a steer.armedis process-local.How did you verify your code works?
bun typecheckinpackages/core,packages/protocol,packages/server: exit 0.session-goal-verify,tool-goal,session-goal-driver): 29 pass.session-goal,plugin/command,session-projector,session-title): 48 pass.Screenshots / recordings
n/a
Checklist
Added by the opencode bot (AI-generated).
Related: #27167
Public API: adds
goal-verifyto thesession.usage.recordedsource values and the built-ingoal/goal_todomodel tools, offered only while a goal exists (backwards compatible apart from exhaustive switches onsource)