[malibio]

Refs #3748.

What this adds

• scripts/eval/harness/pi.ts: runs the harness scenario in one pi --mode rpc process (the extension keeps its watch state in-process, and a reload loses it). The extension and skill are the ones this checkout's installer writes, loaded by path with Pi's own discovery off; Pi's agent directory is not moved, so its sign-in is not copied. HARNESS_MODEL=<provider>/<model> names the model.
• run.ts and trace.ts wiring (Pi tool execution, message and extension UI records).
• A defect the run found, fixed in the shared session module: an agent that wrote id=<uuid>; nodespace node context $id was not watched, because the invocation's id was the text $id. A plain assignment earlier in the same line now supplies it; a prefix assignment and a variable set elsewhere are left alone. Without the fix, the "task finished elsewhere" case was caught only by the CLI's version check at the write; with it, the plugin refuses the tool call and names the change.

Pi results (Pi 1.2.0, openrouter sonnet, one clean run)

Case	Result
Probe: project and skills known with no command	pass (project, 26 skills, the commit standard; status line NodeSpace: Widget)
Skill edited mid-session	pass: the next prompt carried the change note and the agent said what it did not know
Task finished elsewhere	pass: tool call refused with the change; the agent stopped and did not move it to review
Own writes not mistaken for foreign	pass

Tests

bun run --cwd packages/skill test 283 pass (the new test fails without the fix), bun run test:scripts 1544 pass, scripts lint and typecheck clean. The Rust tiers of test:changed cannot build in a fresh worktree here (llama.cpp / OpenSSL header); nothing Rust changed, and the gate runs them.

🤖 Generated with Claude Code

[malibio]

Pragmatic code review (initial review)

Recommendation: COMMENT. Net positive: the driver is small, follows the established driver shape, and its framing and shutdown follow Pi's RPC docs (LF-only framing, disposition: "handled", agent_settled, stdin close). Two items should be fixed before merge: the Claude Code plugin's own copy of nodespaceInvocations was not updated, and un-cancelled Bun.sleep timers keep the driver process alive until the turn deadline. The rest are improvements.

Requirements (issue #3748, Pi items)

• A Pi driver is added to scripts/eval/harness/ and wired into run.ts and trace.ts.
• "The scenario is run against the existing Pi plugin; the Pi column is filled": no live run or results are visible in the diff, so this cannot be verified here. Post the run or trace on the issue. Keep refs, not closes: the issue still has the Codex and Antigravity items.
• [~] "Each defect found is fixed or filed": the $id expansion is one such fix; the Claude Code duplicate below is the same defect left unfixed in the other harness.
• "Completed results table is posted on this issue": outstanding.

Should fix before merge

[Important] The Claude Code plugin has its own, unchanged copy of the parser.
packages/skill/plugins/claude-code/hooks/register.ts:570-640 holds a duplicate of nodespaceInvocations identical to the pre-change body, and shared/nodespace-session.ts itself says "a change to behaviour here belongs there too". After this PR, id=t1; nodespace node context $id is watched under Pi/OpenCode and not under Claude Code, so the cross-harness scenario columns can disagree for a reason that is not the harness, which is what this eval measures. Either apply the same change in register.ts (with a case in plugins/claude-code/tests/register.test.ts), or better have register.ts import the shared function (DRY: the duplication is what let this drift).

[Important] Bun.sleep timers in the Promise.race are never cancelled, so the process outlives the run.
scripts/eval/harness/pi.ts, until() creates a new Bun.sleep(left) for every record read. Each stays pending until the turn deadline (12 min), and Bun keeps the event loop alive for pending sleeps (confirmed: Promise.race([1, Bun.sleep(4000)]) exits after 4003 ms). run.ts has no process.exit, so after the final turn the process lingers for the rest of that turn's deadline, holding hundreds of timers per turn. The same applies to the 10 s Bun.sleep in finally. Use one timer per until() call and clearTimeout it when the read wins (or an AbortSignal.timeout).

[Important] A failed run is recorded as a successful, empty turn.
agent_settled is also emitted after a provider error or exhausted retries; said stays "" and play carries on. The issue itself notes an unsigned Pi stops at "No API key found". Check the assistant message_end's stopReason/errorMessage and agent_settled.aborted, and throw, so a bad credential fails the run instead of producing a column of silence that reads as a Pi defect.

Variable expansion (nodespace-session.ts)

I exercised the function over a set of lines.

• Works: id=t1; ... $id; ${ID} and "$id"; reassignment (id=t1; id=t2 -> t2); chaining (a=t1; b=$a); empty value (yields an empty word, as the shell does); prefix assignment left unresolved (id=t1 nodespace ... $id, correct, since the shell expands before the prefix applies); $ in a value (id=a\$b -> a$b).
• [Improvement] id=$(nodespace node create ...); nodespace node context $id records the literal $. The tokenizer splits at (, leaving the word id=$, recorded as id -> "$"; the later $id becomes $ (observed ["node","context","$"]). Getting an id from a command substitution is the most common way an agent has one, and this case is now rewritten to a bogus value rather than passed through. Do not record (and assigned.delete) an assignment whose value contains $ or a backtick unless it is a lone resolvable $NAME.
• [Improvement] Scope is not modelled. An assignment in a subshell ((id=t1); ... $id -> t1), left of a pipe (id=t1 | cat; ... $id -> t1) or after || leaks into later segments. Low probability, and it can only over-watch the wrong item; say so in the doc comment, or only record assignments whose separator is ;, newline or &&.
• [Improvement] export id=t1 (also local, declare) is not recognised: the head is export, so the segment is skipped (confirmed unresolved). It fails safe, but export is plausible agent output.
• [Nit] The tokenizer drops quote information, so '$id' (a literal) is expanded, and "$id"x yields $idx rather than t1x (fails safe). One sentence in the comment.

Tests (plugin-session.test.ts)

• The first new test is good: it asserts the observable command, the ${ID}/&& variant and the downstream deny.
• [Improvement] The second test's assertion is weak. every(c => !c.startsWith('node context t1')) passes vacuously if nothing ran, and cannot tell "left as $id" from "became $". Assert the exact commands (['node context $id', 'node context $id']).
• Missing cases for the behaviours above: reassignment, $(...) value, export; none in plugins/claude-code/tests/register.test.ts either.
• No test for records() (LF-only split, CR strip, trailing record without newline, U+2028 inside JSON). It is a pure async generator and cheap to test; antigravity-approve.test.ts is the precedent.

RPC driver (pi.ts)

Good: LF-only framing, handled disposition, responses matched by id, child exit while waiting is an error, stdin close as orderly shutdown, --no-session.

• [Improvement] Shutdown can leak. finally waits 10 s, then child.kill() (SIGTERM) without waiting or escalating. A hung Pi, or processes it spawned for the agent's shell commands, can survive, and this is also the path every thrown timeout takes. Kill, await child.exited, and SIGKILL after a short grace.
• [Improvement] The caller's PI_CODING_AGENT_DIR is dropped. delete session.PI_CODING_AGENT_DIR makes Pi fall back to ~/.pi/agent. That is right for the install redirect, but a machine that signs in under a custom dir then gets "No API key found" for a reason nothing names. Capture the caller's value before installSkill and restore it, or delete only the driver's own.
• [Improvement] Credentials / isolation. Not moving the agent dir is the right call (no copy of the sign-in). But the real agent dir still supplies the user's global AGENTS.md, prompt templates and themes, which will shape the scenario and differ per machine. --no-extensions --no-skills covers two of those; add --no-context-files, --no-prompt-templates, --no-themes (listed in cli.md). stderr: "inherit" is fine, but note it if a log is ever posted.
• [Nit] Dialog extension_ui_requests (select, confirm, input, editor) block until answered and the driver never answers, so one would stall to the 12 minute timeout. The NodeSpace extension only calls setStatus, so it is moot today; replying {type:"extension_ui_response", id, cancelled:true} future-proofs it.
• [Nit] send() ignores stdin backpressure, and a write to a dead child throws EPIPE with no context; surface the pi exited error instead.
• [Nit] The try { ... } catch {} in until() also swallows a throw from the done predicate; keep only JSON.parse in the try.

trace.ts / run.ts

Clean; the new event shapes match docs/json.md and rpc-extension-ui.md. Nit: an assistant message_end with an error stopReason prints nothing, so the failed-run case above would not show in the trace either.

Standards

No lint suppression, no any, no sync-boundary markers, no new docs files.

[malibio]

Automated pragmatic-code-review: COMMENT (initial review; posted as a comment because GitHub blocks self-review states). No hard blockers; fix before merge: the Claude Code plugin's own copy of nodespaceInvocations is not updated, un-cancelled Bun.sleep timers keep the driver process alive until the turn deadline, and a failed Pi run is recorded as an empty successful turn. Full findings in the review comment above.

[malibio]

Review addressed (1b98739)

Addressed

• Claude Code parity: its copy of nodespaceInvocations has the same variable resolution. The plugin's own test (bun run --cwd packages/skill test:plugin, 58 pass) and the shared test carry the same cases.
• A value that is still an expansion ($(...), a backtick, a name not set in the line) makes the name unknown rather than the text $; tests pin $(...), reassignment, chaining, a prefix assignment, and export.
• pi.ts: one timer per wait, cleared on the way out (the process now exits when the run ends); an assistant message with an error stop reason throws; shutdown escalates to SIGKILL and waits; a caller's PI_CODING_AGENT_DIR is kept; the agent directory's context files, templates and themes are off.
• The vacuous second test is replaced by exact assertions; records() is exported and its framing is tested (LF only, CR strip, Unicode separators, split multi-byte character, last record with no newline).
• The catch {} swallowing a throw from done: only the JSON parse is inside it now.

Not changed

• Answering dialog extension_ui_requests: the extension only sets a status.
• Stdin backpressure: a handful of small commands per run.
• Subshell and pipe scoping of assignments, and '$id' expanded after quote stripping: the failure is a watch the agent did not intend, not a missed refusal; not worth modelling the shell.

Live: a second clean run on the revised driver passes all four cases and the process exits at the end of the run. Tests: skill 283, plugin 58, test:scripts 1548, lint and typecheck clean.

Re-Review Decision

Decision: NO RE-REVIEW NEEDED

Rationale: The parser change is the review's own suggestion applied to the second copy and tightened for $(...), with the same cases against both. The driver changes are the three the review named, each small, and a live run exercised them. No other logic moved.