feat(session): session handle contract for cross-process resume - #104
Merged
Merged
Conversation
Make the harness drivable by a control plane that runs it in short windows: save_session returns a SessionHandle (id, path, working dir, per-save usage delta and running total), start_session accepts a caller-minted id so a scheduler's task-keyed record and the harness agree without a round-trip, and reset_session drops a spent snapshot. Resume failures are now typed via SessionResumeError.reason (missing / corrupt / schema_mismatch / working_dir_mismatch) with should_clear_handle, so a caller can retry fresh instead of parsing messages. Snapshots record working_dir and a resume into a different directory is refused unless explicitly allowed, since replaying a transcript about other files is worse than starting over. The transcript stays in dream's own store; callers keep only the handle. Co-authored-by: Cursor <cursoragent@cursor.com>
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # The directory the session did its work in. A resume into a different | ||
| # working directory replays a transcript about other files, so the harness | ||
| # refuses it unless the caller opts in. ``None`` for engine-less sessions. | ||
| working_dir: str | None = None |
There was a problem hiding this comment.
Suggestion: Appending working_dir before the existing metadata field changes the positional constructor contract of the public SessionSnapshot dataclass. Existing consumers that pass metadata positionally will bind it to working_dir, causing invalid snapshot data or silently losing metadata; append new fields after metadata or provide a compatibility-preserving constructor. [api mismatch]
Severity Level: Major ⚠️
- ❌ Existing positional snapshot consumers cannot round-trip snapshots.
- ⚠️ Persisted metadata is silently bound to the wrong field.
- ⚠️ Resume fails with a corrupt-snapshot error for affected callers.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/dream/services/session_store.py
**Line:** 134:134
**Comment:**
*Api Mismatch: Appending `working_dir` before the existing `metadata` field changes the positional constructor contract of the public `SessionSnapshot` dataclass. Existing consumers that pass `metadata` positionally will bind it to `working_dir`, causing invalid snapshot data or silently losing metadata; append new fields after `metadata` or provide a compatibility-preserving constructor.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
There was a problem hiding this comment.
Fixed in ce4b168. Rather than reorder the tail, the optional fields (max_turns, working_dir, metadata) are now kw_only=True — each of them is something the harness learned to persist after the fact, so the next one would recreate the same trap. test_snapshot_optional_fields_are_keyword_only pins it.
Greptile SummaryThe PR adds durable session handles and role-scoped session continuation across processes while replacing sprint-contract negotiation with plan-time acceptance criteria.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported session-resume issues are fixed in the current code.
|
| Filename | Overview |
|---|---|
| src/dream/services/session_store.py | Adds versioned session-handle serialization and rejects legacy snapshots before restoration. |
| src/dream/harness.py | Adds named session lifecycle operations, typed resume handling, working-directory checks, and session-handle persistence. |
| src/dream/runner/_role_session.py | Adds resumable role sessions while preserving snapshots that belong to another working directory. |
| src/dream/runner/_run.py | Adds task-scoped role session IDs and simplifies orchestration around plan-derived sprint contracts. |
| src/dream/sprint/_plan_contract.py | Builds sprint contracts directly from plan-time acceptance criteria and unresolved evaluator feedback. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[run_role with session_id] --> B{Saved snapshot exists?}
B -- No --> C[Start named session]
B -- Yes --> D{Snapshot resumable?}
D -- Yes --> E[Resume named session]
D -- Spent or incompatible --> F[Reset and start named session]
D -- Working directory mismatch --> G[Start anonymous non-owning session]
C --> H[Run role]
E --> H
F --> H
G --> H
H --> I{Owns named session ID?}
I -- Yes --> J[Save and return SessionHandle]
I -- No --> K[Do not save; preserve existing snapshot]
Reviews (7): Last reviewed commit: "fix(session): refuse resume when working..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await self._ensure_open() | ||
| opts = options or SessionOptions() | ||
| session_id = uuid.uuid4().hex | ||
| resolved_id = uuid.uuid4().hex if session_id is None else checked_session_id(session_id) |
There was a problem hiding this comment.
Suggestion: Caller-supplied IDs are used directly as the session storage key without checking whether a snapshot already exists or coordinating ownership. Two scheduler tasks can select the same ID, create independent sessions, and later overwrite each other's snapshots, losing one transcript while both callers retain apparently valid handles. Reject an existing ID for start_session, or require an explicit resume/replace operation with ownership or version checks. [state collision]
Severity Level: Major ⚠️
- ❌ Concurrent scheduler tasks can lose one conversation transcript.
- ⚠️ Handles can point to silently replaced session state.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/dream/harness.py
**Line:** 190:190
**Comment:**
*State Collision: Caller-supplied IDs are used directly as the session storage key without checking whether a snapshot already exists or coordinating ownership. Two scheduler tasks can select the same ID, create independent sessions, and later overwrite each other's snapshots, losing one transcript while both callers retain apparently valid handles. Reject an existing ID for `start_session`, or require an explicit resume/replace operation with ownership or version checks.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Contributor
Author
There was a problem hiding this comment.
Fixed in ce4b168. start_session now refuses a caller-minted id that already names a saved snapshot, pointing at resume_session to continue it or reset_session to discard it. The check is skipped when no store is configured, since an id collision costs nothing with nowhere to persist. test_start_session_refuses_an_id_that_already_has_a_snapshot covers it.
A role head previously opened a fresh session every call, so a caller running the harness in short windows lost the conversation between them. Naming the thread now resumes it: run_role restores that snapshot when one is readable and returns its SessionHandle, with the usage delta scoped to the run. An unusable snapshot never strands the role. Missing, corrupt, foreign schema, or a snapshot taken under another working directory all start the thread over under the same name — the same fallback a coding CLI makes when --resume is refused — and a spent snapshot is dropped so later runs stop re-paying the failed attempt. The transcript is persisted before the mid-stream error check, so a session that failed still leaves the history explaining why for the next run of the thread. Co-authored-by: Cursor <cursoragent@cursor.com>
run_role can now name a thread, but a control plane drives run_task, whose
heads each opened their own unnamed session. session_scope threads one key
per task through the autowired heads: every role gets a resumable thread
beneath it ({scope}:planner and so on), so a later run_task under the same
scope continues those conversations instead of restarting them.
Roles stay separate — a planner and an evaluator are different
conversations — but heads bound to the same role deliberately share one
thread. The generator negotiates the sprint contract and then builds
against terms it remembers agreeing to; the evaluator judges against
criteria it proposed itself.
Explicitly supplied heads are left alone, and omitting the scope keeps
today's behaviour of persisting nothing.
Co-authored-by: Cursor <cursoragent@cursor.com>
…iation Naming a step and naming what "done" means for it is one judgement, so the planner does both and the sprint reads the criteria off the ledger. The propose/respond exchange between evaluator and generator cost two role sessions per sprint and six at the cap, all before a line of code existed, and it is the one part of the loop that had two agents negotiating actions rather than contributing intelligence. run_task now takes three heads instead of five. A needs-changes verdict still steers the retry: its unresolved items fold into the next contract's criteria. Co-authored-by: Cursor <cursoragent@cursor.com>
Review findings on the session-handle work, all variations on one thing: a session id is a name two callers can pick, and the harness treated the second one as the owner. - start_session refuses an id that already has a saved snapshot. Two scheduler tasks on the same key would otherwise save over each other while both kept a handle that looked fine. resume_session continues it, reset_session discards it; either is a decision the caller makes out loud. - run_role no longer saves over a snapshot from another working directory. should_clear_handle already called that snapshot reusable, and then the fallback started a fresh session under the same name and saved it. The run now goes on unnamed and returns no handle, leaving the transcript resumable from the workspace that wrote it. - SCHEMA_VERSION 2. A version-1 snapshot predates working_dir, so it decoded as None and skipped the directory check entirely — an old file was a way to resume a transcript into any workspace. It now reads as a foreign schema. - SessionSnapshot's optional fields are keyword-only. Each was learned after the fact and the next one lands beside them; positional callers would bind metadata to whatever arrived last. Co-authored-by: Cursor <cursoragent@cursor.com>
Derived session ids used a colon separator that checked_task_id rejects when
the scope becomes a directory name under ~/.dream/data/sessions. Switch to
hyphen separation and validate at derivation time so chorus task-{uuid} scopes
work in production.
Co-authored-by: Cursor <cursoragent@cursor.com>
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
3 tasks
v2 snapshots with a null working_dir cannot prove they belong in the current workspace, so resume now requires explicit opt-in rather than silently skipping the directory binding check. Co-authored-by: Cursor <cursoragent@cursor.com>
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
Stacked on #92. Makes dream drivable the way a control plane drives a coding CLI: dream owns the transcript, the caller keeps only a handle.
SessionHandlefromsave_session: id, path, working_dir, schema_version, usage delta + running totalstart_session(session_id=…)so chorus can minttask-{uuid}and agree with the harness without a round-tripSessionResumeErrorwith typedreason+should_clear_handlerun_role(session_id=…)+run_task(session_scope=…)— one resumable thread per role under a scope ({scope}-planner,-generator,-evaluator)checked_task_idrejects:in sidecar directory names)Chorus counterpart: #89 stores the handle row; passes
session_scope=dream_session_key_for_task(task_id).Merge order
Test plan
uv run pytest tests/test_services/test_session_store.py tests/test_runner/test_task_session_scope.py tests/test_runner/test_role_session_resume.py -quv run pytest -q— full suite greentask-{uuid}-{role}.json