Add recursive memory attribution diagnostics - #4437
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new cmux memory diagnostic pipeline: proc_listallpids-based process enumeration with caching, grouped recursive child RSS with PID→workspace/pane/surface attribution, server RPC ChangesMemory Diagnostics with Child-Process Attribution
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "cmux (memory)"
participant Server as "TerminalController.v2SystemMemory"
participant Cache as "CmuxTopProcessSnapshotCache"
participant Snapshot as "CmuxTopProcessSnapshot"
participant Annotator as "v2TopMemoryAttributionByPID"
User->>CLI: run `cmux memory` (flags)
CLI->>Server: sendV2(system.memory, params)
Server->>Cache: captureCached(includeProcessDetails:false, maximumAge)
Cache->>Snapshot: return cached or build new snapshot
Server->>Annotator: build PID→attribution from annotatedWindows
Snapshot->>Server: memoryDiagnosticPayload(with attributions, topGroupLimit)
Server-->>CLI: { sample, memory_diagnostic }
CLI-->>User: render JSON or formatted text
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10262-10283: The new "memory" help text currently returned in the
case "memory" switch is a hard-coded English multiline string; replace this
literal with calls to the app's localization API (e.g., NSLocalizedString or the
project's localized wrapper) for each segment (Usage, Flags, Output, Example and
each flag description), use localized plural-aware APIs for "process/processes"
counts, and wire the resulting localized strings together in the same return
path (case "memory") and in the other affected loci (around lines referenced:
11709-11715, 11732-11742, 11882-11883, 12497-12560, 26418) so all user-facing
text is localized; then add matching entries to the string catalogs for every
supported locale (including plural forms) with keys unique to this command
(e.g., "memory_usage", "memory_flag_all", "memory_output_app_footprint",
"memory_example") so translators can provide equivalents for all locales.
- Around line 11893-11894: The catch block that matches CLIError by checking
error.message.hasPrefix("method_not_found:") should not expose the internal
method name; update the thrown CLIError message in that catch (the code using
error.message.hasPrefix(...) and throwing CLIError(...)) to a user-facing
message such as "cmux build is too old or lacks memory diagnostics support" or
similar that omits "system.memory" or any internal provider identifiers.
In `@Resources/Localizable.xcstrings`:
- Around line 113213-113225: The new localization keys
taskManager.summary.appFootprint and taskManager.summary.childRSS only include
en/ja; add entries for every locale already supported by this xcstrings catalog
following the repo’s accepted fallback pattern (create the same key under each
locale with translated values or the fallback marker/value used elsewhere),
ensuring all locale slots are present for both keys (also apply the same fix for
the other occurrences mentioned around the 113283–113317 range).
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 328-370: The file exceeds size guidelines because new
diagnostics/enumeration helpers were added; split these into sibling Swift
source files by extracting the diagnostic aggregation, cache/state logic,
summary rendering, and process-enumeration helpers (e.g., functions/types
referenced around memoryDiagnosticPayload such as memoryDiagnosticGroups,
memoryDiagnosticSummaryText, CmuxTopProcessAttribution, descendantPIDs,
summaryPayload/summary and any new cache or aggregation types) into separate
files under Sources (for example DiagnosticsAggregation.swift,
MemorySummary.swift, ProcessEnumeration.swift, DiagnosticsCache.swift); move
related helper methods and small extensions there, preserve internal/public
access levels, add necessary imports and update any file-private references to
allow access from CmuxTopSnapshot, and run/build tests to ensure no symbol or
visibility breakages.
- Around line 844-857: The diagnostic summary built in memoryDiagnosticPayload()
uses hard-coded English text and units; replace the inline string concatenation
that builds summary (the initial var summary and the appended "; top child
group: ... from workspace ...") with a localized format looked up via
NSLocalizedString or String(localized:) (e.g. use a key like
"MemoryDiagnosticSummary" with placeholders) and build the string with
String.localizedStringWithFormat or String(format:locale:), passing formatted
values from Self.formatDiagnosticBytes(rssBytes)/childRSS/appFootprintBytes and
topGroup["name"]/workspace; also ensure formatDiagnosticBytes returns localized
units (or switch to ByteCountFormatter with current locale) and add the new
key(s) to the string catalogs for every supported locale.
- Around line 998-1013: The code misinterprets proc_listallpids(nil, 0) as a
byte count and divides by pidStride, under-allocating the PID buffer; in
allBSDProcesses() treat the initial return as a PID count (rename byteCount ->
pidCount for clarity), allocate pids with count = max(1, pidCount + 32) (not
pidCount / pidStride), and keep passing the buffer size in bytes to
proc_listallpids (buffer.count * pidStride); also ensure returnedBytes is
treated as a PID count when computing count = min(pids.count,
Int(returnedBytes)) so bsdInfo(for:) iterates the correct number of PIDs.
In `@Sources/TaskManagerSnapshot.swift`:
- Around line 129-132: The code unconditionally sets terminalSurfaceId to
surfaceId, causing non-terminal surfaces to show "View Terminal"; update the
initializer/creation site that assigns terminalSurfaceId (the code block that
sets workspaceId, surfaceId, terminalSurfaceId, processId) to only set
terminalSurfaceId when the surface is actually a terminal—e.g., check the
surface's type/attribution (use the existing surface object or helper like
isTerminalSurface/Surface.kind/.isTerminal) and set terminalSurfaceId =
surfaceId only if that check is true, otherwise set terminalSurfaceId = nil.
- Around line 63-71: The convenience initializer currently always passes an
empty array for childMemoryRows, which can create inconsistent snapshots when a
memoryDiagnostic is provided; change the call so childMemoryRows is derived from
the provided memoryDiagnostic (e.g. replace the literal [] with a computed value
like Self.childMemoryRows(from: memoryDiagnostic) or memoryDiagnostic.flatMap {
Self.childMemoryRows(from: $0) } ?? []) so that when memoryDiagnostic is non-nil
the initializer computes and supplies the appropriate childMemoryRows while
falling back to an empty array when absent; update or add the helper method
(e.g. Self.childMemoryRows(from:)) if needed and keep the other parameters
(rows, agentRows, aggregateRows via Self.programAggregateRows(from: rows),
total, sampledAt, memoryDiagnostic) unchanged.
In `@Sources/TerminalController.swift`:
- Around line 3844-3847: The code currently coerces non-integer or out-of-range
top_group_limit/group_limit values; instead change v2SurfaceSplitSized(params:)
so that if either "top_group_limit" or "group_limit" is present in params you
parse with v2Int and if parsing returns nil or the parsed value is not within
1...100 you return an invalid_params error; only when the key is absent fall
back to the default (12), and only after successful validation set
requestedLimit and compute topGroupLimit = min(max(1, requestedLimit), 100).
Ensure you apply the same presence+range check for both "top_group_limit" and
"group_limit" and reference v2Int, v2SurfaceSplitSized(params:), requestedLimit
and topGroupLimit when locating where to add the validation and error return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d22e0890-244c-47eb-89b1-98dedf682fa3
📒 Files selected for processing (10)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxTopSnapshot.swiftSources/CmuxTopSnapshotScopeCache.swiftSources/TaskManagerSnapshot.swiftSources/TaskManagerTypes.swiftSources/TaskManagerView.swiftSources/TaskManagerWindowController.swiftSources/TerminalController.swiftSources/TerminalControllerTopSupport.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TaskManagerWindowController.swift (1)
130-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid surfacing raw error descriptions in Task Manager.
The catch path currently exposes
String(describing: error)to users. Please return a sanitized localized message here and keep raw error details out of user-facing copy.Suggested fix
} catch { guard !Task.isCancelled else { return } - self?.errorMessage = String(describing: error) + self?.errorMessage = String( + localized: "taskManager.refresh.error", + defaultValue: "Unable to refresh Task Manager data." + ) }As per coding guidelines: user-facing errors/alerts/output must not expose raw upstream messages or internal details.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TaskManagerWindowController.swift` around lines 130 - 140, The catch block in the Task created in TaskManagerWindowController (the Task that calls TerminalController.shared.taskManagerTopPayload and builds a CmuxTaskManagerSnapshot) currently assigns String(describing: error) to self?.errorMessage and thus exposes raw error text; change this to set a user-friendly, localized message (e.g., NSLocalizedString("Failed to refresh tasks", comment: "")) on self?.errorMessage and move the raw error into a non-user-facing log (use your existing logging facility or os_log) so internal details are recorded but not shown to the user; ensure Task.isCancelled checks remain and update any tests/usage that assert errorMessage content.
🤖 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.
Outside diff comments:
In `@Sources/TaskManagerWindowController.swift`:
- Around line 130-140: The catch block in the Task created in
TaskManagerWindowController (the Task that calls
TerminalController.shared.taskManagerTopPayload and builds a
CmuxTaskManagerSnapshot) currently assigns String(describing: error) to
self?.errorMessage and thus exposes raw error text; change this to set a
user-friendly, localized message (e.g., NSLocalizedString("Failed to refresh
tasks", comment: "")) on self?.errorMessage and move the raw error into a
non-user-facing log (use your existing logging facility or os_log) so internal
details are recorded but not shown to the user; ensure Task.isCancelled checks
remain and update any tests/usage that assert errorMessage content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 85982ac4-b6ee-4498-ba32-13aa20a39910
📒 Files selected for processing (5)
CLI/cmux.swiftSources/CmuxTopSnapshot.swiftSources/TaskManagerSnapshot.swiftSources/TaskManagerWindowController.swiftSources/TerminalController.swift
Contributor
Greptile SummaryThis PR adds a recursive memory attribution diagnostic that separates the cmux app footprint from descendant child-process RSS, groups children by command name, and attributes top groups back to workspace/pane/surface. The diagnostic is exposed via a new
Confidence Score: 5/5Safe to merge; the new diagnostic path is additive and isolated from existing process sampling and Task Manager logic. The two previously-flagged issues (cache write race, unlocalized 'Invalid workspace handle') are confirmed fixed. The remaining findings are minor style-level localization nits: one inline string interpolation in the CLI renderer and one internal error string that surfaces a socket method name. Neither affects correctness, data integrity, or user security. No files require special attention; the changes are well-scoped to the new diagnostic surface. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux memory CLI
participant Socket as SocketClient
participant TC as TerminalController (nonisolated)
participant Main as MainActor
participant Cache as SnapshotCache
participant Diag as CmuxTopProcessSnapshot
CLI->>Socket: sendV2("system.memory", params)
Socket->>TC: v2SystemMemory(params)
TC->>Main: "v2MainSync { v2SystemTopBasePayload }"
Main-->>TC: windowNodes (workspace/pane/surface context)
TC->>Cache: captureCached(includeProcessDetails: true, maxAge: 2s)
Cache-->>TC: CmuxTopProcessSnapshot
TC->>TC: v2AnnotateTopWindows(windowNodes, snapshot)
TC->>Diag: memoryDiagnosticPayload(appPID, topGroupLimit, attributionByPID)
Diag-->>TC: memory_diagnostic payload
TC-->>Socket: .ok(payload)
Socket-->>CLI: [String:Any]
CLI->>CLI: renderMemoryText / JSON output
Reviews (8): Last reviewed commit: "Extract memory CLI command" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/TerminalControllerTopSupport.swift (1)
358-385:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let equal-specificity collisions pick an arbitrary surface.
Lines 364-370 now prevent less-specific overwrites, but equal-specificity conflicts still keep the first match, and Lines 374-385 only rank by workspace/pane/surface depth. Shared helper/browser PIDs can appear under multiple webviews at the same specificity, so
top_attributioncan end up pointing at the first traversed workspace/pane/surface instead of remaining ambiguous. Please drop or de-scope equal-specificity conflicts rather than keeping the first winner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalControllerTopSupport.swift` around lines 358 - 385, The current assignTopMemoryAttribution keeps the first attribution when specificities are equal; change it so equal-specificity collisions are not retained: inside assignTopMemoryAttribution(_:,from:to:) compute let existingSpec = v2TopMemoryAttributionSpecificity(existing) and let newSpec = v2TopMemoryAttributionSpecificity(attribution), then if newSpec > existingSpec replace result[pid] = attribution, if newSpec == existingSpec remove the mapping (e.g. result.removeValue(forKey: pid) or set to nil) so equal-specificity PIDs become ambiguous rather than pointing to the first traversed surface; keep the current logic when existingSpec > newSpec to continue.Sources/CmuxTopSnapshot.swift (1)
825-834:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't emit
surface_typefor workspace-only fallback attributions.This branch runs when either
cmuxWorkspaceIDorcmuxSurfaceIDis present, but it always setssurfaceTypeto"terminal". That produces payloads wheresurface_idisnullwhilesurface_typeclaims a concrete surface, which can mislead the new memory attribution UI/CLI.Suggested fix
return CmuxTopProcessAttribution( workspaceID: process.cmuxWorkspaceID, workspaceRef: nil, paneID: nil, paneRef: nil, surfaceID: process.cmuxSurfaceID, surfaceRef: nil, - surfaceType: "terminal", + surfaceType: process.cmuxSurfaceID == nil ? nil : "terminal", reason: process.cmuxAttributionReason ?? "cmux-process-scope" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/CmuxTopSnapshot.swift` around lines 825 - 834, The current branch that builds a CmuxTopProcessAttribution sets surfaceType to the literal "terminal" even when cmuxSurfaceID is nil, producing misleading workspace-only attributions; update the construction in Sources/CmuxTopSnapshot.swift so that surfaceType is only populated when process.cmuxSurfaceID != nil (e.g., set surfaceType to "terminal" when cmuxSurfaceID is present, otherwise pass nil), keeping the other fields (workspaceID, surfaceID, reason) as-is and using the existing CmuxTopProcessAttribution initializer and the process.cmuxAttributionReason value.
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
3863-3867:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnore the legacy fallback key when
top_group_limitis already set.Line 3867 already gives
top_group_limitprecedence, but Lines 3863-3865 still reject the request ifgroup_limitis malformed. That makes an unused fallback parameter a hard error and can break clients that send both keys during migration. Only validategroup_limitwhentop_group_limitis absent.Suggested fix
let topGroupLimitValue = groupLimitParam("top_group_limit") if let invalidLimitKey { return .err(code: "invalid_params", message: "\(invalidLimitKey) must be an integer from 1 to 100", data: nil) } - let groupLimitValue = groupLimitParam("group_limit") - if let invalidLimitKey { - return .err(code: "invalid_params", message: "\(invalidLimitKey) must be an integer from 1 to 100", data: nil) - } + let groupLimitValue: Int? + if topGroupLimitValue == nil { + groupLimitValue = groupLimitParam("group_limit") + if let invalidLimitKey { + return .err(code: "invalid_params", message: "\(invalidLimitKey) must be an integer from 1 to 100", data: nil) + } + } else { + groupLimitValue = nil + } let topGroupLimit = topGroupLimitValue ?? groupLimitValue ?? 12🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController.swift` around lines 3863 - 3867, The current logic rejects malformed group_limit even when top_group_limit (topGroupLimitValue) is present; change the flow so you only call/validate groupLimitParam("group_limit") and check invalidLimitKey if topGroupLimitValue is nil—i.e., if topGroupLimitValue != nil, skip invoking groupLimitParam/invalidLimitKey entirely and let topGroupLimit be topGroupLimitValue, otherwise fall back to parsing/validating group_limit and then set topGroupLimit = groupLimitValue ?? 12; update references to groupLimitParam, invalidLimitKey, topGroupLimitValue, and topGroupLimit accordingly.
🤖 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.
Outside diff comments:
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 825-834: The current branch that builds a
CmuxTopProcessAttribution sets surfaceType to the literal "terminal" even when
cmuxSurfaceID is nil, producing misleading workspace-only attributions; update
the construction in Sources/CmuxTopSnapshot.swift so that surfaceType is only
populated when process.cmuxSurfaceID != nil (e.g., set surfaceType to "terminal"
when cmuxSurfaceID is present, otherwise pass nil), keeping the other fields
(workspaceID, surfaceID, reason) as-is and using the existing
CmuxTopProcessAttribution initializer and the process.cmuxAttributionReason
value.
In `@Sources/TerminalControllerTopSupport.swift`:
- Around line 358-385: The current assignTopMemoryAttribution keeps the first
attribution when specificities are equal; change it so equal-specificity
collisions are not retained: inside assignTopMemoryAttribution(_:,from:to:)
compute let existingSpec = v2TopMemoryAttributionSpecificity(existing) and let
newSpec = v2TopMemoryAttributionSpecificity(attribution), then if newSpec >
existingSpec replace result[pid] = attribution, if newSpec == existingSpec
remove the mapping (e.g. result.removeValue(forKey: pid) or set to nil) so
equal-specificity PIDs become ambiguous rather than pointing to the first
traversed surface; keep the current logic when existingSpec > newSpec to
continue.
---
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 3863-3867: The current logic rejects malformed group_limit even
when top_group_limit (topGroupLimitValue) is present; change the flow so you
only call/validate groupLimitParam("group_limit") and check invalidLimitKey if
topGroupLimitValue is nil—i.e., if topGroupLimitValue != nil, skip invoking
groupLimitParam/invalidLimitKey entirely and let topGroupLimit be
topGroupLimitValue, otherwise fall back to parsing/validating group_limit and
then set topGroupLimit = groupLimitValue ?? 12; update references to
groupLimitParam, invalidLimitKey, topGroupLimitValue, and topGroupLimit
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98382a42-4fc0-41e5-81ae-4e158ec0b063
📒 Files selected for processing (6)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxTopSnapshot.swiftSources/TaskManagerSnapshot.swiftSources/TerminalController.swiftSources/TerminalControllerTopSupport.swift
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CmuxTopMemoryDiagnostics.swift`:
- Around line 167-170: The child groups are losing caller-supplied attribution
because you call nearestCMUXAttribution(for: pid) which rebuilds a fallback from
snapshot fields instead of honoring ancestor attributions; change the logic so
the caller-supplied attributionByPID[pid] is propagated into the ancestor-walk
used by nearestCMUXAttribution. Specifically, update the append call sites (the
expression groups[key]?.append(process: process, attribution:
attributionByPID[pid] ?? nearestCMUXAttribution(for: pid)) and the similar block
around lines 185-205) to supply the existing attribution as a preferred fallback
into the ancestor walk (e.g. add a parameter to nearestCMUXAttribution like
nearestCMUXAttribution(for: pid, preferred: attributionByPID[pid]) or make
nearestCMUXAttribution consult attributionByPID during its walk) so descendant
lookups return the caller-supplied pane/surface refs when available.
In `@Sources/CmuxTopProcessEnumeration.swift`:
- Around line 151-169: The allBSDProcesses() routine drops all results if the
pid buffer stays full (returnedCount >= pids.count), causing an empty snapshot
under high churn; change the loop to preserve and return the last successful pid
snapshot or grow/retry the buffer instead of falling through to return [] —
specifically, in allBSDProcesses() when proc_listallpids fills the buffer
(compare returnedCount and pids.count), either expand the pids buffer (e.g.,
double its size) and retry the call or keep the previous successful pids array
and convert that to bsdInfo entries via bsdInfo(for:) before returning, ensuring
you reference pidCount, pids, returnedCount and bsdInfo(for:) to locate and
modify the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9607e480-cbc8-4e2a-955e-454ec59cb5b7
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/CmuxTopMemoryDiagnostics.swiftSources/CmuxTopProcessEnumeration.swiftSources/CmuxTopProcessSnapshotCache.swiftSources/CmuxTopSnapshot.swiftSources/TaskManagerWindowController.swiftcmux.xcodeproj/project.pbxproj
austinywang
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 20, 2026 10:44
Dismissed stale CodeRabbit changes-requested review after all threads were addressed and the latest CodeRabbit pass succeeded on the current head.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/TerminalController.swift (1)
3880-3888:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
group_limiteven whentop_group_limitis present.Line 3881 skips
group_limitparsing whenevertop_group_limitparsed successfully, so a request like{ "top_group_limit": 12, "group_limit": "abc" }is accepted instead of returninginvalid_params. Please validate the legacy alias whenever it is present, then lettop_group_limitwin only if both values are valid.Suggested fix
- let groupLimitValue: Int? - if topGroupLimitValue == nil { - groupLimitValue = groupLimitParam("group_limit") - if let invalidLimitKey { - return .err(code: "invalid_params", message: "\(invalidLimitKey) must be an integer from 1 to 100", data: nil) - } - } else { - groupLimitValue = nil - } + let groupLimitValue = groupLimitParam("group_limit") + if let invalidLimitKey { + return .err(code: "invalid_params", message: "\(invalidLimitKey) must be an integer from 1 to 100", data: nil) + } let topGroupLimit = topGroupLimitValue ?? groupLimitValue ?? 12Based on learnings,
v2SurfaceSplitSized(params:)should treat a present-but-invalid numeric parameter asinvalid_params.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController.swift` around lines 3880 - 3888, In v2SurfaceSplitSized(params:) change the logic so group_limit is always parsed via groupLimitParam("group_limit") (and checked for invalidLimitKey) even when topGroupLimitValue is non-nil; if groupLimitParam reports invalidLimitKey return the same .err(code: "invalid_params", ...) immediately, otherwise compute the effective value by preferring topGroupLimitValue when both parsed successfully (i.e., let topGroupLimitValue win only after validating groupLimitValue). Ensure you reference and keep using groupLimitValue, topGroupLimitValue, groupLimitParam("group_limit"), and invalidLimitKey in the updated flow.
🤖 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 `@Sources/TerminalControllerTopSupport.swift`:
- Around line 393-399: When newSpecificity == existingSpecificity don't
immediately remove result[pid]; instead attempt to collapse the two attributions
into their common owner and preserve that attribution. Concretely: in the
equality branch (where v2TopMemoryAttributionSpecificity(existing) ==
newSpecificity), compute a shared/common ancestor attribution from existing and
attribution (e.g. with a utility like
v2TopMemoryAttributionCommonAncestor(existing, attribution) or implement a small
helper to walk their owner/pane/workspace ancestry), and if a common owner
exists set result[pid] = commonOwner; only if no common owner can be found then
removeValue(forKey: pid) and set ambiguousSpecificityByPID[pid] =
newSpecificity. This preserves shared-process attribution to the common owner
and marks ambiguous only when no common owner remains.
---
Duplicate comments:
In `@Sources/TerminalController.swift`:
- Around line 3880-3888: In v2SurfaceSplitSized(params:) change the logic so
group_limit is always parsed via groupLimitParam("group_limit") (and checked for
invalidLimitKey) even when topGroupLimitValue is non-nil; if groupLimitParam
reports invalidLimitKey return the same .err(code: "invalid_params", ...)
immediately, otherwise compute the effective value by preferring
topGroupLimitValue when both parsed successfully (i.e., let topGroupLimitValue
win only after validating groupLimitValue). Ensure you reference and keep using
groupLimitValue, topGroupLimitValue, groupLimitParam("group_limit"), and
invalidLimitKey in the updated flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 36424604-8098-4877-b2de-ce178f579921
📒 Files selected for processing (7)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxTopMemoryDiagnostics.swiftSources/CmuxTopProcessEnumeration.swiftSources/CmuxTopProcessSnapshotCache.swiftSources/TerminalController.swiftSources/TerminalControllerTopSupport.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TerminalControllerTopSupport.swift`:
- Around line 399-412: The comparison logic currently uses
v2TopMemoryAttributionSpecificity(existing) (existingSpecificity) which can
differ from the recorded collapse specificity in
commonOwnerSourceSpecificityByPID; change the branches to compare newSpecificity
against the collapse/source-specificity used for the stored owner (use
mergedSourceSpecificity or commonOwnerSourceSpecificityByPID[pid] when present)
instead of existingSpecificity so a later weaker attribution cannot incorrectly
overwrite or ambiguate a PID; update the if/else conditions around
v2TopMemoryAttributionCommonOwner(existing, attribution), the newSpecificity >
... and newSpecificity == ... checks to use mergedSourceSpecificity (or its
resolved value) and ensure commonOwnerSourceSpecificityByPID and
ambiguousSpecificityByPID are updated consistently with mergedSourceSpecificity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12d303da-2571-4d5d-91b6-6b2512f662c1
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/CmuxTopProcessSnapshotCache.swiftSources/TaskManagerSnapshot.swiftSources/TaskManagerTypes.swiftSources/TaskManagerView.swiftSources/TaskManagerWindowController.swiftSources/TerminalControllerTopSupport.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/CmuxTopProcessEnumeration.swift`:
- Around line 160-175: The code incorrectly divides proc_listallpids' return
value by pidStride; proc_listallpids already returns a PID count, so remove
those divisions: compute initialPIDCount directly from initialByteCount (e.g.,
initialPIDCount = max(1, initialByteCount)) and compute returnedPIDCount as
Int(returnedByteCount) (no / pidStride). Update uses of returnedPIDCount and any
logic that derived capacity from initialPIDCount accordingly
(functions/variables to change: proc_listallpids calls, initialByteCount,
initialPIDCount, returnedByteCount, returnedPIDCount, capacity, and the loop
that builds pids and calls bsdInfos/from lastPIDs).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c70babc5-fb07-4305-a2c8-9976a658fc7d
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/CmuxTopProcessEnumeration.swiftSources/TaskManagerSnapshot.swiftSources/TaskManagerTypes.swiftSources/TerminalControllerTopSupport.swift
austinywang
dismissed
coderabbitai[bot]’s stale review
May 20, 2026 11:48
Stale CodeRabbit change request: the proc_listallpids return-value finding was fixed in f62bd35, the inline thread is resolved/outdated, and there are no unresolved review threads.
This was referenced May 22, 2026
lawrencecchen
added a commit
that referenced
this pull request
May 22, 2026
…64.8 leak class Red commit in the two-commit regression pattern documented in repo/CLAUDE.md. PR #4437 migrated CmuxTaskManagerModel to @observable and CmuxTaskManagerView started holding @bindable var model while rendering ScrollView { LazyVStack { ForEach { ... } } }. That violates the "Snapshot boundary for list subtrees" rule documented in repo/CLAUDE.md (citing #2586): no view below a lazy-list boundary may hold a reference to an Observable store. The 3 s refresh timer in TaskManagerWindowController constantly mutates model.snapshot, which orthogonally invalidated every row and thrashed LazyLayoutViewCache, allocating AttributeGraph nodes that never freed. Matches the hang trace posted by @ma-pony on issue #4529. Refactor to the IndexSectionActions / SectionGapActions pattern in Sources/SessionIndexView.swift: - CmuxTaskManagerListView is a new value-typed inner view that owns the ScrollView/LazyVStack subtree. It receives value-typed row arrays and a CmuxTaskManagerRowActions closure bundle; it never sees the model. - CmuxTaskManagerRowActions bundles viewWorkspace/viewTerminal/ killProcess/activate closures. Bound to the model once per render at the outer view, mirroring FeedRowActions.bound() in Feed/FeedPanelView.swift. - CmuxTaskManagerRow and CmuxTaskManagerResources gain Equatable so the row view's == has something to compare. - CmuxTaskManagerSectionHeaderView gains Equatable comparing only the header payload. - CmuxTaskManagerRowView gains Equatable, but `==` intentionally returns `false` in this commit so the new TaskManagerViewSnapshotBoundaryTests fail at runtime (XCTAssertEqual on two row views with the same payload returns false). With .equatable() wired at every ForEach call site, a false-returning == defeats the optimization and re-introduces the per-tick row body re-evaluation that drives the leak. The follow-up commit fixes == to compare only the value-typed `row` payload. - The outer CmuxTaskManagerView keeps @bindable var model for the toolbar/summary/sort header. Those live above the snapshot boundary and need to repaint on snapshot mutation. Refs #4529 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
lawrencecchen
added a commit
that referenced
this pull request
May 22, 2026
#4555) * test: refactor TaskManagerView with broken row Equatable to expose 0.64.8 leak class Red commit in the two-commit regression pattern documented in repo/CLAUDE.md. PR #4437 migrated CmuxTaskManagerModel to @observable and CmuxTaskManagerView started holding @bindable var model while rendering ScrollView { LazyVStack { ForEach { ... } } }. That violates the "Snapshot boundary for list subtrees" rule documented in repo/CLAUDE.md (citing #2586): no view below a lazy-list boundary may hold a reference to an Observable store. The 3 s refresh timer in TaskManagerWindowController constantly mutates model.snapshot, which orthogonally invalidated every row and thrashed LazyLayoutViewCache, allocating AttributeGraph nodes that never freed. Matches the hang trace posted by @ma-pony on issue #4529. Refactor to the IndexSectionActions / SectionGapActions pattern in Sources/SessionIndexView.swift: - CmuxTaskManagerListView is a new value-typed inner view that owns the ScrollView/LazyVStack subtree. It receives value-typed row arrays and a CmuxTaskManagerRowActions closure bundle; it never sees the model. - CmuxTaskManagerRowActions bundles viewWorkspace/viewTerminal/ killProcess/activate closures. Bound to the model once per render at the outer view, mirroring FeedRowActions.bound() in Feed/FeedPanelView.swift. - CmuxTaskManagerRow and CmuxTaskManagerResources gain Equatable so the row view's == has something to compare. - CmuxTaskManagerSectionHeaderView gains Equatable comparing only the header payload. - CmuxTaskManagerRowView gains Equatable, but `==` intentionally returns `false` in this commit so the new TaskManagerViewSnapshotBoundaryTests fail at runtime (XCTAssertEqual on two row views with the same payload returns false). With .equatable() wired at every ForEach call site, a false-returning == defeats the optimization and re-introduces the per-tick row body re-evaluation that drives the leak. The follow-up commit fixes == to compare only the value-typed `row` payload. - The outer CmuxTaskManagerView keeps @bindable var model for the toolbar/summary/sort header. Those live above the snapshot boundary and need to repaint on snapshot mutation. Refs #4529 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: compare CmuxTaskManagerRowView only by row payload so .equatable() can skip body re-eval Green commit: the `==` operator now compares only the value-typed `row` payload, ignoring the closure identities the parent rebuilds every render tick. With `.equatable()` wired at each ForEach call site (added in the red commit's refactor), SwiftUI can now prove that two row views with the same payload are interchangeable, suppressing body re-evaluation on the 3 s `model.snapshot` mutation. This closes the snapshot-boundary class of leak described in repo/CLAUDE.md (citing #2586) and the secondary leak vector from #4529. The dominant vector (git probe ancestor walk on macOS 14/15) is fixed separately in #4552. testTaskManagerRowViewEqualityIgnoresClosureIdentity flips from RED to GREEN with this commit; testTaskManagerRowViewEqualityDetectsRowChanges continues to assert the inverse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review: stabilize PID ordering, tighten actor isolation, restore section header access Three findings on #4555: - CodeRabbit (Major): the snapshot producers already sort PID arrays before sending, but synthesized Equatable on CmuxTaskManagerRow / CmuxTaskManagerResources compares them by array order. Normalize rootProcessIds, foregroundProcessGroupIds, and processIds at init so the invariant ("these arrays are stored deduped + ascending") lives in one place and a future producer that forgets to sort cannot silently reopen the cache-thrash path this PR is closing. - Greptile (P2): annotate the closure types on CmuxTaskManagerRowActions and CmuxTaskManagerRowView with @mainactor. The actions are bound through @mainactor CmuxTaskManagerModel methods and the row view invokes them from SwiftUI button taps. Making the isolation explicit lets Swift 6 catch any future off-MainActor forwarding at compile time. - Greptile (P2): revert CmuxTaskManagerSectionHeaderView from internal back to private. Only CmuxTaskManagerRowView needs broader access for the regression test; the section header has no caller outside this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 23, 2026
This was referenced Jul 4, 2026
ShubhamPatilsd
pushed a commit
to emergent-inc/mosaic
that referenced
this pull request
Jul 9, 2026
…k (#4555) * test: refactor TaskManagerView with broken row Equatable to expose 0.64.8 leak class Red commit in the two-commit regression pattern documented in repo/CLAUDE.md. PR manaflow-ai/cmux#4437 migrated CmuxTaskManagerModel to @observable and CmuxTaskManagerView started holding @bindable var model while rendering ScrollView { LazyVStack { ForEach { ... } } }. That violates the "Snapshot boundary for list subtrees" rule documented in repo/CLAUDE.md (citing manaflow-ai/cmux#2586): no view below a lazy-list boundary may hold a reference to an Observable store. The 3 s refresh timer in TaskManagerWindowController constantly mutates model.snapshot, which orthogonally invalidated every row and thrashed LazyLayoutViewCache, allocating AttributeGraph nodes that never freed. Matches the hang trace posted by @ma-pony on issue manaflow-ai/cmux#4529. Refactor to the IndexSectionActions / SectionGapActions pattern in Sources/SessionIndexView.swift: - CmuxTaskManagerListView is a new value-typed inner view that owns the ScrollView/LazyVStack subtree. It receives value-typed row arrays and a CmuxTaskManagerRowActions closure bundle; it never sees the model. - CmuxTaskManagerRowActions bundles viewWorkspace/viewTerminal/ killProcess/activate closures. Bound to the model once per render at the outer view, mirroring FeedRowActions.bound() in Feed/FeedPanelView.swift. - CmuxTaskManagerRow and CmuxTaskManagerResources gain Equatable so the row view's == has something to compare. - CmuxTaskManagerSectionHeaderView gains Equatable comparing only the header payload. - CmuxTaskManagerRowView gains Equatable, but `==` intentionally returns `false` in this commit so the new TaskManagerViewSnapshotBoundaryTests fail at runtime (XCTAssertEqual on two row views with the same payload returns false). With .equatable() wired at every ForEach call site, a false-returning == defeats the optimization and re-introduces the per-tick row body re-evaluation that drives the leak. The follow-up commit fixes == to compare only the value-typed `row` payload. - The outer CmuxTaskManagerView keeps @bindable var model for the toolbar/summary/sort header. Those live above the snapshot boundary and need to repaint on snapshot mutation. Refs manaflow-ai/cmux#4529 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: compare CmuxTaskManagerRowView only by row payload so .equatable() can skip body re-eval Green commit: the `==` operator now compares only the value-typed `row` payload, ignoring the closure identities the parent rebuilds every render tick. With `.equatable()` wired at each ForEach call site (added in the red commit's refactor), SwiftUI can now prove that two row views with the same payload are interchangeable, suppressing body re-evaluation on the 3 s `model.snapshot` mutation. This closes the snapshot-boundary class of leak described in repo/CLAUDE.md (citing manaflow-ai/cmux#2586) and the secondary leak vector from manaflow-ai/cmux#4529. The dominant vector (git probe ancestor walk on macOS 14/15) is fixed separately in manaflow-ai/cmux#4552. testTaskManagerRowViewEqualityIgnoresClosureIdentity flips from RED to GREEN with this commit; testTaskManagerRowViewEqualityDetectsRowChanges continues to assert the inverse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR review: stabilize PID ordering, tighten actor isolation, restore section header access Three findings on manaflow-ai/cmux#4555: - CodeRabbit (Major): the snapshot producers already sort PID arrays before sending, but synthesized Equatable on CmuxTaskManagerRow / CmuxTaskManagerResources compares them by array order. Normalize rootProcessIds, foregroundProcessGroupIds, and processIds at init so the invariant ("these arrays are stored deduped + ascending") lives in one place and a future producer that forgets to sort cannot silently reopen the cache-thrash path this PR is closing. - Greptile (P2): annotate the closure types on CmuxTaskManagerRowActions and CmuxTaskManagerRowView with @mainactor. The actions are bound through @mainactor CmuxTaskManagerModel methods and the row view invokes them from SwiftUI button taps. Making the isolation explicit lets Swift 6 catch any future off-MainActor forwarding at compile time. - Greptile (P2): revert CmuxTaskManagerSectionHeaderView from internal back to private. Only CmuxTaskManagerRowView needs broader access for the regression test; the section header has no caller outside this file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks done
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cmux memorydiagnostic that separates direct cmux app footprint from recursive child-process RSSsystem.topJSONFixes #4364
Verification
xcrun swiftc -parse Sources/CmuxTopSnapshot.swift Sources/CmuxTopSnapshotScopeCache.swift Sources/TerminalControllerTopSupport.swift Sources/TerminalController.swift Sources/TaskManagerSnapshot.swift Sources/TaskManagerTypes.swift Sources/TaskManagerView.swift Sources/TaskManagerWindowController.swift CLI/cmux.swiftpython3 -m json.tool Resources/Localizable.xcstringsgit diff --checkDid not run local tests,
xcodebuild, orreload.shper workspace instructions.Need help on this PR? Tag
@codesmithwith what you need.Note
Medium Risk
Adds a new diagnostics surface (
system.memory/cmux memory) and changes low-level process enumeration/caching, which could affect accuracy/perf of process/resource reporting if edge cases are missed.Overview
Adds recursive memory diagnostics that separate the cmux app’s physical footprint from descendant process RSS, group child RSS by command, and attribute top groups back to workspace/pane/surface when possible.
Exposes this via a new
system.memoryv2 method and acmux memoryCLI command (with--all,--workspace,--groups,--json), and also embeds amemory_diagnosticblock into existingsystem.topresponses.Updates Task Manager to display App Footprint/Child RSS summary metrics and a new Child Process RSS section, and refactors process sampling to use
proc_listallpids/proc_pidinfo(RUSAGE v4) with a short-lived snapshot cache plus improved attribution extraction from the annotated window tree.Reviewed by Cursor Bugbot for commit 416808b. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds recursive memory attribution that separates the
cmuxapp footprint from recursive child-process RSS and attributes usage to workspace/pane/surface. Exposes diagnostics via the newcmux memoryCLI,system.memoryAPI, and updated Task Manager metrics/UI.New Features
cmux memorywith--all,--workspace,--groups,--json(defaults to top 12 groups), with localized help/errors.system.memoryand embedsmemory_diagnosticinsystem.topJSON.Refactors
proc_listallpids/proc_pidinfo+RUSAGE_INFO_V4, with a short-lived cached snapshot; extends scope-cache keying forproc_bsdinfo.system.memoryin allowed methods; migrates Task Manager model to Swift Observation (@Observable,@Bindable).proc_listallpidscount handling to avoid truncated process lists; bridges missing process tree intermediates; extracts thecmux memoryCLI intoCMUXCLI+Memory.swiftwith dedicated parsing/rendering.Written for commit 416808b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Enhancements
Documentation
Bug Fixes