feat(cli): add pi as a supported coding agent - #804
Open
yczhang-nv wants to merge 48 commits into
Open
Conversation
Adds the pi coding agent to the NeMo Relay CLI as a hook-path agent, plus the pi extension that drives it. pi has no native hook-configuration file and its external stream is observation-only, so hook calls must originate inside an extension. The extension is a thin HTTP client to the gateway: it forwards pi's lifecycle to /hooks/pi and gates tool calls on the gateway's verdict. CLI side: - crates/cli/src/agents/pi/ with descriptor, adapter, launch and doctor - PiPayloadExtractor using SessionHeaderPolicy::RelayOnly so pi never inherits a stray x-claude-code-session-id - /hooks/pi route and pi_hook handler - Pi on CodingAgent, AgentKind, AgentArg and AgentConfigs pi has no plugin marketplace -- no `pi plugin` verb, no manifest, and no MCP client -- so the ~15 marketplace arms reject pi explicitly and point at `pi install <source>` and the auto-discovery directories, rather than synthesizing manifests pi will never read. Two edit sites the compiler does not enforce, both handled: - FileAgentsConfig carries deny_unknown_fields, so [agents.pi] needed the deserializer as well as the runtime struct - InstallTarget::All enumerates agents explicitly; pi is deliberately absent Extension side: - integrations/pi/ forwards session, agent-run, turn and tool lifecycle - tool_call is the only hook that awaits a verdict; the rest are fired without blocking pi's critical path and drained at session_shutdown - a guardrail rejection arrives as HTTP 403 with error.type = nemo_relay_guardrail_rejected, and error.reason is passed to pi verbatim, so the model reads the guardrail's own words Boundary choices worth noting: tool_execution_start is not forwarded as a tool start (it fires before validation and for calls that never execute), and tool_execution_end rather than tool_result is the end boundary (tool_result never fires for blocked calls). Tests: 2 Rust tests pin the 403 and 200 paths on /hooks/pi; 13 Node tests pin the extension's half of the contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Three fixes found by running pi against a gateway with the ATOF exporter enabled and reading the emitted trace. 1. pi's turn_start/turn_end produced marks, not turn scopes. TurnEnded was only emitted for the hardcoded name "stop", which is Codex and Claude Code vocabulary that pi never sends. The gateway therefore opened one implicit turn covering the whole run and pi's own turn boundaries were lost. ClassificationRules gains a turn_end list. Codex and Claude Code declare &["Stop", "stop"], which preserves their behaviour exactly; pi declares its native turn_end. agent_settled is deliberately not in pi's list -- it marks the end of a logical agent run, which can span several turns, so closing the turn there would merge every re-entry attempt into one. 2. Unawaited hook posts raced, reordering the lifecycle. Firing observability posts concurrently let them arrive out of order. An observed trace had agent_start landing after turn_start and agent_end after agent_settled, and a session_shutdown that overtook an in-flight post closed the session and let the straggler open a second one. The extension now serializes every post through a chain. Observability hooks are enqueued rather than awaited, so pi's critical path is still not charged. The gating hook does await, which also makes it wait for anything queued ahead of it -- worth the latency, because a tool span opened under the wrong turn is simply wrong. 3. A generic arrow function stopped the extension loading. `<T>(job) => ...` in a .ts file is ambiguous with JSX, and pi's jiti loader resolves it that way. pi collects extension load errors rather than aborting, so the extension silently did not run. Declared as a function instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
pi's session_shutdown carries reason: quit | reload | new | resume | fork. The extension ignored it -- the mirrored type did not even declare the field -- and forwarded a session end for every reason. On /reload that is wrong. pi tears down and rebuilds the extension runtime while the session itself continues with the same session id, so ending the gateway session there closes its session scope and the session_start that follows opens a second one. One logical session silently became two disconnected traces. The handling was also asymmetric: session_start's reason was already forwarded. Now: reload drains the queue and returns without ending the session; quit and the three session-replacement reasons end it and forward the reason, plus targetSessionFile when pi supplies one. Known limitation, documented at the handler: attemptIndex and turnSeq live in the factory closure and pi re-runs the factory on reload with moduleCache: false, so they restart at 0 mid-session. turn_seq is therefore monotonic within a runtime rather than strictly within a session. Rebuilding them would mean replaying the session. Adds test/lifecycle.test.mjs, which drives the extension's handlers against a stub gateway. Nothing exercised them before -- the existing suite covers the wire contract in isolation -- so attempt_index and turn_seq were implemented and demonstrated in a live trace but never pinned. Now covered: turn attribution across a re-entry (colliding turn_index, monotonic turn_seq), attempt-counter reset on agent_settled, strict post ordering, session id on every post, and the shutdown-reason matrix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`nemo-relay launch pi` printed a note asserting that model traffic is redirected by the extension registering a gateway-backed provider. It is not: nothing registers a provider yet, so pi's model calls go straight to the provider and the gateway sees no LLM traffic at all. The note now says what is actually true -- tool and turn activity is reported, model calls are not routed, and redirection needs the extension to register a provider because pi has no base-URL flag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The hook table grouped `turn_start` and `turn_end` into one row and claimed both carry `turn_seq`. Only `turn_start` does, alongside `attempt_index`; `turn_end` posts `turn_index` alone (`integrations/pi/index.ts:191-205`). Split the row so each boundary states what it actually carries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Closes the two remaining M3 gaps: RELAY-730's turn classification and compaction forwarding, and RELAY-729's attribution. 1. turn_start is classified. Only turn_end was mapped, so the gateway opened the turn implicitly on whichever event arrived first -- agent_start on the first attempt, turn_start on later ones -- and trailing agent_end/agent_settled marks opened an extra empty turn after every run. NormalizedEvent gains TurnStarted and ClassificationRules a turn_start list; Codex and Claude Code declare an empty one and keep their lazily opened turns. Classifying the open is necessary but not sufficient: on its own it adds a *leading* empty turn holding the agent_start mark, because mark() forces a turn open. So for harnesses that report a turn start, a mark arriving between turns is now recorded on the session scope instead. That is what removes the empty turn at both ends, verified by reverting the guard and watching the new test report three turn scopes where pi reported one. 2. Attribution reaches tool spans. tool_call and tool_execution_end carried neither attempt_index nor turn_seq, and turn_end carried turn_index but not turn_seq, so a tool call could be tied to an attempt only by reading arrival order -- which stops working the moment two attempts overlap. The extension now sends both on every attributable hook, and agent_settled sends attempt_index alongside the attempts count. Sending them was not enough. Mark events record the raw payload as their data, but tool spans are built from the extracted call id, name, arguments, result and metadata and drop ToolEvent::payload entirely, so the keys would have been accepted on the wire and silently discarded. PiPayloadExtractor::metadata now promotes the two numeric counters into event metadata. The same promotion puts attribution on the turn scope rather than only on a mark inside it, so "which attempt did this turn belong to" is answerable by walking the scope tree. pi's own turn_index is deliberately not promoted: the gateway assigns its own to the turn scope and the two would collide. 3. Compaction is forwarded. session_before_compact was in none of the three layers. Both halves are now forwarded: session_compact classifies as Compaction, which the runtime treats as proof the context was rebuilt (it marks the owning agent fresh), and session_before_compact stays a mark because it announces an intent any later-loading extension can still cancel. Its willRetry is the only advance notice pi gives an extension that the agent run is about to re-enter. Also drops tool_execution_start from the descriptor's hook_events -- the extension registers it, but only to remember a tool name for the matching end, and never posts it. Verified against a live pi 0.84.0 session with a real model: two turns, both turn_source: turn_start, the read span nested under its turn carrying attempt_index and turn_seq, and the run-level marks on the session scope with no empty trailing turn. Also driven through the hook route with three concurrent tools closing out of submission order and a forced re-entry, where pi's turn_index collides at 0 while turn_seq and attempt_index stay unambiguous. Green: 1163 + 12 + 102 Rust, 29 Node, tsc clean, pre-commit clean apart from cargo-deny/gofmt/go-vet, which are not installed on this machine. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
integrations/pi/ had zero CI: nothing ran its tests, nothing typechecked it, and it was not an npm workspace, so its package scripts were unreachable from the repo root. Adds it as a workspace member and a `test-pi` recipe, threaded through the same four layers OpenClaw uses: a `pi:` path filter, a `run_pi` output from ci_changes, an input on ci_node, and the pass-through in ci.yaml. The filter also covers crates/cli/src/agents/pi/ and the shared adapter, because the extension and the gateway share one wire contract and a change to either can break the other. `run_node` now also fires on a pi-only change, or the job that hosts the step would never start. Unlike test-openclaw, the recipe does not build the Node binding first: the pi extension is a sidecar HTTP client and loads no native addon. Docs: adds docs/nemo-relay-cli/pi.mdx and lists pi in the four places that enumerate agents -- the CLI about page, basic usage, the support matrix, and the root README. The page is explicit about what is not there: no persistent install because pi has no plugin marketplace, no LLM spans because pi's model traffic does not traverse the gateway, and no subagent representation. Two claims were corrected against the binary while writing the page. There is no `nemo-relay pi` shortcut subcommand -- pi runs through `nemo-relay run --agent pi` -- and NEMO_RELAY_PI_EXTENSION is required rather than optional, because pi extensions live in the user's own configuration directories and there is no Relay-managed location to fall back on. just docs-linkcheck passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The two limitations left on the extension README's follow-through list, both of which a reader hits without warning. Tool results are cut at 2000 characters before forwarding, so the gateway records what a tool returned rather than necessarily all of it. And pi has no nested-agent hook, so subagents are not represented at all -- including the multi-process case, where a child pi process running this extension resolves its own session id and appears as an unrelated session rather than as a subagent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPi support now spans the CLI agent registry, Rust hook and session handling, the TypeScript extension, diagnostics, CI, tests, and documentation. Pi hooks support lifecycle forwarding, policy gating, safe argument transforms, inline-shell handling, and conditional model routing. ChangesPi integration
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to The Pi integration can execute rewritten tool arguments without reapplying conditional policy checks to the final values, which may allow an unapproved action to run. The supported launcher path and session handling also have known failure modes, so merge should wait for these issues to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant Pi
participant RelayExtension
participant Gateway
participant PiHook
participant SessionManager
Pi->>RelayExtension: emit lifecycle or policy hook
RelayExtension->>Gateway: post session-aware hook
Gateway-->>RelayExtension: return allow, block, fault, or transform
RelayExtension->>PiHook: POST /hooks/pi for forwarded events
PiHook->>SessionManager: adapt and apply normalized events
SessionManager-->>PiHook: return HookEffects
PiHook-->>RelayExtension: return transformed tool input
RelayExtension-->>Pi: continue, refuse, or apply rewritten input
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
size:XL
Closes RELAY-732, the last real gap: pi's model calls now traverse the gateway,
so LLM spans land in the same trace as tool and turn spans and a Relay guardrail
can block a model call.
The mechanism is one call, not a provider implementation.
pi resolves a base URL per model from a generated catalog and has no base-URL
flag or generic environment override, so redirection has to happen inside the
extension. The ticket pointed at pi's `custom-provider-*` examples, which
register a `streamSimple` and re-implement a provider protocol. That is the
heavy path and it is not needed: `registerProvider(provider, { baseUrl })` with
no `models` makes pi rewrite the URL of every existing model for that provider
and keep their API, headers, costs and context windows
(`applyExtension`, core/provider-composer.ts:215, verified in pi's source rather
than taken from the doc comment). The extension stays a thin client.
Redirection is conditional, and the condition is the design.
The gateway forwards to one statically configured upstream per API family and a
client cannot override it per request -- inbound internal dispatch headers are
stripped, which is deliberate. So pointing a model at the gateway is only correct
when the gateway's upstream is the endpoint that model would otherwise call.
Redirecting an NVIDIA model into a gateway configured for api.openai.com does not
degrade to "no spans"; it breaks a session that worked a moment earlier.
The launcher therefore passes the gateway's own upstreams
(NEMO_RELAY_PI_{OPENAI,ANTHROPIC}_UPSTREAM, from ResolvedConfig, which
prepare_launch already had and pi ignored) and the extension redirects only on a
match. Skips are recorded as a `model_redirect` mark naming the reason --
upstream-mismatch, unserviceable-api, unknown-upstream -- so a trace without LLM
spans explains itself instead of looking broken. The decision is re-made on every
model_select. NEMO_RELAY_PI_REDIRECT=force skips the check, =off disables it.
Turn boundaries now block, because model traffic does not use the hook queue.
Reading the first redirected trace found a real defect: an LLM span opened under
the previous turn. pi sends model requests to the gateway directly over HTTP
while observability hooks go through the extension's serial queue, so the next
turn's model request beat our queued `turn_end` and was parented by the turn that
was still open. `turn_start` and `turn_end` are now awaited. Two local round
trips per turn buys correct parenting, on the same reasoning that already makes
tool_call await -- a span opened under the wrong turn is simply wrong. The
re-captured trace has every span closing inside the scope that opened it.
Verified against live pi v0.84.0 with a real model: three LLM spans nested under
their own turns alongside the tool span, and separately, with the example policy
plugin configured block_llms = true, a real guardrail rejecting a real pi model
call -- pi surfaced it as a clean 403 and the trace recorded the rejection as a
mark rather than a span, because the call never executed.
Also corrects two counts the ticket carried: pi ships 38 providers, not 39, and
6 of them speak an API the gateway has no route for, not 7 -- "Radius" is an
OAuth mode, not a provider.
Green: 1164 + 12 + 102 Rust, 42 Node, tsc clean, docs-linkcheck 0 errors,
clippy -D warnings clean, pre-commit clean apart from cargo-deny/gofmt/go-vet,
which are not installed on this machine.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The Status section linked out to an issue tracker that is not readable from this repository, and named its identifiers inline. Neither belongs in a public README: a reader outside the org gets dead links, and the identifiers carry no meaning for them. Says the same thing in prose instead. Nothing about the described behaviour changes -- it is still a proof of concept verified against pi v0.84.0, and model redirection is still conditional on the gateway fronting the model's provider. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The transform half of the pi tool-policy work. Guardrails could already block a
call; a request intercept could not change one, because the hook verdict
travelled only as an HTTP status code and there was no channel for a rewritten
payload.
The chain already existed. `tool_request_intercepts(name, args) -> Result<Json>`
is public in core and returns rewritten arguments; `start_tool` ran the guardrail
chain and never this one. So the gateway side is wiring: run the chain, use its
output as the span's arguments so the trace records what will execute, and hand
it back.
Handing it back needed one plumbing change. `pi_hook` builds its response before
`apply_events` runs, so `apply_events` now returns `HookEffects` carrying the
rewrite, and the pi adapter merges it into the body:
{"tool_call": {"tool_call_id": "...", "input": {...}}}
Absent a rewrite the body stays `{}`, which is what an allow has always been, so
an older extension is unaffected. `tool_call_id` is echoed so the extension can
refuse a body belonging to a different call.
Gated per agent. Codex and Claude Code have no way to execute a rewrite, so
running the chain for them would record arguments on the span that never ran --
worse than not running it, because the trace would then disagree with reality.
The extension constrains the rewrite rather than validating it, because it
cannot validate it.
pi validates arguments before the `tool_call` hook and never re-validates -- its
own types say so -- and the extension cannot read a built-in tool's schema: pi
exposes `tools` only on the `Extension` interface, which is an extension's own
registered tools. Of the three options the design considered (fetch the schema,
forward the schema, constrain the transform), the first two are therefore
impossible and the third is forced.
So a transform may rewrite the values of existing keys, preserving each value's
JSON type, recursively. Adding a key, removing one, changing a type or changing
an array's length is refused, which keeps the required keys and types the schema
already accepted. This is structural, not schema validation: pattern, enum and
range constraints are not checked and cannot be, and that limitation is
documented and asserted rather than glossed.
A refused transform blocks the call. Running the original arguments would
silently discard a policy decision, which is the failure the transform existed to
prevent, and it is a different axis from NEMO_RELAY_PI_FAIL, which governs an
unreachable gateway rather than one that answered with something unusable.
Verified against live pi v0.84.0 twice. A shape-preserving rewrite of a read
path executed: the model asked for alpha.txt, the gateway rewrote it to beta.txt,
and pi read beta.txt. A key-adding rewrite blocked every one of eight tool calls,
and the model reported it as a policy misconfiguration rather than a refusal of
its request, which is what the reason string is written to produce.
An earlier run of that second check appeared to pass the unsafe transform
through. It had not: the stub only rewrote paths containing alpha.txt, so when
the block worked the model retried with `cat alpha.txt` through bash, which the
stub left alone. The test was wrong, not the code -- logging every call rather
than only the rewritten ones showed it immediately.
Green: 1166 + 12 + 102 Rust, 52 Node, tsc clean, docs-linkcheck 0 errors,
clippy -D warnings clean, pre-commit clean apart from cargo-deny/gofmt/go-vet,
which are not installed on this machine.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
License DiffCompared against Lockfile license changesLockfile License ChangesRustAdded
Removed
Updated/Changed
NodeAdded
Removed
Updated/Changed
PythonAdded
Removed
Updated/Changed
Status output |
pi's `!cmd` and `!!cmd` never reach the tool registry, so `tool_call` does not fire for them and none of the tool gating covered them. They reach pi's `user_bash` hook instead, which is interceptable: a handler that returns a `BashResult` makes pi skip execution entirely and record that result. The extension now posts the command to `/hooks/pi` as a tool start named `user_bash`, so the same conditional-execution guardrail chain and the same 403 contract decide it. The name is deliberately not `bash`: a guardrail receives only the tool name and the arguments, so a policy can tell a command the user typed from one the model proposed only if the two arrive under different names, and "the model may not run shell commands" should not also stop a human typing `!git status`. The cost is that a policy covering both has to name both, which the docs state. pi gives the hook no block-and-reason contract, so a refusal is a synthetic failed `BashResult` that pi records as though the command had run: exit code 126 (found, but could not be executed), an attribution line, then the guardrail's reason verbatim. `NEMO_RELAY_PI_FAIL` governs this path too. A rewritten command is refused rather than run, because pi's result type can replace the result or the execution backend but never the command itself. `emitUserBash` wraps handlers in try/catch, so a throw here fails open and is invisible -- the opposite of `tool_call`. Every path returns an explicit decision, and the catch re-reads the failure policy rather than defaulting to open, so an explicit fail-closed setting is not overridden by an internal error. Two things beyond the gate itself: - Tool events for a harness that reports its own turn start no longer open a turn when none is open. Inline shell is the first tool event that can arrive between turns -- a command typed at an idle prompt -- and opening a turn to hold it invented a boundary pi never reported. This is the rule `mark` already applies, reached from the tool side. It also changes where a `tool_execution_end` that lands after `turn_end` attaches for pi: on the session scope rather than in a manufactured turn. Codex and Claude Code report no turn start and are unaffected. - The descriptor's `hook_events` gains `tool_arguments_transformed`, which the extension has been posting since argument transforms landed. The list is an inventory of what the extension posts, so a test now pins the exact set. Verified live against pi v0.84.0 driven in RPC mode: an allowed command runs and its span sits directly under the session scope; a command refused by the `examples.rust_native_policy` plugin never executes, and the reason reaches the user verbatim with exit code 126; an unreachable gateway under fail-closed refuses with the infrastructure-fault wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
github-actions
Bot
added
size:XXL
Three M5 items, all shaped by the same problem: the ways this integration fails are quiet ones. **A doctor preflight for the load path.** pi adds project-scoped extensions to its candidate set only when the project is trusted, and `-p`, `--mode json` and `--mode rpc` never prompt for trust. The skip is a bare conditional rather than an error path, so pi does not treat it as a failure and never reports it -- and the extension cannot report it either, because it is not running. `nemo-relay doctor pi` now warns when an extension sits on a trust-gated path, and probes the gateway the extension will post to. `AgentInfo` gains a `checks` list, empty for Codex and Claude Code and omitted from their JSON entirely: their setup is written by `nemo-relay install`, so `hook_status` already describes it. pi's is installed by the user, wherever they like, and pi's own trust rules decide whether it loads -- a finding that deserves its own status rather than a sentence in a summary. The gateway probe resolves its URL from the resolved `bind` when the environment variable is unset, because the launcher sets that variable *from* the config: a check that only read the variable would report a working gateway as down for anyone who changed `bind`. It classifies rather than just connecting, so "your gateway is down" and "something else owns that port" are told apart, and it never returns `Fail` -- doctor running before the gateway starts is the normal case, not a broken machine. It is skipped under `--offline`, and for an agent that is neither configured nor asked about, so a machine that does not use pi does not spend the timeout budget dialling a gateway nobody mentioned. **Test harness and coverage.** Both test drivers returned the *last* handler's result; pi returns the *first*. That inverted the trap this extension documents in two places, so the harness itself could not catch a regression in preemption behaviour. There is now one shared driver with pi's semantics, and the preemption case is pinned: an extension ahead of ours decides, and the gateway never sees the call. Filled the gaps that left: the `tool_call` gate had no end-to-end test at all -- every component it composes was pinned and the handler wiring them was not, which is easy to miss precisely because the coverage either side looks complete. Also concurrent tools closing out of submission order, unpaired tool boundaries, compaction-driven re-entry, a slow gateway on both gates, and the bound on what an interrupted session loses. **Two limitations documented rather than papered over.** pi registers no SIGINT handler in any mode, so Ctrl+C in a headless mode kills it with teardown never running; what is lost is bounded to marks queued since the last awaited hook, because both gates and both turn boundaries block on their round trip. And a broader one, found while costing the tool-result policy gap: a tool execution intercept registered by any plugin never runs under the CLI gateway. The registry has exactly one consumer, `tool_call_execute`, which the gateway does not call -- it applies policy through the hook path. Guardrails and request intercepts do run there, because both have standalone runners; there is no response-phase equivalent. Worth stating where a user meets it. Also adds `integrations/pi` to the version bump. It is private and unpublished, so this changes nothing today -- it is there so the version cannot already be stale on the day that changes, since a workspace member absent from that list drifts with no lockfile mismatch and no CI failure to catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`pi install` resolves a local path or a git URL as readily as an `npm:` specifier, so the package being unpublished does not cost a route -- it costs a spelling of one. Publishing would buy that spelling in exchange for an npm namespace, a build step (the sources are TypeScript nothing compiles today) and release wiring, so `private: true` stays, and now says so on purpose rather than reading as an oversight. Both install routes are spelled out with the commands to run, since "user scope" was previously stated as a rule without showing what it looks like. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Two holes in extension resolution that a review found, both verified against
pi v0.84.0 rather than reasoned about.
**`-e` adds to pi's extension set; it does not replace it.** My own comment
claimed the load was safe because pi de-duplicates the merged command-line and
discovered sets -- true, but only by *canonicalized path*. Two distinct
checkouts of one package are two identities to pi (`getPackageIdentity` gives a
local source `local:<path>`), so an explicit `NEMO_RELAY_PI_EXTENSION` pointing
at checkout A while checkout B is installed loads both: two factory calls, two
handler maps, every hook posted twice. A duplicated `turn_start` closes the turn
its twin just opened as superseded, and the inline-shell gate decides one
command twice with the second verdict the one the user gets. (The model-tool
gate is unaffected -- `start_tool` returns early on a known call id.) The
launcher now refuses and names both copies, and `doctor` reports the same
condition, which is reachable with no Relay command involved at all: pi scans
its extensions directory and its recorded packages independently.
An in-extension guard was considered and rejected. It would have to tell a
sibling copy from a runtime pi has since torn down, and the only signals for
that are pi internals -- get it wrong and the extension registers nothing after
`/reload`, a silent total loss of governance, strictly worse than the duplicate.
**A `packages` entry can be an object, and only strings were read.** pi accepts
`string | {source, autoload?, extensions?, ...}` and resolves both through one
path. This was not a hand-edit shape: pi's own configuration selector rewrites
a string entry into the object form the moment a user toggles any resource of
that package, so one keystroke in pi's UI made `doctor` report an installed
extension as missing and made `run --agent pi` refuse to start.
Both forms are read now, and the two filter shapes that leave a package's
extensions disabled -- an empty `extensions` array, and `autoload: false` with
no patterns -- are reported as such rather than as absent or as a plain Pass.
The launch path deliberately ignores that flag: `-e` applies no settings
filter, so the launcher still instruments a session the user's own `pi` runs
are missing.
Three pieces of user-facing text were left contradicting the code by the
previous round, and are corrected here because they are its consequences:
- The trust warning told a project-scoped user to run `nemo-relay run --agent
pi` instead -- the one thing that now refuses a project-scoped copy. It names
the two routes the launcher does resolve.
- `nemo-relay launch pi` is not a command and never was. It appeared in the
marketplace-unsupported error, which a dozen call sites return, and in the
extension's own header.
- The README called `NEMO_RELAY_PI_EXTENSION` an override. It is the
highest-precedence *candidate*: ignored unless the path exists and its
manifest names this package, after which resolution falls through.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Contributor
Author
|
/ok to test 3a9fa20 |
End-to-end testing found `nemo-relay run --agent pi` returning 401 on every model call the moment redirection fires -- the exact path the docs advertise with "Redirected; LLM spans appear under the turn". A gateway started by `run` authenticates its own client before a request intercept can rewrite the route, and rejects a provider call that does not present this invocation's credential. Claude Code's launcher injects it through `ANTHROPIC_CUSTOM_HEADERS`; Codex reads it through its `env_http_headers` provider configuration. `prepare_launch` already exports `NEMO_RELAY_PROXY_CREDENTIAL` for *every* agent, so the value was sitting in pi's environment the whole time -- the extension simply never read it, and `registerProvider` set only the session join key. It goes on the registration, beside the session id, for the same structural reason and a stronger one: the credential authenticates this invocation, so a provider the gateway does not front must never see it, and `registerProvider` runs only on a redirect. Absent -- a standalone `nemo-relay --bind` daemon requires no credential -- the key is omitted rather than sent empty. Not a `NEMO_RELAY_PI_*` name on purpose: the launcher exports one variable for all three agents, and a pi-specific alias would be a second name for one value. Verified: the new test fails against the previous source. The gateway probe is unaffected -- only provider passthrough is authenticated, not `/hooks/pi`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`nemo-relay agents --json` reported `status: "pass"` for a pi whose only
install is project-scoped -- the exact silent skip the preflight exists to
catch -- while its own nested check said `warn`. The fold was gated on
`configured || target_requested`, and `configured` means only that
`[agents.pi] command` is set in Relay config, which almost nobody sets.
That gate is right for readiness ("the hook config is missing" should not make
a bare `doctor` complain about an agent you do not run) and wrong for a
preflight finding, which is *evidence*: every warning branch already requires
the extension to be installed on this machine, and a machine without one
reports `Info`, which folds either way. `doctor pi --json` was already correct;
now `agents --json` agrees with it.
Also documented, from the same end-to-end round:
- **A slow gateway multiplies, it does not just delay.** Posts are serialized
by design, so a gating hook waits out everything queued ahead of it: against
a gateway that holds requests, the first gate of a session pays
`NEMO_RELAY_PI_TIMEOUT_MS` once per queued post, not once. The queue stays --
it is what keeps session and turn boundaries derivable from arrival order --
but the cost now appears where the value is chosen.
- **The shipped `rust-native-plugin` example blocks every pi tool call.** Its
intercept tags arguments with two added keys, and an added key is exactly
what the shape invariant refuses. Correct on both sides, and previously
written down on neither, so it is noted in the pi transform section and in
the example's own README. The example is deliberately not changed: adding
keys is what it exists to demonstrate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`resolveFault` opened every fail-closed block with "could not be reached", including the four cases where the gateway did answer -- HTTP 413 or 500, a 403 without the guardrail marker, an unparseable 2xx body -- and the case where the gateway was never consulted because the inline-shell handler itself threw. The string reaches the model, and the user, verbatim. Live against a gateway returning 413 it read "could not be reached ... Details: gateway returned HTTP 413": the `Details:` line was right and the sentence sent the reader to debug a socket that was working. A fault now carries `reached`, and the opening picks from it. The tail is unchanged, because it is the part a model has to act on and it is the same either way: nothing judged the request, so the request is not what to change. The same round asked for tool arguments to be bounded the way results are. **Not done, because it would be unsafe.** The `tool_call` post is the gated one: a guardrail decides on exactly those arguments, and a request intercept sends a rewritten copy back for pi to execute. The shape invariant checks JSON types and key sets, not content -- so a truncated `content` passes it and a `write` lands on disk cut short. A result has no path back into execution, which is why only results are bounded. That asymmetry, and the 20 MiB gateway ceiling that is the real bound on arguments, are now written down, and a test pins that the invariant cannot tell a shortened string from the original. Also corrects a claim in the transform test header that this PR had already retracted eleven commits earlier: the tool schema is reachable, and not using it is a choice about staleness rather than a limitation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Five findings, four of them on code the last two rounds added.
**Both source readers stopped at the first match.** pi resolves every distinct
package source -- two `packages` entries are two identities to
`getPackageIdentity`, and `collectAutoExtensionEntries` walks a whole
directory -- so two copies inside *one* source both load and post every hook
twice, while the duplicate check saw one. Both readers now return every copy.
The directory scan is restricted to the shapes pi accepts as an entry and
sorted, because `read_dir` order is undefined and the launcher's choice must
not vary run to run.
**The conflict predicate modelled neither direction of pi's active set.** It
counted copies pi's own settings switch off -- a hard launch refusal over a
copy that was never going to register a hook -- and ignored project-scoped
copies, which a *trusted* project does load beside `-e`. It now refuses only on
copies pi is certain to load. The project case cannot be decided from here at
all (`-a` overrides trust, `defaultProjectTrust` pre-answers it, session-only
trust persists nothing), so it becomes a launch note rather than a refusal:
refusing would block every launch in an untrusted project, and `-p`,
`--mode json` and `--mode rpc` never prompt, so untrusted is the common state.
**A non-empty `extensions` filter was read as "enabled".** pi's own
configuration selector disables a resource by writing `-<path>`, a force-exclude
pi applies last and unconditionally -- so one keystroke in pi's UI left doctor
reporting Pass for a package pi loads nothing from. Exact `+`/`-` patterns are
decided now, against the entry points the manifest declares. Globs are not:
pi expands those with `minimatch`, and a false warning costs more than a
missing one, so anything undecidable still reads as enabled.
**Every scope end now names its own closer.** `close_agent_scope` had no
metadata channel while `close_turn_scope` did, so pi's session end repeated
`session_start` and bucketing ATOF by `hook_event_name` never yielded a session
end. Shutdown and sweep closes still pass `None`: no hook stands behind them.
**A pi session that only ever held a mark is now swept.** `is_idle_for`
required an open turn, because the sweeper's job is to close an idle *turn* --
but pi's marks open the session scope instead, which is exactly what
`has_explicit_turn_start` is for, so those sessions were resident until process
shutdown while Codex's and Claude Code's were swept. Narrowed by two guards: a
session pi announced is a user idling between turns and is left alone, and
`turn_index == 0` protects one whose `session_start` was lost but which did
work. The shared non-object-payload question stays deferred -- it changes all
three hook routes, and a plain `{}` defeats the obvious fix.
Also repairs two doctor strings whose line continuations were lost when they
were written, rendering with runs of stray spaces mid-sentence.
Every new test was run against the previous source first; six of them fail
there.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`0"`, `12"`, `102"` and `1206"` are shell-redirect debris -- a misquoted `grep -c` wrote its target into a file named after the count. Each holds one line of `target/debug/deps/...` build output under an absolute local path. They were swept in by a `git add -A` in `01411c19`. They had been sitting untracked in the tree, were noticed, and were judged to predate the branch and left alone -- and then committed by the next blanket add, which is exactly the gap between "not mine" and "not staged". Removed rather than rewritten: they are already pushed, so an amend would not unpublish the path, and rewriting published commits on a branch under review is the manoeuvre that previously landed eight unsigned commits here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Five findings, four of them on the discovery code the last round added. **Selecting the first site could manufacture the duplicate it then refused.** `launchable_extension_path` took the first ungated site regardless of its filter, so with a disabled copy recorded before an enabled one it chose the disabled one -- and `-e` applies no settings filter, so that choice *re-enabled* it and the enabled copy became a genuine second load. The launch was refused over a duplicate the choice created. It prefers a copy pi already loads now, and falls back to a filtered-off one only when that is all there is, because `-e` still makes that work. The doctor's duplicate check asks the same question of the same copy rather than of whichever site sorts first. **`disabled_by_settings` was a bool, and the third state was the common one.** pi sorts patterns into force-include, force-exclude, exclude and include, and only the first two are exact strings; the rest are globs it matches with `minimatch`. An include list that never names our entry (`["other.ts"]`) or an `autoload: false` delta that adds something else back are both decidable and were reported as loaded -- and a genuine glob was reported as loaded too, which is the claim this module exists to stop making on no evidence. The verdict is now `Loads` / `Excluded` / `Undecided`, and doctor warns on the third rather than passing. **The project-copy launch note described copies that cannot double a trace.** It included entries pi's settings switch off, and entries canonically identical to the launched package, which pi de-duplicates by path. **`reached: false` still conflated three faults.** A timeout is not an unreachable gateway -- it may be up and slow, and posts are serialized so a gate also waits out its queue -- and a handler failure is not a transport result at all. The fault carries a four-way origin now (`transport`, `timeout`, `response`, `handler`) with one opening each and the same tail, since nothing judged the request in any of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`settings.json` carries an `extensions` array alongside `packages` -- "local extension file paths or directories" -- in both scopes, and nothing here read it. A user who registered the extension that way got "pi extension not located" from `doctor` and a hard refusal from `nemo-relay run --agent pi`, for a setup pi loads without complaint. It was also invisible to the duplicate check, so it could double-load undetected. Easy to miss, and worth saying why: `SettingsManager::getExtensionPaths()` exists and has no callers, so the key reads as dead until you find the generic loop over pi's four resource types that consumes it by name (`resolve`, pi `v0.84.0`, `core/package-manager.ts:906-931`). I nearly concluded it was dead config on the strength of that grep. Both entry shapes are handled, because pi treats them differently: a *file* entry is the extension, a *directory* entry is a container it walks (`collectResourceFiles` -> `collectAutoExtensionEntries`). A pattern entry (`+`, `-`, `!`) filters the collected set through the same globbing `packages` filters use, over a file set this module does not enumerate pi's way, so any pattern present yields `Undecided` rather than a guess. Found while answering a scoping-doc question about how the extension is registered and activated, which is the honest reason it surfaced now: nobody had enumerated pi's registration routes end to end since the first round. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Five `⚠️ ` markers across four TypeScript and mjs doc comments. The bold lead already carries the emphasis in every one of them -- "**Never throw from the handler.**", "**This is a structural guarantee, not schema validation.**" -- so the glyph added weight to text that was already the loudest thing in the block. Scoped to comments this branch introduced. The `✓`/`✗` in `diagnostics/render.rs` and the configure wizard stay: those are rendered status glyphs in `doctor` output, not decoration, and they predate this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The descriptor comment said the floor is "the version the integration was verified against rather than a lower bound that is expected to keep holding" -- and then the shared validator used it as a plain floor, so `validate_version_output` accepted every stable version above it. `doctor` called pi 0.85.0 supported for a host that can move a hook shape in a minor release, where the symptom is missing spans rather than an error. The comment admitted the semantics were wrong for pi and the code kept them. `AgentDescriptor` gains `verified_through`. `None` for Claude Code and Codex, whose minors are additive, so the floor really is a floor and nothing changes for them. `Some((0, 84))` for pi. **Reported, not enforced.** An upper bound in `validate_version_output` would become `CliError::Launch` at `process::launcher::validate_agent_version` and refuse to start on pi 0.85.0 -- forcing a downgrade of pi to use Relay at all, over a version that has not been shown to be broken. So the band is a third outcome rather than a second error: below the floor errors, 0.84.x passes clean, above warns in `doctor` and logs a warning at launch. Verified against the binary across all five bands, not just in unit tests: 0.83.0 fails "is unsupported"; 0.84.0 and 0.84.9 are clean; 0.85.0 and 1.0.0 warn with the band named. The test also pins that Claude Code and Codex stay silent on a 99.0.0, so adding this field cannot start warning for them by accident. Docs aligned: "0.84.0 or newer" was the claim the code was making and neither was right. pi.mdx, the support matrix and the extension README now say 0.84.x, and say what happens above it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
The page explained itself by comparison -- a "How It Differs From Codex and Claude Code" section -- which neither sibling guide does, and which dates the page to the moment pi was the new one. Removed, with its load-bearing content kept where a reader looking for that fact would go rather than where the comparison put it: - The sidecar shape moves into the intro, stated on pi's own terms: hooks cannot be injected from outside the process, so they originate inside an extension, and all policy stays in the gateway. - "No persistent plugin install" moves into Requirements, next to the other install routes. - The queued round-trip cost, including the timeout multiplication, moves into Limitations, where an operator choosing `NEMO_RELAY_PI_TIMEOUT_MS` will meet it. - The pointer to Model Redirection is dropped; that section already says it. Structure now matches both siblings: Requirements, Transparent Run, Standalone Gateway, Captured Events, the pi-specific policy sections, Smoke Test, Verify Export, Troubleshoot LLM Lifecycle, Limitations. Two consequences of that: - `Troubleshoot Missing LLM Spans` becomes `Troubleshoot LLM Lifecycle`, the name both siblings use for the same section. - The three limitations that were free-standing top-level sections in the middle of the page -- gate authority, tool-result policy, interrupted sessions -- become subsections of one `Limitations` block at the end, matching Claude Code's `Hook Limitations` and Codex's `Cold-Start Limitation`. Top-level headings drop from 16 to 13. No prose was rewritten beyond the moves and the two paragraphs named above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Two problems, one of them mine from the restructure. **"No persistent plugin install" was the wrong claim.** `pi install <source>` creates exactly that -- a user-scoped extension pi loads on every later run, and the one the launcher then finds without any variable set. What does not exist is *Relay-managed* installation: `nemo-relay install pi` is unsupported because pi has no marketplace for Relay to install into. Both the pi guide and the support matrix now say that, and say that installing it yourself does persist. **"No-install local observability" was inherited and false here.** The phrase is copied from the Claude Code and Codex guides, where it is true because Relay injects hooks per run. It cannot be true for pi: a hook can only originate inside the extension, so on a clean machine `nemo-relay run --agent pi` fails until the extension is installed, copied, or pointed at. The section now says so, and says the command names the routes that fix it. **Fourteen headings lost their preceding blank line** when the restructure reassembled the page: sections were joined with a single newline, so each heading landed directly against a paragraph, fence, table or `</Warning>`. Fern renders it, which is why the tests stayed green and the linkcheck passed -- nothing checks this. Restored, and the sibling guides are the reason to care: the source should read the same way across the three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Resolve three conflicts: - `package.json` / `package-lock.json`: main added the `examples/language-binding-plugin/node` workspace while this branch added `integrations/pi`. Keep both. The lockfile is regenerated with `npm install --ignore-scripts` rather than hand-merged; it differs from main's only by the pi workspace entries. - `examples/rust-native-plugin/README.md`: main rewrote the page and dropped the section this branch had appended its pi caveat to. Take the rewrite and re-place the caveat at the end, naming `documentation_tool_request` now that the surrounding registration list is gone. The behavior it warns about is unchanged: `tag_tool_request` still inserts `plugin_tag` and `plugin_tool` into a tool's arguments, which the pi extension rejects as a shape change. Also bump `integrations/pi` to 0.9.0. Main bumped every Node package from 0.8.0 in this range but could not touch a file that did not exist there yet, so the textual merge left pi behind at 0.8.0 with no conflict to show for it. `set_node_package_versions` already lists pi, so this only backfills the bump that recipe would have applied. Verified: `cargo test -p nemo-relay-cli` (1345 tests), the pi extension's typecheck and 97 tests. Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`nemo-relay claude` and `nemo-relay codex` existed; `nemo-relay pi` did not, and nothing was ever decided against it. The pi guide was originally drafted *claiming* the shortcut existed, `--help` disproved it, and the doc was corrected to match the binary -- so the absence became documented fact without anyone weighing whether it should exist. It is three lines because `easy_path` is fully agent-neutral and the setup wizard already runs off `CodingAgent::ALL`, which has included `Pi` since the integration landed: pi is detected on PATH and gets an `[agents.pi]` table like the others. The change is an enum variant, a dispatch arm, a `log_name` arm and help text. **The one asymmetry is stated rather than papered over.** Claude Code and Codex take their hooks from the launcher, so their shortcuts are install-free. pi cannot: hooks originate inside an extension the *user* has installed, so on a clean machine `nemo-relay pi` fails exactly as `nemo-relay run --agent pi` already does, with an error naming the three install routes. The subcommand summary, the long help and both doc pages say so. This makes an already-reachable failure easier to reach; it does not add one. `basic-usage.mdx` also loses its blanket "no-install local observability" claim, which was true of the two shortcuts that existed when it was written and is not true of this one. The new integration test asserts on the resolved launch plan rather than on the parse, because every shortcut takes the same `EasyPathCommand` and a wrong `CodingAgent` in the dispatch arm still parses, still launches, and shows up only as an agent that was never instrumented. Confirmed it fails when that arm is pointed at `CodingAgent::Codex`. Verified: `cargo test -p nemo-relay-cli` (1346 tests), `cargo clippy --all-targets`, `cargo fmt --check`, and pre-commit on the changed files. Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
`nemo-relay install pi` was rejected at the clap layer -- `<HOST>` accepted only
`[codex, claude-code, all]`, so the user saw `error: invalid value 'pi'` with no
guidance. The reason recorded in the code was that pi has no plugin marketplace,
which is true, but "no marketplace" is not "nothing to install into" and the
second does not follow from the first. pi auto-discovers every package directory
under its own agent directory. Relay can write one.
**The file drop, not `pi install`.** Shelling out to `pi install <source>` is
usually called the safer route because it respects pi's own `withLock` around
`settings.json`. That reasoning is entirely about the settings write, and this
route has none: a directory under `<agent dir>/extensions` is discovered without
any settings entry. No lock to contend for, no package-manager semantics to
inherit, no requirement that `pi` be on PATH, and an uninstall that removes a
directory Relay created rather than un-editing an array under someone else's
lock. It is also the exact route the pi guide already documents as `cp -r`.
**The extension is vendored into the binary, and it had to be.** `nemo-relay-cli`
is published to crates.io and Cargo packages only files under the crate root, so
`include_str!("../../../../../integrations/pi/index.ts")` compiles in a workspace
checkout and then fails to build from the published tarball. A build script that
copies into `OUT_DIR` fails for the same reason -- the source it would copy is
not in the tarball either. So `crates/cli/assets/pi-extension/` carries the seven
files pi actually loads; `integrations/pi` stays the source of truth, `just
sync-pi-extension` refreshes the copy, and four tests fail on drift. Verified
with `cargo package --list` that all seven ship. Tests, tsconfig and the README
are not vendored: no runtime role, and the README would force a re-sync on every
wording change.
**Every install records what it wrote.** The guide's own `cp -r` lands in this
same directory, so "a directory exists here" cannot mean "Relay put it there".
Install writes a manifest of paths and hashes; uninstall removes only files that
still match, keeps anything edited along with the directory holding it, and
refuses outright on a directory with no state file -- `--force` included, because
Relay cannot tell an unmanaged copy from one you edited.
**Installing can refuse, and that is the point.** pi de-duplicates its extension
set by path rather than by package, so a second copy is a second package: every
hook fires twice, each turn closes as superseded by its own duplicate, and the
launcher already refuses to start in that state. Creating it here would break a
setup that currently works, so install checks first and names the other copy.
Also: `doctor` gains a staleness check, because pi ships breaking changes through
minor releases and an extension from an older Relay fails by going quiet rather
than by erroring. `install all` now includes pi when `pi` is on PATH, which
changes what that command does. The messages that used to send users to `pi
install <path to integrations/pi>` -- launcher, doctor, agents report -- now lead
with `nemo-relay install pi`.
Docs: pi.mdx said "Relay-managed installation is unavailable", which was true
when written and is not now; the support matrix said the same. `install pi` does
not check pi's version, unlike the other two hosts, because it never runs pi --
plugin-installation.mdx no longer claims otherwise.
Verified: `cargo test -p nemo-relay-cli` (1363 tests, 13 of them new install
coverage plus 4 drift tests), clippy `--all-targets`, `cargo fmt --check`, and
pre-commit `--all-files` -- clean apart from cargo-deny, gofmt and go, which are
not installed on this machine. Exercised end to end against a scratch
`PI_CODING_AGENT_DIR`: install, dry run, reinstall, upgrade from a stale version,
uninstall, the foreign-directory and duplicate-copy refusals, and a round trip
where `nemo-relay install pi` is followed by `nemo-relay pi` resolving the
extension with no variable set.
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Closes the last of the parity gap. Claude Code and Codex take their hooks from the launcher, so setup is the final step they need. pi's hooks can only originate inside an extension, so before this the wizard would finish, congratulate the user, and then hand them a `nemo-relay pi` that does not work. Setup now offers the install it needs. **A direct agent check, not a new abstraction.** The wizard already branches on one agent -- `print_codex_api_key_guide` -- and pi is the only host with anything to provision. A provisioning trait with one implementor would be speculative generality; this follows the shape that is already there, and all the pi-specific logic stays in `agents::pi::install`. **It offers only when pi has no copy at all, in any scope.** An existing install needs no offer, and a project-scoped copy -- the trap the guide warns about twice -- must not quietly become the reason a *second* copy appears, because two copies double every hook and stop the launcher outright. `doctor` reports a project-scoped copy on its own; setup stays out of it. The install runs its own duplicate guard regardless, so the two cannot disagree into a broken state. **Nothing here can fail the wizard**, and that is structural rather than defensive: `offer_pi_extension` returns unit, so there is no error to propagate. The configuration is already saved by the time it runs, a declined or failed install still leaves a working setup for the manual routes, and every path prints `nemo-relay install pi` for later. An interrupted prompt is a skip, exactly as it already is for plugin setup. One scoping limit is worth stating plainly, and the docs now state it: the wizard only runs when no Relay configuration exists yet. On a machine already configured for another agent, `nemo-relay pi` skips setup entirely and the offer never appears -- so `basic-usage.mdx` and `pi.mdx` say to run `nemo-relay install pi` in that case rather than implying the wizard always catches it. `integrations/pi/README.md` also leads with the managed install now. Verified: `cargo test -p nemo-relay-cli` (1365 tests, 2 new covering when an offer is made), clippy `--all-targets`, `cargo fmt --check`, and pre-commit `--all-files` -- 30 hooks pass, and the three that fail (cargo-deny, gofmt, go) are tools absent from this machine rather than anything in this change. Drove the accept path through a pty on a clean `PI_CODING_AGENT_DIR` and `XDG_CONFIG_HOME`: the wizard saves the config, offers the install, writes all seven files, and reports the load path it resolved. Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Two P1 regressions from the pi install feature, both found in review, both
created by it.
**`doctor` aborted for every agent after `nemo-relay install pi`.**
thread 'main' panicked at crates/cli/src/agents/mod.rs:226:25:
internal error: entered unreachable code: pi has no plugin marketplace
One line caused it: teaching `installed_integrations` about pi so `uninstall all`
could see a managed install also handed pi to the *marketplace readiness
collector*, whose `PluginLayout::new` calls `marketplace_manifest_relative` --
`unreachable!()` for pi. So it was never scoped to pi; `doctor claude --json`
aborted too. One `install pi` broke `doctor` for everything, and the installer's
own output tells the user to run it.
A third path was worse than reported: `doctor --plugin pi` panics with **nothing
installed at all**. Adding `pi` to the `<HOST>` value enum for `install` also made
it a valid `--plugin` argument, and every path behind that flag is
`unreachable!()` for pi -- broken from the moment the enum changed.
Fixed by splitting the two questions that were sharing one function.
`installed_integrations` means "has marketplace plugin state" and now excludes pi
explicitly, with a doc comment recording that putting pi back is an abort rather
than a wrong answer. `uninstallable_integrations` means "has state Relay can
remove" and includes pi; `uninstall all` is its only caller. `doctor --plugin pi`
refuses in words and points at `nemo-relay doctor pi`.
**Installing could create the duplicate the launcher refuses to start over.**
`install pi` used `conflicting_extension_site`, which deliberately ignores
project-scoped copies -- correct for the launcher, wrong here. Installing inside a
project holding `.pi/extensions/<copy>` succeeded and left both, and a trusted
project then loads both, doubling every hook and every policy gate.
The asymmetry is the point. The launcher only notes a project copy because
refusing there would block every launch in an untrusted project over a copy that
will not load. Installing is the opposite case: it is the act that creates the
second copy. Declining to create a problem is a lower bar than declining to run
because one exists.
That guard is partial by construction, and the code comment and `pi.mdx` both say
so: it reads only the current directory, so installing from elsewhere leaves the
same project copy in place. It catches installing from the project you work in.
**Why 1365 tests missed both.** None of them ran the binary with a managed install
on disk -- every install test called the function directly, and every doctor test
ran without one. Three CLI regression tests now close that: `doctor` with a
managed install across three invocations including another agent's, `--plugin pi`
refusing rather than aborting, and the project-scope refusal under a controlled
cwd. The first was mutation-checked by restoring the old `installed_integrations`
and confirming it fails with the original panic.
Verified: `cargo test -p nemo-relay-cli` (1369), clippy `--all-targets`,
`cargo fmt --check`, pre-commit `--all-files` -- clean apart from cargo-deny,
gofmt and go, which are not installed on this machine. Reproduced all three panic
paths before the fix and confirmed each is gone after, plus `uninstall all` still
removing a managed pi install.
Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Review round on the pi CLI surfaces. Five defects, three of them created by this work. **A tool span could outlive the scope that opened it.** `close_turn` returned early when no turn was open, *before* closing active tools -- but `ensure_tool_scope_started` parents a span to the session scope when a host with explicit turn boundaries has no turn open, which is exactly pi's inline shell between turns. The session then closed over a span that never ended. Reachable with no signal at all: the extension posts `user_bash_end` through a fire-and-forget path that swallows failures, so one dropped post plus `/quit` was enough. The three closers drain empty collections, so running them before the turn check costs nothing when there is nothing to close. **Uninstall could delete files outside the extension directory.** Every recorded path was joined to the install root and removed, and `Path::join` with an absolute path replaces the root outright -- so a `.nemo-relay-install.json` naming `/etc/...` or `../../..` turned `uninstall pi` into an arbitrary-file delete for the invoking user. The recorded hash was no guard: it is compared against whatever sits at the resolved path, so pairing the traversal with the victim's own digest satisfied it. Paths are now rejected unless every component is `Normal`, and removal additionally canonicalizes the parent and checks containment, which closes the symlinked-parent route as well. **`install all --install-dir` regressed for Codex and Claude Code users.** `All` gained pi, pi rejects `--install-dir`, and that rejection failed the whole command: both marketplace hosts installed correctly and the command still exited 1. pi is now dropped from an `all` run carrying the flag; an explicit `install pi --install-dir` still errors, because there the flag is the user's stated intent. **Install and uninstall could dead-end with a false diagnosis.** Keeping an edited file left a directory with no state and no manifest, which read as somebody's extension -- so every reinstall was refused, `--force` included, while being told the copy already worked. It did not: pi cannot load a directory with no `package.json`. That case is now classified separately and `--force` installs over it. Relatedly, `modified_files` hashed through `read_to_string`, so a file edited into anything non-textual was unreadable, collapsed to "unmodified", and deleted -- the exact file the keep-edited rule exists to protect. It hashes bytes now, and an unreadable file is kept rather than assumed ours. **The drift guard was not wired to the paths that cause drift.** `run_rust` omitted the `pi` filter and `crates/cli/assets/pi-extension/**` matched no filter at all, so an `integrations/pi/**`-only PR could ship a stale vendored extension with every check green. Both fixed; re-syncing the vendored copy in this commit exercised it. Also from the audit: `NEMO_RELAY_PI_TIMEOUT_MS` above 2^31-1 wrapped to ~1 ms and made every gated call fault, which under the default fail-open policy silently stops enforcing -- it is clamped now. A fail-open `user_bash` fault recorded `policy-allowed`, indistinguishable from a real allow, and now records `fault-allowed`. The hook-header comment claimed the gateway strips the session header; it strips it on the provider-passthrough route and *reads* it on the hook route. Docs, all verified against the binary or the source: the launcher does not refuse over a project-scoped copy (it notes one), `doctor pi` is cwd-only like the install guard rather than global, a clean machine gets the wizard's offer rather than a failure, the example plugin injects `plugin_tag`/`plugin_tool` rather than two identifiers that exist nowhere, pi 0.84 prints extension-load failures and exits rather than swallowing them, `user_bash_end` marks the policy decision and not command completion, "tool and turn activity always reach Relay" is qualified by fail-open, pi has no *marketplace* manifest rather than no manifest, the 2000-character tool-result truncation is on the docs site rather than only in the extension README, and the pages that enumerated only Claude Code and Codex now include pi. Verified: `cargo test -p nemo-relay-cli` (1374), 97 extension tests, `tsc` clean, clippy `--all-targets`, `cargo fmt --check`, pre-commit `--all-files` -- clean apart from cargo-deny, gofmt and go, absent on this machine. Both new regression tests were mutation-checked against the original code: the span test reproduces the reported trace exactly, and the traversal tests confirm the victim file survives. Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
Second review round on the pi CLI work. Four of these are regressions from `6267dabf` -- the previous round's fixes -- which makes three rounds running where a fix introduced the next defect. **`--force` could overwrite a working extension.** Classifying the install path used a manifest-name test, so "the manifest does not name Relay" was read as "nothing of value here". It is also every manifest-less extension pi loads from a bare `index.ts` (`collectAutoExtensionEntries`, pi `v0.84.0`) and every third-party package. Before `6267dabf` all three were refused; after it, `--force` overwrote two of them, against the guarantee the guide makes. Reverted rather than narrowed. Detection now asks whether pi would load *anything* here -- any manifest, or an `index.ts`/`index.js` -- and both classes are refused with `--force` included. The leftovers variant survives only so the refusal can say what is actually there, which was the original defect; recovery is removing the directory, and the message says so. **Pruning still escaped the install root.** The previous round hardened file removal against forged install state and left `prune_empty_dirs` joining recorded parents blind, so a recorded `src/deep/x.ts` with `src` symlinked out of the tree removed `<outside>/deep`. Bounded to empty directories, so no data loss -- but the same untrusted input, and two of three consumers hardened is not a threat model. It resolves the directory itself now, not its parent, because a symlinked `src` passes a parent check while pointing anywhere. Uninstall also reports what it refused and exits non-zero rather than claiming plain success. **`install all --install-dir` excluded pi three ways.** Filtering pi out of the run was the wrong fix: the exclusion was silent, the "no supported host was detected" error named pi as undetected when it had been detected and filtered, and `uninstall all --install-dir` exited 0 leaving a managed extension where it had previously exited 1 and said why. Now the *flag* is cleared for pi under `all`, not the host dropped, so every host stays in the run and each gets the only meaning the flag has for it. Named explicitly, `install pi --install-dir` still errors. **A test wrote to a predictable global path.** The absolute-traversal case used a fixed `/tmp` filename and deleted it afterward, clobbering any same-named file already there. Both victims are inside the test's own `TempDir` now. **A new assertion was vacuous.** The orphan-span test compared the shell span's `End` against the last `End` of any kind, which its own value can satisfy -- so it could not fail while its comment claimed to enforce containment. Named scopes on both sides and a strict `<` now. The count assertion above it was the load-bearing one and is unchanged. Three fixes from the previous round were unpinned and survived mutation: the containment guard, the `fault-allowed` status, and the timeout clamp. All three have tests now, each mutation-checked. The symlink test needed its victim directory left *empty* to reach the pruning path at all -- with a file in it, `remove_dir` fails for an unrelated reason and hides the escape. Docs: the never-overwrite guarantee is restored and now says what it covers; "tool and turn activity reach Relay whenever the gateway is reachable" also needed the preemption case, because pi runs every extension's handler and one registered ahead of this can decide a call before Relay's gate sees it; and the shared installation guide no longer calls pi's extension system "no plugin system" or folds it into the marketplace/MCP description -- the two mechanisms are now separated, with pi's diagnose and uninstall commands added. Verified: `cargo test -p nemo-relay-cli` (1377), 99 extension tests, `tsc` clean, clippy `--all-targets`, `cargo fmt --check`, pre-commit `--all-files` -- clean apart from cargo-deny, gofmt and go, absent on this machine. `install all --install-dir` and `uninstall all --install-dir` exercised end to end against a scratch `PI_CODING_AGENT_DIR` with pi on PATH. Signed-off-by: Yuchen Zhang <yuchenz@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
Adds pi (
@earendil-works/pi-coding-agent) as a third supported coding agent alongside Codex and Claude Code.The integration is a sidecar. A NeMo Relay-authored pi extension posts pi's lifecycle to the CLI gateway at
POST /hooks/pi, and the gateway builds the scope tree. Nothing in pi's process loads the Node binding. This shape is forced, not chosen: pi has no native hook-configuration file and its external event stream is observation-only, so hooks cannot be injected from outside the process. The extension is deliberately thin — all policy and all span construction stay in the gateway, on the same managed path Codex and Claude Code already use.Tool and turn activity are captured; a guardrail can block a real pi tool call, a model call, and the bang-prefixed inline shell a user types (
!cmd), which never reaches pi's tool registry. A request intercept can rewrite a tool call's arguments and pi executes the rewrite.Details
Most of this is additive — a new agent variant and a new route. The parts worth a reviewer's judgment are the decisions, not the inventory.
Shared code, where a regression would land. Three changes touch files Codex and Claude Code also depend on:
NormalizedEvent::TurnStartedand theturn_start/compactionlists onClassificationRules. Codex and Claude Code declare&[]for both, so their behavior is unchanged.AgentKind::has_explicit_turn_start(). For a harness that reports its own turn start, an event arriving between turns is genuinely between turns, so it is recorded on the session scope instead of manufacturing a turn to hold it. Only pi sets it.AgentInfo.checks,skip_serializing_ifempty, so Codex and Claude Code entries are byte-identical in JSON andschema_versionstays at 2.Argument transforms are constrained, not validated. An allow response may carry
{"tool_call": {"tool_call_id": "…", "input": {…}}}, applied to pi'sevent.inputin place. A transform may only rewrite the values of existing keys, preserving each value's JSON type. Adding a key, removing one, or changing a type is refused, and a refusal blocks rather than falling back to the original arguments, which would silently discard the policy. This is not schema validation, by choice — pi's tool set is per-session mutable, so a schema read once can go stale mid-session. Conditional-execution guardrails decide on the arguments pi proposed and are not re-run on the rewrite, matching the runtime's managed-call order.The inline-shell gate is named
user_bash, notbash. A guardrail receives only a tool name and arguments, so a policy can tell a command the user typed from one the model proposed only if the two arrive under different names. The trade-off is that a policy covering both must name both; the docs say so and a test asserts it.Model redirection is conditional, and the condition is the design. pi resolves
baseUrlper model from a generated catalog with no flag or generic override, so the extension points the active model's provider at the gateway withregisterProvider. That rewrite is provider-wide, so the decision verifies the provider's whole catalog, snapshotted before any registration. A provider mixing API families at different paths (Fireworks) is skipped rather than broken mid-session. Each decision that explains something is recorded as amodel_redirectmark, so a trace without LLM spans states its own reason.The registration also carries this invocation's proxy credential, which is what makes
nemo-relay run --agent piproduce LLM spans at all: a gateway the launcher started authenticates its own client before any intercept can rewrite the route, so a redirected call without it comes back401. It rides on the registration rather than a per-request hook for the same reason the session id does — only providers actually pointed at the gateway ever send it.nemo-relay doctor pipredicts failures nothing else reports. pi loads a project-scoped extension only for a trusted project, and its non-interactive modes never prompt — so the extension is dropped by a bare conditional that pi does not treat as a failure and never surfaces, and that the extension cannot surface either, because it is not running. Doctor also catches two copies loading at once (pi de-duplicates by path, not by package) and an install whose settings filters switch it off.Where should the reviewer start?
crates/cli/src/agents/shared/adapters.rsandcrates/cli/src/sessions/mod.rs.Known limitations, documented rather than papered over
tool_callhandler unless one blocks, sharing one mutableinputwith no re-validation. Loading first with-estops an earlier extension pre-empting the gate; it does nothing about a later one rewriting arguments after Relay authorized them. pi offers no ordering API, so the gate is authoritative over the model, not over the other extensions.private: trueand deliberately not published to npm. A file drop and a local-pathpi installboth work and cover user scope. A git URL is not a working source: pi clones the repository root, finds nopimanifest there, and loads nothing.Validation
cargo test -p nemo-relay-cli(1216 tests),just test-pi(97 Node tests),just docs-linkcheck(0 errors),cargo clippy --workspace --all-targets -- -D warnings.uv run pre-commit run --all-files: 30 of 33 hooks pass.cargo-deny,go fmtandgo vetfail only because those binaries are not installed on this machine, and no Go or dependency-manifest files are touched.v0.84.0session with a real model, reading the gateway's own ATOF output rather than asserting on hook status codes: turn scopes opening at pi's boundary, tool spans nested under their turn with attempt attribution, LLM spans nested inside their own turns, and a containment check confirming no span outlives the scope that opened it.block_llms = truerejecting a model call (recorded as a rejection mark, not a span, because the call never executed), and the inline-shell gate in pi's RPC mode — the only automatable path that reaches the bang prefix.--offline.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
🤖 Generated with Claude Code