feat(core): add secure local Pi app-chat harness - #2501
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Visual recap — skippedThe visual recap job did not run for this pull request. This is informational only and does not block the PR. Recap skipped for |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 5 potential issues 🔴
Review Details
Code Review Summary
PR #2501 adds a public Agent Harness chat execution path, a credential-reusing local Pi adapter, lifecycle persistence/approval handling, and localized documentation. The overall architecture appropriately centralizes adapter execution and preserves the existing hosted/anonymous routing paths, and the focused test coverage is substantial. I classified this as high risk because the change introduces a local credential-bearing runtime and changes authorization, concurrency, and approval lifecycle behavior.
Key findings
- 🔴 HIGH Local Pi can be enabled without a guard, making a credential-bearing unsandboxed runtime reachable through normal chat requests.
- 🔴 HIGH The run-slot check is read-only, so concurrent requests can both pass the check and start turns with side effects.
- 🔴 HIGH Approval continuation can resolve against a retained completed run, losing continuation events and completion persistence.
- 🟡 MEDIUM Error paths can retain live native sessions until the lifecycle sweep.
- 🟡 MEDIUM Narrowing the exported
toolstype is source-breaking for existing consumers despite a minor release.
🧪 Browser testing: Skipped — PR only modifies backend/core runtime and documentation, with no user-facing UI impact.
| ): Promise< | ||
| { allowed: true } | { allowed: false; status: number; error: string } | ||
| > { | ||
| if (!config.guard) return { allowed: true }; |
There was a problem hiding this comment.
🔴 Require an explicit guard for local Pi harness access
guard is optional and this branch treats an omitted guard as allowed. That permits configuring { adapter: "pi:local" } without a loopback/development guard, exposing the credential-bearing local Pi runtime (and its host capabilities) to any request that reaches the handler. Make local/unsandboxed adapters fail closed when no explicit guard is configured, or enforce an equivalent adapter-owned local-host check.
| activeRunId: activeRun.runId, | ||
| }; | ||
| } | ||
| const slot = await tryClaimRunSlot(threadId); |
There was a problem hiding this comment.
🔴 Atomically reserve the run slot before starting a turn
tryClaimRunSlot only checks for an existing running row; it does not reserve one. Two simultaneous requests can both observe no row, pass this check, run onRunPrepared, and start harness sessions, potentially executing duplicate actions. Insert/reserve the run row atomically (or use a database-backed lease/unique claim) before any awaited preparation or harness startup.
| ); | ||
|
|
||
| if (approved && latest?.runId) { | ||
| const activeRun = getActiveRunForThread(effectiveThreadId); |
There was a problem hiding this comment.
🔴 Resume approvals through the run manager after completion
When the initial run has emitted an approval request, it can already be marked completed while retained in the active-run cache. This branch then resolves approval directly and subscribes to the old completed run, so continuation events are not streamed or persisted through onHarnessEvent/onRunComplete. Only treat a genuinely running run as live; otherwise start the approval continuation through startAgentHarnessApprovalRun.
|
|
||
| export function startAgentHarnessRun( | ||
| opts: StartAgentHarnessRunOptions, | ||
| ): ActiveRun { |
There was a problem hiding this comment.
🟡 Release native sessions when a harness turn fails
After registerLiveAgentHarnessSession, exceptions from streamTurn, event handling, session updates, or detach() exit without stopping/destroying and releasing the native session. A failed local Pi turn can therefore retain session/process resources in liveSessions until the 24-hour sweep. Add error cleanup while preserving retention only for the deliberate pending-approval state.
| instructions?: string; | ||
| skills?: unknown[]; | ||
| tools?: Record<string, unknown>; | ||
| tools?: Record<string, AgentHarnessHostTool>; |
There was a problem hiding this comment.
🟡 Preserve compatibility for the public tools option
The exported AgentHarnessCreateSessionOptions.tools field is narrowed from Record<string, unknown> to Record<string, AgentHarnessHostTool>. Existing consumers passing adapter-native tool definitions no longer type-check, making this a source-breaking public API change shipped under a minor release. Keep the old native-tools surface and add a separate typed host-tools field, or provide a compatible union and migration path.
There was a problem hiding this comment.
Builder reviewed your changes and found 3 potential issues 🔴
Review Details
Code Review Summary
This incremental review covers PR #2501's new foreground harness route, scoped session lookup, approval continuation, host-action bridge, and local Pi adapter. The architecture keeps harness execution separate from AgentEngine, scopes persisted sessions by owner/org, avoids copying Pi credentials, and correctly filters read-only tools in plan/read mode. The PR remains high risk because it adds an unauthenticated-route boundary option, local credential reuse, and concurrent approval/run lifecycle behavior.
Key findings
- 🔴 HIGH — Idle approval continuations can race because the approval path starts a run without claiming the durable thread slot.
- 🔴 HIGH — A consumer disconnect can leave the local Pi
prompt()promise running after the stream unsubscribes, allowing work and resource use to continue outside the harness turn. - 🟡 MEDIUM — Requests without a thread ID generate an effective ID for the run but do not pass it to persistence callbacks, so the first user message/thread is not persisted and the client cannot reliably resume it.
The previously reported guard, atomic slot reservation, live approval event forwarding, failure cleanup, and public tools compatibility issues remain open and were not reposted. No stale prior comment was verified as fixed.
🧪 Browser testing: Will run after this review (PR touches UI code)
| activeRunId: activeRun.runId, | ||
| }; | ||
| } | ||
| const run = startAgentHarnessApprovalRun({ |
There was a problem hiding this comment.
🔴 Serialize idle approval continuations before starting their runs
Two concurrent requests for the same idle pending approval can both pass the active-run check and start separate continuation runs because this path never claims the thread slot. Claim the durable slot before startAgentHarnessApprovalRun and return the existing run to the loser, as in the normal turn path.
Additional Info
Found by 2 of 4 parallel reviewers; distinct from the existing approval-resume comment because this is the idle-continuation race at the new start path.
| void this.session.abort(); | ||
| }; | ||
| input.abortSignal?.addEventListener("abort", onAbort, { once: true }); | ||
| void this.session |
There was a problem hiding this comment.
🔴 Await the local Pi prompt before ending the stream
prompt() is launched fire-and-forget, but the generator's finally unsubscribes immediately when its consumer disconnects or exits early. The model/tool turn can therefore continue after the harness run has ended and its events/session cleanup have been detached; retain the promise and await it (or abort and await it) before returning from streamTurn.
Additional Info
Confirmed by one focused lifecycle reviewer; this is separate from the existing runner failure-cleanup comment.
| } | ||
| } | ||
|
|
||
| await options.onRunPrepared?.({ |
There was a problem hiding this comment.
🟡 Persist the generated thread ID for new harness chats
When the client omits threadId, the handler generates effectiveThreadId for the run but passes the original undefined value to onRunPrepared and the completion callback. The plugin persistence callback therefore skips the first thread/message, and the generated session cannot be reliably resumed. Pass effectiveThreadId to those callbacks.
Additional Info
Found by one independent reviewer; verified against the plugin callback contract.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Builder reviewed your changes — no new findings
Review Details
Code Review Summary
This incremental review covers the latest PR head, including the local Pi harness, scoped session storage, approval/run lifecycle, host-tool adapters, dependency updates, and the newly changed database-client test surface. The implementation remains architecturally coherent: Pi owns local model/OAuth configuration, host actions retain scoped execution and fail-closed approval checks, and the harness route remains separated from the existing AgentEngine path. Focused harness tests, typecheck, and build were reported as passing.
Risk: High — this PR introduces local process execution and credential-adjacent session behavior. The eight previously reported findings were checked against the latest code and remain unresolved; they were intentionally not reposted. In particular, the existing slot-reservation concern still covers the threadId/effectiveThreadId mismatch flagged during this review. No additional confirmed high- or medium-severity issues were found by the parallel reviewers.
🧪 Browser testing: Skipped — PR only modifies backend/config/docs, no UI impact
|
thanks @RogierKonings! some comments above worth taking a look at, as well as these:
|
2 participants
Summary
pi:localadapter that reuses Pi-owned model/OAuth configuration without copying credentialsVerification
pnpm --filter @agent-native/core typecheckpnpm --filter @agent-native/core buildKnown broader-suite environment failures
A full local core suite remains blocked by missing Playwright Chrome plus unrelated pre-existing scaffold/provider/Anthropic test failures; all changed-surface suites pass.