Repository navigation
Fix task manager attribution for launchd-parented helpers - #4190
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughTracks CMUX attribution reasons: derives scopes from environment or hook-monitor arguments, attaches attributionReason to process scopes and process-info, classifies each process in the process tree (including WebKit and root/child heuristics), and includes the reason in CLI output and tests. ChangesCMUX Process Attribution Reasons
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR fixes Task Manager attribution for helpers whose parent has been reparented to
Confidence Score: 5/5Safe to merge; the attribution and scope-detection logic is well-tested and does not touch resource accounting or process-tree membership. The scope-detection refactor is mechanically straightforward: environment lookup is separated cleanly from argument parsing, the subcommand guard correctly applies path normalization only to argv[0], and the regression suite covers the key launchd-parented and WebKit WebContent cases. The two findings are about output verbosity and a loose name match — neither affects whether processes are attributed to the correct surface or whether resource totals are correct. Sources/CmuxTopSnapshot.swift — the attributionReason() fallback and isWebKitWebContentProcess name check are worth a second look before shipping. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Process sampled via sysctl] --> B{cachedCMUXScope returns non-nil?}
B -- Yes --> C[CmuxTopProcessInfo cmuxAttributionReason = scope.attributionReason]
B -- No --> D[cmuxAttributionReason = nil]
C --> E[processTreeNode builds payload]
D --> E
subgraph scope
S1{CMUX env vars present?} -- Yes --> S2[attributionReason = cmux-environment]
S1 -- No --> S3{argv = cmux hooks codex monitor + flags?}
S3 -- Yes --> S4[attributionReason = cmux-hook-arguments]
S3 -- No --> S5[nil - no scope]
end
subgraph reason
R1{cmuxAttributionReason non-nil?} -- Yes --> R2[return stored reason]
R1 -- No --> R3{pid in rootPIDs and WebKit?}
R3 -- Yes --> R4[webview-root-pid]
R3 -- No --> R5{pid in rootPIDs?}
R5 -- Yes --> R6[explicit-root-pid]
R5 -- No --> R7{parentPID in allowedPIDs?}
R7 -- Yes --> R8[child-process]
R7 -- No --> R9[included-process]
end
E --> reason
A --> scope
Reviews (3): Last reviewed commit: "fix: tighten hook monitor command matchi..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/CmuxTopSnapshot.swift`:
- Around line 55-61: Remove the misleading default for attributionReason in the
CmuxTopProcessScope/CmuxTopSnapshot init: change
init(workspaceID:surfaceID:attributionReason:) to require attributionReason (no
"= \"cmux-environment\"") so callers must supply an explicit reason; update any
call sites that relied on the default (verify cmuxScopeFromEnvironment and
cmuxHookMonitorScope still pass explicit reasons) and adjust any tests or
helpers to provide an intentional attribution string.
- Around line 418-423: The helper isWebKitWebContentProcess currently treats any
process whose name contains "WebKit" as a WebContent process, which incorrectly
matches other WebKit helpers; update the logic to only accept processes that are
actually WebContent by replacing the name check with a scoped check such as
name.localizedCaseInsensitiveContains("WebContent") (or stricter
equality/contains for "WebContent") and keep the existing path check for
com.apple.WebKit.WebContent so only processes matching WebContent by name or by
path are considered web content.
In `@Sources/CmuxTopSnapshotScopeCache.swift`:
- Around line 156-166: The uuidOptionValue(in:names:) helper currently only
handles flags where the name and value are separate argv entries; update it to
also recognize and parse "--name=VALUE" forms by checking each argument for a
prefix match with any name + "=", extracting the substring after the "=" (trim
whitespace), validating via UUID(uuidString:), and returning that UUID; retain
the existing logic that checks the next argv slot when the argument exactly
equals a name and continues to trim/validate as before (use the existing symbols
uuidOptionValue(in:names:), arguments, names, and UUID(uuidString:) to locate
and modify the function).
🪄 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: 3a038587-dc2e-4725-b8f7-82ac6fc89b56
📒 Files selected for processing (4)
CLI/cmux.swiftSources/CmuxTopSnapshot.swiftSources/CmuxTopSnapshotScopeCache.swiftcmuxTests/CmuxTopSnapshotScopeTests.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/CmuxTopSnapshotScopeCache.swift`:
- Around line 104-126: The attribution labels used in CmuxTopProcessScope are
inconsistent with the CLI contract: update the attributionReason passed in
cmuxScopeFromEnvironment(...) from "session-env" to "cmux-environment" and
update the attributionReason passed in cmuxHookMonitorScope(...) from
"hook-monitor" to "cmux-hook-arguments" so the emitted attribution_reason
matches the documented vocabulary; locate the CmuxTopProcessScope initializers
in those functions and replace the two literal strings accordingly.
- Around line 129-150: containsSubcommandPath currently allows a match when the
first N tokens equal the normalized path even if argv[0] isn't "cmux", so
tighten the check by requiring the executable name to be "cmux" for the
hook-monitor path match; update the function (containsSubcommandPath) to either
remove the fallback branch or add a guard that checks executableName == "cmux"
before comparing leadingArguments (use the existing executableName
extraction/normalization logic) so only invocations whose argv[0] lowercased
lastPathComponent equals "cmux" will return true.
🪄 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: 35fa2bc0-4393-4388-bd76-aacdf2f742d6
📒 Files selected for processing (3)
Sources/CmuxTopSnapshot.swiftSources/CmuxTopSnapshotScopeCache.swiftcmuxTests/CmuxTopSnapshotScopeTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0d65f30. Configure here.
Merged
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
2 participants
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
Fixes #4189.
This is focused on the WebKit/helper-process attribution gap called out from the broader Task Manager accuracy work in #4129. It does not touch the WKInspectorViewController crash path.
Changes
cmux hooks codex monitor --workspace/--surfacearguments as a fallback ownership signal when the helper has been reparented tolaunchdand environment-based scope is unavailable.cmux hooks codex monitorcommand shape for hook monitor attribution, including a realcmuxexecutable inargv[0], and support both--workspace <uuid>and--workspace=<uuid>option forms.attribution_reasonon process rows.cmux top --processestree output, includingcmux-environment,cmux-hook-arguments, andwebview-root-pid, so counted helper processes can be explained.Local reproduction
psshowed 99 launchd-parentedcom.apple.WebKit.WebContentprocesses totaling about 6.3 GB RSS on this machine.psshowed launchd-parentedcmux hooks codex monitorprocesses with--workspace/--surfaceownership arguments./Applications/cmux.app/Contents/Resources/bin/cmux top --processes --flat --format tsvshowed WebKit roots already flowing through WebView PID roots, but hook monitor attribution depended on environment scope only.Verification
58f244ef4.