feat(todo): add stable session todo identity - #399
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds stable optional todo IDs across backend, storage, tooling, and frontend: DB schema and migration add ChangesSession todo ID migration & resolution
Frontend: shapes, extraction, selectors, UI, and tests
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tool as TodoTool
participant Service as TodoService
participant DB as Database
participant FE as Frontend
Tool->>Service: call todowrite(params.todos with optional ids)
Service->>Service: resolveTodoIDs(previousTodos, incomingTodos)
Service->>DB: upsert resolved todos (include ids)
DB-->>Service: persisted rows (with ids)
Service-->>Tool: return resolved todos (used for title/output/metadata)
Tool->>FE: part includes metadata.todos (with ids)
FE->>FE: extractTodos prefers metadata.todos when completed
FE->>FE: compute lifecycleSignature using ids when present
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 7/10 reviews remaining, refill in 17 minutes and 58 seconds. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces stable IDs for todo items across the database, backend services, and frontend UI to improve task tracking and lifecycle management. Key changes include a database migration adding a primary key id to the todo table, logic in the Todo service to resolve and maintain IDs for incoming tasks, and updates to the frontend to incorporate these IDs into lifecycle signatures. Feedback is provided regarding the SQL migration's ID generation method, which may conflict with the system's standard identifier format that expects timestamp metadata.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/app/src/pages/session/session-todos.test.ts (1)
29-33: Theas Todocast is imprecise but not a regression risk;idis optional in the actual output type.The
todo()helper intentionally omitsidbecause the downstream functionselectSessionTodosreturnsSessionTodoItem, which makesidoptional (Partial<Pick<Todo, "id">>). Rather than splitting fixtures, improve type safety by casting toas SessionTodoIteminstead ofas Todo, or remove the cast entirely since the object already satisfiesSessionTodoItem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/src/pages/session/session-todos.test.ts` around lines 29 - 33, The test helper todo() currently casts fixtures to Todo which is imprecise because id is intentionally omitted; change the cast to as SessionTodoItem (or remove the cast entirely) so the returned object matches the downstream selectSessionTodos output type; update the todo() helper definition (referencing todo(), Todo and SessionTodoItem and usages in selectSessionTodos tests) to either cast to SessionTodoItem or omit the cast to improve type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/session/session.sql.ts`:
- Around line 109-112: The current schema defines a non-unique index
todo_session_position_idx on table.session_id and table.position, which allows
duplicate positions per session; change this to a UNIQUE constraint by replacing
the plain index with a unique index/constraint on (session_id, position) in the
session table definition (update the expression using table.session_id and
table.position and the index identifier todo_session_position_idx or rename to
todo_session_sessionid_position_uq), and add a corresponding migration that
creates the unique constraint (and handles/cleans any existing duplicate rows
before applying) so the database enforces uniqueness after dropping the old
composite PK/id change.
In `@packages/opencode/src/session/todo.ts`:
- Around line 61-69: The current guard (if (!id && !todo.id)) skips
content-based reuse when a non-reusable todo.id is provided; change it to run
content fallback whenever id is still unset by replacing the condition with if
(!id) so the unusedPreviousByExactContent lookup can attempt to assign id for
unchanged items; keep using unusedPreviousByExactContent.get(todo.content), the
while loop that shifts candidates, and the used.has(candidate.id) check to avoid
reusing already-used IDs.
---
Nitpick comments:
In `@packages/app/src/pages/session/session-todos.test.ts`:
- Around line 29-33: The test helper todo() currently casts fixtures to Todo
which is imprecise because id is intentionally omitted; change the cast to as
SessionTodoItem (or remove the cast entirely) so the returned object matches the
downstream selectSessionTodos output type; update the todo() helper definition
(referencing todo(), Todo and SessionTodoItem and usages in selectSessionTodos
tests) to either cast to SessionTodoItem or omit the cast to improve type
safety.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba6938ac-eb33-4e17-96c3-6b002c6548bd
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (20)
packages/app/src/pages/session/composer/session-todo-dock.tsxpackages/app/src/pages/session/session-status-extractors.test.tspackages/app/src/pages/session/session-status-extractors.tspackages/app/src/pages/session/session-todos.test.tspackages/app/src/pages/session/session-todos.tspackages/app/src/pages/session/todos/todo-dock-machine.test.tspackages/app/src/pages/session/todos/todo-model.test.tspackages/app/src/pages/session/todos/todo-model.tspackages/app/src/pages/session/todos/todo-source.test.tspackages/app/src/pages/session/todos/todo-source.tspackages/app/src/pages/session/todos/use-session-todos.tspackages/opencode/migration/20260503025430_todo_ids/migration.sqlpackages/opencode/migration/20260503025430_todo_ids/snapshot.jsonpackages/opencode/src/id/id.tspackages/opencode/src/session/schema.tspackages/opencode/src/session/session.sql.tspackages/opencode/src/session/todo.tspackages/opencode/src/tool/todo.tspackages/opencode/src/tool/todowrite.txtpackages/opencode/test/session/todo.test.ts
💤 Files with no reviewable changes (1)
- packages/app/src/pages/session/session-todos.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/storage/json-migration.ts`:
- Around line 331-333: The fallback todo.id generation is non-deterministic
because JsonMigration currently calls TodoID.ascending() with no seed, producing
a new ID each run; change the fallback in the values push so that when todo.id
is missing you derive a stable ID from the legacy identity (for example use
TodoID.ascending(`${todo.session_id}:${todo.position}`) or another deterministic
combination of the legacy fields) or look up and reuse an already-migrated row
for that (session_id, position) before creating a new id; update the branch that
currently checks typeof todo.id and todo.id.startsWith("todo_") to use the
deterministic seed or reuse logic so repeated runs are idempotent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 87a9f995-c491-4e78-aeed-6381a0dcb892
📒 Files selected for processing (2)
packages/opencode/src/storage/json-migration.tspackages/opencode/test/storage/json-migration.test.ts
Astro-Han
force-pushed
the
codex/i395-todo-identity
branch
from
May 3, 2026 03:26
00b9446 to
b7326f5
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/storage/json-migration.ts (1)
319-343:⚠️ Potential issue | 🟠 MajorSort todo files to ensure deterministic migration output.
Line 340–343 resolves duplicate
todo_...IDs by keeping the first occurrence seen. SincetodoFilescomes directly fromGlob.scan()without explicit sorting, the outcome depends on filesystem traversal order—which is not guaranteed to be consistent across different systems or even repeated runs. This causes migration results to vary for identical input data.Sort the todo files before processing (e.g.,
todoFiles.sort()) to make the migration output deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/storage/json-migration.ts` around lines 319 - 343, The migration iterates todoFiles (used in the batch loop and referenced when computing sessionID and legacyTodoID) without a stable order, so duplicate todo_... IDs are resolved non-deterministically; sort the todoFiles array before building todoSessions / starting the batch loop (i.e., call sort() on todoFiles early in the function that contains seenTodoIDs, todoSessions, and the for-loop) so processing order is deterministic and the seenTodoIDs de-duplication yields consistent migration output.
🧹 Nitpick comments (1)
packages/opencode/test/session/todo.test.ts (1)
54-115: ⚡ Quick winUse the Effect test helper here.
These tests exercise Effect services, so
testEffect(...)would remove the manualInstance.provide/Effect.runPromiseplumbing and keep the harness consistent.As per coding guidelines, use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/session/todo.test.ts` around lines 54 - 115, Replace the manual Instance.provide / Effect.runPromise plumbing in both tests with the testEffect helper from test/lib/effect.ts: wrap each test body with testEffect(...) so you can call effectful operations directly (e.g., Session.create, Todo.Service.use, Todo.defaultLayer) without Instance.provide and Effect.runPromise; update the two tests ("update returns ids and get persists them" and "second update preserves id and persists status changes") to use testEffect and drop explicit Instance.provide, Effect.runPromise, and tmpdir usage inside the provide block while still calling Session.remove at the end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/session/todo.test.ts`:
- Around line 39-50: The test only checks that the unknown/duplicate resolved
ids are not equal to previous[0].id, which misses regressions that reuse other
previous ids; update the assertions in the "ignores unknown and duplicate ids"
test (around Todo.resolveTodoIDs and the resolved/previous variables) to assert
that resolved[0].id and resolved[2].id are not included in the full set of
previous ids (e.g., compare against previous.map(p => p.id) or similar) instead
of only checking previous[0].id, ensuring unknown/duplicate branches are
validated against all previous IDs.
---
Outside diff comments:
In `@packages/opencode/src/storage/json-migration.ts`:
- Around line 319-343: The migration iterates todoFiles (used in the batch loop
and referenced when computing sessionID and legacyTodoID) without a stable
order, so duplicate todo_... IDs are resolved non-deterministically; sort the
todoFiles array before building todoSessions / starting the batch loop (i.e.,
call sort() on todoFiles early in the function that contains seenTodoIDs,
todoSessions, and the for-loop) so processing order is deterministic and the
seenTodoIDs de-duplication yields consistent migration output.
---
Nitpick comments:
In `@packages/opencode/test/session/todo.test.ts`:
- Around line 54-115: Replace the manual Instance.provide / Effect.runPromise
plumbing in both tests with the testEffect helper from test/lib/effect.ts: wrap
each test body with testEffect(...) so you can call effectful operations
directly (e.g., Session.create, Todo.Service.use, Todo.defaultLayer) without
Instance.provide and Effect.runPromise; update the two tests ("update returns
ids and get persists them" and "second update preserves id and persists status
changes") to use testEffect and drop explicit Instance.provide,
Effect.runPromise, and tmpdir usage inside the provide block while still calling
Session.remove at the end.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d3addb9-9893-4613-b931-21809ae7713e
📒 Files selected for processing (7)
packages/app/src/pages/session/session-status-extractors.test.tspackages/app/src/pages/session/todos/todo-model.test.tspackages/opencode/src/session/todo.tspackages/opencode/src/storage/json-migration.tspackages/opencode/src/tool/todo.tspackages/opencode/test/session/todo.test.tspackages/opencode/test/storage/json-migration.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/app/src/pages/session/todos/todo-model.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/session/todo.ts
- packages/opencode/src/tool/todo.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/session/todo.test.ts (1)
56-117: ⚡ Quick winSwitch these service tests to the Effect test harness (
testEffect+it.live).These cases run Effect services and depend on live FS/git behavior, so prefer Effect-native test wrappers over
Effect.runPromise+ Promise-styleInstance.provide.As per coding guidelines for
packages/opencode/test/**/*.test.{ts,tsx}: usetestEffect(...), useit.live(...)for live OS behavior, and preferprovideTmpdirInstance(...)/provideInstance(...)over manualInstance.provide(...)in Promise-style tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/session/todo.test.ts` around lines 56 - 117, The tests use Promise-style Instance.provide and Effect.runPromise for Effect services and live FS/git behavior; replace them with the Effect test harness by converting each test to testEffect(...) and wrapping live behavior with it.live(...), replace manual tmpdir/Instance.provide usage with provideTmpdirInstance(...) / provideInstance(...) utilities, and run Todo.Service.use and Session.create/remove inside the Effect test so you can supply Todo.defaultLayer via .provide(provideTmpdirInstance(...)) instead of Instance.provide and remove all Effect.runPromise calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/test/session/todo.test.ts`:
- Around line 56-117: The tests use Promise-style Instance.provide and
Effect.runPromise for Effect services and live FS/git behavior; replace them
with the Effect test harness by converting each test to testEffect(...) and
wrapping live behavior with it.live(...), replace manual tmpdir/Instance.provide
usage with provideTmpdirInstance(...) / provideInstance(...) utilities, and run
Todo.Service.use and Session.create/remove inside the Effect test so you can
supply Todo.defaultLayer via .provide(provideTmpdirInstance(...)) instead of
Instance.provide and remove all Effect.runPromise calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ea97cd8-1471-4f19-9d74-b891eb0abd15
📒 Files selected for processing (1)
packages/opencode/test/session/todo.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
todo.updatedpayload, and generated SDK v2Todotype.todowriteoutput/metadata and preserve existing ids only when they belong to the current session.selectSessionTodoSnapshotcompatibility alias.Why
Fixes #395.
#394 split todo dock lifecycle logic into focused modules, but lifecycle detection still used only todo status arrays. That meant a same-count, same-status task replacement could look identical to a content-only refresh. The backend also had an architecture mismatch: frontend sync already reconciled todos by
id, but backendTodo.Infoand thetodotable had no stable id.Related Issue
Fixes #395
Human Review Status
Pending. A human should make the final merge decision after reviewing the final diff and verification evidence.
Review Focus
packages/opencode/src/session/todo.ts: id resolution rules, especially unknown ids, duplicate ids, and idless replacement behavior.packages/opencode/migration/20260503025430_todo_ids/migration.sql: migration from(session_id, position)identity to stable todo ids for historical rows.packages/app/src/pages/session/todos/todo-model.ts: lifecycle signature uses[id, status]only when all todos have stable ids, otherwise it keeps the old status-only fallback.packages/app/src/pages/session/session-status-extractors.ts: completedtodowritemetadata wins over idless tool input.Risk Notes
Medium data/API risk: this adds a required
idto v2Todoand changes the todo table primary identity. The API shape remainsTodo[]; this PR does not introduce a session-level revision envelope. Historical tool parts without ids still use the frontend fallback. Historical database todo rows receive generated ids during migration.No SSE replay or reconnect recovery is included.
How To Verify
Screenshots or Recordings
Not attached. This PR does not intentionally change visible UI. Existing todo dock behavior is covered by focused unit tests and the todo dock e2e suite.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishMaintainer labeling request: please add appropriate type, scope, and priority labels if needed.
Summary by CodeRabbit
Refactor
New Features
Migration
Tests
Documentation