Skip to content

fix(server): stop restoring stale Pi models - #15079

Open
poterstar wants to merge 2 commits into
pingdotgg:mainfrom
poterstar:fix/pi-model-inventory
Open

poterstar wants to merge 2 commits into
pingdotgg:mainfrom
poterstar:fix/pi-model-inventory

Conversation

@poterstar

@poterstar poterstar commented Oct 3, 2026 •

Copy link
Copy Markdown

Problem

After logging out of an upstream provider in Pi, T3 Code keeps that provider's cached models in the picker even when Pi's next successful discovery no longer returns them.
Refreshing or restarting T3 Code does not remove them because the registry merges missing Pi models back into the refreshed inventory and persists the merged result.

Reproduced with T3 Code 0.0.46-nightly.20261003.2610 and Pi 1.0.0: log out of OpenRouter while keeping another Pi provider connected, then refresh Pi's models in T3 Code.
Pi's fresh inventory excluded OpenRouter, but T3 Code retained its cached entries.

Change

Treat a completed Pi discovery as the current model inventory, including a valid empty inventory after all providers are disconnected.
Keep previously discovered models when discovery fails, times out, needs interactive input, or has not completed.
Preserve currently configured custom models without restoring removed custom entries.

Reject malformed inventories through Pi's existing discovery failure path.
This is necessary before treating discovery as authoritative: a missing/non-array models field or an invalid model identity must not look like a successful empty or partial inventory and prune valid cached models.

Scope and approval

This is a focused fix for an obvious model-cache bug under the small-fix exception in CONTRIBUTING.md.
It corrects reconciliation of an existing Pi inventory and adds regressions for that behavior; it introduces no settings, client changes, shared contracts, or changes to other providers' retention rules.
Both production changes address the same problem: replacing stale Pi models only when a complete, valid discovery establishes their removal.

Verification

Revalidated on macOS with Node 24.18.1 after merging main at 43f8a8de17.
The fix follows the current package layout: Pi RPC validation is in packages/provider-pi, and cache reconciliation is in the server's ProviderRegistry.
Tests follow the same boundary without importing the server registry into the Pi package.

  • vp test run packages/provider-pi/src/server/status.test.ts apps/server/src/provider/ProviderRegistry.test.ts — 78 tests passed across both complete files, covering populated/empty RPC inventories, seven malformed-response cases, incomplete discovery, custom models, later failures, cache persistence, and registry restart.
    On macOS, ran with SHELL=/bin/sh and a PATH excluding Homebrew to keep the existing Codex binary-path probe deterministic.
  • vp run --filter t3 --filter @t3tools/provider-pi typecheck — passed; existing Effect suggestions remain non-blocking.
  • vp lint and vp fmt --check on the four changed files — passed.
  • knip --workspace apps/server --workspace packages/provider-pi --exports --preprocessor ./scripts/knip-schemas.ts --no-config-hints — passed.
  • git diff --cached origin/main --check — passed for the complete PR diff.

Pi tests exercise the real RPC probe with a mocked subprocess transport.
Registry tests exercise model reconciliation and the persisted cache lifecycle, including restart.
No real model calls or credentials were used.
The patched desktop package has not been built or tested in a live client; this remains a backend-only change, and repository-wide checks are left to CI as directed by AGENTS.md.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 3, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at da26d9a

Macroscope's review found this PR approvable — This is a contained Pi-specific cache reconciliation fix with explicit handling for incomplete or malformed discovery responses. Production behavior is limited to removing stale models after valid discovery, with regression coverage for refresh failures, persistence, and restarts.

You can add or adjust custom eligibility rules. Learn more.

@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 3, 2026 — with ChatGPT Codex Connector
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 3, 2026 07:14

Dismissing prior approval to re-evaluate 94395e2

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 3, 2026
@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bb174163-b02b-4d3b-a01b-9062a9bc7608
📥 Commits

Reviewing files that changed from the base of the PR and between 94395e2 and da26d9a.

📒 Files selected for processing (4)
  • apps/server/src/provider/ProviderRegistry.test.ts
  • apps/server/src/provider/ProviderRegistry.ts
  • packages/provider-pi/src/server/status.test.ts
  • packages/provider-pi/src/server/status.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Pi model discovery now rejects malformed inventories. The provider registry retains cached models during incomplete discovery and applies reported inventories when the provider snapshot is ready or warning with known auth status.

Changes

Pi model inventory

Layer / File(s) Summary
Validate Pi discovery inventories
packages/provider-pi/src/server/status.ts, packages/provider-pi/src/server/status.test.ts
The parser rejects inventories that are not arrays and models with missing or blank provider or ID fields. Discovery propagates parser failures. Tests cover valid, empty, malformed, and partially invalid inventories.
Apply and persist authoritative inventories
apps/server/src/provider/ProviderRegistry.ts, apps/server/src/provider/ProviderRegistry.test.ts
The registry retains prior Pi models unless the installed provider has a ready or warning status and known auth status. Tests cover model removal, incomplete probes, and persistence across restarts.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk

Merge Risk: ⚪ Minimal · up to da26d

Completed Pi discovery can remove stale models, while incomplete or failed discovery retains the cached inventory. The change is mergeable after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to da26d

The change removes stale choices only after a valid, completed refresh and preserves prior choices when discovery fails. No expanded access or weakened permission check was established. Recovery under overlapping refreshes remains a limited uncertainty.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed outcome is the model inventory associated with a configured Pi instance and its status cache. The inspected paths do not establish increased credential authority or cross-instance exposure: the driver stamps instance identity, source correlation checks it, and cache hydration requires matching instance and driver.

Trust Boundaries and Controls

  • observed — The existing subprocess-response boundary now has stricter inventory validation. Only completed discovery produces the known-authentication state accepted by the Pi retention predicate; failed or timed-out discovery produces unknown authentication. This is an inventory-authority control, not evidence of a newly verified upstream identity or permission check.

Resilience and Maintainability Implications

  • observed — Existing managed-provider controls serialize probes, interrupt replaced enrichment work, and reject enrichment from obsolete generations. Discovery connections are scoped, and incomplete discovery cannot authorize model removal. These controls counter stale producer results, although they do not serialize downstream cache writes.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description covers the problem, expected behavior, implementation, scope rationale, verification commands, observed results, and known limitations. It is complete and aligned with the change.
Title check Passed The title is concise, uses conventional commit format, and accurately identifies the main fix: preventing stale Pi models from being restored.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 6, 2026
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 6, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 6, 2026
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 6, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 7, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 7, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 7, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
aliceisjustplaying added a commit to aliceisjustplaying/t3code that referenced this pull request Oct 8, 2026
pingdotgg#15529's new test parses and writes JSON with JSON.parse/stringify inside
Effect.gen, which the effect language service rejects (preferSchemaOverJson),
so the server typecheck failed on fork-stack. `ours` does not have that code to
change, so build-stack.sh now commits fixups/<pr>.patch once every PR is
merged; applying it earlier changes pingdotgg#14941's conflict with pingdotgg#15079 and its
recorded resolution no longer matches. The patch uses the Schema helpers pingdotgg#15079
adds to the same file.
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 9, 2026 06:40

Dismissing prior approval to re-evaluate da26d9a

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants