fix(ssh): reuse installed runtimes and pin Effect dependencies - #50
Conversation
github-actions
Bot
added
the
vouch:trusted
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe SSH runner now reuses validated installed t3 runtimes before using npm. Tests cover runtime selection and fallback behavior. Server dependency metadata and SSH troubleshooting guidance are also updated. ChangesSSH runtime selection
Server dependency metadata
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant RemoteRunner
participant InstalledRuntime
participant Npm
RemoteRunner->>InstalledRuntime: Validate installed t3 runtime
alt Runtime is valid
InstalledRuntime-->>RemoteRunner: Return CLI entry path
RemoteRunner->>InstalledRuntime: Execute CLI
else Runtime is invalid or unavailable
RemoteRunner->>Npm: Resolve and run CLI
Npm-->>RemoteRunner: Return runnable CLI
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The installed-runtime reuse preserves validated fallback behavior and the server dependency uses the existing catalog pin. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 review The current head is Please review the current head and submit an approval if no blocking findings remain. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Commits ==='
git show -s --format='%H%n%P%n%s' 4004f97ce
git show -s --format='%H%n%P%n%s' cb880eeb4
echo '=== Delta stat and names ==='
git diff --stat 4004f97ce..cb880eeb4
git diff --name-status 4004f97ce..cb880eeb4
echo '=== Delta ==='
git diff --find-renames --find-copies --unified=80 4004f97ce..cb880eeb4 -- packages/ssh/src/installedRuntime.test.ts
echo '=== Affected test file outline ==='
ast-grep outline packages/ssh/src/installedRuntime.test.ts --items all
echo '=== Existing Effect compiler exceptions in SSH tests ==='
rg -n -C 8 --glob '*test.ts' '(Effect compiler|effect compiler|filesystem|subprocess|daemon.discovery|daemon-discovery|unsafe)' packages/sshLength of output: 7792 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Daemon-discovery fixture header ==='
sed -n '1,45p' packages/ssh/src/daemonDiscovery.test.ts
echo '=== Whitespace checks ==='
git diff --check 4004f97ce..cb880eeb4
merge_base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main 2>/dev/null || true)"
if [ -n "$merge_base" ]; then
echo "merge-base: $merge_base"
git diff --check "$merge_base"..cb880eeb4
echo '=== Files changed from merge base ==='
git diff --name-status "$merge_base"..cb880eeb4
else
echo 'No local main reference available; merge-base comparison skipped.'
fi
echo '=== Commits since previously reviewed commit ==='
git log --oneline --no-decorate 4004f97ce..cb880eeb4Length of output: 16468
The delta only changes I found no blocking issues. Approving PR ✅ Action performedComments resolved and changes approved. |
|
@coderabbitai Please confirm whether you approve the current head cb880ee and submit an APPROVED review if no blockers remain. The only changes after your completed review are the test fixture compiler annotation and the docstring that addresses your warning. |
|
The scoped compiler annotation and the docstring do not introduce blockers. PR ✅ Action performedComments resolved and changes approved. |
Thread transfer impact✅ Thread transfer remains within every enforced ceiling.
Baseline: Scenario and decoded snapshot size10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.
Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed. |
1 participant
SSH pairing could time out while npm resolved the CLI even when the remote service already had the exact version installed. The published package also allowed npm to select a newer shared Effect dependency whose peer version was unavailable.
The SSH runner now reuses a complete installed runtime after checking its version, package identity, sentinel, and CLI entry. The server package directly pins the shared Effect dependency through the existing catalog. Explicit runner overrides and the RTVision registry fallback keep their existing behavior.
Validation: 57 focused SSH tests, scoped lint and SSH typecheck, and server dependency checks passed. Disposable npm alias and npm-exec installations retained rc.112, loaded node-pty, and passed CLI startup and service preflight. Fable 5.1 reviewed the patch; its dependency-audit, shell-portability, and documentation findings are addressed.
Implemented with GPT-6 in Codex. Reviewed with Fable 5.1 in Claude Code.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests