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.
Address review feedback on the Pi RPC adapter — all changes are correctness fixes around stream lifecycle and diagnostics, no design changes. Blockers: - B1: close stdin after writing prompt so Pi --mode rpc sees EOF and emits agent_end. Without this the orchestrator hangs and only recovers via the external kill path that never fires. - B2: in the finally block, if we exit without a 'done' event due to abort or a stream error, schedule killWithGrace(pid, 1_000). This unblocks exitPromise and releases the 'close'/'error' listeners that pin the ChildProcess. - B3: wrap the stdout for-await in try/catch. A pre-first-byte stdout 'error' (ECONNRESET, EPIPE, …) previously escaped as an unhandled rejection; now it is classified via classifyAdapterError and yielded as an AgentEvent of type 'error'. Important: - I1: simplify extractPassiveUpdate — both branches returned null; drop the unused ParseState argument and reduce to a single return. - I3: drop Buffer<ArrayBufferLike> generic in readPiRpcLines — the generic form requires @types/node 22+, but the project pins ^20.17.0. Use plain Buffer with a null sentinel to keep TS strict-mode happy. - I4: remove `proc as any` from the test mock — cast through `unknown` to ChildProcess (CLAUDE.md forbids `as any`). - I6: collapse the token-alias spread (8 keys across 5 fields) into a single PI_TOKEN_ALIASES map with a comment explaining why aliases exist — Pi versions plus Anthropic-style fallthrough. - I8: replace `proc.stderr?.resume()` with createStderrTailCapture(), retaining the last 4 KB of stderr so auth/extension-load failures surface in the non-zero-exit error message instead of vanishing. Tests: - New: closes stdin after writing prompt (B1) - New: kills the process when the abort signal fires before a done event (B2) - New: yields an error event when stdout emits an error before any line (B3) - New: appends stderr tail to error message on non-zero exit (I8) - Updated: createMockProcessManager casts through unknown (I4) Verified: npm run typecheck (clean), npm test (1947 passed, 2 skipped). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the existing claude/codex/opencode test rows with a pi case in
suites that already enumerate adapters one-by-one. Tightens the safety
net for the new adapter without changing behavior.
- agent-factory: pi resolves to openai-codex/gpt-5.5; MCP skills filtered
- commands-init: getDefaultAgents('pi') uses pi adapter + pi-tier model;
MCP skills excluded (only claude keeps them)
- wizard-effort: effort step is shown for pi (matches EFFORT_ADAPTERS)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The empty-state onboarding tip for the Agents tab listed only four of the
six adapters ORCH ships ("claude, codex, cursor, shell") — pi and opencode
were missing. New users on the Agents tab couldn't discover them without
reading docs.
Update the description to enumerate all six and export ONBOARDING_AGENTS
so a regression test can assert it stays in sync with SUPPORTED_ADAPTERS.
Future adapter additions will fail the test if the tip isn't updated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@ziahm6638 — pushed three follow-up commits addressing the review. Everything ran clean: Review feedbackBlockers
Important
Out of scope (maintainer side, per your review)
Extra polish in this pushWhile auditing the PR for completeness, I also added:
Ready for re-review whenever you have time. Thanks again for the RPC adapter — the line-buffer fix really is a real find. |
The "1694 tests" claim is from before the Pi adapter, the parametrized adapter tests, the Pi stream-lifecycle hardening, and the onboarding regression test all landed on this branch. Bump the badge, the `npm test` comment in the Development section, and the Quick Start checklist in CONTRIBUTING.md to the current count. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds the first integration test under test/integration/ that drives one task through the entire orchestrator pipeline with a real PiAdapter and a mocked ProcessManager.spawn — no subprocess, no network, ~12ms in CI. What "from-and-to" covers in this single test: 1. Dispatch: real Orchestrator.tick() pulls the todo task, matches the idle pi agent via AgentService.findBestAgent, calls PiAdapter.execute which actually spawns (mocked), writes the prompt JSONL to stdin, and closes stdin (B1 invariant — checked via spy on proc.stdin.end). 2. Stream parsing: we feed Pi RPC JSONL lines through the real PassThrough stdout — message_update (text_delta), tool_execution_start (read), tool_execution_end (bash + write), agent_end with usage — and assert they map to the right RunEvent types in the real run store. 3. State machine: assert the full transition trace todo → in_progress → review → done (auto-approve, because makeAgent defaults to approval_policy=auto). The transitions are read from the real event bus, not from internal orchestrator methods. 4. Agent cleanup: after the run, the agent is back to idle, current_task is cleared, and tasks_completed is incremented. 5. Lifecycle: PiAdapter calls processManager.killWithGrace(pid, 1000) after the terminal event — verified. 6. Tokens: usage from agent_end propagates onto the Run, with total including input + output + reasoning and cache_read/write treated as informational (matches the comment in domain/run.ts). This file is the template for adapter e2e tests. For a new adapter, copy this file, swap PiAdapter, and rewrite only the stdout JSONL payloads plus parse-specific assertions. Test count bumped to 1953 across readme + CONTRIBUTING. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reproduced live against pi-coding-agent 0.73.1 with a real OpenRouter run
of one prompt-and-write task:
- With proc.stdin.end() (the B1 fix per review): pi emits the
{type:"response",success:true} preflight, then agent_start, turn_start,
user message_start + message_end, and stalls. The LLM call never runs,
the assistant message_start never arrives, agent_end is never produced.
The orchestrator timed the run out with only one event in run.jsonl
(the echoed user message_start), and the task was marked succeeded
without files_changed or tokens — and no hello.md on disk.
- Without proc.stdin.end(), the same prompt produces the full event
stream — text_delta deltas, tool_execution_start, tool_execution_end
for `write`, turn_end, agent_end with usage — and hello.md is written
on disk. End-to-end run.jsonl has 19 events and tokens populate on
the Run (input + output + cache_read).
Root cause: `pi --mode rpc` (see dist/modes/rpc/rpc-mode.js in 0.73.1) is
explicitly a persistent server-mode session: `case "prompt"` schedules
`session.prompt(...)` asynchronously and the preflight response is emitted
back through stdout. Closing stdin between the write and the response
breaks the session.
The B1 review note had it the other way around. The original PR design
(long-lived process killed via processManager.killWithGrace(pid, 1000)
after the terminal `done` event) is correct, and `createPiRpcEvents`
already does that.
Changes:
- pi.ts: remove `proc.stdin.end()`; replace the misleading "Pi reads
stdin until EOF" comment with a precise note that pi --mode rpc is
persistent and we terminate it via killWithGrace on `agent_end`.
- pi-adapter.test.ts: the test that asserted stdin.end() was called now
asserts stdin.end() is NOT called, with a comment pointing at the
rationale and the verified pi version.
- pi-adapter.e2e.test.ts: matching update — the "B1 invariant" line was
inverted.
Other B-fixes (B2 abort/kill, B3 stream-error try/catch) remain.
Verified:
- npm run typecheck: clean
- npm test: 1953 passed, 2 skipped
- live: `orch run tsk_XscoAUP` with pi 0.73.1 + openrouter writes hello.md,
task transitions todo → in_progress → review → done on attempt 1/3.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
B1 update — live-tested against pi 0.73.1 and reverted. While smoke-running the adapter today (
Root cause: pi-coding-agent 0.73.1 — Pushed
B2 (abort/kill) and B3 (stream-error try/catch) stay — both still correct.
|
Companion to pi-adapter.e2e.test.ts. Covers the UI half of the smoke run:
how a pi agent + a todo task appear, update, and settle inside the
Ink/React TUI as the orchestrator pumps events through the React state.
Walks the rendered output across four phases:
1. initial Tasks tab — todo row with !!! priority indicator and title
2. switch to Agents tab (`a` key) — `pi-bot` row with `pi` adapter chip
and IDLE status
3. dispatch — task flips to in_progress, agent to running, with status
change + agent:started events emitted through onSubscribeEvents
4. live activity — agent:output (pi text_delta payload) and
agent:file_changed (hello.md) appear in the activity feed; agent:completed
plus review→done transitions land the task on done with a ✓
Verified locally — the ink-testing-library frames show the activity feed
exactly as a real user would see it:
▍ now pi-bot ▶ Started task
▏ │ {"text":"Writing hello.md…","raw":{}}
▏ ✎ hello.md
▏ ▶ Completed successfully
now ♦ in_progress → review
✓ Create hello.md via pi Task completed by pi-bot
Bumps the test count badge + CONTRIBUTING quickstart to 1954.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three small wins from a /simplify pass on the PR diff — no behavior change. 1. readPiRpcLines: O(n²) → O(n) The old loop did Buffer.concat([pending, buf]) on every chunk, which copies the whole accumulator each time. Switched to the same chunks[]/totalLen/offset pattern that readLines() in process-manager.ts already uses for the cap-applied path: concat once per chunk arrival, scan with an offset, subarray() the remainder. Algorithmically identical, just without the per-chunk copy. 2. createStderrTailCapture: drop the chunks[] array + shift() Now keeps a single Buffer and on overflow does buf.subarray(buf.length - STDERR_TAIL_BYTES) — zero-copy and O(1). The tail getter also no longer concats per call: it just toString()s the current Buffer, which is already at most STDERR_TAIL_BYTES bytes. 3. Onboarding configs extracted to src/tui/onboarding-config.ts App.tsx no longer has to export ONBOARDING_AGENTS purely for tests. ONBOARDING_GOALS, _TASKS, _AGENTS live in a small module that both App.tsx and the regression test import. Also drops the now-unused `OnboardingConfig` type import from App.tsx. Bonus: pi-tui.e2e.test.tsx now imports makeTask/makeAgent from test/unit/application/helpers.ts instead of duplicating the factories locally (they were drifting from helpers — local makeAgent defaulted to adapter='pi', helpers defaults to 'shell', so the test now explicitly passes adapter: 'pi' through overrides like other adapter tests do). Verified: - npm run typecheck: clean - npm test (full suite): 1954 passed, 2 skipped - Live re-run: pi adapter still writes the file end-to-end through the Orchestrator (tool_execution_end 'Successfully wrote 26 bytes' in the run jsonl — confirms the buffer rewrites didn't break parsing). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Buffer.subarray() returns a view sharing the parent ArrayBuffer. After a
single oversized stderr chunk (e.g. a 64 KB stack trace from the Pi
runtime), the post-overflow assignment
buf = buf.subarray(buf.length - STDERR_TAIL_BYTES)
would pin the entire 64 KB+ backing buffer alive even though only 4 KB
of it is visible — until GC eventually frees it. On a chatty stderr this
is a noticeable per-burst waste.
Wrap the subarray in Buffer.from() so the bytes are copied into a fresh,
exactly STDERR_TAIL_BYTES-sized allocation. The previous Buffer (and its
backing ArrayBuffer) become unreachable immediately.
Caught by `/simplify` efficiency review. No behavior change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
oxgeneral
left a comment
There was a problem hiding this comment.
All blockers from the initial review are addressed, plus a few extras after live-testing:
Blockers
- B1 — initially fixed with
proc.stdin.end(), then reverted inc6274fcafter live testing on pi-coding-agent 0.73.1 revealed thatpi --mode rpcis a persistent server-mode session. Closing stdin stalls the LLM call after user-message_end. Long-lived kill viaprocessManager.killWithGrace(pid, 1000)on thedoneevent (the original design) is correct. Inline reproduction + commit message: c6274fc. - B2 —
finallyblock now scheduleskillWithGrace(pid, 1000)whenever the generator exits without adoneevent (abort or stream error). Test: "kills the process when the abort signal fires before a done event". - B3 — stdout
for awaitis wrapped intry/catch; pre-first-byte stream errors are classified viaclassifyAdapterErrorand yielded as anerrorAgentEvent. Test: "yields an error event when stdout emits an error before any line".
Important (I1, I3, I4, I6, I8) — all applied; see 55e3929 for the consolidated change.
Extras on top
- Parameterized adapter coverage for pi in
agent-factory,commands-init,wizard-effort(0a52296). - Agents-tab onboarding tip now lists all six adapters, with a regression test asserting it stays in sync with
SUPPORTED_ADAPTERS(fa007c7 + 57ad2e3 — extracted tosrc/tui/onboarding-config.ts). - Integration test through real Orchestrator + state machine + JSONL storage (86a18f5).
- Integration test for TUI lifecycle via ink-testing-library (9bc41dc).
- /simplify pass: hot-path O(n²)→O(n) in
readPiRpcLines, O(n) shift → O(1) subarray in stderr capture, leaky-export cleanup (57ad2e3); plusBuffer.from(subarray)to release oversized backing on stderr overflow (6fe17be).
npm test 1954 passed, npm run typecheck clean, CI green. Live smoke against pi 0.73.1 + openrouter wrote the target file end-to-end and transitioned through todo → in_progress → review → done on attempt 1/3.
Thanks @ziahm6638 for the careful work — the line-buffer fix and the RPC stream design were both solid. Merging.
Covers the Pi RPC adapter merge from PR #12 — new features, the review-driven bug fixes (B2 abort/kill, B3 stream errors, I1/I3/I4/I6/I8, B1 revert), the /simplify performance pass on the line reader and the stderr tail buffer, the onboarding-config extraction, and the +260 tests from unit/integration/TUI coverage. Test count moves from 1694 to 1954. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.