feat: add Pi RPC adapter#12
Conversation
There was a problem hiding this comment.
🎉 Thanks for your first PR to ORCH! We're excited to review your contribution.
Our CI will run automatically:
- ✅ TypeScript strict mode check (
tsc --noEmit) - ✅ Full test suite (1493+ tests via Vitest)
- ✅ ESM build verification
A maintainer will review your changes shortly. In the meantime, check the Contributing Guide to make sure everything is in order.
⭐ If you enjoy working with ORCH, a star on the repo goes a long way!
oxgeneral
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the adapter is well-structured, the tests are solid (especially the 20K-character agent_end case that drove the v2 line-reader fix), and all the registration points are covered correctly.
I ran npm run typecheck and npm test against this branch — both green (1942 passed, 1 unrelated flaky TUI test that also passes in isolation on main).
I have a few correctness concerns around stream lifecycle that I'd like addressed before merge. None of them are about the overall design — that's right.
Blockers (correctness)
B1 — Pi may hang waiting for stdin EOF
src/infrastructure/adapters/pi.ts:71-77
After writing the prompt JSONL, proc.stdin is left open. Many CLIs in --mode rpc keep reading stdin and won't terminate without EOF. If Pi has that behavior, the agent_end event never arrives, the generator hangs, and the orchestrator only recovers via the kill-on-done path that never fires.
if (proc.stdin) {
proc.stdin.write(JSON.stringify({ ... }) + '\n');
proc.stdin.end(); // <-- add this
}If Pi RPC actually expects multiple stdin commands per session, please confirm with a code comment — but for ORCH's one-shot model, a single prompt + EOF is the intended pattern.
B2 — Generator can hang on AbortSignal
src/infrastructure/adapters/pi.ts:101-132
When signal.aborted triggers and we break out of the for await loop, control falls into await exitPromise while the Pi process is still alive (kill is only called on the done branch). The orchestrator will eventually call stop() from the outside, but until then the generator holds a pending Promise and the registered 'close' / 'error' listeners pin the proc object.
Suggestion: in the finally block, if signal?.aborted && !gotDoneEvent, kick off processManager.killWithGrace(pid, 1_000).catch(() => {}) before awaiting exitPromise.
B3 — Unhandled stream error before first byte
src/infrastructure/adapters/pi.ts:108
for await (const line of readPiRpcLines(proc.stdout)) doesn't catch errors emitted on the stream. If Pi crashes immediately or stdout errors before producing output, the 'error' event propagates out of the async generator as an unhandled rejection. claude.ts routes through createStreamingEvents in adapters/utils.ts, which handles this — the Pi adapter takes its own path and reintroduces the gap.
Suggestion: wrap the for await in try/catch, classify with classifyAdapterError, and yield an error event before letting the exit-handling logic run. A test for "stdout emits 'error' before any line" would be valuable too.
B4 / B5 — Release artifacts (maintainer side)
CHANGELOG.md entry and readme.md test-count badge are part of the project's release checklist (see docs/RELEASING.md) — I'll handle those as part of the next release. No action needed from you.
Important
I1 — Dead branch in extractPassiveUpdate
src/infrastructure/adapters/pi.ts:315
return state.finalText ? null : null;Both branches return null, and the function never propagates anything when there are no tokens. If the intent was just "no event when no tokens", the whole helper simplifies:
function extractPassiveUpdate(parsed: Record<string, unknown>): ParsedPiEvent | null {
const tokens = extractPiTokensFromMessage(parsed);
return tokens ? { tokens } : null;
}(The state parameter then drops from the signature.)
I3 — Buffer<ArrayBufferLike> generic
src/infrastructure/adapters/pi.ts:388
let pending: Buffer<ArrayBufferLike> = Buffer.alloc(0);The generic form of Buffer<T> only landed in @types/node 22+. This project pins ^20.17.0. It compiles on some installs because npm hoists a newer transitive copy, but at the floor of the range it fails. Plain Buffer is enough:
let pending = Buffer.alloc(0);I4 — as any in test mock
test/unit/infrastructure/pi-adapter.test.ts:43
spawn: vi.fn(() => ({ process: proc as any, pid: proc.pid })),CLAUDE.md rules out any. Cast to ChildProcess (or define a narrow shape with the fields actually used). Other adapter tests follow this convention.
I6 — Token alias spread
src/infrastructure/adapters/pi.ts:363-375
Eight different aliases for five fields (cacheRead / cache_read / cache_read_input_tokens, etc.). Are all of these emitted by Pi across versions, or is some of it defensive against shapes Pi never produces? A short comment + reduction to the names actually observed would help future readers. If they really all appear, consider a small alias map:
const ALIASES = {
input: ['input', 'input_tokens'],
output: ['output', 'output_tokens'],
reasoning: ['reasoning', 'reasoning_tokens'],
cache_read: ['cacheRead', 'cache_read', 'cache_read_input_tokens'],
cache_write: ['cacheWrite', 'cache_write', 'cache_creation_input_tokens'],
};I8 — Silent stderr
src/infrastructure/adapters/pi.ts:88
proc.stderr?.resume() drains stderr without surfacing it. If Pi prints auth or extension-load errors there, they vanish. claude.ts / codex.ts either pipe stderr into events or surface the tail on non-zero exit. Worth aligning.
Out of scope for this PR
- I2 — deduplicating
createPiRpcEventsagainstcreateStreamingEventsinadapters/utils.ts. The right fix is to extend the shared helper with anafterDonehook so all adapters benefit. I'll do that as a follow-up after this lands. assets/adapters-{dark,light}.svgandlanding/index.html— visual assets, maintainer side.
Once B1-B3 and I1, I3, I4 are addressed I'll re-review and we can ship. Thanks again for the careful work — the RPC line-buffer fix was a real find.
Summary
Adds a native
piadapter that runs the Pi coding agent in headless RPC mode.pi --mode rpcand sends ORCH prompts as JSONLpromptcommandsAgentEventsconfig.modelvia--modelandconfig.effortvia--thinkingpiin the adapter registry, model tiers, TUI wizard, init/agent help, and docsVerification
npm run typechecknpm test— 112 files, 1942 passing, 2 skippednpm run build--adapter pi, ran a Pi task, and verified it wrotesmoke.txtcontainingPI_ORCH_ADAPTER_OKNotes
The adapter intentionally preserves Pi's own harness prompt by passing ORCH's system prompt through
--append-system-promptrather than replacing it. Pi RPCextension_ui_requestevents are ignored so extensions/tools can stay enabled without polluting ORCH logs.