Skip to content

feat: add Pi RPC adapter#12

Open
ziahm6638 wants to merge 2 commits into
oxgeneral:mainfrom
ziahm6638:feat/pi-rpc-adapter
Open

feat: add Pi RPC adapter#12
ziahm6638 wants to merge 2 commits into
oxgeneral:mainfrom
ziahm6638:feat/pi-rpc-adapter

Conversation

@ziahm6638
Copy link
Copy Markdown

@ziahm6638 ziahm6638 commented Apr 30, 2026

Summary

Adds a native pi adapter that runs the Pi coding agent in headless RPC mode.

  • spawns pi --mode rpc and sends ORCH prompts as JSONL prompt commands
  • maps Pi RPC events into ORCH AgentEvents
  • preserves Pi sessions by default for long-running context and auto-compaction
  • leaves Pi extensions/skills/context enabled by default and ignores UI-only extension events
  • supports config.model via --model and config.effort via --thinking
  • registers pi in the adapter registry, model tiers, TUI wizard, init/agent help, and docs

Verification

  • npm run typecheck
  • npm test — 112 files, 1942 passing, 2 skipped
  • npm run build
  • Manual built-CLI smoke test: initialized temp project with --adapter pi, ran a Pi task, and verified it wrote smoke.txt containing PI_ORCH_ADAPTER_OK

Notes

The adapter intentionally preserves Pi's own harness prompt by passing ORCH's system prompt through --append-system-prompt rather than replacing it. Pi RPC extension_ui_request events are ignored so extensions/tools can stay enabled without polluting ORCH logs.

@ziahm6638 ziahm6638 requested a review from oxgeneral as a code owner April 30, 2026 13:11
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 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!

Copy link
Copy Markdown
Owner

@oxgeneral oxgeneral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 createPiRpcEvents against createStreamingEvents in adapters/utils.ts. The right fix is to extend the shared helper with an afterDone hook so all adapters benefit. I'll do that as a follow-up after this lands.
  • assets/adapters-{dark,light}.svg and landing/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants