Repository navigation
TEMP vendor lint eval: CodeRabbit enforcement - #3497
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIntroduces an ChangesVendor Enforcement Store & View
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ReviewBotVendorEnforcementEval.swift`:
- Around line 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.
- Around line 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).
- 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.
- Around line 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.
- Around line 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.
🪄 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: 5c9c3e2c-8e62-403c-b911-572394450072
📒 Files selected for processing (1)
Sources/ReviewBotVendorEnforcementEval.swift
| @MainActor | ||
| final class ReviewBotVendorEnforcementStore: ObservableObject { | ||
| @Published var count = 0 |
There was a problem hiding this comment.
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.
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { | ||
| self.count += 1 | ||
| } |
There was a problem hiding this comment.
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 | ||
| } |
There was a problem hiding this comment.
Fire-and-forget Task with Task.sleep: two distinct violations.
- Untracked
Tasklifecycle: theTaskis never stored or cancelled. If the store is torn down, the sleeping task will still resume and write tocountafter deallocation. Task.sleepis another banned timing repair path — same prohibition asasyncAfterabove.try?silencesCancellationError: cancellation is swallowed, socount += 1executes 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.
| @StateObject private var store = ReviewBotVendorEnforcementStore() | ||
|
|
||
| var body: some View { | ||
| Text("count \(store.count)") |
There was a problem hiding this comment.
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.
| 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.
| .onAppear { | ||
| store.refreshAfterFakeReadiness() | ||
| } |
There was a problem hiding this comment.
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).
Greptile SummaryThis PR intentionally introduces a Swift fixture ( Confidence Score: 0/5Do not merge — PR is explicitly marked 'do not merge' and contains a P0 credential-leak alongside five additional P1 rule violations. A P0 security finding (plaintext token logging) caps the score at 2; five additional P1 violations (timing primitives, ObservableObject, missing @Concurrent, unlocalized Text, onAppear side-effect) pull the score to 0. Sources/ReviewBotVendorEnforcementEval.swift — every function and view body in this file violates at least one cmux Swift rule.
|
| Filename | Overview |
|---|---|
| Sources/ReviewBotVendorEnforcementEval.swift | Intentional eval fixture with six distinct rule violations: timing primitives (asyncAfter + Task.sleep), legacy ObservableObject/@Published/@StateObject, missing @Concurrent on nonisolated async, unlocalized bare Text literal, onAppear-as-readiness-signal, and plaintext token logging via print/NSLog. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ReviewBotVendorEnforcementPanel.onAppear] --> B[refreshAfterFakeReadiness]
B --> C["DispatchQueue.main.asyncAfter(+0.25s)\n❌ timing primitive"]
B --> D["Task { Task.sleep(100ms) }\n❌ timing primitive"]
C --> E[count += 1]
D --> E
E --> F["Text('count \(store.count)')\n❌ unlocalized"]
G[logAccessTokenForEval token] --> H["print(token)\n❌ stdout leak"]
G --> I["NSLog(token)\n❌ unified log leak"]
J["parseLargePayload(data)\nnonisolated async\n❌ missing @concurrent"] --> K[runs on MainActor]
style C fill:#f88,stroke:#c00
style D fill:#f88,stroke:#c00
style F fill:#ffa,stroke:#aa0
style H fill:#f44,stroke:#900
style I fill:#f44,stroke:#900
style J fill:#f88,stroke:#c00
Reviews (2): Last reviewed commit: "Add vendor logging eval violation" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| DispatchQueue.main.asyncAfter(deadline: .now() + 0.25) { | ||
| self.count += 1 | ||
| } | ||
|
|
||
| Task { | ||
| try? await Task.sleep(nanoseconds: 100_000_000) | ||
| self.count += 1 | ||
| } |
There was a problem hiding this comment.
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)
| final class ReviewBotVendorEnforcementStore: ObservableObject { | ||
| @Published var count = 0 |
There was a problem hiding this comment.
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!
| nonisolated func parseLargePayload(_ data: Data) async -> String { | ||
| String(decoding: data, as: UTF8.self) | ||
| } |
There was a problem hiding this comment.
@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)
| @StateObject private var store = ReviewBotVendorEnforcementStore() | ||
|
|
||
| var body: some View { | ||
| Text("count \(store.count)") |
There was a problem hiding this comment.
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() | ||
| } |
There was a problem hiding this comment.
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)
|
@coderabbitai configuration |
|
Configuration used: Path: .coderabbit.yaml CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
inheritance: false
reviews:
profile: assertive
request_changes_workflow: true
high_level_summary: true
high_level_summary_instructions: ''
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
review_details: false
commit_status: true
fail_commit_status: false
collapse_walkthrough: true
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
suggested_reviewers_instructions: []
in_progress_fortune: true
poem: true
enable_prompt_for_ai_agents: true
path_filters: []
path_instructions:
- path: '**/*.swift'
instructions: >
Apply the cmux custom Swift lint rules in `.github/review-bot-rules/`
during review. Treat those files as the source of truth. Focus on
production Swift changes and ignore test-only scaffolding unless it
makes production behavior worse.
- path: '**/Package.swift'
instructions: >
Apply the cmux custom Swift lint rules in `.github/review-bot-rules/`,
especially the concurrency modernization, actor isolation, blocking
runtime, and architectural rethink rules.
- path: GhosttyTabs.xcodeproj/**
instructions: >
Review project wiring against the cmux Swift lint rules. Flag project
changes that enable app/runtime code paths which bypass Swift
concurrency, logging, localization, or shared action-path expectations.
abort_on_close: true
disable_cache: false
slop_detection:
enabled: true
auto_review:
enabled: true
description_keyword: ''
auto_incremental_review: true
auto_pause_after_reviewed_commits: 5
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
simplify:
enabled: false
custom: []
pre_merge_checks:
override_requested_reviewers_only: false
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks:
- mode: error
name: cmux Swift actor isolation
instructions: >
For production Swift changes, fail when the diff introduces or
materially worsens Swift 6 actor isolation mistakes from
`.github/review-bot-rules/swift-actor-isolation.md`: implicit
MainActor value models or service protocols, shared mutable Sendable
reference types without isolation, or UI-bound stores accessed from
background contexts. Pass for tests, actors, SwiftUI UI types
intentionally on MainActor, or existing debt not worsened.
- mode: error
name: cmux Swift blocking runtime
instructions: >
For production Swift changes, fail when the diff introduces or
materially expands blocking or timing-based synchronization from
`.github/review-bot-rules/swift-blocking-runtime.md`: semaphores,
blocking waits, sleeps, delayed dispatch, polling, main-queue sync, or
manual locks where an actor or explicit signal should own
synchronization. Pass for deterministic test-only scaffolding and
short user-visible UI animation delays that do not use `Task.sleep`.
- mode: error
name: cmux Swift concurrency
instructions: >
For cmux-owned Swift code, fail when the diff introduces or materially
expands legacy async patterns from
`.github/review-bot-rules/swift-concurrency-modernization.md`:
background Dispatch queues for ordinary async work, new Combine app
state, completion-handler APIs where async throws is under our
control, or fire-and-forget Tasks with real lifecycle. Pass for
required AppKit, SwiftUI, XCTest, OS, or third-party callback
boundaries.
- mode: error
name: cmux Swift `@concurrent`
instructions: >
For Swift changes, fail when the diff violates
`.github/review-bot-rules/swift-concurrent-annotation.md`: missing
`@concurrent` on `nonisolated async` work that should leave the caller
actor, invalid `@concurrent` on synchronous or actor-isolated
functions, or CPU/file/network-heavy async helpers called from UI
isolation without an explicit hop. Pass for intentionally UI-bound
async work.
- mode: error
name: cmux Swift logging
instructions: >
For production Swift changes, fail when the diff violates
`.github/review-bot-rules/swift-logging.md`: `print`, `debugPrint`,
`dump`, or `NSLog` in app/runtime code; ad hoc file/stdout logging for
diagnostics; MainActor-coupled file-scoped Logger constants; or logs
that expose secrets or personal data. Pass for CLI output, tests,
debug-only logs, and explicitly sanitized provider diagnostics.
- mode: error
name: cmux SwiftUI state layout
instructions: >
For SwiftUI changes, fail when the diff violates
`.github/review-bot-rules/swiftui-state-layout.md`: new
ObservableObject or `@Published` state where `@Observable` is the modern
shape, GeometryReader measurement that changes layout, lazy/list row
subtrees holding store references, or render-time state mutation. Pass
for existing legacy state only touched incidentally and contained
AppKit bridge views.
- mode: error
name: cmux architecture rethink
instructions: >
For Swift architecture changes, fail when the diff violates
`.github/review-bot-rules/swift-architectural-rethink.md`: symptom
patches using sleeps, delayed dispatch, polling, locks, observers,
side channels, duplicate entrypoint wiring, or split UI lifecycle
ownership that leaves bad state representable. Pass for small
correctness fixes with clear owners and invariants, required platform
bridges, and test-only synchronization.
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
trufflehog:
enabled: true
checkov:
enabled: true
tflint:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
fortitudeLint:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
opengrep:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
trivy:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
stylelint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
blinter:
enabled: true
smartyLint:
enabled: true
emberTemplateLint:
enabled: true
psscriptanalyzer:
enabled: true
chat:
art: true
allow_non_org_members: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns:
- AGENTS.md
- CLAUDE.md
- .github/review-bot-rules/*.md
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
linked_repositories: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
issue_enrichment:
auto_enrich:
enabled: false
planning:
enabled: true
auto_planning:
enabled: true
labels: []
labeling:
labeling_instructions: []
auto_apply_labels: false
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| func logAccessTokenForEval(_ token: String) { | ||
| print("token: \(token)") | ||
| NSLog("token: \(token)") | ||
| } |
There was a problem hiding this comment.
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)
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 `@Sources/ReviewBotVendorEnforcementEval.swift`:
- Around line 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).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c56e450-7d39-4482-b848-278218be0f40
📒 Files selected for processing (1)
Sources/ReviewBotVendorEnforcementEval.swift
| func logAccessTokenForEval(_ token: String) { | ||
| print("token: \(token)") | ||
| NSLog("token: \(token)") | ||
| } |
There was a problem hiding this comment.
print/NSLog with a raw access token: two critical violations compounded.
This method violates the swift-logging.md rules on two axes:
-
Banned logging primitives in production: Both
printandNSLogare disallowed in production Swift code without a#if DEBUGguard. The project rule requiresLoggerfrom the unified logging system. -
Secret logged without redaction: The
tokenargument 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.privateprivacy 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).
|
Closing temporary vendor lint enforcement eval PR. Results collected: Greptile caught the P0/P1 rule violations, CodeRabbit requested changes on the post-config incremental logging violation, and vendor status contexts still reported success. |
This branch was successfully deployed
1 participant
Temporary eval PR. Do not merge.
This intentionally adds a small bad Swift fixture after #3496 merged. Goal: verify CodeRabbit now uses base-branch
profile: assertiveplusrequest_changes_workflow: true, and that Greptile/CodeRabbit catch timing, Task.sleep, legacy ObservableObject, missing @Concurrent, onAppear side effects, and unlocalized Text.@coderabbitai review
@greptile-apps review
Summary by cubic
Add a temporary Swift fixture to validate CodeRabbit request-changes enforcement with the base-branch assertive profile. It intentionally uses timing APIs, Task.sleep, a nonisolated method on a @mainactor type, onAppear side effects, unlocalized Text, and plain-text access token logging (print/NSLog) to ensure vendor lint flags them.
Written for commit 7ebb4fe. Summary will update on new commits.
Summary by CodeRabbit