Repository navigation
Fix nightly SSH remote daemon checksum mismatch - #2225
Merged
Merged
Conversation
Each nightly build overwrites the shared cmuxd-remote-* assets on the nightly release, but older nightly DMGs have manifests with checksums from their build time. When a user's nightly is even one build behind, the downloaded binary doesn't match their embedded manifest. Two-layer fix: 1. CI: version nightly remote daemon asset names with the build number (e.g. cmuxd-remote-darwin-arm64-2362248028801) so each nightly's manifest points to immutable files. Unsuffixed "latest" copies are still uploaded for tooling compatibility. 2. Client: on checksum mismatch, fetch the live manifest from the release and verify against that. This handles users on older nightlies that predate the CI fix. Fixes #1745
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughNightly remote-daemon release artifacts are now produced with an optional timestamp-style asset suffix; the CI workflow builds suffixed assets, creates unsuffixed "latest" copies, and uploads suffixed artifacts for provenance. Client code adds a fallback that fetches the live manifest and re-validates a downloaded binary on checksum mismatch. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR fixes a nightly SSH remote daemon checksum mismatch caused by later nightly builds overwriting the shared unsuffixed release assets (e.g. The fix has two complementary parts:
Two non-blocking observations:
Confidence Score: 4/5Safe to merge — the core bug fix is correct and the two P2 observations are non-blocking documentation/attestation gaps. The checksum-fallback logic in Swift is sound (only the SHA-256 is used from the live manifest), the versioned-asset strategy in the build script and CI is clean, and the test covers both the unsuffixed and suffixed cases. The two remaining points — the unsuffixed manifest's .github/workflows/nightly.yml — attestation step and unsuffixed manifest copy warrant a clarifying comment on intent. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as cmux App
participant GH as GitHub Release
participant EM as Embedded Manifest
App->>EM: Read embedded manifest (CMUXRemoteDaemonManifestJSON)
EM-->>App: entry.downloadURL + entry.sha256
App->>GH: Download binary from entry.downloadURL
GH-->>App: binary (may be newer nightly overwrite)
App->>App: sha256(binary) vs entry.sha256
alt Checksum matches
App->>App: Accept binary ✓
else Checksum mismatch (stale embedded manifest)
App->>GH: GET releaseURL/cmuxd-remote-manifest.json (live)
GH-->>App: live manifest with updated sha256
App->>App: sha256(binary) vs liveEntry.sha256
alt Live checksum matches
App->>App: Accept binary (fallback OK) ✓
Note over App: debugLog: checksum-fallback succeeded
else Live checksum also mismatches
App->>App: Throw error (code 28) ✗
end
end
Reviews (1): Last reviewed commit: "Fix nightly SSH remote daemon checksum m..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+346
to
+347
| cp "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json" \ | ||
| "remote-daemon-assets/cmuxd-remote-manifest.json" |
Contributor
There was a problem hiding this comment.
downloadURL points to versioned assets
The unsuffixed cmuxd-remote-manifest.json is a verbatim copy of the suffixed manifest, so every downloadURL inside it references the versioned asset name (e.g. cmuxd-remote-darwin-arm64-${NIGHTLY_BUILD}), not the generic cmuxd-remote-darwin-arm64.
The Swift fallback in fetchRemoteManifestLocked is unaffected — it only reads the sha256 field from the live manifest, never downloadURL. However, any external tooling or users who fetch cmuxd-remote-manifest.json and follow its downloadURL fields to find the "latest" binary will be silently redirected to a versioned filename on every build. This may be intentional, but it's worth documenting in the comment on line 338 so future maintainers don't try to "fix" it.
Comment on lines
496
to
+502
| subject-path: | | ||
| remote-daemon-assets/cmuxd-remote-darwin-arm64 | ||
| remote-daemon-assets/cmuxd-remote-darwin-amd64 | ||
| remote-daemon-assets/cmuxd-remote-linux-arm64 | ||
| remote-daemon-assets/cmuxd-remote-linux-amd64 | ||
| remote-daemon-assets/cmuxd-remote-checksums.txt | ||
| remote-daemon-assets/cmuxd-remote-manifest.json | ||
| remote-daemon-assets/cmuxd-remote-darwin-arm64-${{ env.NIGHTLY_BUILD }} | ||
| remote-daemon-assets/cmuxd-remote-darwin-amd64-${{ env.NIGHTLY_BUILD }} | ||
| remote-daemon-assets/cmuxd-remote-linux-arm64-${{ env.NIGHTLY_BUILD }} | ||
| remote-daemon-assets/cmuxd-remote-linux-amd64-${{ env.NIGHTLY_BUILD }} | ||
| remote-daemon-assets/cmuxd-remote-checksums-${{ env.NIGHTLY_BUILD }}.txt | ||
| remote-daemon-assets/cmuxd-remote-manifest-${{ env.NIGHTLY_BUILD }}.json |
Contributor
There was a problem hiding this comment.
The attest-build-provenance step was updated to attest only the suffixed files. The unsuffixed copies created by the cp loop (e.g. cmuxd-remote-darwin-arm64, cmuxd-remote-checksums.txt, cmuxd-remote-manifest.json) are uploaded to the release but carry no attestation, so gh attestation verify will fail for those assets.
If build provenance isn't required for the "latest" aliases (and users who care about provenance are expected to use the versioned filenames), this is fine — just worth an explicit comment so it doesn't look like an oversight. If provenance on the generic names is desired, the unsuffixed copies should be added to subject-path here as well.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_remote_daemon_release_assets.sh (1)
94-107: Assert the suffixed checksum metadata too.This block only checks per-entry fields. If
checksumsAssetNameorchecksumsURLregressed back to the unsuffixed form, the manifest would still be wrong but this test would stay green.Coverage to add
manifest = json.loads(Path(sys.argv[1]).read_text(encoding="utf-8")) +if manifest["checksumsAssetName"] != "cmuxd-remote-checksums-123456.txt": + raise SystemExit(f"FAIL: unexpected checksumsAssetName {manifest['checksumsAssetName']}") +if not manifest["checksumsURL"].endswith("/cmuxd-remote-checksums-123456.txt"): + raise SystemExit(f"FAIL: unexpected checksumsURL {manifest['checksumsURL']}") + for entry in manifest["entries"]: if not entry["assetName"].endswith("-123456"): raise SystemExit(f"FAIL: suffixed asset name missing suffix: {entry['assetName']}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_remote_daemon_release_assets.sh` around lines 94 - 107, Update the manifest validation script to also assert that per-manifest checksum fields are suffixed: after loading manifest and iterating entries, check manifest["checksumsAssetName"] endswith "-123456" and manifest["checksumsURL"] endswith "/" + manifest["checksumsAssetName"]; if not, raise SystemExit with a clear FAIL message. Keep existing assetName/downloadURL checks and use the same error pattern so failures for checksums are reported consistently.scripts/build_remote_daemon_release_assets.sh (1)
48-50: Reject non filename-safe--asset-suffixvalues.This flag is now fed into filenames, manifest URLs, and the tab-delimited
ENTRIES_FILE. A suffix containing/, tabs, or newlines will either break the build or corrupt manifest generation, so it's worth validating up front.Suggested guard
--asset-suffix) ASSET_SUFFIX="${2:-}" + if [[ -z "$ASSET_SUFFIX" || ! "$ASSET_SUFFIX" =~ ^[A-Za-z0-9._-]+$ ]]; then + echo "error: --asset-suffix must match [A-Za-z0-9._-]+" >&2 + exit 1 + fi shift 2 ;;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build_remote_daemon_release_assets.sh` around lines 48 - 50, The --asset-suffix case currently assigns ASSET_SUFFIX but does not validate it; add a validation step immediately after ASSET_SUFFIX="${2:-}" in the --asset-suffix) block that rejects suffixes containing characters unsafe for filenames/manifests (e.g. '/' tab '\t' and newline '\n') by printing a clear error referencing ASSET_SUFFIX/--asset-suffix and exiting non‑zero; ensure the validation runs before shift 2 and before any use with ENTRIES_FILE or manifest generation so invalid values are rejected early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly.yml:
- Around line 340-345: The workflow currently copies the versioned checksum file
"remote-daemon-assets/cmuxd-remote-checksums-${NIGHTLY_BUILD}.txt" which lists
filenames with the build suffix, so the unsuffixed alias binaries
("remote-daemon-assets/cmuxd-remote-${platform}") will fail verification;
instead, after creating the unsuffixed copies (cmuxd-remote-${platform}),
regenerate an unsuffixed checksum file by computing checksums of those alias
files and writing them to "remote-daemon-assets/cmuxd-remote-checksums.txt"
(e.g., run shasum/sha256sum on each
"remote-daemon-assets/cmuxd-remote-${platform}" and output to the unsuffixed
checksum file) rather than copying the versioned checksum file verbatim.
In `@Sources/Workspace.swift`:
- Around line 4149-4151: The code currently builds the live manifest URL from
releaseURL (the release page) in fetchRemoteManifestLocked; instead derive the
manifest URL from a confirmed asset URL (like checksumsURL or entry.downloadURL)
by replacing only the final path component with "cmuxd-remote-manifest.json" so
you always start from an actual download base
(https://github.com/{repo}/releases/download/{tag}) rather than the release
page; update calls that pass releaseURL to instead pass checksumsURL (or
entry.downloadURL) and adjust the logic in fetchRemoteManifestLocked (and the
other similar call sites referenced) to construct the manifest URL by dropping
the last path component of the asset URL and appending
"cmuxd-remote-manifest.json".
---
Nitpick comments:
In `@scripts/build_remote_daemon_release_assets.sh`:
- Around line 48-50: The --asset-suffix case currently assigns ASSET_SUFFIX but
does not validate it; add a validation step immediately after
ASSET_SUFFIX="${2:-}" in the --asset-suffix) block that rejects suffixes
containing characters unsafe for filenames/manifests (e.g. '/' tab '\t' and
newline '\n') by printing a clear error referencing ASSET_SUFFIX/--asset-suffix
and exiting non‑zero; ensure the validation runs before shift 2 and before any
use with ENTRIES_FILE or manifest generation so invalid values are rejected
early.
In `@tests/test_remote_daemon_release_assets.sh`:
- Around line 94-107: Update the manifest validation script to also assert that
per-manifest checksum fields are suffixed: after loading manifest and iterating
entries, check manifest["checksumsAssetName"] endswith "-123456" and
manifest["checksumsURL"] endswith "/" + manifest["checksumsAssetName"]; if not,
raise SystemExit with a clear FAIL message. Keep existing assetName/downloadURL
checks and use the same error pattern so failures for checksums are reported
consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7f00194-7b21-496b-8816-d942c64fb203
📒 Files selected for processing (4)
.github/workflows/nightly.ymlSources/Workspace.swiftscripts/build_remote_daemon_release_assets.shtests/test_remote_daemon_release_assets.sh
Comment on lines
+4149
to
+4151
| /// Fetch the live manifest JSON from the release, returning nil on any failure. | ||
| private static func fetchRemoteManifestLocked(releaseURL: String, version: String) -> WorkspaceRemoteDaemonManifest? { | ||
| guard let manifestURL = URL(string: "\(releaseURL)/cmuxd-remote-manifest.json") else { return nil } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'releaseURL|checksumsURL|downloadURL|cmuxd-remote-manifest\.json|releases/(tag|download)' \
-g '!**/.build/**' \
-g '!**/DerivedData/**' \
-g '!**/node_modules/**'Repository: manaflow-ai/cmux
Length of output: 29894
Derive the live-manifest URL from a confirmed asset URL.
The manifest builder correctly sets releaseURL to the asset download base (https://github.com/{repo}/releases/download/{tag}), but the test data in cmuxTests/GhosttyConfigTests.swift incorrectly uses the release page URL (/releases/tag/...). Using checksumsURL or entry.downloadURL as the seed and replacing only the filename is more robust and avoids ambiguity:
- guard let manifestURL = URL(string: "\(releaseURL)/cmuxd-remote-manifest.json") else { return nil }
+ guard let assetURL = URL(string: releaseURL) else { return nil }
+ let manifestURL = assetURL.deletingLastPathComponent()
+ .appendingPathComponent("cmuxd-remote-manifest.json")At the call site, pass checksumsURL (a confirmed asset URL) instead of releaseURL:
- let downloadedURL = try downloadRemoteDaemonBinaryLocked(entry: entry, version: manifest.appVersion, releaseURL: manifest.releaseURL)
+ let downloadedURL = try downloadRemoteDaemonBinaryLocked(
+ entry: entry,
+ version: manifest.appVersion,
+ releaseURL: manifest.checksumsURL
+ )Also applies to: 4171, 4218–4225, 4265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 4149 - 4151, The code currently builds
the live manifest URL from releaseURL (the release page) in
fetchRemoteManifestLocked; instead derive the manifest URL from a confirmed
asset URL (like checksumsURL or entry.downloadURL) by replacing only the final
path component with "cmuxd-remote-manifest.json" so you always start from an
actual download base (https://github.com/{repo}/releases/download/{tag}) rather
than the release page; update calls that pass releaseURL to instead pass
checksumsURL (or entry.downloadURL) and adjust the logic in
fetchRemoteManifestLocked (and the other similar call sites referenced) to
construct the manifest URL by dropping the last path component of the asset URL
and appending "cmuxd-remote-manifest.json".
Contributor
Author
There was a problem hiding this comment.
Not a real issue. releaseURL in the manifest is always https://github.com/{repo}/releases/download/{tag} (the asset download base), not the release page URL. The code constructs releaseURL + "/cmuxd-remote-manifest.json" which correctly resolves to https://github.com/manaflow-ai/cmux/releases/download/nightly/cmuxd-remote-manifest.json. The test data CodeRabbit flagged is in a different context.
There was a problem hiding this comment.
@lawrencecchen Thanks for the clarification—agreed. In this repo the manifest’s releaseURL is always the asset download base (…/releases/download/{tag}), so constructing releaseURL + "/cmuxd-remote-manifest.json" in fetchRemoteManifestLocked is correct. I’ve retracted the earlier suggestion; no changes needed here.
✏️ Learnings added
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2225
File: Sources/Workspace.swift:4149-4151
Timestamp: 2026-03-27T00:15:38.194Z
Learning: Repo: manaflow-ai/cmux — WorkspaceRemoteDaemonManifest.releaseURL is guaranteed to be the GitHub asset download base "https://github.com/{owner}/{repo}/releases/download/{tag}" (not the tag page). Therefore, Sources/Workspace.swift should (and does) build the live-manifest URL as releaseURL + "/cmuxd-remote-manifest.json". Do not suggest deriving it from checksumsURL or entry.downloadURL; test fixtures that reference "/releases/tag/..." are from a different context and shouldn’t drive code changes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: outoftime
Repo: manaflow-ai/cmux PR: 1528
File: Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:541-546
Timestamp: 2026-03-17T13:59:10.665Z
Learning: Repo: manaflow-ai/cmux — The socket command `report_git_branch` (parsed in Sources/TerminalController.swift) expects the branch name as an **unquoted, bare token**. Wrapping the branch name in double quotes causes it to be silently discarded by the parser. This matches the bash/zsh shell integration convention. Do not suggest quoting the branch argument for this command; the fix was attempted in commit 829437c7 and immediately reverted because it broke branch reporting.
Learnt from: gaelic-ghost
Repo: manaflow-ai/cmux PR: 1926
File: scripts/build-sign-upload.sh:8-10
Timestamp: 2026-03-22T00:14:23.473Z
Learning: Repo: manaflow-ai/cmux — `scripts/lib/cmux-paths.sh` `cmux_paths_init()` intentionally preserves any pre-set `CMUX_*` environment variable overrides verbatim (does not canonicalize relative paths to absolute). This is a deliberate Stage 1 design choice; do not flag relative-override canonicalization as a bug unless a concrete reproducer is provided or a later stage explicitly tightens the override-semantics contract.
Learnt from: apollow
Repo: manaflow-ai/cmux PR: 1089
File: Sources/TerminalController.swift:3180-3193
Timestamp: 2026-03-09T02:08:14.574Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceClearTags(params:) must only clear all tags when the "source" key is absent. If "source" is present but blank or non-string (v2String(...) returns nil), the API should return invalid_params. Current implementation uses hasSourceKey = params.keys.contains("source") and guards with if hasSourceKey && source == nil { return .err(...)}.
Learnt from: arieltobiana
Repo: manaflow-ai/cmux PR: 1873
File: Sources/TerminalController.swift:4071-4087
Timestamp: 2026-03-20T17:18:30.333Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceAction(params:) -> case "set_color": palette names are resolved via WorkspaceTabColorSettings.defaultPaletteWithOverrides(), whose entries are always valid hex (validated by the UI). Therefore, additional normalization of entry.hex is unnecessary.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:4012-4023
Timestamp: 2026-03-16T08:05:21.899Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2SurfaceSplitSized(params:) must validate "ratio" as follows: if the "ratio" key is present, it must be numeric (NSNumber/Double-coercible) and strictly 0 < ratio < 1, otherwise return invalid_params; when "ratio" is absent, use default 0.6. This mirrors the general pattern that a present-but-invalid param should yield invalid_params rather than falling back.
Learnt from: outoftime
Repo: manaflow-ai/cmux PR: 1528
File: Sources/GhosttyTerminalView.swift:3220-3228
Timestamp: 2026-03-17T18:25:33.286Z
Learning: Repo: manaflow-ai/cmux — In Sources/GhosttyTerminalView.swift within TerminalSurface.createSurface(for:), when constructing XDG_DATA_DIRS for Fish vendor_conf.d auto-sourcing, treat empty or whitespace-only values from initialEnvironmentOverrides, env, getenv, and ProcessInfo as unset before prefixing the integrationDir. This avoids producing a trailing colon. Keep XDG_DATA_DIRS in protectedStartupEnvironmentKeys so initialEnvironmentOverrides cannot overwrite the prefixed value.
Learnt from: homanp
Repo: manaflow-ai/cmux PR: 883
File: CLI/cmux.swift:1569-1586
Timestamp: 2026-03-04T22:05:07.913Z
Learning: In manaflow-ai/cmux CLI (CLI/cmux.swift), for parity with existing browser commands, markdown.open intentionally normalizes --surface before resolving --workspace/--window, relying on server-side resolution to disambiguate final routing. Avoid one-off reordering in markdown; consider any change only as a coordinated, cross-command refactor.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:12:42.553Z
Learning: Repo: manaflow-ai/cmux — In Sources/Panels/BrowserPanel.swift, BrowserPanel.updateWorkspaceId(_:) is a dedicated method (var workspaceId) that updates both BrowserPanel.workspaceId and pickerMessageHandler?.updateWorkspaceId(_:) atomically. It is called from BrowserPanel.reattachToWorkspace(_:) when a panel moves between workspaces. This ensures BrowserPickerMessageHandler always posts notifications with the current workspaceId, not a stale one from panel initialization.
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2034
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-25T07:14:56.211Z
Learning: Repo: manaflow-ai/cmux — Sources/AppDelegate.swift — Pattern for “Open Folder”: In AppDelegate.showOpenFolderPanel(), seed NSOpenPanel.directoryURL using preferredMainWindowContextForWorkspaceCreation(debugSource: …) rather than NSApp.keyWindow, and on selection delegate to openWorkspaceForExternalDirectory(workingDirectory:…, debugSource: …). Rationale: handles auxiliary-key-window cases, ensures shouldBringToFront = true, and unifies menu/shortcut behavior with a consistent fallback to createMainWindow when workspace creation returns nil.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3607-3645
Timestamp: 2026-03-25T00:32:48.115Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, v2WorkspaceSetColor(params:) must validate the provided color with WorkspaceTabColorSettings.normalizedHex. If the color parameter is present but invalid, return invalid_params (no fallback). On success, apply the color and return the normalized (uppercase) `#RRGGBB` in the response payload.
Learnt from: qkrwpdlr
Repo: manaflow-ai/cmux PR: 0
File: :0-0
Timestamp: 2026-03-23T07:01:33.134Z
Learning: Repo: manaflow-ai/cmux — In Sources/TerminalController.swift, sendPickedElementToTerminal(workspaceId:summary:) resolves the target workspace via AppDelegate.shared?.workspaceFor(tabId: workspaceId), which searches across all mainWindowContexts (not self.tabManager which is active-window only). It first injects text into the focused terminal panel if it's a terminal, then falls back to iterating all panels in the workspace.
Learnt from: tayl0r
Repo: manaflow-ai/cmux PR: 1909
File: Sources/ContentView.swift:2156-2171
Timestamp: 2026-03-23T06:08:14.740Z
Learning: Repo: manaflow-ai/cmux — In Sources/ContentView.swift, openFileInTextEditor(_:) must attempt workspace.newTextEditorSplit(from:orientation:filePath:focus:) and, if that returns nil, fall back to workspace.newTextEditorSurface(inPane:filePath:focus:) using bonsplitController.focusedPaneId. Rationale: avoid dropping file-open requests when the focused panel is not pane-backed.
Learnt from: MaTriXy
Repo: manaflow-ai/cmux PR: 1460
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-16T08:02:06.824Z
Learning: In Swift sources, for any panel_id-only route handling in v2PanelMarkBackground(params:) and v2PanelMarkForeground(params:), first attempt v2ResolveTabManager(params:). Use the manager only if it actually owns the panelId; otherwise fall back to AppDelegate.shared?.locateSurface(surfaceId:) to locate the correct TabManager across windows. Apply this pattern to all panel_id-only routes to avoid active-window bias.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: CLI/cmux.swift:1948-1978
Timestamp: 2026-03-25T00:33:26.452Z
Learning: Repo: manaflow-ai/cmux — In CLI/cmux.swift, the set-workspace-color command requires exactly one trailing <hex> argument and enforces a strict 6-digit hex format (`#RRGGBB`, optional leading '#'); clear-workspace-color rejects any unexpected positional args beyond --workspace. This matches server-side normalization and prevents malformed inputs.
Learnt from: atani
Repo: manaflow-ai/cmux PR: 819
File: Sources/AppDelegate.swift:0-0
Timestamp: 2026-03-04T14:05:42.574Z
Learning: Guideline: In Swift files (cmux project), when handling pluralized strings, prefer using localization keys with the ICU-style plural forms .one and .other. For example, use keys like statusMenu.unreadCount.one for the singular case (1) and statusMenu.unreadCount.other for all other counts, and similarly for statusMenu.tooltip.unread.one/other. Rationale: ensures correct pluralization across locales and makes localization keys explicit. Review code to ensure any unread count strings and related tooltips follow this .one/.other key pattern and verify the correct value is chosen based on the count.
Learnt from: austinywang
Repo: manaflow-ai/cmux PR: 954
File: Sources/TerminalController.swift:0-0
Timestamp: 2026-03-05T22:04:34.712Z
Learning: Adopt the convention: for health/telemetry tri-state values in Swift, prefer Optionals (Bool?) over sentinel booleans. In TerminalController.swift, socketConnectable is Bool? and only set when socketProbePerformed is true; downstream logic must treat nil as 'not probed'. Ensure downstream code checks for nil before using a value and uses explicit non-nil checks to determine state, improving clarity and avoiding misinterpretation of default false.
Learnt from: moyashin63
Repo: manaflow-ai/cmux PR: 1074
File: Sources/AppDelegate.swift:7523-7545
Timestamp: 2026-03-09T01:38:24.337Z
Learning: When the command palette is visible (as in manaflow-ai/cmux Sources/AppDelegate.swift), ensure the shortcut handling consumes most Command shortcuts to protect the palette's text input. Specifically, do not allow UI zoom shortcuts (Cmd+Shift+= / Cmd+Shift+− / Cmd+Shift+0) to trigger while the palette is open. Do not reorder shortcut handlers (e.g., uiZoomShortcutAction(...)) to bypass this guard; users must close the palette before performing zoom actions. This guideline should apply to Swift source files handling global shortcuts within the app.
Learnt from: zlatkoc
Repo: manaflow-ai/cmux PR: 1368
File: Sources/Panels/BrowserPanel.swift:69-69
Timestamp: 2026-03-13T13:46:01.733Z
Learning: Do not wrap engine/brand name literals (e.g., displayName values such as Google, DuckDuckGo, Bing, Kagi, Startpage) in String(localized: ...). These are brand/product names that are not translatable UI text. Localization should apply to generic UI strings (labels, buttons, error messages, etc.). Apply this guideline across Swift source files under Sources/ (notably in BrowserPanel.swift and similar UI/engine-related strings) and flag only brand-name strings that are part of user-facing UI text appropriately for translation scope.
Learnt from: kjb0787
Repo: manaflow-ai/cmux PR: 1461
File: Sources/GhosttyTerminalView.swift:5904-5905
Timestamp: 2026-03-15T19:22:32.330Z
Learning: In Swift files under the Sources directory that manage terminal/scroll behavior, ensure the following: when preserving scroll across workspace switches, save savedScrollRow only if the scrollbar offset is greater than 0 (indicating the user has scrolled up). On restore, call scroll_to_row only if savedScrollRow is non-nil; if it is nil, rely on synchronizeScrollView() to keep bottom-pinned sessions following new output. This pattern should be applied wherever GhosttyTerminalView-like views implement setVisibleInUI(_:) to maintain consistent user scroll state across workspace switches.
Learnt from: pratikpakhale
Repo: manaflow-ai/cmux PR: 2011
File: Resources/Localizable.xcstrings:15256-15368
Timestamp: 2026-03-23T21:39:50.795Z
Learning: When reviewing this repo’s Swift localization usage, do not flag missing `String.localizedStringWithFormat` for calls that use the modern overload `String(localized: "key", defaultValue: "...\(variable)")` (where `defaultValue` is a `String.LocalizationValue` built with `\(…)`). That overload natively supports interpolation and the xcstrings/runtime substitution handles the resulting placeholders automatically. Only require `String.localizedStringWithFormat` when using the older `String(localized:)` overload that takes a plain `String` (i.e., where format arguments must be passed separately), such as for keys like `clipboard.sshError.single`.
Learnt from: thunter009
Repo: manaflow-ai/cmux PR: 1825
File: Sources/TerminalController.swift:3620-3622
Timestamp: 2026-03-25T00:32:54.735Z
Learning: When validating or reporting workspace/tab colors in this repo, only accept and use 6-digit hex colors in the form `#RRGGBB` (no alpha, i.e., do not allow `#RRGGBBAA`). Ensure validation logic matches the existing behavior (e.g., WorkspaceTabColorSettings.normalizedHex(...) and TabManager.setTabColor(tabId:color:) as well as CLI/cmux.swift). Update any error/help text for workspace color to reference only `#RRGGBB` (not `#RRGGBBAA`).
Regenerate cmuxd-remote-checksums.txt from the unsuffixed alias binaries so `shasum -c` works against the generic asset names. Also document that unsuffixed manifest intentionally keeps versioned downloadURLs and that aliases don't carry attestation.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly.yml:
- Around line 324-361: The workflow is always inserting the $NIGHTLY_BUILD
suffix into the manifest via scripts/build_remote_daemon_release_assets.sh and
then creating suffixed filenames (cmuxd-remote-…-${NIGHTLY_BUILD}) but the
upload only happens for publish runs, causing branch builds to reference
non-existent suffixed assets; wrap the suffixing/aliasing steps in a
publish-only conditional: only pass --asset-suffix to
scripts/build_remote_daemon_release_assets.sh and only run the for-loop that
copies cmuxd-remote-${platform}-${NIGHTLY_BUILD} to unsuffixed names, the shasum
regeneration block, and the cp of cmuxd-remote-manifest-${NIGHTLY_BUILD}.json to
cmuxd-remote-manifest.json when a PUBLISH/RELEASE flag is true (otherwise build
without --asset-suffix so manifest downloadURL/assetName are unsuffixed); apply
the same guard to the other identical block that manipulates the cmuxd-remote-*
files referenced in the review (the block around the alternate lines 511-516).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 573b3128-e9ee-4024-bfdb-4a424bd83e1a
📒 Files selected for processing (1)
.github/workflows/nightly.yml
Comment on lines
324
to
+361
| ./scripts/build_remote_daemon_release_assets.sh \ | ||
| --version "$NIGHTLY_REMOTE_DAEMON_VERSION" \ | ||
| --release-tag "nightly" \ | ||
| --repo "manaflow-ai/cmux" \ | ||
| --output-dir "remote-daemon-assets" | ||
| MANIFEST_JSON="$(python3 -c 'import json,sys; print(json.dumps(json.load(open(sys.argv[1], encoding="utf-8")), separators=(",",":")))' remote-daemon-assets/cmuxd-remote-manifest.json)" | ||
| --output-dir "remote-daemon-assets" \ | ||
| --asset-suffix "$NIGHTLY_BUILD" | ||
| MANIFEST_JSON="$(python3 -c 'import json,sys; print(json.dumps(json.load(open(sys.argv[1], encoding="utf-8")), separators=(",",":")))' "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json")" | ||
| APP_PLIST="build-universal/Build/Products/Release/cmux NIGHTLY.app/Contents/Info.plist" | ||
| if [ ! -f "$APP_PLIST" ]; then | ||
| echo "Missing nightly app Info.plist at $APP_PLIST" >&2 | ||
| exit 1 | ||
| fi | ||
| plutil -remove CMUXRemoteDaemonManifestJSON "$APP_PLIST" >/dev/null 2>&1 || true | ||
| plutil -insert CMUXRemoteDaemonManifestJSON -string "$MANIFEST_JSON" "$APP_PLIST" | ||
| # Also create unsuffixed "latest" copies for the release page and | ||
| # any tooling that fetches the generic asset names. The manifest's | ||
| # downloadURLs still point to the versioned filenames (intentional: | ||
| # the live manifest is used by the client-side checksum fallback | ||
| # which only reads sha256, not downloadURL). The unsuffixed copies | ||
| # are convenience aliases and don't carry build-provenance | ||
| # attestation (attested versioned files are canonical). | ||
| for platform in darwin-arm64 darwin-amd64 linux-arm64 linux-amd64; do | ||
| cp "remote-daemon-assets/cmuxd-remote-${platform}-${NIGHTLY_BUILD}" \ | ||
| "remote-daemon-assets/cmuxd-remote-${platform}" | ||
| done | ||
| # Regenerate unsuffixed checksums with generic filenames so | ||
| # `shasum -c cmuxd-remote-checksums.txt` works against the aliases. | ||
| ( | ||
| cd remote-daemon-assets | ||
| shasum -a 256 \ | ||
| cmuxd-remote-darwin-arm64 \ | ||
| cmuxd-remote-darwin-amd64 \ | ||
| cmuxd-remote-linux-arm64 \ | ||
| cmuxd-remote-linux-amd64 \ | ||
| > cmuxd-remote-checksums.txt | ||
| ) | ||
| cp "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json" \ | ||
| "remote-daemon-assets/cmuxd-remote-manifest.json" |
There was a problem hiding this comment.
Don't suffix remote-daemon URLs for non-publishing builds.
This block still runs on branch builds, but those runs only upload workflow artifacts. Since scripts/build_remote_daemon_release_assets.sh bakes --asset-suffix into the manifest's assetName and downloadURL, the app in a branch artifact will try to fetch cmuxd-remote-…-$NIGHTLY_BUILD from the nightly release even though that upload never happens. That turns cmux ssh into a 404 path, and the new fallback here won't help because it only re-validates checksum mismatches. Lines 511-516 need the same publish-only split so attestation matches whichever naming mode is used.
Possible fix
- ./scripts/build_remote_daemon_release_assets.sh \
- --version "$NIGHTLY_REMOTE_DAEMON_VERSION" \
- --release-tag "nightly" \
- --repo "manaflow-ai/cmux" \
- --output-dir "remote-daemon-assets" \
- --asset-suffix "$NIGHTLY_BUILD"
- MANIFEST_JSON="$(python3 -c 'import json,sys; print(json.dumps(json.load(open(sys.argv[1], encoding="utf-8")), separators=(",",":")))' "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json")"
+ BUILD_REMOTE_DAEMON_ARGS=(
+ --version "$NIGHTLY_REMOTE_DAEMON_VERSION"
+ --release-tag "nightly"
+ --repo "manaflow-ai/cmux"
+ --output-dir "remote-daemon-assets"
+ )
+ MANIFEST_PATH="remote-daemon-assets/cmuxd-remote-manifest.json"
+ if [ "${{ needs.decide.outputs.should_publish }}" = "true" ]; then
+ BUILD_REMOTE_DAEMON_ARGS+=(--asset-suffix "$NIGHTLY_BUILD")
+ MANIFEST_PATH="remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json"
+ fi
+ ./scripts/build_remote_daemon_release_assets.sh "${BUILD_REMOTE_DAEMON_ARGS[@]}"
+ MANIFEST_JSON="$(python3 -c 'import json,sys; print(json.dumps(json.load(open(sys.argv[1], encoding="utf-8")), separators=(",",":")))' "$MANIFEST_PATH")"
@@
- for platform in darwin-arm64 darwin-amd64 linux-arm64 linux-amd64; do
- cp "remote-daemon-assets/cmuxd-remote-${platform}-${NIGHTLY_BUILD}" \
- "remote-daemon-assets/cmuxd-remote-${platform}"
- done
- (
- cd remote-daemon-assets
- shasum -a 256 \
- cmuxd-remote-darwin-arm64 \
- cmuxd-remote-darwin-amd64 \
- cmuxd-remote-linux-arm64 \
- cmuxd-remote-linux-amd64 \
- > cmuxd-remote-checksums.txt
- )
- cp "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json" \
- "remote-daemon-assets/cmuxd-remote-manifest.json"
+ if [ "${{ needs.decide.outputs.should_publish }}" = "true" ]; then
+ for platform in darwin-arm64 darwin-amd64 linux-arm64 linux-amd64; do
+ cp "remote-daemon-assets/cmuxd-remote-${platform}-${NIGHTLY_BUILD}" \
+ "remote-daemon-assets/cmuxd-remote-${platform}"
+ done
+ (
+ cd remote-daemon-assets
+ shasum -a 256 \
+ cmuxd-remote-darwin-arm64 \
+ cmuxd-remote-darwin-amd64 \
+ cmuxd-remote-linux-arm64 \
+ cmuxd-remote-linux-amd64 \
+ > cmuxd-remote-checksums.txt
+ )
+ cp "remote-daemon-assets/cmuxd-remote-manifest-${NIGHTLY_BUILD}.json" \
+ "remote-daemon-assets/cmuxd-remote-manifest.json"
+ fiAlso applies to: 511-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/nightly.yml around lines 324 - 361, The workflow is always
inserting the $NIGHTLY_BUILD suffix into the manifest via
scripts/build_remote_daemon_release_assets.sh and then creating suffixed
filenames (cmuxd-remote-…-${NIGHTLY_BUILD}) but the upload only happens for
publish runs, causing branch builds to reference non-existent suffixed assets;
wrap the suffixing/aliasing steps in a publish-only conditional: only pass
--asset-suffix to scripts/build_remote_daemon_release_assets.sh and only run the
for-loop that copies cmuxd-remote-${platform}-${NIGHTLY_BUILD} to unsuffixed
names, the shasum regeneration block, and the cp of
cmuxd-remote-manifest-${NIGHTLY_BUILD}.json to cmuxd-remote-manifest.json when a
PUBLISH/RELEASE flag is true (otherwise build without --asset-suffix so manifest
downloadURL/assetName are unsuffixed); apply the same guard to the other
identical block that manipulates the cmuxd-remote-* files referenced in the
review (the block around the alternate lines 511-516).
Contributor
Author
There was a problem hiding this comment.
Not actionable. Branch builds only upload to workflow artifacts for CI validation, never installed by users or distributed via Sparkle. The manifest URLs don't matter for those artifacts.
There was a problem hiding this comment.
@lawrencecchen, understood — since branch artifacts are purely for CI validation and are never distributed to or installed by real users, the suffixed manifest URLs in those builds have no practical impact. Fair point, I'll drop the concern.
🧠 Learnings used
Learnt from: lawrencecchen
Repo: manaflow-ai/cmux PR: 2225
File: Sources/Workspace.swift:4149-4151
Timestamp: 2026-03-27T00:15:38.194Z
Learning: Repo: manaflow-ai/cmux — WorkspaceRemoteDaemonManifest.releaseURL is guaranteed to be the GitHub asset download base "https://github.com/{owner}/{repo}/releases/download/{tag}" (not the tag page). Therefore, Sources/Workspace.swift should (and does) build the live-manifest URL as releaseURL + "/cmuxd-remote-manifest.json". Do not suggest deriving it from checksumsURL or entry.downloadURL; test fixtures that reference "/releases/tag/..." are from a different context and shouldn’t drive code changes.
Learnt from: outoftime
Repo: manaflow-ai/cmux PR: 1528
File: Resources/shell-integration/fish/vendor_conf.d/cmux-fish-integration.fish:541-546
Timestamp: 2026-03-17T13:59:10.665Z
Learning: Repo: manaflow-ai/cmux — The socket command `report_git_branch` (parsed in Sources/TerminalController.swift) expects the branch name as an **unquoted, bare token**. Wrapping the branch name in double quotes causes it to be silently discarded by the parser. This matches the bash/zsh shell integration convention. Do not suggest quoting the branch argument for this command; the fix was attempted in commit 829437c7 and immediately reverted because it broke branch reporting.
Learnt from: gaelic-ghost
Repo: manaflow-ai/cmux PR: 1926
File: scripts/build-sign-upload.sh:8-10
Timestamp: 2026-03-22T00:14:23.473Z
Learning: Repo: manaflow-ai/cmux — `scripts/lib/cmux-paths.sh` `cmux_paths_init()` intentionally preserves any pre-set `CMUX_*` environment variable overrides verbatim (does not canonicalize relative paths to absolute). This is a deliberate Stage 1 design choice; do not flag relative-override canonicalization as a bug unless a concrete reproducer is provided or a later stage explicitly tightens the override-semantics contract.
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Fix nightly SSH remote daemon checksum mismatch Each nightly build overwrites the shared cmuxd-remote-* assets on the nightly release, but older nightly DMGs have manifests with checksums from their build time. When a user's nightly is even one build behind, the downloaded binary doesn't match their embedded manifest. Two-layer fix: 1. CI: version nightly remote daemon asset names with the build number (e.g. cmuxd-remote-darwin-arm64-2362248028801) so each nightly's manifest points to immutable files. Unsuffixed "latest" copies are still uploaded for tooling compatibility. 2. Client: on checksum mismatch, fetch the live manifest from the release and verify against that. This handles users on older nightlies that predate the CI fix. Fixes manaflow-ai#1745 * Fix unsuffixed checksums file to use generic filenames Regenerate cmuxd-remote-checksums.txt from the unsuffixed alias binaries so `shasum -c` works against the generic asset names. Also document that unsuffixed manifest intentionally keeps versioned downloadURLs and that aliases don't carry attestation. --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
3 tasks
This branch was successfully deployed
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.
Summary
cmuxd-remote-*) now include the build number in their filename (e.g.cmuxd-remote-darwin-arm64-2362248028801), so each nightly's embedded manifest points to immutable files that won't be overwritten by later builds--asset-suffixparameter for versioned asset namesTesting
Related
Summary by cubic
Fix nightly SSH remote daemon checksum mismatches by versioning nightly asset filenames and adding a client fallback to the live manifest. This makes each nightly’s assets immutable and keeps older nightlies working. Fixes #1745.
cmuxd-remote-darwin-arm64-2362248028801) and embeds the suffixed manifest; also uploads unsuffixed “latest” alias binaries and a manifest copy for compatibility (the copy intentionally keeps versioned downloadURLs).scripts/build_remote_daemon_release_assets.shadds--asset-suffix; CI attests only versioned files, regenerates unsuffixedcmuxd-remote-checksums.txtfrom alias filenames soshasum -cworks, updates upload patterns, and adds tests for both modes.Written for commit e184e7c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements
Tests