Skip to content

feat(pi): add Pi harness (--type pi) with docker + skill injection - #159

Open
CarlesUIPath wants to merge 23 commits into
mainfrom
feat/pi-harness
Open

feat(pi): add Pi harness (--type pi) with docker + skill injection#159
CarlesUIPath wants to merge 23 commits into
mainfrom
feat/pi-harness

Conversation

@CarlesUIPath

Copy link
Copy Markdown
Collaborator

Pi harness (--type pi)

Adds the Pi coding agent (pi, from pi.dev) as a first-class coder-eval harness, on par with claude-code / codex / antigravity / opencode. Registers via the agent SPI (agent.type: pi), streams the standardized event protocol, and runs under both the tempdir and docker drivers.

What's included

  • Agent (agents/pi_agent.py): pi -p --mode json nd-JSON reducer → TurnRecord; native multi-turn loop per communicate() (turn_start/turn_end), max_turns on the visible-turn boundary, internal-retry folding (agent_end.willRetry → finalize at agent_settled/EOF), --append-system-prompt, --session-dir continuity.
  • Skill injection: agent.pluginspi --skill <dir> via the shared _plugin_skill_dirs resolver (also used by OpenCode); pi_skill_paths recorded in environment_info.
  • Docker: pinned PI_VERSION baked into docker/Dockerfile; OPENROUTER_API_KEY forwarded through DockerDriverConfig.env_passthrough--driver docker --type pi works.
  • Robustness: dangling turn_start closed on the next turn; terminal provider error (stopReason=error) surfaced in the result and reset on recovery; warn-once on token-accounting drift instead of silently zeroing tokens/cost.
  • evalboard: Pi logo + label in the harness view; per-row cost apportionment for open-weight (stream-native cost) harnesses.
  • Docs: docs/agents/PI.md + harness-parity table updates; tests for the reducer, skill injection, and the docker pin.

Harness-parity notes

system_prompt is enforced (--append-system-prompt). Not enforced (warned + ignored, documented): permission_mode (sandbox driver is the boundary), allowed_tools/disallowed_tools (Pi's built-in tool names are lowercase and can't map to the Claude-namespaced config default — forwarding would strip all tools), system_prompt_file, plugins non-skill assets.

Benchmark: Pi (Kimi-K3, DeepSeek-V4-Pro) vs Terra & Sonnet-5 nightlies

30 tasks that ran in both nightlies and use deterministic graders (no judge / no live-tenant), 20 categories, 2 seeds per column (Pi = 2 run seeds; Terra/Sonnet = 2 most-recent nightly observations per task). Full write-up: Confluence.

metric Terra Sonnet-5 Kimi-K3 DeepSeek-V4-Pro
success rate 93% 97% 90% 90%
mean score 0.965 0.973 0.952 0.954
mean wall-clock (s) 90.5 191.3 120.8 173.7
mean input tok 437K 2.33M 633K 559K
mean output tok 3,388 13,965 7,533 8,463
mean cost/task (USD) 0.222 1.138 0.305 0.078
total cost, 30 tasks (USD) 6.66 34.15 9.15 2.34
mean turns 1.0 39.7 12.2 11.8

Takeaways: quality is a tight ~2-point band (Sonnet 0.973 ≥ Terra 0.965 ≥ DeepSeek 0.954 ≈ Kimi 0.952); DeepSeek-V4-Pro is the value winner at $0.078/task (~14.6× cheaper than Sonnet); Sonnet's high token/cost is mostly the multi-turn harness accounting (39.7 turns, cache reads) vs Terra's single visible turn — not raw waste.

Test plan

  • make format / make check / make typecheck — clean.
  • pytest tests/test_pi_agent.py tests/test_opencode_agent.py — 202 passed.
  • make lint (CE rules) — 389 passed.
  • Live: Kimi + DeepSeek 2-seed docker runs over 30 tasks (above), 0 _shared/harness errors, skills loaded via --skill.

🤖 Generated with Claude Code

CarlesUIPath and others added 15 commits September 4, 2026 11:46
Add the Pi harness's config surface: AgentKind.PI enum member, a 7-value
PiThinkingLevel literal (strict superset of ThinkingLevel), PiAgentConfig
in the AgentConfig discriminated union, and the models/__init__ export.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drive the pi Node CLI in JSON print mode over a subprocess, reducing its
nd-JSON stream into the standardized event protocol + TurnRecord via
EventCollector (mirrors OpenCodeAgent). Native turn cap off turn_start,
cooperative should_stop, turn_timeout, crash/timeout pending_turn capture,
per-agent --session-dir/--session-id continuity for simulation, and enforced
allowed_tools/disallowed_tools/system_prompt. Registered via register_builtins;
pi = [] extra added. Offline replay tests seeded from a byte-real 0.84.4 capture.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Mirror tasks/opencode_smoke_test.yaml for the Pi harness: type pi, the
spike-verified kimi-k3 model, three criteria (file_exists / file_contains /
run_command), and no smoke-pass tag (run locally — CI E2E lacks the pi CLI +
OpenRouter creds). A loader test asserts it resolves to PiAgentConfig.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New docs/agents/PI.md (mirrors OPENCODE.md), a filled pi column + divergence
prose in HARNESS_PARITY.md, mkdocs nav + blurb (regenerated index tables), and
every prose enumeration of the harness set (README, index, USER_GUIDE,
EXTENDING, llms.txt, CLAUDE.md) extended to include Pi. Fix the stale
agent.type comment in experiments/default.yaml.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Harden PiAgent.start() to drop any prior session tempdir before creating a
  new one, so re-starting the same agent instance cannot leak (defensive; agents
  are single-use in-tree). Add a regression test.
- Correct the PI.md Overview snippet to use --model (matching the code) instead
  of -m.
- Defer a CExxx harness candidate: dead _*TurnState accumulator detection.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The shared config default (experiments/default.yaml) sets Claude-namespaced tool
names (Bash/Read/Write/Edit/Glob/Grep/Skill). Pi's built-in tools are lowercase
and differently named (bash/read/write/edit/grep/find/ls), so forwarding the
PascalCase names to `--tools` allowlisted nonexistent tools and left the agent
with ZERO tools ("I don't have tool access") — the end-to-end smoke created no
files and scored 0/3.

Treat allowed_tools/disallowed_tools as unenforced (add to
_UNSUPPORTED_CONFIG_FIELDS, warn at start()), matching OpenCode/Codex/Antigravity,
so Pi runs with its full native toolset. system_prompt stays enforced
(--append-system-prompt). Updates the two encoding tests and the PI.md /
HARNESS_PARITY.md enforcement claims.

E2E smoke now passes 3/3 (score 1.000).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code review (multi-model) found the tool-forwarding removal (c79f5cb) left
three doc surfaces claiming allowed_tools/disallowed_tools are enforced:
- H1: PiAgentConfig docstring (models/agent_config.py)
- H2: CLAUDE.md harness-parity bullet ("ENFORCES all three")
- M1: pi_agent.py module-docstring command synopsis (--tools/--exclude-tools)
All corrected: only system_prompt is enforced; the tool allowlists are ignored
(Claude-namespaced default can't map to Pi's lowercase toolset), matching the
shipped code + PI.md + HARNESS_PARITY.md.

Also:
- M2: fix _resolve_cost docstring to match the code (rate-card fallback on a
  streamed $0 for a priced model is a deliberate conservative choice — never
  understate max_usd budget gates); demote the expected warning to debug.
- L1: correct the session-dir comment (removed in stop(), NOT kill()).
- L2: tighten test_should_stop_ends_turn_cleanly to pin the cut at turn 0.

No harness correctness bugs were found by the review. Tests 75/75, make lint
389/389, e2e smoke 3/3 (score 1.000).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a HARNESS_LOGO entry for agent.type "pi" (AgentKind.PI): the Pi mark
(public/harness/pi.png, 128x128, rasterized from pi.dev's favicon.svg) plus
short label "Pi" and tooltip "Pi · pi.dev". The badge + selector are already
data-driven, so a Pi run now renders the logo and name instead of the raw id.
Adds harness-badge test cases for the "pi" short label and alt text.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-message Cost column priced each row purely from the rate card
(messageCostUsd = per-message tokens x list rate). A harness that owns its
provider and reports a REAL per-turn total_cost_usd on a model absent from the
rate card (Pi on open-weight models like kimi-k3 / deepseek) priced every row
null, so the column dashed out ("—") even though the turn had a real bill —
while codex (gpt-5.6-terra, in the card) showed numbers.

parseMessages now apportions a turn's real token_usage.total_cost_usd across its
rows by token share whenever the rate card priced nothing for that turn. The
rows then show real numbers that sum EXACTLY to the turn total (last row absorbs
the rounding residual), and so to the task total. Priced harnesses
(Claude/Codex/Bedrock) already have per-row figures and are untouched — the pass
only fills the gap, never overrides. Display-only: the authoritative task total
still reads the backend aggregate, so no double-count.

Verified on a real Pi run: 3 rows that were "—" now read $0.002915/$0.003074/
$0.003325, summing to exactly the $0.0093141 turn total. Adds 3 parseMessages
tests and updates the per-message cost tooltip.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add ARG PI_VERSION=0.84.4 + `npm install -g @earendil-works/pi-coding-agent`
after the claude-code layer, mirroring the CLAUDE_CODE_VERSION pinned-CLI
pattern (Node 22 + npm already present). Correct the lines 61-66 comment so
Pi is named as baked and OpenCode remains the sole not-baked example.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add OPENROUTER_API_KEY to DockerDriverConfig.env_passthrough default so a
`--driver docker --type pi` run reaches OpenRouter without a per-task
env_passthrough_extra, mirroring the CODEX_API_KEY / GEMINI_API_KEY
precedent for the other baked agents. Add a membership test that reads the
model default rather than a hardcoded list.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rewrite PI.md "Running in Docker" (Pi is supported under --driver docker;
baked CLI + OPENROUTER_API_KEY passthrough; docker recommended for
untrusted/adversarial runs since permission_mode is unenforced). Drop the
now-false "docker sandbox driver is unsupported" limitation bullet. Update
the pyproject.toml `pi` extra comment to note the CLI is baked into the
docker image.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add test_pi_cli_baked_and_pinned: assert docker/Dockerfile carries an
`ARG PI_VERSION=<exact>` (never `latest`) and an
`npm install -g @earendil-works/pi-coding-agent@${PI_VERSION}` line that
references the ARG, so the pin and the install cannot drift. Mirrors the
existing claude-code pinned-CLI guard; scoped to the main Dockerfile only
(Dockerfile.runtime deliberately does not bake Pi).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…hain docs

Code-review follow-ups (both Low, no correctness bugs found):
- The Dockerfile comment (and OPENCODE.md) said the OPENROUTER_API_KEY allowlist
  is on `SandboxConfig.env_passthrough`; the field is actually on
  `DockerDriverConfig.env_passthrough` (SandboxConfig has no such field). A
  maintainer grepping SandboxConfig would find nothing. Corrected both.
- docs/DOCKER_ISOLATION.md and docs/tutorials/06 listed the baked toolchain but
  omitted the now-baked Pi CLI (and the codex/antigravity/litellm SDKs). Made
  both lists accurate.

Comment/docs-only; the built image and the pinned Pi layer are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…andling

- Resolve agent.plugins -> skills dirs via the shared _plugin_skill_dirs
  resolver (now harness-labeled) and forward each as `pi --skill <dir>`;
  drop `plugins` from the unsupported set. Records pi_skill_paths in
  environment_info so a report can confirm the skill under test reached
  the agent.
- Close a dangling TurnStartEvent when a new turn_start arrives without a
  prior turn_end (the willRetry mid-turn abort), preserving the
  one-pair-per-inner-turn contract renderers depend on.
- Surface a terminal provider error (stopReason=error) in the result, and
  reset it on a recovered turn so an intermediate retry error never leaks.
- Warn once per turn on token-accounting drift (bad bucket type / missing
  usage object) instead of silently zeroing the turn's tokens and cost.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@CarlesUIPath CarlesUIPath changed the title feat: add Pi harness (--type pi) with docker + skill injection feat(pi): add Pi harness (--type pi) with docker + skill injection Sep 8, 2026
CarlesUIPath and others added 3 commits September 8, 2026 13:29
The captured session event embedded the author's machine-specific cwd
(UID + scratchpad path). The harness never reads session.cwd, so this is
inert, but it should not ship in a public fixture — replace with /work.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	README.md
#	docs/index.md
#	docs/llms.txt
…d the real CLI

test_skill_paths_recorded_in_environment_info declared the patch_exec
fixture but never invoked it, so start() hit the real `pi`-on-PATH check
and failed on CI runners (Quality Gate + Windows Smoke) that have no pi
binary. Call patch_exec() like the sibling skill-injection tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: coder_eval — pr:159

Scope: pr:159 · branch feat/pi-harness (PR #159) · 24a006a · 2026-09-08T16:27Z · workflow variant

Change class: complex — introduces a whole new agent harness (--type pi): a 1182-line streaming/turn state machine, new Pydantic config surface, docker image plumbing and evalboard wiring; correctness requires reasoning about the event protocol, turn lifecycle and parity contracts

Architecture (10/10) and security (9.8/10) are solid and the new Pi harness is structurally faithful to its OpenCode sibling, but risk is concentrated in that new agent: an unsanitized task_id in --session-id makes every dataset-row Pi task die before doing work, a terminal provider error is booked as a clean turn (FAILURE instead of ERROR), a missing findGlob mapping zeroes canonical tool criteria, and Test Health (6.4/10) is too thin to catch any of them — so this is mergeable only after the score-changing Pi defects and the ported teardown/cost tests land.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 8.4 / 10 0 1 1 1 The nd-JSON-CLI agent scaffold is duplicated into pi_agent.py instead of hoisted into a shared base (24-26 co-named near-identical methods, plus a cross-agent import of OpenCode's private _plugin_skill_dirs)
2. Type Safety 8.8 / 10 0 1 0 2 CE047's substring roster guard is vacuous for the 2-char name "Pi", so multiple onboarding surfaces (comparison.md, pages-stub, pyproject description/keywords, README Quick Start table + non-goals, docs/index.md) ship a stale four-harness roster undetected
3. Test Health 6.4 / 10 0 2 3 1 No live-CLI process double: the fake process exits the instant it is awaited, so orphan reap/kill, CancelledError handling, SIGKILL escalation, post-EOF reap, leaked-pipe drain and the loop-top deadline cut are all untested
4. Security 9.8 / 10 0 0 0 2 New third-party npm CLI baked into the shared docker image is outside every dependency-audit gate
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 8 / 10 0 2 0 0 A terminal Pi provider error (stopReason: "error") is scored as a clean COMPLETED turn instead of crashing, so infrastructure failures are booked as agent failures
7. API Surface & Maintainability 8.4 / 10 0 1 1 1 Pi's plugins -> --skill injection is live in code but documented as absent in five places across four surfaces (PI.md, HARNESS_PARITY.md, CLAUDE.md, PiAgentConfig docstring)
8. Evaluation Harness Quality 8.4 / 10 0 1 1 1 Unsanitized task_id is interpolated into pi's path-shaped --session-id, so every dataset-row task (id contains '/') fails before any work

Overall Score: 8.5 / 10 · Weakest Axis: Test Health at 6.4 / 10
Totals: 🔴 0 · 🟠 8 · 🟡 6 · 🔵 8 across 8 axes.

Blockers

  1. [Axis 1] The nd-JSON-CLI agent scaffold is duplicated into pi_agent.py instead of hoisted into a shared base (24-26 co-named near-identical methods, plus a cross-agent import of OpenCode's private _plugin_skill_dirs) (src/coder_eval/agents/pi_agent.py:888) — Hoist the shared nd-JSON-CLI driver into one place (e.g. agents/_cli_stream.py or an NdJsonCliAgent base holding communicate's spawn/read/settle loop, kill/kill_sync/_sweep_process_groups/_reap_orphaned_cli, _crash_turn/_timeout_turn, _warn_token_shape, _rate_card_cost, _close_tool, _RESULT_STATUS, _TERM_GRACE_SECONDS, _DRAIN_SECONDS, _MAX_UNRECOGNIZED_TYPES), leaving each agent only its event grammar and token semantics. Measured: 420 of 780 non-comment code lines in pi_agent.py sit in 23 same-named methods that are >=60% line-similar to opencode_agent.py's, 11 of them byte-identical after stripping comments — communicate (pi:888 vs opencode:1133) is 0.98 similar over 117 code lines, and kill/kill_sync/_sweep_process_groups (pi:772-805) are a verbatim copy of opencode_agent.py:994-1023. No other agent pair in the roster comes close (largest existing overlap in >=8-line identical blocks: codex<->antigravity at 84 lines; pi<->opencode is 457). The PR itself records the decision: pi_agent.py:14-15 "The design mirrors :mod:coder_eval.agents.opencode_agent almost verbatim" and pi_agent.py:119-121 "Genuinely OpenCode-module-private (not exported), so re-declared here at the same value rather than imported (a shared-module hoist is out of scope)" — while pi_agent.py:78 nevertheless does from coder_eval.agents.opencode_agent import _plugin_skill_dirs, so the module already reaches into OpenCode's privates; the hoist is the consistent choice, not an extra one.
  2. [Axis 2] CE047's substring roster guard is vacuous for the 2-char name "Pi", so multiple onboarding surfaces (comparison.md, pages-stub, pyproject description/keywords, README Quick Start table + non-goals, docs/index.md) ship a stale four-harness roster undetected (tests/lint/agent_roster_parity.py:57) — missing_agents_in matches a display name by case-insensitive SUBSTRING (line 102: if not any(name.lower() in haystack for name in AGENT_DISPLAY_NAMES.get(kind, (kind,)))), and the new entry at line 57 is the two-character token "Pi". Every English/marketing surface contains that bigram, so the pi row can never fail. I replayed the exact rule over all seven ROSTER_SURFACES at PR HEAD: CE047 reports missing: [] for every one, yet re.search(r"\bPi\b", region) is False for three of them — pyproject.toml (satisfied by "anthropic" in keywords; its description still reads "(Claude Code, Codex, Gemini/Antigravity, OpenCode)" with no Pi and there is no pi keyword), docs/comparison.md (satisfied by "ci-pipeline.md"; lines 12-13, 28 and 126 list four harnesses), and .github/pages-stub/index.html (satisfied by "copies"; lines 15 and 230 list four harnesses). That is verbatim the defect the rule's own docstring says it exists to prevent — "exactly how OpenCode shipped while being absent from most of them" (lines 6-11) — reproduced for Pi with the gate green. Fix in two parts: (1) anchor the match on word boundaries, e.g. re.search(rf"\b{re.escape(name)}\b", haystack, re.IGNORECASE) in missing_agents_in, and add a guard test that a short spelling like "Pi" does NOT match "anthropic" / "ci-pipeline" / "copies"; when flipping to \b, re-run all seven surfaces and add "claude-code" as an accepted spelling for the claude-code row (\bClaude Code\b will not match the hyphenated claude-code form used in pyproject.toml's keywords). (2) actually name Pi in pyproject.toml's description + keywords, docs/comparison.md, and .github/pages-stub/index.html.
  3. [Axis 3] No live-CLI process double: the fake process exits the instant it is awaited, so orphan reap/kill, CancelledError handling, SIGKILL escalation, post-EOF reap, leaked-pipe drain and the loop-top deadline cut are all untested (tests/test_pi_agent.py:596) — _FakeProcess.wait() (tests/test_pi_agent.py:111-113) is async def wait(self): self.returncode = self._final_returncode; return self.returncode — it reports an exit the first time it is awaited, and communicate() awaits it as exit_waiter on every turn. _ExplodingProcess (line 596) inherits it, so by the time the finally runs proc.returncode is 0 and _reap_orphaned_cli returns at if proc is None or proc.returncode is not None: return (pi_agent.py:1049-1050). I proved this empirically: re-running test_stream_error_becomes_a_crash_with_partial_parked with an added assert proc.killed is True fails — AssertionError: orphan reap never ran / assert False is True. Consequently these stay uncovered: proc.kill() + self._sweep_process_groups() (pi_agent.py:1051-1053), the whole except asyncio.CancelledError: self._finalize_external_cancel(...); self._capture_partial_turn(...); raise handler (pi_agent.py:1024-1026, no test ever cancels the communicate() task), and kill()'s escalation if proc.returncode is None: ... proc.kill() (pi_agent.py:780-781). The consequence is written in the method's own docstring: "AgentCrashError is retried, so attempt 2 would spawn a SECOND pi editing the very files the criteria are about to score." The OpenCode twin this module says it mirrors already solved this and documents exactly this trap — tests/test_opencode_agent.py:1542 class _ExplodingRunningProcess(_HangingProcess): "_ExplodingProcess inherits the plain fake's wait(), which reports an exit code the instant it is awaited — so it can never model the case that matters for teardown." Port _ExplodingRunningProcess, TestTurnAlwaysReapsTheCli (test_opencode_agent.py:1733-1795, incl. test_external_cancel_kills_the_cli and test_a_clean_turn_kills_nothing) and TestExternalCancel (1621-1646) to the Pi suite, asserting proc.killed is True, one AgentEndEvent with crash_reason == "turn cancelled", and a parked crashed=True pending_turn.
  4. [Axis 3] _resolve_cost's two rate-card fallbacks are never asserted, so nothing pins the value that feeds max_usd and the cost report (tests/test_pi_agent.py:204) — grep -n "total_cost_usd\|rate card\|calculate_cost" tests/test_pi_agent.py tests/test_pi_agent_config.py tests/test_pi_smoke_task.py returns exactly one hit — line 204, assert usage.total_cost_usd == pytest.approx(EXPECTED_COST) — which only covers the "stream cost wins" leg. _resolve_cost (pi_agent.py:590-600) has two more legs, both load-bearing per its own docstring ("understating cost would silently defeat max_usd budget gates"): if not self.saw_cost: return rate (591-592) and if self.cost_usd == 0.0 and rate: ... return rate (593-599). Both merely execute incidentally — the shared _turn_end(...) helper defaults cost: float = 0.0 and the test model IS priced (calculate_cost('openrouter/moonshotai/kimi-k3', uncached_input_tokens=10, output_tokens=5, ...) returns 0.000105), so the $0-fallback runs in most tests with no assertion on the result. A regression that returns self.cost_usd unconditionally would keep all 84 tests green while silently booking $0 for every Pi run and blinding run_limits.max_usd (FinalStatus.COST_BUDGET_EXCEEDED). The OpenCode twin has the full matrix — tests/test_opencode_agent.py:386-450 class TestCostFallsBackToTheRateCard with five cases (stream cost wins / missing cost priced from the rate card / unpriced model reports None not 0.0 / zero reported cost on a priced model uses the rate card and logs "not understated" / zero on an unpriced model stays 0.0). Port those five cases against _turn_end streams with the cost key omitted, with cost.total = 0, and with model="nowhere/not-a-real-model".
  5. [Axis 6] A terminal Pi provider error (stopReason: "error") is scored as a clean COMPLETED turn instead of crashing, so infrastructure failures are booked as agent failures (src/coder_eval/agents/pi_agent.py:1112) — _PiTurnState.on_turn_end captures the terminal provider error at lines 519-523 (self.error_message = err if isinstance(err, str) and err else "pi reported stopReason=error") — its own comment at 515-516 says this exists because "a pi -p that exits 0 after exhausting retries still surfaces WHY" — but _settle_turn never consults it: after the returncode guard (1090) and the vocabulary guard (1098) it falls straight through to return AgentEndStatus.COMPLETED (1112). finalize then emits crashed=False and only parks the text in ResultSummary(result=...). The sibling harness this module says it "mirrors almost verbatim" does the opposite — opencode_agent.py:1378-1379: if state.error_message is not None: self._crash_turn(state, collector, f"OpenCode error: {state.error_message}"). FAILURE SCENARIO: OPENROUTER_API_KEY is expired; pi runs its internal retries, emits turn_end with message.stopReason == "error" / errorMessage: "401 unauthorized", then agent_settled, and exits 0. _settle_turn returns COMPLETED, crashed=False, so _communicate_with_retry never retries the transient error and the task is graded on an empty sandbox — recorded as FinalStatus.FAILURE, whose category is "failed" (models/enums.py:36), instead of FinalStatus.ERROR -> "error" (line 31), which reports exclude from task outcomes. Provider outages therefore silently depress the measured pass rate. FIX: crash (or at minimum classify) on a non-None state.error_message in _settle_turn, mirroring OpenCode, and update tests/test_pi_agent.py::test_terminal_error_is_surfaced_in_result (which currently asserts record.crashed is False, line 878) plus docs/agents/HARNESS_PARITY.md if a deliberate divergence is really wanted.
  6. [Axis 6] Pi's turn_end usage telemetry is never validated: all-zero or renamed token buckets finalize as a clean COMPLETED turn, the PI.md-promised warning never fires, and max_usd/max_total_tokens go blind (src/coder_eval/agents/pi_agent.py:1098) — _settle_turn's only telemetry guard is if not stopped_early and not state.max_turns_exhausted and state.recognized_events == 0: (line 1098). OpenCode deliberately added a second arm for exactly this gap — opencode_agent.py:1409-1411: nothing_recognized = state.recognized_events == 0; finished_without_tokens = state.steps_finished > 0 and state.usage.is_empty(); if ... and (nothing_recognized or finished_without_tokens) — with the comment that keying on recognized_events alone "left the identical outcome reachable one layer down". Pi dropped that arm (the PR's own .claude/harness-candidates.md entry notes turns_finished was removed as dead because "Pi deliberately dropped that guard"), and the compensating warn does not cover the drift shape: _as_int returns 0 silently for a missing key (lines 461-462, if value is None: return 0) and _warn_token_shape fires only when the whole usage object is missing/empty (line 483) or a bucket has a bad type (line 463). So a usage dict whose keys were renamed — or one that legitimately reports zeros — logs nothing at all, contradicting docs/agents/PI.md:212 ("the turn is scored and a warning is logged"); the new test tests/test_pi_agent.py:823-834 shows it, feeding a full zero-valued usage dict via _turn_end(inp=0, out=0) and asserting only record.crashed is False, with no warning assertion because none is emitted. FAILURE SCENARIO: a Pi upgrade renames usage.input/output/cacheRead/cacheWrite and moves cost; every event type is still recognized so line 1098 does not fire, each bucket resolves to 0 with no warning, saw_cost stays False so _resolve_cost returns None, and EventCollector.build_turn_record maps the all-zero usage to token_usage=None (streaming/collector.py:183-185) — every turn reports 0 tokens and $0, run_limits.max_usd / max_total_tokens can never trip, and nothing names the drift. FIX (keeps the documented score-don't-crash policy): warn once when a turn_end that carried a usage object yields all-zero buckets — restore an equivalent of steps_finished > 0 and state.usage.is_empty() as a warn condition in _settle_turn, with a test asserting the log line.
  7. [Axis 7] Pi's plugins -> --skill injection is live in code but documented as absent in five places across four surfaces (PI.md, HARNESS_PARITY.md, CLAUDE.md, PiAgentConfig docstring) (docs/agents/PI.md:238) — The PR's own commit 91d67230 feat(pi): load agent.plugins skills via --skill landed AFTER the docs commit 1e4b61a8 docs(agents): 4/4 …, so every doc surface still describes the pre-feature state. Code (authoritative): src/coder_eval/agents/pi_agent.py:740 self._skill_dirs = _plugin_skill_dirs(self.config.plugins, log=logger, harness="pi"), :850-854 for skill_dir in self._skill_dirs: … argv += ["--skill", skill_dir], :179-180 # NOTE … and pluginsIS supported (each resolved skills dir is\n# mapped to a--skill argument), so neither is here., and tests/test_pi_agent.py:436 """plugins IS supported now (-> --skill) …""" / :938 assert "--skill" in argv. The four stale surfaces: (a) docs/agents/PI.md:238-244 — "plugins / skills are not injected. Pi has a native --skill flag, but v1 does not wire agent.plugins → --skillConsequence: Pi cannot run activation suites in v1. Follow-up: map each resolved skill dir to a --skill <dir> arg and add a Pi branch to skill_triggered." (the skill_triggered branch is also unnecessary — criteria/skill_triggered.py:41,73-75 matches skills/<name>/ in ANY string tool parameter, agent-agnostically); (b) docs/agents/PI.md:134-135 — "permission_mode, plugins, and system_prompt_file are unenforced too"; (c) docs/agents/HARNESS_PARITY.md:176-181 — "Pi does not read plugins at all in v1: it warns and ignores them, so it cannot run activation suites yet"; (d) src/coder_eval/models/agent_config.py:419-420 (the public PiAgentConfig docstring) — "plugins are NOT injected (no activation suites in v1)"; (e) CLAUDE.md:147 — "but does NOT read plugins (warned+ignored; no activation suites in v1, though it has a native --skill flag)". Fix: rewrite all five to state that plugins IS honored for its skills half (each resolved skills dir → one --skill <dir>, recorded as pi_skill_paths in environment_info), that Pi CAN therefore run activation suites, and add a pi column to the agent.plugins[].path depth table at docs/agents/HARNESS_PARITY.md:127-130 (Pi reuses OpenCode's _plugin_skill_dirs, so it requires the plugin-root shape, not the bare skills dir).
  8. [Axis 8] Unsanitized task_id is interpolated into pi's path-shaped --session-id, so every dataset-row task (id contains '/') fails before any work (src/coder_eval/agents/pi_agent.py:758) — start() builds the CLI identifier as self._session_id = f"coder-eval-{self.task_id}-{uuid4().hex[:8]}" (pi_agent.py:758) and hands it to the CLI verbatim as --session-id (pi_agent.py:843-844), alongside --session-dir <tempdir>. task_id is path-special on every dataset-backed task: orchestration/task_loader.py:464 sets data["task_id"] = f"{task.task_id}/{row_id}", so an activation/suite row yields --session-id coder-eval-my_suite/row_003-a1b2c3d4. This repo already treats that exact hazard as a known one at the one other place a task_id reaches a filesystem name — sandbox.py:288: safe_task_id = self.task_id.replace("/", "_").replace("\\", "_"), commented "Dataset row tasks have IDs like "parent/row" -- flatten path separators so they don't become subdirectories". Pi is the only agent that puts task_id into a value the CLI consumes (grep -rn "self.task_id" src/coder_eval/agents/*.py shows every other agent only stores it for event labelling), so this is new surface. If pi derives its session file from the session id under --session-dir — the usual implementation for a --session-dir + --session-id pair — the segment resolves to a non-existent subdirectory and the session either errors (every row of the suite finalizes ERROR) or silently fails to persist (multi-turn/simulation continuity is lost while the run still reports SUCCESS). No test covers it: tests/test_pi_agent.py:172 constructs every agent with task_id="t1". Fix: sanitize once (reuse the sandbox.py:288 replacement) before interpolating, and add a test asserting the emitted --session-id contains no path separator for task_id="suite/row_1".

Non-blocking, but please consider before merge

  1. [Axis 1] Four above-A blocks in the new agent: communicate D (26), on_turn_end C (19), _settle_turn C (17), _handle_line C (13) (src/coder_eval/agents/pi_agent.py:472) — Split the two Pi-specific reducers that carry the new complexity: _PiTurnState.on_turn_end (line 472, radon C (19), ~90 lines) does five unrelated jobs in one body — bucket coercion (lines 490-497), turn-total accumulation (499-504), cost accumulation (505-510), stop-reason/error latching (512-523), and assistant-message + TurnEndEvent assembly (525-564); extracting _accumulate_usage(usage) and _append_assistant_message(...) makes each independently testable. PiAgent.communicate (line 888) is radon D (26), above the axis's 10-20 band, and its stdout loop (lines 964-997: deadline arithmetic, asyncio.wait race, post-exit drain, max_turns cut, should_stop cut) is the natural extraction. Calibration note: OpenCodeAgent.communicate measures exactly D (26) too and its _settle_turn is D (24) vs Pi's C (17), so the new module is not worse than the sibling it was copied from — resolving finding #1 (one shared driver) would retire most of this number at the same time. Verified with uv run radon cc -s -n B on pi_agent.py at PR HEAD: M 888:4 PiAgent.communicate - D (26), M 472:4 _PiTurnState.on_turn_end - C (19), M 1055:4 PiAgent._settle_turn - C (17), M 1143:4 PiAgent._handle_line - C (13).
  2. [Axis 3] The reconciliation invariant test asserts only 3 of the 4 token buckets, and cacheWrite is zero in every Pi fixture and helper (tests/test_pi_agent.py:206) — test_reconciliation_invariant is docstringed "Summing the four buckets across messages must equal token_usage exactly" but sums three: lines 213-215 are sum(m.input_tokens ...), sum(m.output_tokens ...), sum(m.cache_read_tokens ...) — there is no sum(m.cache_creation_tokens for m in record.messages) == usage.cache_creation_input_tokens. Worse, no test ever supplies a nonzero cache-creation value: every cacheWrite in tests/fixtures/pi_happy_stream.jsonl is 0, and the _turn_end(...) helper's cache_write parameter (tests/test_pi_agent.py:59) is never passed by any caller, so the only assertion on that bucket is assert usage.cache_creation_input_tokens == 0 (line 203). A drift in the usage.get("cacheWrite") key or in the fold at pi_agent.py:502 / the per-message field at 543 / the TurnEndEvent tokens at 560 would leave the whole suite green while zeroing cache-creation tokens for every Pi run. The OpenCode twin does both: its happy fixture carries a nonzero value (assert usage.cache_creation_input_tokens == 5, tests/test_opencode_agent.py:245) and its invariant test asserts the fourth bucket (assert sum(m.cache_creation_tokens for m in record.messages) == usage.cache_creation_input_tokens, tests/test_opencode_agent.py:258). Add the fourth assertion and give at least one _turn_end(..., cache_write=N) stream.
  3. [Axis 3] _build_env's PLUGIN_TOOLS_DIR export (pi_agent.py:883) is uncovered — the UiPath plugin-discovery pin half of the Agent.start contract has no Pi test (src/coder_eval/agents/pi_agent.py:883) — _build_env's if self._plugin_tools_dir and "PLUGIN_TOOLS_DIR" not in env: env["PLUGIN_TOOLS_DIR"] = self._plugin_tools_dir (pi_agent.py:882-883) is uncovered — line 883 is in the --cov-report=term-missing miss list, and grep -n "plugin_tools_dir\|PLUGIN_TOOLS_DIR" tests/test_pi_agent.py returns nothing. class TestSandboxEnvironment (tests/test_pi_agent.py:410-424) tests only the PATH prepend and whole-environment inheritance, even though start() accepts plugin_tools_dir (pi_agent.py:723) and the docstring calls it part of the mock-shadowing contract. This is the variable the sandbox's mock CLIs read to write their invocation log, so a break silently zeroes cli_called-style criteria on every Pi task rather than erroring. OpenCode covers all three legs of the same two lines — tests/test_opencode_agent.py:695 test_plugin_tools_dir_is_exported (assert captured["kwargs"]["env"]["PLUGIN_TOOLS_DIR"] == "/sandbox/tools"), :704 test_inherited_plugin_tools_dir_wins, and :717 (absent → key not present). Port those three.
  4. [Axis 3] The new per-turn slicing in the evalboard cost apportionment is only ever exercised with a single turn (evalboard/lib/__tests__/parseMessages.test.ts:892) — All three tests in describe("parseMessages — open-weight cost apportionment") pass a one-element turns array (const turns: TurnEntry[] = [ { ... } ] at lines 893-902, 913-918 and 923-929), so the change's new index variable — const turnStart = out.length; (evalboard/lib/runs.ts:1614) and const turnRows = out.slice(turnStart) — is only ever evaluated as 0. Nothing pins the property the variable exists for: that turn N's apportionment addresses turn N's rows only. If it regressed to out.slice(0), turn 2's anyPriced check would see turn 1's already-priced rows and skip apportionment (or overwrite turn 1's costs), and every current test would still pass. Add a two-turn case — turn 1 priced from the rate card, turn 2 unpriced with a real token_usage.total_cost_usd — and assert turn 1's rows keep their rate-card values while turn 2's rows sum exactly to turn 2's total.
  5. [Axis 7] docs/agents/OPENCODE.md's container-credential and Docker-blocker claims are left stale by this PR's own env_passthrough + image changes (docs/agents/OPENCODE.md:301) — src/coder_eval/models/sandbox.py adds "OPENROUTER_API_KEY", to DockerDriverConfig.env_passthrough's default allowlist. That directly contradicts two statements the same PR touched: (a) docs/agents/OPENCODE.md:298-303 — "- No credentials would reach it. The docker driver forwards host environment\n variables through an explicit allowlist (DockerDriverConfig.env_passthrough),\n which carries per-harness blocks for Codex and Antigravity but none for\n OpenCode — so OPENROUTER_API_KEY and friends are not passed through" (the PR edited this exact paragraph, changing only SandboxConfigDockerDriverConfig); and (b) docker/Dockerfile:71-72 — "npm install -g opencode-ai), absent from this image, and no OpenCode\n# credentials are in DockerDriverConfig.env_passthrough". Consequence: docs/agents/OPENCODE.md:305-308 still tells users to "pass the credentials via sandbox.env_passthrough_extra" when building a custom OpenCode image, which is now unnecessary — OPENROUTER_API_KEY reaches the container by default. Fix: state in both places that OPENROUTER_API_KEY IS now in the default allowlist (added for Pi) so only the missing CLI blocks OpenCode-in-docker, and drop the obsolete env_passthrough_extra instruction.
  6. [Axis 8] _TOOL_NAME_MAP omits Pi's real find tool while carrying a dead glob entry, so a canonical Glob criterion scores 0 on Pi runs (src/coder_eval/agents/pi_agent.py:149) — _TOOL_NAME_MAP (pi_agent.py:142-157) maps "glob": "Glob" (line 149), "grep": "Grep" (150) and "list"/"ls": "LS" (151-152) but has no find entry, even though this module's own comments enumerate find as a Pi built-in twice: pi_agent.py:184-185 ("Pi's built-in tool names are lowercase (bash/read/write/edit/grep/find/ls)") and pi_agent.py:857-858 (same list); PI.md:130 repeats it. Unmapped names "pass through unchanged" (pi_agent.py:141), so a Pi find call is persisted as tool_name: "find" and any command_executed / commands_efficiency criterion written against the canonical Glob scores 0 on a Pi run that did the equivalent search — breaking the cross-harness comparability the map exists to provide. This is the same defect class the sibling documents at opencode_agent.py:144-152 for apply_patch ("Unmapped, every tool_name: Write / tool_name: Edit criterion scores 0 on a GPT-family model that edited the file correctly"). Fix: add "find": "Glob" (or "Grep", whichever matches Pi's tool semantics — confirm against a captured run) and extend tests/test_pi_agent.py::TestToolNormalization with a case for it.

Nits

  1. [Axis 1] Tautological Pi tests that restate the Literal/class definition or duplicate the preceding test instead of asserting behaviour (tests/test_pi_agent_config.py:45) — Drop both tautological tests. test_thinking_level_literal_has_exactly_seven_values (lines 45-55) re-states the type definition it is checking — assert set(PiThinkingLevel.__value__.__args__) == {"off", "minimal", "low", "medium", "high", "xhigh", "max"} — and is fully covered behaviourally by the parametrized test_all_seven_thinking_levels_validate (lines 40-42) plus test_invalid_thinking_level_rejected (lines 58-60); it can only fail in lockstep with the edit that would change it, so it guards nothing. test_importable_from_models (lines 21-23) asserts PiAgentConfig.__name__ == "PiAgentConfig", which is vacuous — the real assertion is the from coder_eval.models import ... PiAgentConfig on line 13, already exercised by test_member_of_discriminated_union. No precedent for either shape exists in the suite (grep -rn "__args__" tests/*.py returns nothing), so this is new noise rather than house style.
  2. [Axis 2] if self.config.thinking_level: is a statically always-true guard — the Literal admits no falsy value (src/coder_eval/agents/pi_agent.py:848) — _build_argv reads if self.config.thinking_level: / argv += ["--thinking", self.config.thinking_level] (lines 848-849), but the field is thinking_level: PiThinkingLevel = Field(default="medium", …) (models/agent_config.py:426-429) over Literal["off", "minimal", "low", "medium", "high", "xhigh", "max"] (line 388) — non-Optional, with no empty-string member, on a model declaring validate_assignment=True (agent_config.py:137). The condition can therefore never be False, but it reads as if thinking_level might be unset and invites a future reader to add a | None. The sibling that owns the same field name reads it unconditionally — antigravity_agent.py:438: level = types.ThinkingLevel(self.config.thinking_level). Drop the guard (argv += ["--thinking", self.config.thinking_level]), matching Antigravity.
  3. [Axis 2] Any and a missing return type on the test helper every Pi test funnels through (tests/test_pi_agent.py:165) — async def _run(agent: PiAgent, tmp_path: Any, prompt: str = "do the thing", **kwargs: Any):tmp_path is pytest's pathlib.Path fixture (used as await agent.start(str(tmp_path)) on line 166), and the helper returns the TurnRecord from agent.communicate (line 167), so both are precisely typeable. As written, the ~40 tests that consume its result get an implicit Any back, so an assertion against a field TurnRecord does not have type-checks clean instead of failing. Annotate as async def _run(agent: PiAgent, tmp_path: Path, prompt: str = "do the thing", **kwargs: Any) -> TurnRecord:.
  4. [Axis 3] The Pi tests leak one pi-session-* tempdir per test because start() is called without a matching stop() (tests/test_pi_agent.py:166) — _run does await agent.start(str(tmp_path)) (line 166) and start() creates a real host tempdir (self._session_dir = tempfile.mkdtemp(prefix="pi-session-"), pi_agent.py:759) that only stop() removes (_cleanup_session_dir, 767-770). grep -c "await agent.stop()" tests/test_pi_agent.py returns 3 against 56 start/_run call sites, so nearly every test leaks. Measured: ls -d $TMPDIR/pi-session-* | wc -l went from 553 to 603 across one pytest tests/test_pi_agent.py run — 50 new directories per invocation. Wrap the agent in a fixture that calls await agent.stop() in teardown (or point tempfile.mkdtemp at tmp_path via monkeypatch) so the suite is self-cleaning like the rest of the repo's sandbox tests.
  5. [Axis 4] New third-party npm CLI baked into the shared docker image is outside every dependency-audit gate (docker/Dockerfile:43) — Line 43 adds RUN npm install -g @earendil-works/pi-coding-agent@${PI_VERSION} (pin ARG PI_VERSION=0.84.4, line 42) to the single image every --driver docker task runs, so a new third-party package — its transitive tree resolved by semver at build time, with npm lifecycle scripts enabled, executed as root — is now part of the trusted base image for all harnesses, not just Pi. The PR's Python-side delta adds nothing (pyproject.toml's new extra is literally pi = [], and uv.lock's only change is provides-extras = [..., "pi"]), so the routed pip-audit "No known vulnerabilities found" does not cover this dependency at all, and no npm-side audit exists in CI. The version pin is guarded (tests/test_image_from_dockerfiles.py::test_pi_cli_baked_and_pinned rejects latest), which is the good half; complete it by adding an npm-side supply-chain gate (npm audit --audit-level=high or an SBOM/osv-scanner step over the image) to the same workflow that runs pip-audit, and consider npm install -g --ignore-scripts plus installing Pi in a separate layer/stage so a non-Pi image can be built without it. CVSS:3.1/AV:L/AC:H/PR:H/UI:N/S:U/C:L/I:L/A:L
  6. [Axis 4] OPENROUTER_API_KEY added to the blanket docker env allowlist, so it reaches containers of every agent type (src/coder_eval/models/sandbox.py:263) — Line 263 adds "OPENROUTER_API_KEY", to DockerDriverConfig.env_passthrough's default_factory, and that allowlist is agent-agnostic: isolation/docker_runner.py:1374-1381 does merged_allowlist = set(cfg.env_passthrough) | set(cfg.env_passthrough_extra) then argv += ["--env", env_var] for every name present in the host env, with no reference to agent.type. A claude-code or codex task run under --driver docker therefore receives Pi's OpenRouter credential in its container environment even though nothing in that run needs it, and the container defaults to --network bridge (docker_runner.py:1353-1357) with an agent whose permission_mode may auto-run bash — so the key is readable and exfiltratable by a prompt-injected or adversarial agent that has no legitimate use for it. The forwarding mechanism itself is fine (name-only --env VAR, so the value never lands in the logged argv). The least-privilege fix is to gate provider credentials on the resolved agent kind — forward OPENROUTER_API_KEY only when agent.type == pi, as CODEX_* / GEMINI_API_KEY / ANTHROPIC_API_KEY should likewise be — rather than growing one blanket set; at minimum record the deliberate trade-off next to the entry (the adjacent HOME entry already sets that precedent with "Remove this entry if you don't want host HOME leakage"). CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N
  7. [Axis 7] PI.md documents an unpinned npm install for a CLI whose event-vocabulary drift is a hard crash (docs/agents/PI.md:44) — docs/agents/PI.md:43-46 gives the host install as unpinned: "bash\nnpm install -g @earendil-works/pi-coding-agent\npi --version\n", and the page names no verified version. But the harness hard-fails on vocabulary drift — docs/agents/PI.md:203-208 "### Drift is crashed, not scored" — and both the reducer's _RECOGNIZED_EVENTS and the whole event grammar are captured from one release (src/coder_eval/agents/pi_agent.py:17 "Event grammar (captured from pi 0.84.4)", :192-194 "The full recognized Pi vocabulary (from pi 0.84.4)"), while docker pins it (docker/Dockerfile:39-41 "ARG PI_VERSION=0.84.4" with the rationale "@latest would freeze\n# nondeterministically under layer caching"). A host install of @latest can therefore land on an untested vocabulary. Fix: document the pinned version in the install snippet (npm install -g @earendil-works/pi-coding-agent@0.84.4) and note that it must match docker/Dockerfile's PI_VERSION for host↔docker result parity.
  8. [Axis 8] Pi wired into the evalboard badge map but not into KNOWN_HARNESSES / HARNESS_COLORS (evalboard/app/_components/harness-badge.tsx:20) — The PR adds pi: { src: "/harness/pi.png", label: "Pi · pi.dev", short: "Pi" }, to HARNESS_LOGO (harness-badge.tsx:20) but leaves the two sibling harness registries in evalboard/lib/harness.ts untouched: KNOWN_HARNESSES (harness.ts:11-16) still lists only claude-code, codex, antigravity, delegate-sdk, and HARNESS_COLORS (harness.ts:82-87) the same four. KNOWN_HARNESSES is passed as a hardcoded selector list by evalboard/app/trends/page.tsx:142 and evalboard/app/watchlist/page.tsx:61, so those two pages cannot select a Pi run even once Pi runs exist, and harnessColor (harness.ts:94-95) folds Pi into the gray HARNESS_COLOR_FALLBACK in every multi-harness chart. Membership is documented as display-order/color only, so nothing breaks — but the PR half-wired the surface. Fix: add "pi" to KNOWN_HARNESSES and a validated hue to HARNESS_COLORS (harness.ts:80 notes "Re-run the palette validator before adding a fifth entry"), or leave the badge entry out too for consistency with OpenCode, which is absent from all three maps.

What's Missing

Downstream consumers:

  • 🟠 The new apportionment (runs.ts:2014-2045) is gated on "nothing on this turn was priced by the rate card + the turn reports a real total_cost_usd" — NOT on the harness — so it also fires on existing LiteLLM/OpenRouter runs (litellm backend, OpenCode), whose OpenRouter models are deliberately absent from evalboard/lib/pricing.ts while litellm_cost.apply_actual_cost writes a real turn-level total_cost_usd. That silently reverses the documented decision those runs rely on (evalboard/lib/tests/messageActualCost.test.ts:4-7: "actual per-call cost is no longer distributed onto transcript messages — it lives in the separate providerCalls table"), retroactively for every historical run, so the same money now appears both apportioned across message rows and itemised in the ProviderCall table. The existing test only passes because its fixture turn carries no total_cost_usd; nothing covers a turn with both provider_call_costs and a real total. Either skip apportionment when turn.provider_call_costs?.length > 0 (or key it on the absence of per-call actuals) and revisit that test's stated policy, or state the change explicitly. (trigger: evalboard/lib/runs.ts)

Parallel paths:

  • 🟡 docker/Dockerfile bakes the pinned pi CLI (line 42-43), but the sibling image built from the same directory — docker/Dockerfile.runtime, the relocatable kit that inject-mode task images COPY --from (its contract block only promises /opt/coder-eval/node/bin/{node,claude}, and it installs only the codex extra) — was not extended. A --type pi task whose image uses the runtime kit therefore fails at spawn with "pi CLI not found", and neither the kit's contract comment nor docs/DOCKER_ISOLATION.md § "Tasks that bring their own base image" (which enumerates "standalone CPython + Node + the coder-eval CLI + Claude Code" and already records the kit's other exclusions, e.g. the [uipath] extra) says Pi is unsupported there. (trigger: docker/Dockerfile)
  • 🟡 OPENROUTER_API_KEY — Pi's only credential, and now a default docker passthrough (models/sandbox.py:263) — was not added to either credential-onboarding surface: docs/USER_GUIDE.md's Environment Variables table (which has rows for CODEX_API_KEY/CODEX_BASE_URL/CODEX_MODEL, GEMINI_API_KEY/ANTIGRAVITY_MODEL, LITELLM_*) and .env.example (which has a commented block per harness). The same PR did update USER_GUIDE's --type row to list pi, so a reader is told the harness exists but not what it needs to authenticate; only docs/agents/PI.md names the variable. (trigger: docs/USER_GUIDE.md)
  • 🔵 Roster/--type enumerations outside CE047's seven tracked surfaces were left at four harnesses: docs/tutorials/01-first-evaluation.md:19 (the "or" list of agent guides), docs/CI_GATE.md:180-183 (per-harness credential prerequisites — names GEMINI_API_KEY for Antigravity, nothing for Pi) and the extras examples in docs/CI_GATE.md:58 + action.yml:31 (no mention of the new pi extra). CE047 cannot catch any of these because they are not in ROSTER_SURFACES at all. (trigger: README.md) (restates: Axis 2: CE047's substring roster guard is vacuous for the 2-char name "Pi")
  • 🔵 Provenance/help surfaces for the baked agent binaries were not extended to the second one: the image stamps LABEL org.coder-eval.claude-code-version (docker/Dockerfile:103-105, rationale "so the host can assert it via docker image inspect before a (billed) run") but no org.coder-eval.pi-version, and the Makefile's docker-image target still advertises "core + both agents baked in" / echoes "(core + claude + codex)" (Makefile:114-116). Combined with utils.py::get_version_info recording only claude_code_cli and PiAgent.get_environment_info recording no CLI version, a docker Pi run's harness version is unrecoverable from the run record. (trigger: docker/Dockerfile)
  • 🔵 Pi introduces a third run_limits.max_turns unit (native turn_start steps), documented in the parity table, but the executable cross-harness fixture whose whole purpose is comparing that cap — tasks/run_limits/max_turns_cap.yaml:1-7, "Run it with --type claude-code / codex / antigravity and compare" — was not extended (OpenCode is missing from it too), so the new unit has unit-test coverage only. (trigger: docs/agents/HARNESS_PARITY.md)

Tests:

  • 🟡 The --skill injection shipped to unblock activation suites has no test of the signal those suites actually score: tests/test_pi_agent.py:933-950 asserts only that --skill <dir> reaches argv and lands in environment_info, while nothing exercises skill_triggered against Pi's real engagement shape (per the pinned CLI's docs/skills.md, the agent reads the full SKILL.md, i.e. a read tool call normalized to Read with pathfile_path via _PI_ARG_RENAME, path <root>/skills/<name>/SKILL.md). tests/test_skill_triggered.py carries Claude and Codex cases only, and src/coder_eval/criteria/skill_triggered.py's module docstring still enumerates just those two mechanisms — so a rename in _PI_ARG_RENAME/_TOOL_NAME_MAP would report recall 0.0 for every positive row with no failing test. (trigger: tests/test_pi_agent.py) (restates: Axis 7: Pi's plugins -> --skill injection is live in code but documented as absent in five places across four surfaces)
  • 🟡 Docker support for Pi (baked CLI + OPENROUTER_API_KEY passthrough) ships with no task that exercises it: there is no tasks/agents/pi_hello_world_docker.yaml counterpart to the existing claude_hello_world_docker.yaml / antigravity_hello_world_docker.yaml pair, and the only Pi task deliberately opts out of the CI bucket (no smoke-pass). The whole docker claim rests on a Dockerfile-text assertion (tests/test_image_from_dockerfiles.py::test_pi_cli_baked_and_pinned) plus "OPENROUTER_API_KEY" in DockerDriverConfig().env_passthrough, so nothing verifies that an in-container pi actually starts, authenticates, and produces a scored turn. (trigger: tasks/pi_smoke_test.yaml)
  • 🟡 The three apportionment tests (parseMessages.test.ts:892-931) use a hand-built 1-2 assistant-message turn, never the shape every real Pi turn has: the EventCollector books one synthetic reconciliation entry per turn, and parseMessages appends it LAST (runs.ts:1954-2000), so on a Pi turn it both draws a token-weighted share of the real cost and absorbs the rounding residual — i.e. the RECONCILE row shows money while the assistant rows show less. tests/fixtures/pi_happy_stream.jsonl already provides a realistic stream to derive that fixture from. (trigger: evalboard/lib/tests/parseMessages.test.ts) (restates: Axis 3: The new per-turn slicing in the evalboard cost apportionment is only ever exercised with a single turn)
  • 🔵 tests/test_pi_smoke_task.py:17 resolves the task as a bare cwd-relative Path("tasks/pi_smoke_test.yaml"), so all three tests fail outside the repo root (reproduced: running pytest test_pi_smoke_task.py from tests/ gives 3 × FileNotFoundError: Task file not found: tasks/pi_smoke_test.yaml). The house pattern is a file-anchored path (tests/test_custom_lint.py:2512 uses Path(__file__).parent.parent / "tasks"). (trigger: tests/test_pi_smoke_task.py)

Display & mapping dicts:

  • 🟡 _TOOL_NAME_MAP (pi_agent.py:142-157) was not derived from Pi's actual tool set: find (Pi's glob-by-pattern tool) has no entry so it passes through as find and every canonical Glob criterion scores 0, the "glob": "Glob" entry is dead (Pi has no glob tool), the real Windows built-in powershell is unmapped, and eight entries (patch, multiedit, list, webfetch, todowrite, todoread, task, glob) name tools Pi never emits. Downstream this also skews analysis.py/report command rollups, which key on the normalized tool name. (trigger: src/coder_eval/agents/pi_agent.py) _(restates: Axis 8: TOOL_NAME_MAP omits Pi's real find tool while carrying a dead glob entry)
  • 🔵 Only one of the three evalboard harness maps was extended for the new kind: HARNESS_LOGO got a pi row, while KNOWN_HARNESSES and HARNESS_COLORS (evalboard/lib/harness.ts:11-16, 82-87) did not — so harness-selector.tsx, trends/page.tsx:142 and watchlist/page.tsx:61 cannot select a Pi run, orderHarnesses sorts Pi into the unknown tail, and harnessColor folds it into the gray fallback in every multi-harness chart. _(trigger: evalboard/app/components/harness-badge.tsx) (restates: Axis 8: Pi wired into the evalboard badge map but not into KNOWN_HARNESSES / HARNESS_COLORS)

Daily/nightly:

  • 🟡 The PR states no blast radius for the shared production image: docker/Dockerfile is what .github/workflows/docker-publish.yml builds and pushes as ghcr.io/<org>/coder-eval-agent:latest on every push to main, i.e. the image EVERY --driver docker task of EVERY harness pulls, and it now carries a new third-party global npm install (~23 MB unpacked plus its dependency tree, lifecycle scripts enabled, installed as root). The repo has a live precedent for exactly this risk — commit 48a4d53 bumped google-antigravity "to clear a Defender false positive on the bundled harness" — and there is still no npm-side audit in the workflow that runs pip-audit. (trigger: docker/Dockerfile) (restates: Axis 4: New third-party npm CLI baked into the shared docker image is outside every dependency-audit gate)
  • 🟡 Nothing scheduled or automated will ever run Pi: the only Pi task excludes itself from the CI E2E bucket, no experiment in experiments/ carries a pi variant, and no nightly surface adds --type pi. That matters more for this harness than the others because vocabulary drift is a hard crash by design (docs/agents/PI.md § "Drift is crashed, not scored", _RECOGNIZED_EVENTS captured from pi 0.84.4) while the documented host install is unpinned — so the first signal of a breaking pi release is a human's ad-hoc run, and the PR does not say who validates Pi after a CLI bump. (trigger: tasks/pi_smoke_test.yaml) (restates: Axis 7: PI.md documents an unpinned npm install for a CLI whose event-vocabulary drift is a hard crash)
  • 🔵 Adding OPENROUTER_API_KEY to the agent-agnostic default allowlist changes every scheduled docker run, not just Pi's: any nightly/CI container (claude-code, codex, antigravity) now receives the OpenRouter credential whenever it is present in the runner environment, on the default --network bridge. The PR does not state that consequence for the production run path or record the trade-off next to the entry, as the adjacent HOME entry does. (trigger: src/coder_eval/models/sandbox.py) (restates: Axis 4: OPENROUTER_API_KEY added to the blanket docker env allowlist, so it reaches containers of every agent type)

Harness & Lint Improvements

Elided from this comment to fit GitHub's 65,536-character body limit.
15 static-check proposals (CE048–CE058 + a ruff ANN201 ratchet, a pyright tests-side widening, and an npm supply-chain gate) and 7 harness improvements are in the full report:
tmp/code-review-260908-0927/00-summary.md § Harness & Lint Improvements (and results.json).
They are durable-infrastructure work, separate from this PR's fixes.

Top 5 Priority Actions

  1. Sanitize task_id before it enters the CLI --session-id at src/coder_eval/agents/pi_agent.py:758 (reuse the /_ flatten from sandbox.py:288, ideally the full [A-Za-z0-9._-] safe set), because pi 0.84.4 validates the id and exits 1, so today every dataset-row task burns its retry budget and finalizes ERROR with an empty event stream.
  2. Make _settle_turn consult state.error_message and crash the turn at src/coder_eval/agents/pi_agent.py:1112 (mirroring opencode_agent.py:1378-1379), and restore the second telemetry arm at pi_agent.py:1098 as a warn when a turn_end that carried a usage object yields all-zero buckets, so a provider outage reports ERROR instead of silently depressing the measured pass rate and token-shape drift stops blinding max_usd/max_total_tokens.
  3. Add "find": "Glob" to _TOOL_NAME_MAP at src/coder_eval/agents/pi_agent.py:149 and drop the dead "glob" entry (Pi has no glob tool; find is glob-by-pattern), so command_executed/commands_efficiency criteria written against the canonical Glob no longer score 0 on a Pi run that searched correctly.
  4. Close the Test Health gap on the new code by porting the OpenCode doubles and matrices into tests/test_pi_agent.py: _ExplodingRunningProcess + TestTurnAlwaysReapsTheCli/TestExternalCancel (the current _FakeProcess.wait() at line 111 exits on first await, leaving orphan reap, SIGKILL escalation and CancelledError unreachable), the five TestCostFallsBackToTheRateCard cases (both _resolve_cost fallbacks stay green even when stubbed to return self.cost_usd), the fourth cache-creation bucket in test_reconciliation_invariant (line 206, with one nonzero cache_write stream), the three PLUGIN_TOOLS_DIR cases for pi_agent.py:883, and a two-turn case in evalboard/lib/tests/parseMessages.test.ts:892 that pins per-turn apportionment (mutating runs.ts:1614 to 0 leaves all 642 tests passing).
  5. Restore truth in the guards and docs: anchor CE047's matcher on word boundaries in tests/lint/agent_roster_parity.py:102 (the 2-char "Pi" added at line 57 matches "anthropic"/"ci-pipeline"/"copies", so pyproject.toml:4, docs/comparison.md and .github/pages-stub/index.html ship a stale four-harness roster with the gate green), then rewrite the five surfaces claiming Pi ignores plugins (docs/agents/PI.md:238 and :134, docs/agents/HARNESS_PARITY.md:176 plus a pi column in its depth table, src/coder_eval/models/agent_config.py:419, CLAUDE.md:147) and the three now-false OPENROUTER_API_KEY claims in docs/agents/OPENCODE.md:301 and :327 and docker/Dockerfile:71.

Stats: 0 🔴 · 8 🟠 · 6 🟡 · 8 🔵 across 8 axes reviewed.

@bai-uipath bai-uipath left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve — no risk to the existing harnesses; the OpenCode duplication is the one thing I'd fix.

  • The port is 2× with OpenCode, and only OpenCode. 16 of the 26 same-named methods are ≥0.75 similar, several byte-identical, with zero overlap against claude-code, codex and antigravity. Worth hoisting the shared subprocess scaffold in a follow-up, before a third CLI harness makes it three copies.

  • Nothing here can break an existing harness — the touched shared surfaces are all additive or defaulted. One caveat: the evalboard cost apportionment fires for any turn the rate card can't price, so it changes LiteLLM and OpenCode runs too, and two comments in that file still assert the opposite.

Minor: four doc surfaces still say plugins is warned-and-ignored after the last commit wired skill injection; CE047 passes vacuously for Pi because a two-letter name substring-matches anything (the comparison page, the Pages stub and the PyPI metadata never actually name it); reasoning tokens are folded into output while the stream's own totalTokens excludes them and is never read — worth a guard like OpenCode's; the reap and post-EOF teardown paths are uncovered (90.7% vs OpenCode's 95.8%); TestReviewFixes should be named for the behavior it covers.

Nice work on the byte-real capture driving offline replay and on baking a pinned CLI with a drift guard — both are better than what OpenCode shipped with.

CarlesUIPath and others added 5 commits September 8, 2026 22:58
…y warn, tool map, CE047

Addresses uipreliga review blockers on PR #159:

- B8: sanitize task_id before it enters pi's `--session-id` (dataset-row ids are
  path-shaped "suite/row"; a raw '/' resolved to a non-existent session subdir
  and failed the row). Keep only [A-Za-z0-9._-].
- B5: crash `_settle_turn` on a terminal provider error (stopReason=error that
  survived pi's internal retries) so it books as ERROR (retryable / excluded
  from outcomes) instead of a clean COMPLETED FAILURE. Mirrors opencode_agent.
- B6: warn once when a turn_end carried a usage object whose every bucket is 0
  (the rename-each-key-to-0 drift shape) — otherwise tokens/cost silently vanish
  and max_usd / max_total_tokens go blind.
- Tool map: `find`→`Glob` (Pi's glob-by-pattern search tool), drop the dead
  `glob` entry — so command_executed/commands_efficiency criteria written
  against the canonical `Glob` score on a Pi run that searched.
- B2: anchor CE047's roster matcher on word boundaries (the 2-char "Pi" matched
  "anthropic"/"ci-pipeline"/"copies", hiding a stale roster) + name Pi in
  pyproject.toml description/keywords, docs/comparison.md, and the pages stub;
  add a guard test that a short spelling is not satisfied by a substring.

Tests: +find→Glob, +all-zero-usage warn, +session-id sanitize, terminal-error
now asserts crash; 465 pi + custom-lint tests pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…laims

Blocker 7: the plugins->--skill feature landed after the docs commit, so five
surfaces still claimed plugins is ignored. Rewrite PI.md (both spots),
HARNESS_PARITY.md (prose + add a pi column to the plugin-path depth table),
PiAgentConfig docstring, and CLAUDE.md to state plugins IS honored for its skills
half (each resolved skills dir -> --skill <dir>, recorded as pi_skill_paths) and
that Pi can therefore run activation suites.

Also correct the now-false OPENROUTER_API_KEY claims this PR made stale in
docs/agents/OPENCODE.md and docker/Dockerfile: OPENROUTER_API_KEY IS now in the
default docker allowlist (added for Pi), so the OpenCode CLI (still not baked) is
the only remaining docker blocker for env-auth runs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…, 4)

The plain _FakeProcess exits the instant its stream is awaited, so orphan reap,
SIGKILL escalation and CancelledError handling were all unreachable, and
_resolve_cost's two rate-card fallbacks were never asserted.

- Add _ExplodingRunningProcess (raises from readline, stays alive) and
  _HangingProcess (blocks in readline, returncode stays None) doubles.
- TestTurnAlwaysReapsTheCli: read-loop crash and external cancel both kill the
  CLI; a clean turn kills nothing (else attempt 2 spawns a second pi editing the
  graded files).
- TestExternalCancel: a cancelled turn parks a crashed partial, emits one
  AgentEndEvent (CRASHED, crash_reason='turn cancelled'), and reaps the child.
- TestCostFallsBackToTheRateCard: five cases — stream cost wins / missing cost
  priced from the rate card / unpriced model reports None / reported-/usr/local/bin/bash on a
  priced model uses the rate card / reported-/usr/local/bin/bash on an unpriced model stays 0.

78 pi tests pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…skills.py

Blocker 1 (concrete slice): pi_agent imported opencode_agent's private
_plugin_skill_dirs, coupling the two harnesses through an implementation-private
symbol while a comment claimed a shared-module hoist was "out of scope". Move the
resolver (+ _manifest_skill_dirs and its constants) into a new shared
agents/_skills.py; opencode_agent and pi_agent both import it from there, so the
cross-agent private import is gone. opencode_agent re-exports it, so its existing
test import is unchanged.

The FULL nd-JSON-CLI driver hoist (a shared NdJsonCliAgent base for communicate /
kill / reap / crash / cost across pi + opencode) is a larger refactor of two
harnesses including the production OpenCode one; it is left as a tracked follow-up
rather than bolted onto this review-response, and the deferral is now documented
at the _TERM_GRACE_SECONDS comment instead of the stale "out of scope" note.

214 opencode+pi tests, custom lint (396), check and typecheck all pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… token/telemetry minors

Caveat: the open-weight cost apportionment (evalboard/lib/runs.ts) fired for ANY
unpriced turn with a real total_cost_usd — including LiteLLM-route and OpenCode
runs, which book the real per-call cost in the separate ProviderCall table by
design. Apportioning the turn total onto message rows too surfaced the same money
twice and reversed the documented "actual cost is not distributed onto transcript
messages" decision for every historical such run. Skip apportionment when the turn
carries a `provider_call_costs` audit (Pi has none, so it is unaffected); add a
parseMessages test pinning it.

Minors:
- Pi now cross-checks the stream's own `totalTokens` against
  input+output+cacheRead+cacheWrite (reasoning is billed but excluded from that
  field) and warns once on a mismatch — a renamed/moved bucket under a CLI upgrade
  would otherwise be absorbed silently by _as_int and blind max_total_tokens/max_usd.
  Mirrors OpenCode's `tokens.total` guard. +test.
- Rename TestReviewFixes -> TestTurnLifecycleAndTokenTelemetry (named for behavior).

(The plugins-docs and CE047 minors bai-uipath listed were already fixed in the
blocker pass; the shared-subprocess-scaffold hoist remains a tracked follow-up.)

79 pi tests, 643 evalboard tests, lint/check/typecheck all pass.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants