Repository navigation
fix(pi): honor settled runs and sum usage [#638] - #641
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.
7 tasks
piekstra
added a commit
that referenced
this pull request
Oct 2, 2026
## Summary Give the Pi process-group test a bounded startup budget for both Node processes. Keep the helper's five-second runtime timeout and the cleanup assertion unchanged, and explicitly verify that the helper belongs to the runner's process group. Capture runner diagnostics on failure, report helper tool errors, and wait for a complete positive PID rather than merely the existence of its file. No production adapter or Pi compatibility flags change. This does not overlap the settlement/accounting changes in #641. ## Evidence - The successful main-CI test took 2.98 seconds against a three-second startup limit. The opt-out test exceeded that limit twice: [main CI](https://github.com/open-cli-collective/codereview-cli/actions/runs/36931796321). - A deterministic 3.1-second runner-only delay reproduces the old assertion failure. The updated test passes three repeated delayed runs. This reproduces the insufficient-budget failure mode, not the exact runner scheduling conditions. - The unchanged Linux test passed ten isolated runs. Updated macOS race tests passed 25 repeats; lint reports zero issues. - The checked-in slow-start and detached-helper preloads are executable controls for the real generated extension, not mock Pi/provider results. - The detached-helper control fails the explicit process-group assertion in 0.30 seconds, confirming that the longer startup budget does not permit a detached helper. - The full normal Linux suite passes with both keyring tags. A separate full Linux race run failed two unchanged Claude subprocess timing assertions (`timeout` stop acknowledgement and stream duration); the focused Pi race repeats pass. Both actual GitHub CI test jobs passed on the initial PR head. Re-run the normal test: ```sh go test -race -tags keyring_nopassage,keyring_no1password ./internal/llmadapters -run '^TestPiRPCReviewerHelperStaysInParentProcessGroup$' -count=25 ``` For the slow-start control, set `NODE_OPTIONS` to `--require` plus the absolute path of `internal/llmadapters/testdata/pi_rpc_slow_start.cjs`; the test should pass. With `pi_rpc_detached_helper.cjs` instead, it must fail the process-group assertion. These controls are test-only and must not be used for normal CLI execution. The locally installed Pi 0.68.1 does not support the required reviewer isolation flags. This PR does not weaken those controls or claim to fix that separate runtime compatibility gap. Linux verification of the generated extension is not a real Pi/provider integration result.
4 of 6 tasks
zzwong
added this pull request to stack #647
October 3, 2026 17:19
This was referenced Oct 8, 2026
…pc-settlement-accounting
monit-reviewer
requested 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: 4370d0cc1589
Profile: open-cli-collective-review - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 1 |
| policies:conventions | 0 |
go:implementation-tests (1 finding)
Major - internal/llmadapters/pi_rpc.go:670
The default branch accepts
stopReason: "length"(and missing or unknown reasons) as a successful final answer. Settlement means automatic work has finished, not that generation completed successfully: a token-limit termination must not become a successful review merely because its text happens to pass downstream JSON validation. Acceptstopexplicitly and reject truncated or unsupported completion reasons. Add settlement-contract cases forlength, missing, and unknown reasons using valid-looking JSON, asserting an error, no structured output, and preserved usage.
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (broad); scope: 2 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 2 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; skipped: none; context (not coverage): 1 file; examples:docs/development.md; constraints: Review was limited to static inspection through read-only repository tools; tests were not executed.policies:conventions— complete (broad); scope: 3 files; examples:README.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 3 files; examples:README.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; skipped: none; context (not coverage): 2 files; examples:docs/checkout-native-review-contract.md,docs/development.md; constraints: Inspected source and regression tests; did not execute tests or verify runtime behavior. Review limited to convention adherence using pinned code and repo-local documentation; shared standards linked in docs/development.md were not available in the provided context.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 59s | $0.04 | 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 |
| Engine | pi_rpc · openai-codex/gpt-6.1-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 59s wall · 1m 09s compute |
| Cost | $0.04 |
| Tokens | 11.6k in / 694 out |
Per-workstream usage
orchestrator-selection— openai-codex/gpt-6.1-sol- In: 5.5k
- Out: 266
- Cache read: 0
- Cache create: 0
- Cost: $0.01
- Duration: 8s
go:implementation-tests— openai-codex/gpt-6.1-sol- In: 1.2k
- Out: 216
- Cache read: 60.7k
- Cache create: 0
- Cost: $0.01
- Duration: 36s
policies:conventions— openai-codex/gpt-6.1-sol- In: 937
- Out: 120
- Cache read: 52.6k
- Cache create: 0
- Cost: $0.01
- Duration: 21s
orchestrator-rollup— openai-codex/gpt-6.1-sol- In: 3.9k
- Out: 92
- Cache read: 0
- Cache create: 0
- Cost: $0.01
- Duration: 4s
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: 9d3a6fa9f236
Profile: open-cli-collective-review - Posting as: monit-reviewer
Summary
| Reviewer | Findings |
|---|---|
| go:implementation-tests | 0 |
| policies:conventions | 0 |
Reviewer Coverage
Complete coverage details are retained in the local coverage.json artifact.
go:implementation-tests— complete (constrained); scope: 2 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 2 files; examples:internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; skipped: none; constraints: Static inspection only; read-only review tools do not permit executing tests.policies:conventions— complete (constrained); scope: 3 files; examples:README.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; body-inspected: 3 files; examples:README.md,internal/llmadapters/pi_rpc.go,internal/llmadapters/pi_rpc_test.go; skipped: none; context (not coverage): 3 files; examples:AGENTS.md,CONTRIBUTING.md,docs/development.md; constraints: Review limited to visible convention guidance and pinned changes; canonical shared standards and external harness principles were not available in the repository context. Tests and runtime behavior were inspected, not executed.
0 PR discussion threads considered. 0 summarized; 0 resolved.
Completed in 36s | $0.12 | 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 |
| Engine | pi_rpc · openai-codex/gpt-6.1-sol |
| Reviewed by | cr · monit-reviewer |
| Duration | 36s wall · 40s compute |
| Cost | $0.12 |
| Tokens | 59.5k in / 241 out |
Per-workstream usage
go:implementation-tests— openai-codex/gpt-6.1-sol- In: 29.0k
- Out: 72
- Cache read: 29.1k
- Cache create: 0
- Cost: $0.06
- Duration: 17s
policies:conventions— openai-codex/gpt-6.1-sol- In: 26.8k
- Out: 112
- Cache read: 0
- Cache create: 0
- Cost: $0.05
- Duration: 19s
orchestrator-rollup— openai-codex/gpt-6.1-sol- In: 3.7k
- Out: 57
- Cache read: 0
- Cache create: 0
- Cost: $0.01
- Duration: 3s
zzwong
marked this pull request as ready for review
October 9, 2026 01:52
zzwong
added a commit
that referenced
this pull request
Oct 9, 2026
Fixes #639. Stacked on #641; merge #641 first. ## Summary Most `pi_rpc` tests use a fake Pi, so they can miss changes in Pi's real contracts. Add credential-free installed-Pi tests and a pinned CI check. No production code changed. - New mock-provider cases use a fresh selected agent directory and `HOME`, offline flags, a dummy key, and loopback. Reviewer calls dispatch the real `__pi-review-tool` helper through `TestMain`. - Cover structured JSON, retry recovery, permanent errors, diff-first/path restrictions, read/search/list, exact usage/cache/cost totals, native-tool rejection, cancellation, and process-group cleanup. - Ordinary tests skip runtime cases when the opt-in is unset. The dedicated target requires the exact Makefile version, currently Pi 1.0.0; missing/wrong runtimes and blank pins fail. - Add non-required `pi-runtime` CI with pinned Node 22.22.3/action revisions and `npm install --ignore-scripts`. A failed version lookup stops before installation. Existing check identities remain unchanged. ## Test plan - [x] New retry/error/accounting tests fail against the pre-#638 adapter and pass here. - [x] `make test-pi-runtime` passes on macOS and in a Linux arm64/root container; five repeated race runs pass. - [x] Missing/wrong runtimes, empty/whitespace pins, and failed CI version lookup fail closed; focused Make guard regression passes. - [x] `GOFLAGS=-tags=keyring_nopassage go test ./...`, Go 1.26.3 lint, `actionlint`, and `git diff --check` pass. - [ ] Confirm GitHub CI, including `pi-runtime` on x64/non-root. - [ ] Re-run the pinned target before future Pi/Node upgrades. ## Limits Evidence is local/mock, not deployed. Process-group assertions are Unix-only; Windows has no runtime coverage. Hostile instructions at the default HOME location and in project resources are tested under a fresh selected agent directory. This does **not** establish production ambient-instruction isolation: selected-agent `AGENTS.md` affects non-reviewers, and `APPEND_SYSTEM.md` affects both modes. Follow-up #642 tracks that fix. Codemode remains separate in #640.
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
Fixes #638.
Pi can retry or recover after
agent_end; onlyagent_settledfinishes the session run. CR stopped at the earlier event, losing successful recovery. It also overwrote usage on each turn.Test plan
-race; independently repeat settlement/retry tests five times.GOFLAGS=-tags=keyring_nopassage go test ./... -count=1successfully. The initial run had two timing failures in unchanged subprocess tests; isolated repeat runs and the full rerun passed.GOTOOLCHAIN=go1.26.3 make lintandgit diff --check.All runtime evidence is local/mock-provider evidence, not a deployed outcome. The host Go 1.27 toolchain exceeds the installed linter's build toolchain; lint passed using Go 1.26.3. Rollback is a revert if completion or cancellation regresses. RPC remains the integration; session persistence and Durable are out of scope.