Repository navigation
Fix main app-host shard regressions - #6580
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactors native agent process kind validation from CLI into ChangesNative agent kind validation in AgentLaunchCaptureTrust
Inspector dock promotion and sidebar metrics
Session command and resume handling updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThree targeted CI regression fixes from the global font magnification merge: agent PID trust classification moved into
Confidence Score: 5/5All three fixes are tightly scoped, fully tested, and address specific CI failures without introducing new state or timing dependencies. The agent trust logic is cleanly extracted into a testable package with comprehensive coverage of alias resolution, node/bun path detection, and negative cases. The inspector docking change removes a known-bad synchronous call. The font-size and shell-quoting changes are mechanical and verified by updated snapshot tests. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Hook fires with fallbackPID + hookKind] --> B{envCaptureIsTrusted?}
B -- yes --> C[Use CMUX_AGENT_LAUNCH_ARGV_B64]
B -- no --> D[envArguments = nil]
D --> E[processArguments = fallbackPID.flatMap]
E --> F[Read processName + argv for PID]
F --> G{nativeProcessDescribesKind
processName, argv, hookKind?}
G -- no --> H[processArguments = nil]
G -- yes --> I{argvLooksLikeShellWrapper?}
I -- yes --> J[Walk child PIDs for agent process]
I -- no --> K[Use PID argv as resume command]
H --> L[environmentOnlyRecord]
C --> M{argvLooksLikeShellWrapper?}
M -- yes --> J
M -- no --> N[Use env argv as resume command]
J --> O{found agent child?}
O -- yes --> P[Use child argv]
O -- no --> L
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Hook fires with fallbackPID + hookKind] --> B{envCaptureIsTrusted?}
B -- yes --> C[Use CMUX_AGENT_LAUNCH_ARGV_B64]
B -- no --> D[envArguments = nil]
D --> E[processArguments = fallbackPID.flatMap]
E --> F[Read processName + argv for PID]
F --> G{nativeProcessDescribesKind
processName, argv, hookKind?}
G -- no --> H[processArguments = nil]
G -- yes --> I{argvLooksLikeShellWrapper?}
I -- yes --> J[Walk child PIDs for agent process]
I -- no --> K[Use PID argv as resume command]
H --> L[environmentOnlyRecord]
C --> M{argvLooksLikeShellWrapper?}
M -- yes --> J
M -- no --> N[Use env argv as resume command]
J --> O{found agent child?}
O -- yes --> P[Use child argv]
O -- no --> L
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func nativeAgentProcessKind(for hookKind: String) -> HookAgentProcessKind? { | ||
| switch hookKind.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() { | ||
| case "codex": | ||
| return .codex | ||
| case "claude": | ||
| return .claude | ||
| default: | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
nativeAgentProcessKind(for pid:) instance method
The new static method nativeAgentProcessKind(for hookKind: String) shares the for external label with the existing instance method nativeAgentProcessKind(for pid: pid_t) at line 26355. Swift resolves them correctly by type and static/instance distinction, and the Self. call-site prefix makes the intent clear. However, a more distinctive label — e.g., nativeAgentProcessKind(matchingKind:) — would eliminate the need for readers to mentally disambiguate two identically-named selectors with different semantics (string kind → enum vs. PID → enum).
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| var processArguments: [String]? | ||
| if let fallbackPID { | ||
| let pid = pid_t(fallbackPID) | ||
| let candidate = self.processArguments(for: pid) | ||
| if let expectedKind = Self.nativeAgentProcessKind(for: fallbackKind), | ||
| let candidate, | ||
| Self.nativeAgentProcessKind( | ||
| processName: processName(for: pid), | ||
| arguments: candidate | ||
| ) == expectedKind { | ||
| processArguments = candidate | ||
| } | ||
| } |
There was a problem hiding this comment.
omx / omc hook kinds
nativeAgentProcessKind(for: fallbackKind) returns nil for any kind that is not literally "codex" or "claude", so processArguments stays nil for omx and omc hooks even when a valid PID is provided and the live process would correctly classify as .claude. If an omx/omc hook fires without CMUX_AGENT_LAUNCH_ARGV_B64 (e.g., an unmanaged Claude invocation) and env capture is untrusted, the launcher now falls through to environmentOnlyRecord() where it previously had a full argv. Is this intentional — i.e., are omx/omc hooks guaranteed to always carry the env-capture argv? Are omx and omc hook kinds always expected to have CMUX_AGENT_LAUNCH_ARGV_B64 available in the environment, making the PID fallback unnecessary for those kinds? Or is there a scenario where they'd rely on PID fallback for resume-command construction?
f57b4f5 to
6ac5fa2
Compare
6ac5fa2 to
4c6483c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchCaptureTrustTests.swift`:
- Around line 45-90: The test testPIDProcessMetadataMustMatchHookKind lacks
comprehensive edge case coverage for the AgentLaunchCaptureTrust validation
logic. Add additional test cases using XCTAssertTrue and XCTAssertFalse with
calls to nativeProcessDescribesKind and nativeProcessDescribesKnownAgent to
cover: nil or empty processName values, empty arguments arrays, the bun runtime
similar to how node is currently tested, and path-based detection patterns such
as "/codex/codex" and "/.claude/" to ensure the inference logic correctly
handles these scenarios.
🪄 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: df64b164-bfdb-404e-94f7-9df474eb93c4
📒 Files selected for processing (6)
CLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchCaptureTrust.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchCaptureTrustTests.swiftSources/Panels/BrowserPanelView.swiftSources/WindowChromeMetrics.swiftcmuxTests/CLIRovoDevHookPersistenceTests.swift
| func testPIDProcessMetadataMustMatchHookKind() { | ||
| XCTAssertTrue( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKind( | ||
| processName: "codex", | ||
| arguments: ["/opt/homebrew/bin/codex", "--sandbox", "workspace-write"], | ||
| kind: "codex" | ||
| ) | ||
| ) | ||
| XCTAssertTrue( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKnownAgent( | ||
| processName: "codex", | ||
| arguments: ["/opt/homebrew/bin/codex", "--sandbox", "workspace-write"] | ||
| ) | ||
| ) | ||
| XCTAssertTrue( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKind( | ||
| processName: "node", | ||
| arguments: ["node", "/Users/alice/.claude/local/claude.js"], | ||
| kind: "claude" | ||
| ) | ||
| ) | ||
| XCTAssertFalse( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKind( | ||
| processName: "cmux DEV", | ||
| arguments: [ | ||
| "/tmp/cmux-tests/Build/Products/Debug/cmux DEV.app/Contents/MacOS/cmux DEV", | ||
| "-NSTreatUnknownArgumentsAsOpen", | ||
| ], | ||
| kind: "codex" | ||
| ) | ||
| ) | ||
| XCTAssertFalse( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKind( | ||
| processName: "codex", | ||
| arguments: ["/opt/homebrew/bin/codex"], | ||
| kind: "claude" | ||
| ) | ||
| ) | ||
| XCTAssertFalse( | ||
| AgentLaunchCaptureTrust.nativeProcessDescribesKind( | ||
| processName: "agy", | ||
| arguments: ["/usr/local/bin/agy"], | ||
| kind: "antigravity" | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding edge case test coverage.
The current test covers the main validation scenarios from the PR objective (rejecting test-host/app arguments, matching native kinds). Consider adding tests for:
nilprocessName- Empty arguments array
[] bunruntime (currently only testsnode)- Path-based detection patterns (
"/codex/codex","/.claude/")
These additions would provide more comprehensive coverage of the inference logic, but the current test suite adequately validates the core requirement.
🤖 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
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchCaptureTrustTests.swift`
around lines 45 - 90, The test testPIDProcessMetadataMustMatchHookKind lacks
comprehensive edge case coverage for the AgentLaunchCaptureTrust validation
logic. Add additional test cases using XCTAssertTrue and XCTAssertFalse with
calls to nativeProcessDescribesKind and nativeProcessDescribesKnownAgent to
cover: nil or empty processName values, empty arguments arrays, the bun runtime
similar to how node is currently tested, and path-based detection patterns such
as "/codex/codex" and "/.claude/" to ensure the inference logic correctly
handles these scenarios.
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 ecb20f1. Configure here.
| descriptor == expectedKind | ||
| || nativeProcessAliasesByKind[expectedKind]?.contains(descriptor) == true | ||
| || descriptor == "\(expectedKind)-cli" | ||
| } |
There was a problem hiding this comment.
Teams wrapper PID trust missing
Medium Severity
nativeProcessDescribesKind does not treat codexteams / claudeteams as matching codex / claude, unlike launcherDescribesKind. When inherited CMUX_AGENT_LAUNCH_* env is distrusted and PID fallback runs, argv from those wrapper processes is dropped and resume capture may fall back to env-only or fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ecb20f1. Configure here.
…e silently dropped — #6518 (terminal input after window key restore), #6508 (defer restored WebViews until visible), #6559 (avoid DevTools teardown on redock), #6583 (gate idle port scanning to active workspace), #6528 (canvas scroll-hint debug menu), #6580 (drop re-introduced sideDock promote block). #6582/#6517 deferred (see merge-deferred-gaps)
This branch was successfully deployed
2 participants
Fixes the current main CI app-host shard 4 regressions after the global font magnification merge.
Changes:
Checks run locally:
Main failure context: https://github.com/manaflow-ai/cmux/actions/runs/27937219981
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Agent launch trust changes affect persisted resume/fork commands across many agents; inspector and layout tweaks touch drag routing but are localized UI behavior.
Overview
Addresses main app-host CI shard 4 failures after global font magnification: UI metrics, hosted inspector docking, and agent resume capture.
Right sidebar chrome scales compact control text from 12pt (was 13pt) so height stays 20pt at 100% magnification while still growing above default.
Hosted WebKit inspector side-dock promotion no longer forces adaptive bottom dock during synchronous promotion; it activates the side dock immediately so divider/drag routing stays symmetric, with bottom-dock enforcement deferred to layout once the managed container owns the split.
Agent launch capture moves native process classification into
AgentLaunchCaptureTrust(nativeProcessDescribesKind/nativeProcessDescribesKnownAgentwith per-agent aliases). The CLI drops local Codex/Claude-only logic and rejects PID-derived argv unless the process matches the hook’s agent kind—blocking cmux app / Xcode test-host argv from becoming persisted resume commands.Smaller aligned updates:
SessionIndexModelsresume commands useTerminalStartupWorkingDirectoryPrefix; remote relay awk uses\047for quote stripping; tests expect updated resume strings, hook config assertions, symlink-resolved vault URLs, tmux overlay rects, and workspace rail color hex.Reviewed by Cursor Bugbot for commit ecb20f1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes app-host CI shard 4 regressions from global font magnification. Stabilizes right-sidebar sizing, inspector docking, and agent resume command capture to unblock CI.
Bug Fixes
grok,kiro,rovodev,pi); persisted resume commands now use a safe working-directory prefix viaTerminalStartupWorkingDirectoryPrefix.Refactors
CMUXAgentLaunch(AgentLaunchCaptureTrust) withnativeProcessDescribesKind/nativeProcessDescribesKnownAgent; CLI uses these for PID fallback and ancestry checks with new unit tests.Written for commit ecb20f1. Summary will update on new commits.
Summary by CodeRabbit