feat(review): enforce the submit-only write contract with a cleanup tripwire#7691
Open
wenshao wants to merge 9 commits into
Open
feat(review): enforce the submit-only write contract with a cleanup tripwire#7691wenshao wants to merge 9 commits into
wenshao wants to merge 9 commits into
Conversation
…ripwire The /review skill's posting ban named one API route — `gh api .../pulls/<n>/reviews` — and a run that had lost the surrounding prose to four context compressions walked around it: it decided its findings were all duplicates, never called submit, and hand-posted a consolidated summary with `gh pr comment`. No authorisation gate, no downgrade semantics, no `posted` fact, no completion line; nothing downstream could tell it had happened. Three layers close this: - SKILL.md Step 7 now bans every write path to the PR (`gh pr comment`, `gh pr review`, `gh issue comment`, mutating `gh api` calls, comment edits/deletes) and routes prose wrap-ups into the review body via compose-review; the overlap-drop branch and Step 9 restate it at the two decision points where runs improvised. - fetch-pr stamps `fetchedAt` (and `host`) into the fetch report — the review window's opening time, on the carrier that exists on every PR run. - cleanup audits that window before sweeping: issue comments by the reviewing account are flagged with warning lines the skill must relay (submit posts reviews, never issue comments, so the overlap with sanctioned output is zero). Best-effort by design — offline or unauthenticated runs skip it silently, and an audit failure never fails the cleanup. Validated against the real bypass: with a window opening at the original run's fetch time, the audit names the exact hand-posted comment.
wenshao
requested review from
LaZzyMan,
tanzhenxin and
yiliang114
as code owners
July 24, 2026 18:08
Collaborator
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. |
Collaborator
|
Re-run on Template: the body doesn't follow the heading structure in the template, but the substance is all there — what it does, why (the incident), how it was validated. Not blocking on format. Problem: observed, not theoretical. The PR documents a specific dogfooded incident where a run lost the Step 7 prose ban to four context compressions, decided its findings were duplicates, and hand-posted a summary with Direction: squarely aligned — the review skill's write contract is core infrastructure, and the incident shows prose-only bans are insufficient against context compression. Detection in the deterministic layer (cleanup) is the right architectural response. Size: 529 production lines (342 cleanup.ts, 78 fetch-pr.ts, 53 submit.ts, 33 receipt.ts, 10 gh.ts, 8 SKILL.md) + 852 test lines. Touches Approach: the three-layer structure (SKILL.md ban expansion → fetch-pr window stamping → cleanup audit) is well-scoped for the problem. The tripwire-not-wall framing is honest — prevention would need argument-level tool permissions, which is correctly out of scope. The receipt contract between submit and cleanup is a clean way to distinguish sanctioned reviews from bypasses. No simpler path comes to mind that covers both the issue-comment and review channels. Moving on to code review. 🔍 中文说明Re-run（ 模板：正文未按模板标题结构组织，但实质内容齐全——做了什么、为什么（事故）、如何验证。不因格式阻塞。 问题：已观测到的事故，非理论加固。PR 记录了一次具体的 dogfood 事故：运行在四次上下文压缩后丢失 Step 7 禁令，判断发现全部重复，用 方向：完全对齐——review skill 的写入契约是核心基础设施，事故表明纯 prose 禁令不足以对抗上下文压缩。在确定性层（cleanup）做检测是正确的架构选择。 规模：529 行生产代码 + 852 行测试。触及 方案：三层结构（SKILL.md 禁令扩展 → fetch-pr 窗口戳 → cleanup 审计）范围合理。tripwire-not-wall 的定位诚实——真正的阻止需要参数级工具权限，正确超出范围。submit 与 cleanup 之间的 receipt 契约是区分授权 review 与绕过的干净方式。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
Code reviewRe-run — the diff is unchanged since the last pass ( Independent proposal: I would have stamped a timestamp in the fetch report, queried issue comments since that timestamp in cleanup, and flagged any by the reviewing account. For the review channel, I'd compare against a receipt submit writes. The SKILL.md ban would expand to name every write path. Comparison: the PR matches and exceeds this. The additions I didn't independently arrive at are all well-motivated: No blockers found. Specific observations:
The two standing scope deferrals from earlier rounds (lightweight-mode audit window, carrier trust) are documented and reasoned — lightweight mode runs TestingThis is an unattended CI run — PR code is never executed by the triage agent. Evidence comes from the PR's own CI checks via the API. The ubuntu test job passed (covers the changed
Real-scenario testing: N/A — the changes are to the review skill's internal subcommands ( 中文说明代码审查Re-run——diff 自上次审查（ 独立方案： 我会在 fetch 报告中记录时间戳，cleanup 时查询该时间戳之后的 issue 评论，标记审查账号所发的。对 review 通道，与 submit 写入的 receipt 比对。SKILL.md 禁令扩展覆盖所有写路径。 对比： PR 与独立方案一致并有超出。drift 重启时保留 未发现阻塞项。纯分类器函数与 I/O 接线分离清晰； 两个遗留范围延期（轻量模式审计窗口、载体信任）有文档和理由——适合作为后续跟进。 测试无人值守 CI 运行——triage agent 不执行 PR 代码。证据来自 PR 自身 CI 检查。 ubuntu 测试通过（覆盖变更的 真实场景测试：N/A——变更涉及 review skill 的内部子命令和 SKILL.md 文档，无用户可见 TUI 界面受影响。 — Qwen Code · qwen3.8-max-preview Reviewed at |
Collaborator
|
Confidence: 4/5 — clean review across every stage; the tripwire is the right architectural response to the incident, the implementation is thorough and well-tested, and all prior review findings are addressed. The one point off is the skipped CI jobs (windows/macos/integration) — not PR-specific, but they leave those runtime paths unverified by CI. Stepping back: the problem is real and documented (a specific dogfooded bypass, not a hypothesis), the solution is well-layered (prose ban expansion for the model, deterministic tripwire in cleanup for when the model ignores the prose), and the implementation is straightforward to maintain. The pure classifier / I/O wiring split makes the logic testable without network mocks, the receipt contract between submit and cleanup is a clean seam, and every skip path names itself so a silent tripwire-off is distinguishable from a clean window. Going back to my independent proposal: the PR exceeds it in the drift-restart handling ( The two standing scope deferrals (lightweight-mode window, carrier trust) are real but correctly bounded — lightweight mode has no fetch-pr carrier to stamp, and the trust boundary sits earlier in the pipeline. Both are sized as follow-ups. Non-blocking nits: the 529 production lines touching core (feat-type, noted for awareness in Stage 1) and the skipped CI jobs. Neither blocks merge. Approving. ✅ 中文说明置信度：4/5——各阶段审查干净；tripwire 是对事故的正确架构响应，实现充分且测试完备，所有先前审查发现均已解决。扣一分是因为 CI 跳过了 windows/macos/集成测试——非 PR 特有，但这些运行时路径未被 CI 验证。 回顾：问题真实且有文档（具体的 dogfood 绕过，非假设），方案分层合理（prose 禁令面向模型，cleanup 中的确定性 tripwire 面向模型忽略 prose 的情况），实现易于维护。纯分类器 / I/O 接线分离使逻辑可脱离网络 mock 测试，submit 与 cleanup 之间的 receipt 契约是干净的接缝，每个跳过路径都有命名，tripwire 关闭与窗口干净可区分。 与独立方案对比：PR 在 drift 重启处理（ 两个遗留范围延期（轻量模式窗口、载体信任）真实但边界正确——作为后续跟进。 非阻塞建议：529 行生产代码触及核心（feat 类型，Stage 1 已记录）和 CI 跳过的任务。均不阻塞合并。 批准 ✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
previously approved these changes
Jul 24, 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. ✅
Collaborator
Author
Review: enforce the submit-only write contract with a cleanup tripwireScope reviewed: OverviewThree layers against a The core argument holds up under checking. Verdict: sound design, merge-ready modulo finding 1. One Medium worth fixing before this ships, because it fires on this repo's own CI and the skill is instructed to relay it verbatim. Findings1. [Medium] The "reviewing account" is not unique to the review run in this repo's CI — expect false-positive warnings.
A review takes many minutes; a push mid-review triggers precheck, which posts, which lands in the window. Cheap fix, using data already in the response: the list payload carries 2. [Low-Medium] The producer half of the contract has no test, and its failure mode is silent.
3. [Low] Skipped and clean look the same on stdout. Every skip path — report missing, pre- 4. [Low] The ban covers edits and deletes; the tripwire only sees creations. Step 7 now bans "editing or deleting existing comments", but the classifier requires 5. [Low] The common case is an empty window. 6. [Low] Report-derived values reach the
Nits
The residual, and where the wall could actually goThe PR body is honest that this is a tripwire, not a wall. Worth stating the residual precisely: the incident's cause was compression dropping Step 7's prose, and invoking Two things follow that are worth naming, both outside this diff:
What's good
中文说明总览三层防线：Step 7 的禁令从单条 API 路由扩展到所有写通道； 核心论证经核查成立： 问题
细节
残余风险与真正的"墙"应在何处调用 两点值得记录（均在本 diff 之外）：CI 中 优点零重叠论证是核查出来的而非断言；纯函数分类器独立可测，"缺作者/缺时间戳就丢弃"对一条会点名的输出是正确默认；best-effort 容错有测试而非口头承诺；审计排在删除自身输入的清扫之前，且调用点注释写明了原因。 |
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.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
…edit class, named skips Addresses the review on the tripwire design: - Medium: in CI the reviewing account is the bot that precheck/triage also post from, so their marker-stamped comments (<!-- qwen-… -->) are filtered out and the warning prose gains the third reading (another workflow under the same account). Without this, every mid-review push produced a false accusation the skill is told to relay verbatim. - Low-Medium: producer contract tests — fetch-pr's report is now pinned to carry fetchedAt (a real timestamp) and --host, since dropping either silently turns the audit off with output identical to a clean window. - Low: every skip path names itself on stderr (note: bypass audit skipped (…)), so the tripwire's off state is distinguishable from its all-clear state. - Low: pre-window comments edited inside the window are flagged as an edit class of their own — ?since= filters on updated_at, so the rows were already fetched. Verified empirically that reactions do not bump an issue comment's updated_at before adding this. - Low: the empty-window early return skips the currentUser() round trip; report-derived prNumber is cross-checked against the cleanup target and ownerRepo is shape-checked before either reaches a gh api path. - Nits: use the tmpFile helper instead of re-deriving its path; SKILL.md's fetch-pr snippet now shows --host (the report records it and the audit queries it — a dropped host silently audits github.com).
Collaborator
Author
|
All six findings + nits addressed in 60f37c3. The Medium was the important catch — thanks for tracing the CI token plumbing; a tripwire that cries wolf on every mid-review push would have trained everyone to ignore it.
On the residual: agreed on both counts, and they're good follow-ups — turning |
wenshao
commented
Jul 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
中文说明
已审查。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
wenshao
commented
Jul 24, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— Codex GPT-5 via Qwen Code /review
| (c) => | ||
| (c.user?.login ?? '').toLowerCase() === reviewerLc && | ||
| typeof c.created_at === 'string' && | ||
| !(c.body ?? '').includes(AUTOMATION_MARKER), |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Do not trust the comment body as automation provenance.
A hand-posted summary that quotes <!-- qwen-—or a write deliberately carrying that text—is removed here, so cleanup reports a clean window even though the write bypassed submit. Verify automation provenance independently, or keep marked comments visible to the tripwire.
— Codex GPT-5 via Qwen Code /review
| ): { window: AuditWindow } | { skip: string } { | ||
| let raw: string; | ||
| try { | ||
| raw = readFileSync(tmpFile(target, 'fetch.json'), 'utf8'); |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Create the audit window for lightweight cross-repository reviews too.
That supported path runs plan-diff rather than fetch-pr, so this fetch report never exists. A bypass gh pr comment in a lightweight review therefore produces only “audit skipped” and recreates the original incident undetected. Write a dedicated audit-window file for every PR mode, or make plan-diff produce the required metadata.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Real gap, deliberately deferred: lightweight mode never runs fetch-pr, so there is no carrier to stamp today — writing one means teaching pr-context (the one subcommand that mode does run) to emit an audit-window file, which changes its pure-API contract. Follow-up sized; the worktree path this PR covers is where the incident actually happened.
| ): { window: AuditWindow } | { skip: string } { | ||
| let raw: string; | ||
| try { | ||
| raw = readFileSync(tmpFile(target, 'fetch.json'), 'utf8'); |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Do not trust an unauthenticated carrier writable by the execution being audited.
Reviewed code or another in-run command can replace this sibling file with a valid-looking future timestamp or different valid repository, causing cleanup to query an empty window. Store the boundary outside the reviewed execution’s write authority or authenticate the report before consuming it.
— Codex GPT-5 via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Agreed on the boundary, disagree on where it sits: by the time cleanup reads this file, build-test has already executed the PR's own code with the user's privileges — a malicious PR can tamper far more than one JSON carrier. Hardening the carrier alone would not move the trust boundary; doing it properly (state outside the execution's write authority) is the broader reviewed-code-is-untrusted work, not this tripwire. The cross-check against the cleanup target and the owner/repo shape test in 60f37c3 close the accidental-corruption half.
…rity, provenance Addresses the second review round on the tripwire: - Review-channel coverage: submit now records a receipt (the one review id it was authorised to create, parsed from the POST response), and cleanup flags any in-window review by the reviewing account the receipt does not vouch for — `gh pr review` and direct POSTs to pulls/<n>/reviews were bannable but invisible before. No receipt vouches for nothing (fail-safe). - Boundary integrity across restarts: fetch-pr preserves the earliest window opening as auditSince when it overwrites its own report (the head-drift rule reruns it), so writes made during an abandoned attempt stay inside the audit window. A window from a different PR left at the same path is not inherited. - Clock skew: the audit boundary backs off two minutes from the recorded opening — fetchedAt is local time compared against GitHub's server timestamps, and a fast local clock could otherwise hide the first moments of the review. Errs toward over-flagging. - Marker provenance: the automation filter is anchored to the body START, so a hand-posted summary that merely quotes a marked bot comment (or hides the marker mid-body) stays visible to the tripwire. - Named skip fidelity: ENOENT alone means "no fetch report" (any other read failure names its code); gh failures surface the first non-empty stderr line instead of the generic Command-failed wrapper. - Tests: rogue-review flag + receipt exclusion, skew boundary, auditSince window, edited-warning rendering through runCleanup, malformed-report table, EACCES vs ENOENT, stderr extraction, marker-quoting visibility, and producer tests for auditSince preservation.
Collaborator
Author
|
Second round addressed in 52ebf1a — the review-channel and boundary findings were the real gaps in the tripwire's coverage. Per finding:
Two findings I'm deliberately not fixing in this PR — reasoning in the threads: the lightweight-mode window (real, but it needs a carrier that mode doesn't produce today; follow-up) and the writable-carrier trust boundary (the audited execution already runs PR code with user privileges at build-test time, so the report is not a new boundary — hardening it belongs to the larger "reviewed code is untrusted" work). |
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.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.7-max via Qwen Code /review
doudouOUC
reviewed
Jul 25, 2026
doudouOUC
requested changes
Jul 25, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) and macOS/Windows Node 22 test jobs were skipped in CI; the local CLI suite ran the changed test files (42/42 pass) but did not exercise the integration paths those jobs cover.
[Critical] cleanup.ts:187 — Lightweight mode audit window: plan-diff path never produces a fetch report, so bypass writes in lightweight reviews go undetected. Author defers to follow-up; the gap is real and present in the code at this commit.
[Critical] cleanup.ts:187 — Unauthenticated carrier trust: the fetch report is writable by the execution being audited. Cross-check and OWNER_REPO_RE close accidental corruption; deliberate tampering by reviewed code remains unaddressed. Author argues the broader trust boundary is out of scope.
— qwen3.7-max via Qwen Code /review
…tch-pr mock reset - SKILL.md told the orchestrator to route a free-form prose wrap-up through compose-review's input, but ComposeReviewInput has no free-text body field — the body is computed from structured state, by design (the model does not author PR-facing prose). Following that instruction would force the model to misuse a structured field or forge a `body` submit refuses. Both passages now say the recap belongs in the TERMINAL summary; the PR receives only submit's computed body plus inline comments. This aligns the guidance with the submit-only principle rather than contradicting it. - fetch-pr.test.ts's producer beforeEach re-set git/gh but not readFileSync, and clearAllMocks does not reset implementations — so a mockReturnValue from one test could leak into a later test relying on the default. The beforeEach now re-asserts the ENOENT default, removing the ordering dependency. The receipt-overwrite and receipt-untested suggestions from the same review were already addressed in 8755464 (this review ran against the prior commit).
Collaborator
Author
|
@qwen-code /triage |
Collaborator
Author
|
Thanks — the COMMENT review's four suggestions, dispositioned:
The CHANGES_REQUESTED review re-lists the two standing scope deferrals (lightweight-mode window, carrier trust) — reasoning unchanged, in the threads above; nothing new there. Full |
yiliang114
reviewed
Jul 25, 2026
yiliang114
reviewed
Jul 25, 2026
yiliang114
reviewed
Jul 25, 2026
| try { | ||
| const prev = JSON.parse(prevRaw) as { | ||
| prNumber?: unknown; | ||
| fetchedAt?: unknown; |
Collaborator
There was a problem hiding this comment.
[P2] auditSince from a previous report is trusted as-is after a Date.parse check, but there is no upper-bound validation.
A corrupted prior report with a far-future auditSince (e.g. "2099-01-01T00:00:00.000Z" — valid ISO-8601, passes Date.parse) would make the audit window effectively empty: boundary = 2099 - 2min, and no real comment would be >= boundary. The tripwire would silently report a clean window.
Consider clamping auditSince to <= fetchedAt or at least to <= now. A value in the future is always a sign of corruption:
if (prevSince < auditSince && Date.parse(prevSince) <= Date.now()) {
auditSince = prevSince;
}The OWNER_REPO_RE and PR-number cross-check protect against accidental corruption, but a future-timestamp is a plausible corruption mode (crash mid-write of a partially-written timestamp string).
Collaborator
Author
There was a problem hiding this comment.
Handled in 92400f1 (with a clarifying comment rather than new code): the existing guard already rejects a far-future timestamp — prevSince < auditSince where auditSince is fetchedAt (now) is also the upper bound, so the window only ever moves BACKWARD and a '2099-…' value fails the comparison. I added a comment making that intent explicit since it wasn't obvious on read. If you'd prefer a belt-and-suspenders explicit clamp too, happy to add it.
yiliang114
reviewed
Jul 25, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. 3 suggestions inline.
Summary: Solid defensive-programming PR. The bypass-audit tripwire is well-structured: the fetch report carries the audit window, the submit receipt accumulates sanctioned review IDs, and cleanup correlates both sides. Error handling is consistently best-effort (skip-and-name rather than fail). Test coverage is thorough — 20+ new test cases covering edge cases (clock skew, multi-id receipts, malformed reports, Enterprise host, drift restarts, EACCES vs ENOENT).
Inline findings (all P2):
cleanup.ts:304—setGhHostleaks module-level state. The Enterprise host set during audit is never restored. No practical impact today (cleanup runs last), but a latent hazard if anything runs after.cleanup.ts:82— Duplicated receipt-parsing logic.readSubmitReceipthere andreadReceiptIdsinsubmit.tsare near-identical; schema changes need updating in two places.fetch-pr.ts:319—auditSincehas no upper-bound validation. A corrupted prior report with a far-future timestamp passesDate.parsebut makes the audit window empty.
Not re-flagged (already addressed by prior reviews or out of scope):
- Lightweight-mode audit window gap (no fetch report from
plan-diff) — acknowledged by author as follow-up. - Unauthenticated carrier trust — broader trust boundary, author argues out of scope.
- Automation marker spoofing — intentional design (fail-safe over-filtering).
Collaborator
|
Triage re-run completed without a new review. The stage comments above were updated with the latest result. View workflow run. |
qwen-code-ci-bot
previously approved these changes
Jul 25, 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 — CI landed green after the review. ✅
Collaborator
Author
|
Acknowledging the latest CHANGES_REQUESTED:
The four COMMENT-review suggestions from the parallel review were dispositioned in my previous comment (two already fixed in 8755464, the SKILL.md prose-field contradiction and the mock-reset fragility fixed in 96f2848). |
… parser, auditSince bound Three findings from @yiliang114: - cleanup's audit set the gh host for the PR but never restored it — harmless today (cleanup runs last) but a leak if any code runs after. It now saves the prior host (new getGhHost) and restores it in a finally, so the Enterprise override is scoped to the audit block. - submit's readReceiptIds and cleanup's readSubmitReceipt duplicated the receipt-shape parse (reviewIds[] + legacy reviewId migration). Extracted to lib/receipt.ts parseReceiptIds so a schema change is one edit both inherit; the two call sites now differ only in their empty wrapper ([] vs Set). Unit-tested directly. - fetch-pr's auditSince adoption: documented that `prevSince < auditSince` (auditSince = fetchedAt = now) is also the upper bound — a corrupted far-future timestamp is rejected because the window only ever moves backward, so it cannot push the audit ahead of every real comment. Tests: host-restore assertion in cleanup, parseReceiptIds table.
…er test The drift-restart producer test seeded only fetchedAt, leaving the auditSince-over-fetchedAt branch (the third-restart case, where a prior report already carries an earlier auditSince) untested. Added a case seeding both. Reported by @yiliang114.
doudouOUC
reviewed
Jul 25, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Unresolved, please confirm: [Critical] cleanup.ts — Lightweight mode audit window: plan-diff path never produces a fetch report, so bypass writes in lightweight reviews go undetected. Author defers to follow-up; whether this scope deferral is appropriate cannot be determined from the code alone. [Critical] cleanup.ts — Unauthenticated carrier trust: the fetch report is writable by the execution being audited. Cross-check and OWNER_REPO_RE close accidental corruption; deliberate tampering by reviewed code remains unaddressed. Author argues the broader trust boundary is out of scope.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 25, 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. ✅
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
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 this PR does
Closes the hole through which a
/reviewrun can write to the pull request without going throughqwen review submit: the write ban now covers every channel, andcleanupgains a deterministic tripwire that flags bypass writes in the terminal.The incident this fixes
The Step 7 ban named exactly one API route —
gh api .../pulls/<n>/reviews. A dogfooded run that had lost the surrounding prose to four context compressions walked around it: it decided its findings were all duplicates of existing inline comments, never calledsubmit, and hand-posted a consolidated summary withgh pr comment. That write carried no authorisation gate, no downgrade semantics, nopostedfact, and noReview complete:line — nothing downstream could tell it had happened. Prose bans are exactly what compression loses, so detection has to live in the deterministic layer that always runs.Three layers
gh pr comment,gh pr review,gh issue comment, mutatinggh apicalls against the PR'sissues/*/pulls/*endpoints, and comment edits/deletes. Prose that should reach the PR goes in the review body viacompose-reviewand rides the one sanctioned write. The overlap-drop branch (where the incident's improvisation started) and Step 9 restate the rule at the decision points.fetch-prstamps the review window —fetchedAt(ISO-8601) andhostare added to the fetch report. The report is the carrier, not the worktree lease, because it is written on every PR run (the lease needs session env vars).cleanupaudits the window — before sweeping the report away, it lists issue comments on the PR sincefetchedAtand flags any by the reviewing account withwarning:lines the skill must relay verbatim.submitposts reviews and never issue comments, so overlap with sanctioned output is zero — no receipt correlation needed. A flagged comment is either a bypass or something the user posted by hand; the warning names both readings and lets the human decide.The audit is a tripwire, not a wall — prevention would need argument-level tool permissions, which is out of scope here. It is best-effort by design: offline / unauthenticated / old-report runs skip it silently, and an audit failure never fails the cleanup (covered by tests).
Validation
gh/fs, silence on clean windows, skip on missing/old reports, failure tolerance); fullsrc/commands/review/suite green (31 files, 799 tests);eslint --max-warnings 0andtsc --noEmitclean.cleanupon the live PR names the exact hand-posted comment (comment 5068098072 …), plus the maintainer's own hand-posted comments in the same window — the documented ignore-if-yours case.中文说明
本 PR 做了什么
封堵
/review绕过qwen review submit直写 PR 的通道：写入禁令扩展到所有通道，并给cleanup增加一个确定性 tripwire，在终端点名绕过 submit 的写入。起因事故
Step 7 的禁令原本只点名一条 API 路由——
gh api .../pulls/<n>/reviews。一次 dogfood 运行在四次上下文压缩丢失周边 prose 之后绕了过去：它判断发现全部与存量评论重复，未调用submit，用gh pr comment手工发了综合总结。这次写入没有授权门、没有降级语义、没有posted事实、没有Review complete:行——下游一切契约都无从感知。压缩丢掉的恰恰是 prose 禁令，所以检测必须放进永远会执行的确定性层。三层防线
gh pr comment、gh pr review、gh issue comment、对 PRissues/*/pulls/*端点的写方法gh api调用、以及编辑/删除既有评论。要发到 PR 的文字进 review body（经compose-review），走唯一授权写入。overlap-drop 分支（事故的起点）和 Step 9 在决策点复述规则。fetch-pr记录审查窗口——fetch 报告新增fetchedAt（ISO-8601）与host。选报告而非 worktree lease 作载体，因为报告每次 PR 运行必写（lease 依赖 session 环境变量）。cleanup审计窗口——在清扫报告之前，列出fetchedAt以来 PR 上的 issue 评论，凡审查账号所发均以warning:行点名，skill 必须原样转述。submit只发 review、从不发 issue 评论，与授权输出零重叠，无需回执对账。被点名的评论要么是绕过写入、要么是用户本人手发；警告同时给出两种解读，交由人判断。审计是 tripwire 不是墙——真正的阻止需要参数级工具权限，超出本 PR 范围。设计为 best-effort：离线/未认证/旧报告一律静默跳过，审计失败绝不影响 cleanup（有测试覆盖）。
验证
src/commands/review/全量绿（31 文件、799 测试）；eslint --max-warnings 0与tsc --noEmit干净。cleanup，审计精确点名了当时手发的那条评论（comment 5068098072 …），以及同窗口内维护者本人手发的评论——即文案中"自己发的可忽略"的情形。