Add cmux comments list — read-only CLI surface for diff review comments - #9604
Conversation
Review comments saved in the diff viewer already reach agents through the TextBox pending pool (push). This adds the pull direction: a read-only CLI that asks the running app for a repository's saved comments, so external tools never depend on the store's key derivation or in-memory cache. - comments.list v2 method: canonicalizes repo_root via DiffCommentStore and delegates to a pure commentsListPayload(comments:repoRoot:includeConsumed:) so the reply shape is testable without a socket - cmux comments list [--repo <path>] [--all] [--json]: resolves the git toplevel (default: cwd), prints a human summary or JSON - CommentsListPayloadTests: default listing omits consumed comments, include_consumed adds them with an ISO8601 consumedAt, anchor fields are preserved, an empty store reports zero - cli-contract.md: command table row + no-socket help probe
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesRepository Comment Listing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CMUXCLI
participant TerminalController
participant DiffCommentStore
participant DiffCommentPayload
CMUXCLI->>TerminalController: Request comments.list with repo_root and includeConsumed
TerminalController->>DiffCommentStore: Retrieve repository comments
TerminalController->>DiffCommentPayload: Build filtered response
DiffCommentPayload-->>CMUXCLI: Return repository, count, and comments
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+Comments.swift:
- Around line 37-42: Update the comments list argument handling around
parseOption("--repo") to validate remainder before constructing params or
calling comments.list. Permit only the supported --all flag, and reject any
other -- option, preserving the existing include_consumed behavior for valid
input.
In `@Resources/Localizable.xcstrings`:
- Around line 35957-35967: Add localization entries for every locale represented
elsewhere in the Localizable.xcstrings catalog under cli.comments.usage,
retaining entries even when their value intentionally uses the accepted English
fallback. Keep the existing English text unchanged and ensure the key has
complete coverage for all supported locales.
In `@Sources/TerminalController`+CommentsCommands.swift:
- Line 26: Update the parameter parsing in the comments command around
includeConsumed to distinguish an absent include_consumed key from a present
non-Boolean value. Preserve false as the default only when the parameter is
absent; return invalid_params for malformed values such as strings, and add a
regression test covering this behavior.
- Around line 38-47: Update commentsListPayload and
DiffCommentsBridge.commentJSON to accept and reuse a single ISO8601DateFormatter
for the entire response. Remove formatter creation from the per-comment mapper
path, and construct listed with one pass while preserving the existing
consumedAt filtering and serialization behavior.
- Around line 22-25: Update the comments.list handler around canonicalRepoRoot
and commentsListPayload to validate repo_root with git rev-parse --show-toplevel
before reading comments. Reject paths that are not within a valid repository,
and use the resolved Git top-level path for both the response payload and
DiffCommentStore.shared.comments lookup instead of the normalized input path.
🪄 Autofix
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 Plus
Run ID: 587f8485-6334-49ff-8fed-eb86437eff30
📒 Files selected for processing (10)
CLI/CMUXCLI+CommandSuggestions.swiftCLI/CMUXCLI+Comments.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/Panels/DiffCommentsBridge.swiftSources/TerminalController+CommentsCommands.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommentsListPayloadTests.swiftdocs/cli-contract.md
…wn flags - Localizable.xcstrings: add the ja entry for cli.comments.usage and add the socket.comments.missingRepoRoot key, which the handler referenced but the catalog never defined. Both keys now match the en+ja coverage that the other cli.*.usage and socket.* keys use. - DiffCommentsBridge: add commentJSON(_:formatter:) so a caller mapping many comments allocates one ISO8601DateFormatter per reply instead of one per comment; the existing single-comment signature delegates to it. - cmux comments list: reject unrecognized -- options instead of ignoring them, so a typo cannot read as a supported request.
|
Pushed 14fdf2a addressing the review. Fixed
Discussing rather than changing (details in the threads)
Verification after the fixes, on macOS 26.5 / Xcode 26.6:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/CMUXCLI+Comments.swift (1)
78-90: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFail closed on an invalid
comments.listpayload.
printCommentsListPayloadaccepts malformed payloads and renders output with default values. If the response is not a top-level result with acommentsarray, or a comment is missing required fields, report a CLI error instead of returning0lines, an emptyrepo, or a line at0. Pending comments intentionally omitconsumedAt; do not add a separatenullhandling path for that field.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLI/CMUXCLI`+Comments.swift around lines 78 - 90, Update printCommentsListPayload to validate the top-level result, require a comments array, and require each comment’s required file, line, and repository fields instead of substituting defaults such as “?”, an empty repository, or line 0; report a CLI error and stop processing when validation fails. Preserve the existing pending-state behavior where consumedAt is absent, without adding separate null handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+Comments.swift:
- Around line 37-43: Update the remainder validation in the comments-list option
parsing around parseOption and includeConsumed to reject every token except the
supported --all flag, including unexpected positional arguments. Preserve the
existing unknown-option error path and perform this validation before resolving
the repository.
In `@Resources/Localizable.xcstrings`:
- Around line 36082-36098: The localization entries cli.comments.usage and
socket.comments.missingRepoRoot are incomplete. Update both keys in
Resources/Localizable.xcstrings at lines 36082-36098 and 229905-229921 to
include reviewed entries for all 20 supported locales, preserving the existing
comments.list and repo_root content for socket.comments.missingRepoRoot.
In `@Sources/Panels/DiffCommentsBridge.swift`:
- Around line 227-236: Update the browser comments.list response to create a
single ISO8601DateFormatter and map each comment through
commentJSON(_:formatter:), avoiding the per-comment wrapper allocation. Keep the
commentJSON(_:) wrapper unchanged for single-comment responses.
---
Outside diff comments:
In `@CLI/CMUXCLI`+Comments.swift:
- Around line 78-90: Update printCommentsListPayload to validate the top-level
result, require a comments array, and require each comment’s required file,
line, and repository fields instead of substituting defaults such as “?”, an
empty repository, or line 0; report a CLI error and stop processing when
validation fails. Preserve the existing pending-state behavior where consumedAt
is absent, without adding separate null handling.
🪄 Autofix
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 Plus
Run ID: a7a62943-9cc2-4186-b0f6-3ed68acd39b5
📒 Files selected for processing (4)
CLI/CMUXCLI+Comments.swiftResources/Localizable.xcstringsSources/Panels/DiffCommentsBridge.swiftSources/TerminalController+CommentsCommands.swift
cli.comments.usage and socket.comments.missingRepoRoot now carry entries for all 20 locales the catalog supports. The reviewed en and ja values are unchanged; the remaining 18 locales use the accepted English fallback, which is how 229 of the catalog's other fully-covered keys are written.
|
Pushed 86311bc. Conceding the localization point: Current state of the review:
Verification on the pushed tree: BUILD SUCCEEDED, |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 230016-230019: Update the localized string entry for
socket.comments.missingRepoRoot in Localizable.xcstrings to use product-facing
wording such as “A repository path is required.” for every supported locale,
removing internal identifiers like comments.list and repo_root while preserving
the existing translation structure and metadata.
🪄 Autofix
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 Plus
Run ID: 693cffb9-1e15-4472-92cb-e40744eaa9d1
📒 Files selected for processing (1)
Resources/Localizable.xcstrings
…bridge formatter - comments list now rejects every unrecognized remainder token, not only ones starting with --, so a stray positional cannot be silently ignored. - The webview bridge's comments.list response builds one ISO8601DateFormatter and passes it to commentJSON, matching the socket response. - Route the command's user-facing text through the catalog: nine new cli.comments.* keys for the errors and list output, each covering all 20 locales with the reviewed ja value and the accepted English fallback.
|
Pushed 4b7e990, and I have replied on every thread so the open/closed state is explicit. Fixed in this round
Discussing rather than changing
Closed from my side (no action pending): Verification on 4b7e990: BUILD SUCCEEDED; |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/DiffCommentsBridge.swift (1)
228-237: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude
consumedAtin the shared comment payload.
comments.listserializes comments through this mapper at Lines 169-170, but the mapper omitsDiffComment.consumedAt. The CLI renderer inCLI/CMUXCLI+Comments.swiftusescomment["consumedAt"]to displaypendingorconsumed. As a result,cmux comments list --alllabels every returned comment as pending.Add
consumedAtwhen it is present.Proposed fix
if let endSide = comment.endSide { json["endSide"] = endSide } + if let consumedAt = comment.consumedAt { + json["consumedAt"] = formatter.string(from: consumedAt) + } return json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/DiffCommentsBridge.swift` around lines 228 - 237, Update the shared comment payload mapper commentJSON(_:formatter:) to include DiffComment.consumedAt when it is non-nil, preserving the existing omission behavior when absent. This ensures comments.list provides the consumedAt value used by the CLI renderer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+Comments.swift:
- Around line 40-50: Validate the parsed repoOption immediately after
parseOption in the comments-list argument handling: if it begins with “--”,
reject it with a CLIError stating that --repo requires a path. Keep the existing
remainder validation and repository resolution unchanged, while allowing
dash-prefixed paths that do not start with “--”.
- Around line 111-118: Update the comments list header localization in the
surrounding comments-list flow to select a plural-specific key based on
comments.count, using cli.comments.list.header.one for a single comment and
cli.comments.list.header.other otherwise. Replace the literal comment(s)
localization string while preserving the existing count and repoRoot formatting
arguments.
---
Outside diff comments:
In `@Sources/Panels/DiffCommentsBridge.swift`:
- Around line 228-237: Update the shared comment payload mapper
commentJSON(_:formatter:) to include DiffComment.consumedAt when it is non-nil,
preserving the existing omission behavior when absent. This ensures
comments.list provides the consumedAt value used by the CLI renderer.
🪄 Autofix
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 Plus
Run ID: 8fb21b41-5ec6-49c1-a5e7-290b6a429c74
📒 Files selected for processing (3)
CLI/CMUXCLI+Comments.swiftResources/Localizable.xcstringsSources/Panels/DiffCommentsBridge.swift
- parseOption takes the token after --repo verbatim, so `--repo --all` would resolve a repository named "--all" and hand it to git as a path. Reject a value starting with --, pointing at ./-name for dash-prefixed paths. - Replace the literal "comment(s)" header with cli.comments.list.header.one and .other selected by count, matching how cli.memory.output.processCount is written. Both keys cover all 20 locales.
…trings - Replace the English fallbacks with real translations for all 18 remaining locales across the 13 keys this PR adds. The internationalization rule lists copied English as an unacceptable way to fill a locale slot, so matching the catalog's existing fallback habit was not enough for new keys. CLI tokens and format specifiers are preserved verbatim. - socket.comments.missingRepoRoot now reads "A repository path is required." The pre-merge privacy check wants API identifiers out of user-facing text; the CLI already fails with its own message first, so a direct socket caller loses nothing it cannot get from the invalid_params code. - Document the four comments CLI helpers.
|
Pushed 05419f4 for the pre-merge checks. Two resolved, one I would like to discuss. Full Internationalization — fixed. You were right and my earlier reasoning was wrong twice over. User-Facing Error Privacy — changed to your wording. Docstring Coverage — fixed. Documented the four Swift Package Boundaries — I would rather not do this here. The check asks for Two options, whichever you prefer:
Verification on 05419f4: BUILD SUCCEEDED; |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Both surfaces that serialize review comments now share one type that touches neither AppKit nor controller state: TerminalController keeps only the socket dispatch, DiffCommentsBridge keeps only the WebKit glue, and the mapping and consumed-filtering live in DiffCommentPayload with the tests pointed at it. This also makes the two response paths structurally identical, so the webview reply cannot drift back to allocating a formatter per comment. If the project wants this behind a SwiftPM package, it is now a file move rather than a refactor.
|
Pushed fc5ef0d — I went ahead with option 1 from my last comment rather than leave the boundary finding sitting.
Two things this bought beyond tidiness:
Still your call whether the package lands here or as a follow-up — I did not create it, because it would move types that predate this PR and I would rather a maintainer sanction that. Verification on fc5ef0d: BUILD SUCCEEDED; |
|
On the two remaining errors — I read both rule files before answering, and I think both are pointing at pre-existing structure rather than something this PR introduces. Happy to be overruled on either. Expensive Synchronous Load.
It is also not a path this PR put on the main actor. There is also a correctness reason not to move only my caller off-main: the store's cache is what keeps the socket reply consistent with the bridge's writes. A second, background reader would mean two caches over one file, and a stale read is a real failure mode here — I hit it twice while testing, where an externally edited store file stayed invisible until the app restarted. If the project wants this read off-main, it should move for all three callers together, with the bridge's pending-pool registration staying on Swift Package Boundaries. The ask has grown to include the model and the persistence boundary — Docstring Coverage (warning): every function this PR adds carries a |
|
@coderabbitai could you re-evaluate the two remaining pre-merge errors against the evidence in my previous comment, and tell me plainly whether you want me to implement them in this PR? Specifically:
If you agree these are out of scope for a CLI-surface PR, please say so explicitly so a maintainer reading the gate knows the errors were considered rather than ignored. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Candidate rule files =='
fd -t f . | rg 'swift-expensive-sync-load\.md|swift-package-boundaries\.md|DiffCommentPayload\.swift|DiffCommentStore\.swift|DiffCommentsBridge\.swift|TerminalController.*Comments.*\.swift|TextBoxInput.*\.swift|Package\.swift$'
echo '== Source outlines =='
for f in \
Sources/DiffCommentPayload.swift \
Sources/DiffCommentStore.swift \
Sources/Panels/DiffCommentsBridge.swift \
Sources/TerminalController+CommentsCommands.swift
do
if [ -f "$f" ]; then
echo "--- $f"
ast-grep outline "$f" --items all || true
fi
done
echo '== Rule text =='
while IFS= read -r f; do
echo "--- $f"
sed -n '1,240p' "$f"
done < <(fd -t f . | rg 'swift-expensive-sync-load\.md|swift-package-boundaries\.md')
echo '== Relevant call sites and type declarations =='
rg -n -C 5 -P '\b(DiffCommentStore|DiffCommentPayload|commentsListPayload|commentJSON|comments\s*\(repoRoot:|comments\.list|TextBoxInput)\b' \
Sources cmuxTests CLI 2>/dev/null || true
echo '== Swift package manifests =='
fd -t f '^Package\.swift$' . -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {} \;Length of output: 6167 🏁 Script executed: #!/bin/bash
set -u
echo '== Locate rule files, including hidden paths =='
find . -type f \( -name 'swift-expensive-sync-load.md' -o -name 'swift-package-boundaries.md' \) -print 2>/dev/null || true
echo '== PR changed-file summary =='
git diff --stat origin/main...HEAD 2>/dev/null || git diff --stat HEAD^...HEAD 2>/dev/null || true
git diff --name-status origin/main...HEAD 2>/dev/null || git diff --name-status HEAD^...HEAD 2>/dev/null || true
echo '== DiffComment payload and store =='
for f in Sources/DiffCommentPayload.swift Sources/DiffCommentStore.swift; do
if [ -f "$f" ]; then
echo "--- $f"
nl -ba "$f" | sed -n '1,280p'
fi
done
echo '== Socket handler =='
if [ -f Sources/TerminalController+CommentsCommands.swift ]; then
nl -ba Sources/TerminalController+CommentsCommands.swift | sed -n '1,180p'
fi
echo '== Bridge and TextBoxInput store call contexts =='
rg -n -C 12 -P '\b(store|DiffCommentStore)\.comments\s*\(|\bDiffCommentStore\s*\(' \
Sources/Panels/DiffCommentsBridge.swift Sources/TextBoxInput.swift Sources/TerminalController+CommentsCommands.swift 2>/dev/null || true
echo '== All comments(repoRoot:) declarations and calls =='
rg -n -C 6 -P '\bcomments\s*\(\s*repoRoot\s*:' Sources cmuxTests 2>/dev/null || true
echo '== Candidate package conventions =='
for f in Packages/macOS/CmuxGit/Package.swift Packages/macOS/CmuxFoundation/Package.swift; do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,180p' "$f"
fi
doneLength of output: 16265 🏁 Script executed: #!/bin/bash
set -u
echo '== Rule: expensive synchronous load =='
sed -n '1,260p' .github/review-bot-rules/swift-expensive-sync-load.md 2>/dev/null || true
echo '== Rule: Swift package boundaries =='
sed -n '1,300p' .github/review-bot-rules/swift-package-boundaries.md 2>/dev/null || true
echo '== DiffCommentPayload =='
sed -n '1,240p' Sources/DiffCommentPayload.swift 2>/dev/null || true
echo '== DiffCommentStore =='
sed -n '1,300p' Sources/DiffCommentStore.swift 2>/dev/null || true
echo '== Comments socket handler =='
sed -n '1,180p' Sources/TerminalController+CommentsCommands.swift 2>/dev/null || true
echo '== TerminalController isolation and dispatch context =='
rg -n -C 8 -P '`@MainActor`|class\s+TerminalController\b|extension\s+TerminalController\b|v2CommentsList\s*\(' \
Sources/TerminalController.swift Sources/TerminalController+CommentsCommands.swift 2>/dev/null || true
echo '== Focused PR changes to payload, bridge, and socket handler =='
git diff --find-renames origin/main...HEAD -- \
Sources/DiffCommentPayload.swift \
Sources/Panels/DiffCommentsBridge.swift \
Sources/TerminalController+CommentsCommands.swift 2>/dev/null \
|| git diff --find-renames HEAD^...HEAD -- \
Sources/DiffCommentPayload.swift \
Sources/Panels/DiffCommentsBridge.swift \
Sources/TerminalController+CommentsCommands.swift 2>/dev/null \
|| true
echo '== DiffComment references outside tests =='
rg -n -C 3 -P '\bDiffComment\b|\bDiffCommentStore\b' Sources -g '*.swift' 2>/dev/null || trueLength of output: 80139
✏️ Learnings added
You are interacting with an AI system. |
Per the package-boundary review: the comment model and its wire mapping are reusable domain logic shared by the WebKit bridge, the socket method, and the tests, so they move behind a SwiftPM boundary. - Packages/macOS/CmuxDiffComments exposes public DiffComment and DiffCommentPayload and depends on Foundation only. - Persistence stays app-side: DiffCommentStore keeps its directory resolution and the SessionRestorePolicy test check, so no app-wide dependency crosses into the package. - The app target, bridge, socket handler, and both test files import the package; no behavior changes.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Done in e4f26e5 — thanks for the clear adjudication. Swift Package Boundaries: implemented as you specified.
Expensive Synchronous Load: noted, and thanks for recording the cache-coherence constraint — that matches what I hit in testing. If the persistence design is revisited later, the bridge, Verification on e4f26e5:
|
|
@coderabbitai full review The pre-merge table still reflects the pre-e4f26e5 tree. Since then: the |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Around line 11649-11652: Update the package field in the CmuxDiffComments
XCSwiftPackageProductDependency object to reference the PBXFileReference UUID
whose object definition is the CmuxDiffComments local Swift package, namely
CC0033E15A1B2C3D4E5F0010, instead of the CMUXDebugLog reference. Validate the
UUID against its actual package-reference definition.
In `@docs/cli-contract.md`:
- Line 105: Update the comments CLI entry in the contract to document both
supported subcommand names, `list` and its `ls` alias, while preserving the
existing options, behavior, and socket method details.
🪄 Autofix
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 Plus
Run ID: 21b81237-762d-4995-b005-a8fc16097c8b
📒 Files selected for processing (15)
CLI/CMUXCLI+CommandSuggestions.swiftCLI/CMUXCLI+Comments.swiftCLI/cmux.swiftPackages/macOS/CmuxDiffComments/Package.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffComment.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swiftResources/Localizable.xcstringsSources/DiffCommentStore.swiftSources/Panels/DiffCommentsBridge.swiftSources/TerminalController+CommentsCommands.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommentsListPayloadTests.swiftcmuxTests/DiffCommentStoreTests.swiftdocs/cli-contract.md
The product dependency I added carried CMUXDebugLog's package UUID with a CmuxDiffComments comment, so a clean resolve could have associated the product with the wrong package. My local builds hid it behind an already-resolved SourcePackages cache. Audited every XCSwiftPackageProductDependency in the project against its referenced object: no other mismatch. Verified with SourcePackages deleted so resolution ran from scratch. Also documents the `ls` alias in the comments contract row.
…mespace The ambient-global-state rule flags a caseless enum whose whole API is static helpers, and it was the right call here: an instance can own the formatter, so "one formatter per reply" is now a property of the type rather than something each caller has to remember. Tests can inject a formatter too. TerminalController, DiffCommentsBridge, and the tests construct a mapper; the bridge's list reply collapses to comments.map(payload.json).
|
Pushed 10a04ae. One of the three new errors was right; for the other two I think the evidence points the other way. No Ambient Global State — fixed, and it improved the design. You were right that a caseless enum whose whole API is static helpers is the flagged shape, and the exemption in the rule covers Swiftpm Lockfiles — I believe this is a false positive. Swift Blocking Runtime — also not something this PR introduces. If you want git resolution moved off that helper, it should change for the whole CLI rather than this one call site, and I would rather do that as its own PR. Verification on 10a04ae: BUILD SUCCEEDED; |
|
@coderabbitai please adjudicate the two I pushed back on (Swiftpm Lockfiles, Swift Blocking Runtime) the way you did last round — tell me plainly whether you want changes in this PR, and I will implement or file follow-ups accordingly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="origin/main"
if ! git rev-parse --verify -q "${base}^{commit}" >/dev/null; then
base="HEAD^"
fi
echo '== Applicable rule files =='
for f in \
.github/review-bot-rules/swiftpm-lockfiles.md \
.github/review-bot-rules/swift-blocking-runtime.md
do
if [ -f "$f" ]; then
echo "--- $f"
sed -n '1,300p' "$f"
else
echo "--- missing: $f"
fi
done
echo '== Focused PR changes =='
git diff --find-renames --stat "$base...HEAD" -- \
CLI \
Packages/macOS/CmuxDiffComments \
cmux.xcodeproj \
':(glob)**/Package.resolved' \
2>/dev/null || true
git diff --find-renames -U25 "$base...HEAD" -- \
CLI/CMUXCLI+Comments.swift \
CLI/cmux.swift \
Packages/macOS/CmuxDiffComments/Package.swift \
cmux.xcodeproj/project.pbxproj \
':(glob)**/Package.resolved' \
2>/dev/null || true
echo '== SwiftPM manifests and resolved files =='
fd -HI -t f '^(Package\.swift|Package\.resolved)$' . -x sh -c '
echo "--- $1"
sed -n "1,260p" "$1"
' sh {}
echo '== Xcode package references =='
rg -n -C 5 -P '\b(XCLocalSwiftPackageReference|XCRemoteSwiftPackageReference|CmuxDiffComments|packageReferences)\b' \
cmux.xcodeproj/project.pbxproj 2>/dev/null || true
echo '== CLI process helper declarations and relevant calls =='
fd -t f -e swift . | while IFS= read -r f; do
if rg -q -P '\b(class|struct|enum)\s+CLIProcessRunner\b|\b(runProcess|gitSingleLine|gitStdout)\s*\(' "$f"; then
echo "--- $f"
rg -n -C 7 -P '\b(class|struct|enum)\s+CLIProcessRunner\b|\b(runProcess|gitSingleLine|gitStdout)\s*\(' "$f"
fi
done
echo '== Blocking primitives introduced by this PR =='
git diff -U3 "$base...HEAD" -- '*.swift' \
| rg -n -C 3 -P '^[+](?!\+\+).*\b(DispatchSemaphore|semaphore\.wait|Thread\.sleep|sleep\s*\(|DispatchQueue\.main\.sync|NSLock|os_unfair_lock|pthread_mutex)\b' \
|| trueLength of output: 111479 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="origin/main"
if ! git rev-parse --verify -q "${base}^{commit}" >/dev/null; then
base="HEAD^"
fi
echo '== Rule: SwiftPM lockfiles =='
sed -n '1,320p' .github/review-bot-rules/swiftpm-lockfiles.md 2>/dev/null || true
echo '== Rule: Swift blocking runtime =='
sed -n '1,360p' .github/review-bot-rules/swift-blocking-runtime.md 2>/dev/null || true
echo '== PR package and lockfile status =='
git diff --name-status "$base...HEAD" -- \
Packages/macOS/CmuxDiffComments \
cmux.xcodeproj/project.pbxproj \
cmux.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved \
|| true
echo '== CmuxDiffComments manifest =='
sed -n '1,220p' Packages/macOS/CmuxDiffComments/Package.swift 2>/dev/null || true
echo '== Xcode package references for CmuxDiffComments =='
rg -n -A12 -B8 -F 'CmuxDiffComments' cmux.xcodeproj/project.pbxproj 2>/dev/null || true
rg -n -A12 -B5 -P 'XCLocalSwiftPackageReference|XCRemoteSwiftPackageReference' cmux.xcodeproj/project.pbxproj 2>/dev/null || true
echo '== Resolved-package identities and locations =='
python3 - <<'PY'
import json
from pathlib import Path
for path in Path('.').rglob('Package.resolved'):
try:
data = json.loads(path.read_text())
except Exception as exc:
print(f'{path}: cannot parse: {exc}')
continue
pins = data.get('pins', data.get('object', {}).get('pins', []))
print(f'--- {path} ({len(pins)} pins)')
for pin in pins:
state = pin.get('state', {})
print(f"{pin.get('identity') or pin.get('package')}: {pin.get('location') or pin.get('repositoryURL')} {state.get('revision', '')}")
PY
echo '== Exact comments CLI implementation =='
sed -n '1,210p' CLI/CMUXCLI+Comments.swift 2>/dev/null || true
echo '== Exact process-runner implementation =='
sed -n '250,430p' CLI/CMUXCLI+Process.swift 2>/dev/null || true
echo '== Existing git-root helper usages =='
rg -n -C 6 -P '\b(gitSingleLine|gitStdout|CLIProcessRunner\.runProcess)\s*\(' \
CLI/CMUXCLI+Comments.swift CLI/cmux_open.swift CLI/CMUXCLI+Process.swift 2>/dev/null || true
echo '== Relevant added lines only =='
git diff -U12 "$base...HEAD" -- CLI/CMUXCLI+Comments.swift CLI/CMUXCLI+Process.swift \
Packages/macOS/CmuxDiffComments/Package.swift cmux.xcodeproj/project.pbxproj \
':(glob)**/Package.resolved' \
2>/dev/null || trueLength of output: 166697
I withdraw both findings. A process-runner redesign can be a separate, CLI-wide change if the project decides to remove its existing synchronous execution model. ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai full review Everything you asked for has landed: the |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 36189-36312: Update the localization entry for
cli.comments.list.header.other to define locale-aware plural-category variants,
especially for Russian, Polish, and Ukrainian, using the correct forms for
counts such as 2 while preserving the existing translations for other
categories. Ensure the comment-count formatting selects the variant through the
platform’s locale-aware plural rules rather than always using the “other” form.
🪄 Autofix
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 Plus
Run ID: 5ac428d1-7c94-444b-84ef-4c84f23b8629
📒 Files selected for processing (15)
CLI/CMUXCLI+CommandSuggestions.swiftCLI/CMUXCLI+Comments.swiftCLI/cmux.swiftPackages/macOS/CmuxDiffComments/Package.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffComment.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swiftResources/Localizable.xcstringsSources/DiffCommentStore.swiftSources/Panels/DiffCommentsBridge.swiftSources/TerminalController+CommentsCommands.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommentsListPayloadTests.swiftcmuxTests/DiffCommentStoreTests.swiftdocs/cli-contract.md
… strings The Slavic plural finding was right, but CLDR variations do not fire here: the CLI resolves strings against its own bundle, which carries no string catalog, so every localized CLI string renders its defaultValue. Measured — with the header moved to variations, count=1 printed "1 review comments". So selection stays in code (correct singular today) and the plural strings avoid letting the numeral govern the noun, which keeps one form grammatical for every count above one: ru "Комментарии ревью: %1$lld", uk "Коментарі рецензування: %1$lld", pl "Komentarze przeglądu: %1$lld", ar "تعليقات المراجعة: %1$lld". Bosnian already used a form valid for every count above one.
|
@coderabbitai full review 0391f22 is the current head. Every finding from all rounds is either fixed or withdrawn, and the last one confirmed. Please refresh the pre-merge table so a maintainer sees the state of the branch rather than an earlier tree. On the CLI localization-bundle gap: yes please, open the follow-up issue — that work is real but belongs outside a PR whose subject is one read-only command. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current PR head and changed files =='
git rev-parse HEAD
git show -s --format='%H%n%s' HEAD
base="origin/main"
if ! git rev-parse --verify -q "${base}^{commit}" >/dev/null; then
base="HEAD^"
fi
git diff --name-status "$base...HEAD"
git diff --stat "$base...HEAD"
echo '== Final CLI comments implementation outline =='
ast-grep outline CLI/CMUXCLI+Comments.swift --items all 2>/dev/null || true
sed -n '1,260p' CLI/CMUXCLI+Comments.swift 2>/dev/null || true
echo '== CLI localization bundle references and localization catalog wiring =='
rg -n -C 5 -P '\b(Bundle|bundle|Localizable|localized|localizedString|String\(localized:|NSLocalizedString)\b' \
CLI Resources Sources -g '*.swift' -g '*.xcstrings' 2>/dev/null | head -n 1000 || true
echo '== Changed localization keys and CLI contract =='
git diff -U8 "$base...HEAD" -- Resources/Localizable.xcstrings docs/cli-contract.md CLI/cmux.swift \
2>/dev/null || true
echo '== Current tests related to comments listing =='
ast-grep outline cmuxTests/CommentsListPayloadTests.swift --items all 2>/dev/null || true
sed -n '1,260p' cmuxTests/CommentsListPayloadTests.swift 2>/dev/null || true
echo '== Requester comment permalink =='
gh api repos/manaflow-ai/cmux/issues/9604/comments --paginate \
--jq '.[] | select(.user.login == "haung921209" and (.body | contains("CLI localization-bundle gap"))) | .html_url' \
| tail -n 1Length of output: 157135 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Shared package public API =='
ast-grep outline Packages/macOS/CmuxDiffComments/Sources/CmuxDiffComments --items all 2>/dev/null || true
sed -n '1,220p' Packages/macOS/CmuxDiffComments/Package.swift
sed -n '1,240p' Packages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffComment.swift
sed -n '1,260p' Packages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swift
echo '== Socket handler and bridge mapping =='
sed -n '1,220p' Sources/TerminalController+CommentsCommands.swift
rg -n -C 8 -P '\bcomments\.list\b|\bv2CommentsList\s*\(|\bDiffCommentPayload\b' \
Sources/TerminalController.swift Sources/Panels/DiffCommentsBridge.swift
echo '== Xcode local-package reference and target linkage =='
rg -n -C 6 -F 'CmuxDiffComments' cmux.xcodeproj/project.pbxproj
echo '== Create requested follow-up issue =='
issue_body=$(cat <<'EOF'
## Summary
The `cmux` CLI uses `String(localized:)` for user-visible strings, but its executable bundle does not include the app string catalog. As a result, CLI output uses each call site's `defaultValue` instead of the translations in `Resources/Localizable.xcstrings`.
PR `#9604` adds localized `cli.comments.*` strings and documents this limitation in `CLI/CMUXCLI+Comments.swift`. The limitation also affects existing CLI localization calls. This work is outside the scope of the read-only `cmux comments list` command.
## Required changes
- Define how the CLI locates and loads the localized resources when it runs from supported app and development layouts.
- Package or otherwise expose the required string catalog resources to the CLI.
- Update CLI localization lookup so `String(localized:)` resolves from the intended resource bundle.
- Preserve `defaultValue` behavior when the localization resources are unavailable.
- Add focused tests or probes for at least one non-English locale and the fallback path.
## Affected areas
- `CLI/` localization lookup and executable resource discovery.
- `Resources/Localizable.xcstrings` resource packaging.
- Xcode target and build-resource configuration.
- CLI contract or test probes, as needed.
## Rationale
The current CLI command is correct for its English fallback output. A repository-wide bundle solution is required before the translated catalog values can reach CLI users.
## Acceptance criteria
- A supported non-English locale changes CLI output from the English `defaultValue` to the catalog translation.
- The CLI still produces the `defaultValue` when localization resources are not available.
- Existing CLI commands that use `String(localized:)` use the same resource-resolution path.
- The solution does not require each CLI command to implement custom bundle lookup.
## Backlinks
- Pull request: https://github.com/manaflow-ai/cmux/pull/9604
- Requester comment: https://github.com/manaflow-ai/cmux/pull/9604#issuecomment-5187113377
- Requested by: `@haung921209`
EOF
)
gh issue create \
--repo manaflow-ai/cmux \
--title 'Make localized string-catalog resources available to the cmux CLI' \
--body "$issue_body" \
--assignee haung921209Length of output: 22957
I found no new blocking issue in the current tree. The The pre-merge findings from earlier revisions remain fixed or withdrawn. The CLI localization-bundle gap is the only follow-up item. It does not block this command. I created and assigned ✅ Action performedFull review finished. |
The internationalization check is right that cmux-cli ships no string catalog, so String(localized:) in the CLI renders its default value — measured earlier when a plural variation failed to select. Rather than add a resource phase to a command-line tool, this routes the command's strings through CMUXDiffViewerLocalization, which already resolves CLIExecutableLocator.enclosingAppBundle() and honors AppleLanguages. Verified per locale: help renders 使い方 under ja and 사용법 under ko, the list header renders "Комментарии ревью: 3" under ru, state labels render 未対応 and в ожидании, and the guards render their Russian and Japanese messages. Also documents DiffComment.init and the payload tests.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
Pushed 0d93f18. Full Internationalization — fixed, and the check was pointing at something real. You are right that It now actually localizes, which I could not claim before: So the 20 locales are load-bearing now instead of decorative, and this is the first CLI command in the repo where they are. The CLI-wide switch for the other commands is still the follow-up you offered to file. Swiftpm Lockfiles — this is the one you already adjudicated and withdrew, with the learning recorded: local Docstring Coverage — documented Verification on 0d93f18: BUILD SUCCEEDED; |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Line 5314: Add the existing CmuxDiffComments PBXBuildFile entry with
productRef CC0033E15A1B2C3D4E5F0011 to the cmux app Frameworks build phase
A5001030, preserving the packageProductDependencies declaration and the separate
cmuxTests linkage.
In
`@Packages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swift`:
- Around line 19-35: Update DiffCommentPayload.json(_:) to serialize the
optional consumedAt lifecycle field, then remove the list-only consumedAt
mapping in
Packages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swift
lines 49-52 so the shared mapper owns the complete contract. In
Sources/Panels/DiffCommentsBridge.swift lines 170-171, continue using the shared
mapper without adding separate lifecycle-state handling.
🪄 Autofix
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 Plus
Run ID: 79780be1-e19e-4f9d-a960-093429f515c2
📒 Files selected for processing (15)
CLI/CMUXCLI+CommandSuggestions.swiftCLI/CMUXCLI+Comments.swiftCLI/cmux.swiftPackages/macOS/CmuxDiffComments/Package.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffComment.swiftPackages/macOS/CmuxDiffComments/Sources/CmuxDiffComments/DiffCommentPayload.swiftResources/Localizable.xcstringsSources/DiffCommentStore.swiftSources/Panels/DiffCommentsBridge.swiftSources/TerminalController+CommentsCommands.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CommentsListPayloadTests.swiftcmuxTests/DiffCommentStoreTests.swiftdocs/cli-contract.md
|
State of the branch at 0d93f18, for whoever picks this up: All review threads are closed. Thirteen findings across five rounds — fixed: locale coverage and real translations, the missing Remaining gate items, both of which I believe need no code:
Happy to keep iterating if anything else surfaces. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
2 participants
Implements the CLI part of #9508.
Summary
commentscommand family with one read-only subcommand:cmux comments list [--repo <path>] [--all] [--json].comments.listhandled by the app, soDiffCommentStorestays the single authority. The store keeps a per-repoin-memory cache, so reading the store file from another process can go stale
after in-app edits; routing reads through the app avoids exposing the key
derivation and cache semantics as an external contract.
--allincludes comments alreadydelivered to an agent through a TextBox submission (
consumedAtset).--repodefaults to the git repository containing the current directory.Why
Agents that open a diff for human review (
cmux diff --unstagedbeforepublishing a doc) currently have no supported way to ask "which review comments
exist for this repo, and which are still unaddressed?" The push direction
already works (pending pool → next TextBox submission); this adds the pull
direction for agent-driven loops. Details and motivation: #9508.
Changes
9 files, +38/−1:
Sources/TerminalController+CommentsCommands.swift(new) —comments.listhandler: canonicalizes
repo_rootthroughDiffCommentStore, filtersconsumed comments unless
include_consumed, and reuses theDiffCommentsBridgeJSON mapping (plus aconsumedAtfield).Sources/TerminalController.swift— dispatch case inv2LegacyMainActorResponse(next to the other app-side handlers) +comments.listinv2Capabilities().Sources/Panels/DiffCommentsBridge.swift—commentJSONvisibilityprivate→ internal for reuse (same wire shape as the webview bridge).CLI/CMUXCLI+Comments.swift(new) —commentsnamespace:listresolvesthe git toplevel (default: cwd), calls
comments.list, prints a humansummary or
--json.CLI/cmux.swift,CLI/CMUXCLI+CommandSuggestions.swift— command dispatch,subcommandUsage, root usage line, known-command list.docs/cli-contract.md— Top-Level Commands row + no-socket help probe.cmuxTests/CommentsListPayloadTests.swift(new) — covers the reply shape.The handler delegates to a pure
commentsListPayload(comments:repoRoot: includeConsumed:)so the filtering is testable without a socket.Resources/Localizable.xcstrings—cli.comments.usage(en).cmux.xcodeproj/project.pbxproj— file wiring, normalized withscripts/normalize-pbxproj.py(scripts/check-pbxproj.shandscripts/lint-pbxproj-test-wiring.shpass).Hosting the handler app-side (rather than on
ControlCommandCoordinator) keptthe diff minimal; happy to move it onto the coordinator with a seam context if
that is preferred.
Testing
Local, on macOS 26.5 / Xcode 26.6:
scripts/reload.sh --tag spikebuilds the app and CLI: BUILD SUCCEEDED.scripts/check-pbxproj.shandscripts/lint-pbxproj-test-wiring.shpass.xcodebuild -scheme cmux-unit -only-testing:cmuxTests/CommentsListPayloadTests -only-testing:cmuxTests/DiffCommentStoreTests test: TEST SUCCEEDED,11 cases (4 new + the 7 existing store cases).
tests/test_cli_contract_help.pyagainst the built CLI: 151 help probesand 1 negative probe pass, including the new
cmux comments --help.cmux comments --helpreturns exit 0 with no socket available.cmux comments listprints the anchored comment (file, line, anchor text,message);
--jsonreturns the same records.--alladditionally lists acomment carrying
consumedAt, labelledconsumed.--repopointed at a non-git directory exits 1 with a clear message.Not run to completion: the full
cmux-unitsuite. A partial run reached632 passed / 35 failed, and every failure was in unrelated Browser suites
(theme settings, web view lifecycle, portals, remote store) — the per-suite
health that #8565 already tracks. No comments or diff test failed. Happy to
post a full-suite run if that is useful for review.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds a read-only
cmux comments listCLI backed by v2comments.listto list per-repo diff review comments. ExtractsDiffCommentand its wire mapping into theCmuxDiffCommentspackage and fixes payload integration across the webview and socket to use a shared mapper.New Features
cmux comments list [--repo <path>] [--all] [--json](alias:ls): resolves the git toplevel (default: cwd), strictly rejects unknown flags/positionals and option-like--repovalues, prints a concise summary or JSON; defaults to pending,--allincludesconsumedAt.CMUXDiffViewerLocalization(uses the enclosing app bundle); header selects correct singular/plural and uses grammar-safe plural phrasing.comments.list: canonicalizesrepo_rootand builds replies withDiffCommentPayload(one ISO8601 formatter per reply).Refactors
CmuxDiffCommentspackage exposingDiffCommentandDiffCommentPayload; adopted by the webview bridge, socket handler, CLI, and tests to keep mapping consistent and UI-free.DiffCommentPayloadinstance-based to avoid per-comment allocations; fixed integration so both the webview and socket call it consistently, with tests covering the shared lifecycle JSON (includingconsumedAt).Written for commit f9ecd31. Summary will update on new commits.
Summary by CodeRabbit
New Features
cmux comments list(alias:ls) to view repository review comments.comments.listcapability for integrations.Documentation
Tests