Skip to content

fix(windows): make the PowerShell scripts survive a real shell run - #22

Closed
Firnschnee wants to merge 1 commit into
Christian-Katzmann:mainfrom
Firnschnee:fix/windows-powershell-runtime
Closed

fix(windows): make the PowerShell scripts survive a real shell run#22
Firnschnee wants to merge 1 commit into
Christian-Katzmann:mainfrom
Firnschnee:fix/windows-powershell-runtime

Conversation

@Firnschnee

@Firnschnee Firnschnee commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Part of #21. Three bugs that pass PSScriptAnalyzer but break the first time the
scripts run on a real Windows shell, found while verifying the beta on Windows 11.

run-template-edge.ps1: the Edge fallback never started the dev server

The cmd /c command was quoted so that cmd.exe stripped the first and last quote of
a line containing a redirect (its documented behavior), turning
"node ..." > "log" 2>&1 into node ..." > "log 2>&1. cmd exited 1, no log was
written, and the server never started, so the whole Edge fallback (the path for
users without the .NET SDK) was a dead end. Wrapped the pipeline in one
/s /c "..." pair with the inner path quotes doubled.

Verified: the dev server now binds and serves (server.log shows the listen line,
the readiness probe gets a response). Scope note: this fixes server start only.
The Edge window lifetime (WaitForExit returns early because Chromium forks and
hands off) is a separate open item tracked in #21, not addressed here.

inspect.ps1: StrictMode crash in the FSA scan

Search-Code did if ($Dirs.Count -eq 0), but when none of src/services/app/lib
exist the pipeline yields $null and $null.Count throws under StrictMode. Only the
script's ErrorActionPreference = 'Continue' kept it limping. Changed to
if (-not $Dirs), which covers $null and an empty result alike.

desktop-quit.ps1: false "Stopped" report on a stale pid

Invoke-PortSweep set $closed = $true whenever a parseable server.pid existed, so a
stale pid (left by a crash or force-kill) made the run report "Stopped dev servers"
when nothing was running. Now it only counts as closed when the recorded process is
actually alive.

All three lint clean under the repo's PSScriptAnalyzerSettings.psd1 (Error +
Warning). Tested on Windows 11, .NET SDK 8.0.422, PowerShell 7.6.1.

Summary by Sourcery

Fix Windows PowerShell scripts so they behave correctly when run in a real shell instead of only passing static analysis.

Bug Fixes:

  • Ensure the Edge fallback dev server command starts correctly and logs output when invoked via cmd.exe.
  • Prevent the FSA scan in inspect.ps1 from crashing under StrictMode when no search directories exist.
  • Avoid reporting successful dev server shutdown when only a stale PID file is present in desktop-quit.ps1.

Three bugs that pass PSScriptAnalyzer but break the first time the
scripts run on a real Windows shell, found while verifying the beta on
Windows 11 hardware.

run-template-edge.ps1: the cmd /c redirect was quoted so that cmd.exe
stripped the first and last quote of the command line (its documented
behavior when the line contains a redirect), turning
  "node ..." > "log" 2>&1
into
  node ..." > "log 2>&1
a malformed line. cmd exited 1, no log was written, and the dev server
never started, so the entire Edge fallback was a dead end. Wrap the whole
pipeline in one /s /c "..." pair with the inner path quotes doubled.
(This fixes server start only; the Edge window lifetime is tracked
separately.)

inspect.ps1: Search-Code did `if ($Dirs.Count -eq 0)`, but when none of
src/services/app/lib exist the pipeline yields $null and $null.Count
throws under StrictMode, breaking the FSA scan. Only the script's
ErrorActionPreference = 'Continue' hid it. Use `if (-not $Dirs)`.

desktop-quit.ps1: Invoke-PortSweep set $closed = $true whenever a
parseable server.pid existed, so a stale pid (left by a crash or
force-kill) made the run report "Stopped dev servers" when nothing was
running. Only count it closed when the recorded process is actually
alive.
@sourcery-ai

sourcery-ai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Fixes three Windows PowerShell script issues so they behave correctly when run in a real shell: the Edge dev-server fallback now starts and logs reliably via corrected cmd.exe quoting, the FSA scan in inspect.ps1 no longer throws under StrictMode when no search directories exist, and desktop-quit.ps1 only reports dev servers as stopped when it actually finds and kills a live process from server.pid.

Sequence diagram for updated Invoke-PortSweep process handling

sequenceDiagram
    actor User
    participant DesktopQuitScript
    participant InvokePortSweep
    participant OSProcessManager
    participant TargetProcess

    User->>DesktopQuitScript: run desktop-quit.ps1
    DesktopQuitScript->>InvokePortSweep: Invoke-PortSweep
    InvokePortSweep->>InvokePortSweep: Get-Content server.pid
    InvokePortSweep->>InvokePortSweep: [int]::TryParse(recorded, pidNum)
    alt pid parsed
        InvokePortSweep->>OSProcessManager: Get-Process -Id pidNum
        alt process alive
            OSProcessManager-->>InvokePortSweep: process info
            InvokePortSweep->>TargetProcess: Stop-ProcessTree -ProcessId pidNum
            InvokePortSweep->>InvokePortSweep: $closed = $true
        else process not found (stale pid)
            OSProcessManager-->>InvokePortSweep: $null
            InvokePortSweep->>InvokePortSweep: $closed remains $false
        end
    else pid not parsed
        InvokePortSweep->>InvokePortSweep: $closed remains $false
    end
    InvokePortSweep-->>DesktopQuitScript: return $closed
    DesktopQuitScript-->>User: report dev servers stopped only if $closed
Loading

File-Level Changes

Change Details Files
Ensure Edge fallback dev server actually starts and logs by correcting cmd.exe quoting and arguments when launching the server via StartCommand.
  • Replaced simple /c command invocation with /s /c and a single outer-quoted pipeline to prevent cmd.exe from stripping quotes in redirected commands.
  • Adjusted quoting so the StartCommand and ServerLog path are embedded inside the single /s /c quote pair, with inner path quotes doubled to survive through cmd.
  • Documented the cmd.exe redirect and quote-stripping behavior in comments for future maintainers.
plugins/app-it-windows/skills/app-it-windows/templates/run-template-edge.ps1
Prevent StrictMode crashes in the FSA scan when no search directories exist by making the directory check null- and empty-safe.
  • Replaced the $Dirs.Count -eq 0 guard with an -not $Dirs check so that both $null and empty arrays are handled without throwing.
  • Added a comment explaining why -not $Dirs is used instead of accessing .Count under StrictMode.
plugins/app-it-windows/skills/app-it-windows/templates/inspect.ps1
Avoid falsely reporting that dev servers were stopped when server.pid is stale by verifying the recorded process is actually alive before treating the port as closed.
  • Extended the TryParse check to also require that Get-Process for the parsed PID succeeds (with ErrorAction SilentlyContinue).
  • Updated logic so $closed is only set when a valid, currently running process is found and Stop-ProcessTree is invoked.
  • Added comments clarifying the behavior with stale server.pid files and why the liveness check is necessary.
plugins/app-it-windows/skills/app-it-windows/templates/desktop-quit.ps1

Possibly linked issues

  • #(unknown): The PR’s three PowerShell script fixes exactly match the “make the scripts run on a real shell” issue section.

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • In desktop-quit.ps1, when you detect a parseable but non-running PID (stale server.pid), consider removing or updating the PID file so future runs don't repeatedly encounter the same stale state.
  • The cmd.exe quoting in run-template-edge.ps1 is now fairly intricate; it may be worth wrapping this in a small helper or adding a short example of the resulting command line in comments to reduce future maintenance mistakes around escaping.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In desktop-quit.ps1, when you detect a parseable but non-running PID (stale server.pid), consider removing or updating the PID file so future runs don't repeatedly encounter the same stale state.
- The cmd.exe quoting in run-template-edge.ps1 is now fairly intricate; it may be worth wrapping this in a small helper or adding a short example of the resulting command line in comments to reduce future maintenance mistakes around escaping.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@Firnschnee

Copy link
Copy Markdown
Contributor Author

Thanks. On the stale PID: it is already handled a level up, so I left Invoke-PortSweep as is. The per-app sweep unconditionally removes server.pid/server.port (and the backend pair) at the end of each loop iteration, and the launchers re-clean stale pids on startup, so clearing it inside the sweep would be redundant. On the cmd quoting: I kept it inline with the explanatory comment rather than a helper, since it is a single call site; glad to factor it out if it ever grows a second caller.

Christian-Katzmann pushed a commit that referenced this pull request Jun 20, 2026
Part of #21. Three bugs that pass PSScriptAnalyzer but break the first
time the scripts run on a real Windows shell, found verifying the beta on
Windows 11.

- run-template-edge.ps1: the Edge fallback never started the dev server.
  cmd.exe stripped the outer quotes of a redirected command line; wrapped
  in one `/s /c "..."` pair with inner path quotes doubled.
- inspect.ps1: StrictMode crash in the FSA scan — `$Dirs.Count` on $null.
  Changed to `if (-not $Dirs)`, which covers $null and @() alike.

Cherry-picked from PR #22 (80a3623, by Firnschnee). The third fix in that
PR (Invoke-PortSweep only counting a live recorded PID as "closed") is
intentionally dropped here: the ownership-proof rewrite in 89aaad2 removed
Invoke-PortSweep entirely, so a stale/dead recorded PID is already
reported as stale rather than stopped. Identity-token writes from 89aaad2
and the cmd fix coexist in run-template-edge.ps1.

Co-authored-by: Firnschnee <max.social@posteo.de>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Christian-Katzmann added a commit that referenced this pull request Jun 20, 2026
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Christian-Katzmann

Copy link
Copy Markdown
Owner

Landed on `main` in 501da59 (cherry-picked with `-x`, your authorship preserved) — thank you, @Firnschnee. The Edge-fallback `cmd /s /c` quoting fix and the `inspect.ps1` StrictMode fix both shipped as-is.

One note: the third hunk (`Invoke-PortSweep` only counting a live recorded PID as "closed") was intentionally dropped — a concurrent change (89aaad2) replaced the whole port-sweep cleanup with an ownership proof (a `server.identity` creation-time token, falling back to tree-owns-listener), so `Invoke-PortSweep`/`Get-PortOwner` no longer exist and a stale/dead recorded PID is already reported as stale rather than stopped. Your fix's intent is fully covered there. CHANGELOG updated. Closing since it's merged.

Christian-Katzmann added a commit that referenced this pull request Jun 20, 2026
Bump 0.1.0 -> 0.2.0 across marketplace.json (top-level + all three
plugin entries) and all six plugin.json manifests. Move the entire
CHANGELOG [Unreleased] block (static companion, doctor, verify, JSON
output, fixed-port, hosted-URL wrappers, native bootstrap, ownership-
safe cleanup, the Windows beta fixes) into a dated "## 0.2.0 -
2026-06-20" section; leave a fresh empty [Unreleased].

Reconcile the marketplace listing's Windows status line, the only
outlier: it still said "untested on real hardware" while README and the
CHANGELOG already reflect that real-hardware fixes (#8/#17/#18, #22/#23)
have landed. Calibrate the listing text (marketplace.json + its two
byte-identical app-it-windows plugin.json mirrors) to
"Beta - first real-hardware fixes landed - maintainer wanted" — true,
does not claim "tested", keeps the maintainer-wanted ask. The ~25
deliberate beta/maintainer-wanted placements in the deeper docs/ADR/
SKILL/contract files are left untouched by design.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants