feat(sharecli): Ghostty terminal control plane (18 commits) - #647
feat(sharecli): Ghostty terminal control plane (18 commits)#647KooshaPari wants to merge 18 commits into
Conversation
…nd imports The platform::mount_in_session body referenced smoke_fuser_config_for_backend and select_backend without importing them, breaking cargo build -p sharecli-fuse. This blocked the Swift tray from rebuilding (libsharecli_ffi.dylib didn't build because sharecli-fuse was a dep). Fix: import FuseBackend + select_backend from crate root in session_registry.rs (the helper that constructs the smoke fuser config), and gate the kernel→fskit fallback path on macOS only since select_backend is macOS-specific. Also stashes the external-p0-collaboration-snapshot-20260729 and external-p0-librs-snapshot-20260729 stashes that were on disk. Verification: - cargo check -p sharecli-fuse ✓ clean - cargo check -p sharecli ✓ clean (was broken before) - cargo build -p sharecli-ffi ✓ produces libsharecli_ffi.dylib - cargo test -p sharecli-ipc --lib ✓ 38 passed - swift build -c release ✓ 1.82s - /Applications/ShareCLITray.app ✓ 5s smoke test, no crash, no dyld
…format helpers
Pure-deletion cleanup of ProcessesPage.swift (commit-only path due to
disk-full block; build verification deferred):
P1-4 (ProcessTableView reference removal):
- Doc comment now reflects the actual 8-subpage surface (All / By Project /
By Harness / Tree / Trends / Resources / Spawn / Presets) rather than
referencing the long-deleted bare ProcessTableView shim that shipped
in early PR 2. Also documents the full set of fields carried on
ProcessSummary after the sidecar extensions (start_time, cpu_percent,
ppid, cwd, env_count, state, disk_*_bytes, fd_count, thread_count).
P1-5 (format helper dedup):
- Removed two duplicate helper functions from ResourcesView:
* formatStart(_ ts: UInt64) — exact duplicate of the canonical
formatStart in AllProcessesView
* formatAge(_ ts: UInt64) — exact duplicate of the canonical
formatAge in AllProcessesView (same logic, simpler Xh format)
- Both canonical definitions remain in AllProcessesView and are
accessible from ResourcesView (they're file-scoped but the duplication
was identical implementations, so the canonical versions are the
single source of truth).
Net: -16 lines, +5 lines. No behavior change — calls to formatStart /
formatAge in ResourcesView will resolve to the file-scoped canonical
definitions (which are identical).
Verifies: changes are pure deletions of duplicate code; no functional
differences. Cargo check -p sharecli + swift build queued but blocked
on disk-full (351 MiB free; target/ + .build/ = 6.7 GB recreatable).
Related: PR #621 (merged), 92a5a18 (P0 fd_count/thread_count/disk_io),
97179a1 (FUSE fix).
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughSummaryAdds a Ghostty terminal control plane with:
Must Fix
Should Fix
Consider
Approve / Request ChangesRequest changes. Resolve the inter-crate dependency violation and provide successful formatting, clippy, and workspace test results. WalkthroughThis PR adds a ShareCLI Ghostty control plane with durable session observations, recovery, layouts, authenticated surface RPC, live events, FUSE probing, CLI and IPC commands, and a Swift macOS bridge. ChangesSession and control plane
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
codeant-ai
Bot
added
the
size:XXL
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| public func send(surfaceID: String, bytes: [UInt8]) async throws { | ||
| try await binding(for: surfaceID).send(bytes) | ||
| } |
There was a problem hiding this comment.
Suggestion: Looking up the binding before awaiting its operation copies the binding and lets the operation continue after unregister or replace removes the surface. A teardown can therefore send, read, or resize a stale Ghostty surface instead of returning the documented unavailable-provider error. Revalidate the binding after the asynchronous hop or coordinate unregister/teardown with in-flight operations. [stale reference]
Severity Level: Major ⚠️
- ❌ Surface teardown can receive operations after removal.
- ⚠️ Recovery and live control may target stale surfaces.
- ⚠️ JSON-RPC clients receive inconsistent availability results.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** contrib/ghostty-control/Sources/ShareCLIGhosttyControl/SurfaceProvider.swift
**Line:** 118:120
**Comment:**
*Stale Reference: Looking up the binding before awaiting its operation copies the binding and lets the operation continue after `unregister` or `replace` removes the surface. A teardown can therefore send, read, or resize a stale Ghostty surface instead of returning the documented unavailable-provider error. Revalidate the binding after the asynchronous hop or coordinate unregister/teardown with in-flight operations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| match Command::new(&session.resume.argv[0]) | ||
| .args(&session.resume.argv[1..]) | ||
| .current_dir(&session.resume.cwd) | ||
| .spawn() | ||
| { | ||
| Ok(_) => RecoveryOutcome::Resumed, | ||
| Err(error) => RecoveryOutcome::LaunchFailed(error.to_string()), | ||
| } |
There was a problem hiding this comment.
Suggestion: Recovery executes the persisted first argv element directly after validating only for NUL bytes and an absolute working directory. An exact-confidence AgentSession can be materialized from durable observation input with an arbitrary resume.argv, allowing recovery to launch an unintended executable. Reconstruct the command from the known harness and session ID, or validate the recipe against a strict allowlist before spawning. [security]
Severity Level: Major ⚠️
- ❌ Recovery can execute unintended local programs.
- ⚠️ Imported observations bypass known-harness recipes.
- ⚠️ `recovery.execute` trusts durable command metadata.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/recovery.rs
**Line:** 87:94
**Comment:**
*Security: Recovery executes the persisted first argv element directly after validating only for NUL bytes and an absolute working directory. An exact-confidence `AgentSession` can be materialized from durable observation input with an arbitrary `resume.argv`, allowing recovery to launch an unintended executable. Reconstruct the command from the known harness and session ID, or validate the recipe against a strict allowlist before spawning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| default: | ||
| throw ControlError.methodNotFound(method) |
There was a problem hiding this comment.
Suggestion: The native dispatcher has no cases for surface.layout.snapshot or surface.layout.restore, although the Rust client and CLI send both methods. Those commands will always receive methodNotFound from the newly added Ghostty control server. Implement the layout methods through the provider or remove the client methods until the native protocol supports them. [incomplete implementation]
Severity Level: Critical 🚨
- ❌ Layout snapshot CLI command always fails against Swift server.
- ❌ Layout restore CLI command always fails against Swift server.
- ⚠️ Native layout control is unavailable despite advertised client methods.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift
**Line:** 238:239
**Comment:**
*Incomplete Implementation: The native dispatcher has no cases for `surface.layout.snapshot` or `surface.layout.restore`, although the Rust client and CLI send both methods. Those commands will always receive `methodNotFound` from the newly added Ghostty control server. Implement the layout methods through the provider or remove the client methods until the native protocol supports them.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| var buffer = Data() | ||
| var chunk = [UInt8](repeating: 0, count: 16 * 1024) | ||
| while true { | ||
| let count = chunk.withUnsafeMutableBytes { Darwin.recv(fd, $0.baseAddress, $0.count, 0) } |
There was a problem hiding this comment.
Suggestion: Each accepted connection is handled in a detached task that performs blocking recv calls, and live-event delivery performs blocking send loops. An idle client consumes an executor task indefinitely, while a client that stops reading can block the writer and prevent responses and events on that connection from progressing. Use nonblocking/asynchronous socket I/O with cancellation and write backpressure, and impose connection/resource limits so stalled clients cannot starve the control plane. [performance]
Severity Level: Major ⚠️
- ⚠️ Idle clients consume detached tasks indefinitely.
- ⚠️ Stalled subscribers retain blocked writer tasks.
- ❌ Sufficient same-user clients can impair Ghostty control availability.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift
**Line:** 106:106
**Comment:**
*Performance: Each accepted connection is handled in a detached task that performs blocking `recv` calls, and live-event delivery performs blocking `send` loops. An idle client consumes an executor task indefinitely, while a client that stops reading can block the writer and prevent responses and events on that connection from progressing. Use nonblocking/asynchronous socket I/O with cancellation and write backpressure, and impose connection/resource limits so stalled clients cannot starve the control plane.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| params: SurfaceEventParams { | ||
| subscription_id: event.params.subscription_id, | ||
| surface_id: event.params.surface_id.clone(), | ||
| seq: event.params.seq.saturating_sub(1), |
There was a problem hiding this comment.
Suggestion: The dropped marker uses event.params.seq - 1, which is the predecessor of the newly arriving event rather than the sequence of the event that was removed. For example, when a full queue drops sequence 1 while processing sequence 3, the marker reports sequence 2; with capacity one it can even report zero. Consumers cannot use the marker sequence to identify the missing range, and this differs from the native Swift stream, which reports overflow metadata on the actual subsequent event. Track and report the first missing sequence or otherwise define a consistent resynchronization boundary. [incorrect variable usage]
Severity Level: Major ⚠️
- ⚠️ Rust subscribers receive inaccurate overflow sequence metadata.
- ⚠️ Resynchronization clients cannot identify dropped boundaries reliably.
- ⚠️ Sequence handling diverges from Swift event metadata semantics.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/events.rs
**Line:** 252:252
**Comment:**
*Incorrect Variable Usage: The dropped marker uses `event.params.seq - 1`, which is the predecessor of the newly arriving event rather than the sequence of the event that was removed. For example, when a full queue drops sequence 1 while processing sequence 3, the marker reports sequence 2; with capacity one it can even report zero. Consumers cannot use the marker sequence to identify the missing range, and this differs from the native Swift stream, which reports overflow metadata on the actual subsequent event. Track and report the first missing sequence or otherwise define a consistent resynchronization boundary.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if subscription.queue_capacity == 1 { | ||
| subscription.queue.clear(); | ||
| subscription.queue.push_back(marker); | ||
| } else { |
There was a problem hiding this comment.
Suggestion: With queue_capacity == 1, every overflow removes the queued event and enqueues only the Dropped marker, discarding the newest actual event as well. Consequently, a subscriber using the smallest valid queue capacity never receives live output after its first queued event; it receives only resync markers. Retain the newest event alongside an appropriate overflow indication, or reject a capacity of one if that behavior is not supported. [logic error]
Severity Level: Major ⚠️
- ❌ Capacity-one `surface watch` loses every event after overflow.
- ⚠️ Live terminal output is replaced by resync markers.
- ⚠️ Watchers cannot observe the newest available event.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/events.rs
**Line:** 260:263
**Comment:**
*Logic Error: With `queue_capacity == 1`, every overflow removes the queued event and enqueues only the `Dropped` marker, discarding the newest actual event as well. Consequently, a subscriber using the smallest valid queue capacity never receives live output after its first queued event; it receives only resync markers. Retain the newest event alongside an appropriate overflow indication, or reject a capacity of one if that behavior is not supported.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| { | ||
| let mut stream = std::os::unix::net::UnixStream::connect(&self.socket)?; | ||
| let id = REQUEST_ID.fetch_add(1, Ordering::Relaxed); | ||
| let mut request = json!({"id": id, "method": method, "params": params}); |
There was a problem hiding this comment.
Suggestion: The ordinary RPC client omits the required jsonrpc: "2.0" member. The Swift dispatcher rejects these requests before dispatching them, so surface.list, send, read, resize, capabilities, and layout calls fail whenever this client talks to the native Ghostty control server. Add the JSON-RPC version field to the request constructed here, as is already done by subscribe_surface. [api mismatch]
Severity Level: Critical 🚨
- ❌ Surface list requests fail against the Swift server.
- ❌ Send, read, resize, and capabilities commands fail.
- ❌ Layout snapshot and restore requests fail before dispatch.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/session.rs
**Line:** 213:213
**Comment:**
*Api Mismatch: The ordinary RPC client omits the required `jsonrpc: "2.0"` member. The Swift dispatcher rejects these requests before dispatching them, so `surface.list`, send, read, resize, capabilities, and layout calls fail whenever this client talks to the native Ghostty control server. Add the JSON-RPC version field to the request constructed here, as is already done by `subscribe_surface`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| let state_id = state | ||
| .session_id(&surface, harness) | ||
| .with_context(|| format!("read session state for surface {}", surface.id))?; |
There was a problem hiding this comment.
Suggestion: The state-provider lookup uses ?, so one malformed or unreadable sidecar aborts the entire scan even though the report is documented to isolate per-surface failures. A bad state record for one surface prevents unaffected surfaces from being recorded. Convert this failure into a DiscoveryFailure and continue scanning. [error handling]
Severity Level: Major ⚠️
- ❌ One malformed sidecar prevents recording unaffected surfaces.
- ⚠️ Session watch repeatedly reports degraded scans.
- ⚠️ Durable observations become incomplete during sidecar failures.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/discovery.rs
**Line:** 140:142
**Comment:**
*Error Handling: The state-provider lookup uses `?`, so one malformed or unreadable sidecar aborts the entire scan even though the report is documented to isolate per-surface failures. A bad state record for one surface prevents unaffected surfaces from being recorded. Convert this failure into a `DiscoveryFailure` and continue scanning.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if let Some(session) = &observation.session { | ||
| Self::upsert_locked(&tx, session)?; | ||
| } |
There was a problem hiding this comment.
Suggestion: An observation with no resolved session is appended without changing the materialized sessions row. If a surface previously produced a resumable session and a later scan loses its evidence, session.list and recovery.plan continue returning the stale session and may launch it during recovery. Clear or invalidate the materialized session when an observation explicitly has no session. [stale reference]
Severity Level: Major ⚠️
- ⚠️ Recovery plan retains sessions lacking current evidence.
- ❌ Recovery may relaunch stale agent sessions.
- ⚠️ Session listing diverges from latest observations.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/lib.rs
**Line:** 283:285
**Comment:**
*Stale Reference: An observation with no resolved session is appended without changing the materialized `sessions` row. If a surface previously produced a resumable session and a later scan loses its evidence, `session.list` and `recovery.plan` continue returning the stale session and may launch it during recovery. Clear or invalidate the materialized session when an observation explicitly has no session.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if request.jsonrpc != JSON_RPC_VERSION { | ||
| return surface_error( | ||
| request.id.unwrap_or(Value::Null), | ||
| -32600, | ||
| "jsonrpc must be \"2.0\"".to_string(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The new strict JSON-RPC version check rejects every request from the existing GhosttyControlClient, whose request envelope contains id, method, and params but does not include a jsonrpc field. Consequently all surface CLI operations receive an invalid-request response until the client adds the version field or the server preserves compatibility. [api mismatch]
Severity Level: Critical 🚨
- ❌ Surface send, read, resize, and discovery fail.
- ❌ Session watch cannot discover Ghostty surfaces.
- ⚠️ Native control socket becomes unusable to ShareCLI.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sharecli-session/src/rpc.rs
**Line:** 188:194
**Comment:**
*Api Mismatch: The new strict JSON-RPC version check rejects every request from the existing `GhosttyControlClient`, whose request envelope contains `id`, `method`, and `params` but does not include a `jsonrpc` field. Consequently all surface CLI operations receive an invalid-request response until the client adds the version field or the server preserves compatibility.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let fd_count = count_open_fds(pid.as_u32()); | ||
| let thread_count = count_threads(pid.as_u32()); |
There was a problem hiding this comment.
Suggestion: These probes now run for every managed process whenever ProcessInfo::from_sysinfo is called. On macOS and other non-Linux Unix systems this spawns lsof for each process, and the IPC monitoring paths independently probe descriptors again after listing processes, making routine refresh and monitoring operations scale with multiple external process launches per managed process. Move these probes out of the per-process refresh path or cache/reuse their results. [performance]
Severity Level: Major ⚠️
- ⚠️ macOS process listings spawn probes per managed process.
- ⚠️ Repeated status refreshes accumulate external-command latency.
- ⚠️ Large managed pools increase monitoring overhead.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/runtime.rs
**Line:** 169:170
**Comment:**
*Performance: These probes now run for every managed process whenever `ProcessInfo::from_sysinfo` is called. On macOS and other non-Linux Unix systems this spawns `lsof` for each process, and the IPC monitoring paths independently probe descriptors again after listing processes, making routine refresh and monitoring operations scale with multiple external process launches per managed process. Move these probes out of the per-process refresh path or cache/reuse their results.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| var continuation: AsyncStream<LiveIOEvent>.Continuation! | ||
| let stream = AsyncStream<LiveIOEvent>(bufferingPolicy: .bufferingNewest(queueCapacity)) { | ||
| continuation = $0 | ||
| } | ||
| subscriptions[id] = State( | ||
| surfaceID: surfaceID, | ||
| fromSequence: startingSequence, | ||
| maxChunkBytes: maxChunkBytes, | ||
| queueCapacity: queueCapacity, | ||
| continuation: continuation, | ||
| stream: stream |
There was a problem hiding this comment.
Suggestion: The subscription is retained in subscriptions indefinitely after its AsyncStream consumer is cancelled or released. Because no onTermination handler removes the subscription, every abandoned client remains in the actor state and continues to be considered during publishing, causing unbounded subscription-state growth. Register stream termination to finish and remove the corresponding subscription. [resource leak]
Severity Level: Major ⚠️
- ⚠️ Disconnected control clients leave subscriptions retained.
- ⚠️ Repeated reconnects grow hub state indefinitely.
- ⚠️ Publishing scans abandoned subscriptions on every event.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift
**Line:** 110:120
**Comment:**
*Resource Leak: The subscription is retained in `subscriptions` indefinitely after its `AsyncStream` consumer is cancelled or released. Because no `onTermination` handler removes the subscription, every abandoned client remains in the actor state and continues to be considered during publishing, causing unbounded subscription-state growth. Register stream termination to finish and remove the corresponding subscription.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 62
🤖 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 `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift`:
- Around line 161-170: Update dispatch to parse the request payload once at its
start and extract the request ID plus notification status into reusable values.
Replace the repeated requestHasID(line) and requestID(from: line) calls in each
catch block with those values, while preserving the existing error codes,
messages, and notification responses.
- Around line 173-241: Update dispatch(method:params:) to handle
surface.layout.snapshot and surface.layout.restore requests sent by
src/session.rs, instead of falling through to methodNotFound. Implement the
corresponding provider-backed request/response handling using the existing
layout symbols and parameter validation conventions, or explicitly prevent these
Rust session requests from reaching the native socket if native handling is
intentionally unsupported.
In
`@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swift`:
- Around line 68-70: Update ControlLifecycle deinitialization so it does not
access the MainActor-isolated server from deinit: move socket cleanup into
UnixControlServer or use an isolated deinit, while preserving explicit
lifecycle.stop() calls before teardown.
In `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift`:
- Around line 142-148: Update the chunking logic in the subscription delivery
path to generate chunks separately for each matching subscription using that
subscription’s own maxChunkBytes. Remove the shared limits/minimum chunkSize
calculation, and ensure each subscriber receives appropriately sized chunks
without another subscriber’s limit affecting its events or sequence progression.
- Around line 111-113: Update the AsyncStream buffering setup in LiveIO’s stream
initialization to use a policy that exposes discarded-event results, or
otherwise track each overwritten event and accumulate it into state.dropped.
Ensure subscriber buffer loss is reported to the Rust client rather than only
appearing as a seq discontinuity.
In
`@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift`:
- Around line 96-108: Update serveConnection and its accepted-socket setup so
connection descriptors do not perform blocking Darwin.recv calls on the Swift
cooperative pool. Set O_NONBLOCK on each accepted descriptor, then await
readability through a DispatchSourceRead (or move the entire connection loop to
a dedicated DispatchQueue) rather than polling or spinning; preserve the
existing event-task cancellation and writer cleanup.
- Around line 62-87: Update acceptConnections to acquire lock before reading
listenerFD, copy the descriptor to a local value, then release the lock and
return immediately when that value is negative; use the local descriptor for
Darwin.accept. Keep stop’s synchronized listenerFD update unchanged.
In
`@contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swift`:
- Around line 169-181: Update the socket round-trip test around the fd setup and
response read: configure SO_RCVTIMEO on fd so Darwin.recv cannot block
indefinitely, then accumulate received bytes in a loop until the 0x0a line
terminator is observed. Build the JSON response from the complete accumulated
line while preserving the existing validation of result and surface ID.
In `@contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swift`:
- Around line 100-114: Update the invalid-input tests around
LiveIOEventHub.subscribe to use Swift Testing `#expect`(throws:) instead of empty
catch blocks and Issue.record calls, asserting invalidChunkBytes and
invalidQueueCapacity as appropriate. If LiveIOError is not Equatable, assert the
error type rather than a concrete value. Also add a zero queueCapacity case if
subscribe rejects it.
In `@crates/sharecli-fuse/build.rs`:
- Around line 9-15: Update the macOS linkage block in build.rs to inspect
CARGO_CFG_TARGET_OS at runtime rather than relying on the build-script host cfg,
and emit the MFMount framework search and link directives only when the target
OS is macOS and MFMount.framework exists at the expected location. When the
target is macOS but the framework is absent, emit a cargo:warning= message
instead of silently proceeding.
In `@crates/sharecli-fuse/src/backend.rs`:
- Around line 199-210: Update the wording around fskit_approval_requested and
related NoVerifiedBackend diagnostics to describe SHARECLI_FUSE_FSKIT_APPROVED
only as operator intent or opt-in, never as verified FSKit approval. Preserve
the existing environment-variable behavior unless a real system approval signal
is already available; if one exists, use that signal for verification instead.
- Around line 212-225: Update the macOS branch of kernel_backend_loaded to
handle Command::output errors explicitly: log the probe failure with structured
tracing, including the error details, then return false. Preserve the existing
successful-output detection and the non-macOS behavior.
In `@crates/sharecli-fuse/src/bin/fuse-runtime-probe.rs`:
- Around line 16-26: Update the report construction in the fuse runtime probe to
serialize evidence.mountpoint through Path::display, ensuring non-UTF-8
arguments are representable. Replace
serde_json::to_string_pretty(...).expect(...) with explicit serialization error
handling that returns or reports the failure without panicking, and remove the
production expect.
In `@crates/sharecli-fuse/src/session_registry.rs`:
- Around line 280-291: Update the macOS unavailable-backend error handling in
the session registry and the corresponding fail-closed path in lib.rs to avoid
using FuseBackendDiagnostic::message() when it includes fail-open wording.
Report only the diagnostic cause, then let anyhow::bail! state that mounting
failed; preserve the existing fail-open message in backend.rs.
- Around line 44-56: Ensure the backend parameter is considered used on
non-macOS builds in both default_fuser_config_for_backend and
smoke_fuser_config_for_backend, while preserving the existing macOS-specific
Fskit behavior. Add the appropriate cfg-gated use at the start of each function
so Linux builds remain warning-free with warnings denied.
In `@crates/sharecli-ipc/src/handler.rs`:
- Around line 397-444: Update the persisting routes in the request handler,
especially session.observe, layout.save, and session.compact, to emit a
structured tracing event whenever their SessionStore operation or payload
handling fails before propagating the error to the caller. Include route context
and the error using tracing’s structured fields, while preserving the existing
response behavior.
- Around line 315-328: The hard-coded session database path in Handler::new lets
tests modify real user state. In crates/sharecli-ipc/src/handler.rs:315-328, add
Handler::with_session_db(path) to open an explicit database path, retain
Handler::new as the default-path wrapper, and update existing Handler::new call
sites for its fallible result. In
crates/sharecli-ipc/tests/handler_dispatch.rs:112-168, construct both new tests
with separate TempDir database paths through the injectable constructor.
- Around line 445-456: Update the recovery route in handle so synchronous
SessionStore listing and RecoveryExecutor dry-run/execute work are moved off the
Tokio worker via the existing blocking-task mechanism, preserving their current
results and error propagation. Clamp the parsed max_parallel value to
MAX_RECOVERY_PARALLEL (32) before converting it to usize, and define that
constant alongside the other constants.
In `@crates/sharecli-session/src/adapter.rs`:
- Around line 45-61: Update GhosttySurfaceAdapter::capabilities to advertise
layout as unsupported by setting layout to false, while preserving the existing
read, write, resize, and durable_pty capability behavior. Do not add layout
transport or override methods.
In `@crates/sharecli-session/src/discovery.rs`:
- Around line 157-161: Replace the full-history emptiness check in the discovery
classification flow with a bounded existence check. Add a SessionStore method
such as has_observations(&self, surface_id: &str) backed by a SELECT 1 ... LIMIT
1 query, then use it in the kind selection so surfaces with no rows remain
Discovered and those with any observation remain Updated.
- Around line 139-169: Update the per-surface discovery loop to isolate errors
from state.session_id, store.observations, and store.append_observation instead
of propagating them with ?. Handle each failure using the existing
capability-error pattern: record a DiscoveryFailure in DiscoveryReport, emit a
structured log with surface context and the error, and continue processing
subsequent surfaces. Preserve successful observation behavior and pass
completion.
In `@crates/sharecli-session/src/events.rs`:
- Line 132: Remove the expect-based mutex acquisition in all four hub methods in
the event hub implementation. Recover a poisoned mutex guard or convert the lock
failure into the existing SurfaceEventError result, preserving each method’s
current Result-based behavior and avoiding production unwrap/expect calls.
- Around line 246-259: Align the overflow wire contract with Swift by removing
the synthetic Dropped notification in the Rust event path and propagating
dropped/resync_required on the next real event; update
crates/sharecli-session/src/events.rs:246-259 accordingly. Revise
contrib/ghostty-control/README.md:33-38 to describe this encoding, and update
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swift:38-62
plus surface_subscription_overflow_emits_bounded_resync_marker to assert the
same flags on the next real event rather than a separate marker.
- Around line 260-269: Update the queue-overflow handling around the
queue_capacity check so capacity 1 retains the newest real event instead of
replacing it with only the marker; preserve the dropped-event signaling through
the marker or existing counters. Alternatively, enforce a minimum queue_capacity
of 2 in subscribe and reject capacity 1.
- Line 209: Remove the redundant clone from the subscription_id field assignment
in the event construction, passing the copied u64 value directly from the &u64
obtained through iter_mut while preserving the existing subscription ID.
- Around line 37-42: Remove the Deserialize derive from SurfaceEventNotification
while preserving its existing Serialize implementation and other derives. Keep
the &'static str fields unchanged, making this notification type serialize-only.
In `@crates/sharecli-session/src/layout.rs`:
- Around line 112-149: Move the layouts table and index creation into
SessionStore::init alongside the existing sessions schema statements. Remove
ensure_schema and its calls from get_layout, list_layouts, and save_layout,
leaving those methods to perform only their intended reads or writes without
executing DDL.
In `@crates/sharecli-session/src/lib.rs`:
- Around line 290-305: Bound the observations query and API exposed by
SessionStore::observations so callers cannot load the entire append-only table
into memory. Add and propagate an optional limit (or equivalent newest-first
bounded retrieval) through SessionService::observations and the
session.observations IPC route, while preserving surface filtering and ordering
semantics.
In `@crates/sharecli-session/src/recovery.rs`:
- Around line 80-83: Update the state guard in execute to exclude
SessionState::Active from unattended relaunches and return a distinct
RecoveryOutcome for active sessions. Continue permitting only states indicating
the harness is not running, while preserving the existing UnsupportedSurface
outcome for unrelated unsupported states.
- Around line 87-94: Update the successful spawn branch in the recovery executor
to retain the returned Child and reap it asynchronously on a detached thread,
preserving the non-blocking behavior required by the executor’s doc comment.
Keep returning RecoveryOutcome::Resumed immediately, while ensuring the
background reap handles the child’s wait result without affecting recovery.
In `@crates/sharecli-session/src/resolver.rs`:
- Around line 103-106: The unused _cwd_is_valid helper should not remain dead
code; remove it, or integrate its absolute-path and existence validation into
validate_recipe so invalid recovery working directories produce the existing
clear validation error before spawning.
In `@crates/sharecli-session/src/rpc_tests.rs`:
- Around line 343-360: Extend the Unix event-server tests around
dispatch_surface_with_token with two negative cases: one request omitting the
token field and another using an incorrect token. Assert each produces an error
response and verify RecordingControl.sent remains empty, preserving the
expected-token access control behavior.
- Around line 255-270: Rename
surface_events_keep_per_surface_sequence_and_wire_envelope to describe global
sequence behavior, since it only publishes events for one surface and
SurfaceEventHub uses a shared next_seq. Keep the existing assertions unchanged
unless adding a separate two-surface test and implementing per-surface
sequencing is explicitly required.
In `@crates/sharecli-session/src/rpc_transport.rs`:
- Around line 17-21: The Unix control socket is exposed before its permissions
are restricted. In crates/sharecli-session/src/rpc_transport.rs lines 17-21,
secure the socket before binding by creating or using a parent directory with
mode 0o700 (or equivalently enforcing a restrictive umask around bind), and
retain 0o600 socket permissions. Apply the same pre-listen protection in
crates/sharecli-session/src/rpc.rs lines 431-435 within
serve_surface_unix_with_events, ensuring permission hardening occurs before the
accept loop.
- Around line 39-58: The Unix newline framing logic is duplicated and has
diverged between the servers; extract one shared helper covering buffering,
MAX_SURFACE_LINE_BYTES validation, newline parsing, UTF-8 decoding, notification
suppression, and reply writing, parameterized by the dispatch closure. In
crates/sharecli-session/src/rpc_transport.rs#L39-L58, replace the inline loop
with the helper using dispatch_surface_with_token; in
crates/sharecli-session/src/rpc.rs#L443-L494, replace the framing branch with
the same helper using dispatch_surface_with_token_and_events, leaving only
subscription tracking and event ticking in that function. Ensure both callers
share identical oversize-input handling and payload representation.
- Around line 31-66: Update the connection task in rpc_transport.rs to emit
structured logs for every failure or abnormal termination: input.read I/O
errors, non-UTF-8 lines, oversized buffers, response serialization failures, and
writer.write_all failures; preserve clean EOF handling without treating it as an
error. Follow the existing logging pattern in serve_surface_unix_with_events and
include relevant error details and context for each path.
In `@crates/sharecli-session/src/rpc.rs`:
- Around line 441-516: Update the spawned connection task around the
subscriptions cleanup flow so every exit path from the read/write loop first
unsubscribes all remaining ids via events.unsubscribe. Ensure cleanup runs for
EOF, I/O failures, serialization failures, and normal task termination, while
preserving subscription tracking and event delivery behavior.
- Around line 456-460: Update the request parsing logic to derive should_reply
from the existing request_value result instead of calling
serde_json::from_str::<Value>(line) a second time. Preserve the current object
and "id" presence checks and false fallback for invalid or non-object requests.
- Around line 211-250: Update both dispatch_surface_with_token and
dispatch_surface_with_token_and_events so failed SurfaceRequest deserialization
returns an explicit invalid-request error, preserving the request ID when
available or using Value::Null, instead of calling the unauthenticated dispatch
path. Keep valid-request token comparison and normal dispatch behavior
unchanged.
- Around line 252-257: Document on the SurfaceControl trait that synchronous
methods must not block on PTY or socket I/O, and update
dispatch_surface_method_with_events to invoke provider calls through
tokio::task::spawn_blocking where the contract requires it. Preserve the
existing result and error mapping while ensuring external Send + Sync
implementations cannot block the async runtime.
In `@crates/sharecli-session/src/state.rs`:
- Around line 89-101: Update SidecarStateProvider::session_id and its
record-loading path so repeated lookups during a scan reuse parsed JSONL records
instead of rereading and reparsing the file for every surface. Cache the records
and invalidate them when the sidecar file’s modification time or length changes,
preserving the documented re-read behavior for newly appended registrations.
- Around line 69-86: Update SidecarState::records to continue processing after
serde_json parsing failures instead of collecting into a fallible Vec; log each
malformed line with structured logging including the sidecar path, line number,
and error, then skip it so valid SidecarRecord entries remain available to
scan_and_record.
In `@crates/sharecli-session/tests/layout.rs`:
- Around line 20-32: Update the cleanup in
crates/sharecli-session/tests/layout.rs lines 20-32 and
crates/sharecli-session/src/ledger.rs lines 74-94: drop reopened before cleanup,
then remove the main SQLite path and its -wal and -shm sidecar files, or use a
temporary-directory guard that removes the directory on drop.
- Around line 34-65: Add tests alongside
invalid_split_ratio_is_rejected_before_persistence and
duplicate_surface_is_rejected covering SessionStore::save_layout replacement:
save two snapshots with the same id but different content, then assert
get_layout returns the second snapshot. Add a separate test for
SessionStore::list_layouts that saves snapshots with IDs in non-sorted order and
asserts the returned snapshots are ordered by ID.
In `@docs/openapi/serve.yaml`:
- Around line 27-40: Update the dashboardAsset OpenAPI path template from
router-specific `{*path}` syntax to standard `{path}` syntax, keeping it aligned
with the declared path parameter and documenting that path as the remaining
relative asset path.
In `@docs/session-recovery.md`:
- Around line 51-52: Update the documented launch-agent installation sequence in
the session recovery instructions to replace the user-specific executable path
in ProgramArguments with the result of command -v sharecli before launchctl
bootstrap, or explicitly require that replacement step. Ensure the generated or
edited plist points to the installed sharecli binary on the current machine.
In
`@docs/sessions/20260731-sharecli-ghostty-control-plane/06_TESTING_STRATEGY.md`:
- Around line 5-6: Update the validation list in the testing strategy document
to record successful `cargo build` and `cargo clippy` runs with warnings denied,
alongside the existing workspace-locked offline validation. Include each
command’s result, and address any warnings that prevent the required clippy gate
from being clean.
In `@src/commands/fuse.rs`:
- Around line 37-46: The two FuseRuntimeEvidence JSON producers duplicate report
construction and encode mountpoint inconsistently. Add a shared report builder
next to probe_runtime in sharecli-fuse that returns the complete JSON report and
serializes mountpoint via Path::display, then replace the inline
serde_json::json! objects in src/commands/fuse.rs lines 37-46 and
crates/sharecli-fuse/src/bin/fuse-runtime-probe.rs lines 16-25 with calls to it.
In `@src/main.rs`:
- Around line 578-579: Replace the fixed /tmp socket defaults on every surface
and watch subcommand argument at the referenced locations with a per-user,
process-controlled runtime path, using XDG_RUNTIME_DIR or dirs::runtime_dir()
with a data_local_dir() fallback and preserving SHARECLI_GHOSTTY_SOCKET
overrides. Update the shared socket handling used by these commands to verify
the socket owner before connecting, rejecting sockets not owned by the current
user.
- Around line 1240-1244: In run_session_watch and observe_ghostty_surfaces,
retain the existing eprintln! messages while adding tracing::warn! events for
degraded scans and per-surface failures. Include the relevant surface_id and
error fields in each structured event, preserving the operator-facing stderr
text and existing behavior.
- Around line 1194-1222: Refactor the session command dispatch around the main
match on cmd to handle Observe, LayoutSave, LayoutList, LayoutInspect, Register,
and Watch directly, removing the earlier if let returns and all unreachable!
arms. In the Inspect arm, destructure and use the operation identifier from
SessionCmd::Inspect instead of operation.expect("id"), then remove the
now-unused operation binding while preserving each command’s existing behavior.
- Around line 1123-1132: Handle SessionCmd::Register before deriving the
database path or calling SessionStore::open, moving the existing
register-operation body into session_register. Remove the SessionCmd::Register {
.. } => (None, None) match arm, while preserving database opening only for
commands that use the store.
- Around line 586-587: Update every token argument declaring env =
"SHARECLI_GHOSTTY_TOKEN"—including the arguments near lines 565, 586, 597, 610,
628, 638, and 647—to set hide_env_values = true in its #[arg] attribute,
ensuring the token value is omitted from clap help, generated documentation, and
completions.
In `@src/runtime.rs`:
- Around line 119-125: Update the documentation comments for fd_count and
thread_count to reflect the actual platform-specific implementations: fd_count
uses /proc/{pid}/fd on Linux and fixed-path lsof fallbacks, while thread_count
uses /proc/{pid}/task on Linux, /bin/ps -M on macOS, and returns None on other
platforms or when inaccessible.
- Around line 188-192: The process projection in
crates/sharecli-ipc/src/handler.rs must copy fd_count and thread_count from
ProcessInfo so ProcessSummary receives both metrics; add an IPC serialization
test using non-None values. At src/runtime.rs lines 188-192, update the
projection accordingly. At
desktop/ShareCLITray/Sources/ShareCLITray/ProcessesPage.swift lines 3-7, retain
the expanded ProcessSummary documentation only if both fields are exposed by the
IPC payload; otherwise narrow it to delivered fields.
- Around line 169-170: Update ProcessPool::list and ProcessInfo::from_sysinfo so
count_open_fds and count_threads run in a bounded blocking path rather than
synchronously under async read locks. Enforce command timeouts and output-size
limits, and cache or rate-limit probe results per refresh to avoid repeated
subprocesses. On probe failure, emit structured logs with process and probe
details before returning None.
In `@src/session.rs`:
- Around line 208-233: Update the request envelope constructed in
Session::request to include the JSON-RPC version field with value "2.0"
alongside id, method, and params. Preserve the existing token handling and
response processing for all callers.
- Around line 294-321: Update both subscribe_surface and request to configure
read_timeout and write_timeout on the connected UnixStream before sending the
handshake, preventing indefinite blocking while awaiting acknowledgement. After
subscribe_surface parses the acknowledgement, clear the read timeout so the
subscription’s next_event flow remains blocking for live events; preserve the
existing request response handling and propagate timeout errors consistently.
In `@tests/fr007_ps_all_csv.rs`:
- Around line 159-160: Complete the ProcessInfo fixture in the test by
initializing cpu_percent, ppid, cwd, env_count, state, disk_read_bytes, and
disk_write_bytes, or introduce a test-local helper that supplies all required
fields. Keep the existing fd_count and thread_count values and ensure the
literal compiles without relying on struct update syntax.
In `@tests/integration_cli.rs`:
- Around line 117-121: Update the integration test assertion for
report.mountpoint to compare its value with the exact mountpoint argument passed
to the fuse probe, rather than only checking that the key exists. Keep the
existing selected_backend and non_fuse_fallback assertions unchanged.
In `@tests/session_cli.rs`:
- Around line 79-97: Update
session_watch_once_fails_open_when_native_socket_is_unavailable to pass an
explicit --state-sidecar path under the existing TempDir, and remove
SHARECLI_SESSION_SIDECAR and SHARECLI_GHOSTTY_TOKEN from the child environment
to isolate the test. Also include captured stderr in the
assert!(out.status.success()) failure message, matching the neighboring tests.
In `@tests/session.rs`:
- Around line 40-42: Update both missing-socket tests, including
ghostty_control_client_requires_configured_socket and the additionally
referenced test, to use a unique nonexistent socket path under
std::env::temp_dir(), matching the file’s existing SystemTime nanosecond-suffix
pattern or a TempDir-based path. Remove the hard-coded
/tmp/sharecli-no-ghostty.sock path while preserving each test’s existing
assertions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b19850e0-9c65-4a0e-97e7-3bf5fa95f8f7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
contrib/ghostty-control/.gitignorecontrib/ghostty-control/Package.swiftcontrib/ghostty-control/README.mdcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swiftcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swiftcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swiftcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/SurfaceProvider.swiftcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swiftcontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swiftcontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swiftcontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/SurfaceProviderTests.swiftcrates/sharecli-fuse/Cargo.tomlcrates/sharecli-fuse/build.rscrates/sharecli-fuse/src/backend.rscrates/sharecli-fuse/src/bin/fuse-runtime-probe.rscrates/sharecli-fuse/src/lib.rscrates/sharecli-fuse/src/session_registry.rscrates/sharecli-ipc/Cargo.tomlcrates/sharecli-ipc/src/handler.rscrates/sharecli-ipc/tests/handler_dispatch.rscrates/sharecli-session/Cargo.tomlcrates/sharecli-session/src/adapter.rscrates/sharecli-session/src/discovery.rscrates/sharecli-session/src/events.rscrates/sharecli-session/src/layout.rscrates/sharecli-session/src/ledger.rscrates/sharecli-session/src/lib.rscrates/sharecli-session/src/recovery.rscrates/sharecli-session/src/resolver.rscrates/sharecli-session/src/rpc.rscrates/sharecli-session/src/rpc_tests.rscrates/sharecli-session/src/rpc_transport.rscrates/sharecli-session/src/state.rscrates/sharecli-session/tests/discovery.rscrates/sharecli-session/tests/layout.rscrates/sharecli-session/tests/state.rsdesktop/ShareCLITray/Sources/ShareCLITray/ProcessesPage.swiftdocs/openapi/serve.yamldocs/session-recovery.mddocs/sessions/20260731-sharecli-ghostty-control-plane/00_SESSION_OVERVIEW.mddocs/sessions/20260731-sharecli-ghostty-control-plane/01_RESEARCH.mddocs/sessions/20260731-sharecli-ghostty-control-plane/02_SPECIFICATIONS.mddocs/sessions/20260731-sharecli-ghostty-control-plane/03_DAG_WBS.mddocs/sessions/20260731-sharecli-ghostty-control-plane/04_IMPLEMENTATION_STRATEGY.mddocs/sessions/20260731-sharecli-ghostty-control-plane/05_KNOWN_ISSUES.mddocs/sessions/20260731-sharecli-ghostty-control-plane/06_TESTING_STRATEGY.mdsrc/commands/fuse.rssrc/commands/mod.rssrc/commands/report.rssrc/commands/serve.rssrc/main.rssrc/runtime.rssrc/session.rstests/fr004_status_health.rstests/fr007_health_pool_json_gate_host_watch.rstests/fr007_health_pool_status_csv.rstests/fr007_health_watch_json_gate_host_watch.rstests/fr007_ps_all_csv.rstests/fr007_ps_all_json_gate_host_watch.rstests/fr007_ps_all_watch_json_gate_host_watch.rstests/fr007_status_watch_json_gate_host_watch.rstests/integration_cli.rstests/session.rstests/session_cli.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Use UTF-8 encoding for all text files; do not use Windows-1252 smart quotes or other special characters.
Files:
crates/sharecli-ipc/Cargo.tomlsrc/commands/serve.rscontrib/ghostty-control/Package.swiftdocs/sessions/20260731-sharecli-ghostty-control-plane/06_TESTING_STRATEGY.mddocs/sessions/20260731-sharecli-ghostty-control-plane/05_KNOWN_ISSUES.mdcrates/sharecli-fuse/build.rsdocs/openapi/serve.yamldocs/sessions/20260731-sharecli-ghostty-control-plane/00_SESSION_OVERVIEW.mdsrc/commands/report.rsdocs/sessions/20260731-sharecli-ghostty-control-plane/03_DAG_WBS.mdtests/fr004_status_health.rsdocs/sessions/20260731-sharecli-ghostty-control-plane/04_IMPLEMENTATION_STRATEGY.mdtests/fr007_health_pool_json_gate_host_watch.rssrc/commands/mod.rscrates/sharecli-fuse/src/bin/fuse-runtime-probe.rscontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swifttests/fr007_health_pool_status_csv.rsdocs/sessions/20260731-sharecli-ghostty-control-plane/01_RESEARCH.mddocs/sessions/20260731-sharecli-ghostty-control-plane/02_SPECIFICATIONS.mdcrates/sharecli-session/tests/layout.rscrates/sharecli-session/src/rpc_transport.rstests/integration_cli.rstests/fr007_ps_all_json_gate_host_watch.rscontrib/ghostty-control/README.mdcrates/sharecli-session/src/discovery.rscrates/sharecli-session/src/state.rscontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/SurfaceProviderTests.swifttests/session_cli.rscrates/sharecli-session/tests/state.rscrates/sharecli-ipc/tests/handler_dispatch.rscrates/sharecli-session/src/ledger.rstests/fr007_ps_all_watch_json_gate_host_watch.rscrates/sharecli-fuse/Cargo.tomltests/fr007_status_watch_json_gate_host_watch.rstests/fr007_ps_all_csv.rsdesktop/ShareCLITray/Sources/ShareCLITray/ProcessesPage.swiftcrates/sharecli-fuse/src/lib.rscrates/sharecli-session/src/resolver.rscrates/sharecli-session/tests/discovery.rscrates/sharecli-session/src/rpc_tests.rscrates/sharecli-session/src/adapter.rsdocs/session-recovery.mdcontrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swiftcrates/sharecli-session/src/recovery.rstests/fr007_health_watch_json_gate_host_watch.rscrates/sharecli-session/Cargo.tomlsrc/commands/fuse.rscrates/sharecli-session/src/layout.rscontrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swiftsrc/runtime.rscontrib/ghostty-control/Sources/ShareCLIGhosttyControl/SurfaceProvider.swiftcrates/sharecli-ipc/src/handler.rscontrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swiftcrates/sharecli-session/src/events.rscontrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swiftcrates/sharecli-fuse/src/backend.rscrates/sharecli-fuse/src/session_registry.rscrates/sharecli-session/src/lib.rscrates/sharecli-session/src/rpc.rscontrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swiftsrc/main.rssrc/session.rstests/session.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Keep the project compatible with the pinned stable toolchain defined by
rust-toolchain.toml, and ensure code is clean under rustfmt and clippy with warnings denied.
Files:
crates/sharecli-ipc/Cargo.tomlsrc/commands/serve.rscrates/sharecli-fuse/build.rssrc/commands/report.rstests/fr004_status_health.rstests/fr007_health_pool_json_gate_host_watch.rssrc/commands/mod.rscrates/sharecli-fuse/src/bin/fuse-runtime-probe.rstests/fr007_health_pool_status_csv.rscrates/sharecli-session/tests/layout.rscrates/sharecli-session/src/rpc_transport.rstests/integration_cli.rstests/fr007_ps_all_json_gate_host_watch.rscrates/sharecli-session/src/discovery.rscrates/sharecli-session/src/state.rstests/session_cli.rscrates/sharecli-session/tests/state.rscrates/sharecli-ipc/tests/handler_dispatch.rscrates/sharecli-session/src/ledger.rstests/fr007_ps_all_watch_json_gate_host_watch.rscrates/sharecli-fuse/Cargo.tomltests/fr007_status_watch_json_gate_host_watch.rstests/fr007_ps_all_csv.rscrates/sharecli-fuse/src/lib.rscrates/sharecli-session/src/resolver.rscrates/sharecli-session/tests/discovery.rscrates/sharecli-session/src/rpc_tests.rscrates/sharecli-session/src/adapter.rscrates/sharecli-session/src/recovery.rstests/fr007_health_watch_json_gate_host_watch.rscrates/sharecli-session/Cargo.tomlsrc/commands/fuse.rscrates/sharecli-session/src/layout.rssrc/runtime.rscrates/sharecli-ipc/src/handler.rscrates/sharecli-session/src/events.rscrates/sharecli-fuse/src/backend.rscrates/sharecli-fuse/src/session_registry.rscrates/sharecli-session/src/lib.rscrates/sharecli-session/src/rpc.rssrc/main.rssrc/session.rstests/session.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust for the project and validate Rust changes with Cargo build, Cargo test, and Cargo clippy.
**/*.rs: This project uses Rust edition 2021; follow Rust conventions and language-appropriate error handling.
Never useunwraporexpectin production Rust code.
Log all errors with structured logging.
UsePascalCasefor types,snake_casefor functions, methods, and modules, andSCREAMING_SNAKE_CASEfor constants.
Process-pool tests must use appropriate platform gates such as#[cfg(unix)]and#[cfg(windows)]where platform behavior differs.
Files:
src/commands/serve.rscrates/sharecli-fuse/build.rssrc/commands/report.rstests/fr004_status_health.rstests/fr007_health_pool_json_gate_host_watch.rssrc/commands/mod.rscrates/sharecli-fuse/src/bin/fuse-runtime-probe.rstests/fr007_health_pool_status_csv.rscrates/sharecli-session/tests/layout.rscrates/sharecli-session/src/rpc_transport.rstests/integration_cli.rstests/fr007_ps_all_json_gate_host_watch.rscrates/sharecli-session/src/discovery.rscrates/sharecli-session/src/state.rstests/session_cli.rscrates/sharecli-session/tests/state.rscrates/sharecli-ipc/tests/handler_dispatch.rscrates/sharecli-session/src/ledger.rstests/fr007_ps_all_watch_json_gate_host_watch.rstests/fr007_status_watch_json_gate_host_watch.rstests/fr007_ps_all_csv.rscrates/sharecli-fuse/src/lib.rscrates/sharecli-session/src/resolver.rscrates/sharecli-session/tests/discovery.rscrates/sharecli-session/src/rpc_tests.rscrates/sharecli-session/src/adapter.rscrates/sharecli-session/src/recovery.rstests/fr007_health_watch_json_gate_host_watch.rssrc/commands/fuse.rscrates/sharecli-session/src/layout.rssrc/runtime.rscrates/sharecli-ipc/src/handler.rscrates/sharecli-session/src/events.rscrates/sharecli-fuse/src/backend.rscrates/sharecli-fuse/src/session_registry.rscrates/sharecli-session/src/lib.rscrates/sharecli-session/src/rpc.rssrc/main.rssrc/session.rstests/session.rs
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
New modules must have a test file before the implementation file; bug fixes require a failing test before the fix; refactors require existing tests to pass before and after the change.
Files:
src/commands/serve.rssrc/commands/report.rssrc/commands/mod.rssrc/commands/fuse.rssrc/runtime.rssrc/main.rssrc/session.rs
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Use UTF-8 encoding for text files and do not introduce Windows-1252 smart quotes.
PR bodies must include anFR-reference.
Files:
docs/sessions/20260731-sharecli-ghostty-control-plane/06_TESTING_STRATEGY.mddocs/sessions/20260731-sharecli-ghostty-control-plane/05_KNOWN_ISSUES.mddocs/sessions/20260731-sharecli-ghostty-control-plane/00_SESSION_OVERVIEW.mddocs/sessions/20260731-sharecli-ghostty-control-plane/03_DAG_WBS.mddocs/sessions/20260731-sharecli-ghostty-control-plane/04_IMPLEMENTATION_STRATEGY.mddocs/sessions/20260731-sharecli-ghostty-control-plane/01_RESEARCH.mddocs/sessions/20260731-sharecli-ghostty-control-plane/02_SPECIFICATIONS.mdcontrib/ghostty-control/README.mddocs/session-recovery.md
tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Maintain acceptance and integration tests for the documented functional requirements, including
tests/fr001_*.rsandtests/integration_cli.rs.
Files:
tests/fr004_status_health.rstests/fr007_health_pool_json_gate_host_watch.rstests/fr007_health_pool_status_csv.rstests/integration_cli.rstests/fr007_ps_all_json_gate_host_watch.rstests/session_cli.rstests/fr007_ps_all_watch_json_gate_host_watch.rstests/fr007_status_watch_json_gate_host_watch.rstests/fr007_ps_all_csv.rstests/fr007_health_watch_json_gate_host_watch.rstests/session.rs
🪛 GitHub Check: SonarCloud Code Analysis
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swift
[warning] 108-108: Either remove or fill this block of code.
[warning] 113-113: Either remove or fill this block of code.
[warning] 103-103: Either remove or fill this block of code.
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/SurfaceProviderTests.swift
[failure] 15-15: Add a nested comment explaining why this closure is empty, or complete the implementation.
[warning] 10-10: Refactor your code to get this URI from a customizable parameter.
[failure] 13-13: Add a nested comment explaining why this closure is empty, or complete the implementation.
[warning] 11-11: Refactor your code to get this URI from a customizable parameter.
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/SurfaceProvider.swift
[warning] 67-67: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 63-63: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 71-71: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 63-63: Remove the unused function parameter "bytes" or name it "_".
[warning] 67-67: Remove the unused function parameter "maxBytes" or name it "_".
[warning] 71-71: Remove the unused function parameter "cols" or name it "_".
[failure] 89-89: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 71-71: Remove the unused function parameter "rows" or name it "_".
[warning] 75-75: Remove the unused function parameter "surfaceID" or name it "_".
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift
[failure] 93-93: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 32-32: Initializer has 8 parameters, which is greater than the 7 authorized.
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swift
[warning] 35-35: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 25-25: Remove the unused function parameter "bytes" or name it "_".
[warning] 39-39: Remove the unused function parameter "cols" or name it "_".
[warning] 27-27: Remove the unused function parameter "rows" or name it "_".
[warning] 39-39: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 35-35: Remove the unused function parameter "bytes" or name it "_".
[warning] 28-28: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 11-11: Remove the unused function parameter "surfaceID" or name it "_".
[failure] 11-11: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 39-39: Remove the unused function parameter "rows" or name it "_".
[warning] 13-13: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 14-14: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 13-13: Remove the unused function parameter "rows" or name it "_".
[warning] 22-22: Refactor your code to get this URI from a customizable parameter.
[failure] 39-39: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 27-27: Remove the unused function parameter "cols" or name it "_".
[failure] 27-27: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 36-36: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 25-25: Remove the unused function parameter "surfaceID" or name it "_".
[failure] 25-25: Add a nested comment explaining why this function is empty, or complete the implementation.
[failure] 13-13: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 40-40: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 8-8: Refactor your code to get this URI from a customizable parameter.
[warning] 26-26: Remove the unused function parameter "maxBytes" or name it "_".
[warning] 27-27: Remove the unused function parameter "surfaceID" or name it "_".
[failure] 35-35: Add a nested comment explaining why this function is empty, or complete the implementation.
[warning] 13-13: Remove the unused function parameter "cols" or name it "_".
[warning] 26-26: Remove the unused function parameter "surfaceID" or name it "_".
[warning] 11-11: Remove the unused function parameter "bytes" or name it "_".
[warning] 12-12: Remove the unused function parameter "surfaceID" or name it "_".
🪛 LanguageTool
docs/sessions/20260731-sharecli-ghostty-control-plane/05_KNOWN_ISSUES.md
[grammar] ~13-~13: Ensure spelling is correct
Context: ...event writer. It still needs a concrete Ghostty provider to publish PTY output; witho...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 SwiftLint (0.65.0)
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift
[Warning] 179-179: Classes should have an explicit deinit method
(required_deinit)
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swift
[Warning] 20-20: Classes should have an explicit deinit method
(required_deinit)
🔇 Additional comments (63)
crates/sharecli-ipc/tests/handler_dispatch.rs (1)
170-214: Covered by the shared-database comment on the first test in this file.crates/sharecli-ipc/Cargo.toml (1)
29-29: LGTM!crates/sharecli-ipc/src/handler.rs (1)
23-30: LGTM!Also applies to: 531-540, 566-585, 715-715
src/session.rs (1)
106-121: LGTM!Also applies to: 123-195, 235-278, 337-362
src/main.rs (2)
6-15: LGTM!Also applies to: 102-106, 152-153, 494-577, 669-676, 884-929, 944-944, 1050-1050, 1271-1352, 1354-1364, 1416-1416
621-624: 🎯 Functional CorrectnessNo change needed.
The default
max_chunk_bytesof65536matches the accepted1..=65536range for both client and server event buffer validation.tests/session.rs (1)
1-3: LGTM!Also applies to: 43-45, 47-145, 151-163
tests/session_cli.rs (1)
19-49: LGTM!Also applies to: 51-75
src/runtime.rs (1)
140-145: LGTM!Also applies to: 157-159, 440-441
desktop/ShareCLITray/Sources/ShareCLITray/ProcessesPage.swift (1)
1584-1584: LGTM!tests/fr007_health_pool_status_csv.rs (1)
142-142: LGTM!tests/fr007_health_watch_json_gate_host_watch.rs (1)
242-242: LGTM!tests/fr007_ps_all_json_gate_host_watch.rs (1)
222-222: LGTM!tests/fr007_ps_all_watch_json_gate_host_watch.rs (1)
275-275: LGTM!tests/fr007_status_watch_json_gate_host_watch.rs (1)
194-194: LGTM!docs/session-recovery.md (1)
4-6: LGTM!Also applies to: 22-49, 53-57, 59-66
docs/sessions/20260731-sharecli-ghostty-control-plane/00_SESSION_OVERVIEW.md (1)
1-28: LGTM!src/commands/mod.rs (1)
1639-1640: LGTM!src/commands/report.rs (1)
538-539: LGTM!src/commands/serve.rs (1)
927-928: LGTM!tests/fr004_status_health.rs (1)
93-94: LGTM!tests/fr007_health_pool_json_gate_host_watch.rs (1)
216-216: LGTM!docs/sessions/20260731-sharecli-ghostty-control-plane/01_RESEARCH.md (1)
1-76: LGTM!docs/sessions/20260731-sharecli-ghostty-control-plane/02_SPECIFICATIONS.md (1)
1-70: LGTM!docs/sessions/20260731-sharecli-ghostty-control-plane/03_DAG_WBS.md (1)
1-27: LGTM!docs/sessions/20260731-sharecli-ghostty-control-plane/04_IMPLEMENTATION_STRATEGY.md (1)
1-45: LGTM!docs/sessions/20260731-sharecli-ghostty-control-plane/05_KNOWN_ISSUES.md (1)
1-34: LGTM!crates/sharecli-fuse/Cargo.toml (1)
13-24: LGTM!crates/sharecli-fuse/src/backend.rs (3)
8-91: LGTM!
113-182: LGTM!
232-293: LGTM!crates/sharecli-fuse/src/lib.rs (2)
29-53: LGTM!
986-1017: 🗄️ Data Integrity & IntegrationNo change needed.
smoke_fuser_config()is gated to Linux/macOS, and the non-macOS path already excludes macOS while continuing to compile only for Linux targets.crates/sharecli-fuse/src/session_registry.rs (3)
23-24: LGTM!
625-658: LGTM!
70-95: 📐 Maintainability & Code QualityNo change needed. The helper is already gated to Linux/macOS targets, so the
#[cfg(not(any(target_os = "linux", target_os = "macos")))]branch is non-existent dead code rather than an unreachable path in the active binary.> Likely an incorrect or invalid review comment.src/commands/fuse.rs (1)
10-11: LGTM!crates/sharecli-session/Cargo.toml (1)
8-12: LGTM!crates/sharecli-session/tests/state.rs (1)
12-35: LGTM!Also applies to: 37-112
crates/sharecli-session/src/events.rs (2)
122-156: LGTM!
225-236: LGTM!Also applies to: 272-290
crates/sharecli-session/src/rpc_tests.rs (1)
6-64: LGTM!Also applies to: 82-253, 272-341
contrib/ghostty-control/.gitignore (1)
1-1: LGTM!contrib/ghostty-control/Package.swift (1)
1-14: LGTM!contrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swift (1)
4-8: LGTM!Also applies to: 17-34, 36-59, 61-66
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swift (1)
5-36: LGTM!Also applies to: 64-96
crates/sharecli-session/src/adapter.rs (1)
8-36: LGTM!Also applies to: 64-114
crates/sharecli-session/src/discovery.rs (1)
12-49: LGTM!Also applies to: 52-111, 183-202
crates/sharecli-session/src/layout.rs (1)
10-47: LGTM!Also applies to: 49-109
crates/sharecli-session/src/lib.rs (1)
48-77: LGTM!Also applies to: 132-140, 214-219, 228-249, 264-288, 306-318, 352-410, 429-434
crates/sharecli-session/src/recovery.rs (2)
9-51: LGTM!Also applies to: 98-131
55-71: 🎯 Functional CorrectnessNo change needed:
sessionis borrowed for the enclosing scope lifetime.
std::thread::scopeguarantees spawned threads are joined before the scope’s closure returns, so the&AgentSessioncaptured frommapis valid while the worker runs.> Likely an incorrect or invalid review comment.crates/sharecli-session/src/resolver.rs (2)
8-21: LGTM!Also applies to: 24-34, 42-101, 108-133
35-41: 🎯 Functional CorrectnessConfirm the state-file confidence ranking is intentional.
ResolutionConfidencehas no ordering, butstate_and_argv_are_corroboratedlocksCorroboratedfor matches andExactfor non-matches. Decide which variant means “stronger evidence,” then swap the branches or rename the variants so the label matches that design.crates/sharecli-session/src/state.rs (1)
13-20: LGTM!Also applies to: 23-50, 53-67, 102-109
crates/sharecli-session/tests/discovery.rs (1)
1-173: LGTM!crates/sharecli-session/src/rpc.rs (1)
39-118: LGTM!Also applies to: 152-209, 319-358, 360-378, 406-413
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift (1)
1-160: LGTM!Also applies to: 243-314
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift (1)
1-110: LGTM!Also applies to: 126-141, 184-195
contrib/ghostty-control/Sources/ShareCLIGhosttyControl/SurfaceProvider.swift (1)
1-140: LGTM!contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift (1)
15-61: LGTM!Also applies to: 109-214
contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/ControlTests.swift (1)
6-168: LGTM!contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/SurfaceProviderTests.swift (1)
1-96: LGTM!
| deinit { | ||
| server?.stop() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Swift 6 deinit MainActor isolated stored property access error
💡 Result:
In Swift 6, you may encounter a compiler error when accessing or mutating MainActor-isolated properties within a deinitializer [1][2]. This occurs because, by default, deinitializers are non-isolated and are not guaranteed to execute on the actor to which the class is isolated [3][4]. Because the last reference to an object can be released from any thread, a non-isolated deinitializer cannot safely access actor-isolated state, as this would create a data race [3][2]. To resolve this issue, Swift introduced SE-0371: Isolated Synchronous Deinit [5][6]. The solution is to mark your deinitializer as isolated to the appropriate actor [5][7]. For a class annotated with @MainActor, you can define an isolated deinitializer as follows [5]: @MainActor class MyClass { var someProperty: SomeUnsendableType? isolated deinit { // You can now safely access MainActor-isolated properties here someProperty = nil } } Key points regarding isolated deinitializers: 1. Opt-in requirement: Isolation is not the default for deinitializers. You must explicitly use the isolated keyword (e.g., isolated deinit) to opt into this behavior [5][8]. 2. Runtime behavior: When you use isolated deinit, the Swift runtime ensures the deinitializer body executes on the required actor's executor [5][7]. If the object is already being deallocated on that actor, it executes synchronously [5][8]. If it is being released from a different context, the system schedules the deinitializer to run on the correct executor [5][7]. 3. Task locals: When entering an isolated deinit, task-local values are cleared. You should not rely on accessing any task-local values within the body of an isolated deinit [6][8]. 4. Alternatives: If you are unable to use isolated deinit or are working in an environment where it is not yet available, avoid storing non-sendable types that require explicit cleanup in deinit [2]. Alternatively, consider using nonisolated(unsafe) for properties if you can guarantee memory safety manually, though this is discouraged as it disables compiler data-race checking [2][9]. Avoid attempting to use DispatchQueue.main.async or Task { await MainActor.run {... } } inside a standard deinit, as these patterns often fail to correctly manage the object's lifetime and can lead to use-after-free errors [4][2].
Citations:
- 1: https://forums.swift.org/t/mainactor-isolated-property-can-not-be-mutated-from-deinit/72371
- 2: https://stackoverflow.com/questions/78774528/how-to-deallocate-resource-in-main-actor-isolated-object-in-deinit
- 3: https://forums.swift.org/t/cannot-access-property-with-a-non-sendable-type-from-non-isolated-deinit/57877
- 4: https://forums.swift.org/t/is-mainactor-assumeisolated-truly-necessary-in-deinit-for-a-mainactor-annotated-class/72921
- 5: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0371-isolated-synchronous-deinit.md
- 6: https://forums.swift.org/t/accepted-with-modifications-se-0371-isolated-synchronous-deinit/74042
- 7: https://github.com/apple/swift-evolution/blob/main/proposals/0371-isolated-synchronous-deinit.md
- 8: https://forums.swift.org/t/second-review-se-0371-isolated-synchronous-deinit/73406
- 9: https://forums.swift.org/t/unexpected-deinit-not-on-the-mainactor/81599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## package files"
fd -a 'Package\.swift$|Package@amd64\.swift$' . | sed 's#^\./##'
echo
echo "## target file snippet"
if [ -f contrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swift ]; then
sed -n '1,130p' contrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swift | cat -n
fi
echo
echo "## UnixControlServer declarations/usages"
rg -n "class UnixControlServer|struct Unix|actor Unix|Stop|stop\\(|deinit|`@MainActor`" contrib/ghostty-control || true
echo
echo "## swift-tools-version"
rg -n "swift-tools-version" .Repository: KooshaPari/sharecli
Length of output: 5561
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## UnixControlServer.swift"
cat -n contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift
echo
echo "## Concurrency/Actor annotations in sources"
rg -n "`@MainActor`|nonisolated\\(|Sendable|`@unchecked` Sendable|class UnixControlServer|actor|deinit|isolated deinit|func stop\\(" contrib/ghostty-control/SourcesRepository: KooshaPari/sharecli
Length of output: 13934
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v swift >/dev/null 2>&1; then
swift --version
tmpdir="$(mktemp -d)"
cat > "$tmpdir/ControlLifecycle.swift" <<'SWIFT'
import Foundation
final class UnixControlServer: `@unchecked` Sendable {
func stop() {}
}
`@MainActor`
public final class ControlLifecycle {
private var server: UnixControlServer?
public init() {}
public func stop() {
server?.stop()
server = nil
}
deinit {
server?.stop()
}
}
SWIFT
if swift "$tmpdir/ControlLifecycle.swift" 2>&1; then
echo "## swift compilation: passed"
else
echo "## swift compilation: failed"
fi
else
echo "## swift not installed"
fi
if command -v swiftc >/dev/null 2>&1; then
swiftc --version
tmpdir="$(mktemp -d)"
cat > "$tmpdir/ControlLifecycle.swift" <<'SWIFT'
import Foundation
final class UnixControlServer: `@unchecked` Sendable {
func stop() {}
}
`@MainActor`
public final class ControlLifecycle {
private var server: UnixControlServer?
public init() {}
public func stop() {
server?.stop()
server = nil
}
deinit {
server?.stop()
}
}
SWIFT
if swiftc "$tmpdir/ControlLifecycle.swift" 2>&1; then
echo "## swiftc compilation: passed"
else
echo "## swiftc compilation: failed"
fi
else
echo "## swiftc not installed"
fiRepository: KooshaPari/sharecli
Length of output: 204
🌐 Web query:
Swift SE-0371 nonisolated deinitializer deinit MainActor stored property access
💡 Result:
In Swift, a standard (non-isolated) deinitializer does not run on a specific actor's executor, even if the class itself is isolated to an actor like @MainActor [1][2]. Because deinitialization can occur on any thread depending on where the final reference is released, accessing isolated stored properties from a standard deinit is restricted to prevent data races [1][2]. SE-0371 introduced isolated synchronous deinitializers to solve this limitation [3][4]. Key details: 1. Isolated Deinitializers: You can now declare a deinitializer as isolated by adding the isolated keyword or an explicit actor attribute (e.g., isolated deinit) [3][5]. This instructs the Swift runtime to hop to the appropriate actor's executor before executing the deinit body, allowing safe access to the actor's isolated stored properties [3][5]. 2. Default Behavior: By default, deinitializers remain non-isolated [3][6]. This avoids unnecessary performance overhead—such as mandatory thread hopping—for types that do not require access to isolated state during cleanup [3][7]. 3. Non-isolated Constraints: If you do not mark a deinit as isolated, the compiler enforces that you cannot access isolated stored properties or call isolated methods, because the deinit is not running within that isolation domain [1][8][2]. This diagnostic is a core part of Swift's strict concurrency checking [1][2]. 4. Task Locals: When an isolated deinit is triggered, task-local values are consistently cleared to ensure the deinitializer does not rely on unintended task-local state from the context that triggered the release [4]. In summary, if you need to access MainActor-isolated properties within a deinit, you should mark the deinit as isolated (e.g., isolated deinit) to ensure the code executes on the MainActor [3][9]. If your deinit only interacts with Sendable data or does not require access to isolated members, the default non-isolated behavior is preferred for performance [3].
Citations:
- 1: https://forums.swift.org/t/mainactor-isolated-property-can-not-be-mutated-from-deinit/72371
- 2: https://forums.swift.org/t/cannot-access-property-with-a-non-sendable-type-from-non-isolated-deinit/57877
- 3: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0371-isolated-synchronous-deinit.md
- 4: https://forums.swift.org/t/accepted-with-modifications-se-0371-isolated-synchronous-deinit/74042
- 5: https://github.com/apple/swift-evolution/blob/main/proposals/0371-isolated-synchronous-deinit.md
- 6: https://forums.swift.org/t/returned-for-revision-se-0371-isolated-synchronous-deinit/60060
- 7: https://forums.swift.org/t/se-0371-isolated-synchronous-deinit/59754
- 8: Unexpected non-isolated deinit warning swiftlang/swift#60498
- 9: https://forums.swift.org/t/accessing-main-actor-isolated-property-from-deinit-is-allowed-but-should-it/80377
🌐 Web query:
Apple Swift 6 migration guide deinitializer MainActor isolated stored property deinit
💡 Result:
In Swift 6 and later, deinitializers (deinit) are traditionally non-isolated by default, meaning they cannot directly access properties or methods isolated to an actor (like @MainActor) [1][2]. This often results in compiler errors when attempting to access MainActor-isolated state from within a deinit [3][4][5]. To address this, Swift 6.2 introduced isolated deinit (via SE-0371), which allows a deinitializer to be explicitly isolated to the same actor as its class [6][2][7]. Key strategies for handling MainActor-isolated stored properties in deinit include: 1. Use isolated deinit (Swift 6.2+): If your project targets environments supporting this feature (e.g., iOS 18.4+, macOS 15.4+), mark your deinit as isolated [8]. This allows the deinitializer to run on the class's actor executor, enabling safe access to isolated properties [2][7]. swift `@MainActor` class MyClass { var someProperty: String = "..." isolated deinit { // Can safely access MainActor properties print(someProperty) } } 2. Capture Values and Use Unstructured Tasks (Pre-Swift 6.2 or legacy code): If you cannot use isolated deinit, you must avoid accessing isolated properties directly, as the deinitializer is not guaranteed to run on the MainActor [1]. You can instead capture the necessary data into local variables or constants and perform cleanup in an unstructured Task [1][5]. - Critical: You must ensure the Task does not capture self strongly, as extending the lifetime of self within deinit can lead to runtime crashes [1]. - Example: Capture specific properties into local variables before the Task, and perform the cleanup asynchronously [9][1]. 3. Alternatives to Deinit: For complex cleanup (such as file handles or network connections), it is often better to move away from deinit entirely in favor of explicit lifecycle methods (e.g., close(), cleanup(), or "with-style" wrapper APIs) that can be called deterministically before the object is destroyed [6][1][5]. Note on Default Actor Isolation: With Swift 6.2's "Default Actor Isolation" (SE-0466), many types may be implicitly isolated to @MainActor [2][10]. While this simplifies much of your code, it also means your deinit might now require the isolated keyword to maintain this isolation context [10]. If you encounter compiler errors in a project with default isolation enabled, adding the isolated modifier to your deinit is the standard fix [11][10].
Citations:
- 1: https://github.com/apple/swift-migration-guide/blob/main/Guide.docc/CommonProblems.md
- 2: https://swiftcrafted.dev/article/swift-6-2-approachable-concurrency-complete-guide
- 3: https://forums.swift.org/t/mainactor-isolated-property-can-not-be-mutated-from-deinit/72371
- 4: https://stackoverflow.com/questions/78284030/swift-5-10-cannot-access-property-with-a-non-sendable-type-from-non-iso
- 5: https://developer.apple.com/forums/thread/749698
- 6: https://forums.swift.org/t/unexpected-deinit-not-on-the-mainactor/81599
- 7: https://github.com/swiftlang/swift-evolution/blob/main/proposals/0371-isolated-synchronous-deinit.md
- 8: https://onmyway133.com/posts/how-to-use-isolated-and-nonisolated-in-swift-6-concurrency/
- 9: https://forums.swift.org/t/is-mainactor-assumeisolated-truly-necessary-in-deinit-for-a-mainactor-annotated-class/72921
- 10: https://fatbobman.com/en/posts/default-actor-isolation/
- 11: https://forums.swift.org/t/default-actor-isolation-and-deinit/84511
Stop the Lifecycle client on the MainActor instead of reading server in deinit.
ControlLifecycle is @MainActor, but deinit is not isolated and can run outside MainActor isolation, so server?.stop() is unsafe in Swift 6 language mode. Move socket cleanup into UnixControlServer or make this deinit isolated deinit; either way, callers should still call lifecycle.stop() explicitly before teardown.
🤖 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
`@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/ControlLifecycle.swift`
around lines 68 - 70, Update ControlLifecycle deinitialization so it does not
access the MainActor-isolated server from deinit: move socket cleanup into
UnixControlServer or use an isolated deinit, while preserving explicit
lifecycle.stop() calls before teardown.
| do { | ||
| _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 0, queueCapacity: 1) | ||
| Issue.record("zero max_chunk_bytes must be rejected") | ||
| } catch LiveIOError.invalidChunkBytes { } | ||
|
|
||
| do { | ||
| _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 65_537, queueCapacity: 1) | ||
| Issue.record("max_chunk_bytes above the wire limit must be rejected") | ||
| } catch LiveIOError.invalidChunkBytes { } | ||
|
|
||
| do { | ||
| _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 1, queueCapacity: 257) | ||
| Issue.record("queue capacity above the wire limit must be rejected") | ||
| } catch LiveIOError.invalidQueueCapacity { } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Replace the empty catch blocks with #expect(throws:).
SonarCloud flags the empty blocks on lines 103, 108, and 113. Swift Testing provides #expect(throws:) for this case. It removes the empty blocks, removes the Issue.record calls, and states the expectation in one line.
Note that a zero queueCapacity is not covered. Line 111 covers a capacity above the limit only. Add that case if LiveIOEventHub.subscribe rejects zero.
♻️ Proposed rewrite
`@Test` func liveEventLimitsAreValidated() async throws {
let hub = LiveIOEventHub()
- do {
- _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 0, queueCapacity: 1)
- Issue.record("zero max_chunk_bytes must be rejected")
- } catch LiveIOError.invalidChunkBytes { }
-
- do {
- _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 65_537, queueCapacity: 1)
- Issue.record("max_chunk_bytes above the wire limit must be rejected")
- } catch LiveIOError.invalidChunkBytes { }
-
- do {
- _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 1, queueCapacity: 257)
- Issue.record("queue capacity above the wire limit must be rejected")
- } catch LiveIOError.invalidQueueCapacity { }
+ await `#expect`(throws: LiveIOError.invalidChunkBytes) {
+ _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 0, queueCapacity: 1)
+ }
+ await `#expect`(throws: LiveIOError.invalidChunkBytes) {
+ _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 65_537, queueCapacity: 1)
+ }
+ await `#expect`(throws: LiveIOError.invalidQueueCapacity) {
+ _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 1, queueCapacity: 257)
+ }
+ await `#expect`(throws: LiveIOError.invalidQueueCapacity) {
+ _ = try await hub.subscribe(surfaceID: nil, fromSequence: nil, maxChunkBytes: 1, queueCapacity: 0)
+ }
}#expect(throws:) with a concrete error value requires LiveIOError to conform to Equatable. If it does not, use #expect(throws: LiveIOError.self).
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 108-108: Either remove or fill this block of code.
[warning] 113-113: Either remove or fill this block of code.
[warning] 103-103: Either remove or fill this block of code.
🤖 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 `@contrib/ghostty-control/Tests/ShareCLIGhosttyControlTests/LiveIOTests.swift`
around lines 100 - 114, Update the invalid-input tests around
LiveIOEventHub.subscribe to use Swift Testing `#expect`(throws:) instead of empty
catch blocks and Issue.record calls, asserting invalidChunkBytes and
invalidQueueCapacity as appropriate. If LiveIOError is not Equatable, assert the
error type rather than a concrete value. Also add a zero queueCapacity case if
subscribe rejects it.
Source: Linters/SAST tools
| #[cfg(target_os = "macos")] | ||
| { | ||
| println!( | ||
| "cargo:rustc-link-search=framework=/Library/Filesystems/macfuse.fs/Contents/Frameworks" | ||
| ); | ||
| println!("cargo:rustc-link-lib=framework=MFMount"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find references to MFMount / macFUSE FSKit symbols and any extern blocks in the fuse crate.
rg -n --type=rust -C3 'MFMount|mfmount|macfuse'
fd -e rs . crates/sharecli-fuse -x rg -n -C3 'extern\s+"C"' {}
# Confirm no other build script already links the framework.
fd -g 'build.rs' -x rg -n -C2 'rustc-link' {}Repository: KooshaPari/sharecli
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)build\.rs$|backend\.rs$|\.rs$|Cargo\.toml$' | sed -n '1,200p'
echo
echo "Track changed files:"
git status --short
echo
echo "Find sharecli-fuse files:"
fd -t f . crates/sharecli-fuse 2>/dev/null | sed -n '1,200p' || true
echo
echo "Search relevant identifiers broad:"
rg -n 'CARGO_CFG_TARGET_OS|target_os|MFMount|mfmount|macfuse|FileSystem|fuse|winfsp|windows|macos' crates/sharecli-fuse Cargo.toml crates 2>/dev/null | sed -n '1,240p' || true
echo
echo "build.rs files with rustc-link:"
fd -g 'build.rs' -x sh -c 'echo "--- $1"; rg -n -C2 "rustc-link|target_os|CARGO_CFG_TARGET_OS|MFMount|Framework" "$1" || true' sh {} | sed -n '1,240p'Repository: KooshaPari/sharecli
Length of output: 34981
Gate the macFUSE build-time linkage on target OS and framework presence.
crates/sharecli-fuse/build.rs runs on the host, so #[cfg(target_os = "macos")] still emits link flags when cross-compiling from macOS to another target. Check CARGO_CFG_TARGET_OS, and only emit the MFMount framework link flags when MFMount.framework exists. Add cargo:warning= for the missing framework case so the failure is not silent.
🤖 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 `@crates/sharecli-fuse/build.rs` around lines 9 - 15, Update the macOS linkage
block in build.rs to inspect CARGO_CFG_TARGET_OS at runtime rather than relying
on the build-script host cfg, and emit the MFMount framework search and link
directives only when the target OS is macOS and MFMount.framework exists at the
expected location. When the target is macOS but the framework is absent, emit a
cargo:warning= message instead of silently proceeding.
| fn fskit_approval_requested() -> bool { | ||
| #[cfg(target_os = "macos")] | ||
| { | ||
| std::env::var("SHARECLI_FUSE_FSKIT_APPROVED") | ||
| .map(|value| matches!(value.trim().to_ascii_lowercase().as_str(), "1" | "true" | "yes")) | ||
| .unwrap_or(false) | ||
| } | ||
| #[cfg(not(target_os = "macos"))] | ||
| { | ||
| false | ||
| } | ||
| if kernel_backend_loaded() { FuseBackend::Kernel } else { FuseBackend::Unavailable } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not describe the environment variable as verified approval.
fskit_approval_requested reads SHARECLI_FUSE_FSKIT_APPROVED. Any process in the same environment can set this variable. The doc comment on Line 85 states that "explicit operator approval were both verified", and FuseBackendDiagnostic::NoVerifiedBackend on Line 53 states "no verified FSKit approval". An environment variable is an operator opt-in, not a verification of system approval state.
Correct the wording so operators do not treat the flag as proof of FSKit approval. If a real approval signal exists (for example an FSKit extension enablement check), use it instead of the variable.
📝 Proposed doc correction
- /// Whether framework presence and explicit operator approval were both verified.
+ /// Whether the framework is present and the operator opted in via
+ /// `SHARECLI_FUSE_FSKIT_APPROVED`. This is an opt-in, not proof of FSKit approval.
pub fskit_approved: bool,🤖 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 `@crates/sharecli-fuse/src/backend.rs` around lines 199 - 210, Update the
wording around fskit_approval_requested and related NoVerifiedBackend
diagnostics to describe SHARECLI_FUSE_FSKIT_APPROVED only as operator intent or
opt-in, never as verified FSKit approval. Preserve the existing
environment-variable behavior unless a real system approval signal is already
available; if one exists, use that signal for verification instead.
| fn kernel_backend_loaded() -> bool { | ||
| #[cfg(not(target_os = "macos"))] | ||
| { | ||
| false | ||
| } | ||
| #[cfg(target_os = "macos")] | ||
| Command::new("kmutil") | ||
| .args(["showloaded"]) | ||
| .output() | ||
| .map(|output| String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse")) | ||
| .map(|output| { | ||
| String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse") | ||
| }) | ||
| .unwrap_or(false) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Log the kmutil probe failure.
kernel_backend_loaded maps every Command::output error to false through unwrap_or(false). The crate declares tracing as a dependency. The coding guidelines require structured logging for all errors. An operator who sees selected_backend: "non-fuse" currently cannot tell whether kmutil is absent, was denied, or reported no macFUSE kext.
As per coding guidelines: "Log all errors with structured logging."
🔧 Proposed fix
#[cfg(target_os = "macos")]
Command::new("kmutil")
.args(["showloaded"])
.output()
.map(|output| {
String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse")
})
- .unwrap_or(false)
+ .unwrap_or_else(|error| {
+ tracing::debug!(
+ error = %error,
+ probe = "kmutil showloaded",
+ "macFUSE kext probe failed; treating the KEXT backend as unavailable"
+ );
+ false
+ })
}📝 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.
| fn kernel_backend_loaded() -> bool { | |
| #[cfg(not(target_os = "macos"))] | |
| { | |
| false | |
| } | |
| #[cfg(target_os = "macos")] | |
| Command::new("kmutil") | |
| .args(["showloaded"]) | |
| .output() | |
| .map(|output| String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse")) | |
| .map(|output| { | |
| String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse") | |
| }) | |
| .unwrap_or(false) | |
| } | |
| fn kernel_backend_loaded() -> bool { | |
| #[cfg(not(target_os = "macos"))] | |
| { | |
| false | |
| } | |
| #[cfg(target_os = "macos")] | |
| Command::new("kmutil") | |
| .args(["showloaded"]) | |
| .output() | |
| .map(|output| { | |
| String::from_utf8_lossy(&output.stdout).to_ascii_lowercase().contains("macfuse") | |
| }) | |
| .unwrap_or_else(|error| { | |
| tracing::debug!( | |
| error = %error, | |
| probe = "kmutil showloaded", | |
| "macFUSE kext probe failed; treating the KEXT backend as unavailable" | |
| ); | |
| false | |
| }) | |
| } |
🤖 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 `@crates/sharecli-fuse/src/backend.rs` around lines 212 - 225, Update the macOS
branch of kernel_backend_loaded to handle Command::output errors explicitly: log
the probe failure with structured tracing, including the error details, then
return false. Preserve the existing successful-output detection and the
non-macOS behavior.
Source: Coding guidelines
| let stream = UnixStream::connect(&self.socket)?; | ||
| let mut writer = stream.try_clone()?; | ||
| let mut reader = BufReader::new(stream); | ||
| let id = REQUEST_ID.fetch_add(1, Ordering::Relaxed); | ||
| let mut params = json!({ | ||
| "max_chunk_bytes": max_chunk_bytes, | ||
| "queue_capacity": queue_capacity, | ||
| }); | ||
| if let Some(surface_id) = surface_id { | ||
| params["surface_id"] = Value::String(surface_id.to_owned()); | ||
| } | ||
| if let Some(from_seq) = from_seq { | ||
| params["from_seq"] = Value::Number(from_seq.into()); | ||
| } | ||
| let mut request = json!({ | ||
| "jsonrpc": "2.0", | ||
| "id": id, | ||
| "method": "surface.io.subscribe", | ||
| "params": params, | ||
| }); | ||
| if let Some(token) = &self.token { | ||
| request["token"] = Value::String(token.clone()); | ||
| } | ||
| serde_json::to_writer(&mut writer, &request)?; | ||
| writer.write_all(b"\n")?; | ||
| writer.flush()?; | ||
| let mut line = String::new(); | ||
| reader.read_line(&mut line)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set read and write timeouts on the control socket.
subscribe_surface connects and then calls reader.read_line for the acknowledgement with no deadline. request at Line 211 has the same gap. If the Ghostty side accepts the connection and never answers, the CLI blocks forever, and sharecli session watch stops making progress without any diagnostic.
Set read_timeout and write_timeout on the stream before the handshake. Keep the subscription stream itself blocking after the acknowledgement, or use a longer timeout there, because live events arrive at unpredictable intervals.
🛠️ Proposed fix
let stream = UnixStream::connect(&self.socket)?;
+ stream.set_write_timeout(Some(std::time::Duration::from_secs(5)))?;
+ stream.set_read_timeout(Some(std::time::Duration::from_secs(5)))?;
let mut writer = stream.try_clone()?;
let mut reader = BufReader::new(stream);Clear the read deadline after the acknowledgement is parsed, so next_event can wait for live output:
let ack: sharecli_session::SurfaceSubscribeAck =
serde_json::from_value(response.get("result").cloned().unwrap_or(Value::Null))?;
+ writer.set_read_timeout(None)?;
Ok(SurfaceSubscription {🤖 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 `@src/session.rs` around lines 294 - 321, Update both subscribe_surface and
request to configure read_timeout and write_timeout on the connected UnixStream
before sending the handshake, preventing indefinite blocking while awaiting
acknowledgement. After subscribe_surface parses the acknowledgement, clear the
read timeout so the subscription’s next_event flow remains blocking for live
events; preserve the existing request response handling and propagate timeout
errors consistently.
| fd_count: None, | ||
| thread_count: None, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Complete the ProcessInfo fixture before merging.
The ProcessInfo definition in src/runtime.rs also requires cpu_percent, ppid, cwd, env_count, state, disk_read_bytes, and disk_write_bytes. This literal has no struct-update expression, so the integration test does not compile. Initialize the missing fields or add a complete test-local fixture helper.
🤖 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 `@tests/fr007_ps_all_csv.rs` around lines 159 - 160, Complete the ProcessInfo
fixture in the test by initializing cpu_percent, ppid, cwd, env_count, state,
disk_read_bytes, and disk_write_bytes, or introduce a test-local helper that
supplies all required fields. Keep the existing fd_count and thread_count values
and ensure the literal compiles without relying on struct update syntax.
| let report: serde_json::Value = | ||
| serde_json::from_slice(&out.stdout).expect("fuse probe should emit valid JSON"); | ||
| assert!(report.get("selected_backend").is_some(), "report must include backend"); | ||
| assert_eq!(report.get("non_fuse_fallback"), Some(&serde_json::Value::Bool(true))); | ||
| assert!(report.get("mountpoint").is_some(), "report must include mountpoint"); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert the echoed mountpoint value, not only its presence.
Line 121 checks that the key exists. The test passes the mountpoint on the command line, so it can assert the exact value. That assertion pins the encoding of the field and catches a regression if the path is serialized differently.
♻️ Proposed change
assert!(report.get("selected_backend").is_some(), "report must include backend");
assert_eq!(report.get("non_fuse_fallback"), Some(&serde_json::Value::Bool(true)));
- assert!(report.get("mountpoint").is_some(), "report must include mountpoint");
+ assert_eq!(
+ report.get("mountpoint").and_then(serde_json::Value::as_str),
+ Some("/tmp/sharecli-fuse-probe-test"),
+ "report must echo the requested mountpoint"
+ );📝 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.
| let report: serde_json::Value = | |
| serde_json::from_slice(&out.stdout).expect("fuse probe should emit valid JSON"); | |
| assert!(report.get("selected_backend").is_some(), "report must include backend"); | |
| assert_eq!(report.get("non_fuse_fallback"), Some(&serde_json::Value::Bool(true))); | |
| assert!(report.get("mountpoint").is_some(), "report must include mountpoint"); | |
| let report: serde_json::Value = | |
| serde_json::from_slice(&out.stdout).expect("fuse probe should emit valid JSON"); | |
| assert!(report.get("selected_backend").is_some(), "report must include backend"); | |
| assert_eq!(report.get("non_fuse_fallback"), Some(&serde_json::Value::Bool(true))); | |
| assert_eq!( | |
| report.get("mountpoint").and_then(serde_json::Value::as_str), | |
| Some("/tmp/sharecli-fuse-probe-test"), | |
| "report must echo the requested mountpoint" | |
| ); |
🤖 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 `@tests/integration_cli.rs` around lines 117 - 121, Update the integration test
assertion for report.mountpoint to compare its value with the exact mountpoint
argument passed to the fuse probe, rather than only checking that the key
exists. Keep the existing selected_backend and non_fuse_fallback assertions
unchanged.
| fn session_watch_once_fails_open_when_native_socket_is_unavailable() { | ||
| let dir = TempDir::new().expect("tempdir"); | ||
| let db = dir.path().join("sessions.sqlite"); | ||
| let socket = dir.path().join("missing-ghostty.sock"); | ||
| let out = bin() | ||
| .args([ | ||
| "session", | ||
| "watch", | ||
| "--once", | ||
| "--socket", | ||
| socket.to_str().expect("utf8 path"), | ||
| "--db", | ||
| db.to_str().expect("utf8 path"), | ||
| ]) | ||
| .output() | ||
| .expect("spawn session watch"); | ||
|
|
||
| assert!(out.status.success()); | ||
| assert!(String::from_utf8_lossy(&out.stderr).contains("degraded")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate the watch test from ambient environment variables.
session watch declares state_sidecar with env = "SHARECLI_SESSION_SIDECAR", and token with env = "SHARECLI_GHOSTTY_TOKEN". The test does not pass --state-sidecar, so a developer or CI runner that exports SHARECLI_SESSION_SIDECAR makes the test read a real sidecar file instead of an isolated one. Pass an explicit sidecar path in the temporary directory and remove the related variables from the child environment.
Line 96 also asserts success without printing stderr. The other two tests in this file already include stderr in the failure message.
💚 Proposed fix
let socket = dir.path().join("missing-ghostty.sock");
+ let sidecar = dir.path().join("session-sidecar.jsonl");
let out = bin()
+ .env_remove("SHARECLI_SESSION_SIDECAR")
+ .env_remove("SHARECLI_GHOSTTY_TOKEN")
+ .env_remove("SHARECLI_GHOSTTY_SOCKET")
.args([
"session",
"watch",
"--once",
"--socket",
socket.to_str().expect("utf8 path"),
+ "--state-sidecar",
+ sidecar.to_str().expect("utf8 path"),
"--db",
db.to_str().expect("utf8 path"),
])
.output()
.expect("spawn session watch");
- assert!(out.status.success());
+ assert!(out.status.success(), "watch stderr: {}", String::from_utf8_lossy(&out.stderr));📝 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.
| fn session_watch_once_fails_open_when_native_socket_is_unavailable() { | |
| let dir = TempDir::new().expect("tempdir"); | |
| let db = dir.path().join("sessions.sqlite"); | |
| let socket = dir.path().join("missing-ghostty.sock"); | |
| let out = bin() | |
| .args([ | |
| "session", | |
| "watch", | |
| "--once", | |
| "--socket", | |
| socket.to_str().expect("utf8 path"), | |
| "--db", | |
| db.to_str().expect("utf8 path"), | |
| ]) | |
| .output() | |
| .expect("spawn session watch"); | |
| assert!(out.status.success()); | |
| assert!(String::from_utf8_lossy(&out.stderr).contains("degraded")); | |
| fn session_watch_once_fails_open_when_native_socket_is_unavailable() { | |
| let dir = TempDir::new().expect("tempdir"); | |
| let db = dir.path().join("sessions.sqlite"); | |
| let socket = dir.path().join("missing-ghostty.sock"); | |
| let sidecar = dir.path().join("session-sidecar.jsonl"); | |
| let out = bin() | |
| .env_remove("SHARECLI_SESSION_SIDECAR") | |
| .env_remove("SHARECLI_GHOSTTY_TOKEN") | |
| .env_remove("SHARECLI_GHOSTTY_SOCKET") | |
| .args([ | |
| "session", | |
| "watch", | |
| "--once", | |
| "--socket", | |
| socket.to_str().expect("utf8 path"), | |
| "--state-sidecar", | |
| sidecar.to_str().expect("utf8 path"), | |
| "--db", | |
| db.to_str().expect("utf8 path"), | |
| ]) | |
| .output() | |
| .expect("spawn session watch"); | |
| assert!( | |
| out.status.success(), | |
| "watch stderr: {}", | |
| String::from_utf8_lossy(&out.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 `@tests/session_cli.rs` around lines 79 - 97, Update
session_watch_once_fails_open_when_native_socket_is_unavailable to pass an
explicit --state-sidecar path under the existing TempDir, and remove
SHARECLI_SESSION_SIDECAR and SHARECLI_GHOSTTY_TOKEN from the child environment
to isolate the test. Also include captured stderr in the
assert!(out.status.success()) failure message, matching the neighboring tests.
| fn ghostty_control_client_requires_configured_socket() { | ||
| let client = GhosttyControlClient::new("/tmp/sharecli-no-ghostty.sock", Some("token".into())); | ||
| assert!(client.request("surface.list", serde_json::json!({})).is_err()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a unique temporary path for the "missing socket" tests.
Both tests hard-code a fixed path in /tmp. The other tests in this file already build a unique name from SystemTime under std::env::temp_dir(). If any process on the host binds /tmp/sharecli-no-ghostty.sock, the first test connects to it and blocks on read_line instead of failing fast. Use a TempDir path or the same nanosecond suffix pattern.
💚 Proposed fix
- let client = GhosttyControlClient::new("/tmp/sharecli-no-ghostty.sock", Some("token".into()));
+ let suffix =
+ SystemTime::now().duration_since(UNIX_EPOCH).expect("clock").as_nanos();
+ let socket = std::env::temp_dir().join(format!("sharecli-ghostty-absent-{suffix}.sock"));
+ let client = GhosttyControlClient::new(&socket, Some("token".into()));Also applies to: 147-150
🤖 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 `@tests/session.rs` around lines 40 - 42, Update both missing-socket tests,
including ghostty_control_client_requires_configured_socket and the additionally
referenced test, to use a unique nonexistent socket path under
std::env::temp_dir(), matching the file’s existing SystemTime nanosecond-suffix
pattern or a TempDir-based path. Remove the hard-coded
/tmp/sharecli-no-ghostty.sock path while preserving each test’s existing
assertions.
| } catch let error as ControlError { | ||
| if !requestHasID(line) { return Data() } | ||
| return encode(["jsonrpc": "2.0", "id": requestID(from: line), "error": ["code": error.code, "message": error.message]]) | ||
| } catch let error as LiveIOError { | ||
| if !requestHasID(line) { return Data() } | ||
| return encode(["jsonrpc": "2.0", "id": requestID(from: line), "error": ["code": -32602, "message": String(describing: error)]]) | ||
| } catch { | ||
| if !requestHasID(line) { return Data() } | ||
| return encode(["jsonrpc": "2.0", "id": requestID(from: line), "error": ["code": -32700, "message": "parse error: \(error)"]]) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Avoid re-parsing the request on every error path.
Each catch block calls requestHasID(line) and then requestID(from: line). Both run JSONSerialization.jsonObject again on the full payload. A malformed request up to maxRequestBytes (1 MiB) therefore costs three full JSON parses. A client can send such requests in a loop and amplify CPU cost three times over the success path.
Parse the payload once at the top of dispatch, then reuse the extracted id and notification flag in the error paths.
🤖 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 `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift` around
lines 161 - 170, Update dispatch to parse the request payload once at its start
and extract the request ID plus notification status into reusable values.
Replace the repeated requestHasID(line) and requestID(from: line) calls in each
catch block with those values, while preserving the existing error codes,
messages, and notification responses.
| private func dispatch(method: String, params: [String: Any]) async throws -> Any { | ||
| switch method { | ||
| case "surface.list": | ||
| return try jsonObject(await provider.listSurfaces()) | ||
| case "surface.io.send": | ||
| let surfaceID = try stringParam(params, "surface_id") | ||
| let text = params["text"] as? String | ||
| let bytes = params["bytes"] as? [Any] | ||
| guard (text != nil) != (bytes != nil) else { | ||
| throw ControlError.invalidParams("exactly one of params.text or params.bytes is required") | ||
| } | ||
| let payload = try text.map { Array($0.utf8) } ?? bytesToUInt8(bytes!) | ||
| guard payload.count <= Self.maxSendBytes else { | ||
| throw ControlError.invalidParams("payload must not exceed 65536 bytes") | ||
| } | ||
| try await provider.send(surfaceID: surfaceID, bytes: payload) | ||
| return NSNull() | ||
| case "surface.io.read": | ||
| let surfaceID = try stringParam(params, "surface_id") | ||
| let maxBytes = try intParam(params, "max_bytes") | ||
| guard maxBytes >= 0 && maxBytes <= Self.maxReadBytes else { | ||
| throw ControlError.invalidParams("max_bytes must be between 0 and 1048576") | ||
| } | ||
| let bytes = try await provider.read(surfaceID: surfaceID, maxBytes: maxBytes) | ||
| guard bytes.count <= maxBytes else { | ||
| throw ControlError.provider("surface provider returned more bytes than requested") | ||
| } | ||
| return ["bytes": bytes] | ||
| case "surface.io.resize": | ||
| let surfaceID = try stringParam(params, "surface_id") | ||
| let rows = try uint16Param(params, "rows") | ||
| let cols = try uint16Param(params, "cols") | ||
| guard rows > 0 && cols > 0 else { | ||
| throw ControlError.invalidParams("rows and cols must be greater than zero") | ||
| } | ||
| try await provider.resize(surfaceID: surfaceID, rows: rows, cols: cols) | ||
| return NSNull() | ||
| case "surface.io.capabilities": | ||
| return try jsonObject(await provider.capabilities(surfaceID: stringParam(params, "surface_id"))) | ||
| case "surface.io.subscribe": | ||
| guard let liveEvents else { throw ControlError.liveIO("live surface events unavailable") } | ||
| let surfaceID = params["surface_id"] as? String | ||
| let fromSequence = try optionalUInt64Param(params, "from_seq") | ||
| let maxChunkBytes = try intParamOrDefault(params, "max_chunk_bytes", default: LiveIOEventHub.maxChunkBytes) | ||
| let queueCapacity = try intParamOrDefault(params, "queue_capacity", default: 64) | ||
| let subscription = try await liveEvents.subscribe( | ||
| surfaceID: surfaceID, | ||
| fromSequence: fromSequence, | ||
| maxChunkBytes: maxChunkBytes, | ||
| queueCapacity: queueCapacity | ||
| ) | ||
| let nextSequence = await liveEvents.nextSequenceNumber() | ||
| return [ | ||
| "subscription_id": subscription.id, | ||
| "next_seq": max(nextSequence, fromSequence ?? 0), | ||
| "capabilities": [ | ||
| "max_chunk_bytes": maxChunkBytes, | ||
| "queue_capacity": queueCapacity, | ||
| "replay": false, | ||
| ], | ||
| ] | ||
| case "surface.io.unsubscribe": | ||
| guard let liveEvents else { throw ControlError.liveIO("live surface events unavailable") } | ||
| let subscriptionID = try uint64Param(params, "subscription_id") | ||
| return ["unsubscribed": await liveEvents.unsubscribe(subscriptionID: subscriptionID)] | ||
| default: | ||
| throw ControlError.methodNotFound(method) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers that send layout methods over the Ghostty control socket.
set -euo pipefail
rg -n 'surface\.layout\.(snapshot|restore)' -g '!**/target/**'Repository: KooshaPari/sharecli
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## dispatch cases and method call sites"
rg -n 'dispatch\(|methodNotFound|surface\.layout|surface\.io\.(snap|restore)|"method"' . \
--glob '!**/target/**' \
--glob '!**/build/**' \
--glob '!**/.build/**' \
--glob '!**/node_modules/**'
echo
echo "## Rust dispatcher relevant section"
fd -a 'rpc.rs' . | while read -r f; do
if rg -q 'surface\.layout\.(snapshot|restore)' "$f"; then
echo "FILE: ${f#"$PWD"/}"
nl -ba "$f" | sed -n '290,350p'
fi
done
echo
echo "## Swift control file relevant sections"
for f in contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift; do
if [ -f "$f" ]; then
echo "FILE: $f"
nl -ba "$f" | sed -n '1,260p'
fi
doneRepository: KooshaPari/sharecli
Length of output: 19190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Rust rpc.rs dispatch section"
sed -n '300,345p' crates/sharecli-session/src/rpc.rs
echo
echo "## Swift Control.swift dispatch section"
sed -n '165,245p' contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift
echo
echo "## Rust session client layout call sites"
sed -n '245,285p' src/session.rs
echo
echo "## Read-only structural search for layout method literals and dispatch switch cases"
python3 - <<'PY'
from pathlib import Path
for p in map(Path, [
"src/session.rs",
"contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift",
"crates/sharecli-session/src/rpc.rs",
]):
print(f"\nFILE {p}")
s = p.read_text(encoding="utf-8")
for needle in ["surface.layout.snapshot", "surface.layout.restore"]:
print(f"{needle}: {s.count(needle)}")
PYRepository: KooshaPari/sharecli
Length of output: 8725
Handle Ghostty layout requests on the Swift dispatcher.
src/session.rs sends surface.layout.snapshot and surface.layout.restore, but Dispatch.dispatch(method:params:) in contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift only has native -32601 cases for them. Add these dispatch cases, or block Rust session requests from reaching the native socket.
🤖 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 `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/Control.swift` around
lines 173 - 241, Update dispatch(method:params:) to handle
surface.layout.snapshot and surface.layout.restore requests sent by
src/session.rs, instead of falling through to methodNotFound. Implement the
corresponding provider-backed request/response handling using the existing
layout symbols and parameter validation conventions, or explicitly prevent these
Rust session requests from reaching the native socket if native handling is
intentionally unsupported.
| let stream = AsyncStream<LiveIOEvent>(bufferingPolicy: .bufferingNewest(queueCapacity)) { | ||
| continuation = $0 | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Swift AsyncStream Continuation yield return value bufferingNewest versus bufferingOldest dropped enqueued
💡 Result:
In Swift, AsyncStream.Continuation.yield(_:) returns a YieldResult enum—enqueued, dropped, or terminated—which indicates how the stream handled the yielded value based on the chosen BufferingPolicy [1]. When a buffer reaches its capacity, the BufferingPolicy determines the fate of subsequent yielded values: - bufferingNewest(_:): When the buffer is full, the stream discards the oldest element currently in the buffer to make room for the newly yielded element [2][3]. This ensures the stream retains the most recent values [4][5]. - bufferingOldest(_:): When the buffer is full, the stream discards the newly received element [2][3]. This preserves the initial, older values in the buffer and ignores new incoming data until space becomes available [4][5]. The YieldResult returned by yield(_:) confirms the outcome: enqueued signifies the value was buffered or immediately consumed by an active iterator, while dropped indicates the value was discarded because the buffer limit was reached [1]. If the stream has already finished or been cancelled, terminated is returned [3][1]. If the buffer limit is set to zero or a negative number for either policy, no values are buffered; elements are only received if an iterator is actively awaiting a value at the exact moment yield is called [3].
Citations:
- 1: https://github.com/swiftlang/swift-evolution/blob/5d03375330e975cec9aa105eae257179b4f91a98/proposals/0314-async-stream.md
- 2: https://developer.apple.com/documentation/swift/asyncstream/continuation/bufferingpolicy?changes=_6
- 3: https://github.com/apple/swift/blob/main/stdlib/public/Concurrency/AsyncStream.swift
- 4: https://www.hackingwithswift.com/quick-start/concurrency/how-to-create-and-use-asyncstreams-to-return-buffered-data
- 5: https://github.com/prisma-labs-dev/apple-skills/blob/main/skills/guide-swift-concurrency/references/async-streams.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching LiveIO.swift:\n'
fd -a 'LiveIO.swift$' . || true
file="$(fd 'LiveIO\.swift$' . | head -n 1)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nRelevant lines 1-230:\n'
cat -n "$file" | sed -n '1,230p'
fi
printf '\nSearch for LiveIOEvent/dropped/resync_required:\n'
rg -n "LiveIOEvent|dropped|resync_required|seq|BufferingPolicy|bufferingOldest|bufferingNewest|yield\\\(" "$file" || trueRepository: KooshaPari/sharecli
Length of output: 8555
Report lost events when the subscriber buffer fills.
.bufferingNewest(queueCapacity) lets new events replace the oldest buffered event and does not return .enqueued(remaining:)/.dropped, so state.dropped stays at 0. If a subscriber falls behind, events are silently overwritten and the Rust client only sees a seq discontinuity. Switch to the buffering policy that supports loss signaling, or report/accumulate the discard state yourself.
🤖 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 `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift` around
lines 111 - 113, Update the AsyncStream buffering setup in LiveIO’s stream
initialization to use a policy that exposes discarded-event results, or
otherwise track each overwritten event and accumulate it into state.dropped.
Ensure subscriber buffer loss is reported to the Rust client rather than only
appearing as a seq discontinuity.
| let limits = subscriptions.values | ||
| .filter { $0.surfaceID == nil || $0.surfaceID == surfaceID } | ||
| .map(\.maxChunkBytes) | ||
| let chunkSize = limits.min() ?? Self.maxChunkBytes | ||
| let chunks: [[UInt8]] = bytes.isEmpty ? [[]] : stride(from: 0, to: bytes.count, by: chunkSize).map { | ||
| Array(bytes[$0 ..< min($0 + chunkSize, bytes.count)]) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
One subscriber's chunk limit constrains all subscribers.
chunkSize is the minimum maxChunkBytes across every matching subscription. A single subscriber that requests a small max_chunk_bytes forces that chunk size on all other subscribers of the same surface. It also inflates the global nextSequence, because each chunk consumes one sequence number for everyone. A subscriber that asks for 1024-byte chunks turns a 64 KiB write into 64 events for a subscriber that asked for the full 64 KiB.
Chunk per subscription instead, using each subscription's own maxChunkBytes.
🤖 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 `@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/LiveIO.swift` around
lines 142 - 148, Update the chunking logic in the subscription delivery path to
generate chunks separately for each matching subscription using that
subscription’s own maxChunkBytes. Remove the shared limits/minimum chunkSize
calculation, and ensure each subscriber receives appropriately sized chunks
without another subscriber’s limit affecting its events or sequence progression.
| public func stop() { | ||
| lock.lock() | ||
| let activeSource = source | ||
| source = nil | ||
| listenerFD = -1 | ||
| activeSource?.cancel() | ||
| unlink(path) | ||
| lock.unlock() | ||
| } | ||
|
|
||
| private func acceptConnections() { | ||
| while true { | ||
| let fd = Darwin.accept(listenerFD, nil, nil) | ||
| if fd < 0 { | ||
| if errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR { return } | ||
| return | ||
| } | ||
| guard peerBelongsToCurrentUser(fd) else { | ||
| Darwin.close(fd) | ||
| continue | ||
| } | ||
| Task.detached(priority: .userInitiated) { [weak self] in | ||
| await self?.serveConnection(fd) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
listenerFD is read without the lock.
acceptConnections runs on queue and reads listenerFD at line 74. stop() writes listenerFD = -1 at line 66 under lock, then cancels the source, and the cancel handler closes the descriptor. The read at line 74 is not synchronized with that write, so it is a data race on a field of an @unchecked Sendable class.
The failure is not only a torn read. An acceptConnections iteration that already loaded the old descriptor value can call accept after the cancel handler closed it. If the process reopens any file in that window, the descriptor number is reused and accept targets an unrelated descriptor.
Read listenerFD under lock at the top of acceptConnections, and return when it is negative.
🔒️ Proposed fix
private func acceptConnections() {
while true {
- let fd = Darwin.accept(listenerFD, nil, nil)
+ lock.lock()
+ let listener = listenerFD
+ lock.unlock()
+ guard listener >= 0 else { return }
+ let fd = Darwin.accept(listener, nil, nil)🤖 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
`@contrib/ghostty-control/Sources/ShareCLIGhosttyControl/UnixControlServer.swift`
around lines 62 - 87, Update acceptConnections to acquire lock before reading
listenerFD, copy the descriptor to a local value, then release the lock and
return immediately when that value is negative; use the local descriptor for
Darwin.accept. Keep stop’s synchronized listenerFD update unchanged.
| let request_value = serde_json::from_str::<Value>(line).ok(); | ||
| let should_reply = serde_json::from_str::<Value>(line) | ||
| .ok() | ||
| .and_then(|request| request.as_object().map(|object| object.contains_key("id"))) | ||
| .unwrap_or(false); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Parse the request line once.
serde_json::from_str::<Value>(line) runs twice on the same input. Reuse request_value to compute should_reply.
♻️ Proposed refactor
let request_value = serde_json::from_str::<Value>(line).ok();
- let should_reply = serde_json::from_str::<Value>(line)
- .ok()
+ let should_reply = request_value
+ .as_ref()
.and_then(|request| request.as_object().map(|object| object.contains_key("id")))
.unwrap_or(false);📝 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.
| let request_value = serde_json::from_str::<Value>(line).ok(); | |
| let should_reply = serde_json::from_str::<Value>(line) | |
| .ok() | |
| .and_then(|request| request.as_object().map(|object| object.contains_key("id"))) | |
| .unwrap_or(false); | |
| let request_value = serde_json::from_str::<Value>(line).ok(); | |
| let should_reply = request_value | |
| .as_ref() | |
| .and_then(|request| request.as_object().map(|object| object.contains_key("id"))) | |
| .unwrap_or(false); |
🤖 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 `@crates/sharecli-session/src/rpc.rs` around lines 456 - 460, Update the
request parsing logic to derive should_reply from the existing request_value
result instead of calling serde_json::from_str::<Value>(line) a second time.
Preserve the current object and "id" presence checks and false fallback for
invalid or non-object requests.
| fn records(&self) -> Result<Vec<SidecarRecord>> { | ||
| let text = match std::fs::read_to_string(&self.path) { | ||
| Ok(text) => text, | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), | ||
| Err(error) => { | ||
| return Err(error).with_context(|| format!("read sidecar {}", self.path.display())) | ||
| } | ||
| }; | ||
| text.lines() | ||
| .enumerate() | ||
| .filter(|(_, line)| !line.trim().is_empty()) | ||
| .map(|(line_number, line)| { | ||
| serde_json::from_str(line).with_context(|| { | ||
| format!("parse sidecar {} line {}", self.path.display(), line_number + 1) | ||
| }) | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One malformed JSONL line disables every sidecar lookup.
records collects into Result<Vec<_>>, so a single unparsable line makes the whole call fail. The sidecar is append-only and is written by external launchers and wrappers, so a truncated final line is a realistic outcome of a crash during a write. scan_and_record in crates/sharecli-session/src/discovery.rs propagates this error with ? at Line 142, which aborts the entire discovery pass for every surface.
Skip unparsable lines and log each one with structured logging, as the coding guidelines require. Valid mappings then remain usable.
🐛 Proposed fix
- text.lines()
- .enumerate()
- .filter(|(_, line)| !line.trim().is_empty())
- .map(|(line_number, line)| {
- serde_json::from_str(line).with_context(|| {
- format!("parse sidecar {} line {}", self.path.display(), line_number + 1)
- })
- })
- .collect()
+ let mut records = Vec::new();
+ for (line_number, line) in text.lines().enumerate() {
+ if line.trim().is_empty() {
+ continue;
+ }
+ match serde_json::from_str::<SidecarRecord>(line) {
+ Ok(record) => records.push(record),
+ Err(error) => tracing::warn!(
+ sidecar = %self.path.display(),
+ line = line_number + 1,
+ %error,
+ "skipping unparsable sidecar record"
+ ),
+ }
+ }
+ Ok(records)As per coding guidelines: "Log all errors with structured logging."
📝 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.
| fn records(&self) -> Result<Vec<SidecarRecord>> { | |
| let text = match std::fs::read_to_string(&self.path) { | |
| Ok(text) => text, | |
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), | |
| Err(error) => { | |
| return Err(error).with_context(|| format!("read sidecar {}", self.path.display())) | |
| } | |
| }; | |
| text.lines() | |
| .enumerate() | |
| .filter(|(_, line)| !line.trim().is_empty()) | |
| .map(|(line_number, line)| { | |
| serde_json::from_str(line).with_context(|| { | |
| format!("parse sidecar {} line {}", self.path.display(), line_number + 1) | |
| }) | |
| }) | |
| .collect() | |
| } | |
| fn records(&self) -> Result<Vec<SidecarRecord>> { | |
| let text = match std::fs::read_to_string(&self.path) { | |
| Ok(text) => text, | |
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(Vec::new()), | |
| Err(error) => { | |
| return Err(error).with_context(|| format!("read sidecar {}", self.path.display())) | |
| } | |
| }; | |
| let mut records = Vec::new(); | |
| for (line_number, line) in text.lines().enumerate() { | |
| if line.trim().is_empty() { | |
| continue; | |
| } | |
| match serde_json::from_str::<SidecarRecord>(line) { | |
| Ok(record) => records.push(record), | |
| Err(error) => tracing::warn!( | |
| sidecar = %self.path.display(), | |
| line = line_number + 1, | |
| %error, | |
| "skipping unparsable sidecar record" | |
| ), | |
| } | |
| } | |
| Ok(records) | |
| } |
🤖 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 `@crates/sharecli-session/src/state.rs` around lines 69 - 86, Update
SidecarState::records to continue processing after serde_json parsing failures
instead of collecting into a fallible Vec; log each malformed line with
structured logging including the sidecar path, line number, and error, then skip
it so valid SidecarRecord entries remain available to scan_and_record.
Source: Coding guidelines
| impl SessionStateProvider for SidecarStateProvider { | ||
| fn session_id(&self, surface: &SurfaceRecord, harness: &str) -> Result<Option<String>> { | ||
| let pid = surface.process.as_ref().and_then(|process| process.pid); | ||
| let mut match_record = None; | ||
| for record in self.records()? { | ||
| if record.surface_id != surface.id || record.harness != harness { | ||
| continue; | ||
| } | ||
| // JSONL is append-only: the last record for a surface/harness is | ||
| // authoritative. A newer PID mismatch must not fall back to an | ||
| // older mapping from a recycled process. | ||
| match_record = Some(record); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
The sidecar file is read and parsed once per surface in a scan.
session_id calls self.records(), which reads and parses the whole JSONL file. scan_and_record in crates/sharecli-session/src/discovery.rs calls session_id once for every discovered surface at Line 141. A scan of N surfaces therefore performs N full file reads and N full parses. The sidecar is append-only and grows without bound, so the cost grows over the lifetime of the state file.
The doc comment at Line 59 requires a re-read so a long-running watcher observes new registrations. Keep that property with a cheaper mechanism, for example cache the parsed records and invalidate on the file's modification time and length.
🤖 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 `@crates/sharecli-session/src/state.rs` around lines 89 - 101, Update
SidecarStateProvider::session_id and its record-loading path so repeated lookups
during a scan reuse parsed JSONL records instead of rereading and reparsing the
file for every surface. Cache the records and invalidate them when the sidecar
file’s modification time or length changes, preserving the documented re-read
behavior for newly appended registrations.
| #[test] | ||
| fn layout_snapshot_round_trips_across_store_reopen() { | ||
| let suffix = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos(); | ||
| let path = std::env::temp_dir().join(format!("sharecli-layout-{suffix}.sqlite")); | ||
| let expected = snapshot(); | ||
| let store = SessionStore::open(&path).unwrap(); | ||
| store.save_layout(&expected).unwrap(); | ||
| drop(store); | ||
|
|
||
| let reopened = SessionStore::open(&path).unwrap(); | ||
| assert_eq!(reopened.get_layout("daily").unwrap(), Some(expected)); | ||
| let _ = std::fs::remove_file(path); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Temp SQLite cleanup misses the WAL and SHM sidecar files in both tests. SessionStore::init sets journal_mode=WAL, so every file-backed store creates <path>-wal and <path>-shm beside the database. Both tests remove only the main database file, so two sidecar files leak into the temp directory on every run.
crates/sharecli-session/tests/layout.rs#L20-L32: dropreopenedbefore cleanup, then remove<path>,<path>-wal, and<path>-shm, or replace the manual path with a temp-directory guard that deletes the whole directory on drop.crates/sharecli-session/src/ledger.rs#L74-L94: apply the same cleanup at Line 93 after droppingreopened.
📍 Affects 2 files
crates/sharecli-session/tests/layout.rs#L20-L32(this comment)crates/sharecli-session/src/ledger.rs#L74-L94
🤖 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 `@crates/sharecli-session/tests/layout.rs` around lines 20 - 32, Update the
cleanup in crates/sharecli-session/tests/layout.rs lines 20-32 and
crates/sharecli-session/src/ledger.rs lines 74-94: drop reopened before cleanup,
then remove the main SQLite path and its -wal and -shm sidecar files, or use a
temporary-directory guard that removes the directory on drop.
| #[test] | ||
| fn invalid_split_ratio_is_rejected_before_persistence() { | ||
| let store = SessionStore::open_memory().unwrap(); | ||
| let mut invalid = snapshot(); | ||
| invalid.root = LayoutNode::Split { | ||
| axis: LayoutAxis::Vertical, | ||
| ratio_millis: 0, | ||
| children: vec![ | ||
| LayoutNode::Pane { surface_id: "ghostty:1".to_string() }, | ||
| LayoutNode::Pane { surface_id: "ghostty:2".to_string() }, | ||
| ], | ||
| }; | ||
|
|
||
| assert!(store.save_layout(&invalid).unwrap_err().to_string().contains("ratio")); | ||
| assert_eq!(store.get_layout("daily").unwrap(), None); | ||
| } | ||
|
|
||
| #[test] | ||
| fn duplicate_surface_is_rejected() { | ||
| let store = SessionStore::open_memory().unwrap(); | ||
| let mut invalid = snapshot(); | ||
| invalid.root = LayoutNode::Split { | ||
| axis: LayoutAxis::Horizontal, | ||
| ratio_millis: 500, | ||
| children: vec![ | ||
| LayoutNode::Pane { surface_id: "ghostty:1".to_string() }, | ||
| LayoutNode::Pane { surface_id: "ghostty:1".to_string() }, | ||
| ], | ||
| }; | ||
|
|
||
| assert!(store.save_layout(&invalid).unwrap_err().to_string().contains("more than once")); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for layout replacement and list_layouts.
save_layout in crates/sharecli-session/src/layout.rs uses ON CONFLICT(id) DO UPDATE, and list_layouts returns rows in ID order. Neither path is covered here. Add a test that saves two snapshots with the same id and asserts the second one replaces the first, and a test that asserts list_layouts returns snapshots sorted by ID.
🤖 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 `@crates/sharecli-session/tests/layout.rs` around lines 34 - 65, Add tests
alongside invalid_split_ratio_is_rejected_before_persistence and
duplicate_surface_is_rejected covering SessionStore::save_layout replacement:
save two snapshots with the same id but different content, then assert
get_layout returns the second snapshot. Add a separate test for
SessionStore::list_layouts that saves snapshots with IDs in non-sorted order and
asserts the returned snapshots are ordered by ID.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| if let Some(error) = value.get("error") { | ||
| anyhow::bail!("Ghostty live event RPC failed: {error}"); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
WARNING: Unbounded retry loop in SurfaceSubscription::next_event
Any server message that is neither surface.io.event nor contains error triggers an unconditional continue with no timeout, backoff, or maximum skip count. A misbehaving or buggy server can spin the client forever consuming 100% CPU.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| self.writer.write_all(b"\n")?; | ||
| self.writer.flush()?; | ||
| let mut line = String::new(); | ||
| self.reader.read_line(&mut line)?; |
There was a problem hiding this comment.
WARNING: SurfaceSubscription::unsubscribe blocks indefinitely on read_line
After writing the unsubscribe request, read_line has no timeout. If the Ghostty server crashes or stalls, the client hangs forever with no recovery path.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| stream.write_all(b"\n")?; | ||
| stream.flush()?; | ||
| let mut response = String::new(); | ||
| BufReader::new(stream).read_line(&mut response)?; |
There was a problem hiding this comment.
SUGGESTION: GhosttyControlClient::request has no read timeout
BufReader::new(stream).read_line(&mut response)? blocks indefinitely if the Ghostty server stalls after accepting the connection. A hung server causes the client to hang.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if !(1..=MAX_EVENT_QUEUE_CAPACITY).contains(&queue_capacity) { | ||
| anyhow::bail!("queue_capacity must be between 1 and {MAX_EVENT_QUEUE_CAPACITY}"); | ||
| } | ||
| let stream = UnixStream::connect(&self.socket)?; |
There was a problem hiding this comment.
SUGGESTION: UnixStream::connect has no timeout
UnixStream::connect(&self.socket)? blocks indefinitely if the socket's listen backlog is full or the server is not accepting connections. During server startup or high load, the client could hang.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if once { | ||
| return Ok(()); | ||
| } | ||
| std::thread::sleep(interval); |
There was a problem hiding this comment.
WARNING: std::thread::sleep blocks the async runtime
run_session_watch is an async function but calls std::thread::sleep(interval) at the bottom of the watch loop. This blocks the tokio runtime worker thread for the entire sleep duration, preventing it from serving other tasks.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| /// Linux uses `/proc` first; macOS and other Unix systems fall back to lsof. | ||
| fn count_open_fds(pid: u32) -> Option<u32> { | ||
| #[cfg(target_os = "linux")] | ||
| if let Ok(entries) = std::fs::read_dir(format!("/proc/{pid}/fd")) { |
There was a problem hiding this comment.
WARNING: Blocking I/O in count_open_fds called from async context
std::fs::read_dir(format!("/proc/{pid}/fd")) is blocking I/O invoked from ProcessInfo::from_sysinfo, which is called by the async ProcessPool::list(). This blocks the tokio runtime thread without spawn_blocking.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| let mut state = self.state.lock().expect("surface event hub mutex poisoned"); | ||
| state.next_subscription = state.next_subscription.saturating_add(1); | ||
| let subscription_id = state.next_subscription; | ||
| let next_seq = state.next_seq.saturating_add(1).max(request.from_seq.unwrap_or(1)); |
There was a problem hiding this comment.
SUGGESTION: from_seq: u64::MAX causes permanent sequence number stall
If a client requests from_seq: u64::MAX, next_seq becomes u64::MAX and subsequent seq = seq.saturating_add(1) stays at u64::MAX. All future events share the same sequence number, breaking client-side deduplication.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 239.8K · Output: 52.8K · Cached: 7.5M |
|
Triage note from merge orchestration (2026-08-08): This branch currently shares NO merge-base with main (GitHub reports mergeable=CONFLICTING / mergeStateStatus=DIRTY), so a normal merge or rebase is impossible until the branch is re-rooted onto current main. Unlike the sibling lanes (which were confirmed duplicates and closed), the Ghostty content in this PR is genuinely NOT on main yet: Recommended next step for the owning lane: rebuild this branch from current main (e.g. |
|
Closing as superseded. Branch commits already in main. |
) Surgically re-land the Ghostty control-plane feature from the orphaned PR #647 (9f8a5c4, fork fbacc7d) onto current main, isolating only the ghostty/session-control content and skipping functionality main already landed independently (FR-007 thread metrics, process.cmdline IPC, fuse backend rewrite, dashboard assets). Refs FR-007.
) (#703) Surgically re-land the Ghostty control-plane feature from the orphaned PR #647 (9f8a5c4, fork fbacc7d) onto current main, isolating only the ghostty/session-control content and skipping functionality main already landed independently (FR-007 thread metrics, process.cmdline IPC, fuse backend rewrite, dashboard assets). Refs FR-007.
User description
Substantial feature work adding Ghostty terminal session control:
CodeAnt-AI Description
Add a durable, shell-free control plane for Ghostty surfaces and agent sessions
What Changed
/Volumes, or non-FUSE fallback, with a read-only probe and diagnostics; fixes missing FUSE imports that blocked buildsImpact
✅ Shell-free Ghostty surface control✅ Safer session recovery with fewer false resumes✅ FUSE mounts fail open with clear backend diagnostics💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.