Skip to content

fix(pi): remove stale models after successful discovery - #16602

Closed
RobertoVillegas wants to merge 1 commit into
pingdotgg:mainfrom
RobertoVillegas:fix/pi-stale-model-catalog
Closed

RobertoVillegas wants to merge 1 commit into
pingdotgg:mainfrom
RobertoVillegas:fix/pi-stale-model-catalog

Conversation

@RobertoVillegas

Copy link
Copy Markdown

Problem

After uninstalling a Pi provider extension, its models remain selectable in T3 Code even after refreshing Pi and restarting T3. Pi no longer reports those models, so selecting them fails.

Reproduced on macOS arm64 with Pi 1.0.4 and T3 Code Nightly 0.0.46-nightly.20261006.2735 (fd1c3386c4d6):

  1. Install pi-cursor-sdk in Pi and let T3 discover its models.
  2. Remove the extension from Pi.
  3. Confirm pi --list-models composer reports no matching models.
  4. Refresh the Pi provider in T3 and restart T3.
  5. Composer remains in the Pi model picker. The backend cache ~/.t3/caches/pi.json still contains 246 cursor/… entries, including 15 Composer variants.

Removing only that cache with T3 fully closed resolves the stale list. Removing the plugin's own model cache did not.

ProviderRegistry always retains missing discovered models for Pi, then persists the merged list again. A refresh therefore cannot remove models from an uninstalled extension.

Fix

Treat completed Pi discovery as authoritative, including successful discovery with no usable models. Pi reports authenticated/ready for usable models and unauthenticated/warning for an empty inventory. Failed or interactive discovery keeps unknown auth; those partial snapshots still retain the last known models.

The existing capability merge and custom-model handling are unchanged. No cache migration, manual cleanup, UI change, or other provider behavior change is needed: the next successful Pi refresh removes the stale entries.

This is a focused fix for an obvious refresh defect, submitted under the small bug-fix exception in CONTRIBUTING.md.

Verification

Added four focused regression tests covering removed extension models, successful empty discovery, incomplete discovery, and cache persistence through failed refreshes and registry restarts.

  • Before the fix: the new Pi tests had 3 failures and 1 pass; Composer was incorrectly retained in both the merged inventory and the cache written by the registry.
  • After the fix: pnpm exec vp test run apps/server/src/provider/ProviderRegistry.test.ts apps/server/src/provider/PiProvider.test.ts apps/server/src/provider/providerStatusCache.test.ts — 68 tests passed.
  • pnpm --filter t3 typecheck — passed.
  • Targeted vp fmt --check, vp lint, and git diff --check — passed.

The packaged app was used to establish the original failure and verify the cache-removal workaround; the patched backend is verified through focused tests, not a rebuilt desktop app. No repo-wide checks were run.

AI assistance: OpenAI gpt-6.1-sol in Pi coding-agent.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Oct 6, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 4e4e15c

Macroscope's review found this PR approvable — This is a focused provider-registry bug fix that removes stale Pi extension models after completed discovery while preserving cached models when discovery is incomplete. The production change is small, isolated, and accompanied by targeted regression coverage for inventory and cache behavior.

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

@coderabbitai

coderabbitai Bot commented Oct 6, 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: 22538d88-543f-448f-a146-f742738ae4f0
📥 Commits

Reviewing files that changed from the base of the PR and between f4f148e and 4e4e15c.

📒 Files selected for processing (2)
  • apps/server/src/provider/ProviderRegistry.test.ts
  • apps/server/src/provider/ProviderRegistry.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

The provider registry now removes missing cached Pi models after completed discovery. It retains cached models when a Pi probe is incomplete. Tests cover inventory replacement and persistence across refreshes and registry restarts.

Changes

Pi model inventory

Layer / File(s) Summary
Pi inventory retention and persistence
apps/server/src/provider/ProviderRegistry.ts, apps/server/src/provider/ProviderRegistry.test.ts
The registry retains missing Pi models unless the provider is installed, has ready or warning status, and reports authenticated or unauthenticated auth. Tests cover completed discovery replacing the cached inventory, incomplete probes retaining it, and removed models staying removed after a failed refresh and restart.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 4e4e1

Stale Pi models should be removed after completed discovery and preserved when discovery is incomplete. No actionable merge-blocking risk remains after normal checks.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 4e4e1

The change removes outdated choices after completed discovery without adding permissions or expanding access. Failed or incomplete discovery preserves the last known choices. No material security risk was identified in the changed behavior.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed outcome is removal of missing catalog rows from one Pi instance's in-memory snapshot and persisted cache. Normal registry aggregation and cache routing use instance identity rather than merging all instances of the same driver.

Trust Boundaries and Controls

  • observed — Discovery still consumes responses from the configured Pi process under the existing launch configuration. Before registry synchronization, snapshots must match both the originating instance ID and driver. The PR changes omission handling after that correlation; it does not change the process launch or those identity checks.

Resilience and Maintainability Implications

  • observed — The added regression assertions cover removed extension models, completed empty discovery, incomplete discovery retention, and persistence through a later failed discovery and simulated restart. They do not establish durability after a failed cache write or ordering under overlapping refreshes, and were not executed during this review.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Pi fix: successful discovery removes stale models. It is concise and follows the repository’s conventional title style.
Description check ✅ Passed The description covers the problem, fix, scope rationale, and focused verification results. Although it uses “Fix” instead of the template’s “Change” heading, the change and its boundaries are clear.
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.
Approvability ✅ Passed The PR changes only ProviderRegistry.ts and ProviderRegistry.test.ts. The code changes Pi model-retention behavior after completed discovery; it does not change a product default, authentication b…
✨ 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.

@RobertoVillegas

Copy link
Copy Markdown
Author

Closing as a duplicate of #15079, which I missed in the initial search. Both PRs fix the same Pi inventory retention rule; #15079 also validates malformed RPC inventories before treating discovery as authoritative and already covers cache persistence and restart behavior. My reproduction with pi-cursor-sdk/Composer confirms the same defect on nightly fd1c338. No separate fix is needed here.

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

Labels

size:S 10-29 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.

1 participant