Centralize the Windows windowless process policy - #12946
Conversation
Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Unit Tests 2 files ± 0 461 suites ±0 8m 56s ⏱️ + 1m 20s Results for commit 06445b1. ± Comparison against base commit fffca1e. This pull request removes 1 and adds 13 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12946 +/- ##
=======================================
Coverage 59.82% 59.82%
=======================================
Files 779 779
Lines 46058 46060 +2
=======================================
+ Hits 27552 27555 +3
+ Misses 18506 18505 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new helper-process tests include cleanup/timer patterns that can act on already-reaped processes (risking PID-reuse side effects) and a cancellation assertion that doesn’t actually verify the Wait() error is due to context cancellation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR centralizes the Windows “no attached console” (windowless) policy in pkg/process by exposing process.IsWindowless() and reusing it in both Command and CommandContext, while documenting and testing the intended stdin/SysProcAttr behavior for windowless vs. attached-console execution.
Changes:
- Add
process.IsWindowless()as the shared, live Windows console-attachment query (false on non-Windows). - In Windows windowless mode, set an explicit EOF stdin only when
Cmd.Stdinis nil, and preserve/mergeSysProcAttrfields while applyingHideWindowandCREATE_NO_WINDOW. - Add cross-platform helper-process tests for stdin delivery, exit codes, output, and cancellation; document the policy and its limitations in the CLI architecture guide.
File summaries
| File | Description |
|---|---|
| pkg/process/process.go | Exposes IsWindowless() and documents the stdin default contract for windowless mode. |
| pkg/process/process_windows.go | Implements the windowless query and applies explicit EOF stdin + no-window SysProcAttr settings. |
| pkg/process/process_other.go | Implements the windowless query as false and keeps configuration a no-op on non-Windows. |
| pkg/process/process_test.go | Adds helper-process-based tests for stdin behavior, output/exit codes, and CommandContext cancellation. |
| pkg/process/process_windows_test.go | Adds Windows-specific coverage for the shared policy, stdin default, and SysProcAttr flag preservation. |
| pkg/process/process_other_test.go | Adds non-Windows coverage for unchanged behavior and the always-false windowless query. |
| docs/architecture/rad-cli.md | Documents process.IsWindowless(), the explicit-EOF default behavior, and the non-guarantee of non-interactivity. |
Review details
Suppressed comments (1)
pkg/process/process_test.go:119
- This cleanup always calls Kill and Wait, even if the test has already waited on the process. Guard it so you don't send signals to a PID that may be reused and avoid calling Wait twice.
t.Cleanup(func() {
_ = cmd.Process.Kill()
_ = cmd.Wait()
})
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
Summary
Expose
process.IsWindowless() boolas the shared, live Windows no-console policy query for future tool adapters. Both existing*exec.Cmdconstructors use the same query through command configuration. Console detection remains based onGetConsoleCP, preserving classic consoles and Windows Terminal/ConPTY; non-Windows platforms return false.In windowless mode, configuration assigns an explicit EOF reader only when
Cmd.Stdinis nil. Existing readers are preserved, and callers can replace stdin after construction, includingbytes.NewReader(sqlData)for PostgreSQL restore. Go already connects nil stdin to the null device, so this makes the default explicit rather than introducing a universal anti-hang mechanism.Existing
SysProcAttrfields and flags are preserved while applyingHideWindowandCREATE_NO_WINDOW. No detachment or Job Object breakaway flags, production timeouts, environment switches, global environment mutation, or package-initialization console probes are added. Attached-console/non-Windows behavior, output, exit status, and context cancellation remain unchanged.Reason for change
Fixes #12945.
Implements only section 1 of parent #12887. Tool-specific prompt handling, Bicep context/pipe changes, kubeconfig validation, PostgreSQL flags, Azure SDK changes, and the separate removals in #12942 and #12944 are out of scope. This is an independent draft based on
mainata0654b16470228271daa8875889b189bb8951c21, not stacked on either removal PR or the parent branch.How to test
Run the focused Windows suite, including the unchanged native Job Object/process-tree and Bicep stderr-inheritance regressions:
The process cases cover the shared query, both constructors, explicit EOF, real finite input delivery before configuration and after construction, preserved caller settings, nonzero exits, stdout/stderr, and context cancellation. These run with simulated attached and windowless Windows policy states. The process suite also passed on Linux with race detection, and the Windows arm64 process tests compile successfully.
The architecture guide passes spellcheck. Markdown checks report pre-existing table-format findings on unchanged lines 20, 27, and 35; unrelated table reformatting is intentionally excluded.
File change summary
pkg/process/process.gopkg/process/process_windows.gopkg/process/process_other.gopkg/process/process_test.gopkg/process/process_windows_test.gopkg/process/process_other_test.godocs/architecture/rad-cli.md