Repository navigation
fix(pi): isolate RPC prompts from ambient instructions - #656
Merged
Merged
Conversation
Pi 1.0 RPC can continue after agent_end with automatic retries, compaction recovery, or queued work; agent_settled is the only point where no further automatic work follows. The adapter stopped at agent_end and closed stdin, so a recoverable provider failure returned no answer, and usage was overwritten per event, keeping only the last turn. - Read events through agent_settled and close stdin only after settlement or a terminal failure, draining stdout during shutdown. - Take the answer only from the final completed assistant message; a final error or abort fails with Pi's diagnostic, and earlier turns or failed attempts never stand in for the answer. - Fail truncated or unsettled streams, prompts handled without a run, and runtimes that go quiet after a run boundary without agent_settled (incompatible runtime after a bounded grace period). - Sum usage once per completed assistant message and compaction summary, ignoring cumulative message_update usage and agent_end snapshots. - Document the Pi 1.0 completion contract in the README.
Pi reports usage.cacheWrite1h as the one-hour-retention subset of the cacheWrite aggregate, but the adapter never mapped it, so per-task usage lost the cache duration split that Anthropic-backed models report. Map cacheWrite1h to the one-hour cache-create field and sum it once per completed assistant message alongside the aggregate. Messages that omit it leave it unknown, and no five-minute value is derived because Pi does not label the remainder with a retention.
Most pi_rpc tests use a fake Pi, so they stay green when Pi's real completion and event contracts drift. Add tests that run the adapter against the installed Pi with an isolated agent directory and home, offline mode, and a scripted loopback OpenAI-compatible provider with a dummy key. Reviewer tool calls go through the real __pi-review-tool helper, which TestMain dispatches the way the cr binary does. The tests cover structured JSON without tools, a 503 followed by a successful retry, permanent provider failures, a multi-turn reviewer run with the diff-first gate, a path escape, and exact usage, cache, and cost totals, a denied shell tool, and caller cancellation and task deadlines with process-group cleanup. Hostile global and project instructions, extensions, MCP servers, and skills must not load. Ordinary go test skips them. make test-pi-runtime sets CR_PI_RUNTIME_VERSION from the Makefile pin, so a missing or different Pi fails. A new non-required pi-runtime CI job installs that version with npm --ignore-scripts on pinned Node 22.22.3 and runs the target.
A blank PI_RUNTIME_VERSION made make test-pi-runtime export an empty CR_PI_RUNTIME_VERSION, so every runtime test skipped and the target reported ok. The same value made pi-runtime-version print nothing, which the CI install step would turn into an unpinned, latest Pi install. Both Make targets now require exactly one version word before doing anything. The runtime tests also fail, rather than skip, when the opt-in variable is set but blank. The CI step reads the version in its own assignment so a failed lookup stops the step before npm runs. A focused test runs both targets against a fake go and checks that blank, whitespace, and multi-word pins fail without dispatching go test. The development guide now states that the runtime fixture's isolation does not extend to production agent directories (#642).
zzwong
force-pushed
the
zzwong/issue-639/pi-runtime-smoke
branch
from
October 9, 2026 01:54
e9d463e to
ae1cd95
Compare
# Conflicts: # docs/development.md # internal/llmadapters/pi_rpc.go # internal/llmadapters/pi_rpc_runtime_test.go # internal/llmadapters/pi_rpc_test.go
monit-reviewer
approved these changes
Oct 9, 2026
monit-reviewer
left a comment
monit-reviewer
left a comment
There was a problem hiding this comment.
Automated PR Review
Reviewed commit: 358a968f48a1
Profile: open-cli-collective-review - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
| documentation:docs | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 3 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_runtime_test.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 3 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_runtime_test.go,internal/llmadapters/pi_rpc_test.go; skipped: none; context (not coverage): 2 files; examples:docs/development.md,internal/llmadapters/subprocess.go; constraints: Static inspection only; tests and installed Pi runtime behavior were not executed.policies:conventions— complete (broad); scope: 5 files; examples:README.md,docs/development.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_runtime_test.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 5 files; examples:README.md,docs/development.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_runtime_test.go,internal/llmadapters/pi_rpc_test.go; skipped: none; context (not coverage): 3 files; examples:AGENTS.md,CONTRIBUTING.md,docs/architecture.md; constraints: Canonical shared standards and the external harness principles document were not available in the pinned repository context. Review limited to visible repository conventions and read-only source inspection; tests were not executed.documentation:docs— complete (broad); scope: 2 files; examples:README.md,docs/development.md; body-inspected: 2 files; examples:README.md,docs/development.md; skipped: none; context (not coverage): 7 files; examples:AGENTS.md,CLAUDE.md,Makefile,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_runtime_test.go; 2 omitted; constraints: Documentation-focused static review; inspected implementation and test evidence but did not execute tests or verify external Pi source links.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 55s | $0.09 | openai-codex/gpt-6.1-sol | cr 0.10.328
| Field | Value |
|---|---|
| Model | openai-codex/gpt-6.1-sol |
| Reviewers | go:implementation-tests, policies:conventions, documentation:docs |
| Engine | pi_rpc · openai-codex/gpt-6.1-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 55s wall · 1m 29s compute |
| Cost | $0.09 |
| Tokens | 31.1k in / 787 out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-6.1-sol- In: 6.2k
- Out: 391
- Cache read: 0
- Cache create: 0
- Cost: $0.02
- Duration: 15s
go:implementation-tests— openai-codex/gpt-6.1-sol- In: 923
- Out: 97
- Cache read: 62.8k
- Cache create: 0
- Cost: $0.01
- Duration: 22s
policies:conventions— openai-codex/gpt-6.1-sol- In: 16.8k
- Out: 128
- Cache read: 43.4k
- Cache create: 0
- Cost: $0.04
- Duration: 19s
documentation:docs— openai-codex/gpt-6.1-sol- In: 3.4k
- Out: 119
- Cache read: 45.3k
- Cache create: 0
- Cost: $0.01
- Duration: 24s
orchestrator-rollup— openai-codex/gpt-6.1-sol- In: 3.8k
- Out: 52
- Cache read: 0
- Cache create: 0
- Cost: $0.01
- Duration: 5s
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #642.
--append-system-prompt, suppressing ambient append instructions without adding prompt text.Stacked on #646 at
e9d463ee16231f1a7d9759bf6ef1ddf3de87c844. Required merge order: #641 → #646 → this PR. Refresh and revalidate this stack when either parent changes.Type of Change
Validation
73bf803c51fdb5ee75bf3b0add13ed96d6410818, including the installed-Pipi-runtimerace gate. Its Linux job used Node 22.22.3 / npm 10.9.8 and the ordinary CI installation path with scripted providers.make test-pi-runtimepassed with race detection: eight top-level tests / 24 PASS nodes, zero skips. This covers retry/permanent-error accounting, reviewer read/search/list/diff restrictions, unlisted-tool rejection, cancellation/cleanup, installed extension and preflight.go test -race ... -count=3passed 30 top-level executions / 144 PASS nodes, zero skips, including synthetic selected-store persistence and unsafe argument rejection.go build ./..., tidy with unchanged go.mod/go.sum, lint (zero issues), tracked gofmt andgit diff --checkpassed.go test ./... -count=1failed three inherited pipeline top-level tests / five leaf cases. Literalmake checkexited 2 at test after tidy/fmt/lint passed; its build stage was not reached. Separate build passed. Every failure reproduced on unchanged PR646 with the same existing/tmpancestor-worktree source-trust warning. No markers or security settings were changed.Runtime provenance and limits
All provider execution is local/mock Linux x86_64, Node 24.19.0, npm 11.9.0, Go 1.26.8, Pi 1.0.0 using a verified exact private-consumer fixture. CI's Node 22.22.3 and ordinary npm installation were not reproduced locally; the subsequent hosted run above validated that CI path. Real OAuth refresh, deployed behavior and installed-runtime behavior on other operating systems remain unverified.
The official Pi tarball matched registry SHA512/SHA1. The consumer lock preserves all 146 published production dependency entries, adding only seven independently verified missing SRI values and the verified root tarball. All 122 installed packages / 14,943 regular files matched verified tarball bytes; 25 platform-incompatible optional packages were omitted. Upstream source and manifests were unchanged. All installation phases disabled lifecycle scripts.
The initial ordinary consumer install re-resolved dependency versions and was rejected; direct npm ci on the published root failed because its devDependencies were absent from its production shrinkwrap. Those attempts and all failure records are retained. Successful network commands kept strict TLS verification enabled using only the already installed system trust store. Registry signatures/attestations were not cryptographically verified.
Fixtures used isolated dummy HOME/Pi state, synthetic credentials and loopback scripted providers. No real credentials, real OAuth provider, paid calls or Git-provider writes were used by the tests. The synthetic auth-update test does not establish an actual OAuth refresh result.
Checklist
make checkpassedRefreshed head
358a968f48a12528ac3bbf68ca2b9e9909c92582is CR-approved, and CI run 37873577923 passed all 11 checks. The installed-Pi 1.0.0 race gate and 274 adapter tests also passed locally on this refresh. Earlier local aggregate failures are retained above as historical evidence; no operating-system sandbox guarantee is claimed.Related Issues
Closes #642. Depends on #646, itself stacked on #641.