Skip to content

Update the Kanata bundle and harden Sparkle upgrades - #1247

Merged
malpern merged 4 commits into
masterfrom
codex/kanata-current-upstream-candidate
Jul 30, 2026
Merged

Update the Kanata bundle and harden Sparkle upgrades#1247
malpern merged 4 commits into
masterfrom
codex/kanata-current-upstream-candidate

Conversation

@malpern

@malpern malpern commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

  • update the bundled Kanata runtime to the current upstream candidate and migrate KeyPath to VirtualHID v8
  • quiesce the managed Kanata runtime before Sparkle replaces the app bundle, then repair the installed runtime after relaunch
  • pin Sparkle 2.9.4 and enable signed-feed, pre-extraction verification, and user-controlled automatic download/install
  • replace hand-built update XML and manual archive signing with Sparkle's official generate_appcast tooling
  • verify the release signing identity against the public key embedded in KeyPath without exposing the private key
  • keep release build numbers numeric and monotonic for prerelease versions
  • add release-contract tests and update release documentation

Safety

  • system profiling remains disabled
  • release credentials are read from the encrypted local secrets store or dedicated Keychain account and sent to Sparkle tools over stdin
  • the candidate was built and Developer ID signed with SKIP_DEPLOY=1; /Applications/KeyPath.app was not replaced
  • no release or appcast was published

Validation

  • SparkleUpdateContractTests, ReleaseSigningContractTests, and UpdateServiceDecisionTests: pass
  • full suite excluding three pre-existing Home Row Timing snapshot-size mismatches: 5,053 passed
  • safe signed candidate build: pass, including complete signed appcast, archive, DMG, identity contract, Kanata 1.12.1-prerelease-1, and VirtualHID v8
  • Scripts/release-doctor.sh --ship with website check skipped: pass, with expected dirty-worktree/tooling warnings
  • Scripts/release.sh --dry-run 1.0.1-beta1: pass; preserves numeric build 5
  • SwiftFormat lint, accessibility identifier audit, shell syntax, plist/XML parsing, appcast signature verification, and git diff --check: pass

Known unrelated baseline issue

The unfiltered full suite reports three EasyViewSnapshotTests failures because current render output is 1300x586 while the committed Home Row Timing references are 1300x638:

  • testHomeRowTimingFastTyping
  • testHomeRowTimingPerFinger
  • testHomeRowTimingSlider

This 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

  • GitHub CI and claude-review
  • after merge, a controlled lab upgrade from an older signed KeyPath build through a test appcast, verifying the complete KeyPath + Kanata + VirtualHID replacement and post-update repair path

@malpern
malpern force-pushed the codex/kanata-current-upstream-candidate branch from 2cecce7 to aa7325a Compare July 30, 2026 04:40
@claude

claude Bot commented Jul 30, 2026

Copy link
Copy Markdown

Reviewed the supplied diff (truncated after Scripts/release.shUpdateService.swift, VHIDDeviceManager.swift, AboutView.swift, the new test files, appcast.xml, and the docs changes were not shown, so I could not review those portions).

Findings from the visible portion:

  1. Scripts/build-kanata.sh — the comment in calculate_source_hash() was rewritten to claim the cache hash now "includes the Cargo manifests and lockfile that pin the external karabiner-driverkit revision," replacing the old comment about hashing vendored C/C++ sources. The diff only shows the comment changing, not the function body that computes the hash. If the hash inputs weren't actually updated to include Rust/KeyPathKanataHostBridge/Cargo.toml/Cargo.lock (which now pin karabiner-driverkit via git rev instead of a local [patch.crates-io] path), a DriverKit revision bump would silently fail to invalidate the build cache — the same stale-engine failure mode the old comment was written to prevent (referenced as MAL-57 Layer 3). Worth double-checking the actual hash inputs match the new comment's claim.

  2. Rust/KeyPathKanataHostBridge/Cargo.toml — switching karabiner-driverkit from a [patch.crates-io] local path to a git dependency pinned to a specific rev is reasonable (immutable pin, not a floating branch/tag), just flagging it as a supply-chain-relevant change worth a second pair of eyes given it now fetches from an external GitHub repo at build time rather than the vendored kanata submodule.

  3. Scripts/lib/sparkle.sh / build-and-sign.sh / release-doctor.sh refactor of Sparkle signing looks solid: the private key is only ever piped via stdin to sign_update/generate_appcast/the key-derivation Swift snippet, never placed on argv or exported to unrelated subprocesses, and the new keypath_verify_sparkle_signing_identity check against SUPublicEDKey is a good addition to catch identity drift before a release.

No other actionable correctness/security issues spotted in the visible diff. Given the truncation, I'd recommend a manual pass over UpdateService.swift and the appcast publish path in the untruncated part of release.sh before merging, since those directly affect the auto-update flow.

@malpern

malpern commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Follow-up on the review notes:

  • Confirmed Scripts/build-kanata.sh hashes every .toml and .lock file under External/kanata. The rebased submodule's own Cargo.toml and Cargo.lock both pin karabiner-driverkit to exact commit 9fcaf6fe5af0059466afcd4d6c9713fd10c0216c, so a revision change invalidates the TCC-safe cache.
  • Confirmed the separate host-bridge cache independently hashes its own Rust/KeyPathKanataHostBridge/Cargo.toml and Cargo.lock, which carry the same immutable pin.
  • Manually reviewed the untruncated UpdateService.swift: Sparkle is postponed at its supported relaunch boundary, the runtime-preparation continuation is invoked exactly once through defer, and post-relaunch validation remains authoritative.
  • Manually reviewed the full appcast publish path: the build must produce and verify the complete signed feed, release.sh copies that whole generated feed into the repo, and the old XML-splice path is gone. The existing explicit final git push remains manual, so a partial release cannot silently publish an unsigned/mixed feed.

No follow-up code change is required for these findings.

@malpern
malpern marked this pull request as ready for review July 30, 2026 04:45
@malpern
malpern enabled auto-merge (squash) July 30, 2026 04:45
@malpern
malpern merged commit 0d7703f into master Jul 30, 2026
11 checks passed
@malpern
malpern deleted the codex/kanata-current-upstream-candidate branch July 30, 2026 04:49

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +305 to +307
case let .runtimeShutdownRequired(reason):
AppLogger.shared.log("⏸️ [UpdateService] Pre-update: stopping bundled Kanata runtime (\(reason))")
let report = await engine.runSingleAction(.terminateConflictingProcesses, using: broker)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +185 to +190
Toggle("Download and install updates automatically", isOn: Binding(
get: { updateService.automaticallyDownloadsUpdates },
set: { updateService.setAutomaticDownloads(enabled: $0) }
))
.disabled(!updateService.allowsAutomaticUpdates)
.accessibilityIdentifier("about-auto-install-toggle")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment thread Scripts/release.sh
Comment on lines +106 to +109
if [[ ! "$RELEASE_BUILD_NUMBER" =~ ^[0-9]+$ ]]; then
echo "❌ KEYPATH_RELEASE_BUILD_NUMBER must be numeric: $RELEASE_BUILD_NUMBER" >&2
exit 1
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

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.

1 participant