feat: Windows Console API approval prompts (Phase 3) - #11
Merged
Conversation
Replace Phase 1 errNonInteractive stubs with full Windows Console API implementation. Interactive approval prompts now work on Windows via CONIN$/CONOUT$ handles with GetConsoleMode/SetConsoleMode raw mode, WaitForSingleObject keystroke polling, and FlushConsoleInputBuffer anti-spoofing. Also implements doctor diagnostics and terminal width detection for Windows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Review Summary by QodoImplement Windows Console API approval prompts with anti-spoofing and diagnostics
WalkthroughsDescription• Replace Windows Console API stubs with full interactive approval prompts - Open CONIN$/CONOUT$ directly for anti-spoofing - Raw mode via GetConsoleMode/SetConsoleMode, keystroke polling via WaitForSingleObject - Support scope selection (once, command, session, forever) • Extract getContextVars() to shared code for cross-platform reuse • Implement real doctor diagnostics and terminal width detection on Windows • Delete Phase 1 scaffolding files (ioctl_windows.go, doctor_termios_windows.go) Diagramflowchart LR
A["Phase 1 Stubs<br/>errNonInteractive"] -->|Replace| B["Windows Console API<br/>CONIN$/CONOUT$"]
B -->|Raw Mode| C["GetConsoleMode<br/>SetConsoleMode"]
B -->|Input Polling| D["WaitForSingleObject<br/>Keystroke Read"]
B -->|Anti-Spoofing| E["FlushConsoleInputBuffer"]
F["getContextVars<br/>Unix-only"] -->|Extract| G["Shared Code<br/>prompt_shared.go"]
G -->|Used by| B
H["Doctor Checks<br/>SKIP"] -->|Implement| I["Live TTY Access<br/>Raw Mode PASS"]
J["Terminal Width<br/>Default 80"] -->|Implement| K["GetConsoleScreenBufferInfo<br/>Real Width"]
File Changes1. internal/approve/prompt_windows.go
|
Code Review by Qodo
1. Windows PromptUser shows approval prompt
|
|
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 (3)
WalkthroughRemoved Windows ioctl stubs; added a shared env-var collector and tests; stopped displaying context vars in the Unix TTY prompt and made cancellation return an error; implemented a full interactive Windows console prompt with tests; improved Windows terminal detection and width logic. Changes
🚥 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements interactive approval prompts for Windows by utilizing the Windows Console API (CONIN$/CONOUT$). It includes logic for raw mode handling, signal processing, and ANSI color support with a plain text fallback. Additionally, it migrates shared environment variable logic to a common file and updates the 'doctor' command to verify console access on Windows. A redundant environment variable check was identified in the console opening logic.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
+ Coverage 71.17% 71.27% +0.09%
==========================================
Files 73 73
Lines 8726 8728 +2
==========================================
+ Hits 6211 6221 +10
+ Misses 2022 2011 -11
- Partials 493 496 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/approve/prompt_test.go (1)
47-59: Optional: Simplify cleanup logic.The current approach uses
t.Setenv,os.Unsetenv, and a manualt.Cleanuptogether. SincegetContextVars()checksval != "", callingt.Setenv(v, "")alone is sufficient — it sets the var to empty (whichgetContextVarstreats as "not set") and automatically restores the original value after the test.♻️ Simplified version
func TestGetContextVars_Empty(t *testing.T) { // With no relevant env vars set, should return empty string. - // Save and clear any that might be set. vars := []string{ "AWS_PROFILE", "AWS_REGION", "AWS_DEFAULT_REGION", "TF_WORKSPACE", "TF_VAR_environment", "KUBECONFIG", "KUBECONTEXT", "GCP_PROJECT", "GOOGLE_CLOUD_PROJECT", "AZURE_SUBSCRIPTION", } - saved := make(map[string]string) for _, v := range vars { - if val, ok := os.LookupEnv(v); ok { - saved[v] = val - t.Setenv(v, "") - os.Unsetenv(v) - } + t.Setenv(v, "") // t.Setenv handles save/restore automatically } - t.Cleanup(func() { - for k, v := range saved { - os.Setenv(k, v) - } - }) got := getContextVars() if got != "" { t.Errorf("expected empty string, got %q", got) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/approve/prompt_test.go` around lines 47 - 59, Replace the manual save/restore and os.Unsetenv calls with just t.Setenv(v, "") in the loop because getContextVars treats empty string as unset; remove the saved map and t.Cleanup block. Specifically, inside the loop over vars replace the os.LookupEnv/save/unset logic with a single t.Setenv(v, "") call and delete the saved map and t.Cleanup closure that restores env vars.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/approve/prompt_test.go`:
- Around line 67-73: The tests TestGetContextVars_SingleVar and
TestGetContextVars_MultipleVars are flaky because they don't clear other tracked
environment variables before asserting; modify these tests to unset or reset all
tracked vars used by getContextVars (e.g., AWS_PROFILE, AWS_REGION,
TF_WORKSPACE, KUBECONFIG, etc.) at the start of each test (use t.Setenv(var, "")
or os.Unsetenv) so the environment is deterministic, and to avoid duplication
extract the tracked vars slice into a package-level variable like
contextVarsForTest and have getContextVars (or the tests) reference that shared
slice so TestGetContextVars_Empty can also reuse it.
In `@internal/approve/prompt_windows.go`:
- Around line 69-77: The current rawMode calculation clears ENABLE_MOUSE_INPUT
but leaves Quick Edit enabled; update the mode setup in the block around
rawMode/origMode and the call to windows.SetConsoleMode(inHandle, rawMode) to
also set windows.ENABLE_EXTENDED_FLAGS and explicitly clear
windows.ENABLE_QUICK_EDIT_MODE (i.e., include windows.ENABLE_EXTENDED_FLAGS in
the bits you set and &^ clear windows.ENABLE_QUICK_EDIT_MODE from origMode when
building rawMode) so Quick Edit is disabled before calling SetConsoleMode.
- Around line 142-145: The loops calling windows.WaitForSingleObject currently
ignore its returned error and treat any non-WAIT_OBJECT_0 as a timeout; change
both call sites to check the error value and surface real API failures instead
of retrying: after calling windows.WaitForSingleObject(inHandle, 100) (and the
other analogous call), if err != nil return or propagate an error (with context
like "WaitForSingleObject failed for inHandle") rather than continue looping,
and only treat non-WAIT_OBJECT_0 as a benign timeout when err == nil; update the
surrounding functions that call WaitForSingleObject to return/propagate the
error accordingly so the approval prompt does not spin on real API failures.
In `@internal/cli/doctor_live_windows.go`:
- Around line 84-89: The checkLiveForegroundProcessGroup function currently
returns an out-of-band "SKIP" status; change it to use the documented doctor
contract by returning status "WARN" (or "PASS"/"FAIL" as appropriate) so it
aligns with other Windows live-console checks; update the returned checkResult
in checkLiveForegroundProcessGroup (and keep the name
checkNameLiveForegroundHandoff) to set status: "WARN" and retain or slightly
adjust the detail to "Windows job object support not yet implemented (planned:
Phase 4)".
In `@internal/cli/help_width_windows.go`:
- Around line 23-25: isTerminal() currently only checks GetConsoleMode and
therefore may return true even when Windows VT processing is not enabled; update
the logic so help colorization is safe by enabling VT mode (or detecting it
explicitly) before returning true. Specifically, in isTerminal(fd int) call
windows.GetConsoleMode to get outMode, then attempt to set
windows.SetConsoleMode(outHandle,
outMode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING); if SetConsoleMode succeeds
return true, otherwise fall back to false (or keep original console mode and
return false); ensure this change integrates with shouldColorize() so ANSI
colors are only emitted when VT processing is enabled.
---
Nitpick comments:
In `@internal/approve/prompt_test.go`:
- Around line 47-59: Replace the manual save/restore and os.Unsetenv calls with
just t.Setenv(v, "") in the loop because getContextVars treats empty string as
unset; remove the saved map and t.Cleanup block. Specifically, inside the loop
over vars replace the os.LookupEnv/save/unset logic with a single t.Setenv(v,
"") call and delete the saved map and t.Cleanup closure that restores env vars.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13e6d5f0-2d53-4e6a-bada-62542baaf802
📒 Files selected for processing (9)
internal/approve/ioctl_windows.gointernal/approve/prompt_shared.gointernal/approve/prompt_test.gointernal/approve/prompt_unix.gointernal/approve/prompt_windows.gointernal/approve/prompt_windows_test.gointernal/cli/doctor_live_windows.gointernal/cli/doctor_termios_windows.gointernal/cli/help_width_windows.go
💤 Files with no reviewable changes (3)
- internal/approve/prompt_unix.go
- internal/cli/doctor_termios_windows.go
- internal/approve/ioctl_windows.go
- Restore CONOUT$ console mode after approval prompt (VT processing leak) - Fix ctx cancellation returning nil error causing RequestApproval hang (pre-existing bug, fixed on both Unix and Windows) - Handle WaitForSingleObject WAIT_FAILED with early error return - Fix flaky TestGetContextVars_SingleVar (clear all tracked env vars) - Simplify TestGetContextVars_Empty cleanup (use only t.Setenv) - Strengthen TestRenderPromptPlain to verify rendered content - Make errNonInteractive message platform-neutral (console unavailable) - Update stale Phase 3 skip comment in doctor_test.go Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/approve/prompt_windows.go (1)
75-83:⚠️ Potential issue | 🟠 MajorDisable Quick Edit mode to prevent mouse-triggered selection freeze.
Clearing
ENABLE_MOUSE_INPUTalone leaves Quick Edit mode enabled. A stray mouse click can put the console into selection mode and freeze the approval prompt. The Windows API requires settingENABLE_EXTENDED_FLAGSwhen modifying Quick Edit mode.Suggested fix
- rawMode := origMode &^ (windows.ENABLE_LINE_INPUT | + rawMode := (origMode | windows.ENABLE_EXTENDED_FLAGS) &^ (windows.ENABLE_LINE_INPUT | windows.ENABLE_ECHO_INPUT | windows.ENABLE_PROCESSED_INPUT | windows.ENABLE_MOUSE_INPUT | + windows.ENABLE_QUICK_EDIT_MODE | windows.ENABLE_WINDOW_INPUT)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/approve/prompt_windows.go` around lines 75 - 83, The current raw-mode calculation clears ENABLE_MOUSE_INPUT but leaves Quick Edit mode enabled, which can cause the console to enter selection mode on mouse clicks; update the code that computes rawMode (using origMode and the rawMode variable passed to windows.SetConsoleMode) to first set ENABLE_EXTENDED_FLAGS on the mode and then clear ENABLE_QUICK_EDIT_MODE along with ENABLE_MOUSE_INPUT (and the other flags: ENABLE_LINE_INPUT, ENABLE_ECHO_INPUT, ENABLE_PROCESSED_INPUT, ENABLE_WINDOW_INPUT) before calling windows.SetConsoleMode; ensure the mode you pass includes WINDOWS.ENABLE_EXTENDED_FLAGS so Quick Edit is properly disabled.
🧹 Nitpick comments (1)
internal/approve/prompt_windows.go (1)
282-282: Consider handlingos.Getwd()errors for security context.Both
renderPromptANSI(line 282) andrenderPromptPlain(line 304) ignore the error fromos.Getwd(). If the working directory is deleted or inaccessible, the prompt silently omits this context. For a security-sensitive approval prompt, displaying a placeholder is preferable to silent omission.Suggested approach
- cwd, _ := os.Getwd() + cwd, err := os.Getwd() + if err != nil { + cwd = "(unavailable)" + }Also applies to: 304-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/approve/prompt_windows.go` at line 282, renderPromptANSI and renderPromptPlain currently ignore errors from os.Getwd() (cwd, _ := os.Getwd()), which can silently omit working-directory context; update both functions to check the error returned by os.Getwd(), and when it fails set cwd to a clear placeholder (e.g., "<unknown cwd>" or similar) so the prompt shows an explicit fallback instead of omitting the value; reference the cwd variable and the functions renderPromptANSI and renderPromptPlain when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/approve/prompt_windows.go`:
- Around line 221-225: In readScope, handle windows.WaitForSingleObject failures
the same way as readApprovalDecision: check if event == windows.WAIT_FAILED and
return or propagate the underlying error instead of continuing the loop; update
the code around the call to windows.WaitForSingleObject(inHandle, 100) to detect
WAIT_FAILED, retrieve the last error (e.g., syscall.GetLastError or the
equivalent used elsewhere), and return that error so scope selection surfaces
API failures instead of silently spinning.
---
Duplicate comments:
In `@internal/approve/prompt_windows.go`:
- Around line 75-83: The current raw-mode calculation clears ENABLE_MOUSE_INPUT
but leaves Quick Edit mode enabled, which can cause the console to enter
selection mode on mouse clicks; update the code that computes rawMode (using
origMode and the rawMode variable passed to windows.SetConsoleMode) to first set
ENABLE_EXTENDED_FLAGS on the mode and then clear ENABLE_QUICK_EDIT_MODE along
with ENABLE_MOUSE_INPUT (and the other flags: ENABLE_LINE_INPUT,
ENABLE_ECHO_INPUT, ENABLE_PROCESSED_INPUT, ENABLE_WINDOW_INPUT) before calling
windows.SetConsoleMode; ensure the mode you pass includes
WINDOWS.ENABLE_EXTENDED_FLAGS so Quick Edit is properly disabled.
---
Nitpick comments:
In `@internal/approve/prompt_windows.go`:
- Line 282: renderPromptANSI and renderPromptPlain currently ignore errors from
os.Getwd() (cwd, _ := os.Getwd()), which can silently omit working-directory
context; update both functions to check the error returned by os.Getwd(), and
when it fails set cwd to a clear placeholder (e.g., "<unknown cwd>" or similar)
so the prompt shows an explicit fallback instead of omitting the value;
reference the cwd variable and the functions renderPromptANSI and
renderPromptPlain when making this change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f72677bc-31eb-42be-bd55-84519ab1ccd3
📒 Files selected for processing (6)
internal/approve/prompt_shared.gointernal/approve/prompt_test.gointernal/approve/prompt_unix.gointernal/approve/prompt_windows.gointernal/approve/prompt_windows_test.gointernal/cli/doctor_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/approve/prompt_test.go
…efer) - Handle WAIT_FAILED in readScope (was missed by initial fix, caught by CodeRabbit second-round review) - Add defer f.Close() in TestRenderPromptPlain to prevent fd leak on panic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/approve/prompt_windows.go (1)
221-228: CaptureWaitForSingleObjecterror details and use Windows constants for consistency.Line 222 discards the error from
WaitForSingleObject. While the code fails closed, capturing and logging the error improves diagnostics when console handles are invalid—consistent with the existing pattern at line 232 (slog.Debug("console read failed...")). Additionally, replace the magic number0xFFFFFFFFwith thewindows.WAIT_FAILEDconstant for consistency with the nearbywindows.WAIT_OBJECT_0usage, and explicitly checkwindows.WAIT_TIMEOUT(258) for clarity.Proposed refactor
- event, _ := windows.WaitForSingleObject(inHandle, 100) - if event == 0xFFFFFFFF { // WAIT_FAILED — console handle invalid - return "", true // deny on failure - } - if event != windows.WAIT_OBJECT_0 { - continue - } + event, waitErr := windows.WaitForSingleObject(inHandle, 100) + if event == windows.WAIT_FAILED { + slog.Debug("wait for console input failed while selecting approval scope", "error", waitErr) + return "", true // deny on failure + } + if event == windows.WAIT_TIMEOUT { + continue + } + if event != windows.WAIT_OBJECT_0 { + slog.Debug("unexpected wait result while selecting approval scope", "event", event) + return "", true + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/approve/prompt_windows.go` around lines 221 - 228, Capture and handle the error returned by windows.WaitForSingleObject (called with inHandle) instead of discarding it: replace the magic 0xFFFFFFFF with windows.WAIT_FAILED, explicitly check for windows.WAIT_TIMEOUT vs windows.WAIT_OBJECT_0, and when WaitForSingleObject returns WAIT_FAILED or a non-WAIT_OBJECT_0 result log the error details via slog.Debug (similar to the existing pattern around console read) while preserving the current closed-fail behavior (return "", true on failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/approve/prompt_windows.go`:
- Around line 221-228: Capture and handle the error returned by
windows.WaitForSingleObject (called with inHandle) instead of discarding it:
replace the magic 0xFFFFFFFF with windows.WAIT_FAILED, explicitly check for
windows.WAIT_TIMEOUT vs windows.WAIT_OBJECT_0, and when WaitForSingleObject
returns WAIT_FAILED or a non-WAIT_OBJECT_0 result log the error details via
slog.Debug (similar to the existing pattern around console read) while
preserving the current closed-fail behavior (return "", true on failure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92db0bcc-ac5f-4caf-8205-054cb0054642
📒 Files selected for processing (2)
internal/approve/prompt_windows.gointernal/approve/prompt_windows_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/approve/prompt_windows_test.go
shouldColorize() now calls supportsANSI() which probes whether the console supports ENABLE_VIRTUAL_TERMINAL_PROCESSING. On legacy conhost (pre-Windows 10 1511), the probe fails and help output renders as plain text instead of raw escape sequences. Unix returns true unconditionally since all modern terminals support ANSI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
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
1 participant
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
errNonInteractivestubs with full Windows Console API implementation for interactive approval promptsCONIN$/CONOUT$directly (not stdin/stdout) to preserve anti-spoofing propertiesGetConsoleMode/SetConsoleMode, keystroke polling viaWaitForSingleObject, buffer flush viaFlushConsoleInputBufferfuse doctorconsole checks and terminal width detection on Windowsioctl_windows.go,doctor_termios_windows.go)getContextVars()to shared code for cross-platform useFiles changed (8)
internal/approve/prompt_windows.gointernal/approve/prompt_windows_test.gointernal/approve/prompt_shared.gogetContextVars()(extracted from unix-only file)internal/approve/prompt_unix.gogetContextVars()(moved to shared)internal/approve/prompt_test.goTestGetContextVars_*testsinternal/cli/doctor_live_windows.gointernal/cli/help_width_windows.goGetConsoleScreenBufferInfowidth detectioninternal/approve/ioctl_windows.gointernal/cli/doctor_termios_windows.goTest plan
GOOS=windows go build ./...— cross-compile passesGOOS=windows go vet ./...— vet passesgo test ./...— all 15 packages pass (no Unix regression)fuse run "echo hello"shows approval promptecho "a" | fuse run "echo hello"does NOT auto-approvefuse doctor --securityshows PASS for console checksSpecs:
.agents/plans/2026-03-28-windows-terminal-approval.mdPre-mortem:
.agents/council/2026-03-28-pre-mortem-windows-terminal-approval.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Chores