Repository navigation
Conversation
|
To use Codex here, create a Codex account and connect to github. |
|
@lebara-moaead is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds exec_real_claude that conditionally allocates a PTY via BSD ChangesPTY Wrapper and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 Claude Code TUI falling back to non-interactive mode inside cmux by introducing an
Confidence Score: 4/5Safe to merge for the interactive TUI fix, but hooks that use CMUX_CLAUDE_PID for signal delivery or liveness checks will silently target the interposed script process rather than the real claude process. The exec_real_claude helper correctly falls back to direct exec when the BSD-script probe fails, so the change cannot regress non-PTY environments. However, CMUX_CLAUDE_PID=$$ is set with the explicit comment that 'this PID becomes the actual claude process PID,' which held under the old direct exec but no longer holds: exec'ing script makes $$ track the script wrapper, while claude runs as a forked child at a different PID. Any hook that sends signals, polls /proc, or checks process names via CMUX_CLAUDE_PID will silently target the wrong process after this change. Resources/bin/claude — the CMUX_CLAUDE_PID export and its comment need revisiting now that script is interposed between the wrapper and real claude. Important Files Changed
Sequence DiagramsequenceDiagram
participant cmux as cmux (no PTY)
participant wrapper as Resources/bin/claude (wrapper, PID=$$)
participant script as /usr/bin/script (PID=$$, after exec)
participant claude as real claude (forked child, new PID)
cmux->>wrapper: "spawn with stdin=pipe (non-TTY)"
wrapper->>wrapper: "IN_CMUX=1, !-t 0, script probe OK"
wrapper->>script: exec /usr/bin/script -q /dev/null REAL_CLAUDE [args]
note over wrapper,script: wrapper process replaced — PID=$$ now tracks script, not claude
script->>claude: fork+exec REAL_CLAUDE [args] in PTY slave
claude-->>script: stdout/stderr via PTY master (ONLCR active)
script-->>cmux: passthrough PTY output
claude-->>script: exit(N)
script-->>cmux: exit(N)
Reviews (3): Last reviewed commit: "Address PR feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if not os.access("/usr/bin/script", os.X_OK): | ||
| return # BSD `script` is required for the workaround. |
There was a problem hiding this comment.
CMUX_CLAUDE_FORCE_PTY=0) not tested
The comment in the wrapper documents CMUX_CLAUDE_FORCE_PTY=0 as the escape hatch for the PTY allocation. run_wrapper_pty_probe pops this variable from the environment to ensure the default path is exercised, but there is no corresponding test that sets CMUX_CLAUDE_FORCE_PTY=0 and asserts the real claude still sees a non-TTY stdin. If the condition logic regresses (e.g., the != "0" check is changed), the only way a breakage would surface is in production.
| exec_real_claude() { | ||
| if [[ "${IN_CMUX:-0}" == "1" ]] \ | ||
| && [[ "${CMUX_CLAUDE_FORCE_PTY:-1}" != "0" ]] \ | ||
| && [[ ! -t 0 ]] \ | ||
| && [[ -x /usr/bin/script ]]; then | ||
| # BSD `script` calls tcgetattr on stdin and aborts when it cannot read | ||
| # attrs (some socket types). Probe with /usr/bin/true so a hostile | ||
| # stdin can never regress claude into "failed to start" — fall back to | ||
| # direct exec instead, preserving today's behavior. | ||
| if /usr/bin/script -q /dev/null /usr/bin/true >/dev/null 2>&1; then | ||
| exec /usr/bin/script -q /dev/null "$@" | ||
| fi | ||
| fi | ||
| exec "$@" | ||
| } |
There was a problem hiding this comment.
stdin data in automated invocations
A PTY's line discipline applies ONLCR (newline → CR+LF) and echo by default. In the common interactive case this is harmless, but the guard condition [[ ! -t 0 ]] fires precisely when stdin is a pipe or socket — i.e., the automated/scripted path. Any byte stream pumped into claude's stdin by cmux through that pipe (e.g., tool-result data, JSON payloads) passes through script's PTY master, where the line discipline can corrupt \n → \r\n before claude reads it. Consider calling stty -opost (or stty raw) on the PTY slave to suppress output processing if cmux ever uses stdio for structured data.
![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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 1085-1086: Replace the silent early return guarded by
os.access("/usr/bin/script", os.X_OK) with an explicit test skip so CI shows
skipped vs passed; specifically, where the code checks if not
os.access("/usr/bin/script", os.X_OK) change the behavior to call
pytest.skip("skipping test: /usr/bin/script missing (BSD script required)") (or
raise unittest.SkipTest) so the test runner records a visible skip rather than
silently returning.
🪄 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: 37f885ff-0a3f-49ba-beae-19031d11ffbb
📒 Files selected for processing (2)
Resources/bin/claudetests/test_claude_wrapper_hooks.py
|
Addressed feedback in f35e147: 1 review(s) requesting changes from coderabbitai[bot]. FOLLOW-UP SUMMARY:
|
|
Addressed feedback in 61e8d0d: 1 review(s) requesting changes from coderabbitai[bot]. FOLLOW-UP SUMMARY:
|
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 `@tests/test_claude_wrapper_hooks.py`:
- Around line 1093-1100: The skip gate should check that the exact BSD-style
invocation used by the wrapper succeeds, not just that /usr/bin/script is
executable; in the test_inside_cmux_without_tty_allocates_pty_for_real_claude
replace the os.access check with running the same probe used by
exec_real_claude() ("/usr/bin/script -q /dev/null /usr/bin/true") via
subprocess.run and only skip if that probe fails (non-zero return) so the test
only runs when the BSD-script behavior is available.
🪄 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: c333c6d5-e637-4df4-9b31-14970904c833
📒 Files selected for processing (1)
tests/test_claude_wrapper_hooks.py
| if not os.access("/usr/bin/script", os.X_OK): | ||
| # BSD `script` is required for the workaround; surface an explicit | ||
| # SKIP so CI logs distinguish skipped-on-Linux from passed-on-macOS. | ||
| print( | ||
| "SKIP: test_inside_cmux_without_tty_allocates_pty_for_real_claude " | ||
| "(requires BSD /usr/bin/script)" | ||
| ) | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if [ ! -x /usr/bin/script ]; then
echo "SKIP_CONDITION_VALID: /usr/bin/script missing"
exit 0
fi
if /usr/bin/script -q /dev/null /usr/bin/true >/dev/null 2>&1; then
echo "BSD_STYLE_SCRIPT_SUPPORTED=1"
else
echo "BSD_STYLE_SCRIPT_SUPPORTED=0"
fiRepository: manaflow-ai/cmux
Length of output: 106
🏁 Script executed:
cat -n tests/test_claude_wrapper_hooks.py | sed -n '1090,1105p'Repository: manaflow-ai/cmux
Length of output: 815
🏁 Script executed:
cat -n tests/test_claude_wrapper_hooks.py | sed -n '1084,1115p'Repository: manaflow-ai/cmux
Length of output: 1612
🏁 Script executed:
cat -n tests/test_claude_wrapper_hooks.py | sed -n '988,1082p'Repository: manaflow-ai/cmux
Length of output: 3731
🏁 Script executed:
rg -n "SOURCE_WRAPPER" tests/test_claude_wrapper_hooks.py | head -20Repository: manaflow-ai/cmux
Length of output: 268
🏁 Script executed:
head -30 tests/test_claude_wrapper_hooks.py | grep -A2 "^ROOT"Repository: manaflow-ai/cmux
Length of output: 159
🏁 Script executed:
cat Resources/bin/claudeRepository: manaflow-ai/cmux
Length of output: 17124
Gate this skip on script capability, not just executable presence.
The wrapper itself already probes /usr/bin/script -q /dev/null /usr/bin/true before using it (see exec_real_claude() in Resources/bin/claude). The test's skip gate should match: os.access("/usr/bin/script", os.X_OK) alone can pass on systems with non-BSD implementations, causing false test failures instead of skips. Probe the exact invocation before running assertions.
Suggested patch
- if not os.access("/usr/bin/script", os.X_OK):
+ script_probe_ok = os.access("/usr/bin/script", os.X_OK)
+ if script_probe_ok:
+ probe = subprocess.run(
+ ["/usr/bin/script", "-q", "/dev/null", "/usr/bin/true"],
+ stdin=subprocess.DEVNULL,
+ stdout=subprocess.DEVNULL,
+ stderr=subprocess.DEVNULL,
+ check=False,
+ )
+ script_probe_ok = probe.returncode == 0
+
+ if not script_probe_ok:
# BSD `script` is required for the workaround; surface an explicit
# SKIP so CI logs distinguish skipped-on-Linux from passed-on-macOS.
print(
"SKIP: test_inside_cmux_without_tty_allocates_pty_for_real_claude "
"(requires BSD /usr/bin/script)"
)
return🤖 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 `@tests/test_claude_wrapper_hooks.py` around lines 1093 - 1100, The skip gate
should check that the exact BSD-style invocation used by the wrapper succeeds,
not just that /usr/bin/script is executable; in the
test_inside_cmux_without_tty_allocates_pty_for_real_claude replace the os.access
check with running the same probe used by exec_real_claude() ("/usr/bin/script
-q /dev/null /usr/bin/true") via subprocess.run and only skip if that probe
fails (non-zero return) so the test only runs when the BSD-script behavior is
available.
1 participant
# Summary
exec "$REAL_CLAUDE"calls inResources/bin/claudewith/usr/bin/script -q /dev/nullvia a newexec_real_claudehelper, so the Ink-based TUI sees a PTY on stdin.tests/test_claude_wrapper_hooks.pyto cover the new exec path./usr/bin/scriptfirst so a broken stdin can't regress claude into a "failed to start" state. ## Review Trigger (Copy/Paste as PR comment) ```text@codex review
@coderabbitai review
@greptile-apps review
@cubic-dev-ai review