Skip to content

Keep provider diagnostics and model lists in sync#1660

Open
boudra wants to merge 2 commits into
mainfrom
pi-model-list-empty
Open

Keep provider diagnostics and model lists in sync#1660
boudra wants to merge 2 commits into
mainfrom
pi-model-list-empty

Conversation

@boudra

@boudra boudra commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Linked issue

N/A

Type of change

  • Bug fix
  • New feature (with prior issue + design alignment)
  • Refactor / code improvement
  • Docs

What does this PR do

Provider diagnostics now refresh and report from the same provider snapshot used by the model list. Availability checks stay cheap and only answer whether the provider binary is present; model and mode discovery happen through one catalog refresh path.

This also removes provider-specific diagnostic model probes, lets providers fetch models and modes together where possible, keeps custom added models visible when replacement models are configured, and prevents the model sheet from showing stale discovered models after a terminal refresh error or empty catalog.

How did you verify it

  • npm run format
  • Pre-commit hook: targeted format check, targeted lint, and npm run typecheck
  • npm run typecheck
  • npm run lint
  • Targeted tests:
    • packages/server/src/server/agent/provider-registry.test.ts
    • packages/server/src/server/agent/provider-snapshot-manager.test.ts
    • packages/server/src/server/agent/providers/opencode-agent.test.ts
    • packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts

No screenshot attached: the app change is state consistency in the existing provider model sheet, not a visual layout change.

Checklist

  • One focused change. Unrelated cleanups split out.
  • npm run typecheck passes
  • npm run lint passes
  • npm run format ran (Biome)
  • UI changes include screenshots or video for every affected platform
  • Tests added or updated where it made sense

@greptile-apps

greptile-apps Bot commented Jun 22, 2026

Copy link
Copy Markdown

Greptile Summary

This PR unifies the provider diagnostic and model-list paths so both read from the same fetchCatalog snapshot, eliminating the previous inconsistency where diagnostics fetched models through a separate probe that could disagree with the catalog the UI displayed. It also removes duplicate model/status probes from individual getDiagnostic implementations and simplifies isAvailable to a cheap binary-presence check.

  • fetchCatalog added to AgentClient and ProviderDefinition: ACP-based providers (Copilot, Cursor, Devin, …) now discover models and modes in a single process spawn; OpenCode acquires its server once and fetches both concurrently; Pi delegates listModels to fetchCatalog with a dedicated 120 s timeout. The snapshot manager calls fetchCatalog exclusively, replacing the prior parallel fetchModels+fetchModes calls.
  • Diagnostic sync: getProviderDiagnostic now force-refreshes the snapshot (via refreshSnapshotForCwdrefreshProviders(force=true)) before reading model count and status, ensuring what the user sees in the diagnostic sheet matches the model list. Provider getDiagnostic implementations are stripped down to availability and auth rows only.
  • UI fix in provider-diagnostic-sheet.tsx: When a refresh completes with an empty catalog (terminal error path), discoveredModels now returns [] rather than persisting the last known models; stale models are held only during an active providerSnapshotRefreshing window.

Confidence Score: 5/5

Safe to merge. The refactor is well-scoped: a single shared catalog call path replaces two separate fetches, diagnostics and model lists now read from the same snapshot authority, and all five affected providers are covered.

The core changes — introducing fetchCatalog, unifying the snapshot refresh path, and trimming individual getDiagnostic implementations — are internally consistent and well-tested. The refreshSnapshotForCwd path in getProviderDiagnostic truly force-refreshes (it calls refreshProviders with force: true internally). The only findings are a React render-timing subtlety in provider-diagnostic-sheet.tsx (resolved by restoring the synchronous ref update) and call-count assertions in new tests that pin dispatch details rather than behavior; neither affects runtime correctness in the common case.

packages/app/src/components/provider-diagnostic-sheet.tsx — the useEffect-based ref update introduces a brief window where batched renders may read a stale ref. The remaining files look correct.

Important Files Changed

Filename Overview
packages/server/src/server/agent/provider-snapshot-manager.ts Diagnostic now force-refreshes via a single fetchCatalog call and appends Models/Status from the same snapshot. refreshSnapshotForCwd always calls refreshProviders with force: true, so the force-refresh comment is accurate. Sequencing is correct: refreshSnapshotForCwd awaits the full refresh before getProvider reads the entry.
packages/server/src/server/agent/provider-registry.ts Adds required fetchCatalog to ProviderDefinition. createRegistryEntry.fetchCatalog correctly uses resolved.profileModels and resolved.profileModelsAreAdditive for catalog merging; wrapClientProvider also correctly merges. The hasReplacementModels fast-path skips runtime for model discovery while still fetching dynamic modes.
packages/app/src/components/provider-diagnostic-sheet.tsx Moves stableDiscoveredRef update from a synchronous render-body mutation into a useEffect. Fixes the React rule violation, but introduces a timing gap: if two renders are batched before the effect fires, the ref may not reflect the latest models in the second render's useMemo.
packages/server/src/server/agent/providers/acp-agent.ts Adds fetchCatalog that spawns a single ACP probe process to discover both models and modes, replacing the two-call listModels+listModes pattern used by the snapshot manager. Clean implementation.
packages/server/src/server/agent/providers/opencode-agent.ts Extracts fetchModelsFromClient and fetchModesFromClient private helpers; fetchCatalog acquires the server once and fetches both in parallel. Diagnostic strips the Server and model-count rows (now centralised in snapshot manager). Clean refactor.
packages/server/src/server/agent/providers/pi/agent.ts fetchCatalog is the new single-path for model discovery (with a dedicated 120s timeout). listModels delegates to it. isAvailable simplified to binary-presence check. Pi-specific Configured providers / Paseo MCP tools rows removed from getDiagnostic.
packages/server/src/server/agent/providers/generic-acp-agent.ts Removes runDiagnosticACPProbe and its ACP process spawning from getDiagnostic. Diagnostic now only checks binary availability; models/status come from the centralized snapshot. Significant reduction in diagnostic complexity.
packages/server/src/server/agent/provider-registry.test.ts New fetchCatalog tests cover replacement-model fast-path, additionalModels merging, and injected client dispatch. Key assertions verify catalog content (behavioral), but several tests also assert toHaveBeenCalledTimes for call-count verification of the optimization path.
packages/server/src/server/agent/providers/generic-acp-agent.diagnostic.test.ts Removes three tests that spun up fake ACP process probes and simplifies the remaining test to verify only binary-presence diagnostic rows. Matches the new lightweight getDiagnostic contract.
packages/server/src/server/agent/agent-sdk-types.ts Adds FetchCatalogOptions, ProviderCatalog, and optional fetchCatalog? to AgentClient. Types are clean and well-documented.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant App as ProviderDiagnosticSheet
    participant PSM as ProviderSnapshotManager
    participant Def as ProviderDefinition (createRegistryEntry)
    participant Client as AgentClient (e.g. ACPAgentClient)

    Note over App,Client: Model list refresh path
    App->>PSM: getSnapshot(cwd)
    PSM->>PSM: "refreshProviders(force=true)"
    PSM->>Def: "fetchCatalog({cwd, force})"
    alt client.fetchCatalog exists
        Def->>Client: client.fetchCatalog(options)
        Client-->>Def: "{models, modes}"
    else fallback
        Def->>Client: listModels + listModes (parallel)
        Client-->>Def: models[], modes[]
    end
    Def->>Def: mergeModels(profileModels, additionalModels, rawModels)
    Def-->>PSM: ProviderCatalog
    PSM-->>App: "snapshot entry {models, modes, status}"

    Note over App,Client: Diagnostic path (NEW - synced with model list)
    App->>PSM: getProviderDiagnostic(provider)
    PSM->>PSM: "refreshSnapshotForCwd → force=true via refreshProviders"
    PSM->>Def: "fetchCatalog({cwd, force=true})"
    Def-->>PSM: ProviderCatalog (same authority as model list)
    PSM->>Client: getDiagnostic() [availability + auth only]
    Client-->>PSM: baseDiagnostic string
    PSM->>PSM: append Models N / Status
    PSM-->>App: "{provider, diagnostic}"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant App as ProviderDiagnosticSheet
    participant PSM as ProviderSnapshotManager
    participant Def as ProviderDefinition (createRegistryEntry)
    participant Client as AgentClient (e.g. ACPAgentClient)

    Note over App,Client: Model list refresh path
    App->>PSM: getSnapshot(cwd)
    PSM->>PSM: "refreshProviders(force=true)"
    PSM->>Def: "fetchCatalog({cwd, force})"
    alt client.fetchCatalog exists
        Def->>Client: client.fetchCatalog(options)
        Client-->>Def: "{models, modes}"
    else fallback
        Def->>Client: listModels + listModes (parallel)
        Client-->>Def: models[], modes[]
    end
    Def->>Def: mergeModels(profileModels, additionalModels, rawModels)
    Def-->>PSM: ProviderCatalog
    PSM-->>App: "snapshot entry {models, modes, status}"

    Note over App,Client: Diagnostic path (NEW - synced with model list)
    App->>PSM: getProviderDiagnostic(provider)
    PSM->>PSM: "refreshSnapshotForCwd → force=true via refreshProviders"
    PSM->>Def: "fetchCatalog({cwd, force=true})"
    Def-->>PSM: ProviderCatalog (same authority as model list)
    PSM->>Client: getDiagnostic() [availability + auth only]
    Client-->>PSM: baseDiagnostic string
    PSM->>PSM: append Models N / Status
    PSM-->>App: "{provider, diagnostic}"
Loading

Reviews (2): Last reviewed commit: "fix(server): preserve catalog profile mo..." | Re-trigger Greptile

Comment thread packages/server/src/server/agent/provider-registry.ts
@boudra

boudra commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator Author

On the Pi-specific diagnostic rows: removing those runtime-derived rows is intentional for this PR. Configured providers and Paseo MCP tools both require starting Pi and loading extensions, which is the slow side effect this change is avoiding in diagnostics. The centralized snapshot still appends the authoritative model count and status after one catalog refresh; provider diagnostics stay limited to cheap command/binary/auth facts.

@joyshan1986

Copy link
Copy Markdown

Confirming this resolves a concrete, reproducible case on Windows.

Symptom: Pi showed as not installed / no models in the provider list, while the diagnostic panel simultaneously reported Status: Available with 25 models — the exact diagnostics-vs-model-list mismatch this PR targets.

Root cause (matches this PR): PiAgent.isAvailable() wrapped checkProviderLaunchAvailable + a full pi --mode rpc spawn + getAvailableModels().length > 0 in a hard 2000 ms withTimeout. On this machine pi --mode rpc cold-starts in ~2.1–2.4 s, so isAvailable() always timed out → false → the daemon threw Provider 'pi' is not available and refused to list models. The diagnostic path has no 2000 ms cap, which is why it alone reported "Available".

Environment / measurements:

  • Windows 11, Paseo 0.1.98, pi (@earendil-works/pi-coding-agent) 0.79.10
  • pi --mode rpc + get_available_models → first response at 2135–2227 ms across repeated runs (25 models), never under 2000 ms
  • node dist/cli.js --mode rpc directly ≈ 2140 ms, so it's pi's own cold-start, not the cmd.exe/.cmd shim
  • Even pi --version ≈ 2.4 s

Confirms the fix's direction: as a stopgap I raised only that single literal (20009000 ms) in the installed app.asar, and Pi immediately became available with all 25 models. Since this PR makes isAvailable() answer purely from binary presence (no model fetch under a tight cap), it removes the trigger entirely rather than just widening the window.

Closely related: #1541 (its "Root Cause 1" is this same isAvailable() full-session spawn).

Thanks for the fix!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants