perf(pi): run subagent sessions in-process - #194
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Pi provider now runs through an embedded, pooled ChangesEmbedded Pi harness runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR replaces Pi's child-process RPC integration with a pooled, in-process AgentSession runtime and updates availability checks, tests, and documentation accordingly.
Confidence Score: 4/5The PR appears safe to merge, though cached Pi sessions should be scoped by workspace to prevent a cross-workspace caller from reusing the wrong session. The in-process runtime is coherently integrated, but its global pool key and session-ID-only cache bypass the workspace validation used when reopening persisted Pi sessions. Files Needing Attention: src/local-agent-pi/runtime.ts
|
| Filename | Overview |
|---|---|
| src/local-agent-pi/runtime.ts | Implements the embedded Pi session lifecycle; cached session reuse does not preserve workspace scoping. |
| src/local-agent-runtime-registry.ts | Routes Pi executions through the shared harness runtime pool. |
| src/local-agent-adapters.ts | Removes the Pi RPC implementation and delegates direct adapter runs to the new harness. |
| src/local-agent-availability.ts | Changes Pi availability detection from an external executable check to package resolution. |
| src/local-agent-pi/runtime.test.ts | Covers sequential session reuse and configuration but not reuse of one session ID across different workspaces. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[LocalAgentRuntimeRegistry.run] --> B[HarnessRuntimePool]
B -->|constant pi/in-process key| C[Shared PiHarnessRuntime]
C --> D{providerSessionId cached?}
D -->|Yes| E[Reuse cached AgentSession]
D -->|No| F[Open or create workspace-scoped session]
E --> G[Prompt retained session]
F --> G
G --> H[Return final assistant message]
Reviews (1): Last reviewed commit: "test(pi): cover session reuse" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
d46ee22 to
5906cd8
Compare
Waishnav
changed the title
5906cd8 to
e7a91ed
Compare
e7a91ed to
8a19605
Compare
8a19605 to
341a195
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/local-agent-pi/runtime.test.ts (1)
67-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the disposal and reopen lifecycle.
The test covers reuse, workspace binding, model, and thinking. It does not cover three behaviors that this PR introduces:
close()disposes every cached session.FakePiSession.disposedis never asserted.run()rejects afterclose().- A
providerSessionIdthat is not in the in-memory cache triggers a newcreateSessioncall with that id. This is the durable recovery path described in the PR objectives.💚 Proposed additional assertions
assert.equal(sessions.get("pi_1")?.selectedModel, "openai/gpt-test"); assert.equal(sessions.get("pi_1")?.thinking, "high"); + + const reopened = await runtime.run({ + workspace: "/tmp/c", + prompt: "reopen", + providerSessionId: "pi_persisted", + }); + assert.equal(reopened.providerSessionId, "pi_persisted"); + assert.equal(created, 3, "an uncached session id reopens the durable session"); } finally { await runtime.close(); } + +assert.ok( + [...sessions.values()].every((session) => session.disposed), + "close should dispose every cached session", +); +assert.equal(runtime.isUsable(), false); +await assert.rejects(runtime.run({ workspace: "/tmp/a", prompt: "after close" }), /closed/);Based on learnings, "Add and maintain behavior and regression tests for affected contracts and lifecycle behavior."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-pi/runtime.test.ts` around lines 67 - 86, Add lifecycle and recovery assertions to the runtime test around the existing run/close flow: verify cached FakePiSession instances are marked disposed after runtime.close(), assert subsequent run() calls reject, and exercise a providerSessionId absent from the in-memory sessions map to confirm createSession is called with that requested id. Keep the existing reuse, workspace, model, and thinking assertions intact.Source: Learnings
src/local-agent-pi/runtime.ts (1)
117-128: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
SessionManager.listcost grows with the session history.
openPiSessionManagerlists every persisted session for the workspace and then scans for one id. Each continuation turn that misses the in-memory cache repeats this scan. Check whether the package exposes a direct open-by-id or path-derivation API and use it instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-pi/runtime.ts` around lines 117 - 128, Update openPiSessionManager to avoid calling SessionManager.list and scanning all sessions on cache misses. Use the package’s direct session-open-by-id or session-path-derivation API, while preserving the existing not-found error behavior and returning the opened session manager for the requested sessionId.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agent-profile-schema.md`:
- Line 77: Remove the `pi` entry from the provider CLI list in the documented
schema and update the corresponding packaged provider list in `SKILL.md` so only
executable-backed providers remain; do not alter the embedded Pi runtime
reference.
In `@src/local-agent-adapters.ts`:
- Around line 236-243: Update the default LocalAgentManager execution path and
runLocalAgentProvider so it obtains runtimes through LocalAgentRuntimeRegistry
instead of creating and closing a new Pi runtime per turn. Preserve pooled
session reuse for direct LocalAgentManager callers, while retaining the existing
runtime cleanup lifecycle where the registry owns it.
In `@src/local-agent-pi/runtime.ts`:
- Around line 25-33: Serialize each logical session’s work in
PiHarnessRuntime.run with a per-session promise chain so setModel,
setThinkingLevel, prompt, and result message reads execute sequentially. Update
ensureSession to store the session-creation promise in sessions before awaiting
createSession, allowing concurrent first turns for the same providerSessionId to
share one in-flight creation; preserve existing cached-session reuse and cleanup
behavior.
- Around line 33-41: In the session flow around session.prompt, record
session.messages.length before prompting, then scope finalPiAssistantMessage and
the returned items array to messages added after that index. Ensure a turn with
no new assistant text produces no stale prior response, while preserving the
existing error handling and session metadata.
- Around line 48-53: Update close() to isolate failures from individual
session.dispose() calls so every session is attempted before the pool is
cleared. Preserve the closed guard and sessions.clear() behavior, while ensuring
one thrown disposal error does not stop iteration; retain or propagate the
disposal error according to the existing runtime error-handling convention.
---
Nitpick comments:
In `@src/local-agent-pi/runtime.test.ts`:
- Around line 67-86: Add lifecycle and recovery assertions to the runtime test
around the existing run/close flow: verify cached FakePiSession instances are
marked disposed after runtime.close(), assert subsequent run() calls reject, and
exercise a providerSessionId absent from the in-memory sessions map to confirm
createSession is called with that requested id. Keep the existing reuse,
workspace, model, and thinking assertions intact.
In `@src/local-agent-pi/runtime.ts`:
- Around line 117-128: Update openPiSessionManager to avoid calling
SessionManager.list and scanning all sessions on cache misses. Use the package’s
direct session-open-by-id or session-path-derivation API, while preserving the
existing not-found error behavior and returning the opened session manager for
the requested sessionId.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 63a2cfc9-2b86-401a-8bfa-6fa65d426d2f
📒 Files selected for processing (9)
docs/agent-profile-schema.mdpackage.jsonsrc/local-agent-adapters.test.tssrc/local-agent-adapters.tssrc/local-agent-availability.test.tssrc/local-agent-availability.tssrc/local-agent-pi/runtime.test.tssrc/local-agent-pi/runtime.tssrc/local-agent-runtime-registry.ts
💤 Files with no reviewable changes (1)
- src/local-agent-adapters.test.ts
341a195 to
df5376a
Compare
df5376a to
98ca3a5
Compare
|
Closing this Sol stack PR in favor of the Luna-based implementation. |
1 participant
Pi is already a Node dependency, but DevSpace still launches a separate Pi RPC subprocess for subagent work. This replaces that process boundary with Pi AgentSession objects inside DevSpace, sharing the expensive model/auth runtime while keeping one logical Pi session per DevSpace agent.\n\nPersisted Pi session files remain the recovery boundary, so an in-memory runtime can be discarded and a later turn can reopen the durable session rather than keeping processes alive indefinitely. Stacked on #193.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests