From 8833a796f792dffd11cae60e0d19068a9eeb855a Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 1 Sep 2026 17:49:34 +0000 Subject: [PATCH] fix(ai-qa): stop dual delivery jobs; honor update-linked-issues on reopen Canonical caller is pull_request closed + issues closed. push remains a legacy merged-delivery alias and must not be combined with PR-closed. issues.closed reopen now respects update-linked-issues=false. Co-authored-by: Hicham Zinalabdin --- .github/workflows/ai-qa-selftest.yml | 2 -- ai-qa/README.md | 36 ++++++++++++------- ai-qa/action.yml | 7 ++-- ai-qa/lib/cleanup.js | 5 +++ ai-qa/lib/cleanup.test.js | 27 ++++++++++++++ ...-alignment-test-execution-and-writeback.md | 2 +- docs/consumer-integration.md | 10 ++++-- 7 files changed, 68 insertions(+), 21 deletions(-) diff --git a/.github/workflows/ai-qa-selftest.yml b/.github/workflows/ai-qa-selftest.yml index c85bea3..37092a2 100644 --- a/.github/workflows/ai-qa-selftest.yml +++ b/.github/workflows/ai-qa-selftest.yml @@ -1,8 +1,6 @@ name: ai-qa selftest on: - push: - branches: [main] pull_request: types: [closed] issues: diff --git a/ai-qa/README.md b/ai-qa/README.md index ef83cb9..4c18dc9 100644 --- a/ai-qa/README.md +++ b/ai-qa/README.md @@ -13,8 +13,8 @@ the head branch); this action owns the actual done signal: - **Issue-level.** On delivery **PASS**, **close** linked issues (turn off GitHub auto-close-on-merge so merge is not "done"). On **FAIL**, leave them open (reopen if GitHub already auto-closed them). On `issues.closed`, - reopen an issue closed before delivery was verified, unless this action - closed it or it is `not_planned`. + when `update-linked-issues` is on, reopen an issue closed before delivery + was verified, unless this action closed it or it is `not_planned`. An optional Claude review can still smoke-test the live app and evaluate a PR Test Plan; it is color on the report, not the reason this action exists. @@ -54,10 +54,12 @@ prompt.) 2. **Resolve author identity** — settles on one token and one identity string (`[bot]` or `github-actions[bot]`) reused by every later step. -3. **Route event** — `push` and `pull_request` closed+merged take the - delivery path; closed-without-merge only cleans the head branch; +3. **Route event** — `pull_request` closed+merged takes the delivery + path; closed-without-merge only cleans the head branch; `issues.closed` takes the premature-reopen path (PRs that fire as - issues are skipped). + issues are skipped). `push` is a legacy merged-delivery-only alias — + do not combine it with `pull_request: closed` or every merge runs + delivery twice. 4. **Resolve merged PR and merge commit** — on `push`, looks up the pull request(s) associated with `github.sha` via `GET /repos/{owner}/{repo}/commits/{sha}/pulls` (works across merge, @@ -113,7 +115,8 @@ prompt.) 10. **Closed-without-merge / premature issue-close** — a `pull_request` closed event that was not merged only deletes the head branch. An `issues.closed` event reopens the issue if it was closed before delivery - (linked merged PR without `✓ /ai-qa`), unless this action closed it. + (linked merged PR without `✓ /ai-qa`), unless this action closed it, + `update-linked-issues` is `false`, or it is `not_planned`. ## Inputs @@ -129,7 +132,7 @@ prompt.) | `pass-label` | Label applied when the overall QA signal (health + review) passes. Also applied to linked issues when `update-linked-issues` is on. | No | `✓ /ai-qa` | | `fail-label` | Label applied when the overall QA signal fails. Also applied to linked issues when `update-linked-issues` is on. | No | `✗ /ai-qa` | | `update-pr-body` | When `true`, the Publish step maintains a managed `` block in the merged PR's description reflecting the latest QA status. | No | `true` | -| `update-linked-issues` | When `true`, posts a sticky QA-status comment on each linked issue; on PASS **closes** the issue and applies `pass-label`; on FAIL leaves it open (reopens if GitHub auto-closed it) and applies `fail-label`. | No | `true` | +| `update-linked-issues` | When `true`, posts a sticky QA-status comment on each linked issue; on PASS **closes** the issue and applies `pass-label`; on FAIL leaves it open (reopens if GitHub auto-closed it) and applies `fail-label`. Also gates the `issues.closed` premature-reopen path. | No | `true` | | `cleanup-head-branch` | When `true`, delete the PR head after a delivery PASS, or immediately on close-without-merge. Requires `contents: write` on `GITHUB_TOKEN`. | No | `true` | | `anthropic-api-key` | Anthropic API key for the review step. Optional — without it, the review quietly no-ops and the report still publishes from the deploy-health signal alone. | No | — | | `anthropic-auth-token` | Bearer token for a custom Anthropic-compatible gateway, used instead of `anthropic-api-key`. | No | — | @@ -161,8 +164,6 @@ its own. Pass `test-hint` so the review knows this repo's build/test command. name: ai-qa on: - push: - branches: [main] pull_request: types: [closed] issues: @@ -196,16 +197,25 @@ Turn **off** GitHub auto-close (Settings → General → Issues → "Auto-close issues with merged linked pull requests") so merge is not treated as done; this action closes linked issues only after delivery PASS. +Do **not** also register `push` to the default branch. `push` is a +legacy merged-delivery-only alias for callers that cannot use +`pull_request: types: [closed]`. Combining both fires two full +delivery jobs on every merge (health, review, publish, cleanup) that +race on issues and labels. Prefer the `pull_request` + `issues` pair +above so close-without-merge and premature-issue-close hygiene also +run. + `issues: write` is required because comments, labels, close, and reopen go through the Issues API. `contents: write` is required to delete the head branch via `GITHUB_TOKEN` (the App author token is not granted Contents). ## Self-test -`.github/workflows/ai-qa-selftest.yml` runs this action on push to `main`, -`pull_request` closed, and `issues` closed. The merge path uses a trivial -`health-url` (this repo's raw `README.md`) so the report/label/close/cleanup -pipeline is exercised without a real deploy. +`.github/workflows/ai-qa-selftest.yml` runs this action on `pull_request` +closed and `issues` closed (not `push` — that would duplicate merged +delivery). The merge path uses a trivial `health-url` (this repo's raw +`README.md`) so the report/label/close/cleanup pipeline is exercised +without a real deploy. To exercise the failure path (a deliberately broken or unreachable health-url timing out rather than hanging the job) or the sticky-comment diff --git a/ai-qa/action.yml b/ai-qa/action.yml index 00e278d..f205bad 100644 --- a/ai-qa/action.yml +++ b/ai-qa/action.yml @@ -101,7 +101,8 @@ inputs: closes the issue (this action owns "done" — turn off GitHub auto-close) and applies `pass-label`; on a FAIL it leaves the issue open (and reopens it if GitHub already auto-closed it) and applies `fail-label`. - Set to 'false' to leave linked issues untouched. + Also gates the `issues.closed` premature-reopen path. Set to 'false' + to leave linked issues untouched. required: false default: "true" cleanup-head-branch: @@ -967,7 +968,7 @@ runs: } - name: Reopen prematurely closed issue - if: steps.route.outputs.mode == 'issue-closed' + if: steps.route.outputs.mode == 'issue-closed' && inputs.update-linked-issues == 'true' uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8.0.0 env: ISSUE_NUMBER: ${{ steps.route.outputs.issue-number }} @@ -976,6 +977,7 @@ runs: STATE_REASON: ${{ steps.route.outputs.state-reason }} ISSUE_LABELS: ${{ steps.route.outputs.issue-labels }} PASS_LABEL: ${{ inputs.pass-label }} + UPDATE_LINKED_ISSUES: ${{ inputs.update-linked-issues }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} CLEANUP_PATH: ${{ github.action_path }}/lib/cleanup.js GH_TOKEN: ${{ steps.identity.outputs.author-token }} @@ -1052,6 +1054,7 @@ runs: hasPassLabel, stateReason: process.env.STATE_REASON || '', linkedUndeliveredPr: linkedUndeliveredPr(prs, passLabel), + updateLinkedIssues: process.env.UPDATE_LINKED_ISSUES, }); core.info(`Issue #${issueNumber} premature-close: ${decision.reason}`); if (!decision.reopen) return; diff --git a/ai-qa/lib/cleanup.js b/ai-qa/lib/cleanup.js index 6de6a5f..78877d8 100644 --- a/ai-qa/lib/cleanup.js +++ b/ai-qa/lib/cleanup.js @@ -109,11 +109,16 @@ function shouldCloseIssue(opts) { * hasPassLabel?: boolean, * stateReason?: string|null, * linkedUndeliveredPr?: boolean, + * updateLinkedIssues?: boolean|string, * }} opts * @returns {{ reopen: boolean, reason: string }} */ function shouldReopenPrematureClose(opts) { const o = opts || {}; + // Explicit false only — omit/undefined keeps the action.yml default (on). + if (o.updateLinkedIssues === false || o.updateLinkedIssues === "false") { + return { reopen: false, reason: "update-linked-issues-off" }; + } if (o.closedByThisAction) { return { reopen: false, reason: "closed-by-this-action" }; } diff --git a/ai-qa/lib/cleanup.test.js b/ai-qa/lib/cleanup.test.js index 72ff4e8..5fe7f3e 100644 --- a/ai-qa/lib/cleanup.test.js +++ b/ai-qa/lib/cleanup.test.js @@ -194,6 +194,33 @@ test("does not reopen when no linked undelivered PR exists", () => { assert.equal(d.reason, "no-undelivered-pr"); }); +test("does not reopen when update-linked-issues is off", () => { + const base = { + closedByThisAction: false, + hasPassLabel: false, + stateReason: "completed", + linkedUndeliveredPr: true, + }; + const off = shouldReopenPrematureClose({ + ...base, + updateLinkedIssues: false, + }); + assert.equal(off.reopen, false); + assert.equal(off.reason, "update-linked-issues-off"); + + const offStr = shouldReopenPrematureClose({ + ...base, + updateLinkedIssues: "false", + }); + assert.equal(offStr.reopen, false); + assert.equal(offStr.reason, "update-linked-issues-off"); + + // Omit the flag (and the action.yml default of true) — still reopen. + const omitted = shouldReopenPrematureClose(base); + assert.equal(omitted.reopen, true); + assert.equal(omitted.reason, "closed-before-delivery"); +}); + // --- linkedUndeliveredPr ---------------------------------------------------- test("linkedUndeliveredPr is true when a merged PR lacks the pass label", () => { diff --git a/docs/adr/0003-intent-alignment-test-execution-and-writeback.md b/docs/adr/0003-intent-alignment-test-execution-and-writeback.md index da93992..d821688 100644 --- a/docs/adr/0003-intent-alignment-test-execution-and-writeback.md +++ b/docs/adr/0003-intent-alignment-test-execution-and-writeback.md @@ -81,7 +81,7 @@ canonical skill. **reopens** it if GitHub already auto-closed it. It also maintains a `` block in the PR body. A separate `issues.closed` path reopens an issue closed before delivery was verified, unless this - action closed it or it is `not_planned`. + action closed it, `update-linked-issues` is off, or it is `not_planned`. - Both are gated by `update-pr-body` / `update-linked-issues` (default `true`) and are wrapped so any write failure warns without failing the job. diff --git a/docs/consumer-integration.md b/docs/consumer-integration.md index d5edbbb..e386449 100644 --- a/docs/consumer-integration.md +++ b/docs/consumer-integration.md @@ -180,8 +180,6 @@ toolchain it might need *before* the action: name: ai-qa on: - push: - branches: [main] pull_request: types: [closed] issues: @@ -221,7 +219,13 @@ label — both go through the Issues API, hence `issues: write`. That same scope covers close/reopen of linked issues. `contents: write` is required to delete the head branch via `GITHUB_TOKEN`. With `update-linked-issues` left on, a PASS **closes** each linked issue; a FAIL leaves it open (and reopens -it if GitHub already auto-closed it). The Anthropic credential is optional +it if GitHub already auto-closed it). The same flag also gates the +`issues.closed` premature-reopen path. Do **not** also register `push` to +the default branch alongside `pull_request: closed` — both take the +merged-delivery path, so every merge would run two full jobs that race +on issues and labels. `push` remains a legacy merged-delivery-only alias +for callers that cannot use `pull_request: types: [closed]`. The Anthropic +credential is optional (see section 1) — omit it and `ai-qa` still reports deploy health and runs hygiene, just without the agentic review.