fix(agent-core-v2): carry session state across fork and ignite toolDedupe - #1629
Conversation
- ignite IAgentToolDedupeService in AgentLifecycleService before external hooks so it sits ahead of permission on onBeforeExecuteTool - mirror the ignition in the test harness AgentTestContext - add lifecycle test asserting the dedupe hooks are registered
- copy blobs, plans, background-task output, and media originals into the forked session dir (excluding state.json, wire logs, and logs/) - duplicate the source session's cron tasks with fresh ids retagged to the target session via the shared CRON_SESSION_TAG - roll back the half-fork on failure: drop the materialized handle from the live registry and delete the target session dir
🦋 Changeset detectedLatest commit: 595fa94 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: 595fa94864
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (targetId !== undefined) { | ||
| this.sessions.delete(targetId); |
There was a problem hiding this comment.
When newSessionId already belongs to a live session, the collision check throws after targetId has been assigned, but before this fork materializes a target. This catch then deletes this.sessions[targetId], so a rejected fork can make the pre-existing session disappear from get/list/close. Only remove the handle when it is the target created by this fork.
Useful? React with 👍 / 👎.
| id: ulid(), | ||
| tags: { ...task.tags, [CRON_SESSION_TAG]: targetId }, | ||
| }; | ||
| await this.cronStore.save(workspaceId, clone); |
There was a problem hiding this comment.
If the fork fails after one or more cron clones are saved here, for example during agent restore/replay or while appending the session index, the catch only removes the live handle and session directory. The cloned workspace-level cron docs remain tagged to the target id; retrying the same fork id can create another set of clones, so the successful fork later sees duplicated scheduled jobs. Track the cloned ids and delete them on rollback, or defer saving them until the fork can no longer fail.
Useful? React with 👍 / 👎.
| // 7. Copy the source session's on-disk state into the target — per-agent | ||
| // `blobs/` and `plans/`, background-task output, and media originals. | ||
| // v1 achieved this with `cp -r` of the whole session dir; the wire logs | ||
| // (step 8) and `state.json` (step 9) are rewritten by the fork flow | ||
| // itself, and `logs/` is the source's debug log, so those are excluded. |
There was a problem hiding this comment.
packages/agent-core-v2/AGENTS.md says comments in this package must live only in the top-of-file /** */ block and never beside functions, methods, or statements. This newly added method-body narration violates that directory rule, and the same pattern appears in the new rollback/copy helper code; remove it or fold any necessary role-level context into the file header.
Useful? React with 👍 / 👎.
1 participant
Related Issue
No linked issue — the problems are explained below.
Problem
Two independent v2 engine regressions relative to v1:
blobs/andplans/, background-task output, andmedia-originals/were left behind, so the fork's blob refs resolve to[media missing]and its active plan file is gone. Cron tasks were lost too: v1 kept them inside the session dir so itscp -rfork carried them, while v2 persists them at workspace level. And a fork that failed midway left a broken half-copy in the registry and on disk, trippingSESSION_ALREADY_EXISTSon retry.AgentToolDedupeServiceis self-wiring (its constructor registers the loop step hooks and the executor'sonBefore/onDidExecuteToolhandlers) but nothing injects it, so it was never instantiated — identical tool calls issued in the same step executed multiple times, and repeated identical calls across steps never received escalating reminders.What changed
Fork state carry-over (
SessionLifecycleService.fork)blobs/andplans/, background-task output, and session media originals — excludingstate.json(rewritten with fork provenance), the wire logs (copied with a fork boundary record), andlogs/. Symlinks are never followed out of the session dir.CRON_SESSION_TAGexported fromapp/cron/cronTask.ts.toolDedupe ignition (
AgentLifecycleService)IAgentToolDedupeServiceduring agent creation, before the external hooks service, sotoolDedupesits ahead ofpermissionononBeforeExecuteTooland same-step duplicates are suppressed before authorization runs (matching v1 ordering).AgentTestContext).Tests: added a lifecycle test asserting the dedupe hooks are registered, and fork tests covering file copy/exclusions, rollback + retry, and cron task duplication.
Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.