Require confirmed provider updates#137
Merged
Merged
Conversation
yaacovcorcos
marked this pull request as ready for review
July 26, 2026 12:48
…der-update-action
…findings Ensure the provider updater child process is only ever spawned against a target that was probed, certified, and re-validated under the settings write lock, closing six concurrency/security windows in the confirmed-update boundary: - #1 Immutable probe/settings snapshot threaded through refresh; a single serialized refresh (refreshSemaphore) captures one snapshot and revalidates confirmed targets at the commit boundary. - #2 updateProvider re-validates the confirmed target and captures the exact updater command into immutable locals while holding withSettingsWriteLock, so a concurrent settings write cannot change what gets spawned between validation and capture. The child is spawned OUTSIDE the lock (spawn + await share one update-timeout budget), so a slow or hung spawner.spawn — whose acquire is uninterruptible — can only stall its own request and can never pin the global settings write lock. - #3 Request-owned update state (per-request owner token); a losing duplicate cannot clobber the in-flight update's running state. - #4 Interruption-safe cleanup lands a terminal failed state and kills the child via a scoped finalizer. - #5 Hot getStatuses/stream reads re-derive only a cheap authority key (revision counters) instead of the full maintenance context, so reads never re-probe CLIs or re-resolve the runtime. - #6 Runtime target identity tracked by a monotonic per-provider revision counter (ProviderRuntimeManager.getRevision), excluded from transient install-progress churn; PROVIDER_KINDS derived from ProviderKind.literals. Adds deterministic regression tests for each finding (no sleeps; barriers via Deferred / TestClock.withLive / scheduler drains), including a mutation- sensitive hot-read guard, a mutation-sensitive guard that the settings write lock is released before the unlocked spawn, a hung-process timeout guard, succeeded/unchanged post-update re-probe guards, and a per-provider revision-isolation test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
Independent review certification — head
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This fixes the observed Cursor and Antigravity contradiction where a row could offer Update while the summary said no provider updates were detected, and makes the update path safe under concurrent settings writes, refreshes, duplicate requests, and interruption.
Behavior contract
An action now requires all of the following: provider available, advisory
behind_latest, known installed version, known latest version, completed check timestamp, and a supported update path. Unknown, failed, unavailable, stale cached, unsupported, hidden, disabled, and update-check-disabled states do not expose Update. An already queued/running update remains visibly disabled as Updating even if advisory evidence changes.Concurrency & security hardening
The updater child process is only ever spawned against a target that was probed, certified, and re-validated while holding the settings write lock. Six windows were closed:
refreshSemaphore) captures one immutable probe/settings snapshot, threads it through load → revalidate → project, and drops confirmed targets whose embedded authority no longer matches at the commit boundary.updateProviderre-validates the confirmed target and captures the exact updater command into immutable locals while holdingwithSettingsWriteLock, so a concurrent settings write cannot change what gets spawned between validation and capture. The child is spawned outside the lock (spawn + await share a single update-timeout budget), so a slow or hungspawner.spawncan only stall its own request — it can never pin the global settings write lock.runningstate; owner-scoped writes only land while the request still owns the slot.failedstate and the hung child is stopped via a scoped kill finalizer.getStatuses/stream reads re-derive only a cheap authority key (revision counters) instead of the full maintenance context, so hot reads never re-probe provider CLIs or re-resolve the runtime.ProviderRuntimeManager.getRevision) that advances only on real target mutations (transient install progress is excluded).PROVIDER_KINDSis derived fromProviderKind.literalsso new providers are covered automatically.Authority-key derivation is byte-identical between the hot path (
getProviderAuthorityKey) and the full context (getProviderMaintenanceContext).Tests
Deterministic regression tests were added for each finding (no sleeps; determinism via
Deferred, rawPromise,TestClock.withLive, and scheduler drains), including:resolve/getSnapshotcalls acrossgetStatuses(fails if the hot path reverts to the expensive maintenance context),awaitnever completes is stopped by the scoped kill finalizer and bounded by the update timeout, reported as a soft failure (never a hang),refreshNowre-probes the CLI (rather than reusing the pre-update version) and that the re-probed advisory drives the terminalsucceededvs.unchangedstate and message,Verification
bunx oxfmt --check(clean)bun run lint(0 errors; pre-existing warnings only)tsc --noEmit(clean); webtsc --noEmit(clean)bun run test: 2378 passed, 10 skippedbun run build: 5/5 successfulFollow-ups (out of scope, low-severity)
These were identified during review as cosmetic/polish and are intentionally deferred to keep this security-critical change tightly scoped:
startedAt— when the command lock fails or is interrupted, the terminalfailedstate is written withstartedAt: null, dropping the original start time for a request that had already reachedrunning. Also, arefreshNowfailure that occurs after the update command itself succeeded is currently surfaced as a failed update rather than a succeeded-but-refresh-failed state.queuedflicker — an update with no contender can briefly presentqueuedbeforerunning. Purely cosmetic; the terminal state is unaffected.Risk and rollback
The change centralizes the action gate, serializes the confirmed-update boundary, and invalidates only network-backed update metadata loaded from disk cache. Provider availability, version display, authentication, connection, install/repair, custom paths, and active update state are retained. The hardening is additive and preserves all previously verified behavior (byte-identical user-facing messages). Rollback is a branch revert.