implement more OSC 16162 for fish, pwsh, and bash#2462
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a first-prompt handshake and shifts shell integrations to explicit preexec/precmd flows guarded by a _WAVETERM_SI_FIRSTPROMPT flag. Bash: embeds and installs a new bash_preexec.sh, avoids extdebug conflicts, registers _waveterm_si_preexec/_waveterm_si_precmd, and emits OSC 16162 (M/C/D/A) plus OSC 7 per prompt with uname and Bash version on first prompt. Fish: adds FIRSTPROMPT, preexec handler with base64 command encoding (8192-byte cap), chpwd hook, and fixes PWD encoding. PowerShell: adds FIRSTPROMPT and a prompt wrapper for initial handshake. Zsh: streamlines preexec encoding and marks integration in M payload. Frontend/types and runtime: rename cmd:hascurcwd → shell:hascurcwd, add shell:integration, handle OSC 16162 and OSC 7 changes. Go: embed bash_preexec.sh, refactor terminal description, add CommandInfo/LastCommand, add MarshalJSONString, and ensure RTInfo cleanup when controllers stop. Several UI paths remove RTInfo-clearing calls. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: unknown linters: 'unusedfunc,unusedparams', run 'golangci-lint help linters' to see the list of supported linters 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 |
…ackslashes for UNC
…rc and use it, fix cmd64 when base64 doesn't exist (rare)
![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
🧹 Nitpick comments (2)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
61-65: Consider splitting declaration and assignment (Shellcheck SC2155).The static analysis correctly notes that combining
local encoded_pwd=$(_waveterm_si_urlencode "$PWD")masks the return value of_waveterm_si_urlencode. While this function is unlikely to fail, splitting the declaration and assignment would be more robust:- local encoded_pwd=$(_waveterm_si_urlencode "$PWD") + local encoded_pwd + encoded_pwd=$(_waveterm_si_urlencode "$PWD")Note: The same pattern appears in zsh_zshrc.sh line 48, so consider applying consistently across shells.
Based on static analysis hints
101-103: Array append depends on successful bash_preexec.sh sourcing.These array append operations assume
precmd_functionsandpreexec_functionsarrays exist (provided by bash_preexec.sh sourced at lines 33-40). Consider adding a conditional check:# Add our functions to the bash-preexec arrays +if [[ -n "${bash_preexec_imported:-}" ]]; then precmd_functions+=(_waveterm_si_precmd) preexec_functions+=(_waveterm_si_preexec) +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/util/shellutil/shellintegration/bash_bashrc.sh(1 hunks)pkg/util/shellutil/shellintegration/fish_wavefish.sh(2 hunks)pkg/util/shellutil/shellintegration/zsh_zshrc.sh(1 hunks)pkg/util/shellutil/shellutil.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
_waveterm_si_osc7(61-65)_waveterm_si_blocked(45-47)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (5)
_waveterm_si_blocked(24-26)_waveterm_si_urlencode(28-44)_waveterm_si_osc7(46-50)_waveterm_si_precmd(52-66)_waveterm_si_preexec(68-82)pkg/util/shellutil/shellintegration/fish_wavefish.sh (2)
_waveterm_si_osc7(20-23)_waveterm_si_preexec(42-44)
pkg/util/shellutil/shellintegration/fish_wavefish.sh (2)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (3)
_waveterm_si_blocked(45-47)_waveterm_si_osc7(61-65)_waveterm_si_preexec(84-99)pkg/util/shellutil/shellintegration/zsh_zshrc.sh (3)
_waveterm_si_blocked(24-26)_waveterm_si_osc7(46-50)_waveterm_si_preexec(68-82)
🪛 Shellcheck (0.11.0)
pkg/util/shellutil/shellintegration/bash_bashrc.sh
[warning] 63-63: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (3)
42-42: LGTM!The FIRSTPROMPT flag initialization is consistent with the zsh and fish implementations.
67-82: LGTM! Well-structured precmd implementation.The function correctly:
- Captures exit status immediately to avoid losing it
- Distinguishes first prompt (M message with metadata) from subsequent prompts (D message with exit code)
- Emits OSC 7 on every prompt (necessary since bash lacks a chpwd hook, unlike zsh/fish)
- Clears the FIRSTPROMPT flag after first use
84-99: LGTM! Command encoding implementation is robust.The function correctly:
- Caps command size at 8192 bytes with a descriptive placeholder for oversized commands
- Base64-encodes the command with error handling
- Falls back to a minimal C message if encoding fails
- Matches the pattern used in zsh (lines 68-82) and fish implementations
pkg/util/shellutil/shellutil.go (2)
42-43: LGTM! Standard Go embed pattern.The embedding of bash_preexec.sh follows the same pattern as the other shell integration scripts and is correctly exported.
287-290: LGTM! File write is properly positioned and error-handled.The bash_preexec.sh file is written after the bashDir is created (line 238-242) and before it's needed by bash_bashrc.sh. File permissions (0644) and error handling are appropriate.
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (2)
61-61: LGTM! Documentation clarifications.The comment updates helpfully explain that OSC 7 is only sent on the first prompt, with the chpwd hook handling subsequent directory changes.
Also applies to: 64-64
70-81: LGTM! Streamlined command encoding logic.The refactor unifies the encoding flow by:
- Replacing oversized commands with a placeholder before encoding
- Eliminating duplicate encoding logic
- Making the flow consistent with the bash implementation (lines 84-99 in bash_bashrc.sh)
pkg/util/shellutil/shellintegration/fish_wavefish.sh (5)
12-12: LGTM! Global flag initialization is correct.The use of
set -gfor global scope is appropriate, and the pattern is consistent with bash and zsh implementations.
23-23: LGTM! Improved path quoting.Adding quotes around
$PWDensures proper handling of paths containing spaces or special characters.
27-40: LGTM! Fish prompt integration matches bash/zsh pattern.The function correctly:
- Captures exit status immediately
- Handles first-prompt handshake with M message and system info
- Sends D message with exit code on subsequent prompts
- Delegates directory-change OSC 7 updates to the chpwd hook (lines 60-62)
- Clears FIRSTPROMPT after first use
42-57: LGTM! Fish-native preexec implementation.The function correctly handles command encoding with a fish-specific approach:
- Joins argv to reconstruct the command
- Encodes placeholder for oversized commands (>8192 bytes)
- Uses fish-native
string replaceinstead oftrfor newline removal- Falls back to minimal C message if encoding fails
The branching structure differs slightly from bash/zsh but is functionally equivalent and arguably clearer.
60-62: LGTM! Efficient directory-change tracking.The
--on-variable PWDhook enables OSC 7 updates only when the directory changes, which is more efficient than the bash approach (which emits on every prompt due to lack of a chpwd mechanism). This matches the zsh pattern.
| # Source bash-preexec for proper preexec/precmd hook support | ||
| if [ -z "${bash_preexec_imported:-}" ]; then | ||
| _WAVETERM_SI_BASHRC_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| if [ -f "$_WAVETERM_SI_BASHRC_DIR/bash_preexec.sh" ]; then | ||
| source "$_WAVETERM_SI_BASHRC_DIR/bash_preexec.sh" | ||
| fi | ||
| unset _WAVETERM_SI_BASHRC_DIR | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
Verify bash_preexec.sh sourcing before using its arrays.
The code checks if the file exists before sourcing, but lines 102-103 unconditionally append to precmd_functions and preexec_functions arrays. If bash_preexec.sh isn't sourced successfully (e.g., file missing, syntax error), these arrays won't exist and the append operations will fail.
Run this script to verify the integration is resilient:
🏁 Script executed:
#!/bin/bash
# Check if bash_preexec.sh is properly integrated in the bashrc
# Verify bash_preexec.sh exists in the bash integration directory
fd -t f "bash_preexec.sh" pkg/util/shellutil/shellintegration/
# Check if there's error handling for missing bash_preexec arrays
rg -nP -A2 -B2 'precmd_functions\+=|preexec_functions\+=' pkg/util/shellutil/shellintegration/bash_bashrc.shLength of output: 197
🏁 Script executed:
#!/bin/bash
# Check if bash_preexec.sh defines the required arrays and if there's error handling
# Look at bash_preexec.sh to see if it defines precmd_functions and preexec_functions
echo "=== bash_preexec.sh content (checking for array definitions) ==="
cat pkg/util/shellutil/shellintegration/bash_preexec.sh | head -50
echo -e "\n=== Checking for bash_preexec_imported export in bash_preexec.sh ==="
rg -n "bash_preexec_imported" pkg/util/shellutil/shellintegration/bash_preexec.sh
echo -e "\n=== Context around lines 102-103 in bash_bashrc.sh (wider range) ==="
rg -nP -B10 -A5 'precmd_functions\+=|preexec_functions\+=' pkg/util/shellutil/shellintegration/bash_bashrc.shLength of output: 3009
🏁 Script executed:
#!/bin/bash
# Search for array initialization in bash_preexec.sh
echo "=== Looking for precmd_functions and preexec_functions in bash_preexec.sh ==="
rg -n 'precmd_functions|preexec_functions' pkg/util/shellutil/shellintegration/bash_preexec.sh
echo -e "\n=== Checking if arrays are declared with () syntax or initialized ==="
rg -n -E '\s(precmd_functions|preexec_functions)\s*=\(' pkg/util/shellutil/shellintegration/bash_preexec.sh
echo -e "\n=== Full bash_preexec.sh length and structure check ==="
wc -l pkg/util/shellutil/shellintegration/bash_preexec.shLength of output: 896
Guard array appends with bash_preexec_imported check.
The arrays precmd_functions and preexec_functions are properly declared in bash_preexec.sh (lines 136-137), but lines 102-103 append to them unconditionally. If bash_preexec.sh fails to source successfully (e.g., syntax error, permission issue), bash_preexec_imported won't be set and the arrays won't exist, causing the += operations to fail silently or behave unexpectedly.
Add a guard before lines 102-103:
if [ -n "${bash_preexec_imported:-}" ]; then
precmd_functions+=(_waveterm_si_precmd)
preexec_functions+=(_waveterm_si_preexec)
fi🤖 Prompt for AI Agents
In pkg/util/shellutil/shellintegration/bash_bashrc.sh around lines 33-40 (and
specifically before the array appends at lines ~102-103), the script appends to
precmd_functions and preexec_functions without ensuring bash_preexec.sh actually
sourced and set bash_preexec_imported; add a guard that checks that
bash_preexec_imported is non-empty before performing the += appends so the
arrays are only modified when they exist (i.e., wrap the two append statements
in an if [ -n "${bash_preexec_imported:-}" ]; then ... fi block).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (1)
108-109: Guard array appends withbash_preexec_importedcheck (duplicate of prior issue).Lines 108-109 append to
precmd_functionsandpreexec_functionsunconditionally, but these arrays only exist ifbash_preexec.shsourced successfully. If sourcing fails (e.g., syntax error, file missing, permission denied),bash_preexec_importedwon't be set and the arrays won't exist, causing the+=operations to fail.This was flagged in a prior review. Add a guard:
-# Add our functions to the bash-preexec arrays -precmd_functions+=(_waveterm_si_precmd) -preexec_functions+=(_waveterm_si_preexec) +# Add our functions to the bash-preexec arrays +if [ -n "${bash_preexec_imported:-}" ]; then + precmd_functions+=(_waveterm_si_precmd) + preexec_functions+=(_waveterm_si_preexec) +fi
🧹 Nitpick comments (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (1)
69-69: Consider splitting declaration and assignment to suppress shellcheck warning.Shellcheck flags SC2155 on line 69: declaring and assigning in a single statement can mask return values. While the current code doesn't check the return value of
_waveterm_si_urlencode, addressing this improves defensive coding practices:- local encoded_pwd=$(_waveterm_si_urlencode "$PWD") + local encoded_pwd + encoded_pwd=$(_waveterm_si_urlencode "$PWD")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (5)
_waveterm_si_blocked(24-26)_waveterm_si_urlencode(28-44)_waveterm_si_osc7(46-50)_waveterm_si_precmd(52-66)_waveterm_si_preexec(68-82)pkg/util/shellutil/shellintegration/fish_wavefish.sh (2)
_waveterm_si_osc7(20-23)_waveterm_si_preexec(42-44)
🪛 Shellcheck (0.11.0)
pkg/util/shellutil/shellintegration/bash_bashrc.sh
[warning] 69-69: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (3)
33-46: Approve extdebug guard and bash-preexec sourcing logic.The conditional sourcing of
bash_preexec.sh(lines 40-46) and the early return ifextdebugis enabled (lines 33-37) are appropriate safeguards. The check forbash_preexec_importedprevents redundant sourcing if already imported by user's shell configuration.
73-88: Approve first-prompt handshake and OSC 16162 implementation.The
_waveterm_si_precmdfunction correctly implements the first-prompt handshake, sending OSC 16162 "M" with shell/version/uname on the first prompt and "D" with exit code on subsequent prompts. OSC 7 is emitted on every prompt (appropriate since bash lacks chpwd hook), and the FIRSTPROMPT flag is properly managed.
90-105: Approve command preexecution and encoding logic.The
_waveterm_si_preexecfunction correctly handles command encoding: it caps commands at 8192 bytes (consistent with zsh and fish), encodes via base64, and emits OSC 16162 "C" with a fallback when encoding fails. The implementation mirrors the pattern used in other shells.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/aiusechat/tools.go (1)
46-53: Consider using constants for shell states.The shell state strings are hardcoded in the switch statement. Defining package-level or imported constants would improve maintainability and reduce the risk of typos.
Example approach:
const ( ShellStateReady = "ready" ShellStateRunningCommand = "running-command" ) // Then in the switch: switch rtInfo.ShellState { case ShellStateReady: stateStr = "waiting for input" case ShellStateRunningCommand: stateStr = "running command" default: stateStr = "state unknown" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/aiusechat/tools.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/aiusechat/tools.go (3)
pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Block(30-30)pkg/wstore/blockrtinfo.go (1)
GetRTInfo(89-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
pkg/aiusechat/tools.go (1)
76-76: LGTM!Clean refactoring that improves modularity by extracting the terminal description logic into a dedicated helper function.
| func makeTerminalBlockDesc(block *waveobj.Block) string { | ||
| connection, hasConnection := block.Meta["connection"].(string) | ||
| cwd, hasCwd := block.Meta["cmd:cwd"].(string) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive nil checks for robustness.
While MakeBlockShortDesc validates inputs before calling this helper, makeTerminalBlockDesc is a standalone function that could be invoked from other contexts within the package. Adding nil checks would prevent potential panics.
Apply this diff to add defensive checks:
func makeTerminalBlockDesc(block *waveobj.Block) string {
+ if block == nil || block.Meta == nil {
+ return "terminal block (metadata unavailable)"
+ }
connection, hasConnection := block.Meta["connection"].(string)
cwd, hasCwd := block.Meta["cmd:cwd"].(string)📝 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.
| func makeTerminalBlockDesc(block *waveobj.Block) string { | |
| connection, hasConnection := block.Meta["connection"].(string) | |
| cwd, hasCwd := block.Meta["cmd:cwd"].(string) | |
| func makeTerminalBlockDesc(block *waveobj.Block) string { | |
| if block == nil || block.Meta == nil { | |
| return "terminal block (metadata unavailable)" | |
| } | |
| connection, hasConnection := block.Meta["connection"].(string) | |
| cwd, hasCwd := block.Meta["cmd:cwd"].(string) |
🤖 Prompt for AI Agents
In pkg/aiusechat/tools.go around lines 21 to 23, add defensive nil checks so the
helper never panics when called with unexpected input: return an empty string
immediately if block is nil or if block.Meta is nil, then proceed to extract
"connection" and "cmd:cwd" using the existing comma-ok type assertions and fall
back to empty strings when keys or type assertions fail; ensure the function
always returns a valid string without dereferencing nil maps or values.
…, false for pwsh (no A, C, D tracking yet)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
aiprompts/wave-osc-16162.md(1 hunks)frontend/app/view/term/termwrap.ts(3 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/aiusechat/tools.go(4 hunks)pkg/blockcontroller/blockcontroller.go(4 hunks)pkg/util/shellutil/shellintegration/bash_bashrc.sh(1 hunks)pkg/util/shellutil/shellintegration/fish_wavefish.sh(2 hunks)pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh(2 hunks)pkg/util/shellutil/shellintegration/zsh_zshrc.sh(1 hunks)pkg/waveobj/blockrtinfo.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh
- pkg/aiusechat/tools.go
- pkg/util/shellutil/shellintegration/fish_wavefish.sh
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/blockcontroller/blockcontroller.go (3)
pkg/wstore/blockrtinfo.go (1)
DeleteRTInfo(102-107)pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Block(30-30)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (2)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
_waveterm_si_osc7(75-79)_waveterm_si_blocked(59-61)pkg/util/shellutil/shellintegration/fish_wavefish.sh (1)
_waveterm_si_osc7(20-23)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (5)
_waveterm_si_blocked(24-26)_waveterm_si_urlencode(28-44)_waveterm_si_osc7(46-50)_waveterm_si_precmd(52-66)_waveterm_si_preexec(68-82)pkg/util/shellutil/shellintegration/fish_wavefish.sh (2)
_waveterm_si_osc7(20-23)_waveterm_si_preexec(42-44)
🪛 Shellcheck (0.11.0)
pkg/util/shellutil/shellintegration/bash_bashrc.sh
[warning] 77-77: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
pkg/blockcontroller/blockcontroller.go (4)
84-99: Runtime info cleanup properly integrated.The addition of
wstore.DeleteRTInfowhen replacing an existing controller ensures stale runtime information (including the new shell integration flag) is properly cleaned up during controller lifecycle transitions.
241-248: LGTM – consistent cleanup in stop path.
250-257: LGTM – cleanup matches other stop paths.
267-278: LGTM – async cleanup properly scoped.The runtime info cleanup in the goroutine correctly captures both
idandcto ensure cleanup happens for the right controller.pkg/util/shellutil/shellintegration/zsh_zshrc.sh (2)
52-66: LGTM – integration flag and OSC 7 handling properly implemented.The M payload now includes
"integration":trueon first prompt, and the comment clarifies that OSC 7 is only sent initially (with the chpwd hook handling subsequent directory changes). This aligns with the PR's shell integration objectives.
68-82: LGTM – preexec command encoding streamlined.The refactored encoding flow with the 8192-byte size check and fallback handling is clear and consistent with the bash implementation.
pkg/util/shellutil/shellintegration/bash_bashrc.sh (4)
33-54: Extdebug guard and bash_preexec import handling properly implemented.The extdebug check prevents interference with bash-preexec semantics, and the import verification with early return ensures the integration is disabled cleanly if bash_preexec.sh fails to load. This addresses the concern from the past review comment, as the early return at line 53 ensures lines 116-117 are never reached if bash_preexec_imported is unset.
81-96: LGTM – first-prompt handshake and precmd logic properly implemented.The
_WAVETERM_SI_FIRSTPROMPTflag gates the initial M payload with shell version, uname, and integration flag, followed by D payloads on subsequent prompts. OSC 7 is sent on every prompt (since bash lacks a chpwd hook), and the A payload marks prompt readiness. This aligns with the PR objectives.
98-113: LGTM – preexec command encoding matches zsh implementation.The base64 encoding with 8192-byte size check and fallback is consistent with the zsh integration.
115-117: Array appends are guarded by early return.The past review comment suggested wrapping these appends in an explicit if-guard. However, the early return at line 53 ensures these lines are never reached if
bash_preexec_importedis unset, providing equivalent protection.aiprompts/wave-osc-16162.md (1)
63-87: LGTM – documentation updated to reflect integration flag.The M payload documentation now includes the optional
integrationfield, which tracks whether shell integration is active or disabled.pkg/waveobj/blockrtinfo.go (1)
6-21: LGTM – ShellIntegration field added to runtime info.The new
ShellIntegrationfield with the"shell:integration,omitempty"JSON tag extends the runtime info structure to carry the integration flag from OSC 16162 M payloads through the backend.frontend/types/gotypes.d.ts (1)
708-722: LGTM – TypeScript type updated for shell integration.The
"shell:integration"?: booleanproperty extends the ObjRTInfo type to match the backend's ShellIntegration field, enabling type-safe integration state tracking in the frontend.frontend/app/view/term/termwrap.ts (3)
145-149: UNC path preservation implemented correctly.The code now detects paths starting with
/\\\\(UNC format) and strips only the leading slash while preserving the backslashes, enabling proper UNC path handling in OSC 7 commands.
176-182: LGTM – OSC 16162 command type extended for integration flag.The M command type now includes the optional
integrationfield to match the updated payload specification.
229-242: LGTM – integration flag propagated to runtime info.The M command handler now extracts
cmd.data.integrationand populatesrtInfo["shell:integration"], completing the integration flag flow from shell to frontend.
| _waveterm_si_osc7() { | ||
| _waveterm_si_blocked && return | ||
| local encoded_pwd=$(_waveterm_si_urlencode "$PWD") | ||
| printf '\033]7;file://%s%s\007' "$HOSTNAME" "$encoded_pwd" | ||
| _waveterm_si_blocked && return | ||
| local encoded_pwd=$(_waveterm_si_urlencode "$PWD") | ||
| printf '\033]7;file://%s%s\007' "$HOSTNAME" "$encoded_pwd" | ||
| } |
There was a problem hiding this comment.
Address shellcheck warning on line 77.
The static analysis hint flags line 77 for declaring and assigning in the same statement, which can mask return values from the subshell.
Apply this diff to separate declaration and assignment:
_waveterm_si_osc7() {
_waveterm_si_blocked && return
- local encoded_pwd=$(_waveterm_si_urlencode "$PWD")
+ local encoded_pwd
+ encoded_pwd=$(_waveterm_si_urlencode "$PWD")
printf '\033]7;file://%s%s\007' "$HOSTNAME" "$encoded_pwd"
}🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 77-77: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In pkg/util/shellutil/shellintegration/bash_bashrc.sh around lines 75 to 79, the
current line declares and assigns local encoded_pwd in one statement which can
mask the subshell's return value; change it to first declare the variable with
local encoded_pwd and on the next line assign
encoded_pwd=$(_waveterm_si_urlencode "$PWD") so the subshell return code is
preserved, then continue using encoded_pwd in the printf as before.
…also drop two set to null paths since we clear on connection change in blockcontroller.go now.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/aiusechat/tools.go (1)
21-23: Add defensive nil checks to prevent panics.The function directly accesses
block.Metawithout verifying thatblockorblock.Metaare non-nil. While the callerMakeBlockShortDescvalidates these before calling this helper,makeTerminalBlockDesccould be invoked from other contexts, making defensive checks prudent.Apply this diff to add nil guards:
func makeTerminalBlockDesc(block *waveobj.Block) string { + if block == nil || block.Meta == nil { + return "terminal block (metadata unavailable)" + } connection, hasConnection := block.Meta["connection"].(string) cwd, hasCwd := block.Meta["cmd:cwd"].(string)Based on learnings
🧹 Nitpick comments (1)
pkg/util/utilfn/marshal.go (1)
205-213: Optional: Consider naming consistency and documentation clarity.The function is correct and will work well for encoding strings in shell integration payloads. Two minor refinements:
- Naming consistency: Use
MarshalJSONString(all-caps JSON) to match the convention inMustPrettyPrintJSONon line 31.- Documentation: Clarify that the returned string includes surrounding quotes (e.g., input
helloreturns"hello"), since that's the standard JSON representation of a string.Note: The error case is indeed rare—
json.Marshalon a string practically never fails.Optional diff:
-// MarshalJsonString marshals a string to JSON format, returning the properly escaped JSON string. -// Returns empty string if there's an error (rare). -func MarshalJsonString(s string) string { +// MarshalJSONString marshals a string to JSON format, returning the properly escaped and quoted JSON string. +// Returns empty string if there's an error (practically impossible for string input). +func MarshalJSONString(s string) string { jsonBytes, err := json.Marshal(s) if err != nil { return "" } return string(jsonBytes) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/app/view/term/termwrap.ts(5 hunks)pkg/aiusechat/tools.go(3 hunks)pkg/aiusechat/tools_term.go(5 hunks)pkg/util/utilfn/marshal.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/aiusechat/tools.go (4)
pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Block(30-30)pkg/wstore/blockrtinfo.go (1)
GetRTInfo(89-99)pkg/util/utilfn/marshal.go (1)
MarshalJsonString(207-213)
pkg/aiusechat/tools_term.go (3)
pkg/waveobj/waveobj.go (1)
MakeORef(71-76)pkg/waveobj/wtype.go (1)
OType_Block(30-30)pkg/wstore/blockrtinfo.go (1)
GetRTInfo(89-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
frontend/app/view/term/termwrap.ts (3)
144-149: LGTM! UNC path handling correctly implemented.The logic properly strips the leading slash from UNC paths (e.g.,
/\\server\share→\\server\share) while preserving backslashes, which is correct for Windows UNC path notation.
228-229: Good practice: explicitly clearing exit code on command start.Setting
shell:lastcmdexitcodeto null when starting a new command (C command) ensures the state machine correctly reflects that a new command sequence has begun and the previous exit code no longer applies. This prevents stale exit codes from being associated with running commands.
241-243: No action required—integration flag correctly populated across all shell integrations.Verification confirms that all shell integration scripts (bash, fish, pwsh, and zsh) emit the
integrationflag in their OSC 16162 M payloads:
- bash (line 88):
"integration":truefor normal operation- fish (line 32):
"integration":true- zsh (line 60):
"integration":true- pwsh (line 45):
"integration":false(fallback scenario)The code in termwrap.ts correctly captures and stores this flag via
rtInfo["shell:integration"]. The implementation satisfies the requirement.pkg/aiusechat/tools.go (2)
36-67: Terminal description logic is comprehensive and well-structured.The function provides a rich, context-aware description that includes:
- Shell type and version when available
- Shell integration status with normalized state descriptions
- Running command context with appropriate truncation (30 chars)
- JSON escaping via
MarshalJsonStringfor command stringsThe state machine handling (ready → "waiting for input", running-command → "running command [cmd]") aligns well with the OSC 16162 protocol.
152-153: Privacy concern addressed by commenting out the log statement.The debug log that previously exposed PII (usernames, paths, connection names) is now commented out, addressing the privacy concern raised in the previous review.
pkg/aiusechat/tools_term.go (3)
28-32: Well-designed CommandInfo structure.The type provides a clear API:
Command: the command textStatus: running or completed stateExitCode: optional pointer, present only when completedUsing a pointer for
ExitCodecorrectly distinguishes between "no exit code yet" (nil) and "exit code is 0" (valid zero value).
167-183: LastCommand population logic is correct and defensive.The implementation:
- Properly checks
rtInfo != nilbefore accessing fields- Only populates when
ShellIntegrationis true andShellLastCmdis non-empty- Correctly maps shell states:
running-command→ Status "running" (no exit code)ready→ Status "completed" (includes exit code)- Takes the address of
ShellLastCmdExitCodefor the pointer fieldThe guard conditions prevent false data from being returned when shell integration is disabled or incomplete.
88-88: Description accurately reflects new capability.The updated description correctly informs users that last command and exit code are available when shell integration is enabled, setting appropriate expectations.
1 participant
No description provided.