feat(verify): check the accepting end, and separate empty from unmeasured - #8295
Conversation
…ured Three rules from maintainer rounds on #8132 and #8261, chosen over the larger set those rounds produced because this file is already 2.4x its size six days ago and nothing gates its growth. A capability has two ends. #8132 found a cookie-to-Authorization bridge gated to the desktop shell where it was minted and mounted unconditionally where it was accepted, so every server treated that cookie as a bearer. The tell generalises: the tests were named after the gated end, which is what made the ungated end look covered. "Nothing found" and "could not measure" must be different values, and the damage is done by the consumer rather than the flag. #8261 set `emptyDiff` both for a genuinely empty PR and for a diff capture that FAILED, and the skill answered that flag by recommending the PR be closed as superseded — a transient fetch error closing live work. The verdict contract already applies this rule to our own report; the code under test gets it too. A validity control must run before the artifact it invalidates is built. #8261's re-classifier demoted findings from a dead harness after the findings list was assembled, so a harness proven dead still filed `mutant-survived` against the author. A control that runs late is not a weaker control, it is not a control. The review also asked for two overlapping pairs to be consolidated. One resolved itself when the observability-ranking bullet moved next to the concurrency rule it cites. The other I deliberately left, with a note saying why: merging the type-boundary bullet into the sibling-sweep bullet would edit the one rule in this file with a measured before/after behind it, and that text is byte-identical to what the treatment arm read (verified against the staged copy). Editing the instrument is a change to make with a fresh measurement, not on the way past. Each new assertion was mutation-checked: neutering any of the three pinned phrases turns the suite red.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template: the headings are non-standard ( Problem: real and verified. The three rules were pushed to #8242's branch after it merged, so they landed on a closed branch and never reached Direction: aligned. This is internal verify-pr skill maintenance — three review heuristics distilled from the #8132 and #8261 verification rounds, re-cut on current Size: not applicable — no core paths touched ( Approach: the scope is deliberately tight and the PR explains why: it's a subset of what those rounds produced, with the rest dropped as too close to existing bullets now that Risk: no elevated signals — neither file matches the high-risk revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献! 模板: 标题非标准(用的是 问题: 真实且已验证。这三条规则在 #8242 合并之后才推到它的分支,因此落在了已关闭的分支上,从未进入 方向: 对齐。这是 verify-pr skill 的内部维护——从 #8132 与 #8261 验证轮次提炼的三条审查启发式,在当前 规模: 不适用——未触及核心路径(仅 方案: 范围刻意收紧,且 PR 解释了原因:这是那两轮产出的子集,其余因与现有条目过于接近而未收入( 风险: 无升级信号——两个文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewI read the title and "Why" first and formed my own plan: add the three rules as new bullets in the relevant The three bullets match the file's established voice (bold lead-in, a measured example, the bound on what was and wasn't demonstrated), and each sits in a sensible spot — the two-ends rule beside the minting/accepting security reasoning, the empty-vs-unmeasured and validity-control rules in the ordering section next to the concurrency rule they cite. The new parenthetical on the type-boundary bullet is an honest in-file note explaining a deliberate overlap rather than a silent one — the right call given the sibling-sweep bullet is load-bearing instrumentation. The test additions are idiomatic — the same No correctness, security, or convention concerns. Nothing to block on. Test evidence (this PR's own CI)I don't run PR code here — this quotes the PR's own CI checks on the reviewed commit, fetched via the API. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The ubuntu unit suite is still in progress; the macOS/Windows/integration jobs are skipped by path filters (this PR touches only This is a docs/skill-content change with no product behaviour to settle, so no 中文说明代码审查我先只读标题与「Why」并形成自己的方案:把三条规则作为新条目加入相应 三条条目与文件既有风格一致(加粗领句、一个实测示例、对已演示/未演示部分的有界化),位置也合理——two-ends 规则放在 minting/accepting 安全推理旁,empty-vs-unmeasured 与 validity-control 规则放在其引用的并发规则所在的排序小节。type-boundary 条目上新增的括号说明是对一处刻意重叠的诚实标注,鉴于 sibling-sweep 条目是承载测量的仪器,这是正确做法。 测试改动是惯用写法——与周围 #8005 断言同样的 无正确性、安全性或规范问题。无可阻断项。 测试证据(本 PR 自身 CI)此处不运行 PR 代码——以下引用本 PR 在被审提交上的自身 CI 检查,经 API 获取。ubuntu 单测套件仍在进行;macOS/Windows/集成任务因路径过滤(本 PR 仅触及 这是文档/skill 内容改动,没有需要裁定的产品行为,因此不适用 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — a tight, well-justified skill-maintenance PR that does exactly one thing and pins it with tests; I'd merge without hesitation. Stepping back: the motivation is concrete (three rules stranded on a closed branch, verified absent from My independent proposal matched the PR exactly; I found no simpler path it missed. The diff is all necessary — no drive-by edits, no churn. The test pins are exact-substring and match the added text, so the suite genuinely guards the content. The only nit is cosmetic: the PR body uses its own headings rather than the template's, but the content the template wants is all there, so it's not worth blocking. The one thing standing between this and an approval is CI — the ubuntu unit suite is still in progress on the reviewed commit. I'm not going to attest to a result that doesn't exist yet, so approval is deferred until CI lands green on 中文说明置信度:5/5 —— 一个收紧、有充分依据的 skill 维护 PR,只做一件事并用测试钉住;我会毫不犹豫地合并。 退一步看:动机具体(三条规则被困在已关闭分支,已验证 我的独立方案与 PR 完全一致,没找到它遗漏的更简路径。diff 全部必要——无顺手改动、无 churn。测试钉为精确子串且与新增文本匹配,套件确实守护了内容。唯一的瑕疵是表面性的:PR 正文用了自己的标题而非模板标题,但模板要的内容都在,不值得阻断。 唯一挡在批准之前的是 CI——被审提交上的 ubuntu 单测套件仍在进行。我不会为一个尚不存在的结果背书,因此批准推迟到 CI 变绿。 — Qwen Code · qwen3.8-max-preview Reviewed at |
ReviewDocs-plus-pins PR: three new heuristics in Claims re-checked independentlyEverything the PR body asserts holds up:
The three rules themselves are good ones, and the second and third are the kind that generalise past the incident that produced them. No objection to the content. Findings1. The meta-note is maintainer commentary living inside an agent prompt (
The reasoning is worth preserving; the file is the wrong medium. It reads correctly as a commit-message paragraph, or in the PR body where it already appears almost verbatim. If it has to stay adjacent to the text it protects, a 2. The
Port is not part of same-site computation for any host — registrable domains included — so "for an IP host" attributes the property to the wrong cause. The direct and stronger statement is that cookies are not port-scoped at all (RFC 6265 §8.5: cookies do not provide isolation by port), so scheme+host is the whole boundary:
Same bound, and it survives a reader who checks it. 3. Two of the five pins are the weakest available substring.
As written, the first would stay green against unrelated future text that happens to contain the phrase. The companion pins ( 4. No placement pin. Structural note — flagged, not blocking
The tell that it is already costing something: bullets 2 and 3 of this PR are not about A/B proof at all. "Separate empty from unmeasured" and "run the validity control first" are general code-inspection heuristics about the code under test; they landed in the A/B section because it has become the default drawer. Worth a follow-up in one of two shapes:
The consolidation call in this PR — merge one pair, revert the other and say why in the file — is defensible on its own terms. It is the shape of the container, not the redundancy, that is compounding. VerdictCorrect, verified, and safe to merge. Findings 2 and 3 are worth a follow-up commit before merge (both are one-line edits); finding 1 is a judgement call I would resolve by moving the note out of the prompt. |
There was a problem hiding this comment.
Pull request overview
This PR updates the verify-pr skill’s guidance with three additional hardening rules (and one explicit note about deliberate overlap), and extends the triage workflow test suite to pin the new phrases so regressions are caught.
Changes:
- Added three new verification rules to
.qwen/skills/verify-pr/SKILL.mdcovering (1) checking both minting and accepting ends of capabilities, (2) distinguishing “empty” vs “unmeasurable” results and tracing consumers, and (3) ensuring validity controls run before the artifacts they invalidate are produced. - Added an explanatory parenthetical documenting an intentional overlap to preserve a previously measured “instrument” text byte-identically.
- Extended
scripts/tests/qwen-triage-workflow.test.jsto assert the presence of the new pinned phrases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/tests/qwen-triage-workflow.test.js | Adds assertions that pin the new SKILL.md phrases to prevent accidental removal/rewrites. |
| .qwen/skills/verify-pr/SKILL.md | Adds three new hardening rules (plus a note on deliberate overlap) to improve verification guidance consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM!✅
— qwen3.8-max-preview via Qwen Code /review
|
Released in v0.21.3. |
Why this is a separate PR
These three rules were pushed to #8242's branch at 06:53, and #8242 merged at 06:00. The commit landed on a closed branch and never reached
main—git merge-base --is-ancestorsays so, and none of the three phrases appear inmain'sSKILL.md. Same content, re-cut on currentmain.Change
Three rules from maintainer verification rounds on #8132 and #8261. A deliberate subset:
SKILL.mdis 2.4× its size six days ago and nothing gates its growth, so the other techniques those rounds produced (a PR body describing a head that autofix rounds since removed, a dead CSS rule invalidating a specific test-plan step) were dropped as too close to existing bullets.A capability has two ends — check the one that accepts, not only the one that issues. #8132 found a cookie→
Authorizationbridge correctly gated to the desktop shell on the minting side and mounted unconditionally on the accepting side, so every server instance treated that cookie as a bearer. The generalisable tell is why it hid: the tests were named after the gated end, which makes the ungated end look covered. Bounded the way the file already asks: no exploit was demonstrated, butSameSitedoes not separate127.0.0.1:<other-port>from the daemon's port, because for an IP host the "site" ignores the port."Nothing found" and "could not measure" must be different values — then check what consumes them. #8261 set
emptyDiffboth for a genuinely empty PR and for a diff capture that failed, and the consuming skill answered that flag by recommending the PR be closed as superseded. A transient fetch error could close live work. The damage is done by the consumer, not the flag — which is why the rule ends at the readers. The verdict contract already applies this to our own report (inconclusive, nevermerge-ready, neverfindings); the code under test gets it too.A validity control must run before the artifact it invalidates is built. #8261's re-classifier demoted findings from a dead harness after the findings list was assembled, so a harness proven dead still filed
mutant-survivedagainst the author. Order is the whole property: a control that runs late is not a weaker control, it is not a control.The consolidation the review asked for — one done, one deliberately not
#8242's review named two overlapping pairs.
The first resolved itself: an autofix round moved the observability-ranking bullet next to the concurrency rule it cites, and the two are now 100 lines apart and saying different things (disproving worse readings, versus ranking variants by observability).
The second I merged and then reverted, with a note in the file saying why. The type-boundary bullet does overlap the sibling-sweep bullet directly below it — but sibling-sweep is the one rule in this file with a measured before/after behind it, and consolidating means editing that text. Verified against the staged arm copy still on disk (
skill-armB.md,sha256:82dff804…): the bullet at this head is byte-identical to what the treatment arm read. Editing the instrument is a change to make with a fresh measurement, not on the way past. The redundancy stays and the file now says that out loud.Tests
Three assertions, each mutation-checked — neutering any one of the pinned phrases turns the suite red:
A capability has two endsneuteredmust be different valuesneuteredA validity control must run before the artifact it invalidatesneuteredscripts/tests/qwen-triage-workflow.test.js119/119 on currentmain; prettier, eslint--max-warnings 0clean.中文说明
为什么是一个独立 PR
这三条规则在 06:53 推到了 #8242 的分支,而 #8242 在 06:00 就已合并。该提交落在了一个已关闭的分支上,从未进入
main——git merge-base --is-ancestor如此判定,且这三条短语在main的SKILL.md中均无命中。内容相同,在当前main上重新切出。改动
取自 #8132 与 #8261 两轮维护者验证的三条规则。这是有意的取舍:
SKILL.md已是六天前的 2.4 倍,且没有任何东西约束其增长,因此那两轮产出的其余手法(PR 描述停留在 autofix 已删除的旧 head、死 CSS 规则导致某条 test-plan 步骤不成立)因与现有条目过于接近而未收入。能力有两端——检查"接收"那端,而不只是"签发"那端。 #8132 发现一个 cookie→
Authorization桥在铸造侧正确地限定给桌面外壳,在接收侧却是无条件挂载的,于是每个 server 实例都把该 cookie 当作 bearer。可推广的线索正是它得以藏身的原因:测试是按被门禁的那一端命名的,这让没有门禁的那端看起来已被覆盖。并按本文件既有要求做了有界化:未演示出可利用的攻击,但SameSite无法区分127.0.0.1:<其他端口>与守护进程端口——对 IP 主机而言,「site」忽略端口。「没找到」与「没测成」必须是不同的值——然后查下游拿它做什么。 #8261 的
emptyDiff在「PR 确实没有改动」与「diff 捕获失败」两种情况下都会置位,而消费该标志的 skill 的回应是建议将该 PR 作为 superseded 关闭。一次瞬时的抓取错误就可能关掉仍在进行的工作。伤害由消费者造成,而非标志本身——这正是该规则要落到读取方的原因。裁决契约本就对我们自己的报告施加了这一条(inconclusive,绝不是merge-ready,也绝不是findings);被验证的代码同样适用。有效性控制必须在它所要作废的产物被构建之前运行。** #8261 的重分类器在 findings 列表组装完成之后才降级来自已死 harness 的发现,于是一个被证明已死的 harness 仍然向作者提交了
mutant-survived。顺序就是这条性质的全部:跑晚了的控制不是"弱一点的控制",而是根本不成其为控制。评审要求的两处合并——一处完成,一处刻意保留
#8242 的评审点名了两处重叠。
第一处自行解决了:一次 autofix 轮次把「可观测性排序」条目挪到了它所引用的并发规则旁边,如今两者相隔 100 行,且讲的是不同的事(证伪更坏的读法 vs 按可观测性对变体排序)。
第二处我先合并、随后回退了,并在文件中留下了说明。type-boundary 条目确实与紧随其下的 sibling-sweep 条目重叠——但 sibling-sweep 是本文件中唯一有实测 before/after 支撑的规则,合并就意味着编辑那段文本。对照磁盘上留存的臂副本(
skill-armB.md,sha256:82dff804…)验证:本 head 上的该条目与处理臂实际读到的逐字节一致。改动仪器需要配一次新的测量,而不是顺手为之。冗余予以保留,且文件现在把这一点明说了出来。测试
三条断言,各自做了变异检查——中和其中任意一条被钉住的短语都会让套件变红:
A capability has two endsmust be different valuesA validity control must run before the artifact it invalidates在当前
main上scripts/tests/qwen-triage-workflow.test.js119/119;prettier、eslint--max-warnings 0均干净。