fix(ssh): treat a clean sudo pre-test exit as access propagated - #447
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a hang during --sudo SSH sessions by recognizing that a successful sudo -nv access-propagation pre-test (exit code 0, no stderr) is itself proof that sudo access has already propagated (e.g., NOPASSWD or cached sudo), even when the provider’s “provisioned” stderr pattern is never emitted.
Changes:
- Treat a clean pre-test exit (
code === 0) as “access propagated” for access-propagation pre-tests. - Reuse the computed
accessPropagatedresult to drive both retry/timeout behavior and pre-test exit-code normalization. - Add regression tests covering NOPASSWD sudo (exit
0, no output) vs. still-unpropagated sudo (stderr + non-zero exit).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/plugins/ssh/index.ts | Updates access-propagation classification to treat a successful sudo -nv pre-test exit as propagated, preventing unnecessary retries/timeouts. |
| src/plugins/ssh/tests/index.test.ts | Adds tests ensuring the sudo pre-test proceeds on exit 0 (NOPASSWD) and fails when the propagation window is already closed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe SSH plugin now treats a zero-exit access-propagation pre-test as successful, including when provider pattern detection is absent. The pre-test exit handling uses the computed propagation result for success overrides. Tests add a sudo provider and verify that passwordless 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
A `--sudo` SSH/SCP session first runs a `sudo -nv` pre-test to wait for the sudoers grant to propagate on the target. The propagation guard decides access is ready only from stderr: providers with `provisionedAccessPatterns` (e.g. Azure) require the `sudo: a password is required` message to appear. When the user is granted passwordless (NOPASSWD) sudo, `sudo -nv` succeeds silently and exits 0 with no output, so neither the provisioned nor the unprovisioned pattern matches. The guard therefore reports "not propagated" and the CLI keeps re-running the pre-test every few seconds until the propagation timeout elapses, appearing to hang in the pre-test loop before finally failing with "Access did not propagate ... in time". Treat a clean exit (code 0) from the pre-test probe as an unambiguous signal that access has propagated: a `sudo -nv` that returns 0 means the user already holds sudo. This is scoped to the pre-test, so the normal session path is unaffected. Adds regression tests covering the NOPASSWD (exit 0) case and confirming that a still-propagating, non-sudoer probe still fails once the window closes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gn4m5gmNUEdgYpyiX6U12C
d126801 to
9d82f4c
Compare
Problem
A
--sudoSSH/SCP session first runs asudo -nvpre-test and waits for the sudoers grant to propagate on the target host. The propagation guard decides readiness only from stderr patterns — for providers withprovisionedAccessPatterns(e.g. Azure), it waits forsudo: a password is required. When the user holds passwordless (NOPASSWD) sudo,sudo -nvsucceeds silently with exit code 0 and no output, so no pattern ever matches. The CLI keeps re-running the pre-test until the propagation window closes, appearing to hang before failing with "Access did not propagate ... in time" — even though access was ready the whole time.Change
Treat a clean exit (code 0) from the pre-test probe as an unambiguous signal that access has propagated:
sudo -nvreturning 0 means the user already holds sudo. The check is scoped to the pre-test only, so the normal session retry path is unaffected.Check off any of the following areas of code that are modified:
Type of Change
Validation
Automated: added regression tests covering the NOPASSWD (exit 0) pre-test case and confirming that a still-propagating, non-sudoer probe still fails once the propagation window closes.
yarn vitest run src/plugins/ssh/__tests__/index.test.ts— 9/9 passing.Risk is limited to the pre-test path: a probe that exits non-zero still follows the existing pattern-matching and retry logic unchanged.
Tracking (optional)
CUS-801