Update the Kanata bundle and harden Sparkle upgrades - #1247
Conversation
2cecce7 to
aa7325a
Compare
|
Reviewed the supplied diff (truncated after Findings from the visible portion:
No other actionable correctness/security issues spotted in the visible diff. Given the truncation, I'd recommend a manual pass over |
|
Follow-up on the review notes:
No follow-up code change is required for these findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa7325a98b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case let .runtimeShutdownRequired(reason): | ||
| AppLogger.shared.log("⏸️ [UpdateService] Pre-update: stopping bundled Kanata runtime (\(reason))") | ||
| let report = await engine.runSingleAction(.terminateConflictingProcesses, using: broker) |
There was a problem hiding this comment.
Stop the daemon instead of killing only its process
When the registered com.keypath.kanata daemon is active, this action ultimately calls killAllKanataProcesses(), which sends pkill but does not unregister or stop its SMAppService job. The bundled launchd plist has KeepAlive for unsuccessful exits, so the signal-killed runtime can respawn during the throttle window; even if postcondition inspection detects the restart and returns failure, the defer still releases Sparkle to replace the bundle. Stop and verify the daemon through ServiceLifecycleCoordinator before invoking the install handler.
AGENTS.md reference: AGENTS.md:L331-L332
Useful? React with 👍 / 👎.
| Toggle("Download and install updates automatically", isOn: Binding( | ||
| get: { updateService.automaticallyDownloadsUpdates }, | ||
| set: { updateService.setAutomaticDownloads(enabled: $0) } | ||
| )) | ||
| .disabled(!updateService.allowsAutomaticUpdates) | ||
| .accessibilityIdentifier("about-auto-install-toggle") |
There was a problem hiding this comment.
Document the new automatic-install preference
This adds a user-visible setting that changes whether updates are downloaded and installed automatically, but the commit does not add or update any file under guides/; the only existing user documentation merely says automatic checks can be disabled and does not explain this separate preference or its default. Add this behavior to an appropriate published guide as required for user-visible features.
AGENTS.md reference: AGENTS.md:L363-L364
Useful? React with 👍 / 👎.
| if [[ ! "$RELEASE_BUILD_NUMBER" =~ ^[0-9]+$ ]]; then | ||
| echo "❌ KEYPATH_RELEASE_BUILD_NUMBER must be numeric: $RELEASE_BUILD_NUMBER" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Reject non-increasing release build overrides
When KEYPATH_RELEASE_BUILD_NUMBER is provided, this validates only that it is numeric, so a value equal to or lower than CURRENT_BUILD is written into CFBundleVersion and used in the generated appcast. Sparkle orders updates by this build number, meaning existing users may not be offered the newly published release. Require the override to be strictly greater than the current numeric build before modifying the plist.
Useful? React with 👍 / 👎.
Summary
generate_appcasttoolingSafety
SKIP_DEPLOY=1;/Applications/KeyPath.appwas not replacedValidation
SparkleUpdateContractTests,ReleaseSigningContractTests, andUpdateServiceDecisionTests: pass1.12.1-prerelease-1, and VirtualHID v8Scripts/release-doctor.sh --shipwith website check skipped: pass, with expected dirty-worktree/tooling warningsScripts/release.sh --dry-run 1.0.1-beta1: pass; preserves numeric build5git diff --check: passKnown unrelated baseline issue
The unfiltered full suite reports three
EasyViewSnapshotTestsfailures because current render output is 1300x586 while the committed Home Row Timing references are 1300x638:testHomeRowTimingFastTypingtestHomeRowTimingPerFingertestHomeRowTimingSliderThis branch does not modify Home Row Timing code or those snapshots. The suite completes cleanly when only those three known mismatches are excluded; this PR intentionally does not rewrite unrelated baselines.
Remaining acceptance
claude-review