feat: Windows Console API approval prompts + Job Object process management (Phases 3-4) - #12
Conversation
This comment has been minimized.
This comment has been minimized.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements Windows terminal approval prompting and process containment using Windows Job Objects, refactors process lifecycle and MCP proxy cleanup with platform-specific helpers, tightens approval/signal semantics, enhances Windows console/ANSI detection, and adds CI linting and many documentation tickets capturing the work (48 ticket files). 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 |
Review Summary by QodoWindows Console API approval prompts and Job Object process management (Phases 3-4)
WalkthroughsDescription• Implement Windows Console API approval prompts with raw mode and keystroke polling • Replace Unix process groups with Windows Job Objects for child process containment • Forward console Ctrl events to child process trees on interrupt • Add platform-specific MCP proxy cleanup with job object wrapping • Fix context cancellation bug in approval flow (returns nil error on shutdown) • Promote job object containment failures from debug to warn logging • Replace bytes.Buffer with strings.Builder in Windows executor • Add ANSI support detection and terminal width detection for Windows Diagramflowchart LR
A["Unix TTY prompts<br/>Process groups"] -->|Phase 3| B["Windows Console API<br/>CONIN$/CONOUT$"]
A -->|Phase 4| C["Job Objects<br/>Process tree management"]
B --> D["Interactive approval<br/>Raw mode + keystroke polling"]
C --> E["Child containment<br/>Auto-cleanup on exit"]
C --> F["Signal forwarding<br/>GenerateConsoleCtrlEvent"]
E --> G["fuse run<br/>fuse proxy"]
F --> G
D --> G
File Changes1. internal/adapters/job_windows.go
|
Code Review by Qodo
1. Windows APPROVAL not blocked
|
![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 Phase 3 and Phase 4 of the Windows support plan, introducing native console approval prompts and robust process tree management using Windows Job Objects. Key additions include a jobObject wrapper for process containment, signal forwarding via GenerateConsoleCtrlEvent, and a TUI-based approval system for Windows. The changes also improve diagnostic checks in fuse doctor and ensure consistent cleanup of downstream processes. Feedback was provided regarding the Windows console's ANSI support detection, noting that restoring the original console mode immediately after probing would prevent subsequent help output from rendering colors correctly.
| if err := windows.SetConsoleMode(conOut, mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING); err != nil { | ||
| return false | ||
| } | ||
| _ = windows.SetConsoleMode(conOut, mode) // restore original |
There was a problem hiding this comment.
On Windows, ANSI color support (VT processing) must be explicitly enabled for the console handle via SetConsoleMode. By restoring the original mode immediately after probing, subsequent help output containing ANSI escape sequences will be rendered as raw text (garbage) in the terminal. To ensure colors are displayed correctly, you should leave ENABLE_VIRTUAL_TERMINAL_PROCESSING enabled if the probe succeeds.
| _ = windows.SetConsoleMode(conOut, mode) // restore original | |
| return true |
There was a problem hiding this comment.
Fixed in 2e15d0a. supportsANSI() now leaves VT enabled after a successful probe and caches the result via sync.Once.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| ## Problem | ||
|
|
||
| 1. **unsafe.Pointer (job_windows.go:39):** `unsafe.Pointer` is used for `SetInformationJobObject` struct parameter. No `// SAFETY:` comment explains why this is sound (struct is stack-allocated, pointer valid for syscall duration). Given the project's `#nosec` budget of 0, future reviewers need documented invariants. |
There was a problem hiding this comment.
2. #nosec text added 📘 Rule violation ⛨ Security
A new markdown ticket file contains the literal substring #nosec, which violates the rule disallowing any #nosec occurrences in the repository. Keeping this text can normalize/encourage inline security suppressions and breaks the mandated zero-#nosec policy.
Agent Prompt
## Issue description
A new markdown file contains the literal substring `#nosec`, which violates the repository policy of zero `#nosec` occurrences.
## Issue Context
The compliance rule treats any `#nosec` occurrence (even in docs/tickets) as a violation.
## Fix Focus Areas
- .tickets/fus-j7ta.md[19-19]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
False positive. The #nosec text appears in a ticket description about the suppression budget policy, not as a code suppression directive. The budget check (grep -rn '#nosec' internal/) correctly scopes to Go source files — markdown ticket files are not counted.
| return 0, nil | ||
| } | ||
|
|
||
| if runtime.GOOS == "windows" { |
There was a problem hiding this comment.
1. Windows approval not blocked 📘 Rule violation ⛨ Security
The PR removes the runtime.GOOS == "windows" block in handleApprovalCommand, allowing Windows builds to reach APPROVAL prompting/execution paths. This violates the requirement that Windows builds must deterministically fail fast (no approval prompt or APPROVAL command side effects).
Agent Prompt
## Issue description
Windows builds are allowed to proceed into the APPROVAL flow because the Windows guard in `handleApprovalCommand()` was removed.
## Issue Context
Compliance rule 224206 requires that, on Windows, APPROVAL commands must be blocked/short-circuited with a deterministic error (no approval prompt and no approval-related side effects).
## Fix Focus Areas
- internal/adapters/runner.go[132-142]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
This removal is intentional. Phase 3 (commit 0da2a06) implemented the Windows Console API approval prompt — prompt_windows.go has 324 lines of real CONIN$/CONOUT$ code. The gate was a Phase 2 temporary stub. Removing it is the Phase 3 completion step (ticket fus-k8ub). The codex-shell path never had this gate and already approves on Windows.
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| } | ||
| defer job.close() | ||
|
|
||
| cmd := buildWindowsCommand(ctx, command) |
There was a problem hiding this comment.
Shell injection vulnerability in executeCapturedShellCommandWithStdin due to passing a raw command string to the shell.
This issue appears in multiple locations:
- internal/adapters/runner_exec_windows.go: Lines 86-86
- internal/adapters/runner_exec_windows.go: Lines 39-39
- internal/cli/doctor_live_windows.go: Lines 105-105
Please fix this Kody Rule violation in all listed locations.
// import "mvdan.cc/sh/v3/shell"
fields, err := shell.Fields(command, nil)
if err != nil {
return commandExecution{ExitCode: -1}, fmt.Errorf("parse command: %w", err)
}
if len(fields) == 0 {
return commandExecution{ExitCode: -1}, fmt.Errorf("empty command")
}
cmd := exec.CommandContext(ctx, fields[0], fields[1:]...)Prompt for LLM
File internal/adapters/runner_exec_windows.go:
Line 86:
I have a Go function `executeCapturedShellCommandWithStdin` that executes a shell command and captures its output. It currently takes a single string for the command and passes it to a helper function `buildWindowsCommand`. This helper function then executes the command using a shell (like `cmd.exe /C` or `sh -c`), which creates a shell injection vulnerability. I have a rule that states: 'When executing system commands, do not construct command strings using string concatenation with user input. Use argument lists or safe parsing to avoid command injection.' How can I refactor this function to safely execute the command by parsing the command string into a program and its arguments, thus avoiding the shell and mitigating the injection risk? The function signature is `func executeCapturedShellCommandWithStdin(ctx context.Context, command, cwd string, stdin io.Reader, timeout time.Duration) (commandExecution, error)`.
Suggested Code:
// import "mvdan.cc/sh/v3/shell"
fields, err := shell.Fields(command, nil)
if err != nil {
return commandExecution{ExitCode: -1}, fmt.Errorf("parse command: %w", err)
}
if len(fields) == 0 {
return commandExecution{ExitCode: -1}, fmt.Errorf("empty command")
}
cmd := exec.CommandContext(ctx, fields[0], fields[1:]...)
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
This is the intended design. Fuse is a command firewall — commands are classified through the full policy pipeline before reaching the executor. The raw command string is passed to powershell.exe -Command or cmd.exe /c by design, matching the Unix path which passes to /bin/sh -c. Parsing with shell.Fields() would break the classification pipeline's ability to execute the command as the agent intended it.
| if runtime.GOOS == "windows" { | ||
| t.Skip("terminal capability checks not yet supported on Windows (planned: Phase 3)") | ||
| // Windows console checks require a real console (CONIN$). | ||
| // CI runners typically don't have one — skip if unavailable. | ||
| t.Skip("Windows terminal checks require interactive console (not available in CI)") | ||
| } |
There was a problem hiding this comment.
Unconditional test skip leaves new feature untested. The test TestRunDoctorLive_ReportsTerminalCapabilityChecks is now unconditionally skipped on Windows, which prevents the newly implemented Windows terminal capability checks from being tested. The test should instead detect a CI environment and only skip then, allowing the test to run in local development environments.
if runtime.GOOS == "windows" && os.Getenv("CI") != "" {
// Windows console checks require a real console (CONIN$).
// CI runners typically don't have one — skip if unavailable.
t.Skip("Windows terminal checks require interactive console (not available in CI)")
}Prompt for LLM
File internal/cli/doctor_test.go:
Line 430 to 434:
The Go test `TestRunDoctorLive_ReportsTerminalCapabilityChecks` is being modified. The new code unconditionally skips the test if the operating system is Windows, with a comment explaining that CI runners lack the required interactive console. However, this also prevents the test from running in local Windows development environments where an interactive console is present. This is problematic because the feature being tested (terminal capability checks) was just implemented for Windows in this same pull request. The test should only be skipped in a non-interactive environment (like CI), not on all Windows machines. Propose a fix that allows the test to run locally on Windows but still skip in CI environments by checking for a common CI environment variable.
Suggested Code:
if runtime.GOOS == "windows" && os.Getenv("CI") != "" {
// Windows console checks require a real console (CONIN$).
// CI runners typically don't have one — skip if unavailable.
t.Skip("Windows terminal checks require interactive console (not available in CI)")
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
The skip is correct — CI runners don't have a real console (CONIN$). The skip message was stale though. Fixed in 2e15d0a: updated to "Windows terminal checks require interactive console (not available in CI)". The suggestion to check CI env is reasonable but the console check itself (os.OpenFile("CONIN$")) already serves as the detection mechanism — if it fails, the checks can't run regardless of the CI variable.
![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 (2)
internal/adapters/mcpproxy_cleanup_windows.go (1)
24-26: Add nil check before accessingcmd.Process.Pid.If
proxyChildCleanupis ever called beforecmd.Start()succeeds, accessingcmd.Process.Pidwill panic. While the current caller (mcpproxy.go:84) correctly calls this afterStart(), a defensive check would prevent future misuse.🛡️ Defensive nil check
+ if cmd.Process == nil { + slog.Warn("proxy: process not started, job object not assigned") + return func() { + job.close() + } + } if err := job.assign(cmd.Process.Pid); err != nil { slog.Warn("proxy: job object assign failed, grandchild cleanup not guaranteed", "pid", cmd.Process.Pid, "err", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/mcpproxy_cleanup_windows.go` around lines 24 - 26, The code calls job.assign(cmd.Process.Pid) without ensuring cmd.Process is non-nil which can panic if Start() failed; update proxyChildCleanup (or the block where job.assign is invoked) to first check that cmd.Process != nil before accessing cmd.Process.Pid, and only call job.assign when the PID is available—otherwise skip the assign and emit a clear warning/log (use the same slog logger) indicating the process is nil so cleanup via the job object was not attempted.internal/approve/prompt_shared.go (1)
35-44: Consider usingstrings.Builderfor consistency.The PR description mentions "switching bytes.Buffer to strings.Builder where appropriate." This loop performs string concatenation which, while fine for a small fixed list, could use
strings.Builderfor consistency with that pattern.♻️ Optional refactor using strings.Builder
func getContextVars() string { relevantVars := []string{ "AWS_PROFILE", "AWS_REGION", "AWS_DEFAULT_REGION", "TF_WORKSPACE", "TF_VAR_environment", "KUBECONFIG", "KUBECONTEXT", "GCP_PROJECT", "GOOGLE_CLOUD_PROJECT", "AZURE_SUBSCRIPTION", } - var result string + var b strings.Builder for _, v := range relevantVars { val := os.Getenv(v) if val != "" { - if result != "" { - result += ", " + if b.Len() > 0 { + b.WriteString(", ") } - result += v + "=" + val + b.WriteString(v) + b.WriteByte('=') + b.WriteString(val) } } - return result + return b.String() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/approve/prompt_shared.go` around lines 35 - 44, Replace manual concatenation into the `result` string with a strings.Builder: import "strings", create a `var b strings.Builder`, iterate `for _, v := range relevantVars { val := os.Getenv(v); if val != "" { if b.Len() > 0 { b.WriteString(", ") } b.WriteString(v); b.WriteString("="); b.WriteString(val) } }` and finally set `result = b.String()` so building is consistent with the other refactors; adjust any variable usage accordingly (references: relevantVars, result).
🤖 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 70-81: The test TestGetContextVars_MultipleVars is missing a call
to clearTrackedVars(t) which can allow leftover tracked env vars from the host
to leak into getContextVars() and cause flakiness; fix it by invoking
clearTrackedVars(t) at the start of TestGetContextVars_MultipleVars (before
calling t.Setenv for AWS_PROFILE and KUBECONFIG) so the test runs in a clean
tracked-vars state and only the intended variables appear in the output.
In `@internal/approve/prompt_unix.go`:
- Around line 106-111: The signal-handling branch is inconsistent with the
context-cancellation branch: ctx.Done() returns an error while sigCh returns
nil; change the sigCh case in the approval loop (the branch that prints "Denied
(signal received).") to return a non-nil error consistent with the ctx case
(e.g., fmt.Errorf("approval interrupted: signal received")) so callers can
uniformly detect interruptions; update the return from that case to mirror the
ctx.Done() return signature and message semantics used in this file (the
fmt.Fprintf to tty may be kept).
In `@internal/approve/prompt_windows_test.go`:
- Around line 74-79: The test reads the temp file via os.ReadFile(f.Name())
immediately after renderPromptPlain(f, ...), which can miss buffered writes;
call f.Sync() (or close the file with f.Close()) after renderPromptPlain and
before os.ReadFile to flush buffers so the new reader sees the written data
(apply this change around the renderPromptPlain/f usage in the test).
In `@internal/cli/help_width_windows.go`:
- Around line 31-44: The supportsANSI function currently probes by setting
ENABLE_VIRTUAL_TERMINAL_PROCESSING then immediately restores the original mode,
which prevents ANSI sequences from being interpreted when colored output is
later written; update supportsANSI so that after a successful
SetConsoleMode(conOut, mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) it does
not restore the original mode (i.e., leave the flag enabled), or alternatively
only set the flag when it is not already present by checking
mode&windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING and setting it persistently via
windows.SetConsoleMode; adjust/remove the trailing
windows.SetConsoleMode(conOut, mode) restore call so the console remains in VT
mode when supportsANSI returns true.
---
Nitpick comments:
In `@internal/adapters/mcpproxy_cleanup_windows.go`:
- Around line 24-26: The code calls job.assign(cmd.Process.Pid) without ensuring
cmd.Process is non-nil which can panic if Start() failed; update
proxyChildCleanup (or the block where job.assign is invoked) to first check that
cmd.Process != nil before accessing cmd.Process.Pid, and only call job.assign
when the PID is available—otherwise skip the assign and emit a clear warning/log
(use the same slog logger) indicating the process is nil so cleanup via the job
object was not attempted.
In `@internal/approve/prompt_shared.go`:
- Around line 35-44: Replace manual concatenation into the `result` string with
a strings.Builder: import "strings", create a `var b strings.Builder`, iterate
`for _, v := range relevantVars { val := os.Getenv(v); if val != "" { if b.Len()
> 0 { b.WriteString(", ") } b.WriteString(v); b.WriteString("=");
b.WriteString(val) } }` and finally set `result = b.String()` so building is
consistent with the other refactors; adjust any variable usage accordingly
(references: relevantVars, result).
🪄 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: 46bd1b0b-cd1b-47c1-a496-ed678646fc13
📒 Files selected for processing (31)
.tickets/fus-e3pw.md.tickets/fus-f4qx.md.tickets/fus-g5ry.md.tickets/fus-h6sz.md.tickets/fus-j7ta.md.tickets/fus-k8ub.md.tickets/fus-l9vc.md.tickets/fus-m1wd.md.tickets/fus-n2xe.md.tickets/fus-p3yf.mdinternal/adapters/job_windows.gointernal/adapters/mcpproxy.gointernal/adapters/mcpproxy_cleanup_unix.gointernal/adapters/mcpproxy_cleanup_windows.gointernal/adapters/runner.gointernal/adapters/runner_exec_windows.gointernal/adapters/runner_windows.gointernal/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/doctor_test.gointernal/cli/help.gointernal/cli/help_width_unix.gointernal/cli/help_width_windows.gojustfilespecs/windows-support-plan.md
💤 Files with no reviewable changes (3)
- internal/cli/doctor_termios_windows.go
- internal/adapters/runner.go
- internal/approve/ioctl_windows.go
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.tickets/fus-t4vn.md (1)
24-44:⚠️ Potential issue | 🟡 MinorUpdate the fix snippet to match current implementation.
The snippet still restores console mode, but the shipped behavior leaves VT enabled. Keeping this closed ticket aligned avoids future confusion.
Suggested doc patch
- // Restore original mode (don't leave VT permanently set) - _ = windows.SetConsoleMode(conOut, mode) + // Keep VT enabled so subsequent ANSI writes are interpreted. return true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tickets/fus-t4vn.md around lines 24 - 44, The snippet for supportsANSI() in help_width_windows.go is outdated: it restores the original console mode but the shipped code intentionally leaves VT processing enabled; update the implementation used in shouldColorize to stop restoring the original mode so VT remains enabled. Locate the supportsANSI function and replace the restore step (the call that resets console mode) with no-op so after successfully SetConsoleMode(conOut, mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) the VT flag stays set, and adjust any related comments to reflect that VT is left enabled.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
121-122: Consider aligningGOARCHwith the justfile target.The CI lint step specifies
GOARCH=amd64, but thejust lint-windowstarget (justfile:50-52) omits it:lint-windows: GOOS=windows golangci-lint runThis mismatch means developers on non-amd64 hosts (e.g., Apple Silicon) running
just lint-windowslocally may get different results than CI due to architecture-specific build tags or code paths. Consider updating the justfile to includeGOARCH=amd64for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 121 - 122, The justfile's lint-windows target is missing GOARCH=amd64 which causes a mismatch with the CI step that sets GOARCH=amd64; update the justfile target named "lint-windows" (currently: GOOS=windows golangci-lint run) to also set GOARCH=amd64 so it matches the CI job and yields consistent lint results across environments.internal/approve/prompt_test.go (1)
75-80: Consider asserting exact output for multi-var formatting.Line 76-Line 80 only checks presence. Since
getContextVars()has deterministic order, an exact assertion would better guard delimiter/order regressions.♻️ Optional tightening
func TestGetContextVars_MultipleVars(t *testing.T) { clearTrackedVars(t) t.Setenv("AWS_PROFILE", "staging") t.Setenv("KUBECONFIG", "/home/user/.kube/config") got := getContextVars() - // Both should appear, comma-separated. - if !strings.Contains(got, "AWS_PROFILE=staging") { - t.Errorf("missing AWS_PROFILE in %q", got) - } - if !strings.Contains(got, "KUBECONFIG=/home/user/.kube/config") { - t.Errorf("missing KUBECONFIG in %q", got) - } + want := "AWS_PROFILE=staging, KUBECONFIG=/home/user/.kube/config" + if got != want { + t.Errorf("expected %q, got %q", want, 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 75 - 80, The test currently only checks that the output string (variable got from getContextVars()) contains substrings, which misses delimiter/order regressions; replace the two contains-based assertions with a single exact equality assertion that compares got to the expected comma-separated string (e.g., "AWS_PROFILE=staging,KUBECONFIG=/home/user/.kube/config") so the test for getContextVars() verifies deterministic order and delimiter correctness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.tickets/fus-b2yw.md:
- Around line 16-18: The fenced code block containing the Go test skip call (the
line with t.Skip("terminal capability checks not yet supported on Windows
(planned: Phase 3)")) needs a language identifier; change the opening backticks
from ``` to ```go so the block is marked as Go for syntax highlighting and to
satisfy markdownlint rules.
In `@internal/approve/prompt_windows.go`:
- Around line 94-95: The call to windows.FlushConsoleInputBuffer currently
discards its error, so update the code in prompt_windows.go to check the
returned error from windows.FlushConsoleInputBuffer(inHandle) and return it (or
wrap and return) instead of ignoring it; ensure callers of readApprovalDecision
or the function that performs the prompt propagate/handle that error so stale
console input cannot be consumed as user approval. Include
windows.FlushConsoleInputBuffer and readApprovalDecision in your changes to make
the flush failure a fail-closed error path.
---
Duplicate comments:
In @.tickets/fus-t4vn.md:
- Around line 24-44: The snippet for supportsANSI() in help_width_windows.go is
outdated: it restores the original console mode but the shipped code
intentionally leaves VT processing enabled; update the implementation used in
shouldColorize to stop restoring the original mode so VT remains enabled. Locate
the supportsANSI function and replace the restore step (the call that resets
console mode) with no-op so after successfully SetConsoleMode(conOut,
mode|windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING) the VT flag stays set, and
adjust any related comments to reflect that VT is left enabled.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 121-122: The justfile's lint-windows target is missing
GOARCH=amd64 which causes a mismatch with the CI step that sets GOARCH=amd64;
update the justfile target named "lint-windows" (currently: GOOS=windows
golangci-lint run) to also set GOARCH=amd64 so it matches the CI job and yields
consistent lint results across environments.
In `@internal/approve/prompt_test.go`:
- Around line 75-80: The test currently only checks that the output string
(variable got from getContextVars()) contains substrings, which misses
delimiter/order regressions; replace the two contains-based assertions with a
single exact equality assertion that compares got to the expected
comma-separated string (e.g.,
"AWS_PROFILE=staging,KUBECONFIG=/home/user/.kube/config") so the test for
getContextVars() verifies deterministic order and delimiter correctness.
🪄 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: bb077ebd-5ff7-4d66-b242-ab5889e7ce9d
📒 Files selected for processing (45)
.github/workflows/ci.yml.tickets/fus-0r82.md.tickets/fus-4gzq.md.tickets/fus-556x.md.tickets/fus-b2yw.md.tickets/fus-c7gm.md.tickets/fus-d8fn.md.tickets/fus-fx68.md.tickets/fus-g4vs.md.tickets/fus-h5rz.md.tickets/fus-iviw.md.tickets/fus-izck.md.tickets/fus-j6qd.md.tickets/fus-k3tn.md.tickets/fus-kyal.md.tickets/fus-lzxe.md.tickets/fus-n4d6.md.tickets/fus-n4hd.md.tickets/fus-p3cw.md.tickets/fus-p50r.md.tickets/fus-q8xp.md.tickets/fus-r2kf.md.tickets/fus-r7km.md.tickets/fus-rh1w.md.tickets/fus-t4vn.md.tickets/fus-tssy.md.tickets/fus-tvat.md.tickets/fus-v9mr.md.tickets/fus-w2ht.md.tickets/fus-wrx7.mdintegration_test.gointernal/adapters/codexshell_test.gointernal/adapters/job_windows.gointernal/adapters/mcpproxy.gointernal/adapters/mcpproxy_cleanup_windows.gointernal/adapters/runner_exec_windows.gointernal/adapters/runner_test.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/help_width_windows.gospecs/technical_v1.1.md
✅ Files skipped from review due to trivial changes (26)
- .tickets/fus-lzxe.md
- internal/adapters/codexshell_test.go
- .tickets/fus-izck.md
- .tickets/fus-4gzq.md
- internal/adapters/runner_test.go
- .tickets/fus-c7gm.md
- .tickets/fus-rh1w.md
- .tickets/fus-v9mr.md
- integration_test.go
- .tickets/fus-0r82.md
- .tickets/fus-fx68.md
- .tickets/fus-tssy.md
- .tickets/fus-iviw.md
- .tickets/fus-h5rz.md
- .tickets/fus-q8xp.md
- .tickets/fus-kyal.md
- .tickets/fus-j6qd.md
- .tickets/fus-w2ht.md
- .tickets/fus-556x.md
- .tickets/fus-k3tn.md
- .tickets/fus-r7km.md
- .tickets/fus-wrx7.md
- .tickets/fus-d8fn.md
- internal/approve/prompt_windows_test.go
- .tickets/fus-tvat.md
- internal/cli/doctor_live_windows.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/approve/prompt_shared.go
- internal/adapters/mcpproxy_cleanup_windows.go
- internal/adapters/job_windows.go
- internal/adapters/runner_exec_windows.go
| ``` | ||
| t.Skip("terminal capability checks not yet supported on Windows (planned: Phase 3)") | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to the fenced code block.
The code block should specify go as the language for proper syntax highlighting and to satisfy the markdownlint rule.
📝 Proposed fix
-```
+```go
t.Skip("terminal capability checks not yet supported on Windows (planned: Phase 3)")</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tickets/fus-b2yw.md around lines 16 - 18, The fenced code block containing
the Go test skip call (the line with t.Skip("terminal capability checks not yet
supported on Windows (planned: Phase 3)")) needs a language identifier; change
the opening backticks from ``` to ```go so the block is marked as Go for syntax
highlighting and to satisfy markdownlint rules.
| // Flush any stale input before rendering the prompt. | ||
| _ = windows.FlushConsoleInputBuffer(inHandle) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'FlushConsoleInputBuffer|_ = windows\.FlushConsoleInputBuffer' internal/approve/prompt_windows.goRepository: php-workx/fuse
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Get the function containing line 95 to understand context and intent
sed -n '60,120p' internal/approve/prompt_windows.go | cat -nRepository: php-workx/fuse
Length of output: 2490
🏁 Script executed:
#!/bin/bash
# Find all callsites of the function that contains line 95 (likely renderPrompt or similar)
rg -n "renderPrompt" internal/approve/prompt_windows.go | head -20Repository: php-workx/fuse
Length of output: 621
🏁 Script executed:
#!/bin/bash
# Check the function signature and return type around line 94-95
ast-grep --pattern 'func $_($$$) $$$: {
$$$
_ = windows.FlushConsoleInputBuffer($_)
$$$
}'Repository: php-workx/fuse
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find and read the readApprovalDecision function
rg -n "func readApprovalDecision" internal/approve/prompt_windows.goRepository: php-workx/fuse
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Get the readApprovalDecision function implementation
sed -n '130,200p' internal/approve/prompt_windows.go | cat -nRepository: php-workx/fuse
Length of output: 2716
🏁 Script executed:
#!/bin/bash
# Check how readApprovalDecision reads input - does it validate/clear stale input?
ast-grep --pattern 'func readApprovalDecision($$$) {
$$$
}'Repository: php-workx/fuse
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there's any input flushing or validation in openConsole or before the prompt function
sed -n '50,130p' internal/approve/prompt_windows.go | cat -nRepository: php-workx/fuse
Length of output: 3283
🏁 Script executed:
#!/bin/bash
# Check if there are other flush calls or error patterns in the file
rg -n "FlushConsoleInputBuffer|SetConsoleMode" internal/approve/prompt_windows.goRepository: php-workx/fuse
Length of output: 513
Check error from windows.FlushConsoleInputBuffer before proceeding with interactive input.
On Line 95, the error from windows.FlushConsoleInputBuffer is discarded. If the flush fails, stale queued input remains in the console buffer and will be consumed by readApprovalDecision as fresh user input, potentially triggering approval via a stale keystroke instead of deliberate user intent. This is a security gap that should fail closed (return error) rather than silently continue.
The proposed fix is correct:
🔒 Fail-safe fix
// Flush any stale input before rendering the prompt.
- _ = windows.FlushConsoleInputBuffer(inHandle)
+ if err := windows.FlushConsoleInputBuffer(inHandle); err != nil {
+ return false, "", fmt.Errorf("flush console input buffer: %w", err)
+ }🤖 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 94 - 95, The call to
windows.FlushConsoleInputBuffer currently discards its error, so update the code
in prompt_windows.go to check the returned error from
windows.FlushConsoleInputBuffer(inHandle) and return it (or wrap and return)
instead of ignoring it; ensure callers of readApprovalDecision or the function
that performs the prompt propagate/handle that error so stale console input
cannot be consumed as user approval. Include windows.FlushConsoleInputBuffer and
readApprovalDecision in your changes to make the flush failure a fail-closed
error path.
This comment has been minimized.
This comment has been minimized.
- 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>
…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>
Replace Unix process groups with Windows Job Objects for child process lifecycle management. JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE ensures all children die when fuse exits (replaces Pdeathsig). CREATE_NEW_PROCESS_GROUP + GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT) forwards Ctrl+C to the child tree (replaces Kill(-pid, sig)). cmd.Cancel uses TerminateJobObject for timeout kills. Also addresses code review findings: promote containment failures to slog.Warn, add SECURITY/SAFETY comments, replace doctor probe with non-interactive-safe ping, align bytes.Buffer to strings.Builder, remove Phase 3 leftover APPROVAL gate, add job object wrapping to mcpproxy, and add just lint-windows target. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
P1 bugs: - Fix supportsANSI() to leave VT enabled after probe — ANSI garbage on older Windows conhost (cached via sync.Once) - Fix readScope to propagate failures as errors, not denials — scope timeout/WAIT_FAILED now triggers approval manager fallback path - Fix inconsistent error return between ctx cancellation and signal in both prompt_unix.go and prompt_windows.go P2 robustness: - Remove redundant downstreamIn.Close() outer defer in mcpproxy.go - Reduce doctor probe from ping -n 30 to ping -n 2 (29s → 1s) - Add clearTrackedVars to TestGetContextVars_MultipleVars - Promote job.close() CloseHandle failure from Debug to Warn - Add GOOS=windows golangci-lint step to CI windows-check job - Improve CreateJobObject error message with 'fuse doctor' guidance P3 cleanup: - Update 27 stale Windows skip messages across 3 test files - Update errNonInteractive message in specs/technical_v1.1.md - Add f.Sync() before os.ReadFile in prompt_windows_test.go - Add race window comment to mcpproxy_cleanup_windows.go - Sanitize getContextVars env var values individually Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go modules v2+ require the /v2/ path segment. The CI step used github.com/golangci/golangci-lint/cmd/... but the correct path is github.com/golangci/golangci-lint/v2/cmd/... (matching the justfile). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b0c8905 to
c5936e1
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
==========================================
+ Coverage 71.34% 71.38% +0.04%
==========================================
Files 73 74 +1
Lines 8728 8727 -1
==========================================
+ Hits 6227 6230 +3
+ Misses 2005 2004 -1
+ Partials 496 493 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| func platformSysProcAttr() *syscall.SysProcAttr { | ||
| return &syscall.SysProcAttr{} | ||
| return &syscall.SysProcAttr{ | ||
| CreationFlags: syscall.CREATE_NEW_PROCESS_GROUP, | ||
| } | ||
| } |
There was a problem hiding this comment.
Mutually exclusive process creation flags break process containment. The CREATE_NEW_PROCESS_GROUP flag is incompatible with assigning a process to a Windows Job Object. According to the Windows API documentation, a subsequent call to AssignProcessToJobObject will fail for a process created with this flag. This breaks the PR's primary goal of using Job Objects for process tree cleanup, causing containment to fail on every execution.
func platformSysProcAttr() *syscall.SysProcAttr {
// NOTE: CREATE_NEW_PROCESS_GROUP is incompatible with Job Objects.
// The call to AssignProcessToJobObject will fail if this flag is used,
// preventing process containment. Signaling must be handled differently.
return &syscall.SysProcAttr{}
}Prompt for LLM
File internal/adapters/runner_windows.go:
Line 49 to 53:
The provided Go code for Windows process creation sets the `CREATE_NEW_PROCESS_GROUP` flag in `syscall.SysProcAttr`. The goal is to allow `GenerateConsoleCtrlEvent` to target only the child process tree. However, the overall goal of the changes is to use Windows Job Objects for process containment, which involves calling `AssignProcessToJobObject` on the newly created process. According to Windows API documentation, a process created with `CREATE_NEW_PROCESS_GROUP` cannot be assigned to a job object; the call will fail. Explain this incompatibility and why using this flag will break the intended process containment feature. Suggest removing the flag and finding an alternative way to handle console signals.
Suggested Code:
func platformSysProcAttr() *syscall.SysProcAttr {
// NOTE: CREATE_NEW_PROCESS_GROUP is incompatible with Job Objects.
// The call to AssignProcessToJobObject will fail if this flag is used,
// preventing process containment. Signaling must be handled differently.
return &syscall.SysProcAttr{}
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/approve/prompt_windows.go (1)
94-95:⚠️ Potential issue | 🟠 MajorFail closed if the console input buffer can't be flushed.
If
FlushConsoleInputBufferfails, queued keystrokes can still be consumed byreadApprovalDecisionas fresh approval input. This should return an error so the manager takes the fallback path instead of trusting stale console state.🔒 Suggested fix
// Flush any stale input before rendering the prompt. - _ = windows.FlushConsoleInputBuffer(inHandle) + if err := windows.FlushConsoleInputBuffer(inHandle); err != nil { + return false, "", fmt.Errorf("flush console input buffer: %w", err) + }🤖 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 94 - 95, The FlushConsoleInputBuffer call currently swallows errors which lets stale keystrokes be interpreted as fresh input; modify the code around windows.FlushConsoleInputBuffer(inHandle) to check its returned error/result and if it fails return an error (propagate up from the function that calls it) so that readApprovalDecision or the surrounding approval prompt logic does not proceed and the manager can take the fallback path; reference the windows.FlushConsoleInputBuffer call and ensure the function that contains it returns an error instead of ignoring the failure.internal/adapters/mcpproxy.go (1)
75-83:⚠️ Potential issue | 🟠 MajorRegister pre-start pipe cleanup before the early-return paths.
downstreamInis already open whenStdoutPipe()fails here, and the current defer is only installed aftercmd.Start()succeeds. That leaks the writer end on this error path.🩹 Suggested fix
downstreamOut, err := cmd.StdoutPipe() if err != nil { + _ = downstreamIn.Close() return fmt.Errorf("downstream stdout: %w", err) } + + started := false + defer func() { + if !started { + _ = downstreamIn.Close() + _ = downstreamOut.Close() + } + }() if startErr := cmd.Start(); startErr != nil { return fmt.Errorf("start downstream %s: %w", downstreamName, startErr) } + started = true cleanup := proxyChildCleanup(cmd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/mcpproxy.go` around lines 75 - 83, The stdout pipe error path can leak the writer end because cleanup is only registered after cmd.Start(); call proxyChildCleanup(cmd) (or otherwise ensure pipe/child cleanup) right after creating pipes (e.g., immediately after obtaining downstreamIn/downstreamOut) so any early returns (like on StdoutPipe() error) trigger the cleanup; specifically, move or call proxyChildCleanup(cmd) before calling cmd.Start() or add an early-return defer/cleanup that closes downstreamIn and other opened pipes when StdoutPipe() or similar calls fail.
🧹 Nitpick comments (2)
.tickets/fus-r2kf.md (1)
46-46: Make the second callsite notation consistent for clarity.This bullet currently lists
int(os.Stdout.Fd())without the surroundingisTerminal(...), which is slightly inconsistent with other callsite entries and can confuse quick scans.✏️ Suggested doc-only tweak
-- `monitor.go:31` — `isTerminal(int(os.Stdin.Fd()))` and `int(os.Stdout.Fd())` +- `monitor.go:31` — `isTerminal(int(os.Stdin.Fd()))` and `isTerminal(int(os.Stdout.Fd()))`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tickets/fus-r2kf.md at line 46, The second callsite in the bullet should be made consistent by wrapping the stdout file descriptor in the same isTerminal(...) notation as stdin; update the entry that currently says int(os.Stdout.Fd()) to isTerminal(int(os.Stdout.Fd())) so both callsites read isTerminal(int(os.Stdin.Fd())) and isTerminal(int(os.Stdout.Fd())) (reference: monitor.go, isTerminal, os.Stdin.Fd, os.Stdout.Fd)..tickets/fus-g4vs.md (1)
72-76: Consider documenting the sanitization enhancement.The Notes section could mention the sanitization enhancement that was added during implementation. This would help future maintainers understand that the function does more than a simple extraction.
📝 Suggested addition to Notes section
## Notes **2026-03-31T06:20:38Z** -Closed: implemented in Phase 3/4 commits on feat/windows-terminal-approval branch. +Closed: implemented in Phase 3/4 commits on feat/windows-terminal-approval branch. + +**Implementation note**: Added `sanitize.String(val)` call when building the result string to ensure environment variable values are properly sanitized before display in the approval prompt. This security enhancement prevents potential sensitive data exposure while maintaining the function's portability across platforms.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.tickets/fus-g4vs.md around lines 72 - 76, Update the Notes section to record the sanitization enhancement added in the Phase 3/4 implementation on the feat/windows-terminal-approval branch: add a short sentence describing that the extraction function was enhanced to sanitize inputs (e.g., trimming, escaping, and removing unsafe characters) and note where to find the implementation (referencing the Phase 3/4 commits on feat/windows-terminal-approval and the 2026-03-31 entry) so future maintainers know it does more than simple extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.tickets/fus-c7gm.md:
- Around line 16-18: The fenced code block that contains "Approval requires an
interactive terminal (/dev/tty unavailable)" is missing a language tag; update
the opening fence from ``` to include a language (e.g., ```text) so markdownlint
stops flagging it, then re-run linting to confirm the warning is resolved.
In @.tickets/fus-g4vs.md:
- Line 22: The documentation claims that getContextVars() is a "Simple
extraction — no behavioral changes" but the implementation calls
sanitize.String(val) (in getContextVars()), which is a behavioral/security
change; update the documentation to state that environment variable values are
sanitized (mention sanitize.String) so callers know values are altered, and
ensure both prompt_unix.go and prompt_windows.go references include this
sanitization note.
- Around line 28-50: Update the example code in the ticket to match the
implemented behavior in getContextVars by using sanitize.String(val) when
appending environment values (replace the plain concatenation v + "=" + val with
v + "=" + sanitize.String(val)); also add a short design comment in the ticket
explaining sanitization is required because environment variables may contain
sensitive data and must be sanitized before display.
---
Duplicate comments:
In `@internal/adapters/mcpproxy.go`:
- Around line 75-83: The stdout pipe error path can leak the writer end because
cleanup is only registered after cmd.Start(); call proxyChildCleanup(cmd) (or
otherwise ensure pipe/child cleanup) right after creating pipes (e.g.,
immediately after obtaining downstreamIn/downstreamOut) so any early returns
(like on StdoutPipe() error) trigger the cleanup; specifically, move or call
proxyChildCleanup(cmd) before calling cmd.Start() or add an early-return
defer/cleanup that closes downstreamIn and other opened pipes when StdoutPipe()
or similar calls fail.
In `@internal/approve/prompt_windows.go`:
- Around line 94-95: The FlushConsoleInputBuffer call currently swallows errors
which lets stale keystrokes be interpreted as fresh input; modify the code
around windows.FlushConsoleInputBuffer(inHandle) to check its returned
error/result and if it fails return an error (propagate up from the function
that calls it) so that readApprovalDecision or the surrounding approval prompt
logic does not proceed and the manager can take the fallback path; reference the
windows.FlushConsoleInputBuffer call and ensure the function that contains it
returns an error instead of ignoring the failure.
---
Nitpick comments:
In @.tickets/fus-g4vs.md:
- Around line 72-76: Update the Notes section to record the sanitization
enhancement added in the Phase 3/4 implementation on the
feat/windows-terminal-approval branch: add a short sentence describing that the
extraction function was enhanced to sanitize inputs (e.g., trimming, escaping,
and removing unsafe characters) and note where to find the implementation
(referencing the Phase 3/4 commits on feat/windows-terminal-approval and the
2026-03-31 entry) so future maintainers know it does more than simple
extraction.
In @.tickets/fus-r2kf.md:
- Line 46: The second callsite in the bullet should be made consistent by
wrapping the stdout file descriptor in the same isTerminal(...) notation as
stdin; update the entry that currently says int(os.Stdout.Fd()) to
isTerminal(int(os.Stdout.Fd())) so both callsites read
isTerminal(int(os.Stdin.Fd())) and isTerminal(int(os.Stdout.Fd())) (reference:
monitor.go, isTerminal, os.Stdin.Fd, os.Stdout.Fd).
🪄 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: e2d56533-53fb-4458-9280-84fd7ea11b82
📒 Files selected for processing (60)
.github/workflows/ci.yml.tickets/fus-0r82.md.tickets/fus-4gzq.md.tickets/fus-556x.md.tickets/fus-b2yw.md.tickets/fus-c7gm.md.tickets/fus-d8fn.md.tickets/fus-e3pw.md.tickets/fus-f4qx.md.tickets/fus-fx68.md.tickets/fus-g4vs.md.tickets/fus-g5ry.md.tickets/fus-h5rz.md.tickets/fus-h6sz.md.tickets/fus-iviw.md.tickets/fus-izck.md.tickets/fus-j6qd.md.tickets/fus-j7ta.md.tickets/fus-k3tn.md.tickets/fus-k8ub.md.tickets/fus-kyal.md.tickets/fus-l9vc.md.tickets/fus-lzxe.md.tickets/fus-m1wd.md.tickets/fus-n2xe.md.tickets/fus-n4d6.md.tickets/fus-n4hd.md.tickets/fus-p3cw.md.tickets/fus-p3yf.md.tickets/fus-p50r.md.tickets/fus-q8xp.md.tickets/fus-r2kf.md.tickets/fus-r7km.md.tickets/fus-rh1w.md.tickets/fus-t4vn.md.tickets/fus-tssy.md.tickets/fus-tvat.md.tickets/fus-v9mr.md.tickets/fus-w2ht.md.tickets/fus-wrx7.mdintegration_test.gointernal/adapters/codexshell_test.gointernal/adapters/job_windows.gointernal/adapters/mcpproxy.gointernal/adapters/mcpproxy_cleanup_unix.gointernal/adapters/mcpproxy_cleanup_windows.gointernal/adapters/runner.gointernal/adapters/runner_exec_windows.gointernal/adapters/runner_test.gointernal/adapters/runner_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/help_width_windows.gojustfilespecs/technical_v1.1.mdspecs/windows-support-plan.md
💤 Files with no reviewable changes (1)
- internal/adapters/runner.go
✅ Files skipped from review due to trivial changes (35)
- internal/adapters/runner_test.go
- .github/workflows/ci.yml
- integration_test.go
- justfile
- internal/adapters/codexshell_test.go
- .tickets/fus-izck.md
- .tickets/fus-kyal.md
- .tickets/fus-b2yw.md
- .tickets/fus-h5rz.md
- .tickets/fus-fx68.md
- .tickets/fus-k8ub.md
- specs/technical_v1.1.md
- .tickets/fus-h6sz.md
- .tickets/fus-f4qx.md
- .tickets/fus-tssy.md
- .tickets/fus-4gzq.md
- .tickets/fus-iviw.md
- .tickets/fus-k3tn.md
- .tickets/fus-j7ta.md
- .tickets/fus-w2ht.md
- .tickets/fus-rh1w.md
- .tickets/fus-v9mr.md
- .tickets/fus-q8xp.md
- .tickets/fus-wrx7.md
- .tickets/fus-t4vn.md
- .tickets/fus-tvat.md
- .tickets/fus-m1wd.md
- .tickets/fus-g5ry.md
- .tickets/fus-d8fn.md
- .tickets/fus-j6qd.md
- .tickets/fus-l9vc.md
- .tickets/fus-p3cw.md
- .tickets/fus-r7km.md
- specs/windows-support-plan.md
- .tickets/fus-e3pw.md
🚧 Files skipped from review as they are similar to previous changes (14)
- internal/approve/prompt_shared.go
- internal/approve/prompt_windows_test.go
- internal/approve/prompt_test.go
- internal/adapters/runner_windows.go
- .tickets/fus-lzxe.md
- .tickets/fus-0r82.md
- internal/adapters/mcpproxy_cleanup_unix.go
- internal/adapters/mcpproxy_cleanup_windows.go
- .tickets/fus-n4d6.md
- .tickets/fus-n2xe.md
- .tickets/fus-556x.md
- internal/approve/prompt_unix.go
- internal/cli/doctor_live_windows.go
- internal/cli/help_width_windows.go
| ``` | ||
| Approval requires an interactive terminal (/dev/tty unavailable) | ||
| ``` |
There was a problem hiding this comment.
Specify the fenced-block language.
markdownlint will keep flagging this block until the opening fence is something like ```text.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tickets/fus-c7gm.md around lines 16 - 18, The fenced code block that
contains "Approval requires an interactive terminal (/dev/tty unavailable)" is
missing a language tag; update the opening fence from ``` to include a language
(e.g., ```text) so markdownlint stops flagging it, then re-run linting to
confirm the warning is resolved.
|
|
||
| ## Design | ||
|
|
||
| Simple extraction — no behavioral changes. `getContextVars()` builds a comma-separated string of relevant environment variables (AWS_PROFILE, TF_WORKSPACE, KUBECONFIG, etc.) for display in the approval prompt. Both `prompt_unix.go` and `prompt_windows.go` (Issue fus-p3cw) need this function. |
There was a problem hiding this comment.
Documentation inconsistency: behavioral change was made.
The design claims "Simple extraction — no behavioral changes," but the actual implementation (per context snippet 1) adds sanitize.String(val) to sanitize environment variable values before including them in the result. This is a security enhancement but represents a behavioral change not documented here.
Consider updating this line to acknowledge the sanitization enhancement:
-Simple extraction — no behavioral changes. `getContextVars()` builds a comma-separated string of relevant environment variables (AWS_PROFILE, TF_WORKSPACE, KUBECONFIG, etc.) for display in the approval prompt. Both `prompt_unix.go` and `prompt_windows.go` (Issue fus-p3cw) need this function.
+Simple extraction with sanitization enhancement. `getContextVars()` builds a comma-separated string of relevant environment variables (AWS_PROFILE, TF_WORKSPACE, KUBECONFIG, etc.) for display in the approval prompt, sanitizing each value via `sanitize.String()` to prevent sensitive data exposure. Both `prompt_unix.go` and `prompt_windows.go` (Issue fus-p3cw) need this function.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Simple extraction — no behavioral changes. `getContextVars()` builds a comma-separated string of relevant environment variables (AWS_PROFILE, TF_WORKSPACE, KUBECONFIG, etc.) for display in the approval prompt. Both `prompt_unix.go` and `prompt_windows.go` (Issue fus-p3cw) need this function. | |
| Simple extraction with sanitization enhancement. `getContextVars()` builds a comma-separated string of relevant environment variables (AWS_PROFILE, TF_WORKSPACE, KUBECONFIG, etc.) for display in the approval prompt, sanitizing each value via `sanitize.String()` to prevent sensitive data exposure. Both `prompt_unix.go` and `prompt_windows.go` (Issue fus-p3cw) need this function. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tickets/fus-g4vs.md at line 22, The documentation claims that
getContextVars() is a "Simple extraction — no behavioral changes" but the
implementation calls sanitize.String(val) (in getContextVars()), which is a
behavioral/security change; update the documentation to state that environment
variable values are sanitized (mention sanitize.String) so callers know values
are altered, and ensure both prompt_unix.go and prompt_windows.go references
include this sanitization note.
| ```go | ||
| // getContextVars returns relevant environment variables for the prompt. | ||
| func getContextVars() string { | ||
| relevantVars := []string{ | ||
| "AWS_PROFILE", "AWS_REGION", "AWS_DEFAULT_REGION", | ||
| "TF_WORKSPACE", "TF_VAR_environment", | ||
| "KUBECONFIG", "KUBECONTEXT", | ||
| "GCP_PROJECT", "GOOGLE_CLOUD_PROJECT", | ||
| "AZURE_SUBSCRIPTION", | ||
| } | ||
| var result string | ||
| for _, v := range relevantVars { | ||
| val := os.Getenv(v) | ||
| if val != "" { | ||
| if result != "" { | ||
| result += ", " | ||
| } | ||
| result += v + "=" + val | ||
| } | ||
| } | ||
| return result | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Code specification doesn't match implementation.
The code block shown here (specifically line 45: result += v + "=" + val) differs from the actual implementation in prompt_shared.go (context snippet 1, line 43), which includes sanitization: result += v + "=" + sanitize.String(val).
Since this is a closed ticket being added as documentation, it should accurately reflect what was implemented.
📝 Proposed update to match actual implementation
if result != "" {
result += ", "
}
- result += v + "=" + val
+ result += v + "=" + sanitize.String(val)
}
}
return resultAlso add a comment in the design section explaining why sanitization is needed (environment variables may contain sensitive data that should be sanitized before display).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```go | |
| // getContextVars returns relevant environment variables for the prompt. | |
| func getContextVars() string { | |
| relevantVars := []string{ | |
| "AWS_PROFILE", "AWS_REGION", "AWS_DEFAULT_REGION", | |
| "TF_WORKSPACE", "TF_VAR_environment", | |
| "KUBECONFIG", "KUBECONTEXT", | |
| "GCP_PROJECT", "GOOGLE_CLOUD_PROJECT", | |
| "AZURE_SUBSCRIPTION", | |
| } | |
| var result string | |
| for _, v := range relevantVars { | |
| val := os.Getenv(v) | |
| if val != "" { | |
| if result != "" { | |
| result += ", " | |
| } | |
| result += v + "=" + val | |
| } | |
| } | |
| return result | |
| } | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.tickets/fus-g4vs.md around lines 28 - 50, Update the example code in the
ticket to match the implemented behavior in getContextVars by using
sanitize.String(val) when appending environment values (replace the plain
concatenation v + "=" + val with v + "=" + sanitize.String(val)); also add a
short design comment in the ticket explaining sanitization is required because
environment variables may contain sensitive data and must be sanitized before
display.
- dupl: refactor renderPromptANSI/renderPromptPlain into shared writePrompt - errorlint: use errors.Is instead of != for errNonInteractive checks - gosec: add nolint:gosec with SAFETY justification for unsafe.Pointer - govet: fix variable shadow of err in PromptUser - staticcheck: apply De Morgan's law to drive letter check 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:
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33c0d557-c783-4380-bc33-a21e7ed4bdaf
📒 Files selected for processing (4)
internal/adapters/job_windows.gointernal/adapters/runner_windows.gointernal/approve/prompt_windows.gointernal/approve/prompt_windows_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/approve/prompt_windows_test.go
- internal/adapters/runner_windows.go
| case <-sigCh: | ||
| fmt.Fprintf(conOut, "\n Denied (signal received).\n\n") | ||
| return false, "", nil | ||
| return false, "", fmt.Errorf("approval interrupted by signal") |
There was a problem hiding this comment.
Update the prompt copy to match the new fallback semantics.
Both branches now return errors, and internal/approve/manager.go handles prompt errors via fallback instead of an explicit deny. Denied (signal received) and bare Timed out. still read like final terminal outcomes, which no longer matches what happens next.
Possible fix
case <-sigCh:
- fmt.Fprintf(conOut, "\n Denied (signal received).\n\n")
+ fmt.Fprintf(conOut, "\n Interrupted (signal received).\n\n")
return false, "", fmt.Errorf("approval interrupted by signal") if time.Now().After(deadline) {
- fmt.Fprintf(conOut, "\n Timed out.\n\n")
+ fmt.Fprintf(conOut, "\n Timed out. The command remains pending — approve via fuse monitor.\n\n")
return "", false, errPromptTimeout
}Also applies to: 223-224
1 participant
Summary
/dev/tty+ termios with Windows Console API (CONIN$/CONOUT$) for interactive approval prompts. Anti-spoofing preserved (direct console device, not stdin). ANSI color support with VT processing fallback.fuse doctorvalidates console access and raw mode.Setpgid,Pdeathsig,Kill(-pid, sig)) with Windows Job Objects.JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSEensures children die when fuse exits.GenerateConsoleCtrlEvent(CTRL_BREAK_EVENT)forwards Ctrl+C to child tree. MCP proxy downstream servers also wrapped in job objects for grandchild cleanup.ping, alignbytes.Buffertostrings.Builder, remove Phase 3 leftover APPROVAL gate, addjust lint-windowstarget.New files
internal/approve/prompt_windows.gointernal/approve/prompt_windows_test.gointernal/adapters/job_windows.gointernal/adapters/mcpproxy_cleanup_{unix,windows}.goKey changes
internal/adapters/runner_exec_windows.gocmd.CancelviaTerminateJobObject,forwardConsoleCtrl,waitForManagedCommandinternal/adapters/runner_windows.goCREATE_NEW_PROCESS_GROUPinplatformSysProcAttr()internal/adapters/runner.gointernal/cli/doctor_live_windows.gointernal/cli/help_width_windows.goGetConsoleScreenBufferInfointernal/approve/prompt_shared.gogetContextVars()for cross-platform useTest plan
GOOS=windows GOARCH=amd64 go build ./...— cleanGOOS=windows GOARCH=arm64 go build ./...— cleanGOOS=windows go vet ./...— cleanGOOS=linux go build ./...— clean (regression)go test ./... -race -timeout 120s— 15 packages pass//nolint, 0/0#nosecfuse run "echo hello",fuse doctor --security, Ctrl+C forwarding🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Testing & Quality