Render Grok Build PreToolUse Feed decisions in Grok-native shape (#6303) - #6316
Closed
austinywang wants to merge 3 commits into
Closed
austinywang wants to merge 3 commits into
austinywang wants to merge 3 commits into
Conversation
A single pane running a leaking process (e.g. uv run pytest growing to ~14 GB RSS) makes macOS aggregate the child memory under the app, report hundreds of GB, declare "out of application memory", and OOM-suspend the whole app — killing every other healthy pane with no prior signal. This adds a per-pane guardrail that catches a runaway tree at the pane level first: - A background timer (PaneMemoryGuardrail) polls every live pane ~every 4s. It attributes process-tree memory by the pane's controlling tty: every process under the pane (shell + descendants + background jobs) shares the tty, so it sums physical-footprint bytes across all pids on that tty device via the existing CmuxTopProcessSnapshot libproc walk. - When a pane crosses a configurable threshold (default 8 GB) it edge-triggers an orange warning badge on the workspace tab and a dismissible banner identifying the pane, its process-tree memory, and the foreground command. Hysteresis clears at 0.8x threshold; the banner fires once per crossing and re-arms after it clears. - The banner's "Kill Pane Process" action (with confirm) sends SIGTERM then SIGKILL to the pane's foreground process group, leaving the shell alive; falls back to closing the pane when there is no foreground group. - New Terminal settings: enable toggle + threshold (GB), default on / 8 GB. - Below threshold it stays completely silent (no always-on memory UI). Ghostty foreground-pid / tty-name accessors added on TerminalSurface. Pure edge-trigger engine unit-tested (PaneMemoryGuardrailTests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cmux hooks feed --source grok currently emits Claude-shaped
nonClaudePreToolDecision JSON for a resolved Feed permission, which Grok
Build cannot parse — so the PreToolUse hook only clears via the 120s
fail-open timeout. This test asserts the Grok-native
{"decision":"allow|deny","reason":...} shape and fails until the fix lands.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Route source==grok through the Antigravity native-decision branch so a
resolved Feed permission emits {"decision":"allow|deny","reason":...}
instead of Claude's hookSpecificOutput/"approve" shape that Grok cannot
parse. This lets an approved Write/Bash clear immediately instead of
waiting out the 120s PreToolUse fail-open timeout.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughFixes Grok PreToolUse feed decisions by extending the existing Antigravity native-shape branch in ChangesGrok Feed Decision Fix (issue
Pane Memory Guardrail
Sequence Diagram(s)sequenceDiagram
rect rgba(255, 165, 0, 0.5)
Note over AppDelegate,PaneMemoryGuardrail: App Startup
end
AppDelegate->>PaneMemoryGuardrail: startPaneMemoryGuardrailIfNeeded (configure + start)
rect rgba(100, 149, 237, 0.5)
Note over PaneMemoryGuardrail,CmuxTopProcessSnapshot: Periodic Scan Tick
end
PaneMemoryGuardrail->>PaneMemoryGuardrail: paneProvider() → [PaneMemoryDescriptor]
PaneMemoryGuardrail->>CmuxTopProcessSnapshot: compute process-tree memory off-main
CmuxTopProcessSnapshot-->>PaneMemoryGuardrail: [PaneMemorySample]
PaneMemoryGuardrail->>PaneMemoryGuardrailEngine: ingest(samples)
PaneMemoryGuardrailEngine-->>PaneMemoryGuardrail: newBanners, warnedIds, clearedPanes
PaneMemoryGuardrail->>SidebarUnreadModel: setMemoryWarningWorkspaceIds(warnedIds)
PaneMemoryGuardrail->>PaneMemoryGuardrailBanner: publish activeBanner
rect rgba(220, 20, 60, 0.5)
Note over PaneMemoryGuardrailBanner,PaneMemoryProcessKiller: User Kill Action
end
PaneMemoryGuardrailBanner->>PaneMemoryGuardrail: killActivePaneProcess()
PaneMemoryGuardrail->>PaneMemoryProcessKiller: killProcessGroups(pgids)
PaneMemoryProcessKiller->>PaneMemoryProcessKiller: SIGTERM → grace period → SIGKILL
PaneMemoryGuardrail->>PaneMemoryGuardrailEngine: acknowledge(paneKey)
PaneMemoryGuardrail->>SidebarUnreadModel: setMemoryWarningWorkspaceIds(updated)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 delivers two independent changes: a one-line fix routing
Confidence Score: 3/5The Grok fix and memory guardrail engine are safe, but the kill confirmation dialog can send SIGTERM/SIGKILL to the wrong pane if the active banner changes during the confirmation window, and the SIGKILL grace-period dispatch uses a flagged legacy pattern. The Grok routing fix is a trivially correct one-liner. The memory guardrail engine logic and SwiftUI wiring are solid. Two issues in the guardrail reduce confidence: the kill confirmation dialog reads Sources/PaneMemoryGuardrailBannerView.swift (kill confirmation targets wrong pane if banner changes) and Sources/PaneMemoryGuardrail.swift (asyncAfter + DispatchQueue.global in PaneMemoryProcessKiller). Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant G as Grok Build
participant H as cmux hooks feed --source grok
participant F as Feed socket (cmux app)
participant U as User (Feed sidebar)
G->>H: stdin: PreToolUse JSON
H->>F: feed.push (blocks waiting for decision)
F->>U: show permission card
U->>F: approve / deny
F->>H: "resolved decision {kind:permission, mode:once|deny}"
Note over H: renderAgentDecision() source=="grok" native branch
H->>G: "stdout: {"decision":"allow","reason":"..."}"
Note over G: Unblocks immediately (was: 120s timeout)
%%{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"}}}%%
sequenceDiagram
participant G as Grok Build
participant H as cmux hooks feed --source grok
participant F as Feed socket (cmux app)
participant U as User (Feed sidebar)
G->>H: stdin: PreToolUse JSON
H->>F: feed.push (blocks waiting for decision)
F->>U: show permission card
U->>F: approve / deny
F->>H: "resolved decision {kind:permission, mode:once|deny}"
Note over H: renderAgentDecision() source=="grok" native branch
H->>G: "stdout: {"decision":"allow","reason":"..."}"
Note over G: Unblocks immediately (was: 120s timeout)
Reviews (1): Last reviewed commit: "Render Grok Feed decisions in Grok-nativ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+157
to
+161
| DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + graceSeconds) { | ||
| for pgid in pgids { | ||
| _ = kill(pid_t(-pgid), SIGKILL) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
DispatchQueue.global and asyncAfter are both flagged patterns in new production Swift. asyncAfter is a wall-clock timing primitive where either a DispatchSourceProcess (fire on process exit) or Swift concurrency (Task { try? await Task.sleep(for: .seconds(graceSeconds)); ... }) is the correct shape. DispatchQueue.global for ordinary async work should also be replaced with a Swift concurrency Task. The minimal mechanical fix switches to Swift concurrency; the more robust fix would use a DispatchSourceProcess to watch for the process to exit after SIGTERM and only escalate if it hasn't.
Suggested change
| DispatchQueue.global(qos: .userInitiated).asyncAfter(deadline: .now() + graceSeconds) { | |
| for pgid in pgids { | |
| _ = kill(pid_t(-pgid), SIGKILL) | |
| } | |
| } | |
| Task.detached(priority: .userInitiated) { | |
| try? await Task.sleep(for: .seconds(graceSeconds)) | |
| for pgid in pgids { | |
| _ = kill(pid_t(-pgid), SIGKILL) | |
| } | |
| } |
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!
Comment on lines
+57
to
+91
| Button(role: .destructive) { | ||
| isConfirmingKill = true | ||
| } label: { | ||
| Text(String( | ||
| localized: "paneMemoryGuardrail.banner.kill", | ||
| defaultValue: "Kill Pane Process" | ||
| )) | ||
| } | ||
| .buttonStyle(.borderedProminent) | ||
| .controlSize(.small) | ||
| .confirmationDialog( | ||
| String( | ||
| localized: "paneMemoryGuardrail.confirm.title", | ||
| defaultValue: "Kill this pane's runaway process?" | ||
| ), | ||
| isPresented: $isConfirmingKill, | ||
| titleVisibility: .visible | ||
| ) { | ||
| Button(role: .destructive) { | ||
| guardrail.killActivePaneProcess() | ||
| } label: { | ||
| Text(String( | ||
| localized: "paneMemoryGuardrail.confirm.kill", | ||
| defaultValue: "Kill Process" | ||
| )) | ||
| } | ||
| Button(role: .cancel) {} label: { | ||
| Text(String(localized: "paneMemoryGuardrail.confirm.cancel", defaultValue: "Cancel")) | ||
| } | ||
| } message: { | ||
| Text(String( | ||
| localized: "paneMemoryGuardrail.confirm.message", | ||
| defaultValue: "This sends SIGTERM then SIGKILL to the foreground process group in the pane. The shell stays open." | ||
| )) | ||
| } |
Contributor
There was a problem hiding this comment.
isConfirmingKill is @State on the view and persists through re-renders. If a second pane crosses the threshold within the 4-second poll window while this dialog is open, activeBanner updates to that new pane. The user sees the dialog they opened for pane A, confirms, and killActivePaneProcess() reads the current activeBanner — silently sending SIGTERM/SIGKILL to pane B's foreground process group. Capturing the target pane's key at the moment the Kill button is pressed and passing it through to the kill call avoids the race.
Comment on lines
+171
to
+172
| @MainActor | ||
| final class PaneMemoryGuardrail: ObservableObject { |
Contributor
There was a problem hiding this comment.
PaneMemoryGuardrail is a new @MainActor final class; @Observable (Swift 5.9+) is the current preferred shape for this pattern in the codebase. ObservableObject/@Published causes whole-view invalidation on any published change whereas @Observable gives property-granular tracking and removes the need for @ObservedObject at the call site.
Suggested change
| @MainActor | |
| final class PaneMemoryGuardrail: ObservableObject { | |
| @MainActor | |
| @Observable | |
| final class PaneMemoryGuardrail { |
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!
Contributor
Author
|
Superseding this PR per maintainer decision: it was produced by a fleet worker that was interrupted mid-run (its worktree was torn down before the build/verify step), so it's likely unverified. A fresh worker is redoing #6303 cleanly with a verified build and will open a replacement PR. Reopen if this work is preferred. |
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.
Closes #6303
Root cause
When a Grok Build PreToolUse hook routes through
cmux hooks feed --source grok, cmux blocks waiting for the user's Feed sidebar decision and then writes the decision to stdout for Grok to honor. ButrenderAgentDecisionhad nogrokbranch, so a resolved permission fell through tononClaudePreToolDecision, which emits Claude-shaped JSON:{"hookSpecificOutput":{"hookEventName":"PreToolUse","permissionDecision":"allow",...},"decision":"approve",...}Grok Build (like Antigravity, and per its hook docs) only honors a native blocking decision of the shape:
{"decision":"allow"|"deny","reason":"…"}It does not understand
hookSpecificOutput.permissionDecisionnor the"approve"/"block"values. So an approvedWrite/Bashwas never recognized, and the hook only ever cleared via the 120s fail-open timeout — adding ~2 minutes of dead time to every side-effecting tool call.Fix
Route
source == "grok"through the existing Antigravity native-decision branch inrenderAgentDecision(CLI/cmux.swift), so a resolved Feed permission emits{"decision":"allow"|"deny","reason":…}. One-line, mirrors the already-shippedantigravityhandling; no behavior change for any other agent.This is root cause #3 from the issue. The other suggested items (single pretool wrapper, dedup of stacked feed hooks, side-effecting tool aliases for Grok-native lowercase tool names) are deliberately out of scope here: the reported repro uses Cursor-style
Write/Bash, which already classify correctly, and the duplicate-hook/pruning work involves the user-side hook-install architecture rather than a clean cmux code fix. This PR fixes the unambiguous, on-the-repro-path decision-format mismatch.Verification
Two-commit red/green structure:
cmuxTests/CLIGrokFeedDecisionTests.swift(wired intocmux.xcodeproj), which drives the real bundled CLI against a mock Feed socket that resolves a pending permission, and asserts the emitted stdout is the Grok-native shape (decision==allow/deny, nohookSpecificOutput). It is red without the fix.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes Grok PreToolUse decision formatting so approved tool calls unblock immediately, and adds a per-pane runaway-memory guardrail to prevent one leaking process from freezing the app. Fixes #6303; addresses #6313.
New Features
Bug Fixes
--source grokPreToolUse hooks, emit Grok-native decisions:{"decision":"allow"|"deny","reason":...}. This removes the 120s fail-open delay and matches the Antigravity path.Written for commit 3b36084. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests