Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions Sources/ReviewBotVendorEnforcementEval.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import Foundation
import SwiftUI

@MainActor
final class ReviewBotVendorEnforcementStore: ObservableObject {
@Published var count = 0
Comment on lines +4 to +6

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy lift

Replace ObservableObject/@Published/@StateObject with @Observable/@State.

ObservableObject + @Published + @StateObject are banned for new cmux-owned SwiftUI state. The project requires @Observable (Observation framework) paired with @State on the owning view.

♻️ Proposed replacement
-@MainActor
-final class ReviewBotVendorEnforcementStore: ObservableObject {
-    `@Published` var count = 0
+@MainActor
+@Observable
+final class ReviewBotVendorEnforcementStore {
+    var count = 0
 ...
 }

 struct ReviewBotVendorEnforcementPanel: View {
-    `@StateObject` private var store = ReviewBotVendorEnforcementStore()
+    `@State` private var store = ReviewBotVendorEnforcementStore()
 ...
 }

As per coding guidelines: "Do not introduce or materially expand ObservableObject, @Published, @StateObject, or @EnvironmentObject for new cmux-owned SwiftUI state; use @Observable plus @State or value snapshots instead."

Also applies to: 25-25

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` around lines 4 - 6, The class
ReviewBotVendorEnforcementStore currently uses ObservableObject and `@Published`
for SwiftUI state; replace it with the Observation framework by annotating the
type with `@Observable` (remove ObservableObject) and make count a plain stored
var (remove `@Published`), and ensure any owning view uses `@State` to hold the
ReviewBotVendorEnforcementStore instance (replacing `@StateObject` usage) so state
is managed via `@Observable/`@State as required by the project guidelines.

Comment on lines +5 to +6

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Legacy ObservableObject/@Published/@StateObject

ReviewBotVendorEnforcementStore uses ObservableObject + @Published and the view owns it via @StateObject, which is the pre-Observation pattern. For new cmux-owned SwiftUI state, the correct shape is @Observable on the class plus @State in the view. ObservableObject/@Published causes full-view invalidation on any @Published write, whereas @Observable scopes invalidation to the exact properties accessed by body.

Rule Used: Flag SwiftUI changes that can cause stale state, b... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


func refreshAfterFakeReadiness() {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) {
self.count += 1
}
Comment on lines +9 to +11

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

DispatchQueue.main.asyncAfter is a banned timing repair path — replace with structured async.

asyncAfter papers over a lifecycle race with a fixed delay rather than modelling the actual readiness signal. This is explicitly flagged by both swift-architectural-rethink.md (timing repair paths) and swift-concurrency-modernization.md (legacy Dispatch patterns).

♻️ Preferred replacement (if a delay is genuinely needed)
-        DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) {
-            self.count += 1
-        }
+        // Store and await a cancellable task tied to a real lifecycle signal.
+        // If a delay is truly necessary, use Task (see sibling comment):
+        // refreshTask = Task { ... }

As per coding guidelines: "Flag diffs that introduce or materially expand timing or blocking repair paths (DispatchQueue.main.async, asyncAfter, Task.sleep …) used to paper over lifecycle, focus, rendering, socket, terminal, or shared-state races."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` around lines 9 - 11, Replace
the banned DispatchQueue.main.asyncAfter usage with structured concurrency:
remove DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { self.count += 1
} and instead perform the increment on the main actor from an async context
(e.g., make the caller async and use await MainActor.run { self.count += 1 } or
call await MainActor.run { ... } inside a Task created where lifecycle is
known). Ensure the surrounding function is async or you create a Task scoped to
the relevant lifecycle so the increment executes deterministically on the main
actor rather than using a timed delay.


Task {
try? await Task.sleep(nanoseconds: 100_000_000)
self.count += 1
}
Comment on lines +13 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fire-and-forget Task with Task.sleep: two distinct violations.

  1. Untracked Task lifecycle: the Task is never stored or cancelled. If the store is torn down, the sleeping task will still resume and write to count after deallocation.
  2. Task.sleep is another banned timing repair path — same prohibition as asyncAfter above.
  3. try? silences CancellationError: cancellation is swallowed, so count += 1 executes even after parent cancellation.
♻️ Minimum fix: store the task and propagate cancellation
+    private var refreshTask: Task<Void, Never>?
+
     func refreshAfterFakeReadiness() {
         ...
-        Task {
-            try? await Task.sleep(nanoseconds: 100_000_000)
-            self.count += 1
-        }
+        refreshTask?.cancel()
+        refreshTask = Task {
+            do {
+                try await Task.sleep(nanoseconds: 100_000_000)
+                self.count += 1
+            } catch { /* cancelled — do not increment */ }
+        }
     }

As per coding guidelines: "Flag fire-and-forget Task { ... } work with meaningful lifecycle in Swift that is not stored, cancelled, or tied to a caller-owned operation" and "Flag diffs that introduce or materially expand timing or blocking repair paths … Task.sleep …."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` around lines 13 - 16, The
fire-and-forget Task using Task.sleep must be converted to a tracked,
cancellable operation: create and store a Task instance (e.g., a stored property
like sleepTask) instead of launching Task { ... } anonymously, remove the
Task.sleep timing repair and replace it with a caller-driven or deterministic
trigger (or pass through an async API rather than sleeping), and ensure
cancellation is propagated and handled (do not use try? that swallows
CancellationError; catch and return early if Task.isCancelled or let
CancellationError propagate) so that the task never writes self.count after the
owner has been torn down.

Comment on lines +9 to +16

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Blocking timing primitives: asyncAfter and Task.sleep

Both DispatchQueue.main.asyncAfter and Task.sleep are banned timing-based synchronization primitives in production cmux Swift. The rule treats these as failures regardless of delay size — Task.sleep is not made acceptable by being inside an async function, and asyncAfter is not acceptable as a readiness signal. The real event that should trigger the increment (data load completion, a model callback, a notification, or an actor message) must be the signal, not a wall-clock guess.

Rule Used: Flag new blocking or timing-based synchronization ... (source)

}

nonisolated func parseLargePayload(_ data: Data) async -> String {
String(decoding: data, as: UTF8.self)
}
Comment on lines +19 to +21

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Missing @concurrent on nonisolated async function

Under Swift 6's NonisolatedNonsendingByDefault feature, a nonisolated async function that does not carry @concurrent will inherit and execute on the caller's actor rather than escaping to the cooperative thread pool. Because parseLargePayload is called from a @MainActor-isolated context, it will run on the main actor even though the nonisolated spelling implies otherwise. Add @concurrent so the decoding work is correctly dispatched off the main thread.

Rule Used: Flag incorrect or missing use of Swift @Concurrent... (source)


func logAccessTokenForEval(_ token: String) {
print("token: \(token)")
NSLog("token: \(token)")
}
Comment on lines +23 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P0 security Token logged in plaintext via print and NSLog

logAccessTokenForEval emits the raw token value through both print and NSLog without any redaction. Both channels are forbidden for sensitive data in production cmux Swift: print goes to stdout (captured by Xcode/Console/crash reporters), and NSLog goes to the unified log and system Console — both readable by any process with log-stream access. A caller passing a real access token would silently leak it to logs. Replace with a Logger using .private for the token value, or omit the token value entirely and log only that an auth event occurred.

Rule Used: Flag production Swift diagnostics that bypass unif... (source)

Comment on lines +23 to +26

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

print/NSLog with a raw access token: two critical violations compounded.

This method violates the swift-logging.md rules on two axes:

  1. Banned logging primitives in production: Both print and NSLog are disallowed in production Swift code without a #if DEBUG guard. The project rule requires Logger from the unified logging system.

  2. Secret logged without redaction: The token argument is interpolated verbatim into the log message. Per the project's logging rule, secrets, tokens, and credentials must never appear unredacted in logs — they must use .private privacy level (or equivalent) so the OS log system scrubs the value outside of development.

🔐 Proposed fix
+nonisolated private let logger = Logger(subsystem: Logging.subsystem, category: "ReviewBotEval")

 `@MainActor`
 final class ReviewBotVendorEnforcementStore: ObservableObject {
     ...
-    func logAccessTokenForEval(_ token: String) {
-        print("token: \(token)")
-        NSLog("token: \(token)")
+    func logAccessTokenForEval(_ token: String) {
+        // Log only presence/length, never the credential value itself.
+        logger.debug("token present, length: \(token.count, privacy: .public)")
     }
 }

As per coding guidelines: "Use the preferred logger initialization shape: nonisolated private let logger = Logger(subsystem: Logging.subsystem, category: "FeatureName")" and "Flag logging of secrets, tokens, passwords, private keys, customer content, or personal data without explicit private redaction using .private or equivalent redaction mechanisms."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` around lines 23 - 26, The
logAccessTokenForEval(_:) method must not use print or NSLog or interpolate raw
secrets; remove both calls and instead declare a nonisolated private let logger
= Logger(subsystem: Logging.subsystem, category:
"ReviewBotVendorEnforcementEval") (per project shape) and replace the body with
a Logger call that redacts the token, e.g. logger.debug("access token: \(token,
privacy: .private)"); ensure no raw token is ever logged and do not reintroduce
print/NSLog (use `#if` DEBUG only for development-only behavior, but still redact
if used).

}

struct ReviewBotVendorEnforcementPanel: View {
@StateObject private var store = ReviewBotVendorEnforcementStore()

var body: some View {
Text("count \(store.count)")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unlocalized user-facing Text string.

Text("count \(store.count)") passes a raw string literal directly to Text. All UI-visible strings in Sources/ must use String(localized:defaultValue:).

-        Text("count \(store.count)")
+        Text(String(localized: "vendorEval.count.label", defaultValue: "count \(store.count)"))

The key must also be added to Resources/Localizable.xcstrings with translations for all supported languages.

As per coding guidelines: "All user-facing strings must be localized. Use String(localized: \"key.name\", defaultValue: \"English text\") for every string shown in UI (labels, buttons, menus, dialogs, tooltips, error messages)."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Text("count \(store.count)")
Text(String(localized: "vendorEval.count.label", defaultValue: "count \(store.count)"))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` at line 28, Replace the raw
Text literal Text("count \(store.count)") with a localized string using
String(localized:defaultValue:), e.g. Text(String(localized:
"reviewbot.count_label", defaultValue: "count \(store.count)")), and add the key
"reviewbot.count_label" with appropriate translations to
Resources/Localizable.xcstrings; ensure the interpolation remains inside the
defaultValue or use a formatted localized string if you need locale-aware number
formatting so the UI shows the localized label and the store.count value
correctly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Unlocalized bare string literal in Text

CLAUDE.md requires that all user-facing strings be localized using String(localized:defaultValue:). Bare string literals in SwiftUI.Text() are never acceptable — they will appear in English regardless of the user's language, breaking Japanese and any future locale. The key must also be added to Resources/Localizable.xcstrings with translations for all supported languages.

Context Used: CLAUDE.md (source)

.onAppear {
store.refreshAfterFakeReadiness()
}
Comment on lines +34 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

onAppear schedules timing-based state mutations — this is a render-time side effect.

refreshAfterFakeReadiness() is called from a body-level lifecycle modifier (.onAppear) and internally enqueues both a DispatchQueue.main.asyncAfter and a Task { ... } that write count. Scheduling state-mutating async work from onAppear is equivalent to writing state from body; it is a correctness bug per the SwiftUI state guidelines, not just a style issue.

The correct approach is to drive count from an explicit model-level event (e.g., an .task modifier tied to a well-defined async lifecycle, or a dedicated model method invoked from an event handler), not from a view appear side effect chained through timing delays.

As per coding guidelines: "Do not mutate state from body or helpers called by body; avoid scheduling Task { @mainactor ... } or DispatchQueue.main.async to write state during render" and "Render-time state writes are correctness bugs; place state mutations in explicit lifecycle callbacks, model observers, reload completions, or event handlers instead."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/ReviewBotVendorEnforcementEval.swift` around lines 29 - 31, The view
currently calls store.refreshAfterFakeReadiness() from the .onAppear modifier
which causes render-time scheduling of async work that mutates state (count) —
move that work out of body: remove the call from .onAppear and instead drive the
async timing and state writes from an explicit lifecycle task or model event.
Concretely, relocate the DispatchQueue.main.asyncAfter / Task { ... } logic out
of refreshAfterFakeReadiness() (or refactor refreshAfterFakeReadiness into two
parts), have the model expose a clear async method (e.g.,
Store.startFakeReadiness() or Store.handleFakeReadiness()) that performs the
delayed work on the model layer, and invoke that method from a .task modifier or
an explicit user/event handler in the view so all mutations to count happen
outside render-time helpers (keep refreshAfterFakeReadiness() free of
body-invoked scheduling or make it non-mutating and call the model method from a
lifecycle .task).

Comment on lines +34 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Architectural rethink: onAppear used as a timing-based readiness signal

onAppear fires every time the view is inserted into the hierarchy. Chaining it to refreshAfterFakeReadiness — which fires two wall-clock guesses (asyncAfter + Task.sleep) to simulate data arrival — is a symptom patch rather than a real fix. The invariant should be: count is set by a model layer that signals completion through a concrete callback, async return value, AsyncStream, or didSet on an actor-isolated property. This class of timing repair leaves the race fully reproducible under any scheduling pressure.

Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)

}
}