Skip to content

Fix aggregate memory attribution across workspaces - #9086

Merged
austinywang merged 4 commits into
mainfrom
issue-9069-memory-attribution-helpers
Jul 29, 2026
Merged

austinywang merged 4 commits into
mainfrom
issue-9069-memory-attribution-helpers

Conversation

@austinywang

@austinywang austinywang commented Jul 28, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • model command-group attribution separately from the largest individual member
  • reduce group owners to their common surface, pane, or workspace and represent cross-workspace/partial groups explicitly
  • show multi-workspace ownership consistently in cmux memory and Task Manager without targeting one arbitrary workspace
  • retain top_attribution in the socket payload for backward-compatible diagnostic detail

Regression coverage

  • commit ab3bbb2a7f adds the failing producer, Task Manager, and bundled CLI behavior tests
  • commit 921d783eb7 implements the fix

Validation

  • xcrun swiftc -parse for all changed Swift sources and tests
  • xcrun swiftc -typecheck Sources/CmuxTopProcessOwner.swift Sources/CmuxTopProcessAttribution.swift
  • ./scripts/lint-pbxproj-test-wiring.sh
  • ./scripts/check-pbxproj.sh
  • jq empty Resources/Localizable.xcstrings
  • localization audit: the CLI and Task Manager strings use the same keys, with English and Japanese catalog entries; no web surface changed
  • scripts/swift_file_length_budget.py and .github/swift-file-length-budget.tsv are absent from the base revision; every new Swift file is under 150 lines and every modified existing Swift file is shorter than its baseline

Closes #9069


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes memory attribution for command groups by aggregating across all members, not just the largest process. Multi‑workspace groups now show “multiple owners” or “N workspaces” in cmux memory and Task Manager; single‑owner groups show the common workspace/pane/surface. Addresses #9069.

  • Bug Fixes

    • Added group-level attribution kinds (common, multiple, partial, unattributed) in group_attribution; kept top_attribution for compatibility.
    • CLI uses a shared helper and memory.attribution.* strings to render “multiple owners”, “%lld workspaces”, and “partially attributed”; falls back to top_attribution if needed.
    • Task Manager only navigates when a common owner exists; details use the group’s localized description; top summary uses the group’s common workspace when present.
    • Tests cover multi‑workspace groups, merging different reasons for the same owner, and Task Manager navigation.
  • Refactors

    • Added CmuxTopProcessOwner and CmuxTopProcessAttribution with common‑owner logic and specificity; extracted CmuxTopMemoryDiagnosticGroupAccumulator and exposed its initializer.
    • Introduced CmuxTaskManagerMemoryGroup, CmuxTaskManagerMemoryGroupAttribution, and CmuxTaskManagerMemoryAttribution decoders; moved CLI attribution code to CMUXCLI+MemoryAttribution.swift.

Written for commit b34c4c3. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features

    • Added clearer memory attribution across workspaces, panes, and surfaces.
    • Memory diagnostics now identify shared, multiple, partial, and unattributed ownership.
    • Added localized English and Japanese descriptions for attribution states.
    • Improved display of ownership details and identifiers in memory views.
  • Bug Fixes

    • Prevented ambiguous multi-workspace memory groups from navigating to an incorrect owner.
    • Improved handling of shared ownership when processes differ in scope.
  • Tests

    • Added coverage for multi-workspace, shared-owner, and CLI memory attribution scenarios.

@cursor

cursor Bot commented Jul 28, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Memory diagnostics now model ownership explicitly, classify groups as common, multiple, partial, or unattributed, and propagate localized attribution through task-manager and CLI output. New tests cover multi-workspace groups and common-owner resolution.

Changes

Memory attribution

Layer / File(s) Summary
Ownership and attribution model
Sources/CmuxTopProcessOwner.swift, Sources/CmuxTopProcessAttribution.swift
Adds owner matching, specificity, identity keys, and attribution payload conversion.
Diagnostic aggregation and merging
Sources/CmuxTopMemoryDiagnosticGroupAccumulator.swift, Sources/CmuxTopMemoryDiagnostics.swift, Sources/TerminalControllerTopSupport.swift
Aggregates process ownership and emits group attribution classifications for diagnostic payloads.
Task-manager and CLI rendering
Sources/CmuxTaskManagerMemory*, Sources/TaskManagerSnapshot.swift, CLI/CMUXCLI+Memory*.swift, Resources/Localizable.xcstrings
Parses attribution states and renders localized common, multiple, partial, and unattributed descriptions.
Validation and project wiring
cmuxTests/*MemoryAttributionTests.swift, cmux.xcodeproj/project.pbxproj
Adds multi-workspace and common-owner tests and registers the new sources and test suites.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

Possibly related PRs

  • manaflow-ai/cmux#4437 — Introduced related recursive memory diagnostics plumbing that this change refactors.

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (2 errors, 1 warning)

Check name Status Explanation Resolution
Cmux Swift Package Boundaries ❌ Error New reusable memory-attribution/parsing types live in app-target Sources and are used by cmux, cmux-cli, and tests; this should be behind a package boundary (e.g. CmuxCore). Extract the memory-attribution domain model/decoder/accumulator types into Packages/macOS/CmuxCore (or a small new SwiftPM package) and keep only CLI/Task Manager/AppKit glue in the app target.
Cmux Full Internationalization ❌ Error The new memory.attribution.* strings are only localized for en/ja, while Resources/Localizable.xcstrings supports 20 locales, so the diff isn’t fully internationalized. Add translations for memory.attribution.multipleOwners, .multipleWorkspaces, and .partial in Resources/Localizable.xcstrings for every existing locale, not just en/ja.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (22 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation ✅ Passed The diff adds only plain value-model structs and pure helpers; no new @MainActor/UI-bound state or mutable Sendable reference types were introduced.
Cmux Swift Blocking Runtime ✅ Passed PASS: The diff adds no semaphores, sleeps, main-queue syncs, polling, or manual locks in production code; the only timeout is test-only scaffolding.
Cmux Browser Automation Off-Main ✅ Passed The only changed file is Sources/CmuxTopMemoryDiagnosticGroupAccumulator.swift, a memory attribution accumulator; no browser.* routing or WebKit/off-main policy code changed.
Cmux Expensive Synchronous Load ✅ Passed The memory-attribution changes only reshape in-memory diagnostics/text rendering; no new RestorableAgentSessionIndex.load() or heavy JSONL/transcript loads were added on main/interactive paths.
Cmux Cache Substitution Correctness ✅ Passed The changes derive attribution from live diagnostic payloads and preserve top_attribution for compatibility; no cache substitution appears in snapshot/history paths.
Cmux No Hacky Sleeps ✅ Passed The PR only touches Swift, localization, and Xcode project files; no covered non-Swift runtime scripts were changed and no fixed sleeps/delays were introduced.
Cmux Algorithmic Complexity ✅ Passed No nested rescans were introduced; attribution is one-pass with dict/Set accumulation, and remaining sorts are presentation-oriented with a 12-item top-group cap.
Cmux Swift Concurrency ✅ Passed Changed Swift code is synchronous attribution/parsing only; no new DispatchQueue.global, DispatchGroup, Combine, completion-handler APIs, or fire-and-forget Task patterns found.
Cmux Swift @Concurrent ✅ Passed Changed Swift files add only synchronous memory-attribution helpers; no new async/nonisolated/@Concurrent boundary or UI-hop issue appears in the diff.
Cmux Swiftpm Lockfiles ✅ Passed Policy script reports OK; the PR only adds source/file wiring in cmux.xcodeproj and no .gitignore, Package.swift, or Package.resolved changes.
Cmux Swift Logging ✅ Passed Changed Swift files add only intended CLI output prints; no new runtime Logger/NSLog/debug logging or unsafe diagnostics appear in the diff.
Cmux User-Facing Error Privacy ✅ Passed New CLI/Task Manager output is generic (“multiple owners”, “%lld workspaces”, workspace/pane/surface labels) and exposes no vendor, secret, or raw payload details.
Cmux Swiftui State Layout ✅ Passed No SwiftUI state/layout patterns were introduced; the changed files are CLI/data helpers and tests, with no ObservableObject, @Published, GeometryReader, or lazy-row store refs.
Cmux Architecture Rethink ✅ Passed No timing hacks or split ownership patterns were introduced; the diff centralizes memory attribution in a clear group-level source of truth with compatibility fallback.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The only changed Swift file is a memory diagnostic accumulator; it adds no NSWindow/NSPanel/WindowGroup code or cmuxAuxiliaryWindowIdentifiers usage.
Cmux Source Artifacts ✅ Passed Only changed path is a Swift source file with hand-written code; no logs, caches, build output, or scratch artifacts were added.
Cmux No Test Or Debug Seam In Production Source ✅ Passed No changed production Swift file adds a test/debug seam; new APIs are production-facing and the diff has no #if DEBUG or *ForTesting/*TestHook members.
Cmux No Ambient Global State ✅ Passed No new ambient global state: new behavior lives on types/extensions, and the only file-scope declaration is an immutable default constant.
Title check ✅ Passed The title clearly matches the main change: aggregating memory attribution across workspaces.
Description check ✅ Passed The PR includes the required Summary and Testing/Validation content, but omits Demo Video, Review Trigger, and Checklist sections.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9069-memory-attribution-helpers

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@austinywang

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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 `@cmuxTests/CMUXCLIMemoryAttributionTests.swift`:
- Around line 29-30: Update the assertions in CMUXCLIMemoryAttributionTests to
verify the expected group-level label, and reject any output containing the
workspace workspace: attribution prefix rather than only workspace:1. Ensure
regressions selecting either workspace are rejected while preserving the
existing two-workspace assertion.
- Around line 2-5: Move
testMemoryCommandLabelsMultiWorkspaceGroupWithoutSingleOwner from
CMUXCLIMemoryAttributionTests.swift into the existing
CMUXCLIErrorOutputRegressionTests suite file. Remove the separate extension file
and keep the suite’s imports and test declarations centralized so Swift Testing
discovers the test through the single suite definition.
- Around line 14-18: Make the memory attribution label assertion deterministic
by configuring the CLI’s locale-related environment variables to English in the
test setup near the CMUX_ environment cleanup, or derive the expected label from
the English localization source. Ensure the assertion remains independent of the
host process locale.

In `@Sources/CmuxTaskManagerMemoryGroupAttribution.swift`:
- Line 10: Change the initializer parameter in the payload initializer to a
non-optional [String: Any]. Update its call site in CmuxTaskManagerMemoryGroup
to pass payload["group_attribution"] as? [String: Any] ?? [:], preserving the
existing handling of missing or empty attribution data.

In `@Sources/CmuxTopMemoryDiagnosticGroupAccumulator.swift`:
- Around line 5-37: Update the attributions aggregation to key its dictionary by
attribution.owner rather than the full CmuxTopProcessAttribution, while
retaining one representative attribution for each owner when producing payloads.
Ensure processes with different reason values but the same
workspace/pane/surface owner accumulate into one bucket, preserving the existing
RSS, process count, and PID aggregation behavior in AttributionAccumulator.

In `@Sources/CmuxTopProcessOwner.swift`:
- Around line 43-137: Update CmuxTopProcessOwner construction or commonOwner to
normalize and compare workspace identifiers across ID and ref representations,
so a workspaceID-only owner merges with a workspaceRef-only owner when they
identify the same physical workspace. Preserve the existing grouping behavior
for genuinely different or missing workspace identities, and ensure the
normalized workspaceID/workspaceRef values are carried into the returned owner.
🪄 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 Plus

Run ID: 7d5cc070-ffe1-4f13-b97c-5a9f4f4f3c0d

📥 Commits

Reviewing files that changed from the base of the PR and between 64ec2bb and 1e55a10.

📒 Files selected for processing (16)
  • CLI/CMUXCLI+Memory.swift
  • CLI/CMUXCLI+MemoryAttribution.swift
  • Resources/Localizable.xcstrings
  • Sources/CmuxTaskManagerMemoryAttribution.swift
  • Sources/CmuxTaskManagerMemoryGroup.swift
  • Sources/CmuxTaskManagerMemoryGroupAttribution.swift
  • Sources/CmuxTopMemoryDiagnosticGroupAccumulator.swift
  • Sources/CmuxTopMemoryDiagnostics.swift
  • Sources/CmuxTopProcessAttribution.swift
  • Sources/CmuxTopProcessOwner.swift
  • Sources/TaskManagerSnapshot.swift
  • Sources/TaskManagerTypes.swift
  • Sources/TerminalControllerTopSupport.swift
  • cmux.xcodeproj/project.pbxproj
  • cmuxTests/CMUXCLIMemoryAttributionTests.swift
  • cmuxTests/CmuxTopMemoryAttributionTests.swift
💤 Files with no reviewable changes (1)
  • Sources/TaskManagerTypes.swift

Comment thread cmuxTests/CMUXCLIMemoryAttributionTests.swift
Comment thread cmuxTests/CMUXCLIMemoryAttributionTests.swift
Comment thread cmuxTests/CMUXCLIMemoryAttributionTests.swift Outdated
Comment thread Sources/CmuxTaskManagerMemoryGroupAttribution.swift Outdated
Comment thread Sources/CmuxTopMemoryDiagnosticGroupAccumulator.swift
Comment thread Sources/CmuxTopProcessOwner.swift
@austinywang

Copy link
Copy Markdown
Contributor Author

Addressing the remaining CodeRabbit summary items:

  • Package boundary: no production Swift type added here is shared by the app and CLI targets. The app-side model emits the existing JSON diagnostic contract; the CLI independently decodes [String: Any] and renders it. Tests exercise those two sides but are not another production consumer. Per cmux package policy, extracting an app-owned diagnostic domain without a second type-level consumer would add a package/target dependency without establishing a real boundary, so no extraction is warranted.
  • Internationalization: cmux currently supports English and Japanese for Swift/AppKit strings. All three new keys have both supported localizations in Resources/Localizable.xcstrings; the localization audit and JSON/catalog parsing passed. The bot inferred 20 supported locales from historical catalog contents, which is not the repository policy.
  • Docstrings: the new internal model types already have intent doc comments. Main app-target internal functions are not subject to a generated 80% public-API DocC threshold, so the optional warning does not call for filler documentation.

All six inline findings were separately fixed or answered and their threads are resolved. Full CI run 30407620070 is green, including unit shards, Swift packages, build/lag, universal Release, and final ci-status.

@austinywang
austinywang merged commit 32f1e83 into main Jul 29, 2026
48 of 49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmux memory --all misattributes ~156MB / 14 helper processes to a single workspace

1 participant