Skip to content

Add common PowerShell functions for Specify - #4825

Closed
Bee92-exe wants to merge 3 commits into
github:mainfrom
Bee92-exe:fix/validate-python3-in-powershell
Closed

Bee92-exe wants to merge 3 commits into
github:mainfrom
Bee92-exe:fix/validate-python3-in-powershell

Conversation

@Bee92-exe

@Bee92-exe Bee92-exe commented Oct 2, 2026 •

Copy link
Copy Markdown

Description

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable)

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: N/A

@Bee92-exe
Bee92-exe requested a review from mnriem as a code owner October 2, 2026 21:52
@mnriem mnriem added the triage-out-of-scope Verdict: won't land in core — invalid, duplicate, off-mission, or redirected to an extension label Oct 4, 2026
@mnriem
mnriem requested a balanced review from Copilot October 5, 2026 13:48

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.

Copilot review overview

🟡 Changes recommended

The fix is in an unreferenced duplicate file, while runtime consumers and the test still use the unchanged common.ps1.

Review effort: Balanced
Findings: 1 High severity

Open (1)
What changed in this PR

Adds PowerShell helper logic intended to reject unusable python3 executables, plus regression coverage.

Changes:

  • Adds a duplicate PowerShell common-functions file.
  • Adds a cross-platform interpreter-selection regression test.
File Description
scripts/​powershell/​common-fi.ps1 Adds duplicated common helpers with updated Python detection.
tests/​test_common_ps1_python3_command.py Tests fallback from an unusable python3.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +247 to +251
if (Get-Command python3 -ErrorAction SilentlyContinue) {
$ver = & python3 --version 2>&1
if ($ver -match 'Python 3') {
& python3 -c 'import yaml' *> $null
if ($LASTEXITCODE -eq 0) { return @('python3') }
@mnriem

mnriem commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Thanks for taking the time to investigate this and add regression coverage. The underlying problem is valid: PowerShell should not select a python3 executable merely because Get-Command finds it, since the Windows Store alias can exist but fail when invoked.

I’m closing this PR because #4151 already addresses the same issue with broader coverage, including fallback to python and py -3, nonzero exit handling, and PowerShell error-preference behavior. This implementation also adds the change in common-fi.ps1, while runtime consumers and the test continue to load common.ps1, so the proposed fix is not active and its regression test currently fails.

Thank you again for the contribution. Consolidating the work in the existing PR will help us avoid competing implementations and keep the review focused.

@mnriem mnriem closed this Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-out-of-scope Verdict: won't land in core — invalid, duplicate, off-mission, or redirected to an extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants