Repository navigation
fix: gate Copilot PreToolUse feed hooks - #6577
Closed
austinywang wants to merge 55 commits into
Closed
austinywang wants to merge 55 commits into
austinywang wants to merge 55 commits into
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:
📝 WalkthroughWalkthroughThe Copilot hook configuration is moved from ChangesCopilot PreToolUse Approval Gating
Sequence Diagram(s)sequenceDiagram
participant CopilotCLI as GitHub Copilot CLI
participant cmuxFeed as cmux hooks feed
participant FeedEventClassifier
participant PermissionServer as Permission Server
CopilotCLI->>cmuxFeed: PreToolUse event (toolName, env vars)
cmuxFeed->>FeedEventClassifier: classify("copilot", "PreToolUse", tool)
FeedEventClassifier-->>cmuxFeed: PermissionRequest (actionable=true)
cmuxFeed->>PermissionServer: feed.push {hook_event_name: "PermissionRequest", _source: "copilot"}
PermissionServer-->>cmuxFeed: {mode: "allow"} or {mode: "deny"}
alt mode == "deny"
cmuxFeed-->>CopilotCLI: {permissionDecision: "deny", permissionDecisionReason: "User denied"}
else mode == "allow"
cmuxFeed-->>CopilotCLI: {permissionDecision: "allow"}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 22 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (22 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 three root causes that prevented Copilot
Confidence Score: 5/5Safe to merge. All three root causes are addressed consistently across the Swift backend, CLI, and feed-tui; the fail-closed deny path is exercised at every timeout and error site; and localization coverage is complete for all 20 catalog locales. The classification, serialization, and install-path fixes are mutually consistent and covered by new integration tests that run the actual CLI binary and verify the installed JSON, the shell command's fail-closed deny output, and the decision wire format. Permission-mode policy (persistent modes and bypass disabled for Copilot) is applied symmetrically in the Swift backend (FeedPermissionActionPolicy), the CLI bridge (CodexTeamsApprovalBridge), and the feed-tui TypeScript. No actor isolation, blocking runtime, or i18n gaps were found. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant C as Copilot Agent
participant H as Shell Hook
participant CLI as cmux CLI hooks feed
participant FC as FeedEventClassifier
participant F as Feed Socket
participant U as User (Feed TUI)
C->>H: "preToolUse {toolName: bash, ...}"
Note over H: matcher filters to side-effecting tools only
H->>CLI: cmux hooks feed --source copilot --event preToolUse
CLI->>FC: classify(copilot, preToolUse, bash)
FC-->>CLI: "PermissionRequest, isActionable=true"
CLI->>F: Send approval request (block 120s)
F->>U: Show approval card
alt User approves (Once)
U-->>F: "decision: kind=permission, mode=once"
F-->>CLI: result
CLI->>H: "stdout: permissionDecision=allow"
H-->>C: allow tool
else User denies
U-->>F: "decision: kind=permission, mode=deny"
F-->>CLI: result
CLI->>H: "stdout: permissionDecision=deny"
H-->>C: block tool
else Timeout or cmux unavailable or no Feed socket
CLI->>H: "stdout: permissionDecision=deny (fail-closed)"
H-->>C: block tool
end
%%{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 C as Copilot Agent
participant H as Shell Hook
participant CLI as cmux CLI hooks feed
participant FC as FeedEventClassifier
participant F as Feed Socket
participant U as User (Feed TUI)
C->>H: "preToolUse {toolName: bash, ...}"
Note over H: matcher filters to side-effecting tools only
H->>CLI: cmux hooks feed --source copilot --event preToolUse
CLI->>FC: classify(copilot, preToolUse, bash)
FC-->>CLI: "PermissionRequest, isActionable=true"
CLI->>F: Send approval request (block 120s)
F->>U: Show approval card
alt User approves (Once)
U-->>F: "decision: kind=permission, mode=once"
F-->>CLI: result
CLI->>H: "stdout: permissionDecision=allow"
H-->>C: allow tool
else User denies
U-->>F: "decision: kind=permission, mode=deny"
F-->>CLI: result
CLI->>H: "stdout: permissionDecision=deny"
H-->>C: block tool
else Timeout or cmux unavailable or no Feed socket
CLI->>H: "stdout: permissionDecision=deny (fail-closed)"
H-->>C: block tool
end
Reviews (36): Last reviewed commit: "test: avoid suite captures in migrated h..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/notifications.md (1)
120-155:⚠️ Potential issue | 🟡 MinorAdd the required
versionfield to the Copilot CLI hooks configuration.The JSON example is missing the top-level
version: 1field required by Copilot CLI. Without it, the configuration will fail to load.Proposed fix
{ + "version": 1, "hooks": {🤖 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 `@docs/notifications.md` around lines 120 - 155, The JSON example in the cmux.json hooks configuration is missing the required top-level version field that Copilot CLI needs to properly parse the configuration. Add a version field set to 1 at the root level of the JSON object, before the hooks object, so the complete structure includes both version and hooks properties at the top level.web/app/[locale]/docs/notifications/page.tsx (1)
269-300:⚠️ Potential issue | 🟡 MinorAdd the required top-level
versionfield.The rendered hook example at lines 269–300 is missing
version: 1, while the.github/hooks/notify.jsonexample immediately below (lines 302–308) correctly includes it. Users copying the cmux.json example will get an invalid hook file that Copilot CLI will reject. GitHub's official hook schema requiresversion: 1as a top-level field.🛠️ Proposed fix
- <CodeBlock title="~/.copilot/hooks/cmux.json" lang="json">{`{ + <CodeBlock title="~/.copilot/hooks/cmux.json" lang="json">{`{ + "version": 1, "hooks": {🤖 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 `@web/app/`[locale]/docs/notifications/page.tsx around lines 269 - 300, The CodeBlock component displaying the cmux.json hook example is missing the required top-level version field that the hook schema mandates. Add "version": 1 as the first property in the JSON object within the CodeBlock template string, positioning it before the "hooks" property. This will make the example valid and consistent with the notify.json example shown immediately below it.
🤖 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 `@docs/notifications.md`:
- Around line 120-155: The JSON example in the cmux.json hooks configuration is
missing the required top-level version field that Copilot CLI needs to properly
parse the configuration. Add a version field set to 1 at the root level of the
JSON object, before the hooks object, so the complete structure includes both
version and hooks properties at the top level.
In `@web/app/`[locale]/docs/notifications/page.tsx:
- Around line 269-300: The CodeBlock component displaying the cmux.json hook
example is missing the required top-level version field that the hook schema
mandates. Add "version": 1 as the first property in the JSON object within the
CodeBlock template string, positioning it before the "hooks" property. This will
make the example valid and consistent with the notify.json example shown
immediately below it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52b2a30c-fb94-485b-8ec7-83f5754e1204
📒 Files selected for processing (9)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/FeedEventClassifier.swiftCLI/cmux.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/FeedEventClassificationTests.swiftdocs/agent-hooks.mddocs/feed.mddocs/notifications.mdweb/app/[locale]/docs/notifications/page.tsx
austinywang
force-pushed
the
issue-6574-copilot-pretooluse-hook
branch
from
June 22, 2026 08:09
110987f to
899cca3
Compare
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 `@CLI/cmux.swift`:
- Line 28237: Combine the two separate if statements that both assign the same
value to existing["version"] into a single conditional statement using a logical
OR operator. Replace the semicolon-separated statements with a single if
statement that checks both conditions: if the def.format is .flat OR if
def.requiresVersionKey is true, then set existing["version"] = 1. This improves
readability and follows Swift style conventions by avoiding semicolon-separated
statements on a single line.
- Line 33245: The single-line conditional in the source == "copilot" check
exceeds 280 characters and contains nested ternary operators, dictionary
literals, and localized string initialization that reduce readability. Refactor
this by extracting intermediate variables: first create a variable for the
permission decision based on the mode == "deny" condition, then construct the
result dictionary separately (including the localized string for
permissionDecisionReason when mode is "deny"), and finally pass the dictionary
to the encode function. This will break the logic across multiple lines and
significantly improve maintainability without changing the behavior.
🪄 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: 5ce88d3a-03ce-49fa-98eb-f1545ffa4ae6
📒 Files selected for processing (6)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/cmux.swiftResources/Localizable.xcstringscmux.xcodeproj/project.pbxprojdocs/notifications.mdweb/app/[locale]/docs/notifications/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/CLICopilotHookFeedTests.swift (1)
136-136: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHardcoded English string creates locale-dependent test.
This test assumes the CLI runs in English locale and compares against the hardcoded English denial reason. If the test environment's locale differs, the assertion will fail even though the behavior is correct.
Consider one of these approaches:
- Load the localized string dynamically from the
cli.hooks.feed.permissionDeniedReasonkey for comparison- Assert that
permissionDecisionReasonis non-empty rather than checking exact text- Document that the test requires English locale
💡 Example: non-empty assertion approach
- XCTAssertEqual(denyOutput["permissionDecisionReason"] as? String, "User denied permission via cmux Feed.") + XCTAssertFalse((denyOutput["permissionDecisionReason"] as? String ?? "").isEmpty, "Expected non-empty denial reason")🤖 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 `@cmuxTests/CLICopilotHookFeedTests.swift` at line 136, The XCTAssertEqual assertion in CLICopilotHookFeedTests.swift hardcodes an English string "User denied permission via cmux Feed." which causes test failures in non-English locales. Replace this exact string comparison with either a dynamic lookup of the localized string from the cli.hooks.feed.permissionDeniedReason key, or change the assertion to simply verify that permissionDecisionReason is non-empty instead of matching specific text, ensuring the test passes regardless of the system locale.
🤖 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/FeedEventClassifier.swift`:
- Around line 112-114: Add a comment above the if statement checking for source
== "copilot" that explains why Copilot events require the special case. The
comment should clarify that Copilot approvals must use the generic
"PermissionRequest" wire event (rather than tool-specific events) because
Copilot's permission response format uses a top-level permissionDecision JSON
structure instead of tool-specific decision types.
In `@cmuxTests/FeedEventClassificationTests.swift`:
- Around line 104-105: The test assertions for AskUserQuestion and ExitPlanMode
tools in the classify function calls are incomplete. Currently, lines 104-105
only verify the name property matches "PermissionRequest", but they should also
verify that actionable is true, consistent with the assertions for Bash and Read
tools on lines 100-103. Update both classify calls for AskUserQuestion and
ExitPlanMode to include checks that the actionable property equals true,
mirroring the pattern used for the other tool types.
---
Outside diff comments:
In `@cmuxTests/CLICopilotHookFeedTests.swift`:
- Line 136: The XCTAssertEqual assertion in CLICopilotHookFeedTests.swift
hardcodes an English string "User denied permission via cmux Feed." which causes
test failures in non-English locales. Replace this exact string comparison with
either a dynamic lookup of the localized string from the
cli.hooks.feed.permissionDeniedReason key, or change the assertion to simply
verify that permissionDecisionReason is non-empty instead of matching specific
text, ensuring the test passes regardless of the system locale.
🪄 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: dbde51b9-9e46-411a-a8a7-5c36f49dc8be
📒 Files selected for processing (6)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/FeedEventClassifier.swiftCLI/cmux.swiftResources/Localizable.xcstringscmuxTests/CLICopilotHookFeedTests.swiftcmuxTests/FeedEventClassificationTests.swift
…etooluse-hook # Conflicts: # .github/swift-file-length-budget.tsv
6 of 7 tasks
|
@austinywang is this abandoned? can I re-open my other PR #6503 then so we can merge something in that enables hooks for Github Copilot users with a short-term fix? |
lawrencecchen
added
the
stale-revisit
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
4 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
Closes #6574
Root cause: Copilot
PreToolUsefeed events were not treated as Copilot permission-gate events end to end. The feed card could fall back to telemetry/non-Copilot response serialization, and resolved decisions were emitted in Claude-stylehookSpecificOutputJSON that Copilot does not process. The installed Copilot hook file also still targeted the old config path without the required hooksversion.Fix: classify Copilot
PreToolUseas a blocking Feed approval request, serialize resolved Copilot decisions as top-levelpermissionDecision: allow|deny, and install Copilot hooks to~/.copilot/hooks/cmux.jsonwithversion: 1.Testing
PreToolUseclassification, install path/version, and Copilot hook stdout decision JSON.Notes
preToolUseuses top-levelpermissionDecision(allow,deny, orask).Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Gates GitHub Copilot
preToolUseas a blocking approval and returns a top-levelpermissionDecision. Installs v1 hooks at~/.copilot/hooks/cmux.jsonusing Copilot’s JSON schema; addresses #6574.Bug Fixes
permissionRequestand gatedpreToolUseat~/.copilot/hooks/cmux.json(v1) with direct{"type":"command","bash","timeoutSec"}entries and amatcherfor side‑effecting tools andask_user; keep camelCase event names; remove stale telemetry hooks; preserve user hooks; update docs and samples.permissionRequestand actionablepreToolUseas approvals; read‑onlypreToolUseis telemetry. KeeppermissionRequestfor existing installs and policy denials; support--telemetry-only.preToolUseoutput top‑levelpermissionDecisionwith a localized deny reason. ForpermissionRequestdenials emit{"behavior":"deny","message":…}; approvals are no‑ops.cmux, or when the Feed socket is missing; add 120s + 5s slack topreToolUse. RouteerrorOccurredvianotificationwith structured error parsing. Disable persistent permission modes and bypass for Copilot in UI and backend.Migration
cmux hooks copilot installto write~/.copilot/hooks/cmux.json.Written for commit cb1ea2d. Summary will update on new commits.
Summary by CodeRabbit
PreToolUseroutes through actionable approval/deny handling.~/.copilot/config.jsonto~/.copilot/hooks/cmux.jsonacross docs.