diff --git a/.qwen/skills/triage/SKILL.md b/.qwen/skills/triage/SKILL.md index 47479b0b691..b77b944f47a 100644 --- a/.qwen/skills/triage/SKILL.md +++ b/.qwen/skills/triage/SKILL.md @@ -48,9 +48,13 @@ gh label list --repo "$REPO" --limit 200 defined in this skill's files. If a concern about scale or scope arises, raise it as a question in the Stage 1 comment — never as a block or CHANGES_REQUESTED. The escalation criteria are those defined in `references/pr-workflow.md` - (Stage 0, Stage 1b, and Stage 1c). Escalation means notifying the + (Stage 0, Stage 1-pre, Stage 1b, and Stage 1c). Escalation means notifying the maintainer, not rejecting the PR, except where Stage 0 Tier 1 explicitly - prescribes a `CHANGES_REQUESTED` review for large core refactors. + prescribes a `CHANGES_REQUESTED` review for large core refactors, where + Stage 1-pre prescribes a `CHANGES_REQUESTED` review for a linked issue + closed as not planned or a remaining delta against a merged fix, or where + Stage 1-pre prescribes closing a default-branch PR whose entire diff is + fully subsumed by a merged fix for its linked issue. - ⛔ **Never execute PR-derived code.** The review is static. Do not run `npm`/`node`/`npx`/interpreters/build/test commands against a tree containing the PR's changes; do not `gh pr checkout`, `git apply` the diff, or run any diff --git a/.qwen/skills/triage/references/pr-workflow.md b/.qwen/skills/triage/references/pr-workflow.md index cd122a165e1..bac83598f6d 100644 --- a/.qwen/skills/triage/references/pr-workflow.md +++ b/.qwen/skills/triage/references/pr-workflow.md @@ -21,15 +21,19 @@ COMMENT_ID=$(gh api "repos/$REPO/issues/$PR_NUMBER/comments" -F body=@/tmp/stage **Terminal gate exception:** if any terminal exit triggers (Stage 0 core module hard block, Stage 1a template failure, Stage 1b problem-does-not-exist, -or Stage 1c direction escalation), submit exactly one `CHANGES_REQUESTED` -review and stop. Do not also post or update a Stage 1 issue comment, and do not -continue to Stage 2, Stage 3, or approval. +Stage 1c direction escalation, or Stage 1-pre's two request-changes exits — +linked issue closed as not planned, or a remaining delta against a merged +fix), submit exactly one `CHANGES_REQUESTED` review and stop. Do not also post +or update a Stage 1 issue comment, and do not continue to Stage 2, Stage 3, or +approval. The Stage 1-pre duplicate-close exit is different: it posts the +terminal `stage=1-pre` comment and closes the PR instead of submitting a +review. **Re-runs:** if the triage runs again on the same PR, update each comment in place. **Resolve the comment id by its stage marker AT PATCH TIME — never from memory, list position, or an earlier stage's bookkeeping.** On a re-run the thread holds four or more bot comments whose list order is not the stage order, and a wrong id silently overwrites another stage's comment (observed on a real re-run: the stage=3 comment clobbered with stage=1 content mid-run). The author filter matters too — the marker is public text anyone can paste into a comment, and the bot PAT may be able to edit other users' comments: ```bash BOT_LOGIN=$(gh api user --jq '.login') -stage_comment_id() { # $1 = stage number (1, 2, 3) or "status" +stage_comment_id() { # $1 = stage number (1, 2, 3), "1-pre", or "status" gh api "repos/$REPO/issues/$PR_NUMBER/comments" --method GET --paginate -F per_page=100 | jq -rs --arg bot "$BOT_LOGIN" --arg m "" \ '[.[][] | select(.user.login == $bot) | select(.body | startswith($m))] | last | .id // empty' @@ -75,7 +79,7 @@ Every staged comment (Stage 1 gate-pass, Stage 2, Stage 3) ends with the signatu Reviewed at `` · re-run with `@qwen-code /triage` ``` -**If `HEAD_SHA` comes back empty** (API failure or a null `headRefOid`): **fail closed.** Do not PATCH an existing staged comment — the update rewrites the whole body, so a dropped footer erases the previously valid `Reviewed at` line just as an empty-backtick footer would. Retry the capture, or leave the prior comment (with its footer) untouched until a full OID is available; only a brand-new post that never had a footer may go out without one. Terminal-gate reviews (Stage 1a/1b/1c, submitted via `gh pr review --request-changes`) use the signature only — no footer; they reject before a real review pass. +**If `HEAD_SHA` comes back empty** (API failure or a null `headRefOid`): **fail closed.** Do not PATCH an existing staged comment — the update rewrites the whole body, so a dropped footer erases the previously valid `Reviewed at` line just as an empty-backtick footer would. Retry the capture, or leave the prior comment (with its footer) untouched until a full OID is available; only a brand-new post that never had a footer may go out without one. Terminal-gate reviews (Stage 1-pre request-changes exits and Stage 1a/1b/1c, submitted via `gh pr review --request-changes`) use the signature only — no footer; they reject before a real review pass. **Approval:** the approve step runs **after** the Stage 3 comment. Comment first, then approve **pinned to the reviewed commit** — `gh pr review --approve` does not bind to a SHA, so a force-push in the check-then-act gap would approve unseen code. Use the reviews API with `commit_id` instead, which records the approval against the exact commit you reviewed (branch protection that requires approval of the latest push then won't count it if the head moved): @@ -147,6 +151,183 @@ Save the `worktreePath`. All `read_file`, `grep_search`, `glob` calls below must This is the most important stage — catch problems before anyone spends time reviewing code. +**1-pre. Duplicate / already-fixed check (run before the template check):** + +A PR opened after its linked issue was already fixed stays open forever — no +other gate looks at the linked issue's state. Check it deterministically +before investing in a review. Scope note: the gate executes inside the +triage agent session, so it covers healthy runs only — a run whose agent +cannot reach the model produces no triage at all; that failure shape belongs +to the workflow's response check, not here. + +**Default-branch scope.** Run 1-pre only when the PR targets the default +branch: + +```bash +BASE_REF=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json baseRefName --jq '.baseRefName') +DEFAULT_BRANCH=$(gh repo view "$REPO" --json defaultBranchRef --jq '.defaultBranchRef.name') +``` + +- `BASE_REF` != `DEFAULT_BRANCH` (e.g. a backport to a `release/*` branch) → + skip 1-pre and proceed to 1a: such PRs legitimately carry changes that + already exist on the default branch, so the subsumption check below cannot + judge them. + +**Linked issues.** Read them from GitHub's own closing-reference parser — it +understands all nine closing-keyword forms (`close`/`closes`/`closed`, +`fix`/`fixes`/`fixed`, `resolve`/`resolves`/`resolved`), URL references, and +cross-repo references; a keyword grep misses most of them. Keep only same-repo +references: issue numbers restart at 1 in every repository, so a cross-repo +`Fixes other-org/other-repo#42` resolved against this repo silently returns +this repo's unrelated issue #42, and every lookup below is scoped to this +repo — cross-repo closing references are skipped (this gate can only judge +duplicates against this repo's issues): + +```bash +ISSUES=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json closingIssuesReferences | + jq -r --arg repo "$REPO" '.closingIssuesReferences[] + | select((.repository.owner.login + "/" + .repository.name) == $repo) + | .number' | sort -u) +``` + +The parser is not intent-aware — prose like "resolves #123's closer" links +#123 too, so an accidental prose mention can pull an unrelated issue into +`ISSUES`. There is no deterministic intent check: the linkage decides WHICH +issues the branches below read, and they then act on those issues' states. +The blast radius stays bounded — the only irreversible act (close) +additionally requires this PR's diff to be fully subsumed by the default +branch, which is true only when the change is already landed, so an +accidental linkage can at worst reach a visible, reversible request-changes +review or a maintainer escalation, never a substantively wrong close. + +```bash +# Record each linked issue's state; $N feeds the closer query below. The loop +# only collects states — it never acts per issue, so a mix of OPEN and CLOSED +# issues gets exactly one outcome from the precedence rule below. +for N in $ISSUES; do + SR=$(gh issue view "$N" --repo "$REPO" --json state,stateReason \ + --jq '.state + " " + (.stateReason // "")') + # "OPEN" -> contributes nothing; "CLOSED NOT_PLANNED" -> request changes, + # stop; "CLOSED COMPLETED" -> run the closer query below with this $N +done +``` + +These branches are checked with a fixed precedence — any closed-as-not-planned +first, then any closed-as-completed, and only when no issue is closed does the +run proceed to 1a — so mixed states have exactly one outcome. An OPEN issue +never short-circuits a CLOSED one: `fixes #101 and fixes #102` with #101 open +and #102 closed-as-completed runs the closer query for #102, it does not +proceed to 1a. + +- No linked issues, or every linked issue **open** → proceed to 1a. +- Any linked issue **closed as not planned** → the fix target was rejected: + submit exactly one `CHANGES_REQUESTED` review asking them to reach + agreement in the issue first (bilingual body whose first line is the + `` marker, @mention the author), and stop: + +```bash +gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body-file /tmp/stage-1pre-not-planned.md +``` + +- Any linked issue **closed as completed** → find what closed it (GraphQL — + the REST timeline's `closed` event carries no reliable closer reference): + +```bash +gh api graphql -f query=' + query($owner: String!, $name: String!, $n: Int!) { + repository(owner: $owner, name: $name) { + issue(number: $n) { + timelineItems(last: 20, itemTypes: [CLOSED_EVENT]) { + nodes { + ... on ClosedEvent { + closer { + ... on PullRequest { number state merged } + ... on Commit { oid } + } + } + } + } + } + } + }' -f owner="${REPO%%/*}" -f name="${REPO##*/}" -F n="$N" \ + --jq '.data.repository.issue.timelineItems.nodes // [] | last | .closer | select(. != null and .number != null) | "\(.number) \(.merged)"' +``` + +Only the LAST (most recent) close event counts — earlier closes belong to +reopen cycles and their closers are stale. If the query fails or emits +nothing (the number is a PR, not an issue; the issue does not exist; the +latest close was manual), treat the closer as unresolved. + +- Closed by a **merged PR** → compare this PR's production diff (exclude + test/generated files per the Stage 0 size rules) against the default + branch (`$DEFAULT_BRANCH` — this PR's base, per the scope check above): + - **Fully subsumed** — applying this PR's ENTIRE diff to the default + branch would change nothing: every production line this PR adds already + exists there, AND every production line this PR deletes is already + absent there (check per file via + `gh api "repos/$REPO/contents/?ref=$DEFAULT_BRANCH"`). A diff + with NO production changes (e.g. tests-only) is never fully subsumed — + any file it adds outside the production set is itself a remaining + delta. → post the terminal comment below, then close the PR. This is + the ONLY place triage closes a PR. + - **Any remaining delta** — everything else: an added production line + that is missing there, a deleted production line that still exists + there, or any non-production addition → submit exactly one + `CHANGES_REQUESTED` review: name the merged PR, name the remaining + delta, ask the author to rebase onto the default branch and reduce the + PR to that delta (bilingual body whose first line is the + `` marker, @mention the author). Stop: + +```bash +gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body-file /tmp/stage-1pre-remaining-delta.md +``` + +- Closed manually (no close commit) or the closer cannot be resolved → + never close on ambiguity: flag it in the Stage 1 comment and escalate to + the maintainer. + +**Reopen guard.** The close below is the gate's only irreversible act, and an +explicit `@qwen-code /triage` re-run executes every stage again — including +on a PR a maintainer reopened after this very close. On a reopened PR all +inputs re-derive identically (the issue is still closed as completed, the +closer is still merged, the diff is unchanged), so without a guard the gate +would re-close against the maintainer's deliberate reopen, and keep +re-closing on every later re-run. Check before posting and closing: + +```bash +PRIOR_1PRE=$(stage_comment_id 1-pre) +``` + +The duplicate-close exit is the only Stage 1-pre path that posts a +`stage=1-pre` issue comment (the two request-changes exits post reviews +only). If the PR is OPEN and `PRIOR_1PRE` is non-empty, a duplicate close +already happened and a human reopened the PR: do not post or close again — +flag the reopen in the Stage 1 comment and escalate to the maintainer. The +deliberate reopen means the duplicate judgment needs human eyes, and this +guard is what keeps that judgment from being overridden on the next run. + +```bash +cat > /tmp/stage-1pre-duplicate.md <<'EOF' + + +The linked issue #N was already fixed by #M, and every production change in +this PR is already on the default branch — closing as a duplicate of #M. If +something here is NOT covered by #M, say so and this can be reopened. + +
+中文说明 + +关联 issue #N 已由 #M 修复,本 PR 的生产代码改动均已存在于默认分支, +现作为 #M 的重复 PR 关闭。如本 PR 有 #M 未覆盖的内容,请说明,可以重新打开。 + +
+ +— _Qwen Code · qwen3.7-max_ +EOF +gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file /tmp/stage-1pre-duplicate.md +gh pr close "$PR_NUMBER" --repo "$REPO" +``` + **1a. Template check:** PR body missing required headings from `.github/pull_request_template.md` (read from worktree) → request changes, @mention author, link the template, stop. This is the only public output for this terminal gate. @@ -301,6 +482,8 @@ Risk: { + const section = prSkill.slice( + prSkill.indexOf('**1-pre. Duplicate / already-fixed check'), + prSkill.indexOf('**1a. Template check:**'), + ); + + it('reads linked issues from GitHub closing references, not a keyword grep', () => { + // A keyword grep misses 6 of the 9 closing-keyword forms and matches + // substrings like "prefixes"; GitHub's own parser is the linkage source. + expect(section).toContain('--json closingIssuesReferences'); + expect(section).not.toContain("grep -oiE '(fixes|closes|resolves)"); + }); + + it('runs only for PRs targeting the default branch', () => { + // Backports to release/* branches legitimately carry changes that already + // exist on the default branch; without this scope the gate closes them. + expect(section).toContain('Run 1-pre only when the PR targets the default'); + expect(section).toContain('defaultBranchRef'); + expect(section).not.toContain('?ref=main'); + }); + + it('defines subsumption over the full diff, never over added lines alone', () => { + // Added-lines-only quantification closes deletions-only diffs vacuously; + // the deleted-lines clause must stay. + expect(section).toContain('every production line this PR adds'); + expect(section).toContain('every production line this PR deletes'); + }); + + it('never closes a diff with no production changes', () => { + // Stage 0 exclusions empty the comparison set for tests-only PRs; such a + // diff must be a remaining delta, never "Fully subsumed". + expect(section).toContain('NO production changes'); + expect(section).toContain('never fully subsumed'); + }); + + it('scopes linkage extraction to same-repo closing references', () => { + // A bare `.number` extraction drops the repository qualifier, so a + // cross-repo closing reference resolves against this repo's + // same-numbered unrelated issue. The scoping filter and the skip rule + // must stay. + expect(section).toContain('.repository.owner.login'); + expect(section).toContain('cross-repo closing references are skipped'); + }); + + it('guards the duplicate close against a human reopen', () => { + // A re-run on a reopened PR re-derives identical inputs; without the + // reopen guard the gate re-closes against a maintainer's deliberate + // reopen, and every later re-run closes again. Deleting the guard must + // make this red. + expect(section).toContain('**Reopen guard.**'); + expect(section).toContain('do not post or close again'); + }); + + it('binds each linked-issue state to its gate action', () => { + // The per-issue dispatch is the gate's decision table; deleting it or + // swapping the NOT_PLANNED and COMPLETED actions must not leave the + // suite green. The loop only collects states, so an OPEN issue must not + // short-circuit to 1a over a CLOSED one. + expect(section).toContain('"OPEN" -> contributes nothing'); + expect(section).toContain('"CLOSED NOT_PLANNED" -> request changes'); + expect(section).toContain('"CLOSED COMPLETED" -> run the closer query'); + expect(section).not.toContain('"OPEN" -> proceed to 1a'); + }); + + it('defines one fixed precedence for mixed linked-issue states', () => { + // xe6u: `fixes #101 and fixes #102` with #101 OPEN, #102 CLOSED-COMPLETED + // must have one deterministic outcome. Without an explicit precedence the + // per-issue loop legend and the aggregate bullets contradict each other. + expect(section).toContain('fixed precedence'); + expect(section).toContain('never short-circuits'); + }); + + it('never closes on an unresolvable closer', () => { + // The ambiguity bullet is the only explicit prohibition against closing + // on a closer that cannot be resolved; deleting it must make this red. + expect(section).toContain('never close on ambiguity'); + }); + + it('SKILL.md restates the 1-pre boundary without a production qualifier', () => { + // xe6: SKILL.md's escalation summary must match pr-workflow.md's + // operational definition — request changes on ANY remaining delta, close + // only when the ENTIRE diff is subsumed. Re-qualifying either side with + // "production" contradicts "any non-production addition is a remaining + // delta" and "a diff with NO production changes is never fully subsumed", + // giving a tests-only PR opposite instructions in the two files. + const summary = triageSkillDoc.slice( + triageSkillDoc.indexOf('The escalation criteria are those defined in'), + triageSkillDoc.indexOf('Never execute PR-derived code'), + ); + expect(summary).toContain('a remaining delta'); + expect(summary).toContain('entire diff'); + expect(summary).toContain('fully subsumed'); + expect(summary).not.toContain('production delta'); + expect(summary).not.toContain('production diff'); + }); +});