feat(cloud-agent): desktop vault bridge for Curated Thoughts#542
Merged
Conversation
Implements /agent/desktop persistent WebSocket, pairing routes, five vault_* ADK tools, and Firestore-routed task queue per the approved spec. Wire protocol matches CT v1.9.0; includes generation-guarded desktopBridge, device-doc revoke listener, and cap decay on timeout. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
cloud-agent/src/services/desktopPairing.test.ts (1)
72-78: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd UID-scoped revoke coverage.
This test passes even if revoke deletes another user’s pairing doc by
deviceId. Add a second pairing mapping with a differentuidand the samedeviceId, then assert it remains after revokingu1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-agent/src/services/desktopPairing.test.ts` around lines 72 - 78, The current revokeDesktopDevice coverage only checks that u1’s device doc is removed and the token fails closed, but it does not verify UID scoping when another user has the same deviceId. Update the revokeDesktopDevice test in desktopPairing.test.ts to create a second pairing mapping for a different uid with the same deviceId, then revoke only u1 and assert the other user’s pairing doc still exists. Use pairDesktopDevice, revokeDesktopDevice, and resolvePairingToken to confirm revocation is limited to the intended uid.cloud-agent/src/handlers/wsDesktopAgentHandler.test.ts (1)
63-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding regression tests for the auth-race scenarios.
Once the concurrent-auth-frame and disconnect-during-auth issues flagged in
wsDesktopAgentHandler.tsare fixed, add tests that: (1) emit twoauthmessages back-to-back before the firstresolvePairingToken/getDesktopDeviceDocpromise resolves, and (2) emitcloseon the socket between those awaits — asserting no self-close and no stalemarkDesktopDeviceOnlinecall without a corresponding cleanup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-agent/src/handlers/wsDesktopAgentHandler.test.ts` around lines 63 - 173, Add regression coverage in wsDesktopAgentHandler.test.ts for the auth race paths around handleDesktopWsUpgrade/auth handling: one test should send two auth frames back-to-back before resolvePairingToken/getDesktopDeviceDoc resolves, and another should emit close while auth is still pending. Assert the second auth or stale close does not trigger a self-close, and that markDesktopDeviceOnline is not called without the matching cleanup/fail path on the desktop bridge/session helpers.cloud-agent/src/tools/vaultTools.ts (1)
119-126: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
DESKTOP_TIMEOUTreaching this branch skips cap decay and the apologetic message.Only
DESKTOP_DISCONNECTEDtriggerstriggerCapDecay/TIMEOUT_MSGhere; afaileddoc witherror.code === 'DESKTOP_TIMEOUT'(e.g. written by a future backstop/reconciliation path) falls through to the genericVault query failed: ...branch without decaying the cap, diverging from the documented contract ("after the firstDESKTOP_TIMEOUTorDESKTOP_DISCONNECTED... budget drops to 1"). Currently unreachable in the normal flow (the caller's own local timeout resolves via the!taskbranch first), but worth closing for symmetry/future-proofing.♻️ Proposed fix
if (task.status === 'failed') { const code = task.error?.code - if (code === 'DESKTOP_DISCONNECTED') { + if (code === 'DESKTOP_DISCONNECTED' || code === 'DESKTOP_TIMEOUT') { triggerCapDecay(deps) return TIMEOUT_MSG } return `Vault query failed: ${task.error?.message ?? 'unknown error'}` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloud-agent/src/tools/vaultTools.ts` around lines 119 - 126, Update the failed-task handling in vaultTools’ task status branch so that `DESKTOP_TIMEOUT` is treated the same as `DESKTOP_DISCONNECTED`: call `triggerCapDecay(deps)` and return `TIMEOUT_MSG` instead of falling through to the generic failure message. Keep the existing `task.status === 'failed'` logic in `vaultTools.ts` aligned with the documented contract by checking `task.error?.code` for both codes before returning the fallback `Vault query failed...` message.docs/desktop-vault-bridge.md (1)
80-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DESKTOP_OFFLINEis listed alongside realDesktopTaskErrorcodes but is never persisted to Firestore.
vaultTools.tsreturnsNO_DEVICE_MSGdirectly (no task doc, no error code) when there's no device; the actualDesktopTaskError.codeunion (firestoreSession.ts) only contains'DESKTOP_TIMEOUT' | 'DESKTOP_DISCONNECTED' | 'TOOL_ERROR'. PresentingDESKTOP_OFFLINEin the same table could mislead anyone building Firestore-based monitoring/alerts around this table (e.g. expecting to filter task docs byerror.code == 'DESKTOP_OFFLINE').📝 Proposed clarification
| Code | Surfaced to model | |---|---| -| `DESKTOP_OFFLINE` | No home computer is connected | +| `DESKTOP_OFFLINE`* | No home computer is connected | | `DESKTOP_TIMEOUT` | Didn't respond in time | | `DESKTOP_DISCONNECTED` | Same as timeout | | `TOOL_ERROR` | CT error message, prefixed | *`DESKTOP_OFFLINE` is a synthesized message returned before any task doc is written; it is not a `DesktopTaskError.code` value stored in Firestore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop-vault-bridge.md` around lines 80 - 88, Clarify the Error Codes table so it does not imply that DESKTOP_OFFLINE is a persisted DesktopTaskError code; in docs/desktop-vault-bridge.md, update the table and surrounding text to distinguish the direct no-device response from the Firestore-backed DesktopTaskError union used by firestoreSession.ts. Refer to vaultTools.ts and DesktopTaskError so readers understand that NO_DEVICE_MSG is returned without a task doc, while only DESKTOP_TIMEOUT, DESKTOP_DISCONNECTED, and TOOL_ERROR are stored in Firestore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloud-agent/src/handlers/wsDesktopAgentHandler.ts`:
- Around line 119-137: Results from the desktop socket are accepted without
verifying the task was actually dispatched by this connection, so a client can
overwrite unrelated task state. In onResult within wsDesktopAgentHandler, add a
dispatched.has(taskId) check before any writeDesktopTaskResult call for both
taskResultSchema and taskErrorSchema cases, and ignore/log malformed or
unexpected frames when the taskId was never recorded in dispatched.
- Around line 146-156: The ping handling in wsDesktopAgentHandler should not
treat every touchDesktopDeviceLastSeen rejection as device revocation. Update
the ping branch in the websocket handler to distinguish not-found/revoked errors
from transient Firestore failures: only set deviceDocGone and close the socket
for revocation/unavailable cases, while logging or ignoring retryable errors so
the later offline write can still run. Use the existing wsDesktopAgentHandler
flow, the pingSchema check, and the fs.touchDesktopDeviceLastSeen catch block to
localize the fix.
- Around line 46-54: The auth flow in wsDesktopAgentHandler can be re-entered by
duplicate auth frames because authed is only set after onAuth completes, which
can cause bridge.register() to treat the same socket as a replacement and close
it. Add an in-flight/pending auth guard in the auth handling path, or set a
separate flag before any await inside the auth branch, so subsequent auth frames
are ignored until the first one finishes. Use the existing authed state and the
onAuth/bridge.register logic to locate the fix.
- Around line 78-117: The async auth flow in onAuth can continue after the
WebSocket has already closed, so add ws.readyState checks immediately after each
await (resolvePairingToken and getDesktopDeviceDoc) before setting authed,
registering with bridge.register, marking the device online, or installing
fs.watchPendingDesktopTasks/fs.watchDesktopDeviceDoc listeners. If the socket is
no longer OPEN, return early so the dead connection is never treated as
authenticated.
In `@cloud-agent/src/services/desktopPairing.ts`:
- Around line 64-69: The pairing cleanup in revokeDesktopDevice is deleting
desktopPairings documents using only deviceId, which can affect records outside
the authenticated user. Update the delete flow to scope the query by both uid
and deviceId, or inspect each returned document’s uid before calling
doc.ref.delete(), so only the pairing entries owned by the provided uid are
removed.
In `@cloud-agent/src/services/firestoreSession.test.ts`:
- Around line 298-301: The fake Firestore collection listener in onSnapshot
currently only registers future writes and never emits the current matching rows
on subscribe, so watchPendingDesktopTasks() can miss pre-existing desktop tasks.
Update the onSnapshot implementation in firestoreSession.test.ts to immediately
invoke the callback with the current filtered collection contents for the
watched colPath before returning the unsubscribe function, keeping the existing
watcher registration behavior intact.
In `@cloud-agent/src/services/firestoreSession.ts`:
- Around line 205-206: The offline update in markDesktopDeviceOffline is
unconditional, which can clear a newer live connection after a stale disconnect.
Update the FirestoreSessionService flow so markDesktopDeviceOffline accepts the
expected connectedInstanceId (or generation) and only performs the update when
the stored connectedInstanceId still matches; keep markDesktopDeviceOnline as
the source of the active instance identity and use that same value at the call
site to guard the write.
- Around line 156-165: Make the terminal desktop-task transitions atomic and
conditional in writeDesktopTaskResult and failDesktopTaskIfUnresolved, since
they can race and let a late write overwrite an already-final complete or failed
state. Update the Firestore write path to use a transaction or compare-and-set
guard that first checks the current status, only applies the terminal update
when the task is still unresolved, and returns whether the transition succeeded.
Use the existing desktopTaskPath, writeDesktopTaskResult, and
failDesktopTaskIfUnresolved entry points to keep the logic centralized.
In `@cloud-agent/src/tools/vaultTools.test.ts`:
- Around line 167-176: The `no spendCredit on vault path` test currently has no
assertions, so it cannot verify the intended behavior. Update the
`fakeSession`/`deps` setup in `vaultTools.test.ts` so the test can observe
whether `spendCredit` is invoked, then add an explicit assertion after
`execTool(search, { query: 'x' })` and `fs.resolveTask(...)` to confirm the
vault path does not spend credit. Use the existing `buildVaultTools` and
`execTool` flow, but wire in a spy or mock on the credit-spend path so this test
fails if the behavior regresses.
- Around line 111-124: The test logic in vaultTools.test.ts is misleading
because fs.resolveTask('complete', ...) does not resolve the already-timed-out
search call and instead leaves the following ontology invocation to time out as
well. Update the flow around execTool(search), execTool(ontology), and
fs.resolveTask so the completion is applied to the active pending task for the
ontology call, or remove the unused resolve step if the test is only meant to
exercise decay. Keep the assertions focused on the intended successful
post-timeout behavior in buildVaultTools.
In `@cloud-agent/src/tools/vaultTools.ts`:
- Around line 93-131: Move the billing pause into the same try/finally scope in
vaultTools’s task flow so any exception before the async work still resumes
billing. In the function that uses deps.pauseBilling, crypto.randomUUID, and
deps.resumeBilling, call pauseBilling after entering the try block (or otherwise
ensure it is always paired with the finally), so failures during taskId setup or
other early steps cannot leave billing paused indefinitely.
---
Nitpick comments:
In `@cloud-agent/src/handlers/wsDesktopAgentHandler.test.ts`:
- Around line 63-173: Add regression coverage in wsDesktopAgentHandler.test.ts
for the auth race paths around handleDesktopWsUpgrade/auth handling: one test
should send two auth frames back-to-back before
resolvePairingToken/getDesktopDeviceDoc resolves, and another should emit close
while auth is still pending. Assert the second auth or stale close does not
trigger a self-close, and that markDesktopDeviceOnline is not called without the
matching cleanup/fail path on the desktop bridge/session helpers.
In `@cloud-agent/src/services/desktopPairing.test.ts`:
- Around line 72-78: The current revokeDesktopDevice coverage only checks that
u1’s device doc is removed and the token fails closed, but it does not verify
UID scoping when another user has the same deviceId. Update the
revokeDesktopDevice test in desktopPairing.test.ts to create a second pairing
mapping for a different uid with the same deviceId, then revoke only u1 and
assert the other user’s pairing doc still exists. Use pairDesktopDevice,
revokeDesktopDevice, and resolvePairingToken to confirm revocation is limited to
the intended uid.
In `@cloud-agent/src/tools/vaultTools.ts`:
- Around line 119-126: Update the failed-task handling in vaultTools’ task
status branch so that `DESKTOP_TIMEOUT` is treated the same as
`DESKTOP_DISCONNECTED`: call `triggerCapDecay(deps)` and return `TIMEOUT_MSG`
instead of falling through to the generic failure message. Keep the existing
`task.status === 'failed'` logic in `vaultTools.ts` aligned with the documented
contract by checking `task.error?.code` for both codes before returning the
fallback `Vault query failed...` message.
In `@docs/desktop-vault-bridge.md`:
- Around line 80-88: Clarify the Error Codes table so it does not imply that
DESKTOP_OFFLINE is a persisted DesktopTaskError code; in
docs/desktop-vault-bridge.md, update the table and surrounding text to
distinguish the direct no-device response from the Firestore-backed
DesktopTaskError union used by firestoreSession.ts. Refer to vaultTools.ts and
DesktopTaskError so readers understand that NO_DEVICE_MSG is returned without a
task doc, while only DESKTOP_TIMEOUT, DESKTOP_DISCONNECTED, and TOOL_ERROR are
stored in Firestore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba092a11-ce34-490f-8acf-a3c61bb2fd8f
📒 Files selected for processing (21)
README.mdcloud-agent/scripts/desktopBridgeSmoke.tscloud-agent/src/handlers/wsDesktopAgentHandler.test.tscloud-agent/src/handlers/wsDesktopAgentHandler.tscloud-agent/src/handlers/wsLiveAgentHandler.tscloud-agent/src/index.tscloud-agent/src/services/agentCore.tscloud-agent/src/services/desktopBridge.test.tscloud-agent/src/services/desktopBridge.tscloud-agent/src/services/desktopPairing.test.tscloud-agent/src/services/desktopPairing.tscloud-agent/src/services/firestoreSession.test.tscloud-agent/src/services/firestoreSession.tscloud-agent/src/services/liveToolAdapter.tscloud-agent/src/tools/vaultTools.test.tscloud-agent/src/tools/vaultTools.tscloud-agent/src/tools/vaultToolsWiring.test.tsdocs/desktop-vault-bridge.mddocs/superpowers/plans/2026-07-05-desktop-vault-bridge.mddocs/superpowers/specs/2026-07-05-desktop-vault-bridge-design.mdfirestore.rules
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Adds the Clanker side of the desktop vault bridge: persistent
/agent/desktopWebSocket with pairing-token auth,
desktopPairings+desktopTasksFirestorerouting, and five
vault_*ADK tools (vault_wiki_search,vault_get_ontology,vault_traverse_graph,vault_semantic_search,vault_related_chunks) for/agent/runand/agent/live. Wire protocol matches Curated Thoughts v1.9.0.Specs:
Type of Change
Related Issue
Fixes #
Changes Made
POST /agent/desktop/pairand/agent/desktop/revoke(hash-only token storage)wsDesktopAgentHandler: auth, pending-task listener, device-doc revoke/pauselistener, 40s
lastSeenAtrefresh, generation-guardeddesktopBridgefirestoreSession:getActiveDesktopDevice, desktop task queue, browsergetActiveDeviceexcludestype: 'desktop'vaultTools.ts: five ADK tools, shared dispatcher, 5-call cap withtimeout/disconnect decay, same-instance shortcut, no credit spend
buildAgent(text) andbuildLiveTools(voice)firestore.rules: deny client access tousers/{uid}/desktopTaskscloud-agent/scripts/desktopBridgeSmoke.tsTesting
Platforms Tested
Test Steps
cd cloud-agent && npm run typechecknpm run build && NODE_ENV=test node --test --test-reporter spec dist/cloud-agent/src/handlers/wsDesktopAgentHandler.test.js dist/cloud-agent/src/tools/vaultTools.test.js dist/cloud-agent/src/services/firestoreSession.test.js dist/cloud-agent/src/services/desktopBridge.test.js dist/cloud-agent/src/services/desktopPairing.test.js dist/cloud-agent/src/tools/vaultToolsWiring.test.js— 53/53 passdocker compose -f docker-compose.local.yml up -d, pair via/agent/desktop/pair, runPAIRING_TOKEN=… npx tsx cloud-agent/scripts/desktopBridgeSmoke.ts, then/agent/runwith a vault search promptvault_wiki_searchend to endScreenshots
Before
N/A — new subsystem.
After
N/A — server-side only; mobile Settings UI for pair/revoke is a follow-up.
Checklist
Additional Notes
gcloud firestore fields ttls update expiresAt --collection-group=desktopTasks --enable-ttlCloudBridgeClientis already shipped (v1.9.0); this PR is the matching Clanker contract.Made with Cursor