Repository navigation
Add least-privilege failure reporting and systematic scenario matrix to Windows CLI integration - #67410
Conversation
…to Windows CLI integration - conclusion job now creates (or comments on an existing open) tracking issue naming failed jobs and linking the run when build/integration fails, scoped to issues:write on this job only. - scenarioMatrix is generated from declared shell/launch-mode/env-shape/ path-style dimension arrays instead of a hand-picked list, with bounded growth (dimension-pair coverage) to avoid blowing the job timeout. - per-scenario failures are now tagged with the scenario name for faster triage. Fixes #66822 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deterministic execution of the scenario-generation logic (via PowerShell AST parser + actual loop execution in a container, not just brace counting) caught a genuine duplicate: dimension pair 1 already produces $representativeShell/path/default/workspace-first (its "path" launch mode always resolves to the workspace-first PATH style), but dimension pair 2's skip condition only excluded env=default+path=default, not env=default+path=workspace-first. Fixed the skip condition so the 33 generated scenarios are now verified unique, with full shell x launch and env x path coverage confirmed by execution. Fixes #66822 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review. PR review automation could not emit write-intents from this sandbox because safeoutputs review commands returned a permission-denied approval gate with no responder.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR #67410 has no 'implementation' label and 0 new lines in business logic directories (1 file changed, threshold 100).
|
There was a problem hiding this comment.
🟡 Changes recommended
Default-PATH scenarios use PATH launch without placing the binary on PATH, causing clean-runner failures.
1 open finding
What changed in this PR
Adds systematic Windows CLI scenario coverage and least-privilege failure issue reporting.
Changes:
- Generates an additive shell/environment/PATH scenario matrix.
- Adds scenario-specific failure diagnostics.
- Creates or updates a failure-tracking issue.
| File | Description |
|---|---|
.github/workflows/windows-cli-integration.yml |
Expands integration coverage and adds failure reporting. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes: the new failure-reporting step has a shell syntax bug that will make it fail every time it's actually needed.
📋 Key Themes & Highlights
Key Themes
- Blocking bug: the
Create or update failure issuestep's heredoc (BODY=$(cat <<EOF ... EOF)) uses an indentedEOFterminator, which bash does not recognize as a valid closing delimiter unless<<-EOFwith tab indentation is used. Verified withbash -nandshellcheckagainst the extracted step: both report a fatal parse error (SC1073/SC1039/SC1072). Since this step only runsif: steps.evaluate.outputs.failed == 'true', the bug is silent until the first real failure — exactly the scenario this PR exists to make work. - The PR description states
bash -n+shellcheckwere run on "the extracted ... bash step: clean" — that claim doesn't match what's committed, so the extracted snippet used for validation likely differed from the final indentation.
Positive Highlights
- ✅ Scoping
issues: writeto only theconclusionjob (notbuild/integration) is the right least-privilege shape for this permission. - ✅ The scenario-matrix generation refactor (hand-picked list → declared dimension arrays) is a solid
/codebase-designimprovement — it make the coverage rationale explicit via dimension pairing instead of an opaque literal list, and the PR already caught/fixed a real duplicate-scenario bug (dimension pair 2 overlap) during their own validation, which I independently re-derived (33/33 unique names) against the committed dimension arrays. - ✅ Wrapping the per-scenario body in
try/catchwith the scenario name re-thrown gives much better triage signal than the prior bare error. - ✅ Dedup-by-search-and-comment on the existing open issue (vs. creating a new one per failure) addresses the exact issue (#66441) that was duplicated five times.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 61.7 AIC · ⌖ 13.6 AIC · ⊞ 10.1K
Comment /matt to run again
There was a problem hiding this comment.
Applied Impeccable harden (missing error/edge-state verification) since this PR explicitly adds failure-path reporting; found one blocking issue via reproduction, no other blocking findings in the scenario-matrix refactor (PowerShell generation logic matches the PR's stated/validated behavior).
Blocking: The BODY=$(cat <<EOF ... EOF\n) heredoc in the new "Create or update failure issue" step has its closing EOF indented (matching the surrounding YAML block), which bash does not accept as a terminator without <<-. I reproduced this by extracting the exact step body and running it with bash: it fails with a syntax error (unexpected EOF while looking for matching ')'), so the entire dedupe/create logic — the core feature of this PR — never executes on a real failure.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 61.1 AIC · ⌖ 13.4 AIC · ⊞ 8.2K
copilot-pull-request-reviewer flagged that dimension pair 2's generated scenarios paired every path style (including "default") with launch="path". For path="default" the PATH-rearrangement switch is a no-op, leaving PATH unmodified, so the workspace copy of gh-aw.exe is never resolvable via `where gh-aw` on a clean runner -- those scenarios would fail before invoking the CLI at all. Fix: path="default" now pairs with launch="direct" (consistent with dimension pair 1's own default/default scenario, since PATH rearrangement has no effect on a direct invocation anyway); every other path style still pairs with launch="path". Updated the duplicate-skip condition accordingly (now skips both combinations dimension pair 1 already covers for the representative shell), yielding 32 unique scenarios (down from 33, since one more overlap is now correctly excluded). Verified via actual PowerShell execution in a container (not just static review): scenario count, name uniqueness, full shell x launch and env x path coverage, and a new explicit regression check that no launch="path" scenario has path="default". Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes #66822.
Problem
The
conclusionjob in.github/workflows/windows-cli-integration.ymlhad onlycontents: readand, on failure, wrote a step summary andexit 1— no issue was created. This was a deliberate regression: PR #65929 removed a workinggh issue createstep to avoid grantingissues: write. Five duplicate issues (#66016, #66441, #66780, #67109, #67341) re-requested this and a systematic scenario matrix; the hand-picked 20-entry matrix combined env-shape variants almost exclusively withpwsh-default/workspace-first, leavingcmdand most path styles untested against environment variation.Changes
conclusionjob: addsissues: write, scoped to this job only (build/integration remaincontents: read). A new "Create or update failure issue" step, gated on failure, searches for an existing open tracking issue by title and comments on it instead of creating a new one each day (dedupe per [windows-integration] Add failure issue creation to Windows CLI integration workflow #66441), else creates one with the failed job names and run URL.$scenarioMatrixis now generated from declared$shells × $launchModes × $envShapes × $pathStylesarrays instead of hand-picked literals. Every shell is exercised both directly and via PATH resolution; every env-shape is exercised across every path-style via a fixed representative shell/launch, so growth is additive (32 scenarios) rather than a full 240+ cross product that would risk the job timeout.Review feedback addressed
pathstyle, including"default", withlaunch="path". Since the"default"style leavesPATHuntouched,where gh-awcould never resolve the workspace-copied binary for those scenarios on a clean runner — they would fail before the CLI was even invoked. Fixed:path="default"now pairs withlaunch="direct"(PATH rearrangement has no effect on a direct invocation anyway), and the duplicate-skip condition was updated to exclude both combinations dimension pair 1 already covers for the representative shell. Net scenario count is now 32 (was 33).run: |) indentation stripping, which removes the common leading indentation from every line of the block uniformly — including the heredoc terminator. Confirmed via two independent YAML parsers (Ruby Psych, Python PyYAML) that the actual parsed value hasEOFat column 0, and via real Ubuntu 22.04/bash 5.1.16 execution (matchingubuntu-latest, exact GH Actions shell invocation) that the true executed script is syntactically valid and runs end-to-end correctly, including the dedupe create/comment paths. No code change was needed for this part.Validation
Static checks:
ruby -ryaml/ directactionlinton the file: no issues.bash -n+shellcheckon the extracted "Create or update failure issue" bash step: clean.make agent-report-progress(build + impacted Go tests + schema freshness): passed.Deep validation, using a local Docker/colima
mcr.microsoft.com/powershell:latest(amd64, emulated) container and real Ubuntu 22.04 bash — not brace-counting or static guessing:[System.Management.Automation.Language.Parser]::ParseFile): 0 syntax errors (1654 tokens after the PATH-mismatch fix).(shells×launchModes) + (envShapes×pathStyles) − 2).(shell, launch)pairs and all 22 non-skipped(env, path)pairs appear at least once.launch="path"haspath="default"(the bug class the reviewer flagged).env=default,path=default, missing thatenv=default,path=workspace-firstwas already produced by dimension pair 1 — that overlap is now also correctly excluded.ghbinary in real bash 5.1 covering 5 scenarios — no existing issue (→ creates), existing issue found (→ comments),gh issue listfailing (→ aborts viaset -euo pipefail, no silent fallback to create),gh issue commentfailing (→ aborts),gh issue createfailing (→ aborts). All 5 behaved as expected; the step correctly fails loudly rather than silently succeeding on anygherror.Explicitly NOT verified (cannot be, without triggering Actions):
windows-latestrunner execution/timing of the 32-scenario matrix (wall-clock budget was reasoned about, not measured).gh issue list --search "in:title \"...\""reliably matching the exact dedupe title against real API behavior.Recommend observing the next scheduled run (06:00 UTC) or a maintainer-triggered
workflow_dispatchfor final confirmation of runtime and the dedupe path; no workflow trigger was requested or performed by this session.Co-authored-by: Copilot App 223556219+Copilot@users.noreply.github.com