Repository navigation
fix(server): answer hosted agent relay refusals as agent-device errors - #2575
Conversation
janicduplessis
left a comment
There was a problem hiding this comment.
Fresh review (Claude). No blocking issues found.
Refusal boundary: unchanged. I compared pinLease input by input against origin/main:
body === null(oversized) was refused before callingpinLease. It now reachespinLease, goes torpc = undefined, and is refused with rulerequest.- JSON parse failure, a non-object root, a non-string
methodor non-objectparamsare all refused with rulerequest, the same set as before. - Command methods: a top-level command that fails
isAllowedCommandis refused. The batchforloop uses exactly the old.some()predicate (!isJsonObject(step) || !isAllowedCommand(step.command)), and its early return happens before any rewrite. - Lease methods are rewritten exactly as before, and any other method is refused.
- The forwarded bytes (
Buffer.from(JSON.stringify(rpc))) andrelay()are untouched. Status stays 400.
Envelope vs the agent-device client. Checked against the installed client (0.21.12, daemon-client-lifecycle.js). Xt reads data.code (normalized; UNAUTHORIZED is a known code), data.message, data.hint, data.details (object) and data.retriable (boolean). Qt spreads details into the thrown error's details next to hint/diagnosticId/logPath. reason/rule/command/method don't collide with those keys. The JSON-RPC error.code number is ignored by the client, so -32000 is fine. The daemon itself uses -32001 for its proxy-token UNAUTHORIZED, so you could match that, but it's optional. Because baseUrl is set, the client will add a logPathUnavailable: ... the daemon named no diagnostics record detail. That's harmless.
Leaks and injection. None that cross a principal. The only echoed values are the requester's own command/method strings and its id, and they go back on the same connection. JSON.stringify keeps the body valid JSON whatever the input. Nothing is logged server-side. The echo is unbounded, though: a ~1 MB method string comes back three times (error.message, data.message, details.method), so up to ~3 MB to the sender. That only amplifies against the sender, and the sender's own terminal is the only place escape sequences could land. Optional: truncate the echoed name (e.g. 128 chars) to keep refusal bodies small.
Tests
- The non-object batch-step branch (
refuseCommand(undefined)for e.g.batchSteps: ['doctor']or[null]) has no test, either before or after this PR. It is the branch that matters most here:pinStepreturns a non-object step unchanged, so if this check regressed, the step would be forwarded upstream un-pinned. Consider adding one row to the new table (rule: 'command', nocommandin details). - The new table repeats cases the pre-existing status-only loops already cover:
lease.allocate,'not json',{ method: 'agent_device.command' }with no params, and a disallowed batch step. Per CLAUDE.md ("extend a relevant case instead of repeating it"), consider folding the old status-only loops into the table, or dropping the duplicates there. - Nit: the
id: truerow saysdetails: {}, but the refusal does includedetails.method.toMatchObjecthides that, so the row reads as if there are no details. Use{ method: 'agent_device.lease.allocate' }or rename the row. - The 1 MB+1 row is not flaky as far as I can see.
readBodydrains the whole request before answering, so the client never sees EPIPE/ECONNRESET mid-write.
Docs / style
- The hint always says the connection "allows only ". For
rule: methodrefusals that is slightly off, since lease heartbeat/release methods are also forwarded. Minor. - The guide and website text is accurate for command/method refusals. Malformed requests get rule
requestand a generic message, which the docs don't claim to name, so that's fine. STIM_AGENT_REQUEST_REFUSEDis a newSTIM_*token that isn't indexed instim guide errors. The guide code-scan test only readscommands/macos.ts, notguide/macos.ts, so nothing fails. Your call whether adetails.reasonbelongs in the errors index.- The diff is ASCII-only, adds no new comments, and the empty
catch {}matches existing usage in this file.
Description
When a hosted macOS app's agent-device client sends a request the pinned relay refuses (for example
agent-device doctor --platform macos),relayPinnedanswered400 text/plain "Unsupported request.". The client parses every response body as JSON-RPC, so the agent sawCOMMAND_FAILED"Invalid daemon response" withUnexpected token 'U'instead of a refusal.Solution
relayPinnednow answers the refusal in the shape the agent-device daemon uses for its own typed errors: a JSON-RPC 2.0 error envelope withdata.code: UNAUTHORIZED, a hint,retriable: falseanddetails.reason: STIM_AGENT_REQUEST_REFUSED.details.rulenames what was refused (command,methodorrequest) and the message names the command or method and lists the allowed commands. The request's JSON-RPCidis echoed when it is a string or number. HTTP status stays 400 andpinLeaserefuses exactly the inputs it refused before; device selectors are still stripped and the platform forced to macOS, not refused.stim guide macosandwebsite/docs/macos.mddescribe the refusal.Code written by Codex gpt-6.1-sol.
Test plan
pnpm test packages/server/__tests__/agent-device-driver.test.ts: the existing pinning test now asserts the envelope (status 400,application/json, id echo,details.reason/rule/command/method,retriable: false) for a disallowed command, a disallowed batch step, a disallowed method, non-JSON, a malformed body and an oversized body.AgentDeviceDriver(script run on the MacBook with a lease for a fake app; not run through the Mac mini's stim-server, which runs main):doctor --platform macosanddevicesnow fail withUNAUTHORIZED,details.reason: STIM_AGENT_REQUEST_REFUSED,rule: command, messageRefused command "doctor". Allowed commands: ....open com.apple.TextEditstill gets the daemon's ownMACOS_APP_LEASE_DENIED.Fixes #2574