diff --git a/.github/workflows/pipeline-orchestrator.yml b/.github/workflows/pipeline-orchestrator.yml index e0aabdb..fd486c0 100644 --- a/.github/workflows/pipeline-orchestrator.yml +++ b/.github/workflows/pipeline-orchestrator.yml @@ -159,12 +159,13 @@ jobs: for PR in $PR_NUMBERS; do echo "::group::Checking CI and reviews for PR #${PR}" - # Query PR state: CI status, labels, review threads + # Query PR state: CI status, labels, review threads, reviews PR_DATA=$(gh api graphql -f query=' query($owner: String!, $repo: String!, $pr: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { headRefName + headRefOid labels(first: 15) { nodes { name } } commits(last: 1) { nodes { commit { statusCheckRollup { state contexts(first: 30) { nodes { @@ -175,6 +176,11 @@ jobs: id isResolved comments(last: 1) { nodes { author { login } } } }} + latestReviews(first: 10) { nodes { + author { login } + state + commit { oid } + }} } } }' -f owner="$OWNER" -f repo="$REPO" -F pr="$PR") || { @@ -185,6 +191,7 @@ jobs: LABELS=$(echo "$PR_DATA" | jq -r '[.data.repository.pullRequest.labels.nodes[].name]') BRANCH=$(echo "$PR_DATA" | jq -r '.data.repository.pullRequest.headRefName') + HEAD_SHA=$(echo "$PR_DATA" | jq -r '.data.repository.pullRequest.headRefOid') # Check CI status — look for the "check" job specifically CI_CONCLUSION=$(echo "$PR_DATA" | jq -r ' @@ -286,12 +293,40 @@ jobs: echo " ✅ PR #${PR}: All threads resolved, CI passing." - # --- Step 4: Dispatch quality gate if not already approved --- - HAS_QG_APPROVED=$(echo "$LABELS" | jq -r 'if any(. == "aw-quality-gate-approved") then "yes" else "" end') + # --- Step 4: Dispatch quality gate if ready --- + # Check 1: Has Copilot reviewed the current head commit? + HAS_COPILOT_REVIEW=$(echo "$PR_DATA" | jq -r --arg sha "$HEAD_SHA" ' + [.data.repository.pullRequest.latestReviews.nodes[] + | select(.author.login == "copilot-pull-request-reviewer") + | select(.commit.oid == $sha)] + | length > 0') + + if [[ "$HAS_COPILOT_REVIEW" != "true" ]]; then + echo " ⏳ PR #${PR}: Waiting for Copilot review on current commit. Skipping quality gate." + echo "::endgroup::" + continue + fi + + # Check 2: Is a Copilot code review currently in progress for this commit? + COPILOT_REVIEW_RUNNING=$(gh run list --workflow="Copilot code review" --json headSha,status --jq --arg sha "$HEAD_SHA" '[.[] | select(.headSha == $sha) | select(.status == "in_progress" or .status == "queued")] | length' 2>/dev/null || echo "1") + if [[ "$COPILOT_REVIEW_RUNNING" -gt 0 ]]; then + echo " ⏳ PR #${PR}: Copilot review in progress. Skipping quality gate." + echo "::endgroup::" + continue + fi + + # Check 3: Is there a valid (non-dismissed) approval on the current commit? + HAS_VALID_APPROVAL=$(echo "$PR_DATA" | jq -r --arg sha "$HEAD_SHA" ' + [.data.repository.pullRequest.latestReviews.nodes[] + | select(.author.login == "${{ github.repository_owner }}") + | select(.state == "APPROVED") + | select(.commit.oid == $sha)] + | length > 0') + HAS_QG_EVALUATED=$(echo "$LABELS" | jq -r 'if any(. == "aw-quality-gate-evaluated") then "yes" else "" end') - if [[ -n "$HAS_QG_APPROVED" ]]; then - echo " ✅ PR #${PR}: Already quality-gate-approved. Auto-merge should handle it." + if [[ "$HAS_VALID_APPROVAL" == "true" ]]; then + echo " ✅ PR #${PR}: Valid quality-gate approval on current commit. Auto-merge should handle it." elif [[ -n "$HAS_QG_EVALUATED" ]]; then echo " ⏸️ PR #${PR}: Quality gate already evaluated (needs human review). Skipping." else diff --git a/docs/agentic-workflows.md b/docs/agentic-workflows.md index ef0e2b6..0602bcd 100644 --- a/docs/agentic-workflows.md +++ b/docs/agentic-workflows.md @@ -502,6 +502,18 @@ Branch protection's `dismiss_stale_reviews: true` dismisses the quality gate's A ### 27. Shared imports use `imports:` + `steps:` pattern To pre-fetch data before the agent runs, create a shared `.md` file with a `steps:` block in the frontmatter. The importing workflow uses `imports: [shared/filename.md]`. The steps run as regular workflow steps (with full `gh` CLI access and `GITHUB_TOKEN`), writing data to `/tmp/gh-aw/` for the agent to read. This bypasses MCP tool limitations. Based on the pattern from `github/gh-aw`'s own `copilot-pr-data-fetch.md`. +### 28. Don't dispatch quality gate until Copilot has reviewed the current commit +Copilot code review runs asynchronously. If the orchestrator dispatches the quality gate as soon as CI passes and threads are resolved, it can approve before Copilot reviews. Then Copilot's comments arrive after merge — too late. Always verify a Copilot review exists on the current head commit AND no Copilot review is in-progress before dispatching the quality gate. Use `latestReviews` in GraphQL to check review state and `gh run list` to check for active review runs. + +### 29. Check actual review approval state, not labels +Labels like `aw-quality-gate-approved` persist even after `dismiss_stale_reviews` invalidates the approval. Never use labels as the source of truth for review state. Query `latestReviews` and verify the APPROVE review is on the current head commit and not dismissed. Labels are hints for humans, not gates for automation. + +### 30. Copilot code review always submits as COMMENTED +GitHub Copilot code review never submits APPROVED or CHANGES_REQUESTED — always COMMENTED. This means: (1) Copilot reviews don't count toward required approvals, (2) they're never dismissed by `dismiss_stale_reviews`, (3) when checking if Copilot has reviewed, filter by author login, not by review state. The quality gate approval (from a PAT) is separate from Copilot's review — don't conflate them. + +### 31. Quality gate approval author is the PAT owner, not github-actions +The quality gate uses `GH_AW_WRITE_TOKEN` (a PAT) for `submit-pull-request-review`. The approval appears as the PAT owner (e.g., the repo owner), not as `github-actions[bot]`. Use `${{ github.repository_owner }}` to derive the identity at runtime — never hardcode usernames. This same mistake was made three times in this project (thread resolution, approval detection, changelog docs) before being codified as a rule. + --- diff --git a/docs/changelog.md b/docs/changelog.md index baff245..579f9f4 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -4,6 +4,21 @@ Append-only history of repo-level changes (CI, infra, shared config). Tool-speci --- +## fix: quality gate waits for Copilot review and checks actual approval state — 2026-03-21 + +**Problem 1 (#178)**: The orchestrator dispatched the quality gate as soon as CI was green and threads were resolved, without checking whether Copilot had reviewed yet. The quality gate approved PRs before Copilot's review arrived — then Copilot's comments went unaddressed because the PR was already approved/merged. + +**Problem 2 (#187)**: The orchestrator checked for the `aw-quality-gate-approved` label to skip re-dispatch. But `dismiss_stale_reviews: true` in branch protection dismisses the approval when new code is pushed — while the label persists. The orchestrator saw the stale label and skipped the quality gate, leaving the PR stuck with no valid approval. + +**Fix**: Replaced the label-based quality gate check with three verification steps: +1. **Copilot review exists** — check `latestReviews` for `copilot-pull-request-reviewer` on the current head commit. Skip if missing. +2. **Copilot review not in progress** — check `gh run list` for active "Copilot code review" runs. Skip if one is running. +3. **Valid approval check** — check `latestReviews` for a non-dismissed APPROVE from the quality gate's token owner (resolved via `github.repository_owner`) on the current head commit. Only skip dispatch if a valid approval exists. + +Fixes #178, #187. Follow-up: #203 (no fallback if Copilot review never runs). + +--- + ## fix: re-add labels config to implementer — 2026-03-21 **Problem**: The `labels: [aw]` config on `create-pull-request` was removed weeks ago due to a vague "node ID resolution error" that was never properly investigated. Without it, labeling depends on the agent including labels in its call — which is non-deterministic. Some PRs were created without the `aw` label.