feat: add durable logical sessions and Goal handoff - #162
Conversation
…e-v4 # Conflicts: # src/local_shell_mcp/ui_static/live-workspace.html # ui/src/live-workspace.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d33055dc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| logical_manager = get_session_runtime_manager() | ||
| if __tool_name != "session_manage": | ||
| logical_manager.assert_current_run(live_session_key) |
There was a problem hiding this comment.
When a plan's lease is already due and its agent starts another tool call, this path only validates the run and publishes the start to the ephemeral live channel; logical activity is not recorded until the tool completes. The Live Workspace can therefore claim a continuation on its next 30-second poll, send a takeover, and leave the original tool—especially non-cancellable remote_transfer or mcp_tool_call—mutating concurrently because the run assertion has already passed. Record the start in the logical session, and account for in-flight calls when deciding whether continuation is due.
Useful? React with 👍 / 👎.
| if accepted: | ||
| plan.continuation_count += 1 | ||
| plan.last_continuation_at = now | ||
| plan.last_agent_activity = now |
There was a problem hiding this comment.
If sendMessage is rejected or throws, accepted is false, so this clears continuation_pending without advancing last_agent_activity or another retry timestamp. Because the lease is still overdue and ui/src/live-workspace.ts calls the claim endpoint every 30 seconds, an unavailable or rejecting host is immediately retried every poll indefinitely; rejected attempts also do not count toward the cap. Update a retry timestamp on failure or apply an explicit backoff before making the plan claimable again.
Useful? React with 👍 / 👎.
| NON_DESTRUCTIVE_MUTATION_TOOL_NAMES = { | ||
| "create_file_link", | ||
| "open_live_workspace", | ||
| "plan_manage", |
There was a problem hiding this comment.
Clients that use MCP safety annotations may execute plan_manage without a destructive-action confirmation, yet the same tool supports cancel and finish, which permanently put the current plan into a terminal state and cannot be undone with resume. Since annotations apply to the whole tool rather than individual action values, keeping plan_manage in the non-destructive set makes the advertised safety metadata inaccurate; it should use the conservative destructive classification.
Useful? React with 👍 / 👎.
| session_key = mcp_session_key(mcp) | ||
| logical_session_id = get_session_runtime_manager().current_session_id(session_key) | ||
| channel, live_token = get_live_channel_manager().open( | ||
| session_key=mcp_session_key(mcp), | ||
| session_key=session_key, | ||
| subject=subject, | ||
| scopes=scopes, | ||
| live_id=live_id, | ||
| logical_session_id=logical_session_id, |
There was a problem hiding this comment.
After a server restart, durable sessions reload but _attachments and live channels do not. When the existing app calls live_workspace_reconnect with its old live_id, the new MCP transport has no current attachment, so current_session_id returns None and the replacement channel is created without a logical session even though the app still knows the previous session_id. The continuation endpoint then always returns claimed: false, preventing an overdue Goal from automatically resuming until a model run happens to resume it manually. Persist or securely pass the logical session identity through the reconnect flow.
Useful? React with 👍 / 👎.
| "active_run": item.active_run().public_state() | ||
| if item.active_run() | ||
| else None, | ||
| "progress": item.progress.public_state(), |
There was a problem hiding this comment.
action="list" includes every finding and blocker in each session's full progress object. A report may contain 50 findings and 50 blockers of up to 20,000 characters each, and the list returns up to 100 sessions, allowing a valid request to construct and serialize roughly 200 MB of response data. This can exhaust memory or exceed MCP transport limits during routine session discovery; reserve full progress for get and return only bounded summaries or counts here.
Useful? React with 👍 / 👎.
| if current.session_key == session_key: | ||
| self._attachments[session_key] = (session.session_id, current.run_id) | ||
| return current |
There was a problem hiding this comment.
A later ChatGPT turn commonly reuses the same MCP HTTP session ID, so its required session_manage(action="resume", takeover=true) call reaches this early return because current.session_key matches. No new AgentRun is created and the prior turn is not superseded; tool calls from both turns therefore continue to pass assert_current_run, defeating both the advertised run history and stale-agent mutation protection. Agent-run identity needs to be distinct from transport identity, particularly for continuation messages in the same conversation.
Useful? React with 👍 / 👎.
| logical_manager.record_activity( | ||
| live_session_key, | ||
| "tool.completed" if call_ok else "tool.failed", | ||
| data=completion_data, | ||
| ) |
There was a problem hiding this comment.
If the session directory becomes unwritable or full, an underlying tool—including a destructive filesystem operation—can complete successfully and then have this activity write raise from _save_locked. The wrapper enters its generic exception path and reports the tool as failed, and its second attempt to record failure can raise again; an agent retry may then duplicate a mutation that already happened. Activity persistence errors should be isolated from the completed tool result or surfaced as a non-retryable warning.
Useful? React with 👍 / 👎.
| "todo_read_tool": DeprecatedTool("plan_manage", removed_in="3.3.0"), | ||
| "todo_write_tool": DeprecatedTool("plan_manage", removed_in="3.3.0"), |
There was a problem hiding this comment.
These tools are still present in the immediate parent commit, whose package version is already 4.0.0, but their tombstones claim they were removed in 3.3.0. Clients with a stale tool snapshot will therefore receive factually incorrect upgrade diagnostics and may troubleshoot against the wrong release boundary. Set removed_in to the release that first omits the todo tools.
Useful? React with 👍 / 👎.
1 participant
Summary
session_managetask contexts independent of machine/workdirsession_idLiveChanneland into durable Session statesession_idthrough app-only reconnect so the view can restore its durable Session after in-memory Live Workspace state is lostBehavior
A new tool-driven task can start with:
Agents report semantic checkpoints with:
A later GPT/MCP run inherits the same task with:
takeover=truemarks the prior active agent run as superseded; subsequent execution calls from that stale run are rejected until it explicitly resumes/takes over again.plan_manageis optional Session-owned Goal state. Live Workspace displays/subscribes to the attached Session; the Session itself persists independently of the UI connection, machine, and cwd.Validation
pytest -q: 677 passed, 1 skippednpx --yes bun@1.2.23 run typechecknpx --yes bun@1.2.23 run build:webruff check .git diff --check