feat: add exit code tracking#2
Conversation
Useful for integration testing
feat: add mouse support
WalkthroughThe changes introduce comprehensive exit event tracking functionality to the ht tool. A new Exit event variant broadcasts exit codes and signals through the session lifecycle. The PTY module now returns ExitStatus data, event handlers process Exit events, and the CLI/API handle the new exit subscription option. Documentation, examples, and test suites validate the feature end-to-end. Changes
Sequence Diagram(s)sequenceDiagram
participant Main as main()
participant PTY as start_pty()
participant EventLoop as run_event_loop()
participant Session as Session
participant Subscribers as Subscribers
Main->>PTY: spawn with command
activate PTY
PTY->>PTY: monitor child process
PTY-->>Main: (pid, pty_handle)
deactivate PTY
Main->>EventLoop: pass pty_handle & channels
activate EventLoop
par Event Processing
loop While session active
EventLoop->>EventLoop: poll channels (stdin, output, api, pty_handle)
end
and PTY Completion
PTY->>PTY: child exits/signals
PTY-->>EventLoop: ExitStatus(code, signal)
end
EventLoop->>Session: exit(code, signal)
activate Session
Session->>Session: compute elapsed time
Session->>Subscribers: broadcast Exit(time, code, signal)
Session-->>EventLoop: event sent
deactivate Session
EventLoop->>EventLoop: handle channel closures gracefully
EventLoop->>EventLoop: break on session complete
deactivate EventLoop
Main->>Main: aggregate results
sequenceDiagram
participant Client as Client (stdin)
participant StdioAPI as Stdio API
participant Parser as Command Parser
participant Validator as Validator
participant Executor as Executor
Client->>StdioAPI: send JSON command
activate StdioAPI
alt stdin_open is true
StdioAPI->>Parser: parse JSON line
activate Parser
alt Valid JSON
Parser->>Validator: extract cmd_type field
activate Validator
alt Known cmd_type (input, sendKeys, mouse, resize, takeSnapshot)
Validator->>Executor: dispatch with validated args
activate Executor
Executor-->>StdioAPI: command executed
deactivate Executor
else Unknown cmd_type
Validator-->>StdioAPI: error with contextual message & link
end
deactivate Validator
else Invalid JSON
Parser-->>StdioAPI: error with helpful tip about single-line JSON
end
deactivate Parser
else stdin_open is false
StdioAPI->>StdioAPI: mark stdin_open=false, continue
StdioAPI-->>StdioAPI: wait for process exit
end
deactivate StdioAPI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/http.rs (1)
162-169: Don't drop exit events on /ws/eventsExit subscribers on the websocket never receive
Event::Exitbecause this match falls through toOk(_) => None.Subscription::parsenow exposes anexitflag and the new session/event plumbing expects these notifications, so HTTP clients will miss the exit payload entirely. Please forward exit events here just like the other event types.Ok(e @ Snapshot(_, _, _, _)) if sub.snapshot => Some(Ok(json_message(e.to_json()))), + Ok(e @ Exit(_, _, _)) if sub.exit => Some(Ok(json_message(e.to_json()))), Ok(_) => None,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
README.md(3 hunks)examples/README.md(1 hunks)examples/basic_usage.sh(1 hunks)examples/exit_tracking.sh(1 hunks)examples/signal_handling.sh(1 hunks)scripts/test_exit_tracking.sh(1 hunks)src/api.rs(2 hunks)src/api/http.rs(1 hunks)src/api/stdio.rs(5 hunks)src/cli.rs(3 hunks)src/main.rs(5 hunks)src/pty.rs(5 hunks)src/session.rs(3 hunks)tests/exit_codes.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
examples/basic_usage.sh (1)
src/session.rs (1)
exit(76-79)
examples/exit_tracking.sh (1)
src/session.rs (1)
exit(76-79)
examples/signal_handling.sh (2)
src/session.rs (1)
exit(76-79)scripts/test_exit_tracking.sh (1)
test_signal(80-155)
src/main.rs (1)
src/session.rs (2)
exit(76-79)output(49-55)
scripts/test_exit_tracking.sh (2)
src/session.rs (1)
exit(76-79)examples/signal_handling.sh (1)
test_signal(96-138)
src/pty.rs (1)
src/nbio.rs (2)
read(15-29)write(31-45)
tests/exit_codes.rs (2)
src/api/stdio.rs (1)
start(41-99)src/api.rs (1)
from_str(17-32)
src/api/stdio.rs (1)
src/session.rs (2)
exit(76-79)to_json(121-167)
🪛 LanguageTool
examples/README.md
[style] ~61-~61: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ...anced signal handling ## Contributing Feel free to add more examples! Useful additions mig...
(FEEL_FREE_TO_STYLE_ME)
🪛 Shellcheck (0.11.0)
scripts/test_exit_tracking.sh
[warning] 12-12: YELLOW appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 107-107: i appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (12)
README.md (2)
297-373: Excellent documentation enhancement!The added JSON examples for each event type significantly improve the documentation's usability. The examples are clear, properly formatted, and include realistic values that help users understand the expected data structures.
375-436: Outstanding documentation of exit event semantics!The exit event documentation is thorough and addresses a crucial subtlety: the distinction between the direct PTY child process receiving a signal versus a subprocess being signaled. This is a common source of confusion and the detailed explanation with examples will help users understand the behavior.
The inclusion of common signal-based exit codes (130, 137, 143) and multiple examples covering different scenarios makes this documentation immediately actionable.
examples/exit_tracking.sh (2)
16-50: Clear and effective demonstration!Examples 1-3 provide a good progression from simple success (exit 0) to failure (exit 1) to custom exit codes. The use of grep for parsing is straightforward and doesn't require additional dependencies.
96-123: Good coverage of parsing options!Example 5 demonstrates JSON parsing with
jq(with a proper check for its availability), and Example 6 provides a useful template for conditional logic. These examples complement the simpler grep-based parsing shown earlier.scripts/test_exit_tracking.sh (2)
36-155: Well-structured test helpers with proper error handling!Both
test_exit_codeandtest_signalare well-designed:
- Proper use of
timeoutto prevent hangs- Robust polling loop in
test_signal(lines 107-115) that handles timing issues- Cleanup happens in all code paths
- Clear pass/fail reporting
The shellcheck warnings about unused variables are false positives:
YELLOWis actually used in the conditional on line 189iin the loop on line 107 is just a range counter and doesn't need to be referenced
157-182: Comprehensive test coverage!The test suite covers the essential scenarios:
- Normal exit codes across the valid range (0, 1, 42, 127, 255)
- Built-in commands with known behavior (true/false)
- Command-not-found scenario (127)
- Signal terminations (SIGTERM and SIGKILL)
This provides good confidence in the exit tracking feature.
src/cli.rs (1)
9-106: Excellent UX improvements!The CLI enhancements significantly improve the user experience:
Helpful examples (lines 11-33): The
after_helpsection provides practical, copy-pasteable examples covering common use cases including the new--subscribe exitfunctionality.Clearer documentation (lines 36-48): Field descriptions are more explicit, particularly the note about using
--to separate ht options from command arguments.Better error messages (lines 78-106): The enhanced error handling in
Size::from_strnow:
- Shows the invalid value that was provided
- Includes actionable tips about the expected format
- Helps users quickly correct their input
examples/basic_usage.sh (3)
16-32: Clear and straightforward examples!Examples 1 and 2 effectively demonstrate basic ht usage with simple commands and exit code capture. The progression from success to failure cases is pedagogically sound.
34-43: Good use of timeout for interactive example!Example 3 properly uses
timeoutto prevent the interactive shell from hanging. This is a good practice for demonstration scripts.
69-76: Terminal resize example works well!Example 5 cleanly demonstrates the resize functionality with a custom terminal size.
examples/README.md (1)
1-66: Well-organized examples documentation!This README provides excellent guidance for users exploring the examples:
- Clear descriptions of what each script demonstrates
- Practical run commands
- Useful learning path that builds from basics to advanced topics
- Helpful note about the
HTenvironment variableThe suggested contributions section invites community participation with concrete ideas.
src/api.rs (1)
7-26: Clean implementation of exit event subscription!The changes cleanly extend the subscription model to support exit events:
- Making fields public (lines 7-11) is consistent with direct field access patterns
- The new
exitfield follows the same boolean pattern as other events- The
FromStrimplementation correctly handles the "exit" event nameThe implementation is consistent with the existing event subscription architecture.
| echo "Example 4: Using sendKeys for keyboard input" | ||
| echo "---------------------------------------------" | ||
| echo "This sends Ctrl-C to interrupt a sleep command" | ||
| echo "" | ||
| # Note: This example is more complex and demonstrates programmatic interaction | ||
| cat > /tmp/ht_example_keys.sh <<'EOF' | ||
| #!/usr/bin/env bash | ||
| ht --subscribe init,exit bash -c "sleep 10" & | ||
| HT_PID=$! | ||
|
|
||
| # Wait for init event | ||
| sleep 0.2 | ||
|
|
||
| # Send Ctrl-C | ||
| echo '{"type": "sendKeys", "keys": ["^c"]}' >&${HT_PID} | ||
|
|
||
| # Wait for ht to exit | ||
| wait $HT_PID 2>/dev/null || true | ||
| EOF | ||
| chmod +x /tmp/ht_example_keys.sh | ||
|
|
||
| echo "See /tmp/ht_example_keys.sh for the full example" | ||
| echo "" |
There was a problem hiding this comment.
Example 4 has broken logic and incorrect syntax.
This example has several issues:
-
Line 59 has incorrect syntax:
echo '...' >&${HT_PID}tries to redirect to a file descriptor, but$HT_PIDis a process ID, not a file descriptor. This would need to pipe to the process's stdin instead. -
The script is created but never executed: Line 66 just prints a message about the script location, but doesn't run it.
-
The example doesn't actually demonstrate sendKeys: Unlike the other examples which show working code, this one doesn't execute anything.
Consider either:
- Removing this example if sendKeys is too complex for a basic usage script
- Simplifying it to actually demonstrate working sendKeys usage
- Moving it to a dedicated sendKeys example file with proper implementation
If you want to keep this example, the script would need significant fixes to actually work. Would you like me to propose a working implementation?
| # Start ht with the script | ||
| $HT --subscribe init,exit /tmp/ht_sigterm_test.sh > /tmp/ht_sigterm_output 2>/dev/null & | ||
| HT_PID=$! | ||
|
|
||
| # Wait for init event and get the child PID | ||
| sleep 0.2 | ||
| CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_sigterm_output | head -1 | cut -d: -f2) | ||
|
|
||
| # Send SIGTERM to the child | ||
| kill -TERM $CHILD_PID 2>/dev/null |
There was a problem hiding this comment.
Add validation for CHILD_PID before using it.
Lines 70 and 73 have a potential race condition and missing validation:
- The script waits only 0.2 seconds before reading the PID
- There's no validation that
CHILD_PIDis non-empty before using it in thekillcommand - If the timing is unlucky or the system is slow, the
killcommand will fail with a usage error
Consider adding a polling loop with validation similar to the approach used in scripts/test_exit_tracking.sh (lines 107-115):
-sleep 0.2
-CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_sigterm_output | head -1 | cut -d: -f2)
+# Wait for init event and get the child PID
+for i in {1..20}; do
+ if [ -f /tmp/ht_sigterm_output ]; then
+ CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_sigterm_output | head -1 | cut -d: -f2 || true)
+ if [ -n "$CHILD_PID" ]; then
+ break
+ fi
+ fi
+ sleep 0.1
+done
+
+if [ -z "$CHILD_PID" ]; then
+ echo "Error: Could not get child PID"
+ kill $HT_PID 2>/dev/null || true
+ rm -f /tmp/ht_sigterm_test.sh /tmp/ht_sigterm_output
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Start ht with the script | |
| $HT --subscribe init,exit /tmp/ht_sigterm_test.sh > /tmp/ht_sigterm_output 2>/dev/null & | |
| HT_PID=$! | |
| # Wait for init event and get the child PID | |
| sleep 0.2 | |
| CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_sigterm_output | head -1 | cut -d: -f2) | |
| # Send SIGTERM to the child | |
| kill -TERM $CHILD_PID 2>/dev/null | |
| # Start ht with the script | |
| $HT --subscribe init,exit /tmp/ht_sigterm_test.sh > /tmp/ht_sigterm_output 2>/dev/null & | |
| HT_PID=$! | |
| # Wait for init event and get the child PID | |
| for i in {1..20}; do | |
| if [ -f /tmp/ht_sigterm_output ]; then | |
| CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_sigterm_output | head -1 | cut -d: -f2 || true) | |
| if [ -n "$CHILD_PID" ]; then | |
| break | |
| fi | |
| fi | |
| sleep 0.1 | |
| done | |
| if [ -z "$CHILD_PID" ]; then | |
| echo "Error: Could not get child PID" | |
| kill $HT_PID 2>/dev/null || true | |
| rm -f /tmp/ht_sigterm_test.sh /tmp/ht_sigterm_output | |
| exit 1 | |
| fi | |
| # Send SIGTERM to the child | |
| kill -TERM $CHILD_PID 2>/dev/null |
🤖 Prompt for AI Agents
In examples/exit_tracking.sh around lines 64 to 73, the script reads CHILD_PID
after a fixed 0.2s sleep which can race and then calls kill without validating
CHILD_PID; change this to poll for the PID (e.g., loop that retries grep for a
limited number of attempts with short sleeps), validate that CHILD_PID is
non-empty and numeric before calling kill, and fail with a clear error/exit
status if the PID cannot be obtained within the timeout so kill is only invoked
with a valid PID.
| # Run in background | ||
| $HT --subscribe init,exit /tmp/ht_signal_direct.sh > /tmp/ht_signal_direct_output 2>/dev/null & | ||
| HT_PID=$! | ||
|
|
||
| # Wait for init | ||
| sleep 0.2 | ||
| CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_signal_direct_output | head -1 | cut -d: -f2) | ||
|
|
||
| # Send SIGKILL | ||
| echo "Sending SIGKILL to PID $CHILD_PID..." | ||
| kill -KILL $CHILD_PID 2>/dev/null |
There was a problem hiding this comment.
Add validation for CHILD_PID before using it.
Similar to exit_tracking.sh, this example has a race condition:
- Line 44: Fixed 0.2-second sleep before reading PID
- Line 45: No validation that
CHILD_PIDis non-empty - Line 49: If CHILD_PID is empty, the
killcommand will fail
Consider using a polling loop with validation as suggested for exit_tracking.sh or as implemented in scripts/test_exit_tracking.sh (lines 107-115).
🤖 Prompt for AI Agents
In examples/signal_handling.sh around lines 39 to 49, the script reads CHILD_PID
after a fixed sleep but does not validate it before calling kill, creating a
race condition; update the logic to poll for a non-empty CHILD_PID (e.g., retry
grep of the pid field from /tmp/ht_signal_direct_output with a short sleep
between attempts and an overall timeout), fail with a clear error message if the
PID is not found within the timeout, and only call kill when CHILD_PID is a
valid, non-empty numeric value to avoid invoking kill with an empty argument.
| $HT --subscribe init,exit /tmp/ht_signal_test_$$.sh > /tmp/ht_signal_output_$$ 2>/dev/null & | ||
| local ht_pid=$! | ||
|
|
||
| sleep 0.2 | ||
| local child_pid | ||
| child_pid=$(grep -o '"pid":[0-9]*' /tmp/ht_signal_output_$$ | head -1 | cut -d: -f2) | ||
|
|
||
| echo " Sending $signal_name (signal $signal_num) to PID $child_pid..." | ||
| kill -$signal_name $child_pid 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
Add validation for child_pid before using it.
The test_signal helper function has the same race condition issue:
- Line 110: Fixed 0.2-second sleep
- Line 112: Reads
child_pidwithout validation - Line 115: Uses
child_pidinkillwithout checking if it's empty
The validation script scripts/test_exit_tracking.sh (lines 107-123) demonstrates a more robust approach with a polling loop and empty-check that could be adapted here.
🤖 Prompt for AI Agents
In examples/signal_handling.sh around lines 107 to 116, the code reads child_pid
directly after a fixed sleep and calls kill on it without validating it,
creating a race condition if the PID isn’t yet written; replace the single
sleep+read with a short polling loop (with a sensible timeout, e.g. a few
seconds and small sleep intervals) that repeatedly reads/parses
/tmp/ht_signal_output_$$ until child_pid is non-empty or the timeout elapses,
and if timeout elapses log a clear error message and skip the kill (or return
non-zero), otherwise proceed to call kill on the validated PID; make sure to
handle empty/invalid values safely and avoid failing the script on missing PID
(use conditional logic rather than unguarded kill).
| $HT --subscribe init,output,exit /tmp/ht_trap_example.sh > /tmp/ht_trap_output 2>/dev/null & | ||
| HT_PID=$! | ||
|
|
||
| sleep 0.3 | ||
| CHILD_PID=$(grep -o '"pid":[0-9]*' /tmp/ht_trap_output | head -1 | cut -d: -f2) | ||
|
|
||
| echo "Sending SIGTERM to process with trap..." | ||
| kill -TERM $CHILD_PID 2>/dev/null |
There was a problem hiding this comment.
Add validation for CHILD_PID in trap example.
Example 4 also has the race condition pattern:
- Line 168: Fixed 0.3-second sleep (slightly longer but still not robust)
- Line 169: No validation that
CHILD_PIDis non-empty - Line 172: Uses
CHILD_PIDinkillwithout checking
Consider applying the same polling loop pattern for consistency across all examples.
🤖 Prompt for AI Agents
In examples/signal_handling.sh around lines 165 to 172, the script reads
CHILD_PID from /tmp/ht_trap_output after a fixed sleep and then uses it without
validating it; add a polling loop that retries reading CHILD_PID (with a short
sleep and timeout) until a non-empty numeric PID is found, then validate that
the value is numeric/non-zero before calling kill, and fail with an error
message if the PID cannot be obtained within the timeout to avoid the race
condition.
| let mut stdin_open = true; | ||
| let mut api_running = true; | ||
| let mut output_open = true; | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| result = output_rx.recv() => { | ||
| result = &mut pty_handle => { | ||
| match result { | ||
| Ok(Ok(exit_status)) => { | ||
| eprintln!("process exited with code {}, shutting down...", exit_status.code); | ||
| session.exit(exit_status.code, exit_status.signal); | ||
| }, | ||
| Ok(Err(e)) => { | ||
| eprintln!("pty error: {e}, shutting down..."); | ||
| session.exit(1, None); | ||
| }, | ||
| Err(e) => { | ||
| eprintln!("pty task error: {e}, shutting down..."); | ||
| session.exit(1, None); | ||
| } | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
Wait to drain output before leaving the loop
Dropping output_rx right after the PTY handle resolves can discard any final buffered output, so clients miss the tail of the session.
Keep polling until the output channel closes:
let mut serving = true;
let mut stdin_open = true;
let mut api_running = true;
let mut output_open = true;
+ let mut pty_done = false;
loop {
tokio::select! {
- result = &mut pty_handle => {
+ result = &mut pty_handle, if !pty_done => {
match result {
Ok(Ok(exit_status)) => {
eprintln!("process exited with code {}, shutting down...", exit_status.code);
session.exit(exit_status.code, exit_status.signal);
+ pty_done = true;
},
- Ok(Err(e)) => {
- eprintln!("pty error: {e}, shutting down...");
- session.exit(1, None);
- },
- Err(e) => {
- eprintln!("pty task error: {e}, shutting down...");
- session.exit(1, None);
- }
}
- break;
}
…
}
+
+ if pty_done && !output_open {
+ break;
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main.rs around lines 88 to 110: the loop currently breaks immediately
when the pty_handle resolves which can drop output_rx and lose final buffered
output; instead, on pty_handle completion set the appropriate flags (e.g.,
stdin_open = false and api_running = false) and avoid breaking the loop so the
select continues to poll the output branch until the output channel actually
closes (output_open becomes false), only then break and drop output_rx; in
short, remove the immediate break on pty completion, update state to signal
shutdown, and let the loop exit naturally after the output channel is drained
and closed.
| match result { | ||
| Ok(Ok(exit_status)) => { | ||
| eprintln!("process exited with code {}, shutting down...", exit_status.code); | ||
| session.exit(exit_status.code, exit_status.signal); | ||
| }, | ||
| Ok(Err(e)) => { | ||
| eprintln!("pty error: {e}, shutting down..."); | ||
| session.exit(1, None); | ||
| }, | ||
| Err(e) => { | ||
| eprintln!("pty task error: {e}, shutting down..."); | ||
| session.exit(1, None); | ||
| } |
There was a problem hiding this comment.
Don’t swallow PTY errors
Both the Ok(Err(e)) and Err(e) arms now only log and continue, so run_event_loop returns Ok(()) and the CLI exits with status 0 even after a hard PTY failure. We need to bubble these errors up.
Apply this diff so failures still terminate the program with a non‑zero exit:
match result {
Ok(Ok(exit_status)) => {
eprintln!("process exited with code {}, shutting down...", exit_status.code);
session.exit(exit_status.code, exit_status.signal);
- },
- Ok(Err(e)) => {
- eprintln!("pty error: {e}, shutting down...");
- session.exit(1, None);
- },
- Err(e) => {
- eprintln!("pty task error: {e}, shutting down...");
- session.exit(1, None);
- }
+ pty_done = true;
+ },
+ Ok(Err(e)) => {
+ eprintln!("pty error: {e}, shutting down...");
+ session.exit(1, None);
+ return Err(e);
+ },
+ Err(e) => {
+ eprintln!("pty task error: {e}, shutting down...");
+ session.exit(1, None);
+ return Err(e.into());
+ }
}
- break;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main.rs around lines 95 to 107, the match arms for Ok(Err(e)) and Err(e)
currently only log and call session.exit, then allow run_event_loop to return
Ok(()); change these arms to propagate the failure by returning an Err so the
caller (and CLI) sees a non‑zero exit. Concretely, after logging (and optionally
calling session.exit), replace the continuation with return Err(e.into()) (or
otherwise convert the error into the function's error type) so the function
exits with Err on both PTY error arms.
1 participant
Summary by CodeRabbit
New Features
Documentation