feat: add session recorder for shell interaction recording#237
Merged
Conversation
Contributor
|
Thanks for the pull request. A maintainer will review it when available. Please keep the PR focused, explain the why in the description, and make sure local checks pass before requesting review. Contribution guide: https://github.com/AI-Shell-Team/aish/blob/main/CONTRIBUTING.md |
Contributor
|
This pull request description looks incomplete. Please update the missing sections below before review. Missing items:
|
|
Warning Review limit reached
More reviews will be available in 17 minutes and 9 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR adds asciinema v2 terminal session recording to aish, enabling users to record, save, and replay shell sessions. A new ChangesTerminal Session Recording
Sequence Diagram(s)sequenceDiagram
participant User
participant AishShell
participant ShellRenderer
participant Recorder
participant File as .cast file
User->>AishShell: /record start
AishShell->>Recorder: new(file_path, term_size)
Recorder->>File: Write JSON header
User->>AishShell: input command
AishShell->>ShellRenderer: set_shared_recorder()
AishShell->>Recorder: shared_record_input(input)
Recorder->>File: Write input event
AishShell->>AishShell: execute AI/tool
AishShell->>ShellRenderer: render_markdown()
ShellRenderer->>Recorder: shared_record_output(ANSI)
Recorder->>File: Write output event
User->>AishShell: /record stop
AishShell->>Recorder: flush()
Recorder->>File: Ensure all events written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/aish-shell/src/app.rs (2)
2956-2961:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScript recordings are still only partially wired.
print_md_with_recording()covers theai "..."branch, but script shell output still goes throughScriptExecutor/flush_bash_segment()without any recorder call. Mixed.aishscripts will replay only the AI sections and omit the bash output between them.🤖 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 `@crates/aish-shell/src/app.rs` around lines 2956 - 2961, Script recordings miss bash output because only print_md_with_recording records AI responses; update ScriptExecutor/flush_bash_segment to record emitted bash output segments (use the same recorder used by print_md_with_recording), by passing the shared_recorder into ScriptExecutor (or otherwise giving it access) and invoking the recorder API when flushing segments; adjust ScriptExecutor constructor and all creation sites to accept the recorder reference and ensure flush_bash_segment calls the recorder with the segment text before or after writing to the PTY so mixed .aish scripts replay both AI and shell output.
1490-1544:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecord the
read_line_with_initial()path too.When the slash picker is dismissed and the user continues with
/...text, this branch executes the command without ever callingshared_record_input(). The replay will show the output, but not the command that produced it.Suggested fix
match rl.read_line_with_initial(&prompt_str, (&text, "")) { Ok(Some(line)) => { + crate::recorder::shared_record_input( + &self.shared_recorder, + &format!("{}\n", line), + ); let input = line.trim(); if !input.is_empty() { self.state.history.push(input.to_string());🤖 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 `@crates/aish-shell/src/app.rs` around lines 1490 - 1544, The read_line_with_initial() branch currently pushes the raw input into self.state.history but never invokes the shared recording path; call the shared_record_input helper (the same method used elsewhere for recording inputs) as soon as you have a non-empty trimmed input inside the Ok(Some(line)) branch so that all subsequent paths (SpecialCommand via handle_special_command, Command/OperatorCommand via execute_external_command, BuiltinCommand via state.handle_builtin, Help, ScriptCall via execute_script) use the same recording logic; replace or supplement existing direct record_history(...) calls in those branches with the shared_record_input call (or ensure shared_record_input delegates to record_history with the correct exit_code) so the slash-picker/read_line_with_initial path is recorded consistently.
🧹 Nitpick comments (3)
crates/aish-shell/src/recorder.rs (1)
122-128: 💤 Low valueDocument fallback behavior for recordings directory.
When
data_local_dir()returnsNone(rare but possible on some systems), recordings will be created in the current working directory. This could surprise users who expect a standard location like~/.local/share/aish/recordings/.Consider documenting this fallback in the function's doc comment, or emitting a warning when the fallback is used.
🤖 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 `@crates/aish-shell/src/recorder.rs` around lines 122 - 128, Update the recordings_dir() doc comment to explicitly state that dirs::data_local_dir() may return None and in that case the function falls back to the CWD (creating ./aish/recordings/), and add a warning log when this fallback is taken; locate the logic inside recordings_dir() (which calls dirs::data_local_dir()) and, on the unwrap_or_else branch that uses PathBuf::from("."), emit a warning via the crate's logger/tracing (or add a cfg-gated log if logging isn't already available) so users are informed when the non-standard fallback is used.crates/aish-shell/src/renderer.rs (2)
442-449: 💤 Low valueFixed 5-character separator length lacks explanation.
The recorded separator is hard-coded to exactly 5 characters (
"─────\r\n") regardless of terminal width, while the displayed separator uses the actual terminal width. The comment mentions "clean cast/GIF output" but doesn't explain why 5 specifically.Add a comment explaining the reasoning (e.g., "5 chars provides visual separation in GIF without overwhelming the recording" or "matches asciinema convention"), or consider making this configurable.
🤖 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 `@crates/aish-shell/src/renderer.rs` around lines 442 - 449, The recorded separator uses a hard-coded 5-character string ("─────\r\n") in render_separator while the printed separator uses terminal_width; update render_separator/shared_record_output usage to either (a) add a clear comment next to the hard-coded "─────\r\n" explaining why 5 characters was chosen (e.g., for compact GIF/asciinema separation or to avoid overwhelming recordings), or (b) make the recorded separator length configurable or derived from self.terminal_width (e.g., min(self.terminal_width, 5) or a new config field) so recorded output matches intent; reference the render_separator method and crate::recorder::shared_record_output when making the change.
383-389: ⚡ Quick winFragile newline trimming assumes exact richrs behavior.
The code assumes
richrsalways appends exactly"\n\n"to markdown paragraphs. If richrs changes this behavior (e.g., adds more/fewer newlines, or changes the logic), this trimming will either not trigger (leaving extra newlines) or remove the wrong newline (creating incorrect spacing).Consider either:
- Making this more robust by counting trailing newlines and removing the documented excess amount
- Adding a comment explaining this is a known coupling with richrs v[version] behavior
- Verifying this assumption in tests
🤖 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 `@crates/aish-shell/src/renderer.rs` around lines 383 - 389, The trimming of trailing newlines in the shared recorder block is brittle because it assumes segs.to_ansi() always ends with exactly "\n\n"; update the logic around self.shared_recorder / segs.to_ansi() / crate::recorder::shared_record_output to robustly normalize trailing newlines by counting trailing '\n' and collapsing any run of >=2 newlines down to a single newline (i.e., leave one newline, remove the excess), replace the ends_with("\n\n") + truncate(...) code with that normalization, and add a short inline comment noting the coupling to richrs formatting and add a unit/integration test that validates the behavior for 0, 1, 2, and 3+ trailing newlines.
🤖 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 `@crates/aish-shell/src/app.rs`:
- Around line 1431-1437: The recorder is writing the raw input before secrets
are redacted; move the call to crate::recorder::shared_record_input so it
happens after invoking the security handling (e.g., check_security_gate /
whatever returns the sanitized/consent decision) and record the sanitized line
(or skip recording if the user chose to cancel) instead of the original `line`;
update the Ok(Some(line)) branch to call check_security_gate first and pass its
sanitized result into shared_record_input (or omit recording on cancel) to avoid
leaking secrets into the .cast.
- Around line 981-995: When printing the separator newline before a tool banner,
also record that newline so replay doesn't glue the banner to previous streamed
text: when content_started_flag.load(Ordering::SeqCst) is true, call
crate::recorder::shared_record_output(&shared_recorder_cb, "\n") (or otherwise
prepend "\n" to the recorded string) before recording the tool_line; update the
code around content_started_flag, tool_line, and shared_record_output to ensure
the blank line is recorded prior to printing and flushing.
In `@crates/aish-shell/src/recorder.rs`:
- Around line 143-149: The write_event function currently suppresses all write
errors which can silently corrupt recordings; update write_event to detect and
handle writeln! errors on self.writer: on Err(e) either log the failure (e.g.,
via eprintln! or the crate's logging facility) including context (ts,
event_type) and the error, and/or set a persistent failure flag on the recorder
struct (e.g., recording_write_failed: bool) so callers (like stop/flush logic)
can detect and report a failed recording; ensure you reference the existing
writer field and start_time in the log/flag update so failures are diagnosable.
In `@crates/aish-shell/tests/recorder_integration_test.rs`:
- Around line 41-42: Update the misleading comment above the assertion: correct
the event breakdown to reflect the actual counts produced by record_input —
change "5 events (4 output + 1 input + 1 echo)" to "5 events (3 output + 1 input
+ 1 echo)" so the comment reads "Should have: 1 header + 5 events (3 output + 1
input + 1 echo) = 6 lines"; reference the test's use of record_input and the
assert_eq!(lines.len(), 6) to locate the comment to update.
---
Outside diff comments:
In `@crates/aish-shell/src/app.rs`:
- Around line 2956-2961: Script recordings miss bash output because only
print_md_with_recording records AI responses; update
ScriptExecutor/flush_bash_segment to record emitted bash output segments (use
the same recorder used by print_md_with_recording), by passing the
shared_recorder into ScriptExecutor (or otherwise giving it access) and invoking
the recorder API when flushing segments; adjust ScriptExecutor constructor and
all creation sites to accept the recorder reference and ensure
flush_bash_segment calls the recorder with the segment text before or after
writing to the PTY so mixed .aish scripts replay both AI and shell output.
- Around line 1490-1544: The read_line_with_initial() branch currently pushes
the raw input into self.state.history but never invokes the shared recording
path; call the shared_record_input helper (the same method used elsewhere for
recording inputs) as soon as you have a non-empty trimmed input inside the
Ok(Some(line)) branch so that all subsequent paths (SpecialCommand via
handle_special_command, Command/OperatorCommand via execute_external_command,
BuiltinCommand via state.handle_builtin, Help, ScriptCall via execute_script)
use the same recording logic; replace or supplement existing direct
record_history(...) calls in those branches with the shared_record_input call
(or ensure shared_record_input delegates to record_history with the correct
exit_code) so the slash-picker/read_line_with_initial path is recorded
consistently.
---
Nitpick comments:
In `@crates/aish-shell/src/recorder.rs`:
- Around line 122-128: Update the recordings_dir() doc comment to explicitly
state that dirs::data_local_dir() may return None and in that case the function
falls back to the CWD (creating ./aish/recordings/), and add a warning log when
this fallback is taken; locate the logic inside recordings_dir() (which calls
dirs::data_local_dir()) and, on the unwrap_or_else branch that uses
PathBuf::from("."), emit a warning via the crate's logger/tracing (or add a
cfg-gated log if logging isn't already available) so users are informed when the
non-standard fallback is used.
In `@crates/aish-shell/src/renderer.rs`:
- Around line 442-449: The recorded separator uses a hard-coded 5-character
string ("─────\r\n") in render_separator while the printed separator uses
terminal_width; update render_separator/shared_record_output usage to either (a)
add a clear comment next to the hard-coded "─────\r\n" explaining why 5
characters was chosen (e.g., for compact GIF/asciinema separation or to avoid
overwhelming recordings), or (b) make the recorded separator length configurable
or derived from self.terminal_width (e.g., min(self.terminal_width, 5) or a new
config field) so recorded output matches intent; reference the render_separator
method and crate::recorder::shared_record_output when making the change.
- Around line 383-389: The trimming of trailing newlines in the shared recorder
block is brittle because it assumes segs.to_ansi() always ends with exactly
"\n\n"; update the logic around self.shared_recorder / segs.to_ansi() /
crate::recorder::shared_record_output to robustly normalize trailing newlines by
counting trailing '\n' and collapsing any run of >=2 newlines down to a single
newline (i.e., leave one newline, remove the excess), replace the
ends_with("\n\n") + truncate(...) code with that normalization, and add a short
inline comment noting the coupling to richrs formatting and add a
unit/integration test that validates the behavior for 0, 1, 2, and 3+ trailing
newlines.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4d728d77-8de2-4219-9abc-c464c026446c
📒 Files selected for processing (9)
crates/aish-i18n/locales/en-US.yamlcrates/aish-i18n/locales/zh-CN.yamlcrates/aish-shell/src/app.rscrates/aish-shell/src/lib.rscrates/aish-shell/src/prompt.rscrates/aish-shell/src/readline.rscrates/aish-shell/src/recorder.rscrates/aish-shell/src/renderer.rscrates/aish-shell/tests/recorder_integration_test.rs
Add recorder module for capturing and replaying shell sessions, including integration tests. Update shell components to support recording functionality and refresh i18n locale files.
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
1 participant
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.
Add recorder module for capturing and replaying shell sessions,
including integration tests. Update shell components to support
recording functionality and refresh i18n locale files.
Summary by CodeRabbit
New Features
/recordslash command to start and stop terminal session recordingInternationalization
/recordcommand and recording status messages/recordcommand and recording status messages