fix(server): stale provider update results clear once the provider is up to date#24338

open3

main

GitHub fork: fix/24330-provider-update-message-persists

opened18d agobyFfixfon

View on GitHubTwo-way sync enabledPR last imported 1d ago

Problem

After a provider update ends as unchanged or failed, the result message stays under the provider row until the server restarts. This holds even after the install is fixed outside T3 Code and Refresh providers shows the provider is current. The row then shows 0.164.0 · Latest 0.164.0 together with "Update command completed, but T3 Code still detects an outdated provider version.", which contradicts it (#24330).

ProviderRegistry keeps update results in maintenanceActionStatesRef, and applyProviderUpdateState attaches the stored result to every refreshed snapshot. Nothing removes the entry on refresh. It is only dropped when the instance disappears or when a new update replaces it.

Change

A new retireStaleUpdateState runs in syncProvider, on snapshots that come from a provider source (refresh and stream). When such a snapshot reports versionAdvisory.status === "current", an unchanged or failed update result for that instance is deleted from maintenanceActionStatesRef, so it does not come back on a later refresh. A later update writes a fresh entry as before.

It checks for current and not for "not behind_latest". A probe publishes unknown until the latest-version check finishes, so "not behind" would clear the result of a provider that is still outdated.
It does not run in setProviderMaintenanceActionState. That path re-applies the stored snapshot, whose advisory may already read current, and it would discard the result the update runner has just recorded.
queued, running and succeeded results are kept. A provider that is still behind_latest or unknown keeps its unchanged or failed result.

Known side effect: a failed result on a provider that was already current (for example a targeted version install) is also cleared at the next current snapshot.

Known gap: "Provider updated." (succeeded) still stays, as the reporter also mentions for Claude. It is true after the update and has no "no longer outdated" moment to key on, so I left it alone. It would need its own rule.

The change is limited to apps/server/src/provider/ProviderRegistry.ts and its test file.

Scope and approval

Fixes #24330, a labeled bug report with steps to reproduce and the cause traced to this code path. It has no maintainer comment yet. The report suggests this direction (drop a terminal unchanged/failed state once the provider is no longer outdated) and leaves the choice to maintainers. This change does the first option and keeps the behavior of every provider that is still outdated.

Verification

This is server state with no UI change, so the proof is a test in the module's existing test file. The new update results across refreshes tests use the real ProviderRegistry layer over a fake provider source. They set an update result, switch the source snapshot to a given advisory status, call refresh, and check the snapshot's updateState:

retired: unchanged + current, failed + current
kept: unchanged or failed + behind_latest, unchanged + unknown, queued + current, running + current
a retired result does not come back after a later unknown re-probe, and a later running update still reports

Commands run in the worktree:

vp test run apps/server/src/provider/ProviderRegistry.test.ts -t "update results across refreshes" without the source change: 3 failed, 5 passed. The failures are unchanged + current, failed + current, and the "does not come back" test.
The same command with the change: 8 passed. After reverting only ProviderRegistry.ts again: the same 3 fail.
vp test run apps/server/src/provider/ProviderRegistry.test.ts apps/server/src/provider/providerMaintenanceRunner.test.ts: 63 passed (45 and 18).
vp lint and vp fmt --check on the two changed files: no findings.
vp run --filter t3 typecheck: exit 0.
vp run check:changed: exit 0 (format, lint, knip, typecheck, tests).

Not checked: I did not run the server or the app, so I did not see the settings row itself before and after. The behavior is covered by the tests above only.

Claude Sonnet 5.5 (xhigh) via T3 Code.

Fixes pingdotgg/t3code#24330

Checks

21 successful · 9 skipped

All checks passed

Added 1 commit18d ago

fix(server): stale provider update results clear once the provider is up to dateade8819

MmacroscopeappBotapproved these changes18d ago

MmacroscopeappBot18d ago

Approvability

Verdict: Approved at ade8819

Macroscope's review found this PR approvable — This is a narrowly scoped server bug fix that retires only stale unchanged/failed update results after a provider is confirmed current. Focused tests cover the affected refresh states and preserve active update reporting.

^{You can add or adjust custom eligibility rules. Learn more.}

CcoderabbitaiBot18d ago

Review in Change Stack →

No actionable comments were generated in the recent review.

Walkthrough

ProviderRegistry now retires stored unchanged or failed update results when a refreshed provider reports a current version advisory. Tests cover retained states, other advisory statuses, and behavior across later refreshes.

Changes: Provider update state

Layer / File(s)	Summary
Retire eligible results during refresh `apps/server/src/provider/ProviderRegistry.ts`	`syncProvider` removes stored `unchanged` or `failed` update state when the incoming snapshot has a `current` advisory, before upserting the snapshot.
Verify state across refreshes `apps/server/src/provider/ProviderRegistry.test.ts`	Tests check retirement conditions, retained update states, and behavior through subsequent refreshes.

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

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

pingdotgg/t3code#24062: Changes the latest-version source for npm-global installs, which contributes to the advisory status used by this change.
pingdotgg/t3code#24009: Changes the latest-version lookup for standalone Codex installs, which contributes to the advisory status used by this change.

Suggested labels: bugfix, size:M

Suggested reviewers: juliusmarminge, macroscopeapp

Merge Risk: External image Minimal · up to ade88

No merge-blocking issue is established; the change is ready for normal checks.

Pre-merge checks: ^Passed 4

Check name	Status	Explanation
Title check	Passed	The title clearly and concisely describes the main fix: stale provider update results are cleared after the provider becomes current.
Linked Issues check	Passed	Issue `#24330` requires the stale update result to clear or hide after refresh reports that the provider is current. The supplied change summary and head source show that `ProviderRegistry` retires stor…
Out of Scope Changes check	Passed	The supplied change summary limits the implementation to `apps/server/src/provider/ProviderRegistry.ts` and its test file. The production change supports issue `#24330` directly. The added tests verify …
Description check	Passed	The description covers the problem, change, scope, and focused verification. It links the issue and explains the rationale for proceeding without maintainer approval; it also states the known gap and …

Full details: Linked Issues check

Explanation

Issue #24330 requires the stale update result to clear or hide after refresh reports that the provider is current. The supplied change summary and head source show that ProviderRegistry retires stored unchanged and failed results when a provider-source snapshot has versionAdvisory.status === "current". The tests cover retirement, retention for behind_latest and unknown, retention of active states, and later refreshes. The issue describes the succeeded message as harmless, so its documented retention is not an unmet coding requirement.

Full details: Out of Scope Changes check

Explanation

The supplied change summary limits the implementation to apps/server/src/provider/ProviderRegistry.ts and its test file. The production change supports issue #24330 directly. The added tests verify the required refresh behavior. No unrelated change is established.

Full details: Description check

Explanation

The description covers the problem, change, scope, and focused verification. It links the issue and explains the rationale for proceeding without maintainer approval; it also states the known gap and checks not run.

Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

External image Recent review info

External image Run configuration

Source mode: Auto
Repository: pingdotgg/t3code
Branch: main
File: .coderabbit.config.ts
Commit: d14ace8
Layers: Repository TS config
Review profile: CHILL
Plan: Advanced
Run ID: `723f4cd4-3044-45f3-a477-cf855126c61b`

External image Commits

Reviewing files that changed from the base of the PR and between d14ace8233ad3cf7856b6b834824f888240d67a1 and ade8819452be3509083b5647f65fb3512bd5434a.

External image 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.

Added 2 commits16d ago

Merge remote-tracking branch 'github/main' into takeover-243383cae108
fix(server): preserve targeted provider install failures on refresh4e4abb7

Mmaria16d ago

Note

Written by gpt-6.1-sol on behalf of Maria

thanks @fixfon. your source-snapshot retirement rule and refresh regression tests address the stale result correctly. i kept your commit, merged current main, and pushed a focused follow-up to this branch (4e4abb7fe694238c4606f285291ae5313df84629).

preserve failed and unchanged recommended-version installs by carrying the requested version privately from the maintenance runner into the registry. latest-version status cannot prove that a recommended-version install succeeded.
inline the retirement rule and simplify the update-only state setter. no wire contract or client changes.
extend existing tests for recommended-version retention, succeeded/unknown retention, and runner target propagation. no new test files.

verification: current main failed three refresh regression cases; the original implementation failed both recommended-version cases; the runner failed three target propagation cases before the fix. all 69 focused registry/runner tests now pass. blacksmith vp run check:changed passed format, lint, knip, graphql, affected typechecks and tests. the worker uses a shallow source snapshot, so its local-only history was connected to the fetched base for change detection. both independent gpt-6.1-sol reviews found no issues on this head.

linux chrome exercised the real settings → provider update → server maintenance runner → disposable claude wrapper path. the wrapper delegates capabilities/auth to the installed cli, fails only the installer, and controls its reported version. after failure, a current refresh followed by another outdated probe restores the old error on main; this head keeps it cleared. screenshots use matching data and viewport. the short recording is slowed for readability and makes no timing claim.

unverified: real provider upgrades, recommended-version installs in a live client (covered by registry and runner tests), other provider binaries, electron/mobile, windows/macos and remote/relay/tunnel sessions. the shared server rule applies to all callers and clients. the displayed claude authentication warning is expected on this vm.

main: stale installer error returns after a current refresh and later probe

updated branch: stale installer error stays cleared with the same data

provider update and refresh interaction, slowed for readability

all 28 reported checks are green on this head; cafe reports mergeable. coderabbit reports no actionable findings. the existing macroscope approval is on the contributor's earlier head, so no new macroscope approval is claimed. the two independent reviews above cover the final head. left open and unmerged for maria.

Mmaria14d ago

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified as-is at 4e4abb7fe694238c4606f285291ae5313df84629. no code changes or pushes.

ran github/main (975b3dc568) in a separate worktree first, then the exact pr head, with vp i and one background dev server at a time. drove the actual web client using the repo Playwright dependency and /usr/bin/google-chrome. created a project for a real git repository and threads through the app. real Codex gpt-6.1-sol turns completed on main and the pr, including a fresh thread after an app-driven provider upgrade. no mocks, wrappers, fixtures, or seeded data.

used an actual npm installation of Codex 0.164.0 in a disposable prefix. making its installation directory read-only caused the app update command to fail with real npm EACCES. repaired permissions and installed 0.165.0 externally, then clicked refresh. main retained the failed result despite reporting current. the pr removed it. changing back to 0.164.0 and refreshing restored the stale error on main, while the pr kept it cleared. the screenshots show that matching final state at the same viewport; main has newer settings navigation.

observed over the real client websocket:

text

1	main: 0.164.0 / behind_latest / failed (npm EACCES)
2	main: external install 0.165.0 + refresh -> current / failed (bug)
3	main: external install 0.164.0 + refresh -> behind_latest / failed (stale)
4	pr: 0.164.0 / behind_latest / failed (npm EACCES)
5	pr: external install 0.165.0 + refresh -> current / no updateState
6	pr: external install 0.164.0 + refresh -> behind_latest / no updateState
7	pr: another failed update + outdated refresh -> failed retained
8	pr: writable prefix + app Update now -> 0.165.0 / current / succeeded
9	pr: fresh gpt-6.1-sol thread -> PROVIDER_VERIFY_AFTER_UPDATE_OK

blacksmith passed all 69 registry/maintenance-runner tests and vp run check:changed 7d3208cbea86b91574366bb6e87d05f93c975127 (format, lint, knip, graphql, affected typechecks, tests). restored shallow worker history and passed the comparison base explicitly. all 30 reported pr checks are successful; gitcafe still reports a pending current-base conflict observation, so this is no claim of merge readiness.

main: stale failure returns after a current refresh and later outdated probe

pr: the same outdated probe keeps the retired failure cleared

pr: refreshing after external repair clears the failed update

pr: a fresh real codex turn completes after the app successfully upgrades codex

inspected the captures and verified uploaded video playback and seeking in t3. the recording trims the external-install wait and makes no timing claim. stopped both task dev servers and the blacksmith worker.

Not verified

native iOS/Android, Electron, macOS/Windows, remote/relay/tunnel connections, other provider binaries, and the unchanged installer outcome through the live app. recommended-version failure retention was covered by the existing registry and runner tests, but not a live targeted install. the displayed Claude authentication warning is unrelated; Claude is not authenticated on this VM.

gitcafe renders these external media embeds as links. reopened the comment, checked each image against its uploaded bytes, and opened the video directly to verify playback and seeking. inline media rendering in gitcafe is unavailable.