Feat/realtime broadcast presence - #615
Open
edgarsskore wants to merge 13 commits into
Open
Conversation
Join the private user:{id} channel instead of the shared postgres_changes
queue: presence track = live "online" signal, new_call doorbell receive (with
row-fetch retry), result doorbell send after the row write. Conditional claim
(UPDATE ... eq(status,'pending').select('id')) makes execution exactly-once
under dual delivery during the transition.
- 30-min bookkeeping last_seen write (presence carries liveness), re-asserts
status:'online' only when the channel is joined (no masking a deaf device);
clean-shutdown last_seen stamp.
- Jittered reconnect backoff + self-heal-during-backoff bail-out; realtime
JWT re-auth on token refresh.
- capabilities: { transport_broadcast_v1, app_version } for adoption tracking.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A tool result containing a NUL byte (binary file reads, process output) made the jsonb `result` write fail with Postgres 22P05 "unsupported Unicode escape sequence" — leaving the call stuck 'executing' so the user waited out a 5-min timeout for a tool that actually ran (~300-350/day in prod). The device is the only writer of the result column, so the fix lives here. - stripNullBytes(): recursively strips NUL from the result before the write, with a fast path that only reparses when a NUL is actually present. - Fail-fast fallback: if a result write still fails for any reason, record a terminal 'failed' with a text-only error_message so the user gets an immediate honest error instead of a phantom timeout. Reproduced end-to-end (null-byte tool output -> stuck row / 22P05) and verified fixed (result stored NUL-free, call completes in ~1s) on staging. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
P1 — fail-open DB claim could execute a tool twice. markCallExecuting returns true on a transient write error, and during the transition BOTH transports deliver every call, so a correlated REST blip let both deliveries proceed (legacy claims-on-error and runs; broadcast then finds the row still pending, claims cleanly, and runs the same tool again). Side-effecting commands could run twice. Added an in-memory seen-call-id guard in handleNewToolCall: dual delivery is always intra-process, so the local check is authoritative and cannot fail open. The DB claim stays for row-state/cross-restart/observability. P2 — stripNullBytes corrupted valid content. It serialized to JSON and regexed the ESCAPE TEXT, so content containing the six literal chars backslash-u-0-0-0-0 (e.g. reading a source file with that escape) was silently altered, and a doubled-backslash form threw SyntaxError -> the call was reported failed though the tool succeeded. Rewritten as a recursive walk over strings/keys that strips real NUL characters only. New test covers both regressions plus the original Postgres 22P05 case. P2 — test-remote-channel-reconnect.js was broken by the new channel contract (FakeChannel lacked track/untrack) and is auto-discovered by npm test, so the whole DCMCP suite failed on this branch. Added the presence fakes (resolving with a status string, as realtime-js does) and stubbed the new jittered reconnect sleep so the suite stays sub-second. npm test: 46/46 green. Also: strip NUL from error_message (text rejects it too, and result === null means the fail-fast fallback would not fire); check track()'s resolved status instead of logging every resolution as success; skip untrack() on a non-joined channel so a dead socket can't stall past the 5s shutdown deadline; include call_id in the doorbell-row-missing event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop remote_channel_doorbell_received: it fired on EVERY remote tool call (~126k/day in prod) for permanent per-call telemetry volume, and it is redundant — dispatch stamps metadata.transport, which rides the existing mcp_command_executed event, so transport usage is already segmentable server-side. The doorbell FAILURE paths are still captured. - Seen-call-id cap 500 -> 100 (~10 KB). Dual delivery arrives within milliseconds, so the window only needs to outlive that; 100 is still several minutes of heavy agent traffic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tbeat
- setSession pushed the token it was HANDED to realtime, but auth.setSession()
refreshes an expired one internally first (device asleep >1h with
--persist-session). Pushing the stale parameter overwrote the fresh token
realtime already had, so every private-channel join failed until the next
refresh — ~50 min deaf. Push the CURRENT session token instead.
- untrack() on shutdown: a half-open socket still reports state 'joined', so the
state guard didn't help — the presence push buffers and settles via
realtime-js's 10s timeout, past device.ts's 5s force-exit, skipping
unsubscribe() and the durable offline write. Bounded with a 1s race.
- broadcast { ack: true } + presence { enabled: true } on the device channel.
Without ack, send() resolves 'ok' at socket-write, so notifyResult's status
check and its failure telemetry could never fire.
- updateHeartbeat: skip the write entirely when not joined. Bumping last_seen on
a deaf device kept its row perpetually young, so the staleness sweep could
never age out a stale 'online' — exactly the row dispatch falls back to when
presence is unavailable.
- Dropped remote_channel_private_join_failed: it fired on every CHANNEL_ERROR,
a 1:1 duplicate of remote_channel_subscription_error with no added
specificity. Filter the surviving event on the error text instead.
- RECREATE_TIMEOUT_MS comment corrected: the guard is held for backoff (<=45s)
PLUS the 30s cap, so the watchdog blind spot is ~75s, not 30s.
- Test: replaced the comment claiming a backoff assertion with an actual one
(grows with consecutive attempts, always positive, capped at 30s).
npm test: 46/46.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…etry
- LEGACY FALLBACK IS INDEPENDENT AGAIN. The postgres_changes listener had been
moved onto the private user:{id} channel, so the transition's safety net
shared a single point of failure with the thing it backs up: an 008 policy
problem (or any private-channel auth failure) took out BOTH transports and the
device went completely dark instead of degrading to the old path. It now lives
on its own public channel with its own lifecycle, rebuilt alongside the
private one on recreate and torn down on unsubscribe. Costs one extra channel
per device (its own connection carries 2, far under the 100 quota). Removed
entirely at the flip.
- track() failures are retried (3 attempts) and createChannel no longer resolves
before presence lands. Previously a single non-'ok' track left a fully working
device invisible — the server treats absent presence as authoritative offline,
so dispatch threw "No devices available" with nothing to re-track until the
channel bounced. checkConnectionHealth also re-tracks a joined channel whose
presence never published, so it self-heals.
- Backoff test was flaky (~10%, measured) AND vacuous: the fake heals on
disconnect, so every recreate succeeded, reconnectAttempt reset to 0, and all
samples came from the attempt-1 distribution — "grows" was a coin flip and the
cap was never exercised (and was asserted at 30s when the formula tops out at
45s). Now models a persistent outage so attempts actually climb; 12/12 clean.
- PUBLISH.md: release gate for this transport (008 applied + new server
deployed, in that order) — the migrations README lives in the other repo,
which is not where the person cutting an npm release is looking.
npm test: 46/46.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
P1 — the device advertised transport_broadcast_v1 during registerDevice(), before createChannel() was even attempted. The server treats that flag as binding: for a flagged device absent presence is authoritative offline, so dispatch throws "No devices available". If the private channel could not join (008 missing, authz, a Realtime incident) but the legacy postgres_changes channel was perfectly healthy, the server refused to dispatch at all — the device went dark, which is precisely what the independent legacy channel was added to prevent. The flag is now written only after SUBSCRIBED *and* a successful presence track, and withdrawn when presence definitively fails, so a device that cannot keep the promise reports itself legacy and stays reachable. This also makes the PUBLISH.md release gate far less load-bearing. P1 — recreateChannel() destroyed the legacy channel and only rebuilt it AFTER createChannel() resolved, so any failure (or timeout) left the safety net dead for the whole outage, with every health tick repeating the teardown. It is now rebuilt before createChannel. Related: trackPresenceWithRetry's worst case (~31.5s: three 10s realtime pushes plus backoff) exceeded RECREATE_TIMEOUT_MS (30s), so a recreate where presence never acked ALWAYS timed out — the two constants were in silent conflict. Raised to 45s. Also: bound removeLegacyChannel() in unsubscribe() (it was an unbounded await in front of the deliberately 1s-bounded untrack, defeating it and risking the 5s force-exit); include deviceId in createChannel's prerequisite guard (it is the presence KEY — a null key gets a random one and the device is invisible while every local signal says healthy); add an in-flight guard to the presence self-heal so 10s ticks can't stack pushes on a wedged socket; stop serializing the whole result object in the hot-path debug log (13 MB payloads cost more there than everything the sanitizer was benchmarked against). New test file covering the branch's headline mechanism, which had none: exactly-once under dual delivery (including the fail-open claim case), the device-id filter preceding dedupe, seen-set bounding, onDoorbell routing (other-device, already-claimed, missing row, fetch retry), and the updateCallResult-before-notifyResult ordering the server depends on. npm test: 47/47. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Three blackout bugs, all from the same invariant: the server tiers BOTH its offline sweep and its presence overlay on the device-written capabilities.transport_broadcast_v1 flag, never on the app version. 1. Heartbeat cadence followed the BUILD, not the tier. It was 30 min flat, while a device without the flag is judged by the server's 45s legacy threshold — and the flag is only written after presence is proven. So any device that could not prove the private channel (008 not applied, an authz hiccup, exhausted track() retries) was swept offline ~45s after registering and stayed undispatchable, though its independent legacy channel was joined and could run every call. Split into CAPABLE_HEARTBEAT_INTERVAL (5 min) and LEGACY_HEARTBEAT_INTERVAL (15s), chosen per-tick by heartbeatIntervalMs(); the fixed setInterval became a self-rescheduling scheduleHeartbeat() that re-arms on every tier change and writes immediately when dropping to the fast tier. 2. updateHeartbeat gated its write on the PRIVATE channel, so a device reachable only via the legacy channel never wrote last_seen at all — fixing the cadence alone would not have helped. Now gated on isReachable() (private OR legacy joined), which also keeps the deliberate silence for a genuinely deaf device so the sweep can still age its row out. 3. `status` is transport-agnostic — it is what the server's resolveTargetDevice filters on — but every degraded state of the private channel wrote 'offline'. Since realtime-js re-fires CHANNEL_ERROR on every failed rejoin, the row oscillated against the heartbeat and roughly half of all dispatches failed for healthy machines. Now driven by syncReachabilityStatus() off the same isReachable() predicate, with writes serialized through a single-slot chain so a teardown's write cannot overtake a join's. Also: the capability was only ever withdrawn from trackPresenceInner, which is unreachable unless the channel is already joined — so a device that could never join kept advertising itself and the server's presence overlay reported it authoritatively OFFLINE until the process restarted. It now withdraws after TRANSPORT_WITHDRAW_AFTER_ATTEMPTS (3) failed recreates, bounded by its own timeout because recreateChannel's catch is outside RECREATE_TIMEOUT_MS's reach. Not fewer than 3: ordinary half-open recovery legitimately costs two attempts. Shutdown path: a heartbeat or reachability write could land after setOffline()'s subprocess write and leave an exited process marked online for a full sweep tier. Gated both writers on shuttingDown. auth.getSession() was unbounded network I/O (it refreshes within ~90s of expiry, with its own ~30s retry budget) and could outlast device.ts's 5s force-exit, losing the durable offline write entirely — now bounded with a cached-token fallback. Bounded channel.unsubscribe() and tightened the leave bounds so the whole path fits. New test/test-remote-heartbeat-tier.js (17 cases) covers the tier pairing, the reachability gate, write ordering, the withdrawal (and that a single blip does NOT withdraw), the guard release on a hanging write, and the bounded session fetch. Each was verified to fail with its fix reverted. Pairs with the server-side change lowering DEVICE_OFFLINE_TIMEOUT_CAPABLE_MS to 15 min; the two cadences must stay in step and nothing enforces the copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR
The Broadcast/Presence release gate lives outside both repos, alongside the manual test plan, rather than in PUBLISH.md. Reverts PUBLISH.md to main's content so base..head no longer touches it. The gate content itself is unchanged and still required reading before any npm release carrying transport_broadcast_v1 — it is kept with the operator's other rollout material. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR adds local duplicate-call suppression, conditional database claiming, sanitized result persistence, presence-gated realtime transport, legacy-channel fallback, bounded reconnection, tiered heartbeats, shutdown timeouts, capped tool history, and expanded regression coverage. ChangesRemote device resilience
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Device
participant PrivateChannel
participant LegacyChannel
participant Supabase
Device->>Supabase: register device and capabilities
Device->>PrivateChannel: join and track presence
PrivateChannel-->>Device: acknowledge presence
Device->>PrivateChannel: enable broadcast capability
PrivateChannel-->>Device: deliver new_call
Device->>Supabase: verify pending call
LegacyChannel-->>Device: provide fallback delivery
Device->>Supabase: persist result and status
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/remote-device/remote-channel.ts (2)
802-810: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
sleep()was inserted underwithTimeout()'s JSDoc.The block comment describing "Run an async op but reject if it doesn't settle within
ms" now documentssleep. Movesleepabove the comment.🤖 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 `@src/remote-device/remote-channel.ts` around lines 802 - 810, Move the private sleep method above the JSDoc block describing withTimeout so that comment remains attached to the intended timeout helper; keep sleep’s implementation unchanged.
5-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: drop the constructed
RegExpto silence the SAST rule.
NUL_CHARis a module-level constant, so the ReDoS finding is a false positive, butreplaceAllremoves the pattern entirely and is equally fast.-const NUL_CHAR = String.fromCharCode(0); -const NUL_RE = new RegExp(NUL_CHAR, 'g'); +const NUL_CHAR = String.fromCharCode(0);…with
value.replace(NUL_RE, '')→value.replaceAll(NUL_CHAR, '')at Lines 22 and 33 (requires ES2021 lib; verifytsconfigtarget).🤖 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 `@src/remote-device/remote-channel.ts` around lines 5 - 6, Replace the NUL_RE-based replacements in the relevant methods with value.replaceAll(NUL_CHAR, '') and remove the now-unused NUL_RE constant. Verify the project’s TypeScript target/lib supports ES2021 replaceAll.Source: Linters/SAST tools
test/test-remote-dedupe-and-doorbell.js (1)
41-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
new MCPDevice()registers SIGINT/SIGTERM handlers per test.The constructor calls
setupShutdownHandlers(), so eachmakeDevice()adds two process listeners (7 devices here). It stays under Node's 10-listener warning threshold today, but any added test will start emittingMaxListenersExceededWarning. Considerprocess.removeAllListeners('SIGINT'/'SIGTERM')inmakeDeviceafter construction.🤖 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 `@test/test-remote-dedupe-and-doorbell.js` around lines 41 - 61, The makeDevice test helper accumulates MCPDevice shutdown signal listeners across instances. After constructing the MCPDevice in makeDevice, remove the SIGINT and SIGTERM listeners registered by the constructor before returning the device, without altering the device setup or execution behavior.
🤖 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 `@src/remote-device/device.ts`:
- Around line 357-361: Prevent unhandled rejections from async tool-call
handling: in src/remote-device/device.ts lines 357-361, wrap the
updateCallResult/notifyResult failure-reporting sequence in its own try/catch so
handleNewToolCall cannot reject from the catch path; in
src/remote-device/remote-channel.ts lines 654-703, attach .catch() handlers to
both the current and legacy onToolCall?.({ new: row }) invocations.
In `@src/remote-device/remote-channel.ts`:
- Around line 1330-1350: Correct the teardown budget in the shutdown flow around
LEAVE_BOUND_MS and its Promise.race calls so the worst-case duration stays
within device.ts’s 5-second force-exit; reduce one 500ms bound to 250ms, while
preserving the durable offline write path and the existing untrack protection.
- Around line 654-703: Update the onToolCall invocation in the doorbell row
handling flow to observe rejected promises from the async handler. Attach a
catch handler to this.onToolCall?.({ new: row }) that records the failure using
the existing error-reporting mechanism, preventing handleNewToolCall rejections
from becoming unhandled.
- Around line 984-989: Update the debug logging near the call-result update to
avoid JSON.stringify on updateData.result; derive resultBytes from an
inexpensive size estimate or existing metadata, while preserving the current
summary format and null handling. Ensure console.debug arguments do not trigger
full payload serialization on the hot path.
---
Nitpick comments:
In `@src/remote-device/remote-channel.ts`:
- Around line 802-810: Move the private sleep method above the JSDoc block
describing withTimeout so that comment remains attached to the intended timeout
helper; keep sleep’s implementation unchanged.
- Around line 5-6: Replace the NUL_RE-based replacements in the relevant methods
with value.replaceAll(NUL_CHAR, '') and remove the now-unused NUL_RE constant.
Verify the project’s TypeScript target/lib supports ES2021 replaceAll.
In `@test/test-remote-dedupe-and-doorbell.js`:
- Around line 41-61: The makeDevice test helper accumulates MCPDevice shutdown
signal listeners across instances. After constructing the MCPDevice in
makeDevice, remove the SIGINT and SIGTERM listeners registered by the
constructor before returning the device, without altering the device setup or
execution behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ad8b814-0798-4ab4-bab1-5601ed72077d
📒 Files selected for processing (8)
src/remote-device/device.tssrc/remote-device/remote-channel.tssrc/remote-device/scripts/blocking-offline-update.jstest/bench-strip-null-bytes.mjstest/test-remote-channel-reconnect.jstest/test-remote-dedupe-and-doorbell.jstest/test-remote-heartbeat-tier.jstest/test-strip-null-bytes.js
Tests: 5 artefacts for this feature down to 3, by subject.
- delete test/bench-strip-null-bytes.mjs — a micro-benchmark with no
assertions, so it could never fail while still costing a slot in npm test
- merge test-remote-dedupe-and-doorbell.js and test-remote-heartbeat-tier.js
into test-remote-transport.js (28 cases: exactly-once, doorbell routing,
result ordering, heartbeat tiers, reachability/status, capability
withdrawal, shutdown), which also removes a duplicated Supabase fake
- test-remote-channel-reconnect.js and test-strip-null-bytes.js keep their
own subjects
Comments in remote-channel.ts: 498 lines to 308 (36% -> 25% of the file), no
code change. Removed the "an earlier version did X, which broke Y" narratives —
that belongs in git history and is already in the round-6/7/8 commit message —
compressed the constants block, and dropped the degraded-path rationale that was
restated in several places. What stays is the non-obvious mechanics: why the
teardown calls are bounded, why the withdrawal needs 3 attempts and not 2, why
the two heartbeat cadences have to match the server's sweep tiers.
Still above the repo's 12-17% baseline because the new code carries invariants
that are genuinely easy to get wrong, and each surviving note exists because a
review caught it being wrong first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/test-remote-transport.js (5)
169-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBounded-set test passes for a "clear everything" eviction too.
Asserting only
size <= 100doesn't distinguish LRU trimming from a full wipe, and a wipe silently breaks dedupe right after the boundary. Add a check that the most recent id is still remembered.♻️ Suggested strengthening
for (let i = 0; i < 250; i++) await device.handleNewToolCall(payloadFor(`bulk-${i}`)); assert(device.seenCallIds.size <= 100, `set grew to ${device.seenCallIds.size}`); + assert(device.seenCallIds.has('bulk-249'), 'eviction must keep the most recent ids');🤖 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 `@test/test-remote-transport.js` around lines 169 - 173, Strengthen the “the seen-call-id set stays bounded” test by asserting that the newest bulk call ID, such as `bulk-249`, remains in `device.seenCallIds` after processing all 250 calls. Keep the existing size bound assertion to verify both bounded capacity and retention of the most recent ID.
369-414: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEncode the withdrawal threshold as a named constant.
for (let i = 0; i < 3; i++)at Line 382 and the single-attempt case at Line 412 both encode the implementation's failure threshold implicitly. A named constant (mirroring theSERVER_*constants at the top) makes the coupling explicit if the threshold changes.🤖 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 `@test/test-remote-transport.js` around lines 369 - 414, Define a named constant for the recreate-failure withdrawal threshold, following the existing SERVER_* constant conventions. Use that constant for the sustained-failure loop count and derive the single-failure test case from it so both tests remain coupled to the same threshold.
201-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for exhausted retries.
The happy retry path and the missing-row path are covered, but not "all 3 fetch attempts fail" — the branch that must stop at 3 attempts and not deliver.
♻️ Suggested extra case
+await test('doorbell gives up after exhausting row-fetch retries', async () => { + const { rc, client } = makeRemoteChannel({ row: { id: 'x', status: 'pending' }, failFetches: 3 }); + const delivered = []; + rc.onToolCall = (p) => delivered.push(p); + rc.sleep = () => Promise.resolve(); + await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); + assert(client.attempts() === 3, `must stop at 3 attempts, got ${client.attempts()}`); + assert(delivered.length === 0, 'must not deliver when every fetch failed'); +});🤖 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 `@test/test-remote-transport.js` around lines 201 - 217, Add a test alongside the existing doorbell retry tests that configures three consecutive row-fetch failures, invokes rc.onDoorbell, and verifies exactly three client attempts occur with no tool call delivered. Reuse the existing makeRemoteChannel, sleep stub, and delivered collection patterns.
94-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe fake swallows table and filter information, weakening every write assertion.
from()ignores the table name andupdate()records only the payload, so a regression that writes the right payload to the wrong table or with a wrong/missing.eq()filter still passeswrites.length === 1. Capturing the table and filters is cheap and makes these assertions meaningful.♻️ Proposed refactor
- const chain = { - update: (payload) => { - writes.push(payload); - return chain; - }, - select: () => chain, - insert: () => chain, - eq: () => result(), - }; + const makeChain = (table) => { + let current = null; + const chain = { + update: (payload) => { + current = { table, payload, filters: [] }; + writes.push(Object.assign(current, payload)); + return chain; + }, + select: () => chain, + insert: () => chain, + eq: (col, value) => { + current?.filters.push([col, value]); + return result(); + }, + }; + return chain; + };
from: (table) => makeChain(table)then lets tests assert onwrites[0].table/writes[0].filters.🤖 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 `@test/test-remote-transport.js` around lines 94 - 113, Update the fake transport setup around the chain object and returned from() method so it captures the table passed to from() and each filter supplied through eq(). Record writes as structured entries containing the payload, table, and filters, while preserving the existing chain behavior and write tracking used by the tests.
480-488: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
process.exitCodeand clear the race timer.The 3s timer at Line 482 is never cleared, so it only fails to delay exit because
process.exit()kills it.process.exit()can also truncate buffered stdout when the output is piped (common in CI), losing the pass/fail lines. Settingprocess.exitCodeand clearing the timer avoids both.♻️ Proposed change
let settled = false; + let timer; await Promise.race([ rc.setOffline(DEVICE_ID).then(() => { settled = true; }), - new Promise((r) => setTimeout(r, 3000)), + new Promise((r) => { timer = setTimeout(r, 3000); }), ]); + clearTimeout(timer); assert(settled, 'setOffline must settle rather than block the shutdown path'); }); console.log(`\n${failures ? '🔴' : '✅'} remote transport: ${failures} failing test(s).`); -process.exit(failures ? 1 : 0); +process.exitCode = failures ? 1 : 0;Note: with
exitCode, any lingering handle (e.g. the heartbeat timer) will keep the process alive — worth confirming teardown is complete before switching.🤖 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 `@test/test-remote-transport.js` around lines 480 - 488, Update the shutdown race near rc.setOffline to retain the 3-second timeout handle and clear it after Promise.race settles. Replace the final process.exit(...) call with process.exitCode assignment so output flushes normally, while confirming existing teardown leaves no lingering handles that prevent natural termination.
🤖 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.
Nitpick comments:
In `@test/test-remote-transport.js`:
- Around line 169-173: Strengthen the “the seen-call-id set stays bounded” test
by asserting that the newest bulk call ID, such as `bulk-249`, remains in
`device.seenCallIds` after processing all 250 calls. Keep the existing size
bound assertion to verify both bounded capacity and retention of the most recent
ID.
- Around line 369-414: Define a named constant for the recreate-failure
withdrawal threshold, following the existing SERVER_* constant conventions. Use
that constant for the sustained-failure loop count and derive the single-failure
test case from it so both tests remain coupled to the same threshold.
- Around line 201-217: Add a test alongside the existing doorbell retry tests
that configures three consecutive row-fetch failures, invokes rc.onDoorbell, and
verifies exactly three client attempts occur with no tool call delivered. Reuse
the existing makeRemoteChannel, sleep stub, and delivered collection patterns.
- Around line 94-113: Update the fake transport setup around the chain object
and returned from() method so it captures the table passed to from() and each
filter supplied through eq(). Record writes as structured entries containing the
payload, table, and filters, while preserving the existing chain behavior and
write tracking used by the tests.
- Around line 480-488: Update the shutdown race near rc.setOffline to retain the
3-second timeout handle and clear it after Promise.race settles. Replace the
final process.exit(...) call with process.exitCode assignment so output flushes
normally, while confirming existing teardown leaves no lingering handles that
prevent natural termination.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f498781a-617d-4d4c-9eaf-ba582b8b6d5f
📒 Files selected for processing (2)
src/remote-device/remote-channel.tstest/test-remote-transport.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/remote-device/remote-channel.ts
All four findings verified against HEAD and valid (CodeRabbit was reviewing an earlier commit, but the issues survive): 1. MAJOR — unhandled rejection could kill the device process. handleNewToolCall is async and its catch block awaits updateCallResult/notifyResult, which can themselves throw (`if (!this.client) throw`). Both call sites discarded the promise, so a second failure surfaced as an unhandled rejection — fatal on Node 15+. The failure-reporting path now has its own try/catch, and both call sites go through dispatchToolCall(), which observes the rejection. 2. MINOR — same root cause at the doorbell and legacy call sites; covered by the same helper. 3. MAJOR — updateCallResult's "log a summary, not the payload" comment was an own-goal: the log line then ran JSON.stringify over the whole sanitized result purely to report its length, eagerly, on every call. On the 13 MB payloads this path exists to handle that is a full extra serialization. Now gated on DEBUG_MODE. 4. MINOR — the teardown budget did not add up: 250 drain + 3x500 leave + 500 session + 3000 spawnSync = 5250ms against a 5s force-exit, while the comment claimed everything fit in 1.5s. Leave bounds cut to 300ms, total 4650ms, and the arithmetic is now written out. Two new test cases cover the rejection paths (async and sync throw); both were verified to fail with the fix reverted. Comments in remote-channel.ts cut again, 308 -> 275 (23%). Findings 3 and 4 were both cases of a long comment drifting from or contradicting its own code, which is the argument for keeping them short. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/test-remote-transport.js`:
- Around line 239-243: Update the “a synchronously throwing handler is contained
too” test to reuse the unhandled-rejection listener established by the preceding
test and await a setImmediate drain after onDoorbell(). Assert that the
collected event list remains empty, while preserving the existing non-rejection
assertion.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24e86f0c-ccf6-4d94-be12-c7e6e87f0c52
📒 Files selected for processing (3)
src/remote-device/device.tssrc/remote-device/remote-channel.tstest/test-remote-transport.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/remote-device/device.ts
- src/remote-device/remote-channel.ts
Comment on lines
+239
to
+243
| await test('a synchronously throwing handler is contained too', async () => { | ||
| const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } }); | ||
| rc.onToolCall = () => { throw new Error('sync throw'); }; | ||
| await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); // must not reject | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that synchronous throws do not become unhandled rejections.
This only verifies that onDoorbell() itself does not reject. A discarded handleNewToolCall() promise could still emit unhandledRejection on the next event-loop turn. Reuse the listener and setImmediate drain from the preceding test, then assert the event list remains empty.
Proposed test adjustment
const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } });
rc.onToolCall = () => { throw new Error('sync throw'); };
- await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); // must not reject
+ const unhandled = [];
+ const onUnhandled = (e) => unhandled.push(e);
+ process.on('unhandledRejection', onUnhandled);
+ try {
+ await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID });
+ await new Promise((resolve) => setImmediate(resolve));
+ } finally {
+ process.off('unhandledRejection', onUnhandled);
+ }
+ assert(unhandled.length === 0, `unhandled rejection escaped: ${unhandled[0]?.message}`);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await test('a synchronously throwing handler is contained too', async () => { | |
| const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } }); | |
| rc.onToolCall = () => { throw new Error('sync throw'); }; | |
| await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); // must not reject | |
| }); | |
| await test('a synchronously throwing handler is contained too', async () => { | |
| const { rc } = makeRemoteChannel({ row: { id: 'x', status: 'pending' } }); | |
| rc.onToolCall = () => { throw new Error('sync throw'); }; | |
| const unhandled = []; | |
| const onUnhandled = (e) => unhandled.push(e); | |
| process.on('unhandledRejection', onUnhandled); | |
| try { | |
| await rc.onDoorbell({ call_id: 'x', device_id: DEVICE_ID }); | |
| await new Promise((resolve) => setImmediate(resolve)); | |
| } finally { | |
| process.off('unhandledRejection', onUnhandled); | |
| } | |
| assert(unhandled.length === 0, `unhandled rejection escaped: ${unhandled[0]?.message}`); | |
| }); |
🤖 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 `@test/test-remote-transport.js` around lines 239 - 243, Update the “a
synchronously throwing handler is contained too” test to reuse the
unhandled-rejection listener established by the preceding test and await a
setImmediate drain after onDoorbell(). Assert that the collected event list
remains empty, while preserving the existing non-rejection assertion.
toolHistory keeps the full ServerResult of every call in memory, and
get_recent_tool_calls serialises those entries straight back out. That is
unbounded in two compounding ways:
- one large output (a big read_file, a wide list_directory) makes every later
history dump that includes it large too, and
- any tool whose output CONTAINS a history dump nests the whole history inside
itself. Observed while testing: a start_process running
`cat /tmp/dc_..._get_recent_tool_calls_....json` recorded that dump as its
own output, and each nesting level roughly doubles the JSON escaping. 83KB
of arguments on disk produced a 1.89MB in-memory dump.
That 1.89MB result then exceeded Supabase Realtime's max_record_bytes, which is
how it surfaced — the transport returned `undefined` (fixed separately on the
server side).
Extending EXCLUDED_TOOLS cannot fix this: get_recent_tool_calls is already
excluded, and the nesting arrives through ordinary tools that merely happen to
read a dump back. Capping the stored output does. A preview loses nothing real —
this history is a "what happened recently" aid, not a result cache.
Outputs over 4KB are replaced with a short marker, keeping the { content: [...] }
shape so readers and formatters need no special case. An unserialisable output
(circular) is also dropped rather than retained, since it could never be
returned to a client anyway.
Note this only bounds NEW entries; a long-running process keeps whatever it has
already accumulated until restart.
Also fixes test/test-line-count.js, which set allowedDirectories to its own test
directory at import time and never restored it — leaking into the user's real
~/.claude-server-commander/config.json and refusing every later filesystem call
outside test_output. It was the only test in the suite without a restore. Now
saves the config in setup(), restores in teardown(), and runs teardown on the
failure path too (it previously called process.exit(1) and skipped cleanup).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EuCmPHU99uRKefvCwtWHYR
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/utils/toolHistory.ts`:
- Around line 240-260: Update capOutput to measure serialized output size in
UTF-8 bytes using Buffer.byteLength rather than string length, and when the cap
is exceeded return a minimal ServerResult containing only the replacement text
marker and any required error flag. Do not spread the original output, so
structuredContent and _meta cannot be retained.
In `@test/test-line-count.js`:
- Around line 20-28: Update the test setup and teardown around originalConfig so
teardown restores only the original allowedDirectories value through the
existing configuration API, rather than passing the full snapshot to
updateConfig. Preserve unrelated configuration changes made during the test, and
keep the setup behavior that restricts allowedDirectories to TEST_DIR.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: affeda87-3616-4d5e-9bb6-da5b365a7df6
📒 Files selected for processing (2)
src/utils/toolHistory.tstest/test-line-count.js
Comment on lines
+240
to
+260
| private capOutput(output: ServerResult): ServerResult { | ||
| let size: number; | ||
| try { | ||
| size = JSON.stringify(output)?.length ?? 0; | ||
| } catch { | ||
| // Circular or otherwise unserialisable — it could never be returned to a | ||
| // client anyway, so don't retain it. | ||
| size = Number.POSITIVE_INFINITY; | ||
| } | ||
| if (size <= this.MAX_STORED_OUTPUT_BYTES) return output; | ||
|
|
||
| const shown = Number.isFinite(size) ? `${size} bytes` : 'unserialisable'; | ||
| return { | ||
| ...(output as any), | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `[output omitted from history: ${shown}, over the ${this.MAX_STORED_OUTPUT_BYTES}-byte cap]`, | ||
| }, | ||
| ], | ||
| } as ServerResult; |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Make the replacement result actually bounded.
Line 253 retains the original structuredContent and _meta, so oversized fields are still persisted after content is replaced. Measure UTF-8 bytes with Buffer.byteLength rather than UTF-16 .length, and return only the bounded marker plus any required error flag.
Proposed fix
private capOutput(output: ServerResult): ServerResult {
let size: number;
try {
- size = JSON.stringify(output)?.length ?? 0;
+ size = Buffer.byteLength(JSON.stringify(output), 'utf-8');
} catch {
// Circular or otherwise unserialisable — it could never be returned to a
// client anyway, so don't retain it.
size = Number.POSITIVE_INFINITY;
}
if (size <= this.MAX_STORED_OUTPUT_BYTES) return output;
const shown = Number.isFinite(size) ? `${size} bytes` : 'unserialisable';
return {
- ...(output as any),
content: [
{
type: 'text',
text: `[output omitted from history: ${shown}, over the ${this.MAX_STORED_OUTPUT_BYTES}-byte cap]`,
},
],
- } as ServerResult;
+ isError: output.isError,
+ };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private capOutput(output: ServerResult): ServerResult { | |
| let size: number; | |
| try { | |
| size = JSON.stringify(output)?.length ?? 0; | |
| } catch { | |
| // Circular or otherwise unserialisable — it could never be returned to a | |
| // client anyway, so don't retain it. | |
| size = Number.POSITIVE_INFINITY; | |
| } | |
| if (size <= this.MAX_STORED_OUTPUT_BYTES) return output; | |
| const shown = Number.isFinite(size) ? `${size} bytes` : 'unserialisable'; | |
| return { | |
| ...(output as any), | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `[output omitted from history: ${shown}, over the ${this.MAX_STORED_OUTPUT_BYTES}-byte cap]`, | |
| }, | |
| ], | |
| } as ServerResult; | |
| private capOutput(output: ServerResult): ServerResult { | |
| let size: number; | |
| try { | |
| size = Buffer.byteLength(JSON.stringify(output), 'utf-8'); | |
| } catch { | |
| // Circular or otherwise unserialisable — it could never be returned to a | |
| // client anyway, so don't retain it. | |
| size = Number.POSITIVE_INFINITY; | |
| } | |
| if (size <= this.MAX_STORED_OUTPUT_BYTES) return output; | |
| const shown = Number.isFinite(size) ? `${size} bytes` : 'unserialisable'; | |
| return { | |
| content: [ | |
| { | |
| type: 'text', | |
| text: `[output omitted from history: ${shown}, over the ${this.MAX_STORED_OUTPUT_BYTES}-byte cap]`, | |
| }, | |
| ], | |
| isError: output.isError, | |
| }; | |
| } |
🤖 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 `@src/utils/toolHistory.ts` around lines 240 - 260, Update capOutput to measure
serialized output size in UTF-8 bytes using Buffer.byteLength rather than string
length, and when the cap is exceeded return a minimal ServerResult containing
only the replacement text marker and any required error flag. Do not spread the
original output, so structuredContent and _meta cannot be retained.
Comment on lines
+20
to
+28
| originalConfig = await configManager.getConfig(); | ||
| // Ensure test dir is allowed | ||
| await configManager.setValue('allowedDirectories', [TEST_DIR]); | ||
| await fs.mkdir(TEST_DIR, { recursive: true }); | ||
| } | ||
|
|
||
| async function teardown() { | ||
| if (originalConfig) { | ||
| await configManager.updateConfig(originalConfig); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 5 '\b(getConfig|updateConfig|setValue)\s*\(' src testRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the config manager implementation and relevant test imports.
fd -a 'config(s|Manager)?.ts|line-count.js' . | sed 's#^\./##'
echo '--- candidate config files ---'
fd 'config.*\.ts$|.*(config).*ts$' src test 2>/dev/null || true
echo '--- imports in test/test-line-count.js ---'
sed -n '1,60p' test/test-line-count.js
echo '--- precise configManager definitions/exports/usages ---'
rg -n -C 8 'class .*Config|function .*Config|async function (getConfig|updateConfig|setValue)|default .*Config|export .*configManager|const configManager' src test --glob '!**/test-edit-block-line-endings.js' --glob '!**/test-edit-block.js' | head -n 240
echo '--- src exact config manager file outline ---'
ast-grep outline src/tools/config-manager.ts --view expanded 2>/dev/null || true
sed -n '1,220p' src/tools/config-manager.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 16783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/config-manager.ts outline ---'
ast-grep outline src/config-manager.ts --view expanded 2>/dev/null || true
echo '--- source method sections in src/config-manager.ts ---'
rg -n -C 12 'class ConfigManager|getConfig|setValue\(|updateConfig\(|saveConfig\(|loadConfig|configPath|writeChain|scheduleSave' src/config-manager.ts
echo '--- source lines 50-180 src/config-manager.ts ---'
sed -n '50,180p' src/config-manager.ts
echo '--- source lines 180-340 src/config-manager.ts ---'
sed -n '180,340p' src/config-manager.tsRepository: wonderwhy-er/DesktopCommanderMCP
Length of output: 21542
Restore only allowedDirectories instead of the full configuration.
updateConfig() merges the passed object into the current config before saving, but setup() already replaced the live config’s allowedDirectories and teardown() is passing the original snapshot back. Capture/restore the original allowedDirectories at teardown so any user/config manager change to another field made during the test is not lost.
🤖 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 `@test/test-line-count.js` around lines 20 - 28, Update the test setup and
teardown around originalConfig so teardown restores only the original
allowedDirectories value through the existing configuration API, rather than
passing the full snapshot to updateConfig. Preserve unrelated configuration
changes made during the test, and keep the setup behavior that restricts
allowedDirectories to TEST_DIR.
Three findings from a cross-repo review of this branch.
1. recreateChannel() rebuilt both channels inside realtime-js's post-disconnect
'disconnecting' window. removeChannel() on the last channel makes realtime-js
call disconnect() itself; _teardownConnection() then nulls the conn.onclose
that would clear the state, so only an internal ~100ms fallback timer does.
connect() early-returns for that whole window, so subscribe()'s socket.connect()
was a silent no-op and BOTH channels — including the deliberately independent
legacy one — sat in 'joining' until the 10s join timeout. Every first recreate
was wasted, leaving the device dark on both transports for ~20-45s and burning
one of three TRANSPORT_WITHDRAW_AFTER_ATTEMPTS before a genuine attempt.
Reproduced against the installed realtime-js 2.90.0. Now waits for the client
to leave 'disconnecting' before rebuilding.
2. The per-entry tool-history output cap ran only in addCall(), never on the
load-from-disk path, so it did nothing for anyone upgrading with an existing
tool-history.jsonl — i.e. exactly the population it was written for. Verified
against a real 4.0MB/140-record file holding a single 3.8MB read_file entry:
under the 5MB whole-file trim, so it loaded uncapped and get_recent_tool_calls
still returned it in full. Now capped on the way in as well as on the way out.
3. 'concurrent status writes stay ordered' could not fail. The fake recorded
writes at .update() invocation, which happens synchronously before
setOnlineStatus's only await, so it only ever observed the order writes were
ISSUED — which holds with or without the statusWriteChain serialisation. The
fake now records on completion with per-write latency; with queueStatusWrite
reverted to fire-and-forget the test goes red on the ordering assertion
("got online,offline"), and passes as shipped.
Build clean; test/test-remote-transport.js 30/30.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BdFJB6KxeWHdYqbi63KrPP
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.
Summary by CodeRabbit