fix: prevent "no route for conn:..." errors and add shell path expansion#3152
Open
re2zero wants to merge 1 commit intowavetermdev:mainfrom
Open
fix: prevent "no route for conn:..." errors and add shell path expansion#3152re2zero wants to merge 1 commit intowavetermdev:mainfrom
re2zero wants to merge 1 commit intowavetermdev:mainfrom
Conversation
|
|
Contributor
WalkthroughAdds a new per-connection atomic flag Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
re2zero
force-pushed
the
pr/merge-tideterm-fixes-clean
branch
from
fd6e00d to
d39e3a1
Compare
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Changes ReviewedThe incremental changes add:
NEW ISSUE: See inline comment about StartConnServer rejecting Status_Connected. Issue Details (click to expand)CRITICAL
Previous Issues (unresolved)
Files Reviewed (17 files)
Reviewed by minimax-m2.5-20260211 · 331,014 tokens Reviewed by minimax-m2.5-20260211 · 2,074,692 tokens |
re2zero
changed the title
re2zero
changed the title
re2zero
force-pushed
the
pr/merge-tideterm-fixes-clean
branch
3 times, most recently
from
7648926 to
a53d405
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/remote/conncontroller/conncontroller.go (1)
1153-1155: Consider wrapping the fallback error for better diagnostics.When the fallback wait times out,
context.DeadlineExceededis returned directly. Wrapping this error would help operators distinguish this specific failure path.♻️ Suggested improvement
if conn.WshEnsuring != nil && !conn.WshEnsuring.CompareAndSwap(false, true) { waitCtx, cancelWait := context.WithTimeout(ctx, 5*time.Second) defer cancelWait() - return wshutil.DefaultRouter.WaitForRegister(waitCtx, routeId) + if err := wshutil.DefaultRouter.WaitForRegister(waitCtx, routeId); err != nil { + return fmt.Errorf("waiting for concurrent wsh setup for %q: %w", conn.GetName(), err) + } + return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/remote/conncontroller/conncontroller.go` around lines 1153 - 1155, The current fallback returns context.DeadlineExceeded directly from the WaitForRegister call; change the return to inspect the error from wshutil.DefaultRouter.WaitForRegister(waitCtx, routeId) and if errors.Is(err, context.DeadlineExceeded) wrap it with context (e.g., fmt.Errorf("waiting for register for route %s timed out: %w", routeId, err)) so operators can distinguish this timeout path; ensure you import fmt (and errors if used) and keep the existing waitCtx/cancelWait usage around the wrapped return.pkg/shellexec/shellexec_test.go (1)
8-54: Add regression cases for tilde paths with spaces/metacharacters.Current coverage misses high-risk inputs (for example
~/My Docs,~/a;echo pwn,~/"quoted"), so shell-escaping regressions inposixCwdExprNoWshRemotewon’t be caught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shellexec/shellexec_test.go` around lines 8 - 54, Add regression tests to TestPosixCwdExprNoWshRemote that cover tilde paths containing spaces and shell metacharacters (examples: "~/My Docs", "~/a;echo pwn", `~/"quoted"`), calling posixCwdExprNoWshRemote with both a populated sshUser (e.g., "root") and empty sshUser and asserting the returned string is the safely-escaped/quoted form expected by the function contract (i.e., user-prefixed "~user/..." when sshUser provided, and a quoted "$HOME/..." when sshUser empty), so any regression in shell-escaping behavior will be caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/shellexec/shellexec.go`:
- Around line 176-177: When cwd starts with "~/", the current fishCwdExpr and
pwshCwdExpr return the raw tilde path which breaks on spaces and skips
special-char quoting; change fishCwdExpr to return a double-quoted tilde path
(e.g., use strconv.Quote or wrap with quotes) so "~/path with spaces" becomes
"\"~/path with spaces\"" and change pwshCwdExpr to expand "~" to "$HOME" and
return a double-quoted string (e.g., "$HOME/path with spaces" quoted) so
PowerShell expands safely; update any logic that skipped special-char quoting
for tilde paths to use these quoted/expanded forms instead (refer to fishCwdExpr
and pwshCwdExpr to locate edits).
---
Nitpick comments:
In `@pkg/remote/conncontroller/conncontroller.go`:
- Around line 1153-1155: The current fallback returns context.DeadlineExceeded
directly from the WaitForRegister call; change the return to inspect the error
from wshutil.DefaultRouter.WaitForRegister(waitCtx, routeId) and if
errors.Is(err, context.DeadlineExceeded) wrap it with context (e.g.,
fmt.Errorf("waiting for register for route %s timed out: %w", routeId, err)) so
operators can distinguish this timeout path; ensure you import fmt (and errors
if used) and keep the existing waitCtx/cancelWait usage around the wrapped
return.
In `@pkg/shellexec/shellexec_test.go`:
- Around line 8-54: Add regression tests to TestPosixCwdExprNoWshRemote that
cover tilde paths containing spaces and shell metacharacters (examples: "~/My
Docs", "~/a;echo pwn", `~/"quoted"`), calling posixCwdExprNoWshRemote with both
a populated sshUser (e.g., "root") and empty sshUser and asserting the returned
string is the safely-escaped/quoted form expected by the function contract
(i.e., user-prefixed "~user/..." when sshUser provided, and a quoted "$HOME/..."
when sshUser empty), so any regression in shell-escaping behavior will be
caught.
🪄 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
Run ID: 68291128-4c10-45ff-bf1b-3f6736fc0bb9
📒 Files selected for processing (3)
pkg/remote/conncontroller/conncontroller.gopkg/shellexec/shellexec.gopkg/shellexec/shellexec_test.go
re2zero
force-pushed
the
pr/merge-tideterm-fixes-clean
branch
from
a53d405 to
1f08cf9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/remote/conncontroller/conncontroller.go`:
- Around line 1141-1171: The recovery path calling conn.tryEnableWsh() can hit
StartConnServer which currently rejects calls when the connection is already
Status_Connected; change StartConnServer to allow a restart when the connection
is in Status_Connected if the underlying socket/listener is already present
(e.g., check the connection's existing server socket or listener field rather
than only status), so StartConnServer no longer returns "status is connected" in
that case; update the status-check logic in StartConnServer to permit the call
(or short-circuit to reuse the existing listener) and keep tryEnableWsh,
persistWshInstalled and the WshEnsuring logic unchanged so the auto-recovery in
the conncontroller code can succeed.
- Around line 283-286: The code reads conn.DomainSockListener and
conn.DomainSockName without holding SSHConn.lock, breaking the locking contract;
fix by acquiring the appropriate lock (use conn.lock.RLock()), copy those
guarded fields into local variables while the lock is held, release the lock,
then perform the check/conn.Infof and return based on the local copies (avoid
calling Infof while holding the lock to prevent potential deadlocks). Ensure you
reference SSHConn.lock, conn.DomainSockListener and conn.DomainSockName when
making the change.
In `@pkg/shellexec/shellexec.go`:
- Around line 166-170: The code returning "~"+sshUser+rest for cwd starting with
"~/” fails to quote path segments with spaces/special chars; change the branch
that handles strings.HasPrefix(cwd, "~/") so it preserves the unquoted "~user"
but appends a quoted, shell-escaped path portion: take rest := cwd[1:], escape
any internal double quotes/backslashes in rest, then return "~"+sshUser + "\"" +
escapedRest + "\"". Update the logic around the cwd/sshUser handling in
shellexec.go (the strings.HasPrefix(cwd, "~/") branch) to perform this quoting
so shells perform tilde expansion on ~user and then treat the remainder as a
single quoted path.
- Around line 202-207: The code path that builds a PowerShell string when cwd
starts with "~/": it slices rest := cwd[1:] and returns "~\"" + rest + "\""
without escaping, which allows `$` and backtick ` to be interpreted; change the
construction to produce a safe PowerShell literal by either wrapping rest in
single quotes (and doubling any single quote characters in rest) or by escaping
backticks, double-quotes and $ in rest before returning the "~\"...\"" form;
update the branch that returns "~\""+rest+"\"" to instead sanitize rest (e.g.,
replace `'` -> `''` if using single-quoted form, or replace backtick -> "``",
`"` -> "`\"" and `$` -> "`$" if keeping double-quoted form) so paths like
~/path$var and ~/'path' are handled correctly and no unescaped special
characters remain.
🪄 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
Run ID: 6605b30c-5e8b-48b8-a1db-c945bb4d3aa3
📒 Files selected for processing (3)
pkg/remote/conncontroller/conncontroller.gopkg/shellexec/shellexec.gopkg/shellexec/shellexec_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/shellexec/shellexec_test.go
- Auto-recover wsh routes after connserver restart - Allow domain socket listener reuse when already established - Add WshEnsuring flag to prevent thundering herd - Add shell-specific path expansion (posix, fish, pwsh) with tilde (~) support - Add escapeForPosixDoubleQuotes for safe path quoting - Add unit tests for path expansion logic
re2zero
force-pushed
the
pr/merge-tideterm-fixes-clean
branch
from
1f08cf9 to
9d05f5f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/shellexec/shellexec_test.go (1)
8-48: Consider adding test cases for special characters in Fish paths.The current tests cover spaces but don't verify escaping of
$,`,", and\within~/...paths. These characters are explicitly handled byescapeForPosixDoubleQuotes.💡 Suggested additional test cases
{ name: "tilde-with-spaces", cwd: "~/Documents/My Files", want: "\"$HOME/Documents/My Files\"", }, + { + name: "tilde-with-dollar", + cwd: "~/path$var", + want: "\"$HOME/path\\$var\"", + }, + { + name: "tilde-with-backtick", + cwd: "~/path`cmd`", + want: "\"$HOME/path\\`cmd\\`\"", + }, + { + name: "tilde-with-double-quote", + cwd: "~/path\"quoted\"", + want: "\"$HOME/path\\\"quoted\\\"\"", + }, { name: "absolute-path",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/shellexec/shellexec_test.go` around lines 8 - 48, Add unit tests to TestFishCwdExpr that cover special characters handled by escapeForPosixDoubleQuotes when cwd starts with "~/" — specifically include cases with $, ` (backtick), " (double quote), and \ (backslash) inside the path (e.g., "~/dir$name", "~/dir`name", "~/dir\"name", "~/dir\\name") and assert their expected outputs from fishCwdExpr; locate the test harness in TestFishCwdExpr and add table-driven entries referencing fishCwdExpr and escapeForPosixDoubleQuotes so the assertions verify the correct escaping/quoting behavior for these characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/shellexec/shellexec_test.go`:
- Around line 8-48: Add unit tests to TestFishCwdExpr that cover special
characters handled by escapeForPosixDoubleQuotes when cwd starts with "~/" —
specifically include cases with $, ` (backtick), " (double quote), and \
(backslash) inside the path (e.g., "~/dir$name", "~/dir`name", "~/dir\"name",
"~/dir\\name") and assert their expected outputs from fishCwdExpr; locate the
test harness in TestFishCwdExpr and add table-driven entries referencing
fishCwdExpr and escapeForPosixDoubleQuotes so the assertions verify the correct
escaping/quoting behavior for these characters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 88245287-907f-459e-8a7d-d1dbd8897506
📒 Files selected for processing (3)
pkg/remote/conncontroller/conncontroller.gopkg/shellexec/shellexec.gopkg/shellexec/shellexec_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/remote/conncontroller/conncontroller.go
Contributor
There was a problem hiding this comment.
CRITICAL: StartConnServer still rejects Status_Connected - recovery will fail
In conncontroller.go:451, StartConnServer only allows Status_Connecting:
allowed := WithLockRtn(conn, func() bool {
return conn.Status == Status_Connecting
})However, the recovery code at line 1167 calls tryEnableWsh() when the connection is already in Status_Connected. Inside tryEnableWsh() (line 907), it calls StartConnServer() which will be rejected with "cannot start conn server for ... when status is connected".
This is the exact failure this PR claims to fix - the recovery path cannot work because StartConnServer hasn't been updated to allow recovery from a connected state.
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
2 participants
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.
Summary
Fix "no route for conn:..." errors when using remote file browsing after connserver restart, and add shell-specific path expansion for better cross-shell compatibility.
Changes
Remote connection fixes:
Shell execution path expansion:
posixCwdExpr,posixCwdExprNoWshRemote,fishCwdExpr,pwshCwdExprfor different shell typesescapeForPosixDoubleQuotesfor safe path quotingTest Status
All Go tests pass (excluding unrelated tsgen test failure).