feat(#6571): add xai-vertex provider to the pi runtime - #6572
Conversation
|
🤖 Finished Review · ✅ Success · Started 10:01 PM UTC · Completed 10:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.76 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
the
requires-manual-review
Site previewPreview: https://a956d61d-site.fullsend-ai.workers.dev Commit: |
|
🤖 Finished Review · ✅ Success · Started 10:40 PM UTC · Completed 10:58 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.21 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 11:38 PM UTC · Completed 11:54 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.03 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
|
🤖 Review · Commit: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Verified rather than assumed, on the branch head:
Version pin. PI_XAI_VERTEX_VERSION=0.2.0 / SHA256=b00c67a2…. I recomputed the digest from the released tarball independently of the release workflow that published it — byte-for-byte match — and confirmed the v0.2.0 annotated tag dereferences to pi-xai-vertex@main (7a37272a).
Image. Built the sandbox image from this branch with the v0.2.0 pin: build clean, xai-vertex installed at 0.2.0, extension dirs and their parent all 555 root, and the sandbox user gets Permission denied writing to the parent — confirming the chmod fix from the review.
Runtime, end to end. Ran the triage agent in a real sandbox on this image: Result: stop, 10 turns, exit 0, schema validation passed, metrics.json reporting runtime: pi and model: xai/grok-4.6, zero auth errors. Re-ran the same agent on anthropic-vertex/claude-opus-4-6 against the same image — 17 turns, exit 0, validation passed — so the second extension does not disturb the first.
v0.1.0 would not have worked here. It declared auth as pi's auth.oauth, which makes pi wait for a persisted interactive credential; a sandbox is always fresh, so every run failed No API key found for xai-vertex with zero turns. That is what v0.2.0's ambient ADC fixes, and why this PR needed the pin bump.
CI is green (14 checks) and all eight review threads are resolved — four fixed, two already fixed by later commits, one investigated and answered (pi 0.84.3's built-in xai provider reads only XAI_API_KEY; there is no XAI_BASE_URL, so the asymmetry with the four ANTHROPIC_* unsets is correct).
Follow-ups filed rather than folded in: #6574 (docs restructure, stacked on this) and #6575 (opt-in, non-blocking behaviour coverage for Grok).
Vendor fullsend-ai/pi-xai-vertex v0.1.0 in the sandbox image and wire it into PiRuntime so agents can run Grok 4.6 on Vertex AI alongside the existing Claude-on-Vertex path. Containerfile: add a SHA256-pinned tarball install block mirroring the existing pi-anthropic-vertex pattern, installed under PI_EXTENSIONS_DIR/xai-vertex with npm ci --omit=dev --omit=peer. pi_run.go: translatePiModel now normalises "xai/grok-4.6" to "xai-vertex/xai/grok-4.6" so the provider gate fires correctly instead of falling through to pi's built-in xai provider (which requires XAI_API_KEY). buildPiRunCommand loads the extension with -e, unsets XAI_API_KEY, and pins XAI_VERTEX_PROJECT_ID to the fleet's ANTHROPIC_VERTEX_PROJECT_ID. piBareModelID now uses strings.Cut (first slash) instead of LastIndexByte so three-segment specs report the correct wire model id in metrics. Note: golangci-lint was not available in the sandbox. go vet and gofmt both passed. Closes #6571
Review of the xai-vertex wiring surfaced four gaps, two of them the same class of silent fallthrough the issue exists to close. Model normalisation was case-sensitive (`strings.HasPrefix(model, "xai/")`) while the gate immediately below is deliberately case-insensitive, because pi resolves provider prefixes with EqualFold. So `XAI/grok-4.6` skipped normalisation, the gate never fired, the extension never loaded and XAI_API_KEY was never unset -- the run silently left the Vertex path for xAI's native API. Normalisation is now case-insensitive throughout. A bare model id under FULLSEND_PI_PROVIDER=xai-vertex rendered the two-segment "xai-vertex/grok-4.6". The extension registers "xai/grok-4.6", so pi substituted a fallback model carrying the wrong wire id and only warned -- to stderr, which is captured solely in debug mode. This is the only path a harness can use, since validModelName forbids "/" in harness `model:`, so it is the likely default-to-Grok configuration. Both inputs now render the canonical three-segment spec. The new PI_XAI_VERTEX_VERSION pin had no Renovate customManager, so it would have sat at 0.1.0 with only a comment asking a human to check tags. Adds one mirroring PI_ANTHROPIC_VERTEX_VERSION, plus a test asserting every PI_*_VERSION pin in the Containerfile is tracked -- verified to fail when the manager is removed. docs/runtimes.md gains a Grok-on-Vertex bullet next to the Claude one and the rendered command line, model table and metrics note are corrected. Assisted-by: Claude (fix), Claude (review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
v0.1.0 declared its auth as pi's `auth.oauth`, which means "an interactive login mints a credential pi persists to auth.json" -- so pi refused the provider until one existed. A sandbox is always fresh, so every run failed with "No API key found for xai-vertex" and the agent did zero turns. It passed on developer machines only because they had logged in once. v0.2.0 uses ambient ADC auth, which is what the credentials in a sandbox actually are: pi calls resolve() per request and google-auth-library mints from GOOGLE_APPLICATION_CREDENTIALS, with nothing persisted to disk. Verified against a locally built sandbox image carrying v0.2.0: the triage agent completed 10 turns on xai/grok-4.6 with schema validation passing and no auth errors, and a Claude run on anthropic-vertex against the same image completed 17 turns, confirming the second extension does not disturb the first. SHA256 recomputed from the released tarball independently of the release workflow that published it. Assisted-by: Claude (fix) Signed-off-by: Wayne Sun <gsun@redhat.com>
The export was unconditional, so it replaced any XAI_VERTEX_PROJECT_ID the runner had already set. That collapses a distinction pi supports natively: each Vertex provider resolves its own project variable, so one pi process serves Grok, Claude and Gemini from different GCP projects at the same time -- verified locally with grok on one project and claude plus gemini on another, both extensions loaded together under --no-extensions with two -e flags, all three answering. Overriding left no way to point Grok at its own project. Where Grok is enabled in Model Garden for a different project than Claude, every run would fail 403 PERMISSION_DENIED with nothing to tune, and the extension would not warn because it did have a project -- just the wrong one. Now defaults to the fleet's Vertex project only when unset, so behaviour is unchanged where nothing sets it. Verified the rendered expansion for all five cases: explicit value wins; unset falls back to ANTHROPIC_VERTEX_PROJECT_ID then GOOGLE_CLOUD_PROJECT; nothing set yields empty, which the extension already treats as unconfigured and skips; and an empty string falls through rather than pinning empty. Assisted-by: Claude (fix) Signed-off-by: Wayne Sun <gsun@redhat.com>
878a7de changed the export from an unconditional pin to a default but did not update the prose, which still said it pins both Vertex providers to one project. That is now wrong twice: it defaults rather than pins, and the rationale is inverted -- the change exists so the two providers *can* differ. Signed-off-by: Wayne Sun <gsun@redhat.com>
Four low-severity findings from the review agent, all confirmed: - American spelling: normaliseXaiVertexModel -> normalizeXaiVertexModel and the comments with it. internal/ uses American English uniformly. - Constant placement: piXaiVertexProvider moves into the grouped const block next to piDefaultProvider, rather than standing alone mid-file. - Containerfile: the per-extension chmods replaced a single recursive chmod of PI_EXTENSIONS_DIR, leaving the parent's mode inherited from the build umask. The last install now locks the parent explicitly, non-recursively -- each extension must still lock its own subtree, or a later install would re-walk earlier ones. - Stale doc: "other providers get pi's built-ins only" predates a second vendored extension; now "providers without a vendored extension". Not changed, with reasons: - The xai branch unsets only XAI_API_KEY where the anthropic branch unsets four. Checked pi 0.84.3's built-in xai provider: XAI_API_KEY is the only env var it reads, and there is no XAI_BASE_URL. The anthropic asymmetry exists because the Anthropic SDK honours ANTHROPIC_BASE_URL and ANTHROPIC_AUTH_TOKEN, which have no xAI equivalent. - The case-sensitivity finding on the xai/ prefix was already fixed in 39fe08f, which the review predates. Assisted-by: Claude (fix), fullsend-ai-review (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
runtimes.md served two audiences at once and served neither well: 8,588 words in which the most user-relevant content -- how to select a runtime and a model -- sat at line 314 of 376, behind the security feature matrix, the runtime interface contract, the sandbox hook wire protocol and the workspace layout. Several single bullets ran past 3,900 characters. The user page is now 1,690 words and ordered the way someone actually arrives: what a runtime is, choosing between claude and pi, selecting one, models, running pi, troubleshooting. Selection moved to the top. A troubleshooting section is new, covering the failures that are hard to diagnose from their symptoms -- a silently dropped extension, "No API key found", a 403 from the wrong project, and a model misreporting which model it is. Contributor content moved verbatim to docs/contributing/runtime-implementation.md rather than being deleted: the security matrix, fail modes, interface contract, hook contract and wire protocol, workspace layout, agent rule layering, dummy ops, and the pi verification provenance -- which version of pi's source each claim was checked against, and what to re-check on a bump. Inbound anchors from ADR 0090, the local-run guide and the choosing-a-runtime guide are repointed; make lint-md-links passes with zero errors. Two diagrams are new: how the runtime/model override precedence resolves, and how each Vertex provider resolves its own GCP project from one set of ADC credentials -- the second documents behaviour that was not written down anywhere, and explains why an explicitly-set XAI_VERTEX_PROJECT_ID is left alone. Assisted-by: Claude (docs) Signed-off-by: Wayne Sun <gsun@redhat.com>
The single runtimes page was still doing too much: a reader on Claude Code had to scroll past pi's provider table, its behaviour differences and its troubleshooting to reach the config-key mapping, and a reader on pi had no page to land on. Split by audience again, one level down: - /runtimes (923 words) -- what a runtime is, choosing between claude and pi, selecting one, where the selection appears, harness config keys. - /runtimes/pi (927 words) -- models and providers including the per-provider GCP project behaviour, the at-a-glance table, behaviour differences and troubleshooting. Sidebar "Runtimes" becomes a collapsible section with the pi page under it, matching the Agents section below it. /docs/runtimes is added to the Guides search scope: docs/doc-site.md notes that a docs/ subfolder missing from every scope becomes unreachable whenever a scope pill is active, which a new subfolder would otherwise trip. make lint-md-links passes with zero errors; the inbound anchors from the local-run guide and the implementer page are repointed at the pi page. Assisted-by: Claude (docs) Signed-off-by: Wayne Sun <gsun@redhat.com>
…line A Runtimes section with a single child read as unfinished -- every other sidebar section here has several -- and made the default runtime look second-class next to the opt-in one. Claude Code now has a sibling page rather than a stub. The content was real and scattered: model aliases and FULLSEND_FALLBACK_MODELS (stranded in the overview's selection section), --agent replacing the system prompt where pi appends, native sub-agents, the CLAUDE.md bridge, the failed-tool-call sanitizing limit, and the repo .claude/settings.json auto-load caveat -- several of which only existed on the implementer page. The comparison stays on the overview; the runtime pages hold operational detail, so the fallback-chain mechanics live in exactly one place. Also fixes a nesting bug from the previous commit: renaming "Running pi" to "At a glance" silently demoted "Behaviour differences worth knowing" and "Not yet exercised" to children of the table above them, which is the wrong outline and renders wrongly in the page TOC. Both promoted back to H2. Assisted-by: Claude (docs) Signed-off-by: Wayne Sun <gsun@redhat.com>
"Running pi" / "Running Claude Code" put a verb in front of every sidebar entry, which reads oddly in a list of nouns next to Agents and Getting Started, and buries the thing the reader is scanning for. The pages are now "Pi" and "Claude Code" -- capitalised as titles, while prose keeps the project's own lowercase "pi". Link text across the overview, the implementer page and between the two runtime pages follows. Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · Commit: |
cfc39d4 to
72a7471
Compare
|
🤖 Finished Review · ✅ Success · Started 1:05 AM UTC · Completed 1:22 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.21 |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
docs/guides/getting-started/choosing-a-runtime.md(file-level): Line 18 · [low] stale-doc
Pi row description only examples Gemini as an alternative provider; Grok is now a first-class named provider but is not mentioned.
Suggested fix: Add Grok-on-Vertex alongside Gemini in the example.
|
🤖 Finished Retro · ✅ Success · Started 1:27 AM UTC · Completed 1:40 AM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.85 |
Retro: PR #6572 — add xai-vertex provider to the pi runtimeWorkflow shape: Issue #6571 → triage ($0.91) → code agent ($3.89, run 32781317760) → PR #6572 → 4 successful review agent runs (~$27.21 total, plus 4 cancelled due to rapid pushes) → human rework (9 commits) → merge. Timeline: The code agent produced 1 commit in ~13 minutes from a well-specified issue. The human reviewer (waynesun09) then pushed 9 follow-up commits fixing 4 substantive bugs and restructuring docs over ~3 hours before merging. What the code agent did well
What the human reviewer caught that the agent missed
Review agent valueThe review agent caught case-sensitivity, naming conventions, chmod scope, stale docs, and a Renovate Root cause of reworkThe code agent never visited the external Existing issues with new evidence
Proposals filed
|
1 participant
Summary
Add the xai-vertex provider to the pi runtime so agents can run Grok 4.6 on Vertex AI, alongside the existing Claude-on-Vertex path. Vendors
fullsend-ai/pi-xai-vertexv0.1.0 in the sandbox image and wires it intobuildPiRunCommandwith env hygiene (unsetXAI_API_KEY, pinXAI_VERTEX_PROJECT_ID).Related Issue
Closes #6571
Changes
images/sandbox/Containerfile: Add SHA256-pinned tarball install block forpi-xai-vertexv0.1.0 underPI_EXTENSIONS_DIR/xai-vertex, mirroring the existingpi-anthropic-vertexpatterninternal/runtime/pi.go: AddpiXaiVertexExtensionPathconstantinternal/runtime/pi_run.go:translatePiModel: normalisexai/grok-4.6toxai-vertex/xai/grok-4.6so the provider gate fires correctlypiBareModelID: usestrings.Cut(first/) instead ofLastIndexByteso three-segment specs return the correct wire model id (xai/grok-4.6notgrok-4.6)buildPiRunCommand: add xai-vertex branch that loads the extension with-e, unsetsXAI_API_KEY, and pinsXAI_VERTEX_PROJECT_IDtoANTHROPIC_VERTEX_PROJECT_IDTestPiBareModelID,TestBuildPiRunCommand_XaiVertex, xai/ normalisation cases inTestTranslatePiModel, updateTestPiExtensionPathsWithinSandboxPolicyto cover both extension paths, updateTestSandboxImagePiDefaultsto verify xai-vertex in ContainerfileTesting
go test ./internal/runtime/...passes (86.2% overall coverage)go test ./internal/sandbox/...passesgo vetpassesgofmtpassesCloses #6571
Post-script verification
agent/6571-add-xai-vertex-provider)a1f5fe4d4fff178eb83302f4f74f7e89cbae9876..HEAD)