fix: surface why a launch-time recorder upgrade was skipped - #228
Merged
Merged
Conversation
The app upgrades the recorder (CLI + daemon) on launch when it is newer than the installed one, but upgradeInstalledCLIIfNeeded returned .notNeeded identically whether the recorder was already current or could not be recognized at all. That silent no-op made an app-newer-than-recorder skew impossible to diagnose: nothing upgraded, and nothing said why. Add upgradeBlockedReason: when a recorder is present but no upgrade context can be built — the app cannot verify its own bundled recorder, or the installed recorder is not recognized as app-owned — name the reason. upgradeInstalledCLIIfNeeded records it on recorderUpgradeDiagnostic and logs it instead of skipping silently, and clears it once an upgrade proceeds. The reason is computed only inside the upgrade check (which already does the heavy identity/provenance work), never during rendering, so it does not traverse or hash payloads. Add the regression test that was missing: a legacy install whose Python.framework keeps its symlinks is auto-upgraded by a newer app (the shape #216 enabled and a pre-#216 inspector could not recognize), plus coverage for both blocked-reason branches (installed recorder unrecognized; bundled recorder unverifiable).
Merged
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
1 participant
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.
Problem
The app upgrades its recorder (CLI + daemon) on launch when the app is newer than the installed recorder — but
upgradeInstalledCLIIfNeededreturned.notNeededidentically whether the recorder was already current or whether it could not be recognized at all. So when the app is newer than the recorder yet can't build an upgrade context, nothing upgrades and nothing says why — an app-newer-than-recorder skew is invisible to the user and undiagnosable from outside the app. (This is exactly the shape seen on a machine where a 0.10.8 app sat over an older recorder: "recorder unreachable," daemon left behind, nocli-versions/staged, no signal.)Related: #216 is what lets a 0.10.8 app recognize a symlinked legacy recorder in the first place (the pre-#216 inspector rejected framework symlinks → the recorder was unrecognizable → this same silent
.notNeeded). But there was no regression test for that launch-upgrade path with a real Python.framework-shaped payload, which is why it could regress silently.Change
upgradeBlockedReason: when the launch upgrade does nothing and no recovery is pending, name why a present recorder couldn't be carried to this app's version — either the app can't verify its own bundled recorder, or the installed recorder isn't recognized as app-owned.nilwhen nothing is installed, when the installed recorder already matches, or when a recovery path owns the outcome.upgradeInstalledCLIIfNeededrecords it on a new@Published recorderUpgradeDiagnosticand logsRecorder update skipped: …instead of a silent skip; it's cleared once an upgrade proceeds. The upgrade decision itself is unchanged — this only makes the no-op observable.Tests
testAutomaticUpgradeOfLegacyInstallWithFrameworkSymlinks— the missing regression guard: a legacy install whose Python.framework keeps its four canonical relative symlinks is auto-upgraded by a newer app, and leaves no false blocked-reason.testLegacyUpgradeReportsWhyWhenInstalledRecorderIsUnrecognized— the installed-recorder recognition failure (the exact fix: keep App, CLI, and public contracts aligned #188 chain) is now reported, not silent.testLaunchUpgradeReportsWhenBundledRecorderCannotBeVerified— the bundled-recorder branch (the one a real signed bundle could trip, invisible from outside) is reported.swift test— 505 executed, 0 failures, 6 skipped.swift build -c release— clean.Follow-up
This makes the failing gate observable, which is what lets us finally see which gate nils on a given machine. Surfacing
recorderUpgradeDiagnosticprominently in the recorder-health UI (so a user sees "recorder needs reinstall" instead of a bare "unreachable") is a small follow-up left to the app's owner.