fix: run drift check scripts in steps: where gh CLI is authenticated#662
fix: run drift check scripts in steps: where gh CLI is authenticated#662
Conversation
Adds /fix slash command for iterative review/fix cycles on PRs: - 3-model adversarial review (Opus + Sonnet + Codex) - Auto-fix all findings (CRITICAL, MODERATE, MINOR) - Re-review after fixes, up to 3 rounds - Push all commits via push-to-pull-request-branch safe output - Cross-platform verification via verify-build.yml Also adds: - review-retro.agent.md: Post-merge review retrospective - verify-build.yml: Cross-platform build + test workflow - noop safe-output for clean PRs All 9 review findings from PR #640 addressed: - workflow_dispatch checkout, concurrency groups, target: *, --no-restore removed, explicit round counter, scoped re-review diff, inline validation aligned, cancel-in-progress documented Supersedes #640. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…henticated Root cause: Check-Staleness.ps1 calls gh api/gh issue view which require GH_TOKEN. Inside the gh-aw agent container, credentials are scrubbed. The script was failing silently, causing the agent to never reach the noop call path. Fix: Move script execution to steps: (pre-activation job) where GH_TOKEN is available. Write results to workspace files that the agent reads as pre-computed data. Diagnosed by 3-model consensus (Opus, Sonnet, Codex all agreed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Expert Code Review — PR #662
Summary
| # | Severity | Consensus | File | Finding |
|---|---|---|---|---|
| 1 | 🔴 CRITICAL | 2/3 | instruction-drift.agent.md |
-OutputFile parameter doesn't exist on Check-Staleness.ps1 or Scan-GhAwUpdates.ps1 — workflow will fail on every run |
| 2 | 🔴 CRITICAL | 3/3 | review-retro.agent.md |
Agent instructed to use gh CLI, but credentials are scrubbed inside agent container |
| 3 | 🟡 MODERATE | 2/3 | review-retro.agent.md |
Uses pull_request trigger — explicit gh-aw anti-pattern causing "Approve and run" gate |
| 4 | 🟡 MODERATE | 2/3 | fix.agent.md |
workflow_dispatch checkout doesn't use Checkout-GhAwPr.ps1 — untrusted .github/ exposed |
| 5 | 🟢 MINOR | 2/3 | fix.agent.md |
Concurrency comment says "/fix cancels /review" but cancel-in-progress: false means it queues |
Discarded Findings (single reviewer only, validated by follow-up)
fix.agent.mdpermissions too restrictive for safe-outputs — Discarded. Follow-up confirmed gh-aw compiler generates a separatesafe_outputsjob with full write permissions automatically. The source.mdpermissions correctly control only the agent job.verify-build.ymlunsanitized${{ inputs.pr_number }}— Discarded.type: numberprovides server-side validation at the GitHub API layer; injection is not possible. The env-var pattern would be better hygiene but is not a vulnerability.actions/checkout@v6doesn't exist — Discarded. v6 is valid and used consistently across all workflows in this repo (confirmed via lock file SHA pins:actions/checkout@v6.0.2).- 6 additional single-reviewer findings — Discarded without follow-up per adversarial consensus protocol (cap: 3 follow-ups).
CI & Test Coverage
- Lock files: Both new workflows (
fix.agent.lock.yml,review-retro.agent.lock.yml) and the modifiedinstruction-drift.agent.lock.ymlappear to be compiler-generated (gh-aw v0.62.2). Source changes are accompanied by recompiled lock files. - verify-build.yml: Standard GitHub Actions workflow (not gh-aw). Tests, Mac Catalyst, and Windows builds with proper matrix.
- No application code changes — this PR only modifies workflow definitions. No unit test impact.
Prior Reviews
No prior automated reviews detected on this PR.
Methodology
3 independent reviewers with adversarial consensus. Findings flagged by only 1/3 reviewers were validated by 2 follow-up reviewers. Findings that failed follow-up validation were discarded. All reviews used COMMENT event type per gh-aw stale-review policy.
Generated by Expert Code Review (auto) for issue #662
| GH_TOKEN: ${{ github.token }} | ||
| run: | | ||
| echo "::group::Check-Staleness.ps1" | ||
| pwsh .github/skills/instruction-drift/scripts/Check-Staleness.ps1 \ |
There was a problem hiding this comment.
🔴 CRITICAL — -OutputFile parameter does not exist on either script (Flagged by: 2/3 reviewers)
Check-Staleness.ps1 declares param([string]$ManifestPath, [string]$RepoRoot) — no -OutputFile. PowerShell will throw:
A parameter cannot be found that matches parameter name 'OutputFile'.
The step fails, staleness-report.json is never created, and the entire workflow job fails before the agent starts. The same issue exists for Scan-GhAwUpdates.ps1 (line 52) which declares param([string]$WatermarkFile, [int]$MaxCommits, [switch]$DryRun) — also no -OutputFile. The || true on line 55 suppresses the error for the scan script, but scan-report.json is never written either.
Fix: Either add -OutputFile parameters to both scripts, or redirect stdout:
pwsh .github/skills/instruction-drift/scripts/Check-Staleness.ps1 > staleness-report.json| Collect all review activity on this PR: | ||
|
|
||
| ```bash | ||
| # PR metadata |
There was a problem hiding this comment.
🔴 CRITICAL — Agent is instructed to use gh CLI, but credentials are scrubbed inside the agent container (Flagged by: 3/3 reviewers)
This PR's own commit message (692fb97) documents that gh CLI credentials are scrubbed inside the agent container — that's the exact reason the instruction-drift workflow was refactored to use steps:. The retro workflow has the same fundamental problem: all gh pr view, gh api, gh pr diff calls will return "authentication required" errors.
Additionally, the workflow has no tools: github: toolsets: section to provide MCP alternatives.
Fix: Either:
- Move data gathering to
steps:(pre-agent) like the instruction-drift fix in this same PR, or - Add
tools: github: toolsets: [repos, pull_requests]and rewrite the prompt to use MCP tools (get_pull_request,get_pull_request_diff, etc.) instead ofghCLI.
|
|
||
| on: | ||
| pull_request: | ||
| types: [closed] |
There was a problem hiding this comment.
🟡 MODERATE — pull_request trigger is an explicit gh-aw anti-pattern (Flagged by: 2/3 reviewers)
From .github/instructions/gh-aw-workflows.instructions.md:
Using
pull_requesttrigger for agentic workflows →slash_command:orschedule—pull_requestcauses the 'Approve and run' gate for ALL workflows
Every fork contributor's PR will trigger the "Approve and run" gate for ALL workflows in the repo, degrading the review experience.
Fix: Consider a schedule-based approach (scanning recently merged PRs), or a slash_command: name: retro that maintainers invoke manually. If pull_request: [closed] is the only viable option, add a comment documenting why the anti-pattern is accepted.
| PR_NUMBER: ${{ inputs.pr_number }} | ||
| run: | | ||
| echo "::group::Checkout PR #$PR_NUMBER" | ||
| gh pr checkout "$PR_NUMBER" --recurse-submodules |
There was a problem hiding this comment.
🟡 MODERATE — workflow_dispatch checkout should use Checkout-GhAwPr.ps1 (Flagged by: 2/3 reviewers)
The gh-aw instructions require:
Use
Checkout-GhAwPr.ps1forworkflow_dispatchworkflows that check out a PR — it verifies write access and restores trusted.github/from base branch.
The current gh pr checkout leaves PR-head .github/ in place. If a PR contains a modified .github/copilot-instructions.md with injected instructions, the agent would read attacker-controlled instructions.
Fix: Replace with Checkout-GhAwPr.ps1 which restores the trusted .github/ from the base branch after checkout.
| pull-requests: read | ||
|
|
||
| # Intentional: shared prefix with review workflows — /fix cancels in-progress /review. | ||
| concurrency: |
There was a problem hiding this comment.
🟢 MINOR — Concurrency comment contradicts the actual behavior (Flagged by: 2/3 reviewers)
The comment says: "shared prefix with review workflows — /fix cancels in-progress /review"
But cancel-in-progress: false means /fix queues behind a running review — it does not cancel it.
Fix: Update comment to: "Shared group with review workflows — /fix queues behind a running /review on the same PR."
Third iteration fixing the instruction-drift workflow. **Problem chain:** 1. PR #653: Scripts ran inside agent container → gh CLI not authenticated → scripts failed 2. PR #662: Moved to `steps:` (activation job) → scripts ran with GH_TOKEN ✅ but files written to activation filesystem aren't available in agent job (different runner) 3. **This PR:** Pass data via `$GITHUB_OUTPUT` → template substitution inlines JSON directly into the agent prompt **How it works now:** ``` activation job (steps:, has GH_TOKEN): ├── Check-Staleness.ps1 → $GITHUB_OUTPUT (changes_detected + report JSON) └── Scan-GhAwUpdates.ps1 → $GITHUB_OUTPUT (upstream JSON, only if stale) ↓ template substitution agent prompt (data inlined, no file I/O): ├── changes_detected: false → call noop immediately └── changes_detected: true → analyze + create PR ``` The agent sees the actual JSON data right in its prompt — no `cat` commands, no file paths, no filesystem dependency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Root Cause
Check-Staleness.ps1 calls
gh apiandgh issue viewwhich requireGH_TOKEN. Inside the gh-aw agent container, credentials are scrubbed. The script failed silently on issue/release checks, producing error output that prevented the agent from reaching thenoopcall path.Diagnosed by 3-model consensus (Opus, Sonnet, Codex).
Fix
Move script execution from the agent prompt to
steps:(pre-activation job) whereGH_TOKENis available. The agent reads pre-computed JSON results instead of running the scripts.How It Works Now