Repository navigation
Add offline agent notes CLI - #6485
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a new offline agent notes subsystem to the cmux CLI. A new ChangesOffline Agent Notes Feature
Sequence Diagram(s)sequenceDiagram
participant User as User / CI
participant cmux as cmux.swift router
participant FlushHandler as runOfflineNotesFlush
participant Store as OfflineAgentNotesStore
participant FS as FileSystem (~/.cmuxterm/...)
participant Socket as SocketClient
User->>cmux: cmux notes add --agent codex "some text"
cmux->>FlushHandler: runOfflineNotesCommandWithoutSocket (no socket needed)
FlushHandler->>Store: add(text, agent, ...)
Store->>FS: load() + append record + save(0600)
Store-->>User: OK note=<id>
User->>cmux: cmux notes flush --agent codex
cmux->>FlushHandler: runOfflineNotesCommand(client, ...)
FlushHandler->>Store: notes(includeFlushed: false, agent: "codex")
Store->>FS: load()
FS-->>Store: [OfflineAgentNoteRecord]
Store-->>FlushHandler: pending records
FlushHandler->>Socket: normalizeHandle (workspace/surface/window)
Socket-->>FlushHandler: resolved handles
FlushHandler->>Socket: surface.send_text(offlineNotesPrompt)
Socket-->>FlushHandler: OK
FlushHandler->>Store: markFlushed(ids:)
Store->>FS: save updated state
FlushHandler-->>User: OK flushed=N
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (20 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. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit eab7475. Configure here.
| let sfId = try normalizeSurfaceHandle(surfaceArg, client: client, workspaceHandle: wsId, windowHandle: winId) | ||
| if let sfId { params["surface_id"] = sfId } | ||
| _ = try client.sendV2(method: "surface.send_text", params: params) | ||
| try store.markFlushed(ids: Set(notes.map(\.id))) |
There was a problem hiding this comment.
Flush marks notes after send
Medium Severity
notes flush calls surface.send_text before persisting flushedAt. If markFlushed/save fails after a successful send, the CLI exits with an error while notes stay pending, so a retry delivers the same bundled prompt to the agent again.
Reviewed by Cursor Bugbot for commit eab7475. Configure here.
| ) | ||
| var state = try load() | ||
| state.notes.append(note) | ||
| try save(state) |
There was a problem hiding this comment.
Concurrent note writes lose data
Medium Severity
OfflineAgentNotesStore updates use load-modify-save on a shared JSON file with no locking. Overlapping notes add, flush, or clear processes can overwrite each other’s changes, dropping notes or leaving flush state inconsistent.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit eab7475. Configure here.
Greptile SummaryAdds
Confidence Score: 4/5Safe to merge after clarifying the The new CLI/CMUXCLI+OfflineAgentNotes.swift — specifically the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[cmux notes subcommand] --> B{needs socket?}
B -- "add / list / clear / path" --> C[runOfflineNotesCommandWithoutSocket]
B -- "flush" --> D[Connect to cmux socket]
C --> E[OfflineAgentNotesStore]
E --> F[(~/.cmuxterm/offline-agent-notes.json)]
D --> G[runOfflineNotesFlush]
G --> H{--dry-run?}
H -- yes --> I[Print prompt to stdout]
H -- no --> J[normalizeWindow/Workspace/Surface handles]
J --> K[surface.send_text RPC]
K --> L[store.markFlushed]
L --> F
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[cmux notes subcommand] --> B{needs socket?}
B -- "add / list / clear / path" --> C[runOfflineNotesCommandWithoutSocket]
B -- "flush" --> D[Connect to cmux socket]
C --> E[OfflineAgentNotesStore]
E --> F[(~/.cmuxterm/offline-agent-notes.json)]
D --> G[runOfflineNotesFlush]
G --> H{--dry-run?}
H -- yes --> I[Print prompt to stdout]
H -- no --> J[normalizeWindow/Workspace/Surface handles]
J --> K[surface.send_text RPC]
K --> L[store.markFlushed]
L --> F
Reviews (2): Last reviewed commit: "Add offline agent notes CLI" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func add( | ||
| text: String, | ||
| agent: String?, | ||
| cwd: String?, | ||
| workspaceId: String?, | ||
| surfaceId: String?, | ||
| now: Date = Date() | ||
| ) throws -> OfflineAgentNoteRecord { | ||
| let note = OfflineAgentNoteRecord( | ||
| id: UUID().uuidString.lowercased(), | ||
| text: text, | ||
| agent: Self.nonEmpty(agent), | ||
| cwd: Self.nonEmpty(cwd), | ||
| workspaceId: Self.nonEmpty(workspaceId), | ||
| surfaceId: Self.nonEmpty(surfaceId), | ||
| createdAt: now.timeIntervalSince1970, | ||
| flushedAt: nil | ||
| ) | ||
| var state = try load() | ||
| state.notes.append(note) | ||
| try save(state) | ||
| return note |
There was a problem hiding this comment.
add(), markFlushed(), and clear() all follow a load → modify → save pattern with no file lock. When two cmux notes add calls overlap (e.g., two agents queueing notes at the same time), both processes read the same on-disk state, each appends its note to its own in-memory copy, and the second .atomic write silently discards the first write's note. The caller gets an "OK" response with no indication of loss. For an offline notes tool that multiple agents may hit concurrently during connectivity gaps, this is a silent data-loss race.
The standard fix for a CLI that shares a JSON file across processes is to acquire an exclusive POSIX file lock (flock on the file descriptor) around every read-modify-write cycle in load()/save(), or to delegate storage to a channel that serializes writes (e.g., an append-only approach).
| let note = try store.add( | ||
| text: text, | ||
| agent: agent ?? environment["CMUX_AGENT_LAUNCH_KIND"], | ||
| cwd: cwd ?? environment["PWD"], | ||
| workspaceId: workspace ?? environment["CMUX_WORKSPACE_ID"], | ||
| surfaceId: surface ?? environment["CMUX_SURFACE_ID"] | ||
| ) | ||
| let pendingCount = try store.notes().count |
There was a problem hiding this comment.
load() immediately after add(): store.notes().count triggers a second disk read just to count pending notes. The add() method already saves the updated state; returning the pending count from add() directly, or having runOfflineNotesAdd count from the returned state, avoids one extra round-trip to disk per invocation.
| let note = try store.add( | |
| text: text, | |
| agent: agent ?? environment["CMUX_AGENT_LAUNCH_KIND"], | |
| cwd: cwd ?? environment["PWD"], | |
| workspaceId: workspace ?? environment["CMUX_WORKSPACE_ID"], | |
| surfaceId: surface ?? environment["CMUX_SURFACE_ID"] | |
| ) | |
| let pendingCount = try store.notes().count | |
| let note = try store.add( | |
| text: text, | |
| agent: agent ?? environment["CMUX_AGENT_LAUNCH_KIND"], | |
| cwd: cwd ?? environment["PWD"], | |
| workspaceId: workspace ?? environment["CMUX_WORKSPACE_ID"], | |
| surfaceId: surface ?? environment["CMUX_SURFACE_ID"] | |
| ) | |
| // Derive count from the note just added rather than re-loading the file | |
| let pendingCount = (try store.notes()).count |
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!
![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`+OfflineAgentNotes.swift:
- Around line 60-63: Add cross-process locking to prevent race conditions in the
read-modify-write operations across the methods `add`, `clear`, and
`markFlushed`. Each of these methods currently performs a load-modify-save
sequence without holding an exclusive lock, which allows concurrent invocations
to lose updates through last-writer-wins conflicts. Implement an exclusive lock
mechanism (such as file-based locking or a lock file) that is acquired before
calling `load()` and held until after `save()` completes in all three methods.
This ensures that only one process can be modifying the notes state at a time.
- Around line 66-79: The notes function in notes(includeFlushed:agent:) performs
multiple full passes over the collection with separate filter and sort
operations, which is inefficient in this hot path. Combine the two filter
operations into a single pass by merging the includeFlushed and agent filtering
logic together, and delay or conditionally apply the sort operation only when
the sorted result is actually needed. Additionally, review the call site in
runOfflineNotesAdd where notes().count is invoked and refactor it to use a
dedicated method that counts filtered notes without performing the expensive
sort operation, following the coding guidelines to avoid repeated full scans
over unbounded collections.
- Around line 119-122: The two setAttributes calls are using try? which silently
ignores any permission-setting failures, potentially leaving the directory and
file with weaker permissions than intended while reporting success to the
caller. Replace both try? with try on the setAttributes calls for directory.path
and storeURL.path to ensure permission-hardening errors are properly propagated
up the call stack rather than being silently swallowed.
In `@cmuxTests/CMUXOpenCommandTests.swift`:
- Line 309: The XCTAssertNotNil assertion in the test only validates that the
first note in allNotes has the flushed_at field populated, which can miss
partial flush-marking bugs where some notes lack this field. Update the
assertion to iterate through all items in the allNotes collection and verify
that each one has a non-nil flushed_at value (as a Double) rather than just
checking the first element with the allNotes.first accessor.
- Line 226: The current assertion at line 226 in CMUXOpenCommandTests.swift uses
optional casting with XCTAssertNil which passes when the key is missing, masking
payload-shape regressions. Instead of relying on the cast failure, explicitly
assert that the flushed_at key exists in the notes.first dictionary and its
value is NSNull (representing JSON null). This ensures the test validates the
actual payload structure rather than just a missing or null value.
🪄 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: dd89cea4-1477-40ed-afa0-796a3e16e2ee
📒 Files selected for processing (4)
CLI/CMUXCLI+OfflineAgentNotes.swiftCLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CMUXOpenCommandTests.swift
| var state = try load() | ||
| state.notes.append(note) | ||
| try save(state) | ||
| return note |
There was a problem hiding this comment.
Add cross-process locking around read-modify-write operations.
add, clear, and markFlushed each do load() then save() without an exclusive lock, so parallel cmux notes invocations can drop updates via last-writer-wins.
💡 Suggested direction
+// Introduce an exclusive lock file (e.g. "<store>.lock") and wrap all
+// load-modify-save sequences in one critical section.
+// add(...)
+// clear(...)
+// markFlushed(...)Also applies to: 84-96, 99-106, 108-123
🤖 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`+OfflineAgentNotes.swift around lines 60 - 63, Add cross-process
locking to prevent race conditions in the read-modify-write operations across
the methods `add`, `clear`, and `markFlushed`. Each of these methods currently
performs a load-modify-save sequence without holding an exclusive lock, which
allows concurrent invocations to lose updates through last-writer-wins
conflicts. Implement an exclusive lock mechanism (such as file-based locking or
a lock file) that is acquired before calling `load()` and held until after
`save()` completes in all three methods. This ensures that only one process can
be modifying the notes state at a time.
| func notes(includeFlushed: Bool = false, agent: String? = nil) throws -> [OfflineAgentNoteRecord] { | ||
| let normalizedAgent = Self.nonEmpty(agent)?.lowercased() | ||
| return try load().notes | ||
| .filter { includeFlushed || $0.flushedAt == nil } | ||
| .filter { note in | ||
| guard let normalizedAgent else { return true } | ||
| return note.agent?.lowercased() == normalizedAgent | ||
| } | ||
| .sorted { lhs, rhs in | ||
| if lhs.createdAt == rhs.createdAt { | ||
| return lhs.id < rhs.id | ||
| } | ||
| return lhs.createdAt < rhs.createdAt | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Collapse repeated collection passes in the offline notes hot path.
notes() currently performs multiple full passes (filter + filter + sorted), and runOfflineNotesAdd calls notes().count, forcing an unnecessary sort for a count-only operation.
As per coding guidelines, this path should avoid “repeated full scans” and “multiple filter/sorted passes” over unbounded user-owned notes.
Also applies to: 218-218
🤖 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`+OfflineAgentNotes.swift around lines 66 - 79, The notes function
in notes(includeFlushed:agent:) performs multiple full passes over the
collection with separate filter and sort operations, which is inefficient in
this hot path. Combine the two filter operations into a single pass by merging
the includeFlushed and agent filtering logic together, and delay or
conditionally apply the sort operation only when the sorted result is actually
needed. Additionally, review the call site in runOfflineNotesAdd where
notes().count is invoked and refactor it to use a dedicated method that counts
filtered notes without performing the expensive sort operation, following the
coding guidelines to avoid repeated full scans over unbounded collections.
Source: Coding guidelines
| try? fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path) | ||
| let data = try encoder.encode(state) | ||
| try data.write(to: storeURL, options: .atomic) | ||
| try? fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: storeURL.path) |
There was a problem hiding this comment.
Do not ignore permission-hardening failures.
Swallowing errors from setAttributes can leave directory/file permissions weaker than intended while reporting success.
🔧 Proposed fix
- try? fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path)
+ try fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path)
@@
- try? fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: storeURL.path)
+ try fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: storeURL.path)📝 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.
| try? fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path) | |
| let data = try encoder.encode(state) | |
| try data.write(to: storeURL, options: .atomic) | |
| try? fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: storeURL.path) | |
| try fileManager.setAttributes([.posixPermissions: 0o700], ofItemAtPath: directory.path) | |
| let data = try encoder.encode(state) | |
| try data.write(to: storeURL, options: .atomic) | |
| try fileManager.setAttributes([.posixPermissions: 0o600], ofItemAtPath: storeURL.path) |
🤖 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`+OfflineAgentNotes.swift around lines 119 - 122, The two
setAttributes calls are using try? which silently ignores any permission-setting
failures, potentially leaving the directory and file with weaker permissions
than intended while reporting success to the caller. Replace both try? with try
on the setAttributes calls for directory.path and storeURL.path to ensure
permission-hardening errors are properly propagated up the call stack rather
than being silently swallowed.
| XCTAssertEqual(notes.count, 1) | ||
| XCTAssertEqual(notes.first?["agent"] as? String, "codex") | ||
| XCTAssertEqual(notes.first?["text"] as? String, "Check the release notes when online") | ||
| XCTAssertNil(notes.first?["flushed_at"] as? Double) |
There was a problem hiding this comment.
Assert explicit JSON null for flushed_at instead of relying on cast failure.
XCTAssertNil(notes.first?["flushed_at"] as? Double) also passes when the key is missing, so this can hide payload-shape regressions.
Suggested test hardening
- XCTAssertNil(notes.first?["flushed_at"] as? Double)
+ let first = try XCTUnwrap(notes.first)
+ XCTAssertTrue(first.keys.contains("flushed_at"))
+ XCTAssertTrue(first["flushed_at"] is NSNull)📝 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.
| XCTAssertNil(notes.first?["flushed_at"] as? Double) | |
| let first = try XCTUnwrap(notes.first) | |
| XCTAssertTrue(first.keys.contains("flushed_at")) | |
| XCTAssertTrue(first["flushed_at"] is NSNull) |
🤖 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 `@cmuxTests/CMUXOpenCommandTests.swift` at line 226, The current assertion at
line 226 in CMUXOpenCommandTests.swift uses optional casting with XCTAssertNil
which passes when the key is missing, masking payload-shape regressions. Instead
of relying on the cast failure, explicitly assert that the flushed_at key exists
in the notes.first dictionary and its value is NSNull (representing JSON null).
This ensures the test validates the actual payload structure rather than just a
missing or null value.
| let allPayload = try XCTUnwrap(Self.jsonObject(from: all.stdout)) | ||
| let allNotes = try XCTUnwrap(allPayload["notes"] as? [[String: Any]]) | ||
| XCTAssertEqual(allNotes.count, 2) | ||
| XCTAssertNotNil(allNotes.first?["flushed_at"] as? Double) |
There was a problem hiding this comment.
Verify all flushed notes have flushed_at populated.
The assertion only checks the first item, so the test can miss partial flush-marking bugs.
Suggested assertion update
- XCTAssertNotNil(allNotes.first?["flushed_at"] as? Double)
+ XCTAssertTrue(allNotes.allSatisfy { ($0["flushed_at"] as? Double) != nil }, "\(allNotes)")🤖 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 `@cmuxTests/CMUXOpenCommandTests.swift` at line 309, The XCTAssertNotNil
assertion in the test only validates that the first note in allNotes has the
flushed_at field populated, which can miss partial flush-marking bugs where some
notes lack this field. Update the assertion to iterate through all items in the
allNotes collection and verify that each one has a non-nil flushed_at value (as
a Double) rather than just checking the first element with the allNotes.first
accessor.
eab7475 to
42ce1e5
Compare
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 `@CLI/cmux.swift`:
- Line 34187: The notes command synopsis at line 34187 omits several available
flags (--cwd, --all, --dry-run) that are documented in detailed help and are
shown in other command synopses like tree, top, and memory. Update the notes
command synopsis to include all possible flags for consistency, or alternatively
simplify it to use [options] as a placeholder. Match the style used by other
commands in the help listing to ensure uniform documentation format.
In `@CLI/CMUXCLI`+OfflineAgentNotes.swift:
- Around line 335-336: The sequence of calling client.sendV2 followed by
store.markFlushed is not atomic and may cause duplicate deliveries if sendV2
succeeds but markFlushed fails. To fix this, wrap the markFlushed call in a
do-catch block to handle potential failures (such as disk full or permission
errors), emit a warning log when markFlushed fails while still treating the send
as partially successful, and consider pre-writing a "flushing" state to the
store before calling sendV2 so that failed flushes can be recovered on retry
without duplication.
🪄 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: eabe26b9-1fd7-4ac4-ba00-93ef9be4cbdc
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
CLI/CMUXCLI+OfflineAgentNotes.swiftCLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CMUXOfflineAgentNotesCommandTests.swift
| tree [--all] [--workspace <id|ref|index>] [--window <id|ref|index>] | ||
| top [--all] [--workspace <id|ref|index>] [--window <id|ref|index>] [--processes] [--sort <cpu|mem|proc>] [--flat] [--format <tree|tsv>] | ||
| memory [--all] [--workspace <id|ref|index>] [--groups <count>] | ||
| notes <add|list|flush|clear|path> [--agent <name>] [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider listing all subcommand flags for consistency.
The one-line synopsis omits several flags that are available in the detailed help: --cwd (used by add), --all (used by list/clear/flush), and --dry-run (used by flush). Other commands in this help listing (tree, top, memory) show all their possible options. For consistency, consider:
- notes <add|list|flush|clear|path> [--agent <name>] [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>]
+ notes <add|list|flush|clear|path> [--agent <name>] [--cwd <path>] [--workspace <id|ref|index>] [--surface <id|ref|index>] [--window <id|ref|index>] [--all] [--dry-run]Alternatively, simplify to notes <add|list|flush|clear|path> [options] if the full flag list is too verbose. Detailed help is available via cmux notes --help, so this is a minor UX improvement rather than a functional issue.
🤖 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/cmux.swift` at line 34187, The notes command synopsis at line 34187 omits
several available flags (--cwd, --all, --dry-run) that are documented in
detailed help and are shown in other command synopses like tree, top, and
memory. Update the notes command synopsis to include all possible flags for
consistency, or alternatively simplify it to use [options] as a placeholder.
Match the style used by other commands in the help listing to ensure uniform
documentation format.
| _ = try client.sendV2(method: "surface.send_text", params: params) | ||
| try store.markFlushed(ids: Set(notes.map(\.id))) |
There was a problem hiding this comment.
Non-atomic send and mark sequence may cause duplicate delivery on partial failure.
If sendV2 succeeds but markFlushed throws (e.g., disk full, permission issue), notes are delivered to the terminal but remain marked as pending. A subsequent retry would re-send the same notes.
Consider catching markFlushed errors and emitting a warning while still reporting partial success, or pre-writing a "flushing" state before send:
💡 Suggested direction
_ = try client.sendV2(method: "surface.send_text", params: params)
- try store.markFlushed(ids: Set(notes.map(\.id)))
+ do {
+ try store.markFlushed(ids: Set(notes.map(\.id)))
+ } catch {
+ // Notes were delivered but local state update failed.
+ // Warn user but don't fail the command to avoid confusion.
+ fputs("warning: notes delivered but failed to update local state: \(String(describing: error))\n", stderr)
+ }🤖 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`+OfflineAgentNotes.swift around lines 335 - 336, The sequence of
calling client.sendV2 followed by store.markFlushed is not atomic and may cause
duplicate deliveries if sendV2 succeeds but markFlushed fails. To fix this, wrap
the markFlushed call in a do-catch block to handle potential failures (such as
disk full or permission errors), emit a warning log when markFlushed fails while
still treating the send as partially successful, and consider pre-writing a
"flushing" state to the store before calling sendV2 so that failed flushes can
be recovered on retry without duplication.
|
Closing this because #6485 changed the desktop CLI/runtime, but the requested feature is iOS-only. I am moving the work to an iOS-scoped branch. |
This branch was successfully deployed
1 participant
Summary
cmux notesfor storing offline agent notes without a cmux socket.cmux notes flushto submit pending notes into an agent terminal once cmux is available.Tests
python3 scripts/normalize-pbxproj.py && scripts/check-pbxproj.shpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvpython3 scripts/check-test-determinism.py --strict./tests/test_ci_pbxproj_test_wiring.shCMUX_ALLOW_LOCAL_XCODEBUILD=1 xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-offnot-tests -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testOfflineNotesAddAndListWorkWithoutSocket -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testOfflineNotesFlushSubmitsPendingNotesAndMarksThemFlushed testDogfood
offnot