Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The four new events are projected as described: the new session-goal tests and the event-manifest test pass, and packages/core and packages/schema type-check. packages/client no longer type-checks, though. The new events are now part of the session log stream, but the generated client types (packages/client/src/effect/api/api.ts, src/promise/generated/types.ts) were not regenerated, so src/effect/generated/client.ts fails with TS2345. On the base it type-checks cleanly. Please run bun run generate in packages/client and commit the output.
Public API (all additive, backwards compatible): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1 in the public event manifest and the session log stream; a new SessionGoal export from @opencode/schema (ID, Phase, Reason, Snapshot, TodoStatus, TodoItem, TodoInput, Verdict, TodoVerification, State). Once released, these event names and payloads are a durable contract, so any later change to them needs a version bump. No endpoints, config or CLI changes.
Other concerns, tied to lines inline: the revision guard ignores the goal id; todos and the round counter are not tied to a goal and survive a clear; verification by position can apply to a different list than the one that was judged; State (with the process-local armed) is defined but nothing builds it yet.
For maintainers: this adds tables and durable events with no producer or consumer yet. #27167 is marked "not planned as of yet", and the earlier /goal command (#45379) was reverted in #49866, so it's worth deciding whether the series is wanted before this schema lands. Durable events and migrations are hard to take back later. The one-line Windows fix in packages/core/script/migration.ts is unrelated to the feature, and would fit better in its own pull request.
| .onConflictDoUpdate({ | ||
| target: SessionGoalTable.session_id, | ||
| set: values, | ||
| setWhere: sql`excluded.revision > ${SessionGoalTable.revision}`, |
There was a problem hiding this comment.
The guard compares only revision, not goal_id. Say the stored goal is at revision 5 and a different goal arrives with revision 1, without a cleared first. The upsert is then silently skipped, but the event is still persisted, so the log and the projection disagree. Either key the guard on goal_id too (replace when the id differs), or make it explicit that revisions are per session and increase across goals. Since the bus projects each committed event exactly once and in order, it's also worth checking whether the guard is needed at all.
Clearing a goal deletes only the goal row. The session_todo rows (and their verifier feedback) are left behind, so the next goal inherits the old list. The revision carried by session.goal.cleared is also never used. Should the todo list belong to the goal (a goal_id column, or cleared together with it)?
| }) | ||
|
|
||
| /** Apply one verifier verdict: update items by position and advance the round counter. */ | ||
| export const projectTodoVerified = Effect.fn("SessionGoal.projectTodoVerified")(function* ( |
There was a problem hiding this comment.
Verification updates items by position alone. If a session.todo.updated replaced the list after the verifier read it, the verdicts land on whatever items now sit at those positions, and out-of-range positions are silently ignored. Separately, rounds is incremented even when there is no goal row, and verdict is dropped. Carrying a reference to the list it judged (e.g. the goal revision or a list version) and rejecting stale verifications would make this safe.
|
|
||
| /** Read model. `armed` is process-local truth, mirroring `session.interrupt`'s `interrupted`. */ | ||
| export interface State extends Schema.Schema.Type<typeof State> {} | ||
| export const State = Schema.Struct({ |
There was a problem hiding this comment.
Nothing in this pull request builds State. armed is described as process-local, and rounds lives on the row but not on Snapshot. Since this is exported from @opencode/schema, I'd leave State out until the pull request that actually builds it, so the public schema doesn't get ahead of its implementation.
10a5046 to
7333ece
Compare
|
All addressed. ① ② The revision guard is gone. You were right to ask whether it was needed: the bus projects each committed event exactly once and in order, so the latest event always wins. Dropping it also removes the log/projection disagreement you described. ③ The todo list belongs to the goal. A ④ A stale verification is rejected. ⑤ ⑥ The Windows fix is out of this branch and will be its own PR. On your point for maintainers: agreed, and it is the reason this is the first of four PRs rather than one. The round driver — the only part with an autonomous loop — is isolated in its own PR so it can be rejected on its own, and this PR plus the endpoints and the desktop indicator are a self-contained feature without it. I have asked on #27167 whether the series is wanted; if the answer is no, this is the cheapest point to stop. Tests: |
ae6ef4e to
90af6df
Compare
589eff7 to
0bb40b6
Compare
0749b2d to
34bfee0
Compare
There was a problem hiding this comment.
The earlier points are addressed: the revision guard is gone and a new goal id resets the todo list, clearing a goal removes its todos, a verification is ignored when its revision is stale or there is no goal row, and State is no longer exported. The tests (9/9 and 7/7) pass, and packages/core, packages/schema and packages/client now type-check.
Public API (all additive, backwards compatible): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 (revision, todos) and session.todo.verified.1 (revision, items); a new SessionGoal export from @opencode/schema (ID, Phase, Reason, Snapshot, TodoStatus, TodoItem, TodoInput, TodoVerification); and new SessionGoal* types in the generated client.
The packages/httpapi-codegen change (stripBrand and the Import artifact filter) is not part of this feature, and with this repository's dependencies it does nothing: running bun run generate in packages/client gives identical output with and without it. That output also differs from the committed types.ts in the order of about 50 declarations, so the committed file seems to come from a different local setup (possibly another effect version, which the comment mentions). Please drop the codegen change and regenerate the client from a clean bun install. If newer effect versions really do emit brands, that fix belongs in its own pull request.
One new inline point on how session.todo.updated writes the goal revision. For maintainers, the earlier questions still stand: #27167 is "not planned as of yet", and the earlier /goal command (#45379) was reverted in #49866.
packages/core/src/session/goal.ts:90: This writes the todo event'srevisiononto the goal row with no check, so the goal revision and the list revision share one counter that either event can set. Asession.todo.updatedcarrying a lower revision than the current goal moves it backwards, and the next verification is then compared against that number. If there is no goal row, the list is still written but the revision is silently dropped, so no verification can ever match it. Please either reject a todo update whose revision doesn't advance the goal's revision (or doesn't belong to the current goal), or keep a separate list revision onsession_todo, so the stale-verdict check can't be bypassed.packages/httpapi-codegen/src/index.ts:1219: With this repository's pinned dependencies, this change (and theImportartifact filter above it) makes no difference:bun run generateinpackages/clientproduces identical output with and without it. Please drop it from this pull request and regeneratetypes.tsfrom a clean install. If brands do leak with a newereffect, a separate pull request alongside that upgrade would be the right place for it.
34bfee0 to
a3790f4
Compare
There was a problem hiding this comment.
This push is a rebase onto the latest dev. The pull request's own changes are unchanged, and the goal and todo tests and the packages/core, packages/schema and packages/client type checks still pass.
The points from my previous review are still open:
- The
packages/httpapi-codegenchange (stripBrandand theImportartifact filter) has no effect with this repository's dependencies. It should be dropped, and the client types regenerated from a clean install. session.todo.updatedwrites its revision onto the goal row without checking it (goal.tsline 90), so it can move the revision backwards and get around the stale-verdict check.
Public API impact is unchanged since the last review: four new durable events, the SessionGoal export from @opencode/schema, and matching client types, all additive.
|
Both addressed in Codegen. The One thing worth knowing: removing it makes one of that package's own tests fail on this machine — Todo revision. I took your second option: Tests: |
There was a problem hiding this comment.
The new commit fixes the todo revision point from my last review: session.todo.updated is now dropped when there is no goal, or when its revision doesn't advance the goal's. The unrelated packages/httpapi-codegen change has been removed. The goal and todo tests (11/11) and the event manifest test (7/7) pass, and packages/core, packages/schema and packages/client type-check.
The committed packages/client/src/promise/generated/types.ts still differs from what bun run generate produces from a clean install, but only in the order of about 50 declarations. Regenerating it would avoid that churn in the next client regeneration; it doesn't block anything.
Public API (all additive, backwards compatible): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types. Nothing changed since the last review apart from the documented drop rule for session.todo.updated.
983895e to
fa55d19
Compare
There was a problem hiding this comment.
This push is a rebase onto the latest dev with no changes to the pull request itself. The goal and todo tests (11/11) and the event manifest test (7/7) still pass, and packages/core, packages/schema and packages/client type-check. There are no new concerns.
Public API (all additive, backwards compatible, unchanged since the last review): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
fa55d19 to
267ffad
Compare
There was a problem hiding this comment.
The files in this push are identical to the previous head (same git tree); only the commits were rewritten. Nothing new to review, and the earlier results stand: the goal and todo tests pass, the event manifest test passes, and packages/core, packages/schema and packages/client type-check.
Public API (all additive, backwards compatible, unchanged): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
267ffad to
8de34bf
Compare
There was a problem hiding this comment.
This push rebases onto the latest dev. Apart from the merged migration list and schema snapshot, the pull request's own changes are unchanged. The goal and todo tests (11/11) and the event manifest test (7/7) pass, and packages/core, packages/schema and packages/client type-check. The merged migration order is fine: the runner skips migrations by id, so 20261012112921_modern_red_wolf still runs on databases that already have the newer session_allowance migration.
One small point: the rebased packages/core/schema.json fails prettier --check (the base's copy passes). Arrays that are written on one line elsewhere are split over several lines, which roughly doubles the file's diff, from 228 to about 411 changed lines. Running prettier on it removes that churn. This doesn't block anything.
Public API (all additive, backwards compatible, unchanged): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
There was a problem hiding this comment.
The formatting point from my last review is addressed: packages/core/schema.json now passes prettier --check, and its diff against the base is down to the real snapshot change. The commit only reformats that file, and the goal and todo tests still pass (11/11). There are no new concerns.
Public API (all additive, backwards compatible, unchanged): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
2b56f49 to
f0b158e
Compare
There was a problem hiding this comment.
The client regeneration is now consistent: bun run generate reproduces the committed types.ts exactly. The goal and todo tests (11/11) pass, and packages/core and packages/client type-check.
The rebased packages/core/schema.json brings back the session_v2.allowance column that dev just dropped (inline). Running bun run migration on this head generates an extra migration that runs ALTER TABLE session_v2 DROP COLUMN allowance, and that would fail on databases that already ran 20261024175301_drop_session_allowance. Please take schema.json from the latest dev and run bun run migration again, so the goal tables chain onto the current snapshot. A second run of bun run migration should then produce nothing.
Public API (all additive, backwards compatible, unchanged): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
| "autoincrement": false, | ||
| "default": "0", | ||
| "generated": null, | ||
| "name": "allowance", |
There was a problem hiding this comment.
This brings back session_v2.allowance, which the latest dev removed in 20261024175301_drop_session_allowance; the base's snapshot no longer has it. sql.ts doesn't declare it, so the next bun run migration generates ALTER TABLE session_v2 DROP COLUMN allowance;. I reproduced that on this head, and that migration would fail on databases that already dropped the column. Please take schema.json from the latest dev and run bun run migration again, so only the session_goal and session_todo entries are added on top of the base's snapshot.
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.
f0b158e to
6355ec6
Compare
There was a problem hiding this comment.
The schema.json problem is fixed: the dropped session_v2.allowance column is no longer in the snapshot, and running bun run migration on this head now reports no schema changes. The client regenerates without a diff, the goal and todo tests (11/11) and the event manifest test (7/7) pass, and packages/core, packages/schema and packages/client type-check. There are no new concerns.
Public API (all additive, backwards compatible, unchanged): new durable events session.goal.updated.1, session.goal.cleared.1, session.todo.updated.1 and session.todo.verified.1; the SessionGoal export from @opencode/schema; and matching client types.
|
Closing: #59437 landed a |
Issue for this PR
Refs #27167. This is the first of four PRs; it does not close the request on its own.
Type of change
What does this PR do?
The durable half of a session goal: a schema contract,
session_goal/session_todotables, four durable events, their projections and the read model. No endpoints, no UI, no round driver.packages/schema/src/session-goal.ts—Phase(active | paused | blocked | complete),Snapshot(with arevisionCAS reference andmaxGoalRounds),TodoItem/TodoInput,Verdict, and theStateread model. The goal id is server-generated and validates thegoal_prefix it emits.session_goal/session_todofollow the shape ofsession_inboxandinstruction_state:session_idwithonDelete: "cascade", JSON columns, the shared timestamps.session.goal.updated/.cleared,session.todo.updated/.verified.excluded.revision > revision), so replaying an event is idempotent; a clear deletes by(session_id, goal_id)and the id is never reused; the todo list is replaced wholesale in one transaction; a verification updates items by position and advances the round counter.Why it is split this way
The round driver is the only part with an autonomous loop, so it gets its own PR and can be rejected on its own. Even then, this PR plus the endpoints and the desktop indicator are a self-contained feature: a persisted goal, manual control, a todo list, and the indicator — and they already fix the class of defect in #48239, where the stop button did not stop the loop.
How did you verify your code works?
packages/core/test/session-goal.test.tscovers the migration and projection idempotency (replaying an event leaves the same row; a stale revision does not overwrite a newer one).packages/schema/test/event-manifest.test.tscovers the durable key list.bun testinpackages/core(31 passed across the touched suites) andbun typecheckinpackages/coreandpackages/schema.The migration is generated by
bun run migration;migration.gen.ts,schema.gen.tsandschema.jsonare its output. The one-line change inpackages/core/script/migration.tsfixes the script's path split, which crashed on Windows.Screenshots / recordings
n/a
Checklist
Added by the opencode bot (AI-generated).
Related: #27167, #48239
Public API: adds durable events session.goal.updated/cleared and session.todo.updated/verified, the SessionGoal schema export and matching client types (backwards compatible)