feat: sudo password prompt via chat UI (askpass bridge)#845
Closed
szmidtpiotr wants to merge 2 commits into
Closed
feat: sudo password prompt via chat UI (askpass bridge)#845szmidtpiotr wants to merge 2 commits into
szmidtpiotr wants to merge 2 commits into
Conversation
Agent/CLI sudo commands that need a password now prompt the user through the chat UI instead of hanging. A per-run token registers a loopback-only askpass bridge; the password is requested over the chat WebSocket and resolved back to the waiting sudo process. - sudo-askpass module: per-run token registry + loopback askpass HTTP route - providers (claude-sdk, cursor, gemini, opencode) register/unregister a sudo run and inject SUDO_ASKPASS env around each invocation - chat WebSocket handles sudo-password-response; new sudo_password_request / sudo_password_cancelled message kinds - frontend: SudoPasswordModal + useSudoPasswordPrompt hook, Shell sudo util - tests for askpass integration, service, sudo-prompt, and shell sudo util Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a sudo askpass bridge: server-side service that registers per-run tokens and env, an internal loopback route for askpass helpers, WebSocket message extensions for prompting and resolving passwords, subprocess environment wiring for multiple CLIs, frontend chat and shell modals to collect passwords, and tests. ChangesSudo password handling system
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/opencode-cli.js (1)
205-212:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
spawnEnvis undefined – runtime error and sudo env not injected.
Object.assign(spawnEnv, sudoRun.env)referencesspawnEnv, which is never declared. This will throw aReferenceErrorat runtime. Additionally, even if it didn't error, the spawn call at line 211 uses{ ...process.env }instead of the merged environment, so the sudo askpass variables would never reach the subprocess.🐛 Proposed fix
void providerModelsService.resolveResumeModel('opencode', sessionId, model).then((resolvedModel) => { const args = ['run', '--format', 'json']; if (sessionId) { args.push('--session', sessionId); } if (resolvedModel) { args.push('--model', resolvedModel); } if (command && command.trim()) { args.push(command.trim()); } const sudoRun = registerSudoRun(ws, sessionId, 'opencode'); - Object.assign(spawnEnv, sudoRun.env); + const spawnEnv = { ...process.env, ...sudoRun.env }; opencodeProcess = spawnFunction('opencode', args, { cwd: workingDir, stdio: ['pipe', 'pipe', 'pipe'], - env: { ...process.env }, + env: spawnEnv, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/opencode-cli.js` around lines 205 - 212, The code references an undeclared spawnEnv and then ignores the merged env when calling spawnFunction; declare and populate a spawnEnv object from process.env, merge in the sudoRun.env returned by registerSudoRun (sudoRun and registerSudoRun are the relevant symbols) using Object.assign or spread, and pass that merged spawnEnv as the env option to spawnFunction when creating opencodeProcess so the sudo askpass variables are injected and no ReferenceError occurs.
🧹 Nitpick comments (2)
server/modules/sudo-askpass/askpass-integration.test.ts (1)
53-77: ⚡ Quick winMove teardown into
finallyto prevent resource leakage on failures.Line 75-76 and Line 93 only run on success paths. If an assertion fails earlier, the HTTP server and sudo-run registration can leak and make later tests flaky.
Suggested fix
test('askpass helper fetches the typed password over the loopback route', async () => { const { server, port } = await startServer(); setServerPort(port); const sent: string[] = []; const ws = { send: (data: string) => sent.push(data) }; - const { token } = registerSudoRun(ws, 'sess-int', 'claude'); - const { askpassPath } = ensureAskpassFiles(); - - const helper = runHelper(askpassPath, '[sudo] password for piotr: ', { - ...process.env, - AIGM_ASKPASS_TOKEN: token, - AIGM_ASKPASS_PORT: String(port), - }); - - await waitFor(() => sent.length > 0); - const requestId = JSON.parse(sent[0]).requestId; - resolveSudoPassword(requestId, { password: 'hunter2' }); - - assert.equal(await helper.code(), 0); - assert.equal(helper.stdout(), 'hunter2'); - - unregisterSudoRun(token); - await new Promise((r) => server.close(r)); + const { token } = registerSudoRun(ws, 'sess-int', 'claude'); + try { + const { askpassPath } = ensureAskpassFiles(); + const helper = runHelper(askpassPath, '[sudo] password for piotr: ', { + ...process.env, + AIGM_ASKPASS_TOKEN: token, + AIGM_ASKPASS_PORT: String(port), + }); + + await waitFor(() => sent.length > 0); + const requestId = JSON.parse(sent[0]).requestId; + resolveSudoPassword(requestId, { password: 'hunter2' }); + + assert.equal(await helper.code(), 0); + assert.equal(helper.stdout(), 'hunter2'); + } finally { + unregisterSudoRun(token); + await new Promise((r) => server.close(r)); + } });Also applies to: 79-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/modules/sudo-askpass/askpass-integration.test.ts` around lines 53 - 77, The test currently registers a sudo run and starts an HTTP server but only calls unregisterSudoRun(token) and server.close() on the success path; wrap the main test logic (everything after startServer()/registerSudoRun() and before teardown) in a try/finally block so that unregisterSudoRun(token) and awaiting server.close() always run in the finally clause, ensuring resources started by startServer(), registerSudoRun(), and ensureAskpassFiles()/runHelper() are cleaned up even on assertion failures; reference the existing symbols startServer, setServerPort, registerSudoRun, ensureAskpassFiles, runHelper, resolveSudoPassword, unregisterSudoRun, and server.close when making the change.src/components/shell/view/Shell.tsx (1)
397-445: ⚖️ Poor tradeoffConsider extracting a shared sudo password modal component.
The inline modal (lines 397-445) duplicates significant structure and styling from
SudoPasswordModal.tsx. While the state shapes differ (Shell usessudoPrompt: stringvs. chat usesPendingSudoRequest), the UI layer (~48 lines of JSX) is nearly identical. Extracting a presentational component that accepts{ prompt, password, onChange, onSubmit, onCancel }would reduce duplication and simplify future modal updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/shell/view/Shell.tsx` around lines 397 - 445, Extract the inline modal JSX in Shell.tsx (the block guarded by sudoPrompt && isConnected) into a reusable presentational component (e.g., SudoPasswordPresentational) that accepts props { prompt, password, onChange, onSubmit, onCancel, inputRef } and reuses the same styling/structure as SudoPasswordModal.tsx; then replace the inline block with that component wired to Shell's local state and handlers (sudoPrompt, sudoPassword, setSudoPassword via onChange, submitSudoPassword as onSubmit, cancelSudoPrompt as onCancel, sudoInputRef as inputRef). Ensure the new component is used by the existing SudoPasswordModal.tsx (or imported there) to avoid duplication and keep Shell.tsx focused on state/logic only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/modules/websocket/utils/sudo-prompt.test.ts`:
- Around line 8-29: Implement the actual detection inside isSudoPasswordPrompt
in server/modules/websocket/utils/sudo-prompt.ts: strip ANSI escape sequences,
split the input into lines and consider the last non-empty line, then test that
line against a regex that matches "[sudo] password for <username>:", "[sudo]
password:" and the Polish variant "[sudo] hasło użytkownika <username>:" (use a
case-insensitive/Unicode-aware pattern and allow any username token). Replace
the current stub that always returns false with this logic so the tests (and
wrapped/preceded-output cases) pass.
---
Outside diff comments:
In `@server/opencode-cli.js`:
- Around line 205-212: The code references an undeclared spawnEnv and then
ignores the merged env when calling spawnFunction; declare and populate a
spawnEnv object from process.env, merge in the sudoRun.env returned by
registerSudoRun (sudoRun and registerSudoRun are the relevant symbols) using
Object.assign or spread, and pass that merged spawnEnv as the env option to
spawnFunction when creating opencodeProcess so the sudo askpass variables are
injected and no ReferenceError occurs.
---
Nitpick comments:
In `@server/modules/sudo-askpass/askpass-integration.test.ts`:
- Around line 53-77: The test currently registers a sudo run and starts an HTTP
server but only calls unregisterSudoRun(token) and server.close() on the success
path; wrap the main test logic (everything after startServer()/registerSudoRun()
and before teardown) in a try/finally block so that unregisterSudoRun(token) and
awaiting server.close() always run in the finally clause, ensuring resources
started by startServer(), registerSudoRun(), and
ensureAskpassFiles()/runHelper() are cleaned up even on assertion failures;
reference the existing symbols startServer, setServerPort, registerSudoRun,
ensureAskpassFiles, runHelper, resolveSudoPassword, unregisterSudoRun, and
server.close when making the change.
In `@src/components/shell/view/Shell.tsx`:
- Around line 397-445: Extract the inline modal JSX in Shell.tsx (the block
guarded by sudoPrompt && isConnected) into a reusable presentational component
(e.g., SudoPasswordPresentational) that accepts props { prompt, password,
onChange, onSubmit, onCancel, inputRef } and reuses the same styling/structure
as SudoPasswordModal.tsx; then replace the inline block with that component
wired to Shell's local state and handlers (sudoPrompt, sudoPassword,
setSudoPassword via onChange, submitSudoPassword as onSubmit, cancelSudoPrompt
as onCancel, sudoInputRef as inputRef). Ensure the new component is used by the
existing SudoPasswordModal.tsx (or imported there) to avoid duplication and keep
Shell.tsx focused on state/logic only.
🪄 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: dc4a0465-4e43-4825-ba62-8dfe389ec88e
📒 Files selected for processing (21)
server/claude-sdk.jsserver/cursor-cli.jsserver/gemini-cli.jsserver/index.jsserver/modules/sudo-askpass/askpass-integration.test.tsserver/modules/sudo-askpass/sudo-askpass.service.jsserver/modules/sudo-askpass/sudo-askpass.service.test.tsserver/modules/websocket/services/chat-websocket.service.tsserver/modules/websocket/utils/sudo-prompt.test.tsserver/modules/websocket/utils/sudo-prompt.tsserver/opencode-cli.jsserver/routes/internal-askpass.jsserver/shared/types.tssrc/components/chat/hooks/useSudoPasswordPrompt.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/SudoPasswordModal.tsxsrc/components/shell/utils/sudo.test.tssrc/components/shell/utils/sudo.tssrc/components/shell/view/Shell.tsxsrc/i18n/locales/en/chat.jsonsrc/stores/useSessionStore.ts
Comment on lines
+8
to
+29
| test('detects the default English sudo prompt', () => { | ||
| assert.equal(isSudoPasswordPrompt('[sudo] password for piotr: '), true); | ||
| }); | ||
|
|
||
| test('detects the Polish-locale sudo prompt', () => { | ||
| assert.equal(isSudoPasswordPrompt('[sudo] hasło użytkownika piotr: '), true); | ||
| }); | ||
|
|
||
| test('detects the generic [sudo] password prompt without a username', () => { | ||
| assert.equal(isSudoPasswordPrompt('[sudo] password: '), true); | ||
| }); | ||
|
|
||
| test('detects the prompt when wrapped in ANSI colour escapes', () => { | ||
| assert.equal(isSudoPasswordPrompt('\x1b[0m[sudo] password for piotr: \x1b[0m'), true); | ||
| }); | ||
|
|
||
| test('detects the prompt when it is the last line after earlier output', () => { | ||
| assert.equal( | ||
| isSudoPasswordPrompt('Reading package lists...\r\n[sudo] password for piotr: '), | ||
| true | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
Positive prompt-detection assertions are incompatible with the current implementation stub.
These expectations cannot pass right now: server/modules/websocket/utils/sudo-prompt.ts (Line 1-10) still returns false for all input. Please ship the detector implementation in the same PR (or mark these as pending) so CI stays green.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/modules/websocket/utils/sudo-prompt.test.ts` around lines 8 - 29,
Implement the actual detection inside isSudoPasswordPrompt in
server/modules/websocket/utils/sudo-prompt.ts: strip ANSI escape sequences,
split the input into lines and consider the last non-empty line, then test that
line against a regex that matches "[sudo] password for <username>:", "[sudo]
password:" and the Polish variant "[sudo] hasło użytkownika <username>:" (use a
case-insensitive/Unicode-aware pattern and allow any username token). Replace
the current stub that always returns false with this logic so the tests (and
wrapped/preceded-output cases) pass.
Object.assign(spawnEnv, ...) referenced an undeclared variable causing
a ReferenceError at runtime. Also, sudoRun.env was never passed to the
spawned process. Replace with const spawnEnv = { ...process.env, ...sudoRun.env }
and pass it as the env option.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
|
Hey @szmidtpiotr, thanks for the PR. I understand the feature and how it can be useful to some users, but this makes things overly complicated for most users. So, I'm closing this. If we get enough requests for this, we will circle back on it. |
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.
Small chunk from #801 (splitting #816 per maintainer request).
What
Agent/CLI
sudocommands that need a password now prompt the user through the chat UI instead of hanging. A per-run token registers a loopback-only askpass bridge; the password is requested over the chat WebSocket and handed back to the waiting sudo process viaSUDO_ASKPASS.server/modules/sudo-askpass/— per-run token registry + loopback askpass HTTP routeSUDO_ASKPASSaround each invocationsudo-password-response; newsudo_password_request/sudo_password_cancelledmessage kindsSudoPasswordModal+useSudoPasswordPrompthook; Shell sudo-prompt detection utilVerification
npm run typecheckpasses (client + server) against currentmain(v1.33.2).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes