test: probe custom Windows GPU runner - #9652
Conversation
This comment has been minimized.
This comment has been minimized.
|
New build in progress, come back later! |
pentreathm
changed the title
🚦 CI Status
Build skipped — no changes detected under Waiting for lint to start… Waiting for tests to start… |
Security review —
|
|
Claude finished @pentreathm's task in 1m 46s —— View job Security re-review after
|
| Prior finding | Status in 3b8a91398 |
Evidence |
|---|---|---|
HIGH — pull_request trigger onto a custom runner |
Resolved | The pull_request: block is gone. Triggers are now workflow_dispatch + push restricted to chore/probe-windows-image-builder (:3-9). No fork-controlled path onto the self-hosted runner; only users with push access can start it. |
MEDIUM — Stop + trailing exit $LASTEXITCODE making the probe flaky |
Largely resolved | Get-ComputerInfo is now a lone piped statement (:42-44), the fragile Win32_DesktopMonitor / Get-Volume -DriveLetter C calls are gone, and $global:LASTEXITCODE = 0 (:60) neutralizes the GPU step's exit code. One residue below. |
LOW — unbounded recursive scans of both Program Files trees |
Resolved | Replaced with an exact-path hashtable (:67-75). The only remaining -Recurse is scoped to C:\Tools\gh (:90), which is small. |
LOW — AdapterRAM misreporting a 16 GB T4 |
Resolved | Dropped; nvidia-smi --query-gpu=name (:53) is now the source of truth. |
LOW — Win32_DesktopMonitor ambiguous on headless |
Resolved | Replaced by an explicit \bWDDM\b assertion on nvidia-smi output (:56), which answers the actual question decisively. |
LOW — no concurrency: block |
Resolved | :13-15, cancel-in-progress: true, matching test.yml / visual-regression.yml convention. |
LOW — contents: read broader than needed |
Resolved | Now permissions: {} (:11). |
STEP W re-run against the final tree: no secrets.*, no uses: at all, no actions/checkout, no pull_request_target, no LLM invocation, no artifact upload, no snapshot key. W.1–W.10 are N/A or satisfied. timeout-minutes: 10 (:21) bounds the hold on a scarce GPU runner.
Remaining — LOW, robustness not security
1. :96-103 — partial toolchain failures pass silently. Under $ErrorActionPreference = "Stop", a native command's non-zero exit does not throw; it only sets $LASTEXITCODE. GitHub appends exit $LASTEXITCODE, so the step's result is decided solely by the last invocation, & $expectedTools.mf --version (:103). If aws --version (:102) fails but mf succeeds, the step goes green and the probe reports a healthy toolchain it did not verify. Conversely, a non-zero mf --version fails the step with a bare Process completed with exit code N and no indication of which tool broke. Checking $LASTEXITCODE after each invocation — the pattern already used correctly at :50 and :54 — makes each tool's result explicit.
2. :49 — 2>&1 on a native command under Stop can terminate before the intended error message. In Windows PowerShell 5.1, merging a native command's stderr into the pipeline turns those lines into NativeCommandError records; with $ErrorActionPreference = "Stop" that becomes terminating. So if nvidia-smi writes any warning to stderr, the step dies at :49 with a PowerShell exception instead of the "nvidia-smi failed with exit code …" message at :50 that was written to explain it. A healthy nvidia-smi writes nothing to stderr, so this only bites in exactly the failure case the probe is meant to diagnose.
3. :53-54 — the GPU-name query has a null path and an unreliable exit code. If nvidia-smi --query-gpu=name emits no rows, Select-Object -First 1 yields $null and .Trim() throws a NullReferenceException before the guarded check at :55. Separately, Select-Object -First 1 tears down the upstream pipeline early, so $LASTEXITCODE at :54 may be stale from the :49 invocation rather than reflecting this one. Assigning to a variable first and testing for null/empty before .Trim() covers both.
4. :114-122 — exact-match env comparison is brittle. [Environment]::GetEnvironmentVariable(…, "Machine") returns the expanded string; a value stored as REG_EXPAND_SZ or with a trailing backslash fails -ne against the literal even though the cache is configured correctly. Not wrong today, just a likely source of a confusing red run later.
5. :80-88 — hashtable enumeration order is unspecified, so the summary table's row order varies between runs. Cosmetic.
6. Dead trigger after merge. The push filter names chore/probe-windows-image-builder (:7), which disappears on merge, and there is no pull_request trigger anymore — so post-merge this file is inert but lingers in dev. Since validation happens on the branch via push / workflow_dispatch anyway, not merging it is the cleanest outcome; otherwise please open a tracking issue for removal.
Verdict
The one blocking-class concern from my previous pass — fork PRs reaching the custom GPU runner through the pull_request trigger — is gone, and it was fixed by removing the trigger rather than by patching around it, which also moots the org-settings questions (runner-group public-repo access, fork-approval policy) that I couldn't read. permissions: {}, a bounded timeout, a concurrency group, no secrets, no checkout, no third-party actions, and no image snapshot: the attack surface is now essentially nil.
Everything left is probe reliability on a scarce GPU runner, not security. Finding 1 is the one I'd actually act on, since a silently-green toolchain check defeats the purpose of the probe.
DEPENDENCY_REVIEW: PASS
No files changed — I can't modify .github/workflows/ (GitHub App permissions don't allow workflow modifications), so any of the above would need to be applied by a human. See the FAQ for workarounds.
· Branch chore/probe-windows-image-builder
1 participant
Summary
win-gpu-t4-explorerTemporary workflow used to validate the new custom image before wiring the heavy automation suite.