Skip to content

Conversation

@sawka
Copy link
Member

@sawka sawka commented Nov 20, 2025

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 20, 2025

Walkthrough

This PR updates OSC 7 current-directory URL emission across shell integration scripts: bash_bashrc.sh, zsh_zshrc.sh, fish_wavefish.sh, and pwsh_wavepwsh.sh now emit OSC 7 URLs using the host prefix file://localhost instead of embedding the detected hostname. Additionally, zsh_zshrc.sh adjusts URL-encoding substitutions to include explicit backslashes when escaping characters (%, space, #, &, ;, +). No public or exported entities were added, removed, or modified.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Changes span four files with a consistent pattern (OSC 7 host -> localhost) plus a distinct encoding tweak in zsh.
  • Review attention:
    • zsh_zshrc.sh: validate the revised percent-encoding substitutions and shell escaping semantics.
    • bash_bashrc.sh, fish_wavefish.sh, pwsh_wavepwsh.sh: confirm OSC 7 formatting and that removing hostname behaves as intended across terminals/OSes.
    • pwsh_wavepwsh.sh: ensure removed hostname-resolution logic has no side effects on other code paths.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No pull request description was provided by the author, so there is no content to evaluate against the changeset. Add a description explaining the OSC7 URL encoding changes across all shell integrations, including why localhost replaces hostname and how the URL encoding fixes were applied.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title focuses on fixing OSC7 encoding for zsh with bad regex substitutions, which is the primary concern addressed in the zsh file. However, the PR also fixes OSC7 encoding across bash, fish, and PowerShell, making the title partially incomplete.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch sawka/fix-osc7-encoding

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 71d9c83 and 4ee1c1d.

📒 Files selected for processing (3)
  • pkg/util/shellutil/shellintegration/bash_bashrc.sh (1 hunks)
  • pkg/util/shellutil/shellintegration/fish_wavefish.sh (1 hunks)
  • pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh (1 hunks)
⏰ 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). (1)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
pkg/util/shellutil/shellintegration/fish_wavefish.sh (1)

20-25: OSC 7 URL format consistent with other shells.

The change to file://localhost aligns with the parallel updates in bash, pwsh, and fish. The use of fish's built-in string escape --style=url (line 23) is idiomatic and appropriate for URL encoding.

pkg/util/shellutil/shellintegration/pwsh_wavepwsh.sh (1)

27-35: Correct URL format for Windows paths.

The change to include file://localhost/ with a forward slash is appropriate for Windows environments. Since $PWD.Path on Windows resolves to drive-letter paths (e.g., C:\...), the resulting URL format like file://localhost/C%3A%5CUsers%5C... is a valid and standard OSC 7 URL for Windows. The [System.Uri]::EscapeDataString() method properly handles the encoding.

pkg/util/shellutil/shellintegration/bash_bashrc.sh (1)

63-79: OSC 7 URL update is correct; verify zsh file changes.

The change to the OSC 7 format (line 78) using file://localhost is correct and consistent with the other shells. The existing _waveterm_si_urlencode function (lines 63-73) properly encodes the path, and the format will produce valid URLs like file://localhost/path/to/dir on Unix systems.

However, the PR title mentions "fix osc7 encoding for zsh (bad regex substitutions)" but the zsh_zshrc.sh file is not included in this review. The bash file uses parameter expansion (not regex), so the "bad regex substitutions" must refer to differences in the zsh file. Please verify that zsh_zshrc.sh has been updated with the regex fixes mentioned in the PR title.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (1)

48-48: Optional: Consider separating declare and assign to allow return value inspection.

Shellcheck flags SC2155 for declaring and assigning in one statement. While this is acceptable in the current context (since return values are not being checked), separating the declaration from the assignment would allow future error handling if needed.

If error handling is planned, consider:

  local encoded_pwd
  encoded_pwd=$(_waveterm_si_urlencode "$PWD")
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6420ae9 and 71d9c83.

📒 Files selected for processing (1)
  • pkg/util/shellutil/shellintegration/zsh_zshrc.sh (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (1)
pkg/util/shellutil/shellintegration/bash_bashrc.sh (2)
  • _waveterm_si_blocked (59-61)
  • _waveterm_si_urlencode (63-73)
🪛 Shellcheck (0.11.0)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh

[warning] 48-48: 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). (1)
  • GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (2)

34-41: Correct and explicit URL-encoding escapes.

The explicit backslash escapes for special characters (%, space, #, &, ;, +) ensure these characters are treated literally in the glob pattern context, making the substitutions robust and preventing unintended special character interpretation. This aligns with proper parameter expansion syntax and mirrors the intent of the bash version.


49-49: Verify the OSC 7 URL construction change.

The OSC 7 directory URL was changed from using a host variable to hardcoding localhost. While this is valid for local shell integration, please confirm this is the intended fix and that it doesn't break functionality in any context (e.g., remote sessions, container environments). The original code may have used a dynamic host variable for a reason.

@sawka sawka merged commit 8b246d7 into main Nov 20, 2025
3 of 4 checks passed
@sawka sawka deleted the sawka/fix-osc7-encoding branch November 20, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants