Repository navigation
Fix cmux claude-teams hook injection gap (#2229 follow-up to #2465) - #2629
pstanton237 wants to merge 1 commit into
Conversation
|
@pstanton237 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR modifies Claude Teams launch configuration to support hook injection for command notifications. Changes include a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR fixes issue #2229 by mirroring four hook-injection steps from
Confidence Score: 3/5Safe to merge for the primary fix path; a logic gap means users who explicitly pass --teammate-mode still lose hook injection The core fix is correct and well-structured, matching the Resources/bin/claude contract, and the two-commit CI structure follows CLAUDE.md policy. Score reduced from 5 because the claudeTeamsHasExplicitTeammateMode early return inadvertently skips --settings injection — reintroducing the original symptom for any explicit --teammate-mode invocation — and there is no regression test covering that scenario. CLI/cmux.swift lines 10279–10288 (claudeTeamsLaunchArguments guard logic) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant cmux as cmux CLI
participant rCT as runClaudeTeams
participant cTLA as claudeTeamsLaunchArguments
participant Claude as claude binary (execv)
User->>cmux: cmux claude-teams [args]
cmux->>rCT: commandArgs=[args]
rCT->>rCT: configureClaudeTeamsEnvironment()<br/>(sets CMUX_CLAUDE_HOOK_CMUX_BIN,<br/>TMUX shim, PATH shim, NODE_OPTIONS)
rCT->>cTLA: commandArgs, hooksDisabled
alt first arg is mcp/config/api-key/rc/remote-control
cTLA-->>rCT: return commandArgs verbatim (no injection)
else user passed --teammate-mode explicitly
cTLA-->>rCT: return commandArgs verbatim ⚠️ skips --settings too
else normal path
cTLA-->>rCT: [--teammate-mode, auto, --settings, HOOKS_JSON, ...args]
end
rCT->>rCT: setenv(CMUX_CLAUDE_PID, getpid()) [unconditional]
rCT->>Claude: execv(launchPath, launchArguments)
Claude->>cmux: claude-hook session-start (via CMUX_CLAUDE_HOOK_CMUX_BIN)
cmux-->>Claude: set_agent_pid → populates agentPIDs["claude_code"]
Claude->>cmux: claude-hook stop / session-end / notification / prompt-submit / pre-tool-use
Reviews (1): Last reviewed commit: "Fix cmux claude-teams hook injection gap" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard !claudeTeamsHasExplicitTeammateMode(commandArgs: commandArgs) else { | ||
| return commandArgs | ||
| } | ||
| return ["--teammate-mode", "auto"] + commandArgs | ||
| var result: [String] = ["--teammate-mode", "auto"] | ||
| if !hooksDisabled { | ||
| result.append(contentsOf: ["--settings", Self.claudeHooksJSON]) | ||
| } | ||
| result.append(contentsOf: commandArgs) | ||
| return result |
There was a problem hiding this comment.
--teammate-mode bypass silently skips --settings hook injection
The guard !claudeTeamsHasExplicitTeammateMode early return exits before the --settings injection block, so any call of the form cmux claude-teams --teammate-mode <value> [args] loses all hook injection. This recreates the exact same sidebar-lifecycle breakage that this PR was written to fix — just for the explicit---teammate-mode path.
For example, cmux claude-teams --teammate-mode auto --continue should be equivalent to the default path, but the guard causes it to skip --settings entirely. The guard was intended to prevent double-injecting --teammate-mode auto, but --settings is an independent concern and should not be gated on it.
Consider separating the two guards so --settings is always injected (unless hooks are disabled or the user already passed --settings):
private func claudeTeamsLaunchArguments(
commandArgs: [String],
hooksDisabled: Bool
) -> [String] {
if let first = commandArgs.first,
Self.claudeTeamsPassthroughSubcommands.contains(first) {
return commandArgs
}
var result: [String] = []
if !claudeTeamsHasExplicitTeammateMode(commandArgs: commandArgs) {
result.append(contentsOf: ["--teammate-mode", "auto"])
}
let userAlreadyHasSettings = commandArgs.contains("--settings")
if !hooksDisabled && !userAlreadyHasSettings {
result.append(contentsOf: ["--settings", Self.claudeHooksJSON])
}
result.append(contentsOf: commandArgs)
return result
}| // | ||
| // Because we execv below, this process's PID becomes the claude | ||
| // process PID, so `getpid()` is the correct value to export. | ||
| setenv("CMUX_CLAUDE_PID", String(getpid()), 1) |
There was a problem hiding this comment.
CMUX_CLAUDE_PID set unconditionally for passthrough subcommands
setenv("CMUX_CLAUDE_PID", ...) runs even when claudeTeamsLaunchArguments returned the args verbatim for passthrough subcommands (mcp, config, api-key, rc, remote-control). This leaks a cmux-specific env var into claude mcp, claude config, etc. It is harmless today, but it is inconsistent with the verbatim-passthrough intent ("no injection"). Mirroring the passthrough check would keep the two paths aligned:
let launchArguments = claudeTeamsLaunchArguments(commandArgs: commandArgs, hooksDisabled: hooksDisabled)
let isPassthrough: Bool = {
if let first = commandArgs.first {
return Self.claudeTeamsPassthroughSubcommands.contains(first)
}
return false
}()
if !isPassthrough {
setenv("CMUX_CLAUDE_PID", String(getpid()), 1)
}| def main() -> int: | ||
| try: | ||
| cli_path = resolve_cmux_cli() | ||
| except Exception as exc: | ||
| print(f"FAIL: {exc}") | ||
| return 1 | ||
|
|
||
| failures: list[str] = [] | ||
| test_default_injects_settings_and_pid_env(cli_path, failures) | ||
| test_hooks_disabled_env_var_skips_injection(cli_path, failures) | ||
| test_mcp_subcommand_passthrough_skips_injection(cli_path, failures) | ||
|
|
There was a problem hiding this comment.
--teammate-mode path
The test suite covers (a) default injection, (b) CMUX_CLAUDE_HOOKS_DISABLED=1, and (c) mcp passthrough — but there is no case for cmux claude-teams --teammate-mode auto [args] or any other explicit --teammate-mode scenario. Given the logic gap noted in claudeTeamsLaunchArguments, adding a test here would immediately surface whether --settings is (or is not) injected when --teammate-mode is already present. Without it, the contract for that path is untested and a future regression would go undetected.
Consider adding to main():
def test_explicit_teammate_mode_still_injects_settings(cli_path: str, failures: list[str]) -> None:
proc, argv, _, _ = run_claude_teams(
cli_path, extra_args=["--teammate-mode", "auto", "--version"]
)
expect(
proc.returncode == 0,
f"explicit-teammate-mode: expected clean exit, got {proc.returncode}: {proc.stderr.strip()}",
failures,
)
expect(
"--settings" in argv,
f"explicit-teammate-mode: expected --settings injection even when --teammate-mode is explicit, got {argv}",
failures,
)![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_cli_claude_teams_hook_injection.py (1)
275-298: Expand passthrough coverage beyondmcpThis only validates one passthrough subcommand. Since the contract includes
config,api-key,rc, andremote-controltoo, consider parameterizing this test to assert all no-injection passthrough cases.Refactor sketch
-def test_mcp_subcommand_passthrough_skips_injection(cli_path: str, failures: list[str]) -> None: - # `mcp` is a claude subcommand that doesn't accept --settings; mirroring - # bin/claude:166-168, it must be passed through verbatim with no injection. - proc, argv, _, _ = run_claude_teams(cli_path, extra_args=["mcp", "--help"]) +def test_passthrough_subcommands_skip_injection(cli_path: str, failures: list[str]) -> None: + for subcommand in ["mcp", "config", "api-key", "rc", "remote-control"]: + proc, argv, _, _ = run_claude_teams(cli_path, extra_args=[subcommand, "--help"]) + expect( + proc.returncode == 0, + f"{subcommand} passthrough: expected clean exit, got {proc.returncode}: {proc.stderr.strip()}", + failures, + ) + expect( + "--settings" not in argv, + f"{subcommand} passthrough: expected NO --settings injection, got {argv}", + failures, + ) + expect( + "--teammate-mode" not in argv, + f"{subcommand} passthrough: expected NO --teammate-mode injection, got {argv}", + failures, + ) + expect( + argv == [subcommand, "--help"], + f"{subcommand} passthrough: expected verbatim argv, got {argv}", + failures, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_hook_injection.py` around lines 275 - 298, The test test_mcp_subcommand_passthrough_skips_injection only asserts passthrough for "mcp"; update it to iterate over the full passthrough set ["mcp","config","api-key","rc","remote-control"] (or a named constant) and run run_claude_teams for each subcommand, then assert proc.returncode==0 and that "--settings" and "--teammate-mode" are not in argv and argv equals [subcommand, "--help"] for each case; keep the same failure messages but include the current subcommand in them so failures are identifiable, and reuse the existing argv/proc/expect logic inside the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 10268-10288: The function claudeTeamsLaunchArguments incorrectly
returns early when claudeTeamsHasExplicitTeammateMode(commandArgs:) is true,
which prevents adding the hooks settings; instead, update
claudeTeamsLaunchArguments to not return immediately on explicit --teammate-mode
but still append "--settings", Self.claudeHooksJSON when hooksDisabled is false
(and preserve passthrough behavior for Self.claudeTeamsPassthroughSubcommands).
Concretely: remove the early return path that returns commandArgs when
claudeTeamsHasExplicitTeammateMode(...) is true and ensure the code builds
result by respecting explicit teammate-mode flags in commandArgs while still
conditionally injecting ["--settings", Self.claudeHooksJSON] unless
hooksDisabled; apply the same change to the other analogous block (the duplicate
at the other location that uses the same helper).
In `@tests/test_cli_claude_teams_hook_injection.py`:
- Around line 55-65: parse_settings_arg currently returns any JSON value which
can be a non-dict and later cause AttributeError in
assert_hook_settings_structure; change parse_settings_arg to validate the parsed
value is a dict (mapping) and return {} for any non-dict or on JSON errors so
the failure aggregator can record a failure instead of crashing; update the
function (parse_settings_arg) to: after json.loads(argv[index + 1]) check
isinstance(result, dict) (or mapping) and return result only if true, otherwise
return {}.
---
Nitpick comments:
In `@tests/test_cli_claude_teams_hook_injection.py`:
- Around line 275-298: The test test_mcp_subcommand_passthrough_skips_injection
only asserts passthrough for "mcp"; update it to iterate over the full
passthrough set ["mcp","config","api-key","rc","remote-control"] (or a named
constant) and run run_claude_teams for each subcommand, then assert
proc.returncode==0 and that "--settings" and "--teammate-mode" are not in argv
and argv equals [subcommand, "--help"] for each case; keep the same failure
messages but include the current subcommand in them so failures are
identifiable, and reuse the existing argv/proc/expect logic inside the loop.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7adb57d8-49d2-4eba-93b5-ee31a658cc47
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_cli_claude_teams_hook_injection.py
| def parse_settings_arg(argv: list[str]) -> dict: | ||
| if "--settings" not in argv: | ||
| return {} | ||
| index = argv.index("--settings") | ||
| if index + 1 >= len(argv): | ||
| return {} | ||
| try: | ||
| return json.loads(argv[index + 1]) | ||
| except json.JSONDecodeError: | ||
| return {} | ||
|
|
There was a problem hiding this comment.
Guard non-object --settings JSON to avoid crashing the failure aggregator
If json.loads(...) returns a non-dict (valid JSON array/string), assert_hook_settings_structure on Line 243 will raise AttributeError instead of appending a failure.
Proposed fix
-def parse_settings_arg(argv: list[str]) -> dict:
+def parse_settings_arg(argv: list[str]) -> dict:
if "--settings" not in argv:
return {}
index = argv.index("--settings")
if index + 1 >= len(argv):
return {}
try:
- return json.loads(argv[index + 1])
+ parsed = json.loads(argv[index + 1])
+ return parsed if isinstance(parsed, dict) else {}
except json.JSONDecodeError:
return {}Also applies to: 236-243
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_cli_claude_teams_hook_injection.py` around lines 55 - 65,
parse_settings_arg currently returns any JSON value which can be a non-dict and
later cause AttributeError in assert_hook_settings_structure; change
parse_settings_arg to validate the parsed value is a dict (mapping) and return
{} for any non-dict or on JSON errors so the failure aggregator can record a
failure instead of crashing; update the function (parse_settings_arg) to: after
json.loads(argv[index + 1]) check isinstance(result, dict) (or mapping) and
return result only if true, otherwise return {}.
|
Tip For best results, initiate chat on the files or code changes.
The fix looks good to me. I'll record the decoupled-branch invariant for future reviews. ✏️ Learnings added
🧠 Learnings used |
|
Perfect fix — the split into two independent branches (auto-prepend vs. settings injection) is exactly right, and the test coverage is comprehensive. On the P2 (CMUX_CLAUDE_PID on passthrough): Your reasoning is solid. Mirroring On the nit (json.loads guard): Agreed — a malformed The two-commit structure is clean, the behavior matrix is clear, and the expanded passthrough tests are a good catch for future-proofing. This looks ready to merge. Tip: You can customize Greptile's behavior for this repo with |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_cli_claude_teams_hook_injection.py (1)
55-65:⚠️ Potential issue | 🟡 MinorGuard
parse_settings_argagainst non-object JSON.A valid non-object JSON value (array/string) from Line 62 can make
assert_hook_settings_structurecrash at Line 135 (settings.get(...)) instead of recording a failure.Proposed fix
def parse_settings_arg(argv: list[str]) -> dict: if "--settings" not in argv: return {} index = argv.index("--settings") if index + 1 >= len(argv): return {} try: - return json.loads(argv[index + 1]) + parsed = json.loads(argv[index + 1]) + return parsed if isinstance(parsed, dict) else {} except json.JSONDecodeError: return {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_hook_injection.py` around lines 55 - 65, The parse_settings_arg function can return non-object JSON (e.g., list or string) which later causes assert_hook_settings_structure to crash when calling settings.get; change parse_settings_arg to validate the parsed JSON is a mapping/dict before returning it (i.e., after json.loads(argv[index + 1]) ensure isinstance(result, dict) and otherwise return {}), so only object-shaped settings propagate to the rest of the test.
🧹 Nitpick comments (1)
tests/test_cli_claude_teams_hook_injection.py (1)
275-303: Also assert passthrough keepsCMUX_CLAUDE_PID/CMUX_CLAUDE_HOOK_CMUX_BINpopulated.This test already validates argv passthrough well; adding env assertions here would lock in the intentional unconditional export behavior too.
Suggested test extension
- proc, argv, _, _ = run_claude_teams(cli_path, extra_args=[subcommand, "--help"]) + proc, argv, pid_value, hook_cmux_bin_value = run_claude_teams( + cli_path, extra_args=[subcommand, "--help"] + ) prefix = f"{subcommand} passthrough" @@ expect( argv == [subcommand, "--help"], f"{prefix}: expected verbatim argv, got {argv}", failures, ) + expect( + pid_value not in {"", "__UNSET__"}, + f"{prefix}: expected CMUX_CLAUDE_PID to be set, got {pid_value!r}", + failures, + ) + expect( + hook_cmux_bin_value not in {"", "__UNSET__"}, + f"{prefix}: expected CMUX_CLAUDE_HOOK_CMUX_BIN to be set, got {hook_cmux_bin_value!r}", + failures, + )Based on learnings: in
CLI/cmux.swift runClaudeTeams,setenv("CMUX_CLAUDE_PID", ...)is intentionally unconditional before passthrough-subcommand handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_claude_teams_hook_injection.py` around lines 275 - 303, Extend test_passthrough_subcommands_skip_injection to also assert that the environment exported for passthrough contains non-empty CMUX_CLAUDE_PID and CMUX_CLAUDE_HOOK_CMUX_BIN: after calling run_claude_teams (the existing call that returns proc, argv, env, ...), add expects that env contains "CMUX_CLAUDE_PID" and "CMUX_CLAUDE_HOOK_CMUX_BIN" and that both values are truthy/non-empty strings; reference the test function name test_passthrough_subcommands_skip_injection and the run_claude_teams helper, and ensure these env asserts are done for each subcommand in the passthrough_subcommands loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/test_cli_claude_teams_hook_injection.py`:
- Around line 55-65: The parse_settings_arg function can return non-object JSON
(e.g., list or string) which later causes assert_hook_settings_structure to
crash when calling settings.get; change parse_settings_arg to validate the
parsed JSON is a mapping/dict before returning it (i.e., after
json.loads(argv[index + 1]) ensure isinstance(result, dict) and otherwise return
{}), so only object-shaped settings propagate to the rest of the test.
---
Nitpick comments:
In `@tests/test_cli_claude_teams_hook_injection.py`:
- Around line 275-303: Extend test_passthrough_subcommands_skip_injection to
also assert that the environment exported for passthrough contains non-empty
CMUX_CLAUDE_PID and CMUX_CLAUDE_HOOK_CMUX_BIN: after calling run_claude_teams
(the existing call that returns proc, argv, env, ...), add expects that env
contains "CMUX_CLAUDE_PID" and "CMUX_CLAUDE_HOOK_CMUX_BIN" and that both values
are truthy/non-empty strings; reference the test function name
test_passthrough_subcommands_skip_injection and the run_claude_teams helper, and
ensure these env asserts are done for each subcommand in the
passthrough_subcommands loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6749c0d-cce6-496f-8029-d4012c107784
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_cli_claude_teams_hook_injection.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CLI/cmux.swift
Fixes #2229. Follow-up to #2465. The plain `claude` path picks up `--settings` hook JSON via the shell wrapper (Resources/bin/claude:208), but `cmux claude-teams` bypasses the shell entirely — it execv's straight into the real claude binary from runClaudeTeams — so it never picked up the same hook injection. Result: in claude-teams mode the sidebar Running / Needs Input / Notification lifecycle never fires, breaking the multi-agent workflow that claude-teams exists for. Confirmed by two reporters in #2229. Mirror the four things Resources/bin/claude does into runClaudeTeams: - Inject `--settings <HOOKS_JSON>` as a verbatim duplicate of the bash HOOKS_JSON literal (same intentional bash<->Swift duplication pattern used for claudeNodeOptionsRestoreModule). - Export CMUX_CLAUDE_HOOK_CMUX_BIN = executablePath in configureClaudeTeamsEnvironment so hooks dispatch through this bundled cmux binary instead of whatever's on PATH (mise shims, stale installs). Mirrors Resources/bin/claude:189. - Export CMUX_CLAUDE_PID = getpid() right before execv in runClaudeTeams. Because we execv into claude, the current process PID becomes claude's PID, matching `export CMUX_CLAUDE_PID=$$` in Resources/bin/claude:188. Without this, `cmux claude-hook session-start` reads nil and skips set_agent_pid claude_code, leaving workspace.agentPIDs["claude_code"] unpopulated — which GhosttyTerminalView.swift's Running indicator gates on. - Pass `mcp | config | api-key | rc | remote-control` through verbatim (mirrors Resources/bin/claude:166-168) since these subcommands don't accept --settings or --teammate-mode. `--teammate-mode` and `--settings` injection are kept independent so an explicit `--teammate-mode auto|manual` does not silently disable hook injection. `--teammate-mode auto` is auto-prepended only when the user didn't pass an explicit mode. Honors CMUX_CLAUDE_HOOKS_DISABLED=1 as an opt-out (e.g. Settings > Automation > Claude Code Integration toggled off).
966790d to
d6a8819
Compare
1 participant
Summary
cmux claude-teamsnever injected the Claude Code lifecycle hooks that #2465 set up for the plainclaudepath, so claude-teams sessions get no Running / Needs Input / Notification updates in the sidebar — breaking the multi-agent workflow that claude-teams exists for. Two users in #2229 confirmed the symptom (including plain text Q&A, ruling out tool-use dedup theories).This PR mirrors
Resources/bin/claudeintorunClaudeTeams:--settings <HOOKS_JSON>(verbatim duplicate of the bash literal, same intentional bash↔Swift duplication pattern asclaudeNodeOptionsRestoreModule)CMUX_CLAUDE_HOOK_CMUX_BIN = executablePathso hooks dispatch through this cmux binary instead of PATHCMUX_CLAUDE_PID = getpid()beforeexecvsoclaude-hook session-startcan populateagentPIDs["claude_code"]mcp | config | api-key | rc | remote-controlthrough verbatimCMUX_CLAUDE_HOOKS_DISABLED=1as an opt-out--teammate-modeand--settingsinjection independent so an explicit--teammate-mode auto|manualdoesn't bypass hook setupCloses #2229. Follow-up to #2465 (which fixed the
claudepath but notclaude-teams).Coordination with #2515
@lawrencecchen's open #2515 also touches
Resources/bin/claude(adding a newStopFailurehook event). Whichever PR lands first, the other will need to update the matching literal — for #2515 that means addingStopFailureto the newclaudeHooksJSONliteral incmux.swift, and for this PR it means rebasing onto the newHOOKS_JSON. The diff is mechanical either way and I'm happy to do the rebase whichever direction works best.Summary by CodeRabbit
New Features
Bug Fixes