fix: audit remaining issues — safety, privacy, accessibility, cleanup#181
fix: audit remaining issues — safety, privacy, accessibility, cleanup#181
Conversation
- ABXD-22: Add isEnabling guard to prevent concurrent enableDaemon() calls - ABXD-54: Add verifyDaemonSignature() — codesign --verify before launch (warn-only) - ABXD-17: Protect masterFD with OSAllocatedUnfairLock, atomic swap in teardown
- ABXD-52: Change user data (container/image/network IDs, paths) to privacy: .private - ABXD-49: Add protobuf codegen verification step to PR CI workflow - ABXD-72: Pin SPM deps to .upToNextMinor ranges in ArcBoxClient and DockerClient
- ABXD-57: Extract parseISO8601Date() to Utilities.swift, replace inline parsing in NetworksViewModel and VolumesViewModel - ABXD-58: Extract terminal defaults (cols/rows/bufferSize) to named constants in DockerTerminalSession - ABXD-59: Add Log.machine + warning logs to MachinesViewModel empty stubs - ABXD-85: Replace lastError! force unwrap with default URLError(.unknown)
…osition - ABXD-37: Add accessibilityLabel to 6 RowViews, 5 toolbar buttons, StatusBadge - ABXD-60: Create EmptyStateView generic component, refactor 7 EmptyState files - ABXD-61: Extract sidebar and detailPanel from ContentView body
- ABXD-48: Audit confirms all ViewModel inits are lightweight — added comments - ABXD-62: Add onOpenURL handler stub + handleDeepLink() for arcbox:// scheme - ABXD-43: Supplement K8s Watch TODO with 410 Gone handling note
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea5dc204e7
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
Addresses remaining Desktop Release Readiness Audit items by tightening daemon-start safety, improving log privacy, adding first-pass accessibility labels, and reducing UI duplication with shared components, plus CI/proto-generation hygiene.
Changes:
- Hardened daemon startup/teardown paths (re-entrancy guard, signature verification hook) and fixed a DockerClient retry force-unwrap.
- Reduced potential PII exposure by shifting many OSLog interpolations from
.public→.private. - Improved UI maintainability/accessibility via reusable
EmptyStateView, ContentView decomposition, and added VoiceOver labels to rows/buttons.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Packages/K8sClient/Sources/K8sClient/K8sClient.swift | Expanded watch-design TODO documentation (incl. 410 Gone note). |
| Packages/DockerClient/Sources/DockerClient/DockerClient.swift | Removed lastError! by defaulting to URLError(.unknown) and throwing non-optional error. |
| Packages/DockerClient/Package.swift | Switched dependency requirements to .upToNextMinor(from:). |
| Packages/ArcBoxClient/Sources/ArcBoxClient/StartupOrchestrator.swift | Adjusted startup failure log privacy for error messages. |
| Packages/ArcBoxClient/Sources/ArcBoxClient/DaemonManager.swift | Added enable re-entrancy guard, signature verification, and tightened log privacy. |
| Packages/ArcBoxClient/Package.swift | Switched dependency requirements to .upToNextMinor(from:). |
| ArcBox/Views/Volumes/VolumesListView.swift | Added accessibility label for “New volume” toolbar button. |
| ArcBox/Views/Volumes/VolumeRowView.swift | Added combined accessibility element/label for row content. |
| ArcBox/Views/Volumes/VolumeEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/Templates/TemplateEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/Sandboxes/SandboxEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/Networks/Tabs/NetworkContainersTab.swift | Tightened log privacy for network container loading errors. |
| ArcBox/Views/Networks/NetworksListView.swift | Added accessibility label for “New network” toolbar button. |
| ArcBox/Views/Networks/NetworkRowView.swift | Added combined accessibility element/label for row content. |
| ArcBox/Views/Networks/NetworkEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/Machines/MachinesView.swift | Added accessibility label for “New machine” toolbar button. |
| ArcBox/Views/Machines/MachineEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/Kubernetes/ServiceRowView.swift | Added combined accessibility element/label for row content. |
| ArcBox/Views/Kubernetes/PodRowView.swift | Added combined accessibility element/label for row content. |
| ArcBox/Views/Images/ImagesListView.swift | Added accessibility label for “Pull image” toolbar button. |
| ArcBox/Views/Images/ImageRowView.swift | Added combined accessibility element/label for row content. |
| ArcBox/Views/Images/ImageEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/Views/ContentView.swift | Extracted sidebar and detailPanel computed properties. |
| ArcBox/Views/Containers/ContainersListView.swift | Added accessibility label for “New container” toolbar button. |
| ArcBox/Views/Containers/ContainerRowView.swift | Added accessibility labels for row/buttons and combined row label. |
| ArcBox/Views/Containers/ContainerEmptyState.swift | Refactored to shared EmptyStateView. |
| ArcBox/ViewModels/VolumesViewModel.swift | Tightened log privacy + deduped ISO8601 parsing via utility. |
| ArcBox/ViewModels/ServicesViewModel.swift | Tightened log privacy for K8s service load errors. |
| ArcBox/ViewModels/PodsViewModel.swift | Tightened log privacy for K8s pod load errors. |
| ArcBox/ViewModels/NetworksViewModel.swift | Tightened log privacy + deduped ISO8601 parsing via utility. |
| ArcBox/ViewModels/MachinesViewModel.swift | Added stub warnings + machine logger category. |
| ArcBox/ViewModels/KubernetesState.swift | Tightened log privacy for Kubernetes start errors. |
| ArcBox/ViewModels/ImagesViewModel.swift | Tightened log privacy for image operations and icon fetch failures. |
| ArcBox/ViewModels/ContainersViewModel.swift | Tightened log privacy for container operations and inspect logging. |
| ArcBox/Services/DockerEventMonitor.swift | Tightened log privacy for event stream reconnect warnings. |
| ArcBox/Models/Utilities.swift | Added shared parseISO8601Date(_:) helper. |
| ArcBox/Models/DockerTerminalSession.swift | Replaced magic numbers with constants + added locked FD teardown to prevent close races. |
| ArcBox/Logging.swift | Added Log.machine category. |
| ArcBox/Components/StatusBadge.swift | Added combined accessibility element/label. |
| ArcBox/Components/EmptyStateView.swift | Introduced reusable empty-state component used across views. |
| ArcBox/ArcBoxApp.swift | Added onOpenURL hook + deep-link handler stub + tightened log privacy for connection errors. |
| .github/workflows/pr.yml | Added CI step to verify generated protobuf code matches checked-in output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Replace .upToNextMinor(from: "1.0.0") with from: at actual compatible minimums to prevent dependency downgrade when Package.resolved is reset - CI: use local proto generation instead of --remote to avoid flaky external dependency; add untracked file check for generated code - Privacy: change remaining .public log interpolations to .private for codesign errors, deep links, icon fetch failures, machine IDs - VoiceOver: use .contain instead of .combine on ContainerRowView so action buttons remain individually focusable - DaemonManager: run codesign verification off MainActor to avoid blocking UI during startup
CI has no local arcbox checkout and the GitHub raw download fails with 404 on private repos. Use actions/checkout with sparse-checkout to fetch only the proto definitions, then generate with --local.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ABXD-54: Verify daemon binary code signature before registration. | ||
| // Run off MainActor to avoid blocking UI during codesign --verify. | ||
| await Task.detached { [self] in | ||
| self.verifyDaemonSignature() | ||
| }.value |
There was a problem hiding this comment.
Task.detached uses an @Sendable closure; capturing self (a @MainActor class that isn’t Sendable) will trigger strict-concurrency errors. Since verifyDaemonSignature() doesn’t use instance state, avoid capturing self by making the verifier a static/free function or by computing daemonPath first and passing only that value into the detached task.
| dependencies: [ | ||
| .package(url: "https://github.com/apple/swift-openapi-generator", from: "1.0.0"), | ||
| .package(url: "https://github.com/apple/swift-openapi-runtime", from: "1.0.0"), | ||
| .package(url: "https://github.com/swift-server/swift-openapi-async-http-client", from: "1.0.0"), | ||
| .package(url: "https://github.com/apple/swift-openapi-generator", from: "1.10.0"), | ||
| .package(url: "https://github.com/apple/swift-openapi-runtime", from: "1.10.0"), | ||
| .package(url: "https://github.com/swift-server/swift-openapi-async-http-client", from: "1.3.0"), |
There was a problem hiding this comment.
The PR description says SPM dependencies were changed from from: to .upToNextMinor(from:), but this file still uses from: (which allows updates up to the next major). If the intent is to pin to minor updates for audit reasons, switch these .package(...) entries to .upToNextMinor(from:) as described (or update the PR description if the behavior change is not intended).
| dependencies: [ | ||
| .package(url: "https://github.com/grpc/grpc-swift-nio-transport.git", from: "1.0.0"), | ||
| .package(url: "https://github.com/grpc/grpc-swift-protobuf.git", from: "1.0.0"), | ||
| .package(url: "https://github.com/apple/swift-protobuf.git", from: "1.28.1"), | ||
| .package(url: "https://github.com/grpc/grpc-swift-nio-transport.git", from: "1.2.0"), | ||
| .package(url: "https://github.com/grpc/grpc-swift-protobuf.git", from: "1.3.0"), | ||
| .package(url: "https://github.com/apple/swift-protobuf.git", from: "1.35.0"), |
There was a problem hiding this comment.
The PR description says SPM dependencies were changed from from: to .upToNextMinor(from:), but this file still uses from: (which allows updates up to the next major). If the intent is to pin to minor updates for audit reasons, switch these .package(...) entries to .upToNextMinor(from:) as described (or update the PR description if the behavior change is not intended).
| @@ -0,0 +1,57 @@ | |||
| import SwiftUI | |||
|
|
|||
| /// Reusable empty state component with icon, title, description area, and optional action button. | |||
There was a problem hiding this comment.
The doc comment says this component includes an "optional action button", but the implementation only provides icon, title, and a generic content slot. Either add an explicit optional action API (e.g. button title + handler) or update the comment to match the current capabilities.
| /// Reusable empty state component with icon, title, description area, and optional action button. | |
| /// Reusable empty state component with icon, title, and an optional custom content area. |
| import OSLog | ||
| import SwiftUI |
There was a problem hiding this comment.
import OSLog appears unused in this file (logging is done via the Log.machine wrapper). Consider removing the import to avoid an unused-import warning (or keep it only if you plan to reference Logger/OSLogMessage types directly here).
Summary
Resolves 18 audit issues from the Desktop Release Readiness Audit project (ABXD-17 through ABXD-85). All changes produced by 6 parallel agent worktrees, cherry-picked onto a single branch.
Commit 1: DaemonManager safety (ABXD-17/22/54)
isEnablingguard to prevent concurrentenableDaemon()calls acrossawaitsuspension pointsverifyDaemonSignature()— runscodesign --verify --strictbefore daemon launch (warn-only, doesn't block dev builds)masterFDwithOSAllocatedUnfairLock, atomic swap to -1 in teardownCommit 2: Log privacy + infra (ABXD-52/49/72)
privacy: .publicto.privateacross all ViewModels (11 files)pr.yml— runsgenerate.sh --remoteand fails on driftfrom:to.upToNextMinor(from:)in ArcBoxClient and DockerClient Package.swiftCommit 3: Code cleanup (ABXD-57/58/59/85)
parseISO8601Date()to Utilities.swift, replace duplicates in Networks/Volumes VMsLog.machine.warning()to MachinesViewModel stub methodslastError!with defaultURLError(.unknown)in DockerClient retryCommit 4: View layer (ABXD-37/60/61)
.accessibilityLabel()to 6 RowViews, 5 toolbar buttons, StatusBadgesidebaranddetailPanelcomputed propertiesCommit 5: Features (ABXD-48/62/43)
onOpenURLhandler stub +handleDeepLink()forarcbox://schemeAlso closed (no code changes needed)
package-dmg.sh --signStats
Test plan
xcodebuild test— all tests passcodesign --verifywarning appears in Xcode console for ad-hoc builds<private>for container IDs in Console.app