Skip to content

Centralize the Windows windowless process policy - #12946

Merged
brooke-hamilton merged 4 commits into
mainfrom
brooke-hamilton-windowless-process-policy
Sep 11, 2026
Merged

brooke-hamilton merged 4 commits into
mainfrom
brooke-hamilton-windowless-process-policy

Conversation

@brooke-hamilton

Copy link
Copy Markdown
Member

Summary

Expose process.IsWindowless() bool as the shared, live Windows no-console policy query for future tool adapters. Both existing *exec.Cmd constructors use the same query through command configuration. Console detection remains based on GetConsoleCP, preserving classic consoles and Windows Terminal/ConPTY; non-Windows platforms return false.

In windowless mode, configuration assigns an explicit EOF reader only when Cmd.Stdin is nil. Existing readers are preserved, and callers can replace stdin after construction, including bytes.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 SysProcAttr fields and flags are preserved while applying HideWindow and CREATE_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 main at a0654b16470228271daa8875889b189bb8951c21, 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:

go test .\pkg\process .\pkg\cli\bicep .\test\windowless -run '^(TestCommands_|TestConfigure_|TestProcessHelper$|TestVersion_ClosesCallerStderrPipePromptly$|TestRadVersion_)' -count=1 -timeout=2m

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

File Summary of change
pkg/process/process.go Document and expose the shared query and stdin contract without changing constructor signatures.
pkg/process/process_windows.go Reuse the shared query and supply explicit EOF only for unset windowless stdin.
pkg/process/process_other.go Return false for the platform policy query; retain no-op configuration.
pkg/process/process_test.go Add bounded helper-process coverage for stdin delivery, output, exit status, and context cancellation.
pkg/process/process_windows_test.go Cover attached/windowless policy, both constructors, and caller flag/field preservation.
pkg/process/process_other_test.go Cover both constructors, the false query, and unchanged configuration/input on non-Windows platforms.
docs/architecture/rad-cli.md Add a focused paragraph documenting the query, EOF default, caller input, and prompt-control limitations.

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

Unit Tests

    2 files  ± 0    461 suites  ±0   8m 56s ⏱️ + 1m 20s
6 543 tests +12  6 541 ✅ +12  2 💤 ±0  0 ❌ ±0 
7 819 runs  +12  7 817 ✅ +12  2 💤 ±0  0 ❌ ±0 

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.
github.com/radius-project/radius/pkg/process ‑ TestCommand_UnchangedOnNonWindows
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/Command
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/Command/default_EOF
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/Command/finite_input_after_construction
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/Command/nonzero_exit
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/CommandContext
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/CommandContext/default_EOF
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/CommandContext/finite_input_after_construction
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/CommandContext/nonzero_exit
github.com/radius-project/radius/pkg/process ‑ TestCommands_UnchangedOnNonWindows/context_cancellation
…

♻️ This comment has been updated with latest results.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.82%. Comparing base (fffca1e) to head (06445b1).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.Stdin is nil, and preserve/merge SysProcAttr fields while applying HideWindow and CREATE_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.

Comment thread pkg/process/process_test.go Outdated
Comment thread pkg/process/process_test.go
Signed-off-by: Brooke Hamilton <45323234+brooke-hamilton@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@brooke-hamilton
brooke-hamilton marked this pull request as ready for review September 9, 2026 22:37
@brooke-hamilton
brooke-hamilton requested review from a team as code owners September 9, 2026 22:37
@radius-functional-tests

radius-functional-tests Bot commented Sep 9, 2026

Copy link
Copy Markdown

Radius functional test overview

🔍 Go to test action run

Click here to see the test run details
Name Value
Repository radius-project/radius
Commit ref 06445b1
Unique ID funcfa23da6f2c
Image tag pr-funcfa23da6f2c
  • Dapr: 1.14.4
  • Azure KeyVault CSI driver: 1.4.2
  • Azure Workload identity webhook: 1.3.0
  • Bicep recipe location ghcr.io/radius-project/dev/test/testrecipes/test-bicep-recipes/<name>:pr-funcfa23da6f2c
  • Terraform recipe location http://tf-module-server.radius-test-tf-module-server.svc.cluster.local/<name>.zip (in cluster)
  • applications-rp test image location: ghcr.io/radius-project/dev/applications-rp:pr-funcfa23da6f2c
  • dynamic-rp test image location: ghcr.io/radius-project/dev/dynamic-rp:pr-funcfa23da6f2c
  • controller test image location: ghcr.io/radius-project/dev/controller:pr-funcfa23da6f2c
  • ucp test image location: ghcr.io/radius-project/dev/ucpd:pr-funcfa23da6f2c
  • deployment-engine test image location: ghcr.io/radius-project/deployment-engine:latest

Test Status

⌛ Building Radius and pushing container images for functional tests...
✅ Container images build succeeded
⌛ Publishing Bicep Recipes for functional tests...
✅ Recipe publishing succeeded
⌛ Starting ucp-cloud functional tests...
⌛ Starting corerp-cloud functional tests...
✅ ucp-cloud functional tests succeeded
✅ corerp-cloud functional tests succeeded

@brooke-hamilton
brooke-hamilton added this pull request to the merge queue Sep 11, 2026
Merged via the queue into main with commit 25d9274 Sep 11, 2026
80 checks passed
@brooke-hamilton
brooke-hamilton deleted the brooke-hamilton-windowless-process-policy branch September 11, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Centralize the Windows windowless non-interactive process policy

3 participants