fix: exit view attach on first Ctrl-C#1353
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe view-session WebSocket now supports immediate termination after graceful close attempts. Related tests and changelog text reflect first-Ctrl-C exit behavior, provider shutdown timing is refined, and standalone smoke tests wait for broker readiness. ChangesView socket teardown
Provider shutdown timing
Standalone smoke readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd8244ac1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // while waiting for the broker's close acknowledgement. A local | ||
| // detach has no state to flush, so terminate it immediately rather | ||
| // than requiring another Ctrl-C to make Node leave the event loop. | ||
| socket.terminate?.(); |
There was a problem hiding this comment.
This changes agent-relay agent attach --mode view behavior for users by making Ctrl-C detach on the first press, but the commit leaves CHANGELOG.md's [Unreleased] section empty. Release notes will omit this shipped CLI fix unless this adds a concise Fixed entry and, while [Unreleased] is empty, bumps the heading to [Unreleased - Patch].
AGENTS.md reference: AGENTS.md:L31-L38
Useful? React with 👍 / 👎.
![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/cli/src/cli/lib/attach-view.test.ts`:
- Line 536: Update the raw-input detach test assertion near
sockets[0].terminated to also assert that sockets[0].closed is true, preserving
coverage for both the graceful close request and termination 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c818296c-f952-4542-851f-b551216c2bf0
📒 Files selected for processing (3)
CHANGELOG.mdpackages/cli/src/cli/lib/attach-view.test.tspackages/cli/src/cli/lib/attach-view.ts
d3aac21 to
f9e3f19
Compare
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 `@scripts/ci-standalone-smoke.sh`:
- Around line 131-132: Validate AGENT_RELAY_STANDALONE_STARTUP_TIMEOUT_SECONDS
as a non-negative decimal before assigning STARTUP_TIMEOUT_SECONDS or evaluating
STARTUP_DEADLINE arithmetic. Reject values such as 30s, 08, abc, and empty
input, then apply the existing safe/default timeout behavior; only perform the
deadline calculation after validation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dbd2cd80-ec08-4d00-9e0f-204f92067ac4
📒 Files selected for processing (5)
CHANGELOG.mdpackages/cli/src/cli/lib/attach-view.test.tspackages/cli/src/cli/lib/attach-view.tspackages/cli/src/cli/lib/node-provider-child.test.tsscripts/ci-standalone-smoke.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- packages/cli/src/cli/lib/attach-view.test.ts
| STARTUP_TIMEOUT_SECONDS="${AGENT_RELAY_STANDALONE_STARTUP_TIMEOUT_SECONDS:-30}" | ||
| STARTUP_DEADLINE=$((SECONDS + STARTUP_TIMEOUT_SECONDS)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '120,145p' scripts/ci-standalone-smoke.sh 2>/dev/null || true
echo
echo "== bash arithmetic behavior for timeout values =="
python3 - <<'PY'
import subprocess, shlex
cases = ["30s", "08", "0", "-5", "abc", "", "30"]
for val in cases:
script = f'startup_timeout_seconds="{val}"; if [[ "$startup_timeout_seconds" =~ ^(0|[1-9][0-9]*)$ ]]; then echo "regex ok"; else echo "regex fail"; fi; STARTUP_DEADLINE=$((SECONDS + startup_timeout_seconds)); echo deadline $STARTUP_DEADLINE'
cp= subprocess.run(["bash","-c",script], text=True, capture_output=True)
print(f"{val!r}: rc={cp.returncode}; out={cp.stdout.rstrip()}");
if cp.stderr:
print(f" err={cp.stderr.rstrip()}")
PYRepository: AgentWorkforce/relay
Length of output: 1423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '120,145p' scripts/ci-standalone-smoke.sh 2>/dev/null || true
echo
echo "== bash arithmetic behavior for timeout values =="
python3 - <<'PY'
import subprocess
cases = ["30s", "08", "0", "-5", "abc", "", "30"]
for val in cases:
script = f'''
startup_timeout_seconds="{val}"
if [[ "$startup_timeout_seconds" =~ ^(0|[1-9][0-9]*)$ ]]; then
echo "regex ok"
else
echo "regex fail"
fi
STARTUP_DEADLINE=$((SECONDS + startup_timeout_seconds))
echo deadline $STARTUP_DEADLINE
'''
cp = subprocess.run(["bash", "-c", script], text=True, capture_output=True)
print(f"{val!r}: rc={cp.returncode}; out={cp.stdout.rstrip()}")
if cp.stderr:
print(f" err={cp.stderr.rstrip()}")
PYRepository: AgentWorkforce/relay
Length of output: 1441
Validate the configurable timeout before arithmetic evaluation.
AGENT_RELAY_STANDALONE_STARTUP_TIMEOUT_SECONDS is used in Bash arithmetic, so values like 30s or 08 fail with value too great for base, and non-decimal values such as abc or an empty value default to 0, causing an immediate readiness deadline. Add a non-negative decimal guard before computing STARTUP_DEADLINE.
🤖 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 `@scripts/ci-standalone-smoke.sh` around lines 131 - 132, Validate
AGENT_RELAY_STANDALONE_STARTUP_TIMEOUT_SECONDS as a non-negative decimal before
assigning STARTUP_TIMEOUT_SECONDS or evaluating STARTUP_DEADLINE arithmetic.
Reject values such as 30s, 08, abc, and empty input, then apply the existing
safe/default timeout behavior; only perform the deadline calculation after
validation.
1 participant
Summary
node agent attach --mode viewWhy
View mode consumes Ctrl-C as raw terminal input to suppress terminal input-report noise. Its graceful WebSocket shutdown could leave the Node process alive while waiting for the broker to acknowledge the close, requiring a second Ctrl-C.
User impact
Detaching a read-only view now exits on the first Ctrl-C. The viewed agent is not stopped.
Validation
npm exec vitest -- run packages/cli/src/cli/lib/attach-view.test.tsnpx tsc --noEmit -p packages/cli/tsconfig.json