mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-10 17:27:10 +00:00
feat(triage): add confidence score, sequence diagram, files overview, and review footer to PR comments (#6789)
* feat(triage): add confidence score, sequence diagram, files overview, and review footer to PR comments Enrich the /triage bot's PR comments with four presentation elements, all conditional and kept in the existing human-maintainer voice: - Stage 3 opens with a one-line `Confidence: N/5` score mapped to the approve / defer / request-changes verdict (fork-refactor guardrail caps at 3/5). - Stage 2 may add a light/dark mermaid sequence diagram — only for PRs that introduce or reshape a multi-step runtime flow. - Stage 2 may add a collapsed changed-files overview table — only when many source files are touched. - Every staged comment ends with a footer recording the reviewed commit SHA, so a maintainer can tell on re-run whether new commits landed since. Heavy elements (diagram, table) trigger only on complex PRs; a small, focused PR still gets the plain findings + tmux testing comment. Fetch now captures headRefOid for the footer. * fix(triage): use a single auto-themed sequence diagram, not the light/dark anchor trick The `#gh-light-mode-only` / `#gh-dark-mode-only` fragment only theme-scopes images on GitHub, not anchor-wrapped mermaid — verified on a real comment's body_html, where both `<pre lang="mermaid">` blocks survive with no theme-hiding class, so the two copies render stacked. Switch the skill template to a single plain mermaid block with no theme directive; GitHub auto-themes unthemed mermaid to the reader's own light/dark mode. * docs(triage): warn against ; and em-dash inside mermaid message text A `;` or `—` inside a sequence-diagram message breaks GitHub's mermaid parser (`;` is read as a statement separator), so the diagram fails to render. Tell the skill to keep message text to plain words plus commas and parentheses. * fix(triage): address #6789 review findings on the enrichment additions Review of the triage-enrichment PR flagged 3 Critical plus several Suggestion items on the new skill text; this addresses them: - Escape attacker-controlled fork PR paths before they enter the changed-files table (|, backticks, <>&, @, CR/LF), and pull the list via paginated REST so files past the first 100 are not silently dropped. - Pin the full headRefOid in the reviewed-commit footer (a 7-char prefix is spoofable via force-push) and reuse it from the initial fetch instead of a second gh call; drop the footer when HEAD_SHA is empty rather than emitting empty backticks that overwrite a valid SHA on re-run. - Correct the mermaid punctuation rule: em dashes render fine (the prohibition was false), while `;` breaks the parser and `#` clips the label (both verified against the bundled Mermaid). - Add `files` to the initial fetch; qualify the SHA footer as skipped for terminal-gate reviews; fold Stage-0 escalation into the confidence-cap rule; give Stage 2 a concrete footer template and Chinese-block placement; unify "enrichments" terminology and point SKILL.md at pr-workflow.md as the single source of truth. * fix(triage): address #6789 re-review of the enrichment fixes The first review-fix commit introduced a few inconsistencies the re-review caught; this resolves them: - Drop the undefined `$PR_JSON` reference and the "reuse, do not call twice" framing: capture HEAD_SHA with a fresh per-stage `gh pr view --json headRefOid` so the footer actually reflects the head each stage reviewed (reuse cannot detect a mid-run force-push). - Remove `files` from the initial fetch — the changed-files table uses the paginated REST endpoint, so the fetched `files` was unused. - Replace the prose path-escaping with a deterministic `sanitize_path()` that escapes `&` first (no double-encoding) and renders inside `<code>` (so a backtick in a filename is escapable). - Confidence rubric: 3/5 is "defer (comment)", never `--request-changes`; spell out defer-vs-escalate and how to word a guardrail-capped 3/5. - Unify the signature to underscore italics. * fix(triage): address #6789 round-3 review of the enrichment fixes Round-3 re-review caught a rendering bug I introduced plus three hardening gaps: - Fix a broken code fence: the sanitize_path recipe opened with 4 backticks and "closed" with 3, swallowing the following prose and the files-table template into one giant bash block. - Footer empty-guard now fails closed. Dropping the footer on a re-run PATCHes the whole body and erases the prior valid SHA just like empty backticks would, so keep the existing comment and its footer until a full OID is available. - Extend the mermaid punctuation guard to participant aliases and labels (a `;` there also forges a second actor), and require labels from a safe char set. - Budget the changed-files table (cap ~30 rows, trim cells to 200 chars, append an "and N more" row) so the mandatory Stage 2 post cannot exceed GitHub's comment limit; render example paths with <code> to match the sanitizer. * fix(triage): address #6789 round-4 review of the enrichment fixes - Footer pins the SHA actually inspected (captured once at review start), and before every post and before --approve the workflow re-reads the head and bails on a mismatch — closes the force-push TOCTOU that a post-time capture or a pre-approve gap left open. - Reconcile the footer rules: Stage 2 and Stage 3 now both defer to the fail-closed rule on empty HEAD_SHA instead of "omit the footer" (which would blank a prior valid footer on re-run). - Mermaid participant safety: generate aliases (P1, P2) and keep sanitized names only in `as` labels, since reserved words (loop/end/activate) can't be aliases. - Files table: <code> does not stop GFM from parsing Markdown, so also encode link/emphasis syntax; sanitize the "What changed" column; cap tmux output so the whole Stage 2 comment stays under the size limit. - 3/5 wording: make the defer path unambiguous (not request-changes). * fix(triage): bind approval to reviewed commit and normalize mermaid labels (#6789 P1s) - Approve via the reviews API pinned to $HEAD_SHA (commit_id) instead of `gh pr review --approve`, closing the check-then-act force-push window before approval — a review recorded against the reviewed commit is not counted for a moved head under "require approval of latest push". - Define a deterministic `as`-label normalizer for diagram participants (keep [A-Za-z0-9 _.()-], drop CR/LF and Mermaid control chars, cap length) so a newline in a fork-supplied component name cannot inject a second actor. * fix(triage): use the SHA-pinned approval form in Stage 3 too (#6789 critical) The Approval note switched to the commit_id-pinned reviews API, but the actual Stage 3 Step 2 approve block still used gh pr review --approve (no SHA binding). Update the step-proximate code so an agent following Stage 3 uses the pinned form. * fix(triage): actually encode markdown metachars in sanitize_path + guard empty HEAD_SHA (#6789) - The prose said to encode [](){}* but the sed never did it, so a fork filename like src/[x](https://attacker).ts still rendered as a link inside the <code> cell. Add the five sed clauses (after the & pass, so no double-encoding). - HEAD_SHA capture now fails on gh error and on an empty OID, and the stale check requires a non-empty current head, closing the empty-vs-empty pass.
This commit is contained in:
Shaojin Wen
GitHub
parent
a1806e404f
commit
7e6d324e6e
2 changed files with 103 additions and 9 deletions
|
|
@ -26,7 +26,7 @@ Run staged admission via `gh`. Post comment after each stage.
|
|||
|
||||
```bash
|
||||
gh issue view "$NUM" --repo "$REPO" --json number,title,body,author,labels,comments,url
|
||||
gh pr view "$NUM" --repo "$REPO" --json number,title,body,author,labels,additions,deletions,changedFiles,baseRefName,headRefName,isCrossRepository,isDraft,reviewDecision,url
|
||||
gh pr view "$NUM" --repo "$REPO" --json number,title,body,author,labels,additions,deletions,changedFiles,baseRefName,headRefName,headRefOid,isCrossRepository,isDraft,reviewDecision,url
|
||||
gh label list --repo "$REPO" --limit 200
|
||||
```
|
||||
|
||||
|
|
@ -71,6 +71,8 @@ Bilingual: English first, Chinese in `<details>`. @mention author when blocking.
|
|||
- **Issue**: one comment, Stage 2 updates it in place. Key-point bullet format.
|
||||
- **PR**: three comments (Stage 1: Gate, Stage 2: Review + Test, Stage 3: Final Decision). Key-point bullet format.
|
||||
|
||||
**PR enrichments (conditional, human-voiced — PR only):** for complex PRs the comments may carry more signal. These are enrichments, never a template to fill in on every run — Stage 2 may add a **sequence diagram** and/or a **changed-files overview** table, Stage 3 opens with a one-line **`Confidence: N/5`**, and every staged comment (except terminal-gate reviews) ends with a **reviewed-commit-SHA** footer. Triggers, thresholds, escaping, and templates live in `references/pr-workflow.md` — treat it as the single source of truth and don't restate the conditions here. Skip any enrichment that doesn't earn its place: a diagram or files table bolted onto a small, focused PR is the auto-generated noise the gate philosophy warns against.
|
||||
|
||||
## ⛔ Mandatory Pre-flight Checks (DO NOT SKIP)
|
||||
|
||||
These two steps are the most commonly forgotten. Execute them before any other action.
|
||||
|
|
|
|||
|
|
@ -45,13 +45,34 @@ EXISTING=$(gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
|
|||
if [ "$EXISTING" -eq 0 ]; then gh pr review ... ; fi
|
||||
```
|
||||
|
||||
**Signature:** every comment ends with:
|
||||
**Signature & footer:** capture the reviewed commit's **full** OID **once, when you begin inspecting the code** — the SHA the worktree/diff actually reflects. Not a 7-char prefix (28 bits; a fork author can force-push a colliding prefix), and **not** a fresh read at post time (that would attest to code you never reviewed). Reuse this `HEAD_SHA` for every stage's footer, and before each post — and again before `--approve` — re-read the head and bail if it moved:
|
||||
|
||||
```
|
||||
— *Qwen Code · qwen3.7-max*
|
||||
```bash
|
||||
HEAD_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid') || exit 1
|
||||
[ -n "$HEAD_SHA" ] || { echo 'empty head SHA — fail closed'; exit 1; } # once, at review start
|
||||
# before any post or approval — refuse to attest to code you didn't review:
|
||||
NOW=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid') || exit 1
|
||||
[ -n "$NOW" ] && [ "$NOW" = "$HEAD_SHA" ] || { echo 'head moved or unreadable — restart or defer'; exit 1; }
|
||||
```
|
||||
|
||||
**Approval:** the `gh pr review --approve` command is a separate step that runs **after** Stage 3 comment is posted. Comment first, then approve only when genuinely confident.
|
||||
Every staged comment (Stage 1 gate-pass, Stage 2, Stage 3) ends with the signature line, then a footer recording the commit this pass reflects. Because comments are updated in place on re-run, the SHA lets a maintainer tell at a glance whether new commits landed since the last review:
|
||||
|
||||
```
|
||||
— _Qwen Code · qwen3.7-max_
|
||||
|
||||
<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.
|
||||
|
||||
**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):
|
||||
|
||||
```bash
|
||||
gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
|
||||
-f commit_id="$HEAD_SHA" -f event=APPROVE -f body='LGTM, looks ready to ship. ✅'
|
||||
```
|
||||
|
||||
Only approve when you're genuinely confident.
|
||||
|
||||
### Gate Philosophy
|
||||
|
||||
|
|
@ -214,6 +235,8 @@ Approach: <state your honest assessment — the scope feels right / feels like i
|
|||
</details>
|
||||
|
||||
— _Qwen Code · qwen3.7-max_
|
||||
|
||||
<sub>Reviewed at `<HEAD_SHA>` · re-run with `@qwen-code /triage`</sub>
|
||||
```
|
||||
|
||||
Save this comment's ID. Terminal exits — stop here if any applies:
|
||||
|
|
@ -265,6 +288,53 @@ gh pr diff "$PR_NUMBER" --repo "$REPO"
|
|||
|
||||
When posting findings, summarize in a few sentences like a human would — "the auth logic is duplicated in two places, worth extracting" not a line-by-line breakdown. Save inline comments for things that genuinely block the merge.
|
||||
|
||||
#### 2a-bis. Optional enrichments (only when they add signal)
|
||||
|
||||
Selective and conditional — these enrich the human-voice comment for complex PRs; they are **not** a template to fill in on every run. Add each only when it genuinely helps the maintainer, and skip silently otherwise. A diagram or files table bolted onto a small, focused PR is exactly the auto-generated noise the gate philosophy warns against — when in doubt, leave it out.
|
||||
|
||||
**Sequence diagram** — add when the PR introduces or reshapes a multi-step runtime flow: a new tool/callback lifecycle, a request → response → re-inject path, a state machine, a cross-component handshake. Skip for one-line fixes, pure refactors, and config/doc/test-only changes. Keep it to the key path (≤ ~8 participants), not every branch. Use a single plain `mermaid` block with **no** `%%{init: {'theme': …}}%%` directive — GitHub renders unthemed mermaid in the reader's own light/dark mode automatically, so one block stays legible in both:
|
||||
|
||||
````markdown
|
||||
```mermaid
|
||||
sequenceDiagram
|
||||
participant User as User
|
||||
participant Tool as new_tool
|
||||
User->>Tool: invoke
|
||||
Tool-->>User: result
|
||||
```
|
||||
````
|
||||
|
||||
Diagram text (participants, labels) stays English in the main comment; the `<details>` Chinese translation can summarize it in prose rather than duplicating the diagram. Keep message text to plain words and light punctuation — commas, parentheses, and em dashes all render fine (verified against the repo's bundled Mermaid), but a `;` **inside a message** breaks the parser (it is read as a statement separator) and a `#` clips the rest of the label (verified — `review PR #6789` renders as just `review PR`); drop the `;` and write numbers as plain digits (`PR 6789`, not `#6789`). This applies to **participant aliases and display labels too**, not just messages — Mermaid reads `;` as a statement separator there as well, so a hostile component name like `participant X as evil; participant Y as APPROVED` forges a second actor. Since you may name participants after PR components (untrusted on a fork), give each participant a generated alias (`P1`, `P2`, …); for the `as` display label, run the name through a **deterministic normalizer** that keeps only `[A-Za-z0-9 _.()-]` (dropping CR/LF, `;`, `#`, `:`, and every other Mermaid control character) and caps it to ~40 chars — otherwise a label such as `evil` + newline + `participant P2 as APPROVED` injects a second actor. The generated alias is separate because a bare safe-charset rule isn't enough on its own (Mermaid rejects reserved words like `loop`, `end`, `activate` as aliases). Never drop a raw fork-supplied name into the diagram. Do **not** wrap two themed copies in `#gh-light-mode-only` / `#gh-dark-mode-only` anchors: GitHub only theme-scopes that fragment on images, not on anchor-wrapped mermaid, so both copies render stacked (verified empirically on a real comment — the anchors survive as inert links and neither `<pre lang="mermaid">` gets a theme-hiding class).
|
||||
|
||||
**Changed-files overview** — add only when the PR touches many source files (~5+) and a per-file map genuinely helps a reviewer navigate. Pull the list with the paginated REST endpoint — `gh api "repos/$REPO/pulls/$PR_NUMBER/files" --paginate --jq '.[].filename'` — not `gh pr view --json files`, which caps at the first 100 files and silently drops the rest. **A fork PR's paths are attacker-controlled:** a filename can carry `|`, backticks, `<`, `>`, `&`, `@mentions`, or CR/LF that break out of the table cell and render forged bot text (a fake approval or confidence line). Before a path enters the table, run it through a deterministic sanitizer — order matters (escape `&` **first**, or later escapes double-encode), and a `` ` `` can't be escaped inside a `` `…` `` span, so render each path inside `<code>…</code>` where HTML entities resolve. If a path still looks hostile, show a bounded placeholder instead of the raw name:
|
||||
|
||||
```bash
|
||||
sanitize_path() { # single-line, HTML-safe, cell-bounded
|
||||
printf '%s' "$1" | tr -d '\r\n' | cut -c1-200 |
|
||||
sed -e 's/&/\&/g' -e 's/</\</g' -e 's/>/\>/g' \
|
||||
-e 's/`/\`/g' -e 's/|/\|/g' -e 's/@/\@/g' \
|
||||
-e 's/\[/\[/g' -e 's/\]/\]/g' -e 's/(/\(/g' -e 's/)/\)/g' -e 's/\*/\*/g'
|
||||
}
|
||||
# render in the table as: <code>$(sanitize_path "$path")</code>
|
||||
```
|
||||
|
||||
Fold the table in a `<details>` so it doesn't dominate the comment, and write one honest line per file in your own words — not a mechanical restatement of the diff. **Budget it:** show at most ~30 rows, cap each cell (the sanitizer already trims to 200 chars), and append a final `…and N more files` row instead of listing every path — the table shares the comment's ~65 KB limit with the findings, tmux output, the bilingual summary, and the footer, and the Stage 2 post is mandatory. Skip the table entirely for small, focused PRs.
|
||||
|
||||
Two more escaping notes: `<code>` shows HTML entities literally but GFM **still parses Markdown inside it**, which is why the `sed` above also encodes link/emphasis syntax (`[` `]` `(` `)` `*`); and the **What changed** column needs the same discipline — keep it plain prose with no `|`, backticks, or `<`/`>` (or run it through the sanitizer too). Cap the tmux capture (~500 lines / ~15 KB) so findings + diagram + table + testing + the bilingual summary stay under the comment limit together.
|
||||
|
||||
```markdown
|
||||
<details>
|
||||
<summary>Files changed (30 of N shown)</summary>
|
||||
|
||||
| File | What changed |
|
||||
| ------------------------------------------ | ----------------- |
|
||||
| <code>packages/core/src/foo.ts</code> | <one honest line> |
|
||||
| <code>packages/core/src/foo.test.ts</code> | <one honest line> |
|
||||
| …and 12 more files | |
|
||||
|
||||
</details>
|
||||
```
|
||||
|
||||
#### 2b. Real-Scenario Testing
|
||||
|
||||
**Runs in the main working tree, not the worktree** — tmux needs the local build environment.
|
||||
|
|
@ -298,7 +368,7 @@ tmux kill-session -t "$S"
|
|||
- Cannot run after exhausting workarounds → FAIL, not skip.
|
||||
- Fork code: sandbox (strip write tokens/secrets).
|
||||
|
||||
Post a single Stage 2 comment (must include `<!-- qwen-triage stage=2 -->` at the top): code review findings + testing result.
|
||||
Post a single Stage 2 comment (must include `<!-- qwen-triage stage=2 -->` at the top), in this order: code review findings → optional sequence diagram (2a-bis) → optional changed-files overview (2a-bis) → real-scenario testing result (below) → the bilingual `<details>` Chinese summary → signature + footer last (the same tail order as the Stage 1 template). Include the two enrichments only when 2a-bis says they earn their place; a small, focused PR is just findings + testing.
|
||||
|
||||
**⛔ BEFORE POSTING: verify your comment contains the tmux output.** Read back through your draft — does it have a fenced code block with the actual terminal capture? If not, add it now. The maintainer cannot approve without seeing what actually happened.
|
||||
|
||||
|
|
@ -312,7 +382,13 @@ Post a single Stage 2 comment (must include `<!-- qwen-triage stage=2 -->` at th
|
|||
<!-- paste capture-pane output here inside ``` -->
|
||||
````
|
||||
|
||||
Sign with `— *Qwen Code · qwen3.7-max*` and save this comment's ID.
|
||||
Close with the signature then the footer, and save this comment's ID — on an empty `HEAD_SHA`, follow the fail-closed rule above (leave an existing comment and its footer untouched; never blank it):
|
||||
|
||||
```markdown
|
||||
— _Qwen Code · qwen3.7-max_
|
||||
|
||||
<sub>Reviewed at `<HEAD_SHA>` · re-run with `@qwen-code /triage`</sub>
|
||||
```
|
||||
|
||||
### Stage 3: Reflect
|
||||
|
||||
|
|
@ -333,7 +409,21 @@ Step back and look at the whole picture — the motivation, the implementation,
|
|||
|
||||
If your independent proposal was materially simpler — say so. Not as a blocker, but as an honest question the contributor should think about.
|
||||
|
||||
**Step 1: Post the reflection comment** (must include `<!-- qwen-triage stage=3 -->` at the top). Write what you're actually thinking. "Looks good, ships the feature cleanly, the before/after shows it works" — not a five-bullet summary of the stages. If you have reservations, say them plainly. If you're approving with mild concerns, name them. Sign with `— *Qwen Code · qwen3.7-max*` and save this comment's ID.
|
||||
**Step 1: Post the reflection comment** (must include `<!-- qwen-triage stage=3 -->` at the top).
|
||||
|
||||
Open it with a one-line confidence score — `**Confidence: N/5** — <one honest line>` — as the human-readable summary of everything above. It is your read, not a rubric dump, and it must stay consistent with the verdict you're about to act on in Step 2:
|
||||
|
||||
| Score | Meaning | Verdict |
|
||||
| ----- | --------------------------------------------------------------- | --------------- |
|
||||
| 5/5 | Clean across every stage; would merge without hesitation | approve |
|
||||
| 4/5 | Solid; only non-blocking nits (name them) | approve |
|
||||
| 3/5 | Works, but real reservations or something a human should second | defer (comment) |
|
||||
| 2/5 | Significant concerns; leaning against as-is | request changes |
|
||||
| 1/5 | Should not merge in its current form | request changes |
|
||||
|
||||
A fork `refactor` that hits the approval guardrail below, **or a PR that Stage 0 escalated for maintainer awareness**, caps at 3/5 no matter how clean every stage looked — the guardrail drives the action, not the score. At 3/5 the action is always the **defer path** (a comment, never `--request-changes`): name any concerns in the defer comment for the maintainer's attention without approving, and @mention the maintainer for an unresolvable question or when the cap is pure policy. When the cap is pure policy on an otherwise-clean PR, say so in the one-line score so 3/5 doesn't read as real doubt — e.g. `Confidence: 3/5 — clean review, but the fork-refactor guardrail needs a maintainer's sign-off`. Never post a 4–5/5 alongside a `--request-changes`, or a 1–2/5 alongside an `--approve`: the score and the verdict tell the same story.
|
||||
|
||||
Then write what you're actually thinking. "Looks good, ships the feature cleanly, the before/after shows it works" — not a five-bullet summary of the stages. If you have reservations, say them plainly. If you're approving with mild concerns, name them. Sign with `— _Qwen Code · qwen3.7-max_`, add the reviewed-commit footer (empty `HEAD_SHA` → fail closed, as above — don't blank a prior footer), and save this comment's ID.
|
||||
|
||||
**Step 2: Act on the verdict.**
|
||||
|
||||
|
|
@ -353,7 +443,9 @@ If Stage 0 escalated the PR for maintainer awareness, do **not** approve automat
|
|||
All stages genuinely clean, `GUARD` is `ok`, and no Stage 0 maintainer escalation remains — approve:
|
||||
|
||||
```bash
|
||||
gh pr review "$PR_NUMBER" --repo "$REPO" --approve --body "LGTM, looks ready to ship. ✅"
|
||||
# Approve pinned to the reviewed commit (see the Approval note above) — never `gh pr review --approve`, which binds to no SHA.
|
||||
gh api "repos/$REPO/pulls/$PR_NUMBER/reviews" \
|
||||
-f commit_id="$HEAD_SHA" -f event=APPROVE -f body='LGTM, looks ready to ship. ✅'
|
||||
```
|
||||
|
||||
Reflection shows it shouldn't merge — request changes immediately, citing the specific concerns from the comment:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue