feat(cursor): operator-driven legacy stream-json -> ACP session migrator (transplant)#34
Open
heavygee wants to merge 14 commits into
Open
feat(cursor): operator-driven legacy stream-json -> ACP session migrator (transplant)#34heavygee wants to merge 14 commits into
heavygee wants to merge 14 commits into
Conversation
![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: 2e3d0a18fc
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a731c1ec55
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
…, P2 401-refresh) P1 (WAL preservation) — Without checkpoint, un-checkpointed transactions in the legacy store.db-wal sibling would either appear stale in the ACP target or be lost outright when the cleanup step removes the WAL file. Added a PRAGMA wal_checkpoint(TRUNCATE) flush of the legacy store before any cp, inside the same pre-flight window where the launcher has been confirmed quiescent. Idempotent on non-WAL stores (no-op). P2 (skipVerify must still run session/load) — The schema docstring described skipVerify as skipping the prompt step. The implementation was bypassing the entire verify block, which would have let a corrupted transplant flip HAPI metadata. Split the verify into 'always-load, optionally-prompt'; skipVerify now skips ONLY the prompt. Tests updated: the existing skipVerify-happy-path expectation rewritten + a new test verifies skipVerify still refuses on session/load failure. P2 (web client 401 refresh) — migrateCursorSessionToAcp now triggers the same onUnauthorized refresh dance as the shared request() helper instead of hard-failing on an expired JWT. Matches existing session-action UX.
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
…ues)
P1 — Lock release must do a real SQLite busy-probe, not just stat-poll.
killSession kicks off cleanup asynchronously; an idle-but-not-yet-closed
runner can let the stat-based size-stability heuristic report 'released'
while still owning the file lock. Added sqliteLockHeldByOtherProcess()
which opens the legacy store with BEGIN IMMEDIATE; SQLITE_BUSY is the
authoritative signal that another process holds a write lock. The
size-poll is retained as a secondary signal for partial writes.
P2 — preflightSession now refuses on session.active=true as well as
lifecycleState==='running'. Some legacy rows may lack lifecycleState
but still be active in the hub cache; the CLI/API paths relied on the
lifecycle-only check, while the web UI already hid the button for
active sessions. Now uniform across all entry points.
P2 — Atomic ACP target creation. Replaced the existsSync + mkdirSync
(recursive: true) TOCTOU with an exclusive mkdirSync(recursive: false);
EEXIST is caught and converted to a target_already_exists refusal. The
rollback path only fires for the dir we exclusively created.
P2 — cursorSessionId basename validation. Reject anything that isn't
[A-Za-z0-9_.-]+ before any path join, with explicit rejection of '.'
and '..'. Prevents path-traversal in either the chats lookup or the
acp-sessions target.
P2 — hapi.db backup now uses SQLite's serialize() so committed pages in
hapi.db-wal are included. The raw cp from before could omit recent
sessions or pre-migration metadata. Added Store.backupTo(destPath)
exposing the consistent-snapshot path.
Tests:
- session.active=true refusal
- cursorSessionId path-traversal refusal (../escape, has/slash, .,
.., has\backslash)
- existing tests unchanged; full hub suite 299 pass.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2cbe6edb41
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
P1 — Lock release uses session.active poll as the primary signal. An archive() kicks off async cleanup; the closest hub-side proxy for "runner process has exited" is sessionCache.active flipping false (set when the runner socket disconnects). The migrator now polls that before falling back to the SQLite busy-probe + size-poll, then sleeps a 500ms grace after all three pass so any in-flight kernel syscall on the WAL/SHM can complete. SyncEngine injects the cache-poll fn. P2 — Abort bulk migration when the hapi.db backup fails. The previous code logged + continued; without the backup, operators have no rescue handle if a metadata flip lands wrong. Now throws unless the DB is in-memory (test fixtures). API handlers wrap in try/catch and surface the error as HTTP 500 with the backup-failure message. P2 — Windows agent-binary spawn. AcpVerifyProbe.start() now matches cursorAcpRemoteLauncher's shape with shell:true + windowsHide:true on win32, so the `agent.cmd` shim is reachable. Without this the verify probe fails with ENOENT even though normal Cursor ACP sessions work. P2 — Bulk CLI timeout removed. A finite 30-minute axios timeout could cut off a slow operator run mid-flight (verifyTimeoutMs is 120s per session by default; ~15 sessions hits the cap). The hub-side bulk operation is idempotent per session, so the request can safely block until completion. Operators that want to cap a run can split it with a smaller --lifecycle filter. Tests: full hub suite 299 pass + integration (real agent acp) 3 pass on the 34-msg fodder fixture. Typecheck clean across cli/hub/web. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This was referenced Jun 6, 2026
Owner
Author
|
@codex review Operator has topped up Codex credits after hitting the 5h Code Review cap. Current HEAD is
Last automatic review verdict was on commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96578a55b9
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
P1 — Wait for real runner exit before transplanting. The previous awaitSessionInactive() poll was bogus: archiveSession() synchronously calls handleSessionEnd() which sets sessionCache.active=false, so any cache-based wait returns immediately. An idle Cursor process with no SQLite write lock could also pass the busy-probe instantly while still mid-SIGTERM-cleanup. Replaced with a real minimum dwell time (2s default) inside awaitLockRelease, gated on SQLite busy-probe + size-stability. Removed the SyncEngine-side awaitSessionInactive injection (it was the source of the false reassurance). P1 — Honor the agent ACP active-process lock. Cursor's `agent` binary enforces single-instance semantics (see cli/src/agent/backends/acp/agentCliGuard.ts); spawning the verify probe while another `agent acp` is live can SIGTERM the live one and crash the operator's Cursor session. The migrator now reads the $HAPI_HOME/locks/agent-acp-active/pid file and refuses with a new 'acp_transport_active' reason if the lock points to a live PID. The check is injectable for tests. P2 — Exclude active sessions from archived bulk candidates. The default archived bulk path filtered only on lifecycleState==='running', but preflightSession also treats session.active===true as running for legacy rows that lack lifecycle metadata. Without mirroring that here, `hapi cursor migrate --all` would include an active session, migrateOne would refuse it 'running_refused', and the bulk run would exit as failed. Extracted the filter to a pure exported predicate `isLegacyCursorMigrationCandidate()` and added unit tests. Tests: 35 pass (was 28) + integration 3 pass on the 34-msg fodder. Typecheck clean across cli/hub/web. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@codex review Round 4: addresses the 3 P1/P2 findings from the previous Codex pass on commit 96578a5:
HEAD is now eb446b3. 35 unit pass + 3 integration pass on the 34-msg fodder fixture. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb446b391b
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
P1 — Block resumes while mutating the store. Between preflight and the destructive flip+rm, a session could be resumed via the existing hub paths. The resume reads cursorSessionProtocol from hapi.db (still 'legacy' until the flip), spawns a stream-json runner against the legacy source — which we are about to remove. Added getCurrentSession dependency that the migrator calls right before the metadata flip; if it sees active=true / lifecycleState=running / a concurrent flip to acp, it rolls back the ACP placement and refuses with new reason 'session_resumed_during_migrate' (or 'already_acp' for the concurrent flip case). SyncEngine injects a real impl reading the session cache. P2 — Register ACP verify probes in the active lock. The probe now acquires the same $HAPI_HOME/locks/agent-acp-active/ lock the CLI guard uses, just before spawning agent acp, and releases it AFTER the SIGTERM in stop(). Without this, a concurrent `hapi cursor migrate` or model-list request could see no active ACP lock, spawn another agent acp, and the second spawn would SIGTERM this verify per Cursor's single-instance enforcement (cli/src/agent/backends/acp/agentCliGuard.ts). New `hapiHome` option on AcpVerifyProbe lets tests pin the lock to an isolated dir so they don't clobber the operator's real lock. P2 — Override Windows home variables for the verify spawn. HOME alone is insufficient on Windows; the agent binary may resolve via USERPROFILE / HOMEDRIVE / HOMEPATH and end up reading the operator's real .cursor tree. The probe env now sets all three on win32. P2 — Reject single-session --dry-run. The CLI accepted --dry-run for per-session use but the per-session API has no dryRun field, so the flag was silently dropped and the real migration ran. Now throws a clear error instructing the operator to use --keep-source or the bulk path. P2 — Preserve private permissions on hapi.db backups. Store.backupTo() now passes mode 0o600 to writeFileSync and chmods after as belt-and-braces. Without this the rescue copy (which contains session transcripts) could land world-readable under a permissive umask, even though the live hapi.db is locked down to 0600. Tests: 37 pass (was 35) — added rollback test for resume-mid-migration and concurrent-flip-to-acp scenarios. Integration 3 pass on the 34-msg fodder fixture; integration tests now use isolated $HAPI_HOME to avoid touching the operator's real ACP lock. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@codex review Round 5: addresses the 5 P1/P2 findings from the previous Codex pass on commit eb446b3:
HEAD is now be7696a. 37 unit pass (+2 rollback tests) + 3 integration pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be7696a857
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 6, 2026
P2 v2 — Atomic agent-acp-active lock acquisition. The previous mkdirSync(recursive:true) + writeFileSync was not race-safe: two concurrent migrations could both pass isAgentAcpTransportActive() and then BOTH 'acquire' the lock by writing their pid in succession. Now uses mkdirSync(recursive:false) which is atomic — exactly one caller wins on EEXIST contention. Stale locks (dead pid) are cleared and retried once. start() throws a typed error on race loss; verifyInTempHome catches it and the migrator surfaces it as the acp_transport_active refusal so the operator sees the authoritative reason. Added 4 unit tests for the lock primitive (acquire / refuse-when-held / stale-cleanup / no-clobber-on-stop). P1 v2 — Authoritative active-check inside the metadata flip. The migrator's getCurrentSession recheck (round 5) was best-effort: a resume landing AFTER the recheck but BEFORE the DB update would still slip through. Moved the same check INSIDE flipCursorSessionProtocolToAcp so the read-active and write-protocol pair are within a single synchronous Bun call (no JS interleaving). Added new flip result 'session-active' + corresponding UpdateAfterMigrateResult variant + corresponding refusal mapping back to 'session_resumed_during_migrate'. Added a unit test that drives the path with updateOverride returning session_active. Tests: 313 unit pass (was 308; +4 probe lock, +1 atomic flip) plus 3 integration on the 34-msg fodder fixture. Typecheck clean across cli/hub/web. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@codex review Round 6: addresses the 2 P1/P2 findings from previous pass on be7696a:
HEAD is now a6908df. 313 unit pass + 3 integration pass on 34-msg fodder fixture. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6908df576
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 7, 2026
P1 v3 — Detect resume-then-exit during the migration window. The active-flag rechecks (both the migrator's getCurrentSession and the flip-time atomic check) only catch resumes that are STILL active at the moment of the check. A brief resume that came in, wrote turns to store.db/store.db-wal, and exited before the check would slip through — the store.db would have diverged from the snapshot we copied to the ACP target. Added an mtime+size fingerprint of the legacy store.db captured right after the WAL checkpoint; compared immediately before the metadata flip and rolled back with new reason 'legacy_store_modified_during_migrate' on any divergence. P2 — Respect metadata.homeDir + metadata.host. Hub processes can run under a different account than the user who created the Cursor session (service-account / multi-machine setups). The legacy chats live under the recorded session-owner home, not the hub's. The migrator now prefers metadata.homeDir for resolving both the legacy source (~/.cursor/chats) and the ACP target (~/.cursor/acp-sessions), falling back to homeDir() when the field is absent. Cross-host sessions (metadata.host != local hostName) are refused with new reason 'cross_host_session' so we never touch a same-id store under the wrong user account. hostName is a new injectable dep with default process.env.HAPI_HOSTNAME || os.hostname(). P2 v3 — Don't clobber freshly-starting CLI ACP locks. The previous retry path treated 'lock dir exists but no pid file' as stale and cleared it — but the CLI guard's registerActiveAcpTransport does mkdir BEFORE writeFileSync, so a probe landing in that small window would delete the live caller's lock. probeLockHolder now returns 'starting' for the pidless case, which falls through to the refuse- path. Only a present-but-dead pid file is treated as stale. Tests: 317 hub pass (was 313; +cross-host refusal, +fingerprint divergence rollback, +homeDir resolution, +mid-startup probe lock refusal). Integration 3 pass on the 34-msg fodder fixture (with injected hostName). Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@codex review Round 7: addresses the 3 P1/P2 findings from pass on a6908df:
HEAD is now 8145b73. 317 hub pass (+4 new tests) + 3 integration pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8145b73551
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 7, 2026
P1 v4 — Include WAL/SHM sidecars in the divergence fingerprint. The previous fingerprint only stat'd the main store.db. SQLite stages new commits in store.db-wal before merging into the main file, so a brief resume that wrote turns to the WAL but did not trigger a checkpoint would leave store.db unchanged while the WAL contained the new data. Now fingerprints all three (main, -wal, -shm) and refuses on any change/appearance/disappearance. Added a unit test that simulates the WAL-only divergence case. P2 v2 — Reorder preconditions BEFORE archive. Bulk runs with --force-archive-running could kill a session and then refuse to migrate it for reasons known at preflight time (cross-host, acp_transport_active, target_already_exists). Moved all filesystem/state-only refusals to run before archiveSession so we never amputate a session we cannot then migrate. New tests assert that archiveCalls remains empty for the two refusal cases the bot flagged (cross-host + acp-transport-active). Tests: 320 hub pass (was 317; +1 WAL fingerprint, +2 no-archive-on- refusal). Integration 3 pass on 34-msg fodder. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com>
Owner
Author
|
@codex review Round 8: addresses the 3 P1/P2 findings from pass on 8145b73:
HEAD is now 6bc2b78. 320 hub pass (+3 new tests) + 3 integration pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6bc2b7853a
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 7, 2026
P2 v5 — Treat pid-less ACP locks as active. The CLI agent guard
creates the agent-acp-active lock directory BEFORE writing the pid
file. defaultIsAgentAcpTransportActive() only checked the pid file,
so a CLI startup window reported active=false; bulk migrations with
--force-archive-running would then archive a session and let
verifyInTempHome() refuse on the same lock anyway, leaving the
session orphaned. Now the default treats lock-dir-exists-but-no-pid
(and malformed pid files) as active so we refuse early, before any
archive side effect.
P3 — Bulk dry-run no longer counts as migrated. The previous
`migrated: migrated + dryRun` rollup meant a pure preview reported
`Migrated: N` even though no metadata or filesystem changes were
made. Split CursorMigrateBulkResult.dryRun out so operators and
automation can distinguish preview from real mutation. CLI summary
updated to show both counters.
P2 v5 — Resume-race recheck no longer trips on stale lifecycle.
After our own archiveSession() returns, the cache shows active=
false synchronously but the metadata cleanup that flips
lifecycleState 'running' → 'archived' may still be in flight. The
recheck was refusing as session_resumed_during_migrate in that
window — making force-archive migration of running sessions a
cleanup-race coin flip. Now we trust the active flag as the
authoritative live-runner signal:
- Resume-race recheck: refuse only on active=true (always), or on
lifecycleState=running when wasActive=false (external resume).
- Atomic flip in flipCursorSessionProtocolToAcp: refuse only on
active=true.
Tests: 2 new tests cover the stale-lifecycle no-trip + external-resume
trip branches. 46 pass in migrator suite, 320 in hub overall, 3
integration pass. Typecheck clean across hub/cli/web.
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d61dcf92f
ℹ️ 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".
Owner
Author
|
@codex review Three new commits since the bot's last pass at
Live dogfood receipts (4 successful auto-migrations against real archived legacy cursor sessions on the operator's box, including the operator's own 374MB jellybot-dev session) are in the updated PR body under 'Live dogfood on the soup (2026-06-07)'. The post-mortem on the Please focus the new pass on the verify-probe spawn changes in |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c682facc1
ℹ️ 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".
heavygee
added a commit
that referenced
this pull request
Jun 7, 2026
F1 — acpVerifyProbe.ts: use path.delimiter for PATH joining
Windows PATH entries are `;`-separated. Previous `:` join produced
invalid combined entries on win32 (`C:\...\.local\bin:C:\Windows\...`)
that cmd.exe treated as one segment.
F2 — acpVerifyProbe.ts: accept session-owner home via agentLookupHome
Deployment shape where hub runs as a service account whose
process.env.HOME differs from the human user who installed Cursor
(recorded in `metadata.homeDir`). Migrator threads `this.deps.homeDir()`
into the probe so the binary lookup uses the right home. Falls back
to process.env.HOME when no explicit value supplied.
F3 — acpVerifyProbe.ts: preserve explicit options.env.PATH precedence
Previous fix PREPENDED cursor-bin fallback — overrode a caller's
intentionally pinned `agent`. Switch to APPEND so the existing PATH
wins; fallback only kicks in when the existing PATH doesn't already
contain `agent`. Still fixes the minimal-systemd-PATH dogfood case.
F4 — sessions.ts + cli.ts: reject malformed JSON on migrate-to-acp
`c.req.json().catch(() => ({}))` silently converted parse errors
into `{}` which passed Zod (all-optional schema) and ran the
migration with destructive defaults. Distinguish "no body" from
"malformed JSON" (return 400 on the latter).
F5 — syncEngine.ts auto-migrate helper: pass forceArchiveRunning: true
Inactive rows with stale `lifecycleState=running` (orphaned by hub
crashes that didn't finish the lifecycle transition) were being
refused with `running_refused` and silently fell back to the legacy
launcher forever. resumeSession's caller-side `if (active) return`
guard already ensured no live runner — the stale lifecycle is
exactly the kind of stuck row sync-on-open is meant to clean up.
Tests: 2 new tests pin precedence (F3) + path.delimiter (F1) via stubbed
competing agent binaries in priority vs fallback dirs. Existing live-
dogfood regression test extended to cover both explicit agentLookupHome
AND fallback to process.env.HOME. 339 hub tests pass, typecheck clean.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
heavygee
added a commit
that referenced
this pull request
Jun 7, 2026
Codex review #34 P2 (round 13, finding 3369444056): on a multi-user host with a default 022 umask and a non-private ~/.cursor tree, the new ACP session directory was being created with the default 0o755 mode and the transplanted store.db inherited the source file's mode bits (typically 0o644). Other local users could traverse the session dir and read the transplanted transcript — even when the source store and HAPI DB were private. - mkdirSync(acpSessionDir, { recursive: false, mode: 0o700 }) - chmodSync(store.db, 0o600) after copyFileSync (copy inherits source) - writeFileSync(meta.json, ..., { mode: 0o600 }) 339 hub tests pass, typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
@codex review Round 13 (HEAD
Two new tests pin the PATH-precedence (F3) and Please review the latest commits:
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
@codex review Round 14 (HEAD
New tests: hub 4 new (flag transitions across failure/exception/success-no-clear/flip-atomic-clears), web banner 10 (visibility paths + a11y). Hub 343 pass (was 339; +4), web banner 10/10, typecheck clean across cli/web/hub. Note: bot may be at quota cap; if so this triggers a queued pass for when credits refill. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35daa9f2eb
ℹ️ 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".
| // host's live ACP transports cannot block migration. Operator | ||
| // CLI/REST callers that want the pre-isolation conservative | ||
| // behavior can inject `() => tryAcquireAcpActiveLock(home)`. | ||
| acquireAcpActiveLock: deps.acquireAcpActiveLock ?? (() => ({ release() {} })), |
There was a problem hiding this comment.
In the SyncEngine path no acquireAcpActiveLock override is passed, so production uses this no-op handle while the default probe is also constructed with skipLockAcquire: true. If two legacy Cursor sessions are opened/migrated at about the same time, both verifiers can spawn agent acp concurrently; the existing ACP guard documents that Cursor's agent CLI is single-process and a second spawn can terminate the first, so one or both migrations can fail and fall back to legacy despite being otherwise migratable. Use a real shared lock here, or only skip the probe's internal lock when the caller actually acquired one.
Useful? React with 👍 / 👎.
Comment on lines
+655
to
+662
| if (latest && !wasActive && latest.lifecycleState === 'running') { | ||
| // Lifecycle says running but active=false and we did NOT | ||
| // archive this session ourselves. Something external lifted | ||
| // it back to running between preflight and now (rare but | ||
| // possible if the session was preflight-archived and then | ||
| // a peer un-archived it via the lifecycle API). | ||
| tryRm(acpSessionDir) | ||
| return refusal(session.id, 'session_resumed_during_migrate', `session lifecycle became running during migration (lifecycleState=running, active=false, wasActive=false); rolled back ACP placement`, start, this.deps.now) |
There was a problem hiding this comment.
When an inactive legacy Cursor row still has metadata.lifecycleState === 'running' after a hub crash, the auto-migrate path passes forceArchiveRunning: true and intentionally skips the archive RPC because active is false, but this recheck later treats the same stale lifecycle value as a resume race and rolls back the ACP placement. In that stale-row case the session will silently fall back to the legacy launcher every time instead of being migrated and having the lifecycle cleared by flipCursorSessionProtocolToAcp.
Useful? React with 👍 / 👎.
| // PATH. Thread the migrator's homeDir dep through so the | ||
| // probe doesn't fall back to process.env.HOME when a more | ||
| // specific lookup home is available. | ||
| agentLookupHome: this.deps.homeDir() |
There was a problem hiding this comment.
For sessions with metadata.homeDir different from the hub process home, migrateOneWithLock correctly reads the legacy store under the recorded owner home, but the default verifier still augments PATH from this.deps.homeDir() rather than that recorded home. In service-account hub deployments where Cursor's agent binary is installed in the human user's ~/.local/bin, verification will fail with agent not found even though the store was located successfully; thread the chosen source home into the probe's agentLookupHome.
Useful? React with 👍 / 👎.
| export type RenameSessionRequest = z.infer<typeof RenameSessionRequestSchema> | ||
|
|
||
| /** Per-session legacy stream-json → ACP migrator request. See tiann/hapi#824. */ | ||
| export const CursorMigrateToAcpRequestSchema = z.object({ |
There was a problem hiding this comment.
Because this Zod object is not strict, unknown keys are silently stripped; with all fields optional, an operator typo such as {"keep_source": true} validates as {} and the migration proceeds with the destructive default that removes the legacy source after a successful flip. For this destructive endpoint, reject unrecognized keys so malformed-but-valid JSON cannot accidentally drop the safety option.
Useful? React with 👍 / 👎.
* fix(web/markdown): disable single-dollar inline math in remark-math Default remark-math configuration treats $...$ as inline LaTeX, which turns any prose containing two currency amounts (e.g. "save $400 vs the $200 plan") into a KaTeX block — paragraphs collapse, whitespace is stripped, the running text is re-rendered as math symbols. Pass `singleDollarTextMath: false` to remarkMath so single $ is plain text. Block math `$$...$$` (on its own line) still renders, matching GitHub-flavored markdown semantics. Single source of truth: MARKDOWN_PLUGINS is shared by MarkdownText, Reasoning, and MarkdownRenderer — fix lands in all three surfaces. Adds 3 regression tests that drive the unified pipeline end-to-end: prose with multiple "$N" amounts produces no `class="katex"` and no `<math>` element; `$$...$$` block math still does. Co-authored-by: Cursor <cursoragent@cursor.com> * chore(web): declare unified/remark-parse/remark-rehype/hast-util-to-html The new markdown-text regression test imports these directly to drive the unified pipeline end-to-end. They were resolving via transitive deps from remark-math and rehype-katex, which is fragile — a future dep upgrade can remove the transitives and break the test. Declare them explicitly under devDependencies. No code change; lockfile records the same versions that were already installed transitively (unified@11.0.5, remark-parse@11.0.0, remark-rehype@11.1.2, hast-util-to-html@9.0.5). Addresses the HAPI Bot review finding on PR tiann#805. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…tiann#680) (tiann#815) Replaces hardcoded \"Claude Code\" strings in voice context injections with the active session's flavor label (Cursor, Codex, Gemini, etc.) via getFlavorLabel() from @hapi/protocol. Falls back to \"coding agent\" for unknown or missing flavors. Threads an agentLabel string param through: - formatMessage - formatNewSingleMessage - formatNewMessages - formatHistory - formatSessionFull - formatPermissionRequest - buildSessionVoiceContextPlan (passes to its internal formatMessage call) voiceHooks adds a single getAgentLabel(session) helper that reads session.metadata.flavor and resolves the display label once per call. Tests updated to cover the new parameter shape and to assert that \"Claude Code\" is never substituted regardless of agent flavor. formatReadyEvent already used a generic \"coding agent\" phrasing and needs no signature change. Closes tiann#680 Co-authored-by: Cursor <cursoragent@cursor.com>
… limit) (tiann#823) * fix(cursor): requeue user message on transient agent exit (auth, rate limit) cursorLegacyRemoteLauncher.runMainLoop popped a user message off the queue before spawning `agent` and silently discarded it whenever `agent` exited non-zero (auth expiry, rate limit, transient network). The wrapper logged the failure at debug level only, never surfaced it to the web UI, and emitted `ready` as if a normal turn had ended. Capture stderr from the spawned process; classify exit-1 with a transient signature (Authentication required, rate limit, ETIMEDOUT, ECONNRESET, EAI_AGAIN) as recoverable; re-head the message via `queue.unshift`, surface a friendly banner via `sendSessionEvent({type:'message',...})`, and backoff ~2s before the loop picks it up again. Cap at 5 consecutive transient failures, after which the message is dropped with a clear "resolve and resend" event so we never spin forever on a genuinely broken auth. Non-transient non-zero exits also surface the stderr to the UI now (instead of only the local ring buffer), so a real crash is visible to the operator. Backoff is overridable via CURSOR_LEGACY_TRANSIENT_BACKOFF_MS for tests. Tests cover: success path, transient auth requeue + banner, rate-limit banner, non-transient crash surfaced without requeue, and the 5-failure drop cap. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cursor): preserve slash-command isolation on requeue; wait for stderr flush Two findings from the cold-review bot on the requeue path: 1. `enqueueCursorUserMessage` uses `pushIsolated` for pass-through slash commands (e.g. `/compress`) so they never batch with sibling prompts. The transient-requeue path used plain `unshift`, which dropped the isolate bit and allowed the next collected batch to merge the slash command with a sibling - changing command semantics. Add `MessageQueue2.unshiftIsolated` and use it when the popped batch was isolated or when `parseCursorSpecialCommand` recognises the message. 2. `runAgentProcess` resolved on `child.on('exit', ...)`. Node may emit `exit` while the stderr pipe is still draining, so a fast "auth required" error printed-and-exited could be classified as non-transient with empty stderr and silently drop the user message - the exact bug this PR was supposed to fix. Resolve on `close` instead, which waits for stdio streams to flush. Adds a unit test that requeues `/compress` after a transient auth failure and asserts the second spawn still receives the slash command alone (not batched with a sibling). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cursor): restrict transient retry to exit code 1 only Upstream codex review tiann#823 (Minor): the helper treated any non-zero exit with matching stderr as transient, which could requeue a signal-killed (SIGTERM 143, SIGKILL 137) or crashed (SIGABRT 134) process whose stderr happens to contain a keyword like "rate limit". Documented contract is exit-1-for-transient; tighten the classifier accordingly. Adds regression test covering exit 143 + rate-limit stderr → no retry. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cursor): clean up transientBackoff abort listener on timer completion Upstream codex review tiann#823 (Minor): transientBackoff added an abort listener with { once: true } but only removed it when the abort fired. Because the launcher reuses one AbortController, repeated transient retries accumulated stale listeners until the next abort. Switch to a single completion path that clears the timer AND removes the abort listener whichever side wins. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cursor): cap in-memory stderr capture at 8 KB Upstream codex review tiann#823 (Minor): runAgentProcess accumulated every stderr chunk for the full child lifetime. A noisy `agent` failure could grow CLI process memory without bound even though only the first 400 chars are ever displayed. Cap the retained copy at 8 KB; debug log of the full stream is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…#825) * fix(hub): preserve flavor session ids in metadata across archive transitions When a session ends (terminate, crash, local-launch failure, handoff), the runner's archive write replaces sessions.metadata wholesale. If the CLI's locally cached Metadata is null (e.g. Zod parse failed at bootstrap and api.ts nulled it out) or stale, the spread in archiveAndClose ships a sparse blob and the resume token (cursorSessionId, codexSessionId, claudeSessionId, etc.) gets cleared from the row even though the on-disk chat data is still intact. Fix at the hub layer because update-metadata is the single chokepoint for every metadata write surface (CLI, web, future): in the store-level updateSessionMetadata, read the prior row's metadata inside a transaction and carry forward a small allowlist of flavor resume tokens when the incoming write omits them. Explicit overwrites still win. The allowlist mirrors pickExistingSessionMetadata in sessionFactory.ts which already preserves the same fields on bootstrap. Closes tiann#820 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(hub): address cold-review findings on metadata merge Three bot findings on the initial patch: 1. (P1) Sparse archive payloads still resulted in metadata blobs that failed MetadataSchema parse downstream — required `path`/`host` were not in the carry-forward set, so even though the resume token survived, hub session cache and CLI getSession nulled-out the row and resume_unavailable came back. Add PARSE_IDENTITY_FIELDS = `path`, `host` to the carry-forward. 2. (P2) Preserving `cursorSessionProtocol` whenever it was omitted carried a stale protocol over to a freshly written `cursorSessionId`, misrouting a future remote resume. Pair-aware logic: drop the prior protocol when next sets a new id; preserve the protocol only when next is silent on both id and protocol. 3. (P2) The successful update-metadata broadcast emitted the pre-merge payload to other CLIs in the session room, so even though the DB row was preserved, peer caches diverged. Switch the broadcast value to `result.value` (the persisted merged value) so live caches stay in sync with the truth. Refactor preserveProtocolResumeFields into mergeSessionMetadata with two tiers (PARSE_IDENTITY_FIELDS + SIMPLE_RESUME_TOKENS) plus the cursor pair handler. 6 new tests cover the regressions; existing 16 still pass plus 1 new socket-level test for the broadcast. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(hub): preserve flavor + machineId across sparse metadata merges Bot P2 on the prior fix: PARSE_IDENTITY_FIELDS (path, host) made the blob parseable and SIMPLE_RESUME_TOKENS preserved the chat-id, but flavor and machineId were still being dropped by sparse archive payloads. Consequences: - flavor: hub/src/web/routes/sessions.ts and sync/syncEngine.ts read `metadata?.flavor ?? 'claude'` to pick which session id field to resume. With flavor missing, a Cursor/Codex/Gemini session was routed as Claude and the preserved cursorSessionId was ignored. - machineId: telegram/bot.ts and the CLI's resumable listing read `metadata?.machineId` to scope sessions to the current host. With machineId missing, the row dropped out of the resume picker. Add a third carry-forward tier ROUTING_FIELDS = [flavor, machineId] between PARSE_IDENTITY_FIELDS and SIMPLE_RESUME_TOKENS in mergeSessionMetadata. 3 new tests cover preservation, no-invention, and explicit override. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(hub,cli): support explicit-clear sentinel for carry-forward fields Upstream cold-review (Major): the carry-forward semantics introduced in the prior commits ("omit field → preserve from prior") collide with cli/src/codex/session.ts resetCodexThread(), which intentionally clears codexSessionId by deleting it from the metadata blob before calling updateMetadata. With omit-as-preserve, the cleared id was restored from the prior row and /clear on a Codex session no longer dropped the persisted thread. Add an explicit-clear sentinel: when next sets a carry-forward field to `null`, the merge drops the key entirely from the persisted blob (key removed; not stored as null since MetadataSchema fields are `string().optional()`). `undefined` (key missing from next) keeps its "carry forward" meaning. The two semantics now compose cleanly: - next.field = "x" → next wins (caller sets a new value) - next.field = null → drop the field (caller intentionally clears) - next omits field → carry forward prior (caller didn't touch it) Update resetCodexThread() to send `codexSessionId: null` so the reset actually drops the persisted thread under the new merge. 4 new hub tests cover: explicit clear of a single token, clear-one- preserve-others independence, no-op clear on a never-set field, and the success-ack value reflects the cleared blob. cli/src/codex tests (224/224) and hub suite (301/301) green; bun typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…ive rows (tiann#826) * feat(hub+web): add POST /sessions/:id/reopen + Reopen button on inactive rows Archived sessions retain their full transcript and metadata in the DB, but today there is no path back to them from the web UI; the only way to revive one is shell access plus sqlite metadata patching plus a manual /resume call. This change adds a single one-click affordance: - Hub: new POST /api/sessions/:id/reopen route on the existing sessions router. The route delegates to a new engine method `reopenSession` that: - is idempotent (active session -> 200 with `resumed:false`), - validates Cursor sessions still have a `cursorSessionId` once they have any messages (otherwise we cannot resume the agent thread), - clears `lifecycleState='archived'`, `archivedBy`, `archiveReason` via a versioned metadata update, and stamps `lifecycleStateSince`, - defaults `cursorSessionProtocol='stream-json'` for pre-tiann#799 Cursor sessions (sessions that have a `cursorSessionId` but no protocol set), so routing still reaches the legacy launcher; ACP sessions keep their explicit protocol, - forwards to the same `resumeSession` path the existing /resume route uses, including the `canFreshSpawnNeverStartedSession` fallback. 422 is returned with `{ missing: [...] }` when the agent metadata needed to resume is gone; other engine errors map to 404/409/503/500 with the existing shape (mirrors /resume). - Web: a "Reopen" entry in the SessionActionMenu that appears next to "Delete" on inactive sessions only. Wired into both the SessionList rows and the SessionHeader more-menu, with a small dismissable error dialog for the 422 missing-metadata case. - Tests: route-level coverage for the four response shapes (200 reopen, 200 idempotent, 404, 422) plus 409/503 error mappings; sessionCache tests for the archive-metadata clear (including the legacy Cursor protocol default); React component test for the menu item rendering on inactive vs active sessions; mutation hook test for the api wiring and the ApiError surface needed by the UI. Closes tiann#819 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(reopen): address codex review findings on fork PR #33 Four P2 findings from the cold-review bot, three fixed and one explained: 1. Mutation now returns the reopen response so the UI can route to a possibly different sessionId. SyncEngine.resumeSession may merge the row into a freshly-spawned session id (matching the send-message resume flow); the chat view now navigates there, the row list calls onSelect on the new id. 2. reopenSession on the client now goes through `request()` instead of a hand-rolled fetch, so 401 + onUnauthorized refresh works the same as every other session action. `request()` now throws `ApiError` (with status/code/body) on non-401 errors - backward compatible because ApiError extends Error. 3. (Reply only) Pre-tiann#799 Cursor protocol propagates correctly without the extra plumbing the bot suggested: `clearSessionArchiveMetadata` writes `cursorSessionProtocol='stream-json'` to the DB; the CLI's `bootstrapExistingSession` preserves it via `pickExistingSessionMetadata`; if it's still absent at the launcher, `isLegacyCursorSession` defaults to stream-json whenever `cursorSessionId` is present. 4. Archive metadata is now restored when resume fails. `reopenSession` captures a snapshot of `lifecycleState`/`archivedBy`/`archiveReason`/ `lifecycleStateSince` before the clear; if `resumeSession` returns an error (no machine online, spawn timeout, etc.), the snapshot is put back via the new `SessionCache.restoreSessionArchiveMetadata`. Engine test covers both the rollback and the no-rollback-on-success cases. Error rendering helper moved to `web/src/lib/reopenError.ts` so the chat header and the session row share one implementation, and gained a unit test. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(web): preserve engine error codes in ApiError.code on /reopen `/sessions/:id/reopen` returns `{ error, code }` where `code` is the stable taxonomy (`no_machine_online`, `resume_unavailable`, etc.) and `error` is the human-readable message. The generic `request()` error path was reading only `parsed.error`, so `ApiError.code` ended up being a message like "No machine online" rather than `no_machine_online`, breaking taxonomy-based branching in web callers. `parseErrorCode` now prefers `parsed.code` and falls back to `parsed.error` for legacy routes that only set `error`. Added api/client.test.ts covering the three response shapes /reopen actually emits (503 with code, 500 without code, 422 with missing[]). Addresses upstream codex-action review on tiann#826. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(reopen): restore archive metadata exactly on rollback (drop fresh lifecycleStateSince) For an archived session that predates `lifecycleStateSince` (the field is absent from its metadata), `clearSessionArchiveMetadata` stamps a fresh timestamp. If `resumeSession` then fails, the rollback was leaving that fresh timestamp in place, making the rolled-back row look like it was just archived rather than preserving the original lifecycle age. `restoreSessionArchiveMetadata` now does an EXACT restore: when a snapshot field is undefined the corresponding key on the metadata is deleted, not left alone. Applies symmetrically to lifecycleState / archivedBy / archiveReason / lifecycleStateSince. Test updated to assert the deletion of the fresh timestamp. Addresses upstream codex-action review on tiann#826. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…eam-json path (closes tiann#822) (tiann#828) * fix(cursor): drop timing heuristic from tiann#784 intercept; scan raw payload (tiann#801 follow-up) PR tiann#801 shipped a two-strategy intercept for the synthetic AskQuestion skip response in legacy stream-json mode. Real-traffic data from a post-merge run shows the marker-match strategy never fires (the converter's `extractToolResult` discards the marker for tool shapes it does not recognize, returning `{}`) and the timing-signature defense-in-depth strategy fires only on false positives - notably the Anthropic Vertex Claude tool calls cursor-agent surfaces in legacy sessions, which all land as `name=unknown` with the `{}` extracted result and frequently complete under the 500 ms threshold. Measured on a single legacy-resumed session (`7b769423`): 1,136 `name=unknown` tool calls, 16 rewritten as `no_input_surface`, zero actual marker strings stored anywhere in the session. The 16 rewrites were legitimate fast tool calls (Anthropic Vertex `toolu_vrtx_*` IDs) mischaracterized as fabricated skip responses. Changes: - Remove the timing-signature heuristic and its supporting state (started-at map, elapsed-ms calculation, latency threshold, test-only state reset). - Move the marker scan from the post-`extractToolResult` output to the raw `tool_call` payload, so it can see the marker on stream-json shapes the converter does not specifically recognize. Function-shaped tools exclude `function.arguments` from the scan to avoid matching agent-controlled input. Other shapes scan the full payload (no agent-input field exists at the top level). - Refresh tests: drop timing-based positive cases, add a marker-in-raw- payload positive case for `name=unknown` shapes, and add a regression that legitimate fast `name=unknown` tool calls without the marker pass through with `status: completed`. - Document scope: this intercept now lives only on the legacy stream- json path, which only resumed pre-ACP sessions hit. New cursor remote sessions go through `cursorAcpBackend` and the `cursor/ask_question` ACP extension method (tiann#799) - immune to this bug. The intercept drains with the legacy session population. Tracking: tiann#784. Builds on tiann#801, complements tiann#799. * fix(cursor): exclude agent input from marker scan; surface top-level Anthropic tool names (Codex P2) Codex flagged a false-positive case on the fork-stage review of this branch (#35, P2): an Anthropic tool_use shape with a top-level `name` (e.g. `{id, name: 'TodoWrite', input: { ... }}`) gets labelled `name=unknown` by the converter and passes the AskQuestion gate. If the agent's `input` quotes the synthetic-skip marker - which happens whenever an agent edits or documents this very bug - the intercept would rewrite a perfectly fine TodoWrite as a fabricated skip. Two-part fix: 1. `extractToolName` now reads the top-level `name` field as a final fallback. A real `TodoWrite` / `Bash` / `str_replace_based_edit_tool` surfaces with its actual name and is rejected by the AskQuestion gate before the marker scan runs. The original AskQuestion fabrication case still surfaces as `unknown` (per tiann#784 issue body the name is stripped in the fabricated payload) and remains detectable. 2. Defense in depth: introduce `AGENT_INPUT_KEYS = {input, args, arguments}` and exclude these from the non-function shape's marker scan. Even if a tool reaches this code path with `name=unknown` and the marker buried in its `input`, the intercept won't fire on agent- controlled text. Two new regression tests: - Anthropic tool_use shape `{id, name: 'TodoWrite', input: {todos: [ marker]}}` → passes through with `status: 'completed'`. - `name=unknown` shape with marker only inside `input` → passes through with `status: 'completed'`. All 20/20 tests pass; typecheck clean (cli + web + hub).
tiann#835) Fixes incomplete cliModelSkus while agent acp holds the CLI lock (tiann#831) and replace single-pid ACP lock with cross-process refcount (tiann#832). Web picker merges machine/session catalogs and waits for SKU readiness before showing variant labels. Fixes tiann#831 Fixes tiann#832 Co-authored-by: Cursor <cursoragent@cursor.com>
…#836) * fix(web): dedupe sidebar sessions by flavor resume id Wire deduplicateSessionsByAgentId into SessionList and resolve cursor threads via cursorSessionId so resume/archive no longer shows duplicate inactive rows for the same ACP session. Fixes tiann#833 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(web): use SessionSummary.agentSessionId for sidebar dedup SessionList only receives SessionSummary from the API; native ids like cursorSessionId are already mapped into metadata.agentSessionId by toSessionSummary. Drop resolveAgentSessionIdFromMetadata to fix typecheck. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(web): hide inactive empty session stubs in sidebar Filter inactive rows with no agentSessionId and no title signal before grouping sessions, and expose lifecycleState on SessionSummary for future sidebar rules. Completes the tiann#833 P0 follow-up alongside agent-id dedup. Fixes tiann#833 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(web): scope sidebar dedup key by flavor Prevent cross-flavor collisions when flattened agentSessionId retains a stale native id. Add regression test and relax claudeRemote CI timeout. Fixes tiann#833 Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
) Pre-write resume token into session metadata before awaiting session/load so hub and web see cursorSessionId immediately after resume spawn. Fixes tiann#834 Co-authored-by: Cursor <cursoragent@cursor.com>
…rimitive (tiann#798) * feat(web): scratchlist v1.1 — composer-toggle drawer + reusable FUE primitive The v1 always-visible amber band proved too heavy for what's a 20% feature in a typical session. v1.1 dials it back to a composer-toggle that opens an on-demand drawer, paired with a reusable FUE (First-User Experience) primitive so existing operators get a subtle pulsing dot + on-click explainer the first time they see the toggle. UX changes: - Notepad icon in the composer toolbar (next to schedule-send) toggles scratchlist mode. Drawer renders only while mode is on. - Composer's send button repaints amber and reads "Send to scratchlist" while the mode is sticky; submit routes adds into the scratchlist instead of the chat. Click the icon again to leave. - Small entry-counter badge appears on the toggle when entries exist; empty-state shows just the icon (no zero-state guilt UI). New reusable FUE primitive: - web/src/lib/use-fue.ts: state machine (unseen → engaging → acknowledged) with localStorage persistence, namespaced under hapi.fue.v1.<featureId> so it can't collide with any future upstream onboarding flow. - web/src/components/Fue.tsx: <FueDot> (small pulsing badge) and <FueCallout> (portal-rendered popover with title/body + "Got it" affirmative-action dismiss). No auto-timeout — reading speed varies and silent disappearance undercuts user trust. - AGENTS.md adds a "Adding new web features — consider an FUE" section so future contributors discover the primitive. Refactors: - ScratchlistPanel.tsx: split rendering into <ScratchlistInventory> (presentational list) and <ScratchlistDrawer> (composer-controlled drawer with hint copy). Original <ScratchlistPanel> kept exported for the existing fixture-based tests. - SessionChat.tsx: scratchlist state lifted into useScratchlist hook so the composer-toolbar counter and the drawer share one source of truth. onSend wrapped to route through scratchlist.add when mode is on. Tests: - 9 useFue hook tests (initial state, engage idempotency, no auto-acknowledge, dismiss, featureId switching, post-acknowledged engage no-op, resetFue helper). - 5 placement helper tests (above/below switching, viewport edge clamping, visualViewport offset support). - All 21 existing scratchlist lib tests + 14 ScratchlistPanel tests continue to pass. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): prevent cross-session leak in useScratchlist hook Per upstream review on PR tiann#798 (github-actions[bot] [Major]): > useScratchlist persists current `entries` whenever `sessionId` > changes. On A -> B navigation, React first commits with B's id > and A's entries; after paint, this persist effect can write A's > entries to hapi.scratchlist.v1.B before the rehydrate effect > loads B. The previous keyed panel existed specifically to avoid > this race. Lifting state out of the v1 panel (which sidestepped the race via key={props.session.id} forced remount) re-introduced this same data- loss window. The composer-controlled drawer in v1.1 cannot remount on session change because its parent SessionChat doesn't either. Fix: keep the loaded sessionId in state alongside the entries so they swap atomically, and persist against the LOADED sessionId rather than the prop. After A->B, the loaded sessionId is still A until rehydrate runs, so a spurious persist re-writes A's storage with A's entries - a no-op instead of a corruption. Tests: - New use-scratchlist.test.ts with 6 tests: - hydrates from localStorage on mount - add() persists to current session's storage only - rerender to a new session preserves the new session's existing entries - after switching, add() targets the new session - regression test that spies on Storage.prototype.setItem and asserts the rerender lifecycle never produces a (B-key, A-entries) write - remove()/move() target the loaded sessionId - The setItem-spy test correctly fails against the buggy code (verified by temporarily reverting the fix) and passes with the fix in place. - Full web suite: 88 files, 756 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): route attachment/scheduled submits to chat instead of dropping them Per upstream review on PR tiann#798 (github-actions[bot] [Major]): > Prevent scratchlist mode from dropping attachments — in scratchlist > mode the wrapper returns success after adding only `text`, while > HappyComposer still treats composer attachments as sendable input. > A text+attachment submit therefore routes through this branch, > stores only the text, and silently discards the attachment instead > of sending or preserving it. Same hazard applies to scheduledAt: scratchlist entries are pure-text notes - they can't represent attachments or schedule metadata - so any submit carrying either MUST fall through to props.onSend (chat) even when the scratchlist toggle is on. Otherwise the wrapper short-circuits to scratchlist.add(text), reports success to the composer, and the composer dutifully clears attachments + schedule that the user just queued. Fix: extracted the routing rule into shouldRouteToScratchlist(mode, attachments, scheduledAt) - returns true only when mode is on AND the payload is pure text. onSendForComposer uses it. Tests: - 5 new shouldRouteToScratchlist unit tests (mode off, mode on + text-only, mode on + attachments, mode on + schedule, mode on + both) - All in web/src/components/SessionChat.test.ts (13 tests total now) - Full web suite: 88 files, 761 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): clear pendingSchedule when scratchlist-mode submission falls back to chat Per upstream review on PR tiann#798 (github-actions[bot] [Major]): > The follow-up change correctly falls through to props.onSend when > scratchlist mode is on but scheduledAt is present, yet the > accepted-send cleanup still checks only !scratchlistMode. That > means a scheduled chat send made while the amber scratchlist UI is > active is accepted, but pendingSchedule stays set, so the next > normal send can accidentally reuse the same schedule. Fix: handleSend now gates the cleanup branch on the actual route taken (routedToScratchlist) rather than the scratchlist UI state. Reuses the same shouldRouteToScratchlist helper so route + cleanup share a single source of truth. Tests: - 2 new tests in SessionChat.test.ts that pin the decision matrix handleSend depends on: - 'cleanup gate: scheduled chat send while scratchlist toggle is on still clears schedule' - 'cleanup gate: pure-text scratchlist add does NOT clear schedule' - Full web suite: 88 files, 763 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): UnifiedButton must reflect actual routing, not raw scratchlist toggle Per upstream review on PR tiann#798 (github-actions[bot] [Major]): > Send button advertises scratchlist routing even when the submit > will go to chat — shouldRouteToScratchlist correctly falls back > to normal chat for attachments or scheduledAt, but UnifiedButton > still turns amber and labels the action as "Send to scratchlist" > whenever scratchlistMode is true. A scheduled send or attachment > send made in that state will be submitted to chat while the UI > says it is being stashed, which can send content to the agent > unexpectedly. Fix: - UnifiedButton's prop renamed `scratchlistMode` -> `routesToScratchlist` to make the contract explicit: "this submit really will go to the scratchlist", not "the scratchlist toggle is on". - The call site computes `routesToScratchlist` from `scratchlistMode && !hasAttachments && pendingSchedule == null`, mirroring SessionChat's shouldRouteToScratchlist exactly. The button is now amber + "Send to scratchlist" only when the actual send path will hit scratchlist; attachments / pending schedule force a chat- style render that matches the real routing. - UnifiedButton exported so it can be unit-tested directly. Tests: - 3 new render tests in ComposerButtons.test.tsx covering: - routesToScratchlist=true → amber + "Send to scratchlist" - routesToScratchlist=false → black + "Send" (the regression case) - omitted prop → defaults to chat-style render - Full web suite: 89 files, 766 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): exit scratchlist mode when promoting an entry to the composer Per upstream review on PR tiann#798 (HAPI Bot, follow-up after b256fe5): > Found one major issue: promoting a scratchlist item to the composer > keeps scratchlist mode enabled, so the next send re-adds it to the > scratchlist instead of sending to chat. Promoting an entry to the composer means "I want to send this for real now". With scratchlist mode still on, the next composer submit routes back to scratchlist (per the v1.1 modal-mode contract), so the user's click loop becomes promote -> send -> re-add -> nothing-actually-sent. Fix: ScratchlistDrawerHost now calls onExitScratchlistMode whenever it promotes an entry to the composer. Promote-to-queue does NOT exit the mode (queue path bypasses the wrapper anyway, and the operator may still be capturing related notes). Tests: - Exported ScratchlistDrawerHost so its host-level callbacks can be unit-tested in isolation (previously only ScratchlistDrawer was testable; the wiring was untested). - New SessionChat.exit-mode.test.tsx with 2 tests: - promote-to-composer fires setText AND onExitScratchlistMode - promote-to-queue fires onSend but does NOT exit mode - Full web suite: 90 files, 768 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(scratchlist): Ctrl/Cmd+Shift+S toggles scratchlist mode (v1.1 hotkey) The v1 always-visible panel had Ctrl/Cmd+Shift+S to expand the panel and focus the input. v1.1 mounts the drawer only when scratchlistMode is on, so the v1 listener (inside the panel) is dead code: it can't fire while the drawer is unmounted, and the user has no way to open the drawer without clicking the toolbar icon. Re-bind the shortcut at SessionChat scope so it's always alive and toggles the mode. Convention matches sibling globals (Ctrl/Cmd-m cycles agent model). Ctrl/Cmd-Shift-S is unreserved by Chrome / Firefox / Safari (browser Save As is Ctrl-S / Cmd-S, no Shift), so the user's save-page muscle memory keeps working. Modifier requirement (Ctrl/Cmd+Shift) means it can't collide with literal-character typing in any input - no focus suppression needed. The matcher is extracted to a pure helper isScratchlistToggleHotkey so it's unit testable without mounting SessionChat. 6 new tests pin the modifier matrix: - Ctrl+Shift+S (Linux/Windows) -> match - Cmd+Shift+S (macOS) -> match - Cmd/Ctrl+S without Shift -> reject (browser Save reservation) - bare S / Shift+S -> reject (literal typing) - Ctrl+Shift+Alt+S -> reject (avoid OS clashes) - other modifier+key combos -> reject Tooltip + FUE body now mention the hotkey so it's discoverable from the same UI surface that introduces the feature (en + zh-CN). Web suite 90 files / 774 tests, all green. Typecheck clean. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): hotkey skips dialogs / inputs / contentEditable Bot finding on PR tiann#798 (PRRT_kwDOQuQOSc6HGtLn): the window-level Ctrl/Cmd+Shift+S listener fires for every focus target, so the shortcut can toggle scratchlist mode "behind" an open modal (rename session, schedule picker, FUE callout, image preview), making the next composer send route to scratchlist instead of chat. UX bug. Add isScratchlistHotkeyBlockedTarget(target) and gate the listener on it. Block targets: - any descendant of an open [role="dialog"] (Radix UI's DialogContent renders role="dialog"; FueCallout, ScheduleTimePicker, ImagePreview also use role="dialog") - HTMLInputElement (single-line inputs) - HTMLSelectElement - any contentEditable host (with attribute-based fallback for jsdom, which doesn't implement isContentEditable) NOT blocked: - HTMLTextAreaElement (the composer textarea is the expected focus target when the operator presses the hotkey - blocking would defeat the shortcut) - the document body / unfocused targets 8 new unit tests pin the matrix. Function is exported / pure so callers can reuse the same blocked-target rule for future global shortcuts. Suggested fix from the bot applied modulo: - Use !== null on closest() result (explicit boolean for return type) - Add attribute-based contentEditable fallback for jsdom test env * feat(scratchlist): copy-to-clipboard action on each entry Add a per-entry "Copy to clipboard" button between the send-to-queue and delete actions. On click, write the entry text via the shared safeCopyToClipboard helper (which already handles the navigator.clipboard primary path + the execCommand fallback for Safari / non-secure-context edges); on success, briefly flip the icon to a check and the aria-label/title to "Copied!" for 1500ms so the operator gets visual + screen-reader confirmation. Failures (clipboard denied AND execCommand fallback unavailable) silently no-op rather than throw at the click handler. Mirrored across both surfaces: - ScratchlistInventory (used by the v1.1 composer-toggle drawer) - ScratchlistPanel inline list (the v1 always-visible panel) A small useCopiedFeedback() hook owns the "which entry just got copied" state + the 1.5s auto-clear timeout. Pure state machine; the caller wires safeCopyToClipboard separately so the hook itself stays free of jsdom clipboard quirks. Cleared on unmount via the standard ref-tracked timeout pattern, so promote-and-navigate-away can't leak. Locale keys: scratchlist.action.copy / scratchlist.action.copied (en + zh-CN). Three new tests: - v1 panel happy path: writeText called with the entry text, button flips to the "Copied!" label, entry is preserved (copy is non-destructive). - v1 panel failure path: writeText rejects AND execCommand returns false; button stays in "Copy to clipboard" state — no false success. - v1.1 drawer happy path: writeText called, label flips, and crucially no other entry handlers (onSend, onDelete, setText, onExitScratchlistMode) fire — copy is independent of all the other actions. Web suite 90 files / 785 tests, all green. Typecheck clean. * fix(scratchlist): reset all per-session state via keyed wrapper Bot finding on PR tiann#798 (PRRT_kwDOQuQOSc6HHOsa): when the operator navigates between sessions on the same route (/sessions/A -> /sessions/B), React reuses the SessionChat component instance. Effects run AFTER the first paint, so for a single render window the new session is rendered with the previous session's scratchlist entries (useScratchlist's rehydrate-effect) AND drawer-open state (scratchlistMode reset effect). Visual leak; drawer actions targeting stale state. Apply the bot's suggested fix verbatim modulo the type extraction: export function SessionChat(props) { return <SessionChatInner key={props.session.id} {...props} /> } Canonical React idiom for "fully reset state on prop change": the keyed wrapper unmounts and re-mounts the inner component when session.id changes, so every hook (useScratchlist's initial-state factory, useState, useHappyRuntime, ...) starts fresh. This supersedes the now-redundant effect-based reset: - useEffect(() => { setScratchlistMode(false) }, [session.id]) REMOVED useScratchlist's atomic-loaded-sessionId persistence (added on the prior PR round) stays as defense-in-depth for any caller that uses the hook without the keyed-wrapper pattern. Web suite 90 files / 785 tests, all green. Typecheck clean. * fix(web): retain composer text on send failure (closes tiann#776) When the message composer submits and the hub responds with a 4xx/5xx or the fetch fails outright, assistant-ui clears the composer synchronously the moment send is invoked. Without intervention the operator's typed text is destroyed at exactly the moment they most need it preserved. SessionChat additionally clears any pending schedule on accept, so a failed scheduled send was also silently downgrading to immediate on the next attempt. Behaviour: - useSendMessage exposes onError({ sessionId, text, scheduledAt, error }) so the route can hand the input back to the composer. sessionId is the resolved target (post-resolveSessionId), so an inactive-session resume that resolves a new id, kicks off async navigation, then fails the POST restores into the resumed session's composer rather than the old one. - router.tsx stores sendErrors keyed by sessionId. Per-session lookup replaces the clear-on-session-change effect, so errors do not bleed between sessions and a session-scoped failure persists across navigation. - HappyComposer accepts ComposerSendError, restores text via api.composer().setText() once per failure id, and re-establishes any pending schedule via onSchedule({ type: 'absolute', ms: scheduledAt }). It renders a red ring on the composer wrapper and a role="alert" inline message; both clear the moment the operator types or sends. - onError forks on input.attachments. Text-only sends use the composer-restore path (removeOptimisticMessage drops the row so the failed bubble does not duplicate the restored text). Attachment sends keep the legacy failed-bubble UX (status='failed' + in-thread retry button) because the composer-restore path can't reinstate uploaded attachment metadata. retryMessage extracts attachments from the stored optimistic message via getMessageAttachments so failed-bubble retry of an attachment send re-fires with its files. Acceptance (issue tiann#776): - Submit -> 500/502/503/network error -> composer text not cleared - Submit -> 400/401/403 -> composer text not cleared, error inline - Submit -> 2xx -> composer clears as today - Operator can edit retained text and retry without re-typing - Failed scheduled sends restore as scheduled, not as immediate Tests in web/src/hooks/mutations/useSendMessage.test.tsx cover text-only 4xx/5xx/network retention, scheduled-send carry-through, optimistic-row removal on text-only failure, sessionId carry-through under resolveSessionId, attachment failure fallback, and attachment retry preservation. Full web suite passes (705 tests). bun typecheck clean. No SCHEMA_VERSION bump (frontend-only). * fix(test): correct AttachmentMetadata fixture shape + JSX namespace import Two pre-existing test-only typecheck failures surfaced once scratchlist v1.1 was stacked into the driver soup. * SessionChat.test.ts - the attachment() fixture used the legacy schema (kind, sizeBytes) instead of the current AttachmentMetadataSchema (filename, size, path). Updated to match the live shape so the cast is honest. * ComposerButtons.test.tsx - JSX namespace is no longer global under the current TS lib config; switched the helper signature from JSX.Element to React's ReactElement (same runtime, named import). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(scratchlist): soften panel chrome - drop strong amber fill, keep subtle accent border (tiann#812) Per tiann#812 (and PR 827 from @swear01) the always-visible amber fill on the scratchlist panel was too loud as a scroll element. This swaps the warning *fill* for the chat-user-surface tone and uses neutral text/pills/focus, but keeps the warning *border* as a soft accent so the panel still reads as a different destination from a normal user message. The strong destination signal continues to live on the composer Send button (it goes amber-500 only while scratchlist mode is routing) and the active toggle button - those carry the moment-of-action signal the user actually presses, and ComposerButtons tests + the FUE copy already depend on that behavior, so they're unchanged. Credit to @swear01 (PR 827) for the styling note; this branch absorbs that restyle and supersedes the Settings-toggle approach because v1.1 hides the panel by default behind the composer drawer toggle (no Settings entry needed). Adds a regression-guard test asserting the panel uses the chat-user-surface bg + warning-border (not the warning fill). Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
… to ACP Closes tiann#824 When the operator reopens a legacy stream-json Cursor session in HAPI, the hub now transparently transplants its `~/.cursor/chats/<wsh>/<uuid>/store.db` into `~/.cursor/acp-sessions/<uuid>/`, verifies it loads via `agent acp`, flips `metadata.cursorSessionProtocol = 'acp'`, and removes the legacy source - all before `resumeSession` returns. Subsequent opens are pure ACP. The primary justification is safety, not feature parity. tiann#784 (`cursor-agent` fabricates `Questions skipped by the user` responses in legacy stream-json mode) still fires regularly in dogfood despite tiann#801's mitigation: the agent ships destructive side effects against fabricated consent. Migration to ACP closes the protocol-level door because the `AskQuestion` tool does not exist on the ACP side, so there is nothing to fabricate. working. That tradeoff was reasonable at the time. The accumulated tiann#784 evidence makes legacy sessions actively unsafe; this PR makes the upgrade path invisible enough that users stop avoiding it. A pre-PR spike established that legacy and ACP `store.db` files use the identical SQLite schema; only the directory layout differs. The migrator therefore: 1. Sanity-checks the source store and pre-flips state (`session.active`, `lifecycleState`, on-disk presence, target collision) 2. Optionally archives a stale-running row (`forceArchiveRunning: true` is the default for the auto-migrate path because the caller already verified `session.active === false`) 3. Atomically creates `~/.cursor/acp-sessions/<uuid>/` with mode `0o700` 4. Copies `store.db` and chmods to `0o600` (multi-user-host hardening) 5. Writes a minimal `meta.json` sidecar (`schemaVersion`, `cwd`, optional `title`) with mode `0o600` 6. Spawns `agent acp` under HAPI_HOME isolation and verifies the session loads via `session/load`. On long histories the verify also drives a trivial single-turn prompt; on short ones load-only is enough 7. Flips `cursorSessionProtocol = 'acp'` AND clears the `cursorMigrationState` banner flag in a SINGLE metadata write 8. Removes the legacy source store (only after verify succeeded and the protocol flip committed). The legacy `~/.cursor/chats` parent dir is left as-is Every failure leaves the legacy state intact. No `rm` fires without a verify success AND a committed protocol flip. The transplant takes 15-20s on long histories (copy a multi-hundred-MB store, spawn `agent acp`, replay thousands of notifications, tear down the probe). Without a progress indicator the wait reads as "broken" to a fresh reviewer. A minimal banner ships alongside the migrator: - Hub sets `metadata.cursorMigrationState = 'in_progress'` BEFORE the long-running transplant. The session-cache refresh emits the existing `session-updated` SSE event (no new event type), so the web client picks it up in milliseconds. No client-side polling needed. - Hub clears the flag in the SAME metadata write that flips `cursorSessionProtocol` to `'acp'` on success, so the banner disappears in the same render tick the chat re-renders as ACP - no flicker window. - Hub clears the flag explicitly in the auto-migrate helper's `finally` on failure/exception, so the banner never gets stuck if migration falls back to the legacy launcher. - Web renders an accessible (role=status, aria-live=polite) banner with an indeterminate spinner. Deliberately no fake percentage - we do not have phase data and a fake progress bar would lie. This PR is intentionally sequenced AFTER swear01's three ACP mop-up PRs (merged today as ad038bb, 8094b50, fa363c2), all of which are prerequisite for safe concurrent ACP launches. The verify probe spawns `agent acp` directly via `AcpVerifyProbe` under HAPI_HOME isolation (the migrator overrides `HOME` to a temp dir for the verify pass), so it never touches `<real-HAPI_HOME>/locks/agent-acp-active/` at all. Per swear01's tiann#835 design note, the post-flip ACP launcher claims the lock through the standard `registerActiveAcpTransport` entry and behaves like any other concurrent ACP start. The migrator itself never writes `pid` or `count` files directly. The auto-migrate path is gated by `HAPI_CURSOR_LEGACY_AUTO_MIGRATE`. Set to `0`, `false`, `no`, or `off` to suppress it entirely (legacy sessions keep running through the existing stream-json launcher). Default is on. A REST endpoint at `POST /api/sessions/:id/migrate-to-acp` allows explicit migration of a single session outside the sync-on-open path (e.g. for a specific cold archived session a user wants to re-engage). Bulk migration surfaces (CLI subcommand, web button, bulk REST endpoint, candidate-listing API) were deliberately stripped per reviewer feedback; per-session sync-on-open + this escape hatch are the only two paths. - 343 hub unit tests (4 new for the migration banner flag transitions, 53 for the migrator core, 32 for the verify probe, 15 for the auto- migrate helper guard matrix, plus existing suites) - 3 integration tests against a real `agent acp` (skipped by default unless `HAPI_CURSOR_LEGACY_MIGRATOR_INTEGRATION=1`) - 10 web unit tests for the banner component (visibility paths + a11y) - Typecheck clean across cli, web, hub Co-authored-by: Cursor <cursoragent@cursor.com>
heavygee
force-pushed
the
spike/cursor-legacy-to-acp-migrator
branch
from
35daa9f to
c1c1a0b
Compare
Owner
Author
|
@codex review Rebased + squashed onto upstream/main Stacking order is now satisfied: tiann#835 / tiann#836 / tiann#837 are merged into upstream/main. Ready to issue upstream as soon as you give a clean verdict (or operator can stamp |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1c1a0b64f
ℹ️ 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".
| // PATH. Thread the migrator's homeDir dep through so the | ||
| // probe doesn't fall back to process.env.HOME when a more | ||
| // specific lookup home is available. | ||
| agentLookupHome: this.deps.homeDir() |
There was a problem hiding this comment.
In service-account hub deployments where metadata.homeDir points at the human owner's Cursor install but os.homedir() is the service account, this default probe still resolves agentLookupHome from this.deps.homeDir() rather than the per-session home chosen from metadata.homeDir below. The migrator will find/copy the legacy store under the recorded home, but the verify probe will not append the owner's ~/.local/bin/~/.npm-global/bin to PATH, so migrations can fail with agent not found for exactly the cross-home case this comment says it covers.
Useful? React with 👍 / 👎.
Comment on lines
+114
to
+116
| if (!existsSync(pidsDir)) { | ||
| removeAcpLockDir(); | ||
| return false; |
There was a problem hiding this comment.
When another process calls isAgentAcpTransportActive() while a new ACP process is in registerActiveAcpTransport() after writeLockCount(...) but before addLockPid(...), the lock directory exists with no pids/ directory yet. This branch deletes that just-created live lock as stale, so the caller can immediately spawn a conflicting agent --list-models/ACP process during the startup window. Treat a count-without-pids lock as active (or create the pid entry before publishing the count) instead of removing it.
Useful? React with 👍 / 👎.
…ajors) Finding 1 (Major) — verify probe `agentLookupHome` ignored `metadata.homeDir` in service-account hub deployments. `migrateOne` resolved the legacy store under `metadata.homeDir` (the recorded session-owner home) but the default createProbe factory still set `agentLookupHome` from `this.deps.homeDir()` (the hub user's home). On a service-account hub, the store lookup succeeded but `agent acp` discovery fell back to the hub user's `~/.local/bin`, so verify silently failed and sync-on-open quietly fell back to legacy. Fix: - Widen `CursorLegacyMigratorDeps.createProbe` signature from `(env) => AcpVerifyProbe` to `(env, agentLookupHome) => AcpVerifyProbe` - Default factory uses the passed `agentLookupHome` - `verifyInTempHome` threads `opts.sourceHome` (already the resolved session-owner home) through as the 2nd arg 2 new regression tests pin the contract: - service-account case (metadata.homeDir != deps.homeDir()): captured agentLookupHome MUST equal metadata.homeDir - legacy session record (no metadata.homeDir): falls back to deps.homeDir() correctly Finding 2 (Major) — `bun.lock` win32-x64 pinned to 0.20.0 while `cli/package.json` required 0.20.1 (rebase artifact from the v0.20.0 → v0.20.1 release commit landing in upstream/main between the original spike and the rebase). Frozen-install Windows users would either get the wrong native binary or have the lock rejected. Fix: regenerated bun.lock so the entry resolves `@twsxtd/hapi-win32-x64@0.20.1`. `bun install --frozen-lockfile` now passes clean. Test budget: - 391 hub unit tests pass (2 new for createProbe agentLookupHome contract, +0 regressions) - Typecheck clean across cli + web + hub - `bun install --frozen-lockfile` clean Co-authored-by: Cursor <cursoragent@cursor.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
heavygee
added a commit
that referenced
this pull request
Jun 8, 2026
After successful resurrection the session is back to inactive + cursorSessionProtocol=stream-json by design (legacy launcher knows how to dispatch the stream-json store). Once #34's invisible auto- migrate ships, SyncEngine.resumeSession will transplant this to ACP transparently on the operator's next Reopen — no manual migrate command required. This adds three terminal-stdout lines at the end of the script so the operator sees the next step without having to remember #34 exists: [hapi-resurrect-session] session is on legacy stream-json. [hapi-resurrect-session] next: open this session in the HAPI web UI and click Reopen. [hapi-resurrect-session] once #34 (tiann#824) auto-migrate is live, that [hapi-resurrect-session] Reopen will transparently transplant this session to ACP [hapi-resurrect-session] (cp store.db, verify session/load, flip protocol; ~15-20s banner). [hapi-resurrect-session] kill-switch: HAPI_CURSOR_LEGACY_AUTO_MIGRATE=0 in hub env. If LIVE_PROTO is already 'acp' (e.g. operator re-ran resurrect on a session that auto-migrate had already handled), the script just prints "session is already on ACP. No further action needed." instead. Pure UX polish. No code-path change. Replaces the earlier attach-agent-chat.ts post-attach hint that I dropped — that approach was obsoleted by #34 moving migration from operator-driven CLI to invisible auto-fire inside resumeSession. 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
3 participants
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
Adds invisible, automatic, per-session ACP migration triggered on first
open of a legacy stream-json Cursor session. The hub transplants the legacy
~/.cursor/chats/<wsh>/<uuid>/store.dbinto~/.cursor/acp-sessions/<uuid>/,verifies it loads via
agent acp, flipscursorSessionProtocol = 'acp',removes the legacy source — all before
resumeSessionreturns its target.The user sees one upgrade pause; subsequent opens are pure ACP.
Sees
tiann/hapi#824for the design and the spike report atdocs/plans/2026-06-06-cursor-legacy-to-acp-spike.mdfor the underlyingevidence (identical SQLite schema, only the directory layout differs
between legacy and ACP).
Why this is safety, not feature-parity
This PR's primary justification is remediation for
#784(cursor-agentfabricates
Questions skipped by the userresponses in legacy stream-jsonmode). Despite
#801's mitigation, the bug still fires regularly indogfood: the agent ships destructive side effects against fabricated
consent. Migration to ACP closes the protocol-level door — the
AskQuestiontool doesn't exist on the ACP side, so there's nothing tofabricate.
#799deliberately left existing sessions on legacy "because they keepworking." That tradeoff was reasonable at the time. The accumulated
#784evidence makes legacy sessions actively unsafe; this PR makes the upgrade
path invisible enough that operators stop avoiding it.
Coordination with upstream ACP mop-up
This PR stacks on top of swear01's three open ACP mop-up PRs:
#835fix(cursor): merge SKU catalog under ACP lock and refcount agent guard(fixes fix(cursor): incomplete cliModelSkus during ACP lock — variant picker shows raw wire params tiann/hapi#831 + fix(cursor): agent-acp-active lock clears while another ACP session still runs tiann/hapi#832) — cross-process refcount + live-pid reconciliation for
the
agent-acp-activelock. The migrator's post-flip ACP launcher claimsthis lock through the standard path; this PR does not modify
cli/src/agent/backends/acp/agentCliGuard.ts.#836fix(web): hide sidebar fake sessions for Cursor resume/archive(fixes
#833) — sidebar dedup. Post-migration the same session row getsa new
cursorSessionProtocolvalue; fix(web): hide sidebar fake sessions for Cursor resume/archive tiann/hapi#836'sdeduplicateSessionsByAgentIdensures the upgraded session doesn't show as two rows during the
transition.
#837fix(cursor): register cursorSessionId before ACP session/load(fixes
#834) — pre-writescursorSessionId+cursorSessionProtocolbefore awaiting
session/load, the same flip-before-resume orderingthis PR's migration helper observes.
Local soup is already running this PR's branch stacked on top of tiann#835/tiann#836/tiann#837
so the migrator is exercised against the eventual upstream shape.
Live dogfood on the soup (2026-06-07)
After the original PR draft landed I dogfooded against the live hub
(
hapi-hub.serviceon the operator's box) instead of trusting the unitsuite alone. Four migrations on real sessions:
5a2fba34-...(jellybot-dev, active operator session)jellybot-dev6c63c1f4-...androidsideloader3dd30dc4-...sparlingAll four triggered via the existing Reopen path in the Web UI (operator
clicks; hub auto-fires the helper inside
resumeSession). All four:cursorSessionProtocol=acpinhapi.db~/.cursor/acp-sessions/<curId>/{store.db,meta.json,WAL}~/.cursor/chats/<wsh>/<curId>/store.db(except wherean active runner held an open fd — see "Known wart" below)
The active-session migration (jellybot-dev: operator's own working
session, 374MB store.db, 8650 messages of history) succeeded mid-flight
— the conversation continued in the upgraded ACP session without the
operator having to do anything beyond clicking Reopen.
Post-mortem: three follow-up commits after the original draft
The first live dogfood surfaced an environmental gap the unit suite did
not catch. Three follow-up commits:
3cc4c037— observabilityThe helper's success path and all four early-return guards were silent.
On the first dogfood run, zero log lines meant I could not distinguish
"helper bailed at guard X" from "helper succeeded silently" from "helper
was never called." Added five log points:
[auto-migrate] considering(unconditional entry)[auto-migrate] skipped: <reason>per guard[auto-migrate] starting transplant[auto-migrate] success(structured: durationMs, sourceRemoved,replayNotifications, lastUsedModelPreserved)
[auto-migrate] legacy ... left as stream-json(soft fail; existed)This is now the source of the receipts in the table above.
5d61dcf9(attempt 1) →6c682fac(fix): PATH augmentation for verify-probeSecond dogfood run produced:
Root cause:
hapi-hub.serviceships systemd's minimal PATH(
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin) andnever sees Cursor's standard install location at
$HOME/.local/bin/agent.hapi-runner.servicehand-fixes this in its drop-in viaEnvironment=PATH=$HOME/.local/bin:.... The hub had no equivalent,so every
spawn('agent', ...)inside the verify probe returned ENOENT.5d61dcf9prepended$HOME/.local/binand$HOME/.npm-global/bintobaseEnv.PATHbefore spawning. Wrong fix: the migrator passesHOME=<tmpdir>to the probe for HAPI_HOME isolation, sobaseEnv.HOMEpointed at a tmpdir that doesn't contain.local/bin.6c682facswitched the lookup toprocess.env.HOME(the hub process'sown home, set by systemd
User=) and added a regression test thatexplicitly distinguishes
process.env.HOMEfrom a per-spawnHOMEoverride. Right fix: the agent binary lives at the operator's real
home regardless of any per-verify
HOMEoverride (Cursor installs atthe user's
$HOME/.local/binat install time, not at spawn time).The safety contract held across all of this: every failed verify left
the legacy session exactly as it was; no
rmfired without a verifysuccess.
In-flight upgrade banner (UX A++, ships in this PR)
The migration takes 15-20 seconds on long sessions (copy a multi-hundred-MB
store, spawn
agent acp, replay thousands of notifications, tear down theprobe). The first dogfood pass shipped this window with no progress
indicator — and the operator's lived feedback made clear that was a
non-starter:
Shipping the migrator without the banner means every first-time reviewer
forms a sticky bad first impression — "looks broken." The polish IS the
feature surface here, not an optional follow-up.
Design (intentionally minimal, no SSE event-type expansion):
Hub sets
metadata.cursorMigrationState='in_progress'BEFORE thelong-running transplant call. The session-cache refresh emits the
existing
session-updatedSSE event (no new event type), so the webclient picks it up in milliseconds. No client-side polling change
needed; the SSE channel is already real-time.
Hub clears the flag in the SAME metadata write that flips
cursorSessionProtocolto'acp'on success. A single SSE eventcarries both transitions, so the banner disappears in the same render
tick the chat re-renders as ACP — no flicker window between
"banner gone" and "ACP content rendered".
Hub clears the flag explicitly in the auto-migrate helper's
finallyon failure / exception, so the banner never gets stuck if migration
soft-fails and the legacy launcher takes over.
Web renders an accessible (
role=status,aria-live=polite) bannerwith an indeterminate spinner and short explanation when the flag is
set. Deliberately no fake percentage — we don't have phase data and a
fake progress bar would lie. The minimal-information design beats
engineering-brain over-design (per-phase SSE events, progress
percentages) which doesn't actually solve the lived rendering-timing
pain.
Files added (UX A++ slice on top of the migrator core):
shared/src/schemas.ts: addcursorMigrationState: z.enum(['in_progress'])toMetadataSchemahub/src/sync/syncEngine.ts:flipCursorSessionProtocolToAcpdrops the flag in the atomic write;setCursorMigrationStateInProgress/clearCursorMigrationStatehelpers with version-mismatch retry;maybeAutoMigrateLegacyCursorSessionsets the flag before transplant and finally-clears on failure/exception onlyhub/src/sync/syncEngineAutoMigrate.test.ts: 4 new tests pin the flag transitions (failure / exception / success-no-clear / flip-atomic-clears)web/src/components/CursorMigrationBanner.tsx: thin banner component (50 lines)web/src/components/CursorMigrationBanner.test.tsx: 10 tests covering all visibility paths + a11yweb/src/components/SessionChat.tsx: render banner betweenSessionHeaderandTeamPanelweb/src/lib/locales/{en,zh-CN}.ts: copy strings (English + Simplified Chinese)web/src/types/api.ts: re-exportMetadatafor component typingSurfaces (drastically reduced from earlier draft)
Per the cost critique on
#824, the bulk-migration surface area isstripped. No CLI subcommand, no web button, no REST bulk endpoint, no
Store.backupTo, nodryRun, no candidate-listing API.SyncEngine.resumeSessioncallsmaybeAutoMigrateLegacyCursorSessionbefore resolving the target. Migration is transparent.
HAPI_CURSOR_LEGACY_AUTO_MIGRATE=0|false|no|offdisables auto-migration (default = ON).
POST /api/sessions/:id/migrate-to-acpformanual one-off triggers (used by tests + operator scripts).
Verify-probe isolation
The probe spawn uses an isolated
HOMEandHAPI_HOME(both pointed ata per-probe
mkdtempSyncdirectory). This means:agent acpcannot see or mutate the user's real~/.cursor/acp-sessions/agent-acp-activelockThe real
cpof the legacy store into~/.cursor/acp-sessions/<uuid>/happens after the probe succeeds in the temp
HOME. The legacy sourceis
rmd only after thehapi.dbprotocol flip lands cleanly.Refusal contract (silent — caller falls back to legacy launcher)
cursorSessionIdThe session is never left in a half-migrated state. Verify failure rolls
back the placement before the protocol flip; WAL checkpoint + size
checks ensure store.db consistency across the copy.
Verification
cursorLegacyMigrator.test.ts(45 tests coveringrefusals, rollback, telemetry, force-archive, ACP-lock dependency
injection, fingerprint protection) +
syncEngineAutoMigrate.test.ts(8 tests covering the guard matrix + soft-fail behaviour).
agent acp(opt-in viaCURSOR_AGENT_INTEGRATION=1):buildSyntheticLegacyStoreLEGACY_FODDER_WSH+LEGACY_FODDER_UUIDbun run typecheckclean.Out of scope (mentioned in tiann#824 for future work)
hapi cursor import --from-cursor-diskfor the ~329 chats in~/.cursor/chats/that HAPI has no metadata row for. Separate discoverycleanly when
active=true; the next resume on an inactive row triggersthe migration invisibly.
Test plan (reviewer)
maybeAutoMigrateLegacyCursorSessionruns before thelegacy launcher resolves its target (otherwise the user gets a stale
legacy spawn).
cursorLegacyMigrator.test.tsmatch thecontract in
apiTypes.ts(CursorMigrateRefusalReason).HOME+HAPI_HOMEisolation preventspollution of the user's real
~/.cursor/acp-sessions/.HAPI_CURSOR_LEGACY_AUTO_MIGRATE=0cleanly disables auto-migration.