feat(daemon): close pi adapter parity gaps β imagePaths, extraAllowedDirs, error events, sendAgentEvent routing#763
Conversation
β¦Dirs, error events, sendAgentEvent routing
There was a problem hiding this comment.
π‘ Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98fea9e98b
βΉοΈ 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".
mrcfps
left a comment
There was a problem hiding this comment.
@monotykamary thanks for closing the pi adapter parity gaps here β the image forwarding and error-event coverage are useful additions. I found one integration issue in the server wiring that currently breaks pi-rpc event delivery and needs a fix before merge.
Generated by Looper 0.6.1 Β· runner=reviewer Β· agent=opencode
lefarcen
left a comment
There was a problem hiding this comment.
Hi @monotykamary! Thanks for tackling these parity gaps β closing the pi adapter's missing capabilities is definitely the right direction. I've run a deep review on both the implementation and the reasoning.
Found 4 substantive issues that need attention before this can land safely β 2 in the server wiring logic that affect whether error tracking actually works, 1 in the image-forwarding path, and 1 documentation gap about access assumptions. See inline comments below for each.
lefarcen
left a comment
There was a problem hiding this comment.
The P1 server wiring issue from mrcfps's review (and my 07:12 review) is still present on this head.
The problem: server.ts passes sendAgentEvent into attachPiRpcSession, but attachPiRpcSession invokes its callback with the two-arg channel/payload shape: send('agent', payload) for normal events and send('error', { message }) from fail(). sendAgentEvent expects a single event object, so the first argument becomes the string 'agent'/'error' and the real payload is discarded. That means pi text deltas/status updates are emitted as malformed SSE, agentProducedOutput is never set, and the new error events won't set agentStreamError.
The fix: Adapt the callback at the call site or change attachPiRpcSession's callback contract consistently. Either:
- Wrap
sendAgentEventso it translatessend('agent', payload) β sendAgentEvent(payload)andsend('error', { message })β existing error path, OR - Change
attachPiRpcSessionto invokesend(payload)directly (single-arg) and handle errors outside.
Can't approve until this integration is corrected β it breaks the error-surfacing path this PR is trying to add.
|
Hi @monotykamary! π Thanks for making open-design better! |
β¦pping, image hardening, docs clarification - Fix P1: wrap sendAgentEvent in channel/payload adapter so pi-rpc events route correctly. agent channel payloads go through sendAgentEvent, error channel payloads go through createSseErrorPayload + agentStreamError. - Fix P2: map message_update error deltas (type=error) to error events so partial-output failures surface after the agent started streaming. - Fix P2: harden image forwarding with realpath symlink resolution, stat.isFile() check, extension whitelist, and count/total-byte budgets. - Fix P2: clarify in docs that --append-system-prompt is only a hint and does not grant filesystem access; document cwd staging fallback.
lefarcen
left a comment
There was a problem hiding this comment.
All findings resolved in eab3633 β
The P1 server wiring callback mismatch is fixed β sendAgentEvent now correctly receives single-arg event payloads via the channel/payload adapter wrapper. The error delta mapping, image hardening, and docs clarification are also in place. Nice work addressing the full review feedback!
CI is still running; once green this is good to merge from my side.
mrcfps
left a comment
There was a problem hiding this comment.
@monotykamary thanks for the quick follow-up on the pi adapter parity fixes β the callback bridge, error-event mapping, and docs updates address the main flow well. I found one remaining image-boundary hardening gap that should be straightforward to tighten. π
Generated by Looper 0.6.1 Β· runner=reviewer Β· agent=opencode
lefarcen
left a comment
There was a problem hiding this comment.
All prior findings from my 07:12 review are now addressed in eab3633. The server wiring callback is correctly adapted, error delta mapping is complete, image forwarding is hardened with symlink checks and budgets, and docs clarify the --append-system-prompt behavior.
LGTM π
β¦warding After fs.realpathSync, the resolved path can escape UPLOAD_DIR via symlinks. Add uploadRoot param to attachPiRpcSession and verify that realPath stays inside the resolved upload root. Both uploadRoot and realPath are realpathSync'd to handle macOS /var β /private/var. Added 2 fixture tests: symlink outside uploadRoot is rejected, symlink inside uploadRoot is allowed.
mrcfps
left a comment
There was a problem hiding this comment.
@monotykamary thanks for the quick follow-up on the pi adapter parity work β the runtime fixes and image-boundary hardening are moving in the right direction. I found one remaining compile-time integration issue from the new uploadRoot parameter that needs to be addressed before this can merge. π
lefarcen
left a comment
There was a problem hiding this comment.
The compile-time integration issue from mrcfps's 08:57 review is still present on this head.
The problem: apps/daemon/src/connectionTest.ts line 867 calls attachPiRpcSession with imagePaths: [] but without uploadRoot. Since commit 99a8c1b added uploadRoot as a required parameter to the function signature (line 267 of apps/daemon/src/pi-rpc.ts), TypeScript will error with "Property 'uploadRoot' is missing" when building @open-design/daemon.
The fix: Add uploadRoot: undefined (or another safe value) to the attachPiRpcSession call in connectionTest.ts. Since imagePaths is empty in the connection-test path, uploadRoot doesn't need a real directory β undefined is sufficient and the image-forwarding code will skip it.
|
@monotykamary The connectionTest.ts call site (line 867) still needs the Could you add |
|
@monotykamary I checked the current head (99a8c1b) and the compile-time integration issue mrcfps pointed out still needs fixing. The call site at attachPiRpcSession({
child,
prompt,
cwd,
model: model ?? null,
send,
imagePaths: []
})But This will unblock the daemon package build that's currently failing CI. |
The connection test call site was missing the new uploadRoot param, causing TS2345 in CI. Added uploadRoot: undefined since imagePaths is empty and the boundary check is a no-op for this path.
mrcfps
left a comment
There was a problem hiding this comment.
@monotykamary I reviewed the current ff24a81 head across the pi adapter arg wiring, pi-rpc event and image handling, server routing, connection-test call site, tests, and adapter docs. The prior callback bridge, image-boundary, and uploadRoot call-site concerns are addressed, and I don't see any remaining actionable implementation issues. Thanks for the careful follow-through on these parity fixes β this is a solid improvement for pi users. π
lefarcen
left a comment
There was a problem hiding this comment.
Compile-time integration issue resolved in ff24a81 β
The uploadRoot: undefined parameter is now correctly passed to attachPiRpcSession in connectionTest.ts. This unblocks the daemon package build and satisfies the function signature requirement mrcfps and I flagged.
All prior findings from my reviews are now addressed. CI is green. LGTM π
|
Hi @monotykamary β quick note from the Open Design team. You've already shipped 5 merged PRs here, which puts you in our top contributors. We've spun up a Discord #contributors channel and wanted to make sure you have the invite β you're exactly the kind of person it's built for: π https://discord.gg/3C6EWXbdQQ What you get there:
Drop in whenever β no introduction needed. The recent run has visibly moved the bar β thanks. |
Summary
The pi adapter was missing capabilities that every other recently-updated adapter (Qoder, Copilot, OpenCode) now supports. This PR closes four parity gaps so pi runs get the same error surfacing, file-access, and multimodal capabilities as the rest of the adapter catalog.
Why
Recent commits added Qoder CLI support, connection tests, Copilot stdin delivery, and OpenCode error surfacing (#691). The pi adapter:
imagePathsβ other adapters forward images via--attachmentor inline args; pi's RPC protocol natively supportsimageson thepromptcommand but the daemon never sent them.extraAllowedDirsβ other adapters use--add-dirto widen the agent's filesystem view so it can read skill seeds and design-system specs outside the project cwd; pi had no equivalent.extension_errorandauto_retry_endfailure events β these RPC events were silently dropped bymapPiRpcEvent, so extension errors and exhausted retries would never surface to the user.sendinstead ofsendAgentEventβ this bypassed theagentStreamErrorflag and thetrackingSubstantiveOutput/agentProducedOutputguard added for issue Fix OpenCode runs completing in seconds without producing any outputΒ #691. A pi run that exited 0 without producing visible content was markedsucceededwith an empty chat β the exact silent-failure shape Fix OpenCode runs completing in seconds without producing any outputΒ #691 fixed for OpenCode.Changes
apps/daemon/src/agents.tsbuildArgsnow acceptsextraAllowedDirs(was_extra), filters to absolute paths, and forwards each as--append-system-prompt. AddssupportsImagePaths: trueflag.apps/daemon/src/pi-rpc.tsattachPiRpcSessionacceptsimagePaths, reads files into base64, and includesimageson the RPCpromptcommand.mapPiRpcEventhandlesextension_errorβerrorevent andauto_retry_endwithsuccess: falseβerrorevent.apps/daemon/src/server.tstrackingSubstantiveOutput = trueand passessendAgentEventinstead ofsend, forwardingsafeImageswhensupportsImagePathsis set.apps/daemon/src/connectionTest.tsimagePaths: []toattachPiRpcSessionfor forward-compatibility.docs/agent-adapters.mdTesting
vitest run tests/pi-rpc.test.tsvitest run tests/agents.test.ts(pi subset)tsc -p apps/daemon/tsconfig.json --noEmitNew tests
tests/pi-rpc.test.ts(9 new):extension_errormaps to error event (3 variants: string, non-string, missing)auto_retry_endwithsuccess=falsemaps to error (3 variants: with message, success=true, missing finalError)attachPiRpcSessionforwards images whenimagePathsprovidedattachPiRpcSessionomits images whenimagePathsis emptyattachPiRpcSessionskips unreadable image paths gracefullytests/agents.test.ts(3 new + 1 assertion):pi.supportsImagePaths === trueextraAllowedDirsas--append-system-promptflagsextraAllowedDirsextraAllowedDirsWhat is NOT changed