Repository navigation
fix(app): separate location identity from sync failures - #46695
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
New client tests construct session.moved events without the required projectID, which is inconsistent with the generated types/schema and can break typechecking or misrepresent real event payloads.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refines location/session recovery so transient connection or workspace-sync failures don’t trigger “missing directory” recovery flows, while still following real location moves and only offering directory recovery after a confirmed LocationNotFoundError (with a placement recheck).
Changes:
- Introduces
LocationNotFoundErrorin the protocol/OpenAPI and updates the generated client to surface the 404 response forGET /api/location. - Updates the server
location.gethandler to confirm missing/non-directory local paths before booting location services (and to avoid classifying non-missing filesystem/config/IO failures as “missing”). - Updates client session/location data resolution to preserve live
session.movedupdates across concurrent reads and to refresh session placement on reconnect, with added unit and E2E regression coverage.
File summaries
| File | Description |
|---|---|
| packages/server/test/location.test.ts | Adds server-level tests covering local path confirmation, non-missing failure handling, and workspace placement behavior. |
| packages/server/src/routes.ts | Registers FSUtil node so the location handler can stat local paths. |
| packages/server/src/handlers/location.ts | Adds local filesystem confirmation + LocationNotFoundError handling for location.get, scoped by workspace placement. |
| packages/protocol/test/location.test.ts | Verifies LocationNotFoundError encoding and OpenAPI declaration scoping to location.get. |
| packages/protocol/src/groups/location.ts | Declares LocationNotFoundError as an endpoint error for location.get. |
| packages/protocol/src/errors.ts | Introduces LocationNotFoundError with optional workspaceID and HTTP 404 status. |
| packages/protocol/src/api.ts | Adjusts API composition so LocationGroup is added without location middleware. |
| packages/protocol/openapi.json | Updates OpenAPI to include the new 404 response and schema. |
| packages/client/test/solid-data.test.ts | Adds tests for preserving session.moved effects across concurrent reads (contains contract issues noted in comments). |
| packages/client/src/solid/data.ts | Tracks in-flight session reads to preserve move events until the next authoritative read. |
| packages/client/src/promise/generated/types.ts | Adds generated LocationNotFoundError type guard/type. |
| packages/client/src/promise/generated/client.ts | Adds 404 to declared statuses for location.get. |
| packages/app/test-browser/session-resolution.test.ts | Adds browser test ensuring reconnect refresh keeps cached content visible. |
| packages/app/src/workspaces/location.tsx | Adds retry-in-place behavior and placement refresh before offering directory recovery UI. |
| packages/app/src/session/session-resolution.ts | Gates resolution work on connection state to preserve cached content across disconnect/reconnect. |
| packages/app/src/session/route.tsx | Wires connected + workspace/session info into LocationProvider for recovery decisions. |
| packages/app/src/runtime/server/errors.ts | Adds isLocationNotFoundError classifier for precise missing-location handling. |
| packages/app/src/runtime/server/errors.test.ts | Adds unit coverage for isLocationNotFoundError classification boundaries. |
| packages/app/e2e/regression/session-location-sync.spec.ts | Adds E2E regressions for sync failures and moved-session refresh flows. |
| packages/app/e2e/regression/session-location-missing.spec.ts | Updates E2E missing-location coverage to use the new 404 error contract and refresh behavior. |
Review details
Suppressed comments (1)
packages/client/test/solid-data.test.ts:268
session.movedevents requireprojectIDon every move, but this fixture only includes it forseq === 1. That makes the test data inconsistent with the event schema/types and can hide regressions (or fail typechecking if tests are included in typecheck).
data: {
sessionID: info.id,
location: { directory: `/moved/${info.id}/${seq}` },
...(seq === 1 ? { projectID: `project_${info.id}`, subpath: "intermediate" } : {}),
},
- Files reviewed: 18/20 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| created: 1, | ||
| type: "session.moved", | ||
| durable: { aggregateID: stale.id, seq: 1, version: 1 }, | ||
| data: { sessionID: stale.id, location: { directory: "/moved" } }, |
data.location.syncInfo()before opening the session, matching the TUI fix in fix(tui): seed moved worktree location #44143. Manual workspace movement remains available.App-only: no server, protocol, shared-client, CLI, or TUI changes.
Valid worktree with a failing agent-list request:
Cold session entry from Home, three serial production-build samples per revision:
v2)Small local sample; not a performance guarantee.