Repository navigation
fix: repair NIGHTLY Sparkle quarantine metadata - #1703
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new UpdateQuarantineRepair utility to discover and repair com.apple.quarantine metadata on downloaded archives and extracted apps; integrates repair calls into the Sparkle updater delegate and UpdateDriver flow; adds tests and updates the Xcode project to include the new source and test files. Changes
Sequence DiagramsequenceDiagram
participant SPU as SPUUpdater
participant UDel as UpdateDriver<br/>(Delegate)
participant QR as UpdateQuarantineRepair
participant FS as File System<br/>(xattr)
rect rgba(100,150,200,0.5)
Note over SPU,FS: Pre-extract / downloaded archive repair
SPU->>UDel: willExtractUpdate(item)
UDel->>QR: repairDownloadedArchiveIfNeeded(host, version, ...)
QR->>FS: locate archive in Sparkle cache
QR->>FS: read `com.apple.quarantine` xattr
QR->>QR: evaluate & possibly modify quarantine fields
QR->>FS: write updated xattr
QR-->>UDel: UpdateQuarantineRepairResult
UDel->>UDel: logUpdateQuarantineRepair(...)
end
rect rgba(150,100,200,0.5)
Note over SPU,FS: Extraction / extracted app repair
SPU->>UDel: extraction progress / start
UDel->>QR: repairExtractedApplicationIfNeeded(...)
QR->>FS: locate extracted app in Installation path
QR->>FS: read `com.apple.quarantine` xattr
QR->>QR: evaluate & possibly modify quarantine fields (LS metadata)
QR->>FS: write updated xattr
QR-->>UDel: UpdateQuarantineRepairResult
UDel->>UDel: logUpdateQuarantineRepair(...)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmuxTests/UpdateQuarantineRepairTests.swift (1)
137-172: Consider adding test cleanup to avoid temp file accumulation.The test helpers create temporary files and directories but don't clean them up. While acceptable, consider adding
addTeardownBlockor overridingtearDown()to remove the created temp directories to avoid accumulating test artifacts over time.♻️ Optional cleanup implementation
final class UpdateQuarantineRepairTests: XCTestCase { private var tempDirectories: [URL] = [] override func tearDown() { super.tearDown() for url in tempDirectories { try? FileManager.default.removeItem(at: url) } tempDirectories.removeAll() } private func makeTemporaryDirectory(named name: String) throws -> URL { let directoryURL = FileManager.default.temporaryDirectory .appendingPathComponent("UpdateQuarantineRepairTests", isDirectory: true) .appendingPathComponent(UUID().uuidString, isDirectory: true) .appendingPathComponent(name, isDirectory: true) try FileManager.default.createDirectory(at: directoryURL, withIntermediateDirectories: true) tempDirectories.append(directoryURL.deletingLastPathComponent().deletingLastPathComponent()) return directoryURL } // ... rest unchanged }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/UpdateQuarantineRepairTests.swift` around lines 137 - 172, Test helpers (makeTemporaryDirectory, makeTemporaryFile, createFile) create temp dirs/files but never remove them; add cleanup by tracking created root temp directories (e.g., a private var tempDirectories: [URL]) and implement tearDown() to iterate and remove each URL with FileManager.default.removeItem(at:) (or register addTeardownBlock when creating each directory) so makeTemporaryDirectory appends the created root URL to tempDirectories and tearDown removes and clears the list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/UpdateQuarantineRepairTests.swift`:
- Around line 137-172: Test helpers (makeTemporaryDirectory, makeTemporaryFile,
createFile) create temp dirs/files but never remove them; add cleanup by
tracking created root temp directories (e.g., a private var tempDirectories:
[URL]) and implement tearDown() to iterate and remove each URL with
FileManager.default.removeItem(at:) (or register addTeardownBlock when creating
each directory) so makeTemporaryDirectory appends the created root URL to
tempDirectories and tearDown removes and clears the list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84cdc53a-6810-4e29-a1aa-600f487b6fbb
📒 Files selected for processing (4)
GhosttyTabs.xcodeproj/project.pbxprojSources/Update/UpdateDelegate.swiftSources/Update/UpdateQuarantineRepair.swiftcmuxTests/UpdateQuarantineRepairTests.swift
…699-nightly-quarantine # Conflicts: # GhosttyTabs.xcodeproj/project.pbxproj
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Sources/Update/UpdateDriver.swift (1)
322-325: Avoid persisting raw quarantine blobs in logs.
beforeRawValue/afterRawValuecan carry source metadata (including URLs). Prefer redacted or structured fields to reduce sensitive-data exposure in persistent logs.🔒 Suggested redaction
- let before = result.beforeRawValue ?? "<none>" - let after = result.afterRawValue ?? "<none>" - UpdateLogStore.shared.append("quarantine repair extracted-app: \(result.outcome) path=\(path) before=\(before) after=\(after)") + UpdateLogStore.shared.append("quarantine repair extracted-app: \(result.outcome) path=\(path)")Sources/Update/UpdateDelegate.swift (1)
128-133: Consider centralizing quarantine-repair log formatting.This formatter duplicates logic already present in
Sources/Update/UpdateDriver.swift(extracted-app logging). A shared formatter/helper will prevent divergence and make future redaction updates one-place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Update/UpdateDelegate.swift` around lines 128 - 133, The quarantine-repair log formatting in UpdateDelegate.logUpdateQuarantineRepair duplicates logic used in UpdateDriver (extracted-app logging); extract the formatting into a single shared helper (e.g., a new UpdateLogFormatter.formatQuarantineRepair(stage:result:) or similar) that takes the stage string and an UpdateQuarantineRepairResult and returns the fully formatted log message (handling url/path, beforeRawValue, afterRawValue and redaction rules), then update UpdateDelegate.logUpdateQuarantineRepair to call that helper and likewise switch the duplicated code in UpdateDriver to use the same helper so formatting lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Update/UpdateDelegate.swift`:
- Around line 128-133: The quarantine-repair log formatting in
UpdateDelegate.logUpdateQuarantineRepair duplicates logic used in UpdateDriver
(extracted-app logging); extract the formatting into a single shared helper
(e.g., a new UpdateLogFormatter.formatQuarantineRepair(stage:result:) or
similar) that takes the stage string and an UpdateQuarantineRepairResult and
returns the fully formatted log message (handling url/path, beforeRawValue,
afterRawValue and redaction rules), then update
UpdateDelegate.logUpdateQuarantineRepair to call that helper and likewise switch
the duplicated code in UpdateDriver to use the same helper so formatting lives
in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7b9305e-66ee-46e3-95dc-ed4d2138908d
📒 Files selected for processing (2)
Sources/Update/UpdateDelegate.swiftSources/Update/UpdateDriver.swift
austinywang
added a commit
that referenced
this pull request
Mar 18, 2026
This reverts commit 629b63d.
austinywang
added a commit
that referenced
this pull request
Mar 18, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* test: add quarantine regression coverage * fix: repair Sparkle quarantine metadata for nightly updates * fix: repair extracted Sparkle app on extraction callbacks
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
…1703)" (manaflow-ai#1725) This reverts commit f7c1cf6.
rodchristiansen
pushed a commit
to rodchristiansen/cmux
that referenced
this pull request
Sep 2, 2026
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.
Fixes #1699.
Regression
e15825826f36ea007c0262f375f5585888dc4e21on March 17, 2026, which restored automatic Sparkle update checks for NIGHTLY.com.apple.quarantinevalue. That attribute is created on the client during Sparkle download/extract, so the fix is in the updater integration.What changed
0383;...;;).(null)for the creating app.Verification
./scripts/reload.sh --tag fix-1699-quarantinexcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-fix-1699-quarantine-tests build-for-testing\n- Tests were not executed locally per repo policy; only the app build and test build artifacts were verified.Summary by cubic
Fixes a NIGHTLY regression where Sparkle wrote malformed quarantine metadata that made Gatekeeper show (null) as the source app. We now repair
com.apple.quarantineon the downloaded archive and the extracted app during the update.Written for commit aa6dda8. Summary will update on new commits.
Summary by CodeRabbit