feat(triage): add confidence score, sequence diagram, files overview, and review footer to PR comments - #6789
Merged
Merged
Conversation
… 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.
Collaborator
|
Thanks for the PR! (Re-triage on latest commit Template: restructured ✓ — "What & why" combines the first two template sections, "Verification" covers the test plan, and the Problem: this is a Direction: clearly aligned. This improves the triage bot's own output quality — internal tooling that helps maintainers make faster decisions. No public contract, auth, sandbox, or telemetry surface touched. CHANGELOG has no direct reference (expected — this is meta-tooling). Size: not applicable. 2 skill documentation files (+102/-8), zero production code. Approach: scope feels right. Four conditional enrichments, each gated on complexity thresholds so small PRs don't get bloated. The security analysis for Mermaid injection (semicolons as statement separators, Moving on to code review. 🔍 中文说明感谢贡献！（基于最新 commit 模板：重组后完整 ✓ —— "What & why" 合并了模板的前两个章节，"Verification" 覆盖了测试计划， 问题：这是 方向：明确对齐。改进 triage 机器人自身的输出质量——帮助维护者更快做决定的内部工具。不涉及公共接口、认证、沙箱或遥测。CHANGELOG 无直接参考（预期之内——这是元工具）。 规模：不适用。2 个 skill 文档文件（+102/-8），零生产代码。 方案：范围合理。四个按需触发的增强元素，每个都有复杂度阈值门控，小 PR 不会被撑大。Mermaid 注入安全分析（分号作为语句分隔符、 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Stage 2 · Review + TestCode review of 2 skill documentation files (+102/-8). This PR has been through 6 rounds of review — all prior findings (path injection, SHA spoofing, broken code fences, approval TOCTOU, Code review — no blockers found. The changes are well-structured across two files:
No correctness bugs, no security gaps, no AGENTS.md violations. The documentation is precise — every conditional enrichment has explicit trigger conditions and skip guidance, so the bot won't bolt diagrams onto trivial PRs. Real-scenario testing: N/A. This PR modifies skill documentation ( 中文说明2 个 skill 文档文件的代码审查（+102/-8）。本 PR 经历了 6 轮审查——所有历史发现（路径注入、SHA 伪造、代码围栏损坏、审批 TOCTOU、 代码审查——未发现阻塞问题。 两个文件的改动结构清晰：
无正确性 bug、无安全漏洞、无 AGENTS.md 违规。 真实场景测试：不适用。 本 PR 修改的是 skill 文档（指导 AI 代理评论格式的 — Qwen Code · qwen3.7-max Reviewed at |
Collaborator
Stage 3 · ReflectConfidence: 5/5 — clean, focused, well-engineered docs/skill enhancement across six rounds of iteration. This PR does exactly what it says: adds four conditional presentation enrichments to triage bot comments, each gated on complexity thresholds so they don't become auto-generated noise on simple PRs. The design intent — "selective + conditional, never a fixed template" — is consistently implemented. What makes this stand out is the security depth. The Mermaid injection analysis (semicolons as statement separators, The SHA attestation chain (capture once → verify before each post → fail closed on empty → pin approval via All prior review findings have been addressed. No new issues found. Two documentation files, zero production code, no core paths touched. Ships the feature cleanly. 中文说明置信度：5/5 — 经过 6 轮迭代的干净、聚焦、工程精良的文档/skill 增强。 PR 精确实现了其声明：为 triage 机器人评论添加四个按需展示元素，每个都有复杂度阈值门控，不会在简单 PR 上产生自动生成噪音。设计意图——"选择性+按需，绝非固定模板"——在实现中一以贯之。 突出之处是安全深度。Mermaid 注入分析（分号作为语句分隔符、 SHA 证明链（捕获一次 → 每次发布前验证 → 空值时 fail closed → 通过 所有历史审查发现已修复。未发现新问题。两个文档文件，零生产代码，未触及核心路径。 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Collaborator
Author
Stage 2 · Review + Test代码审查：改动很克制—— sequenceDiagram
participant Model as Agent
participant Sched as ToolScheduler
participant RO as Read-only Tools
participant User as User
Model->>Sched: write_file (in plan mode)
Sched-->>Model: blocked, not read-only, do NOT retry, pivot first
Model->>RO: grep_search, read_file, glob
RO-->>Model: context gathered
Model->>Sched: exit_plan_mode (complete plan)
Sched-->>User: confirm plan?
User-->>Model: approved, execute
Files changed
Real-scenario testing (tmux): 在 plan 模式下让 agent 去改一个文件,观察它是否按新引导先用只读工具补足上下文、再 — Qwen Code · qwen3.7-max Reviewed at Stage 3 · ReflectConfidence: 4/5 — 改动小而准:把"被阻断就立即退出"改成"先用只读工具补足上下文、再带完整计划退出",消息文案与系统提示两处同改、测试都跟上了。不满分只因这是对模型的行为引导——tmux 里这个 prompt 下 agent 确实先 pivot 了,但引导类改动无法保证覆盖所有情形。 方案与我独立设想一致(改引导,而非新增控制流分支),范围克制,六个月后维护也不会骂人。Approve。 — Qwen Code · qwen3.7-max Reviewed at |
…/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.
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.
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 13, 2026
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed at 73234ee — no new findings from this review pass. Three existing Critical threads still stand (SHA truncation, HEAD_SHA error handling, file path escaping) and need resolution before merge.
— qwen3.7-max via Qwen Code /review
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.
Collaborator
Author
|
Thanks for the thorough review — addressed all 15 threads in 844329f. Critical
Suggestions
Resolving these threads now. 中文感谢细致评审——15 条已在 |
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No new blockers from this review pass. Three existing Critical threads still stand and need resolution before merge:
- SHA truncation to 7 chars (
.qwen/skills/triage/references/pr-workflow.md) HEAD_SHAcapture lacks error handling (.qwen/skills/triage/references/pr-workflow.md)- File path escaping in changed-files table (
.qwen/skills/triage/references/pr-workflow.md)
Suggestions are inline.
doudouOUC
previously approved these changes
Jul 13, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Re-reviewed at 844329f — the follow-up commit resolves the prior review pass cleanly. Verified each fix against the diff:
- Footer now pins the full OID (SHA-truncation concern) ✓
- Empty
HEAD_SHA→ drop the footer, so a re-run never overwrites a valid SHA with blank backticks ✓ - Fork-PR filenames escaped/normalized before entering the files table ✓
- File list pulled via
gh api …/files --paginateinstead of the 100-capped--json files✓ - Em dashes allowed;
;and#warned against; numbers written as plain digits (PR 6789) ✓ - SKILL.md now defers to
pr-workflow.mdas the single source of truth (removes the drift risk) + terminal-gate footer exemption ✓ - Rubric caps Stage 0-escalated PRs at 3/5; Stage 2 gains an inline footer template and explicit bilingual-details ordering ✓
One residual non-blocking nit left inline. Confidence: 4/5 → approve. Thorough follow-up — thanks.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
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.
Collaborator
Author
|
Round 2 — addressed the re-review in 12af83f.
Separately: the failing Resolving these threads. 中文第二轮已在 |
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 13, 2026
81 tasks
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.
Collaborator
Author
|
Round 3 — addressed in 4652ec5.
Resolving these. 中文第 3 轮已在 `4652ec5de` 处理:修复我引入的**代码围栏 bug**(sanitize_path 用 4 反引号开、3 反引号闭,吞掉了后面的散文和表格模板——现已平衡);**footer 空值改为 fail-closed**(re-run 会重写整个 body,丢页脚等于抹掉旧 SHA,故空值时不 PATCH、保留旧页脚);**标点守卫扩展到 participant 标签**(`;` 在标签里也能伪造 actor,须用安全字符集);**文件表加预算**(≤30 行、单元格截 200 字符、加"…and N more"行、控制在 GitHub ~65KB 内);**示例表改用 ``** 以匹配 sanitizer。现将这些标为 resolved。
|
wenshao
commented
Jul 13, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex $qreview via Qwen Code /review
qwen-code-ci-bot
requested changes
Jul 13, 2026
- 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).
Collaborator
Author
|
Round 4 — addressed in e5ce793.
Resolving these. 中文第 4 轮已在 `e5ce793f7` 处理:footer 改记"实际审阅的 SHA"(审阅开始时取一次),每次发帖与 approve 前都重比对 head、不一致就中止(堵住 force-push TOCTOU);统一 footer 规则(Stage 2/3 空 SHA 时都走 fail-closed、不再"omit footer");mermaid 用生成式 alias(`P1`…)、保留字不能当 alias;`` 不阻止 GFM 解析 markdown → 也编码 `[](){}*`,并约束 What-changed 列;加 tmux/总体预算;3/5 措辞明确为 defer。现标为 resolved。
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed at e5ce793 — no new blockers from this review pass. One new Suggestion is inline.
Two existing Critical threads still stand and need resolution before merge:
- HEAD_SHA error handling (line 57): the bash snippet still has no
[ -n "$HEAD_SHA" ]guard — the fail-closed behavior is documented in prose only - File path escaping (line 309): the
sanitize_pathcode block still omits[,],(,),*escapes that the prose paragraph explicitly says to add
— qwen3.7-max via Qwen Code /review
| HEAD_SHA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json headRefOid --jq '.headRefOid') # 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') | ||
| [ "$NOW" = "$HEAD_SHA" ] || { echo 'head moved since review — restart or defer'; exit 1; } |
Collaborator
There was a problem hiding this comment.
[Suggestion] The remediation message 'head moved since review — restart or defer' gives two contradictory options without specifying which to choose. An LLM agent executing this workflow has no deterministic way to decide: should it re-run the entire triage from Stage 0, or post a defer comment and stop?
Suggested change
| [ "$NOW" = "$HEAD_SHA" ] || { echo 'head moved since review — restart or defer'; exit 1; } | |
| [ "$NOW" = "$HEAD_SHA" ] || { echo "head moved since review — aborting this triage run; re-run @qwen-code /triage to restart"; exit 1; } |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
previously approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
reviewed
Jul 13, 2026
Collaborator
There was a problem hiding this comment.
Reviewed the latest e5ce793f revision. The HEAD_SHA handling and filename sanitizer follow-ups already noted in the latest review still seem worth tightening before merge, and I left two additional inline comments on commit-bound approval and Mermaid display-label sanitization. The tmux example also does not yet enforce the new output budget. Other than those follow-ups, the overall direction looks good to me.
…abels (#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.
Collaborator
Author
|
Round 5 — addressed the two P1 items in a2c5d7a (treating P1 as Critical-tier). The one
Resolving the two P1 threads. 中文第 5 轮:两条 **P1**(按 Critical 级处理)已在 `a2c5d7ae4` 修复——审批改走 reviews API 并用 `commit_id` pin 到 `$HEAD_SHA`(`--approve` 不绑 SHA),堵住"比对到审批之间"的 force-push 窗口;`as` 标签加确定性规范化器(只留 `[A-Za-z0-9 _.()-]`、去 CR/LF 与 mermaid 控制符、限长),防换行注入第二个 actor。那条 Suggestion(措辞)按"只修 Critical/P1"的决定留着不动。现将两条 P1 标为 resolved。 |
qwen-code-ci-bot
previously requested changes
Jul 13, 2026
…ritical) 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.
Collaborator
Author
|
Round 6 — fixed the one [Critical] in the latest commit: the Stage 3 Step 2 approve block still used 中文第 6 轮:唯一的 [Critical] 已修——Stage 3 Step 2 的审批代码块之前还是 `gh pr review --approve`(不绑 SHA),而 Approval 段已改用 `commit_id` pin 的 API。现已把这个"最贴近执行"的代码块也改成 pinned 形式。剩下的 Suggestion 按政策留开。 |
yiliang114
reviewed
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Two items from the latest round:
[Critical] sanitize_path escapes HTML entities and table-breaking chars, but the prose at line 108 acknowledges that [, ], (, ), * also need encoding — and the sed doesn't apply it. A fork filename like src/[Security review passed](https://attacker.example).ts would render as a clickable link inside the <code> cell. The fix is one more sed clause that the diff already describes.
[Suggestion] The HEAD_SHA capture has no error handling. If gh pr view fails (rate limit, auth expiry, network), the variable is silently empty and the stale-head check compares empty-vs-empty. A simple || { echo 'failed to capture head SHA'; exit 1; } would close this.
…ard 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.
Collaborator
Author
|
Thanks @yiliang114 — both fixed in ab406d0:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| 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' |
Collaborator
There was a problem hiding this comment.
[Suggestion] cut -c1-200 runs before the sed entity-escaping pass. Each of the 11 substitutions expands a single character to 4–6 characters (| → |, & → &, etc.). A hostile 200-character path consisting largely of escapable characters could produce ~1200 characters of output after encoding, undermining the stated per-cell budget.
Suggested change
| -e 's/\[/\[/g' -e 's/\]/\]/g' -e 's/(/\(/g' -e 's/)/\)/g' -e 's/\*/\*/g' | |
| printf '%s' "$1" | tr -d '\r\n' | | |
| 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' | | |
| cut -c1-200 |
— qwen3.7-max via Qwen Code /review
|
|
||
| #### 2b. Real-Scenario Testing | ||
|
|
||
| **Runs in the main working tree, not the worktree** — tmux needs the local build environment. |
Collaborator
There was a problem hiding this comment.
[Suggestion] The claim that GFM "still parses Markdown inside <code>" is factually incorrect per the CommonMark spec (which GFM extends) — content inside inline HTML tags like <code>…</code> is not parsed as Markdown. The extra escaping of [, ], (, ), * is harmless (defense in depth), but the incorrect rationale could mislead a future maintainer who tries to reason about the sanitizer's correctness. Consider softening to "as a precaution, the sed above also encodes link/emphasis syntax."
— qwen3.7-max via Qwen Code /review
|
|
||
| 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: |
Collaborator
There was a problem hiding this comment.
[Suggestion] The changed-files sanitizer gets a complete, copy-pasteable sanitize_path() shell function, but the Mermaid participant-label normalizer — which has the same injection risk from fork-supplied names — is specified only in prose ("keeps only [A-Za-z0-9 _.()-] … caps to ~40 chars"). Without a reference implementation, different LLM agent runs could produce subtly different outputs (different cap lengths, edge-case handling), defeating the "deterministic" requirement. Consider adding a normalize_label() function analogous to sanitize_path().
— qwen3.7-max via Qwen Code /review
| 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. ✅' |
Collaborator
There was a problem hiding this comment.
[Suggestion] The gh api approval call has no error handling. If it fails (rate limit, head moved between staleness check and this call, permission issue), the Stage 3 comment is already live saying "looks good" but no approval record exists — the PR sits in a "looks approved but isn't" limbo. Consider adding || { echo 'APPROVAL FAILED — comment posted but review not recorded'; exit 1; } or equivalent.
— qwen3.7-max via Qwen Code /review
yiliang114
approved these changes
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Both findings fixed: sanitize_path now encodes Markdown metachars, and HEAD_SHA capture fails on gh error / empty OID.
yiliang114
dismissed
qwen-code-ci-bot’s stale review
July 13, 2026 16:26
Both findings fixed in ab406d0
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 13, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Jul 13, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Both findings from the earlier review are now fixed in ab406d0:
sanitize_pathMarkdown metachar encoding — thesednow includes the five missing clauses for[]()*, preventing fork filenames likesrc/[x](https://attacker).tsfrom rendering as links inside<code>cells.HEAD_SHAerror handling — the capture command now fails ongherror and on an empty OID, and the stale-head check requires a non-empty current head, closing the empty-vs-empty pass.
LGTM.
doudouOUC
reviewed
Jul 13, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
|
|
||
| ```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 |
Collaborator
There was a problem hiding this comment.
[Suggestion] When headRefOid is JSON null (e.g., head branch deleted after PR creation), gh pr view --json headRefOid --jq '.headRefOid' outputs the literal string null (jq's default rendering of a null value). The guard [ -n "$HEAD_SHA" ] passes because "null" is a non-empty string, so the script proceeds with HEAD_SHA="null" — violating the stated "fail closed" intent. The footer would read Reviewed at null`` and the approval API call (commit_id="null") would be rejected with 422, leaving a comment-orphan.
Suggested change
| [ -n "$HEAD_SHA" ] || { echo 'empty head SHA — fail closed'; exit 1; } # once, at review start | |
| [ -n "$HEAD_SHA" ] && [ "$HEAD_SHA" != "null" ] || { echo 'empty or null head SHA — fail closed'; exit 1; } # once, at review start |
— qwen3.7-max via Qwen Code /review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The
@qwen-code /triagebot's PR comments are thorough (gate → review + tmux testing → reflect) but present findings as plain prose. Borrowing from a structured review-bot summary, this adds four presentation elements that raise the signal density of the comments without abandoning the skill's deliberate human-maintainer voice — every heavy element is conditional and skipped on small, focused PRs.Changes (
.qwen/skills/triage/)Confidence: N/5that summarizes the verdict. A rubric ties each score to the existing approve / defer / request-changes action; a forkrefactorthat hits the approval guardrail caps at 3/5 and escalates.mermaidsequence diagram (nothemedirective, so GitHub renders it in the reader's own light/dark mode), but only when the PR introduces or reshapes a multi-step runtime flow (new tool/callback lifecycle, request → response → re-inject path, state machine). One-line fixes, pure refactors, and config/doc/test-only changes skip it.<details>files table, but only when the PR touches many source files (~5+).Reviewed at <sha>. Because triage updates comments in place on re-run, this lets a maintainer tell at a glance whether new commits landed since the last pass.Fetchnow capturesheadRefOid.Design intent — selective + conditional: these augment the comments for complex PRs; they are never a fixed template. A small PR still gets the plain findings + tmux testing comment.
Note on diagram theming
An earlier draft rendered the diagram twice (light + dark) wrapped in
#gh-light-mode-only/#gh-dark-mode-onlyanchors, borrowed from a production review bot. Verified empirically on a real comment (viabody_html): GitHub only theme-scopes that fragment on images, not on anchor-wrapped mermaid — both copies survive as visible<pre lang="mermaid">blocks and render stacked. The skill now uses a single plainmermaidblock with nothemedirective, which GitHub auto-themes to the reader's light/dark mode. See the demo comment below for the rendered result.Verification
prettier --checkclean, so the CI Prettier gate passes.@qwen-code /triage <PR>on a live PR and confirm the new Stage 2/3 output renders as intended.中文说明
背景与目的
@qwen-code /triage机器人的 PR 评论已经很完整(gate → 审查 + tmux 测试 → 反思),但结论以纯文字呈现。借鉴一个结构化 review 机器人的摘要格式,本 PR 新增四个提升信息密度的展示元素,同时不破坏 skill 刻意维持的"人类 maintainer 口吻"——所有重元素都是按需触发,小而聚焦的 PR 会跳过。改动(
.qwen/skills/triage/)Confidence: N/5概括裁决,配评分→裁决(approve / defer / request-changes)对照表;forkrefactor命中 approval guardrail 时封顶 3/5 并转交维护者。mermaid时序图(不带theme指令,GitHub 会按读者的亮/暗模式自动适配),仅当 PR 引入或重塑多步运行流程时(新工具/回调生命周期、请求 → 响应 → 重注入路径、状态机);一行修复、纯重构、纯配置/文档/测试改动跳过。<details>文件表,仅当改动文件较多(~5+)时。Reviewed at <sha>。因为 triage 在 re-run 时就地更新评论,这让维护者一眼看出上次审查后有没有新 commit。Fetch 现在会取headRefOid。设计原则是选择性 + 按需:这些是对复杂 PR 的增强,绝非固定模板;小 PR 仍是纯结论 + tmux 测试评论。
关于时序图主题
早期草稿把图渲染两遍(亮+暗)、用
#gh-light-mode-only/#gh-dark-mode-only锚点包裹(照搬自某生产 review 机器人)。经真机评论实测(body_html):GitHub 的该 fragment 只对图片做主题隔离,对锚点包裹的 mermaid 不生效——两份都作为可见的<pre lang="mermaid">保留、叠着显示。skill 现改用单张不带theme指令的mermaid,由 GitHub 按读者亮/暗模式自动适配。渲染效果见下方演示评论。验证
prettier --check通过,不卡 CI Prettier gate。@qwen-code /triage <PR>,确认新版 Stage 2/3 产出渲染符合预期。