test(server): replace Pi adapter unit tests with live replay fixtures - #13515
Conversation
| const piVersion = yield* readPiVersion; | ||
| const outputPath = readArgValue("--out") ?? (yield* path.fromFileUrl(variant.transcriptFile)); | ||
| const workspace = yield* Effect.promise(() => | ||
| makeCheckpointWorkspace(`pi-rpc-record-${fixture.name}`), |
There was a problem hiding this comment.
🟡 Medium scripts/record-pi-rpc-replay-fixture.ts:214
Pi records fixtures with workspaceFiles against an empty workspace, so the transcript and variant.assertOutput result do not represent the fixture's configured workspace. Build the fixture input before calling makeCheckpointWorkspace and pass fixtureInput.workspaceFiles when seeding it.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/scripts/record-pi-rpc-replay-fixture.ts around line 214:
Pi records fixtures with `workspaceFiles` against an empty workspace, so the transcript and `variant.assertOutput` result do not represent the fixture's configured workspace. Build the fixture input before calling `makeCheckpointWorkspace` and pass `fixtureInput.workspaceFiles` when seeding it.
| const entry = this.transcript.entries[this.cursor]; | ||
| if (entry?.type !== "emit_inbound") return emitted; |
There was a problem hiding this comment.
🟡 Medium Adapters/PiAdapterV2.testkit.ts:309
Pi replays with a runtime_exit entry always fail with PiReplayIncompleteError, even after all process I/O has been consumed. drainInbound only advances over emit_inbound, so the terminal record remains at the cursor; consume runtime_exit as a terminal transcript record as well.
const entry = this.transcript.entries[this.cursor];
+ if (entry?.type === "runtime_exit") {
+ this.cursor += 1;
+ emitted = true;
+ continue;
+ }
if (entry?.type !== "emit_inbound") return emitted;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/orchestration-v2/Adapters/PiAdapterV2.testkit.ts around lines 309-310:
Pi replays with a `runtime_exit` entry always fail with `PiReplayIncompleteError`, even after all process I/O has been consumed. `drainInbound` only advances over `emit_inbound`, so the terminal record remains at the cursor; consume `runtime_exit` as a terminal transcript record as well.
| modelSlug: variant.modelSelection.model, | ||
| }), | ||
| } satisfies ProviderReplayTranscript; | ||
| yield* fs.makeDirectory(path.dirname(outputPath), { recursive: true }); |
There was a problem hiding this comment.
🟡 Medium scripts/record-pi-rpc-replay-fixture.ts:290
The recorder writes the transcript before variant.assertOutput, so a failed live assertion replaces the existing fixture or --out target with an unverified recording. Run the assertion before writing the file.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/scripts/record-pi-rpc-replay-fixture.ts around line 290:
The recorder writes the transcript before `variant.assertOutput`, so a failed live assertion replaces the existing fixture or `--out` target with an unverified recording. Run the assertion before writing the file.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — The PR is confined to Pi replay/testing infrastructure and does not alter production behavior or product defaults. Human review is still warranted because replay mismatch errors retain complete RPC frames, which can expose arbitrary command arguments or output through test diagnostics. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
| import { provideDeterministicTestRuntime } from "../src/orchestration-v2/testkit/DeterministicRuntime.ts"; | ||
| import { ORCHESTRATOR_REPLAY_FIXTURES } from "../src/orchestration-v2/testkit/fixtures/index.ts"; | ||
| import { materializeFixtureInput } from "../src/orchestration-v2/testkit/fixtures/shared.ts"; | ||
| import { runOrchestratorV2ProviderReplayScenario } from "../src/orchestration-v2/testkit/ProviderReplayHarness.ts"; |
There was a problem hiding this comment.
This aliases a service module's layer at a service boundary, hiding the module's public shape. Please import * as IdAllocator and use IdAllocator.layer at the Effect.provide call.
Posted via Macroscope — Effect Service Conventions
| import * as Cause from "effect/Cause"; | ||
| import * as Effect from "effect/Effect"; | ||
| import * as Layer from "effect/Layer"; | ||
| import * as Queue from "effect/Queue"; |
There was a problem hiding this comment.
Please keep the service module namespace here: import * as IdAllocator and provide IdAllocator.layer instead of renaming its layer export.
Posted via Macroscope — Effect Service Conventions
|
|
||
| class PiReplayIncompleteError extends Schema.TaggedError<PiReplayIncompleteError>()( |
There was a problem hiding this comment.
These error attributes store whole RPC frames, including arbitrary command arguments and output; message also stringifies them. Please retain only bounded, safe diagnostics (for example frame type and cursor), and keep raw failure data out of error attributes and messages. This needs changes to the error definition and construction, so there is no single-hunk fix.
Posted via Macroscope — Effect Service Conventions
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: unavailable · PR result: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
6448cfe to
1563a64
Compare
87e65c3 to
264e917
Compare
| return `Pi replay ended with ${this.remaining} unconsumed entries at cursor ${this.cursor} in scenario ${this.scenario}. Next: ${JSON.stringify(this.next)}.`; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
next stores an unconsumed RPC frame (which may contain command arguments or output) and the error message stringifies it. Please replace it with bounded diagnostics such as the entry type and cursor; update both the error definition and its construction in assertComplete().
Posted via Macroscope — Effect Service Conventions
|
Macroscope skipped reviewing this pull request. Per-review cost limit exceeded (workspace setting). This review would cost an estimated $19.63, which exceeds your per-review limit of $15.00. The top 3 files driving up this estimate:
Tip To get this pull request reviewed, you can:
|
Pi had no replay fixtures: PiAdapterV2 was only exercised by a hand-written fake process whose frames were never observed from a real Pi. The testkit swaps the ChildProcessSpawner PiAdapterV2 spawns `pi --mode rpc` with for one that answers from a transcript of stdio records, so the real adapter and PiRpc framing run unchanged. Recorded request ids are rebound to the ids the replaying adapter sends, and writes from independent adapter fibers may arrive in either order without loosening what must match. The recorder runs a registered fixture's input through the real orchestrator and PiAdapterV2 against a live pi, pinned to openrouter/deepseek-v4-flash with the user's extensions, skills and context files disabled, and normalizes session files, UUIDs, timestamps, the home directory and system-prompt text. simple and multi_turn are recorded live on Pi 0.87.1. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…paction fixtures Five more live Pi 0.87.1 recordings on openrouter/deepseek-v4-flash, each with a Pi-specific output assertion: - message_steering: the steer is a `prompt` with streamingBehavior "steer", Pi queues it into the running loop, and the run keeps one provider turn. - turn_interrupt_mid_tool: Stop aborts a running bash tool, Pi ends it as an error, the item projects as interrupted and the session as stopped. - provider_thread_resume: past the 30-minute idle release, a fresh Pi process switch_sessions to the recorded file and still knows turn one. - thread_rollback: rollback forks the session tree at the discarded turn's user entry and the next turn runs on the forked session file. - pi_compaction: `/compact <instructions>` that Pi refuses as too small, then a bare `/compact` that really summarizes, then a recall turn. The recorder writes a tiny compaction.keepRecentTokens into the throwaway workspace's .pi/settings.json (and trusts it with --approve) so a short fixture conversation is compactable. simple and multi_turn now also check that each settled turn carries its own get_session_stats usage. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Each deleted test drove a hand-written fake Pi through behaviour a live
recording now covers end to end through the orchestrator. Mutating the
adapter path each one tested fails the covering fixture:
- streams assistant text and settles on agent_settled -> simple (live usage,
reasoning, settled get_session_stats usage), multi_turn
- captures session-tree refs and rolls back via fork -> thread_rollback
- sends RPC compact for /compact, compacts a bare /compact through
compactThread, keeps a too-small compact as failed -> pi_compaction
- stops with restart by aborting then terminating -> turn_interrupt_mid_tool
- steers through an atomic prompt; the steer half of "steers the active
turn" -> message_steering
- registers from get_state and resumes via switch_session ->
provider_thread_resume; its rejects-resume-while-active check stays
The late prompt rejection from a settled slash-command turn is a race a live
Pi cannot produce on demand, so it stays as its own test.
makeFakePi now answers get_state with Pi 0.87.1's recorded idle shape
(steeringMode, followUpMode, messageCount, pendingMessageCount, UUID
sessionId), fork with Pi's {text, cancelled}, and failed compactions omit
`result` as Pi does. The no-session-file case sets sessionFile undefined,
which is what a --no-session Pi reports.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Review follow-ups on the Pi replay harness: - Only the adapter's own `t3-N` request ids are rebound during replay. Any other id, such as the Pi request id an extension_ui_response must echo, now has to match exactly, so answering the wrong dialog fails. - A write may still arrive ahead of its recorded position when independent adapter fibers race, but no longer past the next prompt, compact or process start, so a request sent a turn early fails. - simple compares the reasoning item with the recorded thinking block instead of a phrase the model happened to think. PiAdapterV2.testkit.test.ts pins both matching rules. It fails with the old any-id rebinding and with the unbounded look-ahead. Also fixes the typecheck and lint errors the fixtures commit introduced: the typed get_state override queue, RunId-typed assertions, Schema JSON encoding in the recorder, unused exports and an unused import. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- The recorder seeds the fixture's workspaceFiles into the recording workspace, like replay does. - The recorder replays the normalized transcript and runs the fixture's assertions before writing it, so a failing live recording never replaces an existing fixture or --out target. - Replay consumes a trailing runtime_exit as Pi exiting on its own, closing the newest process's stdout, instead of reporting it unconsumed. - Replay error messages carry the cursor, entry label and frame type rather than stringified frames, matching the other providers' replay errors. - IdAllocator is imported as a namespace. - Removes PiAdapterV2.testkit.test.ts. The recorded fixtures exercise the harness's id rebinding and look-ahead bound; multi_turn fails when the adapter sends its second prompt early. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
a0878b5 to
09db321
Compare
c7e472f
into
t3code/codex-turn-mapping
…#13515) Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Pi had no replay fixtures, no replay harness and no recorder.
PiAdapterV2was tested only against a hand-written fakepiprocess, and some of the fake's frames don't match what Pi actually sends. This PR adds a replay harness and a live recorder, records seven Pi scenarios through the whole orchestrator, and deletes the unit tests those recordings now cover.Harness and recorder
PiAdapterV2.testkit.tsreplays a transcript through theChildProcessSpawnerthatPiAdapterV2uses to spawnpi --mode rpc, so the real adapter andPiRpcframing run unchanged.expect_outboundfor what the adapter writes to stdin,emit_inboundfor what Pi writes to stdout, and a syntheticprocess_startcarrying the spawn argv. Records from a second process carry an@pNlabel suffix.t3-Nrequest ids are rebound. Any other id, such as the Pi request id anextension_ui_responsemust echo, has to match exactly.prompt,compactor process start. The recorded fixtures exercise this:multi_turnfails when the adapter sends its second prompt early.runtime_exitis consumed as Pi exiting on its own. Mismatch and incomplete errors report the cursor, entry label and frame type, not whole frames, like the other providers' replay errors.scripts/record-pi-rpc-replay-fixture.ts(vp run record:pi-replay -- --scenario <name>) runs a registered fixture's own input through the real orchestrator and realPiAdapterV2against a livepi.openrouter/deepseek/deepseek-v4-flash.~/.pisession store is never touched.compaction.keepRecentTokens, so/compacthas something to summarize, plus anyworkspaceFilesthe fixture declares.Fixtures (all recorded live on Pi 0.87.1)
simpleget_session_statsusage. The turn's native ref is the strong session-tree id of its user message.multi_turnget_session_statsusage.message_steeringprompt+streamingBehavior: "steer"with no abort, and the run keeps one provider turn.turn_interrupt_mid_toolstoppedwith no error.provider_thread_resume(new Pi-only registration)switch_sessions to the recorded file and the model still knows turn one.thread_rollbackpi_compaction(new)/compact <instructions>goes out as RPCcompactwith custom instructions. Pi refuses it as too small, which shows as a failed row. A bare/compactreally summarizes and carries Pi's summary and token counts. The meter keeps the post-compaction estimate, and the next turn recalls the marker.Unit tests deleted (
PiAdapterV2.test.ts: 56 → 49 tests, +39 / −495 lines)To prove each fixture covers what its deleted test covered, I broke the adapter path that test exercised and checked the covering fixture fails. All 8 mutations failed their fixture.
agent_settledsimple,multi_turntokenUsagethread_rollback,simplenativeTurnRef/compactpi_compaction/compactas a prompt/compactthrough compactThreadpi_compactionpi_compactionturn_interrupt_mid_toolmessage_steeringstreamingBehaviormessage_steeringget_stateand resumes viaswitch_sessionprovider_thread_resumenew_sessionTwo narrow tests replace parts of the deleted ones that a live Pi can't produce on demand: rejects a resume while a turn is active and keeps a settled turn's late prompt rejection off the next turn.
Kept: lifecycle, race and failure tests (slow/failed/vetoed lifecycle requests, retry and extension-error paths, settle-probe races, detached compaction, extension UI dialogs, MCP injection, unsolicited activity), the native-fork tests (no Pi fork fixture yet), the snapshot tests (orchestration never calls
readThreadSnapshot, so no fixture reaches them), and thePiRpcframing/early-exit tests.makeFakePishapes now come from the recordings.get_statereturns Pi's recorded idle shape (steeringMode,followUpMode,messageCount,pendingMessageCount, UUIDsessionId).forkreturns{text, cancelled}. Failed compactions omitresultinstead of sendingnull, which matches both the docs and Pi's source. The "nonpersistent session" test now setssessionFile: undefined, which is what a--no-sessionPi reports.Net: +2362 / −496 across 23 files, of which +822 are recorded transcripts.
Findings
PiAdapterV2handled every recorded frame correctly, including DeepSeek's thinking deltas, Pi'squeue_updatesteering acks,compaction_endwithoutresult, andcontextUsage.tokens: nullright after a compaction.get_statedefaultthinkingLevelfor this model ishigh. A short conversation is never compactable under the defaultkeepRecentTokens(20k), so the recorder shrinks it in the throwaway workspace only.Verification
vp test run src/orchestration-v2/testkit/OrchestratorReplayFixtures.integration.test.ts -t "/pi ": 7 passed. The full file passed 82/82 once, and the Pi subset was run repeatedly with no flakes.vp test run PiAdapterV2.test.ts OrchestratorReplayFixtures.contract.test.ts: 57 passed.promptthat failsmulti_turn. For the review fixes: a trailingruntime_exitreplays green with the fix and fails as unconsumed without it; a tampered frame fails with a bounded mismatch message; a live re-record ofsimplepasses the replay-then-assert path; a forced assertion failure leaves the--outtarget untouched.vp exec tsc --noEmit -p .in apps/server: noerror TSorwarning TS.vp run knip:check: clean.vp linton touched files: clean apart from an existingno-unused-varsinfixtures/shared.ts.sk-or-, bearer or auth header, and no home path.Not run: repo-wide checks.
Model: Claude Opus 5.5 (Claude Code)
🤖 Generated with Claude Code