Skip to content

fix(#7026): add Vertex model fallback for aliased pi runs - #7027

Open
fullsend-ai-coder[bot] wants to merge 3 commits into
mainfrom
agent/7026-pi-vertex-fallback
Open

fix(#7026): add Vertex model fallback for aliased pi runs#7027
fullsend-ai-coder[bot] wants to merge 3 commits into
mainfrom
agent/7026-pi-vertex-fallback

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown
Contributor

Summary

Implements runner-level Vertex model fallback for pi alias requests. When a model alias (opus, sonnet, haiku, fable) hits a Vertex 404 or 403, the runner retries with the next entry in FULLSEND_FALLBACK_MODELS. Pinned explicit ids fail loudly — no silent fallback.

Related Issue

Closes #7026

Changes

  • Extract single-model execution into piExecModel to support retry loop
  • Add isVertexModelUnavailable to detect Vertex 404 ("Publisher model not found") and 403 ("data sharing not enabled") error shapes
  • Add isPiAliasedModel / piFallbackChain to build the ordered, deduplicated model chain
  • Update metrics.Model to reflect the model that actually answered
  • Remove the old "fallback models are not supported on pi yet and are ignored" warning
  • Update docs/runtimes.md to document the new behavior

Testing

  • TestIsVertexModelUnavailable — 404/403 patterns, case insensitivity, non-model errors
  • TestIsPiAliasedModel — documented aliases, empty model default, bare IDs, provider/id specs, config aliases
  • TestPiFallbackChain — alias+fallbacks chain, pinned ID (no fallback), provider/id (no fallback), empty fallbacks, deduplication, config aliases
  • All existing pi tests pass (runtime suite at 89.4% coverage)
  • go vet, gofmt, secret scan all clean

Closes #7026

Post-script verification

  • Branch is not main/master (agent/7026-pi-vertex-fallback)
  • Secret scan passed (gitleaks — 403540d77c8c32f89ec7e813bf68c7ff016f582e..HEAD)
  • PR body secret scan passed (gitleaks — no-git)

When the pi runtime receives an alias request (opus, sonnet, haiku,
fable) and FallbackModels are configured, the runner now tries each
model in order. A Vertex 404 ("Publisher model not found") or 403
("data sharing not enabled") on the current model triggers the next
fallback. Pinned explicit ids (provider/id or bare catalog ids) fail
loudly with no silent fallback.

- Extract single-model execution into piExecModel to support retries
- Add isVertexModelUnavailable to detect Vertex 404/403 error shapes
- Add isPiAliasedModel / piFallbackChain to build the ordered chain
- Update metrics.Model to reflect the model that actually answered
- Remove the old "fallback models are ignored" warning
- Update docs/runtimes.md to document the new behavior

Closes #7026
@fullsend-ai-coder
fullsend-ai-coder Bot requested a review from a team as a code owner September 4, 2026 20:26
@fullsend-ai-coder fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Sep 4, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 8:28 PM UTC · Completed 8:50 PM UTC

Commit: b59f2e2 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown

Site preview

Preview: https://374873ef-site.fullsend-ai.workers.dev

Commit: 40f2202f157546053e34493404ce8658169e08ca

@codecov

codecov Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.66990% with 23 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/runtime/pi_run.go 77.66% 12 Missing and 11 partials ⚠️

📢 Thoughts on this report? Let us know!

@fullsend-ai-review fullsend-ai-review Bot added the risk/moderate PR risk: moderate label Sep 4, 2026
@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

Risk Assessment: moderate (2/5)

Details

Moderate risk: clean metadata profile (small file count, no protected/security paths, trusted bot author) offset by very high file churn and concentrated fix/revert history across the pi runtime area; the PR is well-scoped to the linked issue with tests, and Tier 1 signals are unchanged from prior review, preserving the anchored score of 2.

Previous run

Risk Assessment: moderate (2/5)

Details

Moderate risk: clean metadata profile (small file count, no protected/security paths, trusted bot author) offset by very high file churn and concentrated fix/revert history across the pi runtime area; the PR is well-scoped to the linked issue with tests, and Tier 1 signals are unchanged from prior review, preserving the anchored score of 2.

Previous run (2)

Risk Assessment: moderate (2/5)

Details

Moderate risk: clean metadata profile (small file count, no protected/security paths, good test ratio, trusted bot author) offset by high file churn and concentrated fix/revert history in the pi runtime area; the PR itself is well-scoped to the linked issue with tests.

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

Review

Findings

Low

  • [edge-case] internal/runtime/pi_run.go:921ToolCalls (atomic counter) accumulates across all fallback attempts via metricsHandler. For the two targeted Vertex error shapes, pi emits no ToolUseEvents before failing, so the metric is correct in practice. Widening fallback-eligible errors in the future could silently inflate the metric.

  • [missing-validation] internal/runtime/pi_run.go:877validatePiModel is called only on the primary model, not on individual fallback entries from FallbackModels. An invalid fallback entry passes through translatePiModel, producing a model spec pi cannot serve. All documented aliases currently have piModelAliases entries and FallbackModels is runner-controlled, so this gap cannot manifest today.

Previous run

Review

Findings

Medium

  • [stale-doc] docs/contributing/runtime-implementation.md:558 — The pi "Process and exit codes" section describes how Run invokes pi with a single --model <provider/id> argument and documents exit-code semantics assuming a single invocation. The PR adds a fallback loop that retries with each model in piFallbackChain on Vertex 404 ("Publisher model not found") and 403 ("data sharing not enabled") for aliased models, fundamentally changing Run's invocation and error-handling behavior. The section does not mention the aliased-vs-pinned model distinction, piFallbackChain, or the retry-on-Vertex-error semantics.
    Remediation: Add a paragraph documenting: (1) Run builds a piFallbackChain for aliased models from FULLSEND_FALLBACK_MODELS; (2) on Vertex 404/403 (isVertexModelUnavailable), Run retries with the next model in the chain; (3) pinned model IDs (isPiAliasedModel returns false) skip the chain and fail immediately.

Low

  • [edge-case] internal/runtime/pi_run.go:904ToolCalls (atomic counter) accumulates across all fallback attempts via metricsHandler. For the two targeted Vertex error shapes, pi emits no ToolUseEvents before failing, so the metric is correct in practice. Widening fallback-eligible errors in the future could silently inflate the metric.

  • [missing-validation] internal/runtime/pi_run.go:877validatePiModel is called only on the primary model, not on individual fallback entries from FallbackModels. An invalid fallback entry passes through translatePiModel, producing a model spec pi cannot serve. All documented aliases currently have piModelAliases entries and FallbackModels is runner-controlled, so this gap cannot manifest today.

  • [edge-case] internal/runtime/pi_run.go:935 — When piExecModel returns both a non-nil lastResult and a non-nil execErr, the post-loop code forwards the ResultEvent through metricsHandler and then returns the execErr. This matches the old code's behavior (not a regression), but the user sees a result block followed by an error in the rare scenario where a stream produces a complete ResultEvent before the process fails to wait.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR
Previous run (2)

Review

Findings

High

  • [stale-doc] docs/runtimes/pi.md:116 — The "At a glance" table lists "Fallback chains" under "Not supported." After this PR, pi supports fallback chains for aliased model requests on Vertex 404/403, directly contradicting this claim.
    Remediation: Update the "Not supported" row to remove "Fallback chains" and add conditional support language matching the updated docs/runtimes.md table row ("Alias requests: tried in order on Vertex 404/403; pinned ids: fail loudly").

Medium

  • [error-handling-gap] internal/runtime/pi_run.go:916 — Events from failed fallback attempts are forwarded to the event handler/renderer without filtering. When the first model fails with a Vertex model-unavailable error, piExecModel emits all events (including the error ResultEvent) to the renderer. The user sees a fully-rendered error result block from the failed attempt before the StepWarn about falling back and the second attempt's output.
    Remediation: Suppress forwarding of ResultEvent from failed attempts, or buffer events and only forward on success.

  • [stale-doc] docs/runtimes/pi.md:78 — States "the run fails at the first model call, and pi has no fallback." After this PR, aliased models fall back through the chain on Vertex 404/403.
    Remediation: Qualify the statement: aliased models now fall back on Vertex 404/403; pinned ids still fail at the first call.

  • [stale-doc] docs/cli/run.md:51 — Lines 51 and 53 say "pi and codex ignore it with a warning" and "pi ignores the chain with a warning" about FULLSEND_FALLBACK_MODELS. After this PR, pi uses the fallback chain for aliased model requests.
    Remediation: Update line 51 to say "pi uses it for aliased models (Vertex 404/403 fallback); codex ignores it with a warning" and update line 53 accordingly.

Low

  • [comment-preservation] internal/runtime/pi_run.go:854 — The refactor removed four inline design-rationale comments: (1) the hooks-predicate "wired check" equivalence comment, (2) the extension-preflight host-directory hash comment, (3) the sub-agent usage "children's tokens" comment, and (4) the unconditional parent-entry per_model_usage invariant comment. The code they documented is unchanged; only the rationale is lost.
    Remediation: Restore the removed design-rationale comments in their new locations.

  • [edge-case] internal/runtime/pi_run.go:904ToolCalls (atomic counter) accumulates across all fallback attempts via metricsHandler. For the two targeted Vertex error shapes, pi emits no ToolUseEvents before failing, so the metric is correct in practice. Widening fallback-eligible errors in the future could silently inflate the metric.

  • [missing-validation] internal/runtime/pi_run.go:868validatePiModel is called only on the primary model, not on individual fallback entries from FallbackModels. All documented aliases currently have piModelAliases entries so this gap cannot manifest today.


Next steps:

  • /fs-fix — agent addresses review findings automatically
  • /fs-fix <your instruction> — agent fixes with your specific guidance
  • Push commits directly — review re-runs automatically on push
  • /fs-fix-stop — disable automatic fix runs for this PR

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 8:52 PM UTC · Completed 9:02 PM UTC

Commit: b59f2e2 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.47

- Suppress ResultEvent forwarding from failed fallback attempts in
  piExecModel so the renderer does not show a fully-formed error block
  before the fallback warning; the final attempt ResultEvent is
  forwarded after the loop selects the successful attempt
- Update docs/runtimes/pi.md At a glance table: move Fallback chains
  from Not supported to its own row with conditional support language
- Update docs/runtimes/pi.md model-alias section to reflect that aliased
  models now fall back on Vertex 404/403
- Update docs/cli/run.md to say pi uses the fallback chain for aliased
  models instead of ignoring it with a warning
- Restore four design-rationale comments removed during the refactor:
  hooks-predicate wired check, extension-preflight host-directory hash,
  sub-agent children tokens, and per_model_usage invariant

Note: pre-commit hooks were not run. pre-commit could not complete
(infrastructure failure: HTTP 403 fetching remote hooks), and the hooks
were run directly (gofmt, go vet, trailing-whitespace, docs-links).

Addresses #7027
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 1 (bot-triggered)

Addressed 5 of 7 review findings with code and doc fixes. Updated three stale doc references to reflect pi's new conditional fallback support. Suppressed error ResultEvent forwarding from failed fallback attempts so users see only the final result. Restored four design-rationale comments removed during the refactor. Disagreed on two low-severity findings (ToolCalls accumulation and fallback validation) that the reviewer acknowledged are correct in practice.

Fixed (5):

  1. [stale-doc] docs/runtimes/pi.md:116 — At a glance table lists Fallback chains under Not supported (docs/runtimes/pi.md): Moved Fallback chains out of Not supported into its own row with conditional support language: alias requests tried in order on Vertex 404/403, pinned ids fail loudly
  2. [error-handling-gap] pi_run.go:916 — Events from failed fallback attempts forwarded to renderer (internal/runtime/pi_run.go): Suppressed ResultEvent forwarding from piExecModel; the fallback loop now forwards only the final attempt's ResultEvent through metricsHandler after the loop completes, so the renderer sees exactly one result block
  3. [stale-doc] docs/runtimes/pi.md:78 — States pi has no fallback (docs/runtimes/pi.md): Qualified the statement: aliased models (opus, sonnet, etc.) fall back through FULLSEND_FALLBACK_MODELS on Vertex 404/403; pinned ids fail at the first model call
  4. [stale-doc] docs/cli/run.md:51 — Says pi ignores FULLSEND_FALLBACK_MODELS with a warning (docs/cli/run.md): Updated line 51 to say pi uses the chain for aliased models (Vertex 404/403 fallback) and line 53 to say pi uses the chain for aliased models on Vertex 404/403 and ignores it for pinned ids
  5. [comment-preservation] pi_run.go:854 — Four inline design-rationale comments removed during refactor (internal/runtime/pi_run.go): Restored all four removed comments: (1) hooks-predicate wired-check equivalence, (2) extension-preflight host-directory hash rationale, (3) sub-agent children's tokens rationale, (4) unconditional parent-entry per_model_usage invariant

Disagreed (2):

  1. [edge-case] pi_run.go:904 — ToolCalls counter accumulates across fallback attempts: The reviewer acknowledges this is correct in practice: the two Vertex error shapes that trigger fallback (404 and 403) cause pi to fail before emitting any ToolUseEvents. This is a future consideration for if fallback-eligible errors are widened, not a current defect requiring a code change
  2. [missing-validation] pi_run.go:868 — validatePiModel called only on primary model, not fallback entries: The reviewer acknowledges this gap cannot manifest today: all documented aliases have piModelAliases entries. Adding validation for fallback entries would be speculative hardening beyond the PR's scope; the fallback chain is built from the same alias table that validatePiModel checks

Tests: passed

Decision points
  • Suppress ResultEvent at piExecModel level vs. filtering in the fallback loop (alternatives: Buffer all events in piExecModel and only forward on success, Suppress only ResultEvent in piExecModel and forward the final one after the loop; rationale: Suppressing only ResultEvent in piExecModel is minimally invasive: non-result events (progress, tool use) still stream in real-time, while the error result block is held back. Buffering all events would delay all output until the attempt completes, degrading UX for successful attempts)

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:04 PM UTC · Completed 9:26 PM UTC

Commit: 93988d4 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.79

fullsend-ai-review[bot]

This comment was marked as outdated.

@fullsend-ai-coder

fullsend-ai-coder Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Finished Fix · ✅ Success · Started 9:28 PM UTC · Completed 9:34 PM UTC

Commit: 93988d4 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.43

…odes

Add a "Model fallback chain" bullet to the pi Process and exit codes
section in runtime-implementation.md, documenting:
(1) Run builds a piFallbackChain for aliased models from
    FULLSEND_FALLBACK_MODELS
(2) Vertex 404/403 triggers retry with the next model
(3) Pinned model IDs skip the chain and fail immediately

Addresses #7027
@fullsend-ai-coder

Copy link
Copy Markdown
Contributor Author

🔧 Fix agent — iteration 2 (bot-triggered)

Addressed the medium-severity stale-doc finding by adding fallback chain documentation to the Process and exit codes section. Disagreed on three low-severity findings the reviewer acknowledged are correct in practice or not a regression.

Fixed (1):

  1. [stale-doc] docs/contributing/runtime-implementation.md:558 — Process and exit codes section does not document fallback chain behavior (docs/contributing/runtime-implementation.md): Added a Model fallback chain bullet to the Process and exit codes section documenting: (1) Run builds a piFallbackChain for aliased models from FULLSEND_FALLBACK_MODELS, (2) Vertex 404/403 detected by isVertexModelUnavailable triggers retry with the next model, (3) pinned ids and provider/id specs produce a single-element chain with no fallback, and (4) metrics.Model is updated to reflect the model that actually answered

Disagreed (3):

  1. [edge-case] pi_run.go:904 — ToolCalls counter accumulates across fallback attempts: The reviewer acknowledges this is correct in practice: the two Vertex error shapes that trigger fallback (404 and 403) cause pi to fail before emitting any ToolUseEvents, so no tool calls are double-counted. This is a future consideration if fallback-eligible errors are widened, not a current defect requiring a code change
  2. [missing-validation] pi_run.go:877 — validatePiModel called only on primary model, not fallback entries: The reviewer acknowledges this gap cannot manifest today: all documented aliases have piModelAliases entries, and FallbackModels is runner-controlled. Adding validation for fallback entries would be speculative hardening beyond the PR's scope
  3. [edge-case] pi_run.go:935 — Both lastResult and execErr can be returned together from piExecModel: The reviewer states this matches the old code's behavior and is not a regression. The scenario (stream produces a complete ResultEvent before process fails to wait) is rare and the forwarding-then-error-return pattern is the same one the pre-refactor code used

Tests: passed

Decision points

Next steps:

  • /fs-review — request a re-review of the changes
  • /fs-fix <your instruction> — run another fix pass with specific guidance
  • Push commits directly — review re-runs automatically on push
    Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Sep 4, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 9:36 PM UTC · Completed 9:56 PM UTC

Commit: 40f2202 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $5.12

@fullsend-ai-review
fullsend-ai-review Bot dismissed stale reviews from themself September 4, 2026 21:55

Superseded by updated review

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-merge All reviewers approved — ready to merge ready-for-review Triggers review agent dispatch risk/moderate PR risk: moderate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pi: no automatic fallback when the aliased model is not served in the project — every default run fails instead of degrading

1 participant