Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .qwen/skills/triage/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
193 changes: 188 additions & 5 deletions .qwen/skills/triage/references/pr-workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 "<!-- qwen-triage stage=$1 -->" \
'[.[][] | select(.user.login == $bot) | select(.body | startswith($m))] | last | .id // empty'
Expand Down Expand Up @@ -75,7 +79,7 @@ Every staged comment (Stage 1 gate-pass, Stage 2, Stage 3) ends with the signatu
<sub>Reviewed at `<HEAD_SHA>` · re-run with `@qwen-code /triage`</sub>
```

**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):

Expand Down Expand Up @@ -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
Comment thread
yiliang114 marked this conversation as resolved.
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.
Comment thread
yiliang114 marked this conversation as resolved.
- 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
`<!-- qwen-triage stage=1-pre -->` marker, @mention the author), and stop:

```bash
gh pr review "$PR_NUMBER" --repo "$REPO" --request-changes --body-file /tmp/stage-1pre-not-planned.md
Comment thread
yiliang114 marked this conversation as resolved.
```

- 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/<path>?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
`<!-- qwen-triage stage=1-pre -->` 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'
<!-- qwen-triage stage=1-pre -->

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.

<details>
<summary>中文说明</summary>

关联 issue #N 已由 #M 修复,本 PR 的生产代码改动均已存在于默认分支,
现作为 #M 的重复 PR 关闭。如本 PR 有 #M 未覆盖的内容,请说明,可以重新打开。

</details>

— _Qwen Code · qwen3.7-max_
EOF
gh pr comment "$PR_NUMBER" --repo "$REPO" --body-file /tmp/stage-1pre-duplicate.md
Comment thread
yiliang114 marked this conversation as resolved.
gh pr close "$PR_NUMBER" --repo "$REPO"
Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.
Comment thread
yiliang114 marked this conversation as resolved.
```

**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.
Expand Down Expand Up @@ -301,6 +482,8 @@ Risk: <if Stage 1e matched, list the high-risk paths and recommended review dept

Save this comment's ID. Terminal exits — stop here if any applies:

- Duplicate of a merged fix, no remaining delta (Stage 1-pre) → closed.
Comment thread
yiliang114 marked this conversation as resolved.
- Duplicate with remaining delta, or issue closed as not planned (Stage 1-pre) → request changes, stopped.
- Core module hard block (Stage 0) → rejected, do not proceed.
- Template failure (Stage 1a) → stopped.
- Problem does not exist (Stage 1b) → request changes, do not proceed to Stage 2.
Expand Down
98 changes: 98 additions & 0 deletions scripts/tests/qwen-triage-workflow.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const prSkill = readFileSync(
'.qwen/skills/triage/references/pr-workflow.md',
'utf8',
);
const triageSkillDoc = readFileSync('.qwen/skills/triage/SKILL.md', 'utf8');
const verifySkill = readFileSync('.qwen/skills/verify-pr/SKILL.md', 'utf8');
const hasGnuRealpath =
spawnSync('realpath', ['-m', '--', '/'], { stdio: 'ignore' }).status === 0;
Expand Down Expand Up @@ -6805,3 +6806,100 @@ describe('triage skips the autofix bot’s own bookkeeping issues (#9264)', () =
expect(autofixDoc.env.AUTOFIX_BOT).toBe(`\${{ ${botIdentityCore} }}`);
});
});

describe('stage 1-pre duplicate gate', () => {
const section = prSkill.slice(
prSkill.indexOf('**1-pre. Duplicate / already-fixed check'),
Comment thread
yiliang114 marked this conversation as resolved.
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".
Comment thread
yiliang114 marked this conversation as resolved.
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');
});
});
Loading