feat(ci): add automated PR failure patrol - #6766
Conversation
|
Thanks for the PR! (Updated on re-run — current head Template looks good ✓ — all required headings present. Problem: this is a new automation feature, not a bug fix — the problem (maintainers manually triaging stale PR CI failures) is real and well-motivated. No reproduction needed for a feature addition. The PR's local read-only scan against Direction: aligned with the project's background-automation direction. CI failure patrol fits naturally alongside the existing autofix and triage infrastructure. The scope has been tightened considerably through the review history — the PR now ships exactly what the title promises: rerun / comment / no_action on stale PR failures, with classifier hardening and state management baked in. Size: 952 production logic lines ( Approach: the scope feels right — one focused feature with supporting infrastructure (skill file, workflow, tests). The architecture is clean: 中文说明感谢贡献！（re-run 更新——当前 head 模板完整 ✓ — 所有必要段落都在。 问题：这是一个新的自动化功能，不是 bug 修复——问题（maintainer 手动巡检 stale PR CI failure）是真实存在的。功能新增不需要复现。PR 中针对 方向：与项目的 background-automation 方向一致。CI failure patrol 和现有 autofix、triage 基础设施自然衔接。范围在 review 历史中显著收窄——PR 现在精准地实现了标题承诺的内容：对 stale PR failure 执行 rerun / comment / no_action，分类器加固和状态管理已内置。 规模：952 行生产逻辑（ 方案：范围集中——一个聚焦的功能加配套基础设施（skill 文件、workflow、测试）。架构干净： — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal: for a CI failure patrol, I'd want a scheduled Action that scans open PRs for stale failures, an LLM classifier to categorize them, a driver script that acts on decisions with credential isolation, state management to prevent repeated actions, and log redaction to avoid leaking secrets into comments. The PR matches or exceeds this baseline. The three-job pipeline ( Security model is well-considered: No critical blockers found. The code is straightforward, well-named, and follows project conventions (ESM, named exports, no Reuse checkNo existing patrol/rerun infrastructure exists in the repo. The TestingUnit tests: 30 tests pass across 2 test files (25 in Real-scenario testing (tmux): N/A — this is CI infrastructure (GitHub Actions workflow + Node.js script) with no user-facing CLI behavior. The patrol runs inside GitHub Actions and writes to PR comments via the GitHub API. There is no interactive terminal surface to drive. Unit tests are the appropriate verification mechanism here. 中文说明代码审查独立方案：对于 CI failure patrol，我需要一个定时 Action 扫描 open PR 的 stale failure，LLM 分类器进行归类，一个 driver 脚本根据决策执行操作并隔离凭据，状态管理防止重复操作，以及日志脱敏避免泄露 secrets。 PR 的方案达到或超越了基线。三阶段流水线（ 安全模型考虑周全： 未发现关键阻断问题。代码简洁、命名清晰、遵循项目惯例（ESM、named exports、无 测试单元测试： 2 个文件共 30 个测试全部通过（ 真实场景测试（tmux）： 不适用——这是 CI 基础设施（GitHub Actions workflow + Node.js 脚本），没有用户可见的 CLI 行为。patrol 在 GitHub Actions 内部运行，通过 GitHub API 写入 PR 评论。没有可驱动的交互式终端界面。单元测试是此处合适的验证机制。 — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — solid implementation of a real maintainer pain point; clean architecture with well-designed security model; 30 tests pass. Stepping back: this PR solves a genuine problem — maintainers spending time manually triaging stale CI failures — and it does so with an architecture that earns trust at every layer. The classifier has no credentials. The driver revalidates state before every write. The action budget is capped. Logs are redacted. Mentions are neutralized. Every guardrail I'd want for a system that writes to PRs is present. The code is straightforward — no over-abstraction, no unnecessary complexity. The 30 unit tests cover the critical paths well, including the security-sensitive areas (credential redaction, mention neutralization, input integrity). The workflow design is clean: One minor reservation: the The PR is ready to ship once the required secrets ( 中文说明信心：4/5 — 扎实地实现了 maintainer 的真实痛点；架构干净，安全模型设计合理；30 个测试全部通过。 退一步看：这个 PR 解决了一个真实的问题——maintainer 花时间手动巡检 stale CI failure——并且以在每个层面都赢得信任的架构来实现。分类器没有凭据。driver 在每次写入前重新验证状态。action 预算有上限。日志被脱敏。mentions 被中和。对于一个会写入 PR 的系统，我想要的每一个防护措施都在。 代码简洁——没有过度抽象，没有不必要的复杂性。30 个单元测试覆盖了关键路径，包括安全敏感区域（凭据脱敏、mention 中和、输入完整性）。workflow 设计干净： 一个小顾虑： PR 可以在上游仓库配置所需 secrets（ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ The test env isolation issue flagged in the review is minor — fix it if CI catches it.
Code Review — PR #6766 (feat(ci): add stale failure patrol)Reviewed SHA: SummaryThis is a well-structured CI automation PR. The credential isolation between classification and action steps is clean, the attempt capping and stale threshold logic are correct, and the test coverage is thorough. Below are Suggestion-level observations for the author's consideration. Suggestions1. SAFE_OTHER_DECISION mutable shared object ( The module-level constant is returned by reference from
2. selectPrTarget skips baseRefName check when falsy (
3. base_refresh downgrade preserves confidence ( When
4. commandAct silent return on stale target ( When the PR target is no longer current, the function returns silently with no stdout or log output.
5. renderPrActionMarker posts invisible marker only ( The
6. commandAct silent return on no_op ( When the bot has already acted on this SHA (a prior marker exists), the function exits silently.
7. Internal functions untested (
Nice to have8. argsMap CLI parser ( Values starting with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Handled the bounded CI patrol review items in commit 025b17a. Fixed:
Not expanded in this closeout:
Verification:
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Independent review — the trust model is solid. The credential split is clean (classify runs with GH_TOKEN/GITHUB_TOKEN empty and only sandboxed read_file/write_file; scan uses github.token read-only; act/reset use CI_BOT_PAT). The trusted driver revalidates everything before writing — in actOnDecision: PR state/draft/base/headSha, current run+job id match, alreadyHandled, currentActionCount >= 3, and currentFailure all gate the mutation. Mutation ordering is correct: rerun calls rerunFailedJobs before writing the marker, so a failed API call cannot create phantom handled state. validDecision requires confidence: 'high' for any non-no_action decision, bounds reasons to 200 chars, and matches all identity fields (prNumber/headSha/runId/runAttempt/failureKey). The act step verifies ci-flaky-input.json by SHA-256 against the value passed from scan, so the (untrusted) classifier cannot tamper with candidate inputs. safeReason neutralizes mentions and HTML, and the test asserts @\u200b… zero-width-break and <script> escaping. LGTM on the security/correctness boundary.
[P2] PR body advertises an update_branch action that the driver intentionally rejects. The body says the patrol will "update a behind branch only when a supplied main-only commit clearly fixes the logged error," but ACTIONS = new Set(['rerun', 'comment', 'no_action']), validDecision rejects anything else, SKILL.md enumerates only those three, and ci-flaky-rerun.test.js explicitly asserts action: 'update_branch' yields zero client calls. So a classifier that returned update_branch would be silently dropped (stderr only), and a maintainer reading the body would expect auto-update-branch behavior that never happens. Either land the update_branch path or trim the body to the three actions actually shipped — otherwise the description and behavior diverge.
[P3] rerun success + marker-comment failure can double-rerun. If client.rerunFailedJobs(runId) succeeds but client.comment(marker) throws, the catch in actOnDecisions swallows it, no marker is written, currentActionCount is unchanged, and the next patrol tick sees the same unhandled failing run and reruns it again (until the marker lands or count hits 3). Rerunning failed jobs is effectively idempotent (new attempt), so this wastes CI budget rather than corrupting state — but if comment failures persist (e.g. a transient GitHub API blip), a flaky run could be rerun several times within the 3-count budget. Consider a best-effort marker retry or a short "rerun issued" tombstone written before the API call with a separate "confirmed" marker after — the current ordering is the right default, just noting the gap.
|
Automated follow-up summary:
The remaining suggestion threads are being resolved under the minimal-scope policy. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Unresolved, please confirm: [Critical] behindBy/ahead_by compare semantics (comments 3569405514, 3568181669) — referenced code (compare/, behindBy) absent at this commit [Critical] stateMarkers first-match-only (comment 3568654791) — one-marker-per-comment design appears intentional, cannot confirm forgery vector [Critical] Reset matching discards workflowName (comment 3570587076) — function already checks workflowName via succeededAfter, claim may reference prior version [Critical] Green-main SHA not bound atomically (comment 3570587083) — update_branch action absent from current code [Critical] update_branch lacks main CI validation (comment 3572220757) — update_branch action absent from current code [Critical] Failed pending lookup aborts recovery (comment 3574503732) — no pending-state recovery logic in current code [Critical] Recovery marker timing (comment 3574503735) — no recovery logic in current code
— qwen3.7-max via Qwen Code /review
|
Checked review #6766 (review) against current head No actionable Critical remains on the current code:
Paginated reviewThreads currently show 0 active unresolved threads. Qwen Code CI and the automatic review are passing; |
|
@qwen-code /triage |
|
Follow-up to the latest top-level review: no additional code change is needed on the current head. The |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
The scope has been tightened to exactly what the title promises, the trust boundaries between scan/classify/act are correctly drawn, and the 26-test matrix covers the reachable behavior and security boundaries. Prior CHANGES_REQUESTED from this bot on 612d8439 was on code paths no longer present on the current head — see the Stage 3 comment for details.
|
Patrol summary for
Two broader trust/state issues are intentionally not expanded automatically: making rerun dispatch plus marker persistence crash-atomic requires a pending/completed transaction design, and trusting a marker by bot login is insufficient while multiple workflows share the same bot PAT (a dedicated identity or signed state is needed). Both cross the PR's architecture/security boundary and remain human-gated for maintainer direction. Fresh CI and exact-head automated review are still running, so this is pending verification rather than complete. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] jobLog ZIP vs plain text (comment 3575751261) — author verified plain text with live scan; cannot verify API behavior from code alone [Critical] Durable terminal-state protocol for rejected/malformed decisions (comment 3572220726) — truncated body, references removed update_branch paths [Critical] PR freshness TOCTOU at action time (comment 3568181666) — headRefOid-only check may be insufficient but is documented design limitation
— qwen3.7-max via Qwen Code /review
|
Closeout for the remaining review suggestions: I am not adding these three extra regression tests in this PR. They are test-hardening suggestions around future regressions, not current CI failures or missing fixes for the behavior changed in this closeout. This PR has already had multiple review-fix rounds, so I am keeping this as follow-up scope instead of expanding the patrol test matrix again. |
|
Closeout for this round:
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output. Unresolved, please confirm: [Critical] B12 (comment 3568654791): parseMarker captures only first marker match — the specific forged-marker-in-reason attack is mitigated by safeReason escaping < and >, but the non-global regex pattern remains [Critical] B29 (comment 3574503735): recovery writes fresh completed marker after rerun succeeded — action count resets, unclear if this is a bug or intended behavior [Critical] B31 (comment 3575751261): jobLog may fetch ZIP binary instead of plain text depending on GitHub API behavior — needs live testing to confirm
— qwen3.7-max via Qwen Code /review
|
Automation pass: I reviewed the six new test-coverage suggestions around Leaving these as follow-up/no-code items and resolving the threads so this PR can stay bounded. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
Coverage: 5/5 diff chunks reviewed (chunk agents + whole-diff cross-file/test-coverage/build&test passes). Build, typecheck, lint, and the 30 patrol tests all pass on this head.
Re-checked the 39 open blockers against the current head (c638f90af). The patrol was substantially rewritten — the update_branch action, the behindBy/main-green-evidence logic, and the pending/completed marker-recovery path were all removed, which retires every blocker that targeted them. The rest are fixed by the current code; none still stands as a blocking defect.
Fixed by this diff: non-flaky/no_action now posts a hidden marker so it isn't re-classified forever (3566358597); per-decision error isolation + decision-array dedup (3567365019, 3568181656); Cookie redaction is unanchored, not column-1 (3568181652); per-PR dedup moved after handled-filtering so an older unhandled failure isn't starved (3568181653); action count keyed on head SHA (3568181659, 3572220760); first-match marker forgery neutralized by safeReason HTML-escaping (3568654791); attempt bound from the live run REST object, not the rollup (3570587071, 3568181671, 3572220734); rerun dispatched before the marker (3570587073); reset matches workflow+check+runId (3570587076); TIMED_OUT admitted (3570587089); marker is check-specific (3570587094); capped targets filtered before classification (3570587099); malformed decision handled inside the per-entry try (3572220743); queued-check ordering by DB id (3572220763); currentPr projects number so the cap actually trips (3581057773); action-time freshness checks state/draft/base/head/run/job together (3568181666); gh stderr surfaced (3566358602); scan no longer pipes through tee (3568181676); classifier restricted to read_file/write_file with empty GH tokens (3568181678); reset step runs under if: always() (3572220739); the MAX_ACTIONS test now isolates the cap via an attempt mismatch (3578525728); act writes use CI_BOT_PAT, so the read-only job permissions are correct rather than a 403 (3575461246); and the behindBy/main-green/atomic-main/update_branch blockers went away with the action (3569405514, 3568181668, 3568181669, 3570587083, 3572220757); pending-state recovery removed (3574503732, 3574503735).
Documented design trade-offs (not blocking): rerun-before-marker can double-rerun if the marker comment write fails (3566358599), and malformed/low-confidence decisions post no marker (3569452204, 3572220726) — both are called out in the PR's Risk & Scope as deferred crash-safe decisions. Reset is skipped only when the whole classify job fails (3569405528), which currentActionCount's succeededAfter reset makes bookkeeping-only. Newest-first ordering can starve older failures under a sustained >5-failures/10-min arrival (3572220754) — a low-probability edge given the 7-day window and bounded batch.
Refuted by author evidence (not blocking): jobLog fetching a ZIP (3575751261) — the author verified with a live read-only scan that gh api .../jobs/{id}/logs returns plain text; not independently confirmable from code alone, but the standard endpoint behavior plus the live evidence resolve it.
— qwen-latest-series-invite-beta-v77 via Qwen Code /review
| expect(Math.max(...lines.map((line) => line.length))).toBeLessThanOrEqual( | ||
| 300, | ||
| ); | ||
| expect(evidence).not.toContain('private-material'); |
There was a problem hiding this comment.
[Suggestion] The private-material assertion is a dead guard. The -----BEGIN/END PRIVATE KEY----- block sits at the top of the input, so skillLog drops it from the selected window (it is outside the fallback-error context window and outside the trailing-20 lines) whether or not redactLog redacts it. I confirmed with the real skillLog: the redacted [redacted private key] line never reaches the output. So if the private-key regex in redactLog regressed, this test — the only one that feeds a private key — would still pass and key material would silently leak into the classifier evidence. (The sibling secret assertions are real guards because those lines fall inside the selected window.)
Suggested fix: Feed the private-key block adjacent to a matched error line so only redaction can prevent the leak, e.g. skillLog(['TypeError: boom', '-----BEGIN PRIVATE KEY-----', 'private-material', '-----END PRIVATE KEY-----'].join('\n')) and assert not.toContain('private-material').
| ) { | ||
| continue; | ||
| } | ||
| const log = skillLog(await client.jobLog(target.jobId)); |
There was a problem hiding this comment.
[Suggestion] scan downloads the full job log (client.jobLog) before applying the actionCount >= MAX_ACTIONS cap. A long-lived genuinely-failing PR that has already hit the 3-action cap stays in status:failure within the active window, so every 10-minute patrol re-walks it — run → job → jobLog (up to the 16 MB buffer) → paginated comments → currentPr → run — only to continue at the cap. The cap check needs only comments + currentPr, not the log.
Suggested fix: Fetch comments and currentPr and check currentActionCount(...) >= MAX_ACTIONS (plus the cheap PR-state predicates) before the jobLog download. alreadyHandled must stay after the log since it compares failureKey.
| ); | ||
| } | ||
|
|
||
| function safeReason(reason) { |
There was a problem hiding this comment.
[Suggestion] safeReason neutralizes HTML (<, >, &) and @-mentions but not Markdown image syntax. reason_en/reason_zh are LLM-derived from attacker-influenceable CI logs; if the LLM echoes  into a reason, gh pr comment renders it and GitHub fetches the image (through camo) for every maintainer who views the PR — an info-leak channel in the same defense this function is building. Low confidence: it requires the sandboxed LLM to faithfully copy attacker text into a <=200-char reason.
Suggested fix: Break Markdown image syntax the same way @ is handled, e.g. add .replaceAll('!', '!\u200b') (a ZWSP after ! stops  from rendering).
(confidence: low)
… compute the verdict (QwenLM#6892) * fix(review): prove coverage on both topologies, and check the prompt survived the trip Three defects, all measured against the harness's own transcripts of real /review runs against QwenLM/qwen-code PRs QwenLM#6766 (Step 3B) and QwenLM#6579 (Step 3A). 1. Step 3A reviews were told nobody had read them. Coverage was attributed by one question: did an agent whose launch prompt says `chunk N of M` make a successful tool call? No Step 3A prompt says that — there every dimension agent walks the whole diff — so no chunk was ever attributed to anyone. Run against a real 3A review whose twelve agents each opened the diff, walked both chunks and filed findings, check-coverage returned Coverage: 0/2 chunk(s) reviewed. 16 agent(s) ran; 16 did work ERROR: 2 chunk(s) were not reviewed — 1, 2. Nobody read those lines. in one breath. compose-review runs the same computation on the way to the verdict, so a flawless small-PR review was capped away from Approve and the body it would have POSTED to the pull request said nobody had read it. Step 3A is the topology most pull requests get. The only reason it never blew up is that check-coverage lived inside Step 3B and was never reached from 3A — two bugs cancelling. Coverage is now the intersection of two things the harness wrote down: the lines each agent was pointed at (its launch prompt) and the fact that it opened the diff (a successful tool call naming the diff file). Topology-blind. It also no longer credits a chunk to an agent on the strength of any successful call — a glob for test files was enough. 2. The whole-diff agents were still launched blind. agent-prompt built the territory agents' prompts and left the other half of the fan-out to prose. All three whole-diff agents of the 3B run — cross-file tracer, test-coverage matrix, build & test — got a prompt naming no diff file at all. The test-coverage matrix was told to "Read the diff chunks" and given no path to read them from; it read the post-change source instead, which on a deletion shows it nothing. These agents own the classes a chunk agent is structurally blind to, and the gate could not see it: it only asked that question of agents whose prompt said `chunk N of M`. `agent-prompt --whole-diff` now builds their diff-reading block too. 3. The prompt the CLI built was rewritten on the way to the agent. The 3B run invoked agent-prompt correctly for all five chunks and then paraphrased what it printed: the delivered prompt dropped the rule against reciting a stock sentence, dropped the half-read warning, replaced the project's review rules with a three-sentence summary of its own, and invented an instruction that was never in the original. Nothing could see it, because a paraphrase keeps the diff path. So agent-prompt records what it emitted, at a path derived from the plan that the caller is never given; check-coverage reads it back against the launch prompt the harness recorded. Replayed against that run's real transcripts, all five chunk agents are now named. Verified end to end: a fresh /review of QwenLM#6829 (3A) calls `agent-prompt --whole-diff`, passes it verbatim, and Step 3D reports 2/2 chunks reviewed, 12 agents, 12 did work — a gate that path could not reach before, and could not have passed if it had. * fix(review): build every agent's prompt, from a roster the plan derives Two more failures, both measured against the harness's own transcripts of real /review runs. 1. Agent 0 was never launched, and nothing could tell. The skill says issue fidelity runs on every PR review. Dogfooded on QwenLM#6766, it did not run — and every check passed, because every check asks a question of an agent that RAN. An agent that does not run leaves no transcript to ask. An omission is invisible precisely because it is an omission. So `check-coverage` now derives a roster from the plan — which the caller does not write — and names every required agent that never ran, with the exact `agent-prompt` call that builds it. The plan already knew everything the roster turns on: the topology, whether the diff deletes anything, which files were rewritten heavily enough to need invariant agents, whether there is a worktree to build in and a pull request to check an issue against. `agent-prompt --role <role>` builds all of them: 0, 1a, 1b, 1c, 2, 3, 4, 5, 6a/6b/6c, 7, the test matrix, and the three invariant agents per heavy file. The briefs move out of SKILL.md and into code, because a brief the orchestrator retypes is a brief that drifts. 2. A 4 652-character prompt is not a thing an orchestrator will paste twelve times. With the briefs welded into the launch prompt, the first dogfood delivered **2 893** characters of one: it kept the head, added a preamble of its own, and cut nineteen hundred characters out of the middle. The delivery check caught it — and the run then read the check's exit-3, concluded "the agents clearly did their job", skipped `compose-review`, and filed an **Approve it had written itself**. A gate that always fails is a gate that gets talked around. So the brief goes where the diff already goes: on disk, read by the agent that needs it. The launch prompt drops to ~500-800 characters — it names the role, points at the brief file, and lists the diff reads — and whether the agent actually read its brief stops being a hope and becomes a tool call the harness wrote down (`unreadBriefs`). Verified end to end. A fresh /review of QwenLM#6847 built all twelve role prompts, and against the harness's transcripts: 12 of 12 delivered **byte-for-byte verbatim**, 12 of 12 **opened their brief**, 3-19 successful tool calls each — Agent 0 among them. Step 3D: `1/1 chunks reviewed, 12/12 agents did work`, no errors. * fix(review): the verdict is computed, not carried `compose-review` has computed the event and the body since the C/S table stopped being prose. The skill then told the orchestrator to "copy event/body verbatim into the review JSON" — a transcription, into a document the model writes, of a decision the CLI had already made. That is the exact anti-pattern `submit`'s own header repudiates, and it left two ways for a run to author its own verdict: - **The terminal.** Step 6's verdict was composed by the model, from prose rules. Dogfooded, a run read the coverage check's refusal, concluded that "the agents clearly did their job", never called `compose-review` at all, and printed `Review complete — Approve` on a review whose gate had just refused. - **The wire.** `submit` took `{event, body}` as fields. Nothing stopped a run that had skipped the computation from posting the conclusion it preferred. So `submit` composes. It takes the findings — the inline comments and the states Step 6 established — and derives everything that follows, including how many blockers there are: `criticalsInline` and `suggestionsInline` are counted off the `**[Critical]**` / `**[Suggestion]**` prefixes of the comments actually attached, not accepted as numbers beside them. (A number beside a list is a number that can disagree with the list, and one did: the breaching run posted a body reading "Suggestions are inline" next to an empty `comments` array and a summary claiming `0 Suggestion inline`.) A payload carrying `event`/`body` is refused rather than silently overruled — the caller was trying to author a verdict. Two body checks are deleted, not weakened: a body that promises inline comments it does not carry, and a body whose footer is preceded by a literal `\n`. Both were checks on a string the caller built. The caller no longer builds it. `compose-review` now prints the verdict line itself, and Step 6 prints that. There is one place a verdict exists; skipping the command does not get you a different one, it gets you none. Verified: a payload with `event: APPROVE` and an unreviewed dimension is refused at the wire; the same findings without a verdict compose to `COMMENT`. A fresh /review of QwenLM#6788 called `compose-review`, was told `Verdict: Comment`, and showed the user "Comment — downgraded from Approve (CI failing: route)" — the presubmit downgrade applied by code, on a run that did not post. * fix(review): make Step 3B carryable, and stop the delivery check crying wolf Dogfooding the Step 3B path — the one topology none of this had been run against — found two defects, and the second is the more important of the two. 1. Eighty-seven kilobytes of chunk prompts, in one response. The briefs moved onto disk for the dimension agents and not for the territory agents. Measured on PR QwenLM#6606 (5 511 diff lines, 17 chunks): 17 chunk launch prompts of ~5 149 characters each — **87 546 characters** the orchestrator was expected to paste unedited. At a twelfth of that load it had already cut nineteen hundred characters out of a single prompt. Chunk agents get the same split: the brief on disk, and a launch prompt that carries only what cannot live anywhere else — `chunk N of M`, which attributes the territory, and the `offset`/`limit`, which are the lines coverage proves were delivered. 87 546 → 14 789 characters. And `check-coverage` now asks the territory agents the same question it asks the others: did you open your brief? 2. The delivery check failed a correct run — all nine agents of it. It was a substring test: the built prompt had to appear in the launch prompt, contiguously. That is a stricter claim than the skill makes, and both of the differences it fired on were legitimate. The orchestrator had inserted **the one-sentence summary of the change that the skill explicitly tells it to add**, which breaks contiguity by construction — and it had reflowed a hard-wrapped sentence onto one line, which changes not one character of meaning. This is the failure this skill keeps re-learning, and this time it was ours: a gate that fires on a correct run is a gate that gets talked around, and there is a dogfood transcript of a model doing exactly that. The rule the check enforces is now the rule the skill states — **you may add; you may not remove, alter, or reorder** — over whitespace-collapsed lines, in order. Verified against the harness's transcripts of a real Step 3B review of QwenLM#6766: nine agents (five chunks, issue fidelity, cross-file tracer, test matrix, build & test), 9/9 delivered intact, 9/9 opened their brief, 6-22 successful tool calls each. `check-coverage`: 5/5 chunks, every list empty, exit 0. * feat(review): path-scoped rules; and move the briefs out of the skill, where they were never reaching the agents Two changes, and the second found a hole the first would not have. 1. Rules that attach to a path, not to a dimension. The nine dimensions are domain-blind by design — "find security bugs" is a lens, not a syllabus — and that holds until a file's failure modes are not guessable from reading it. A GitHub Actions workflow is the clearest case: it is YAML, so it reads as configuration, and the reviewer who treats it as configuration misses every one of its attack classes. Nothing in this review knew to ask whether a `pull_request_target` job checks out the contributor's head — which is the difference between a CI file and a remote code execution with the repository's write token. This repo runs `qwen-autofix.yml`, which posts to pull requests. `agent-prompt` now appends a checklist for such a file to the brief of every code-reviewing agent **whose territory actually contains one**. Scoped, because a rule that fires on every review is a rule that gets skimmed. `/review` runs on other people's repositories, so the calibration matters as much as the content: the blockers are the six that are unambiguously wrong; the two that shade into taste (SHA-pinning, `permissions:`) are Suggestions, exempt the conventions almost everyone keeps, and are scoped to lines the diff touches. No style rules — a linter owns those, and the Exclusion Criteria already forbid them. 2. The briefs move out of SKILL.md — and three things turned out never to have reached an agent at all. The briefs have been built in code since the roster landed, and SKILL.md still carried 38 KB of the same prose. Duplication is drift, and a 178 KB skill is ~45 000 tokens in the orchestrator's context on every review — which is itself a cause of the failure this whole line of work has been chasing. The skill now keeps what each agent is *for* (a table) and drops what it is *sent* (the command's copy is the one that arrives). Doing that surfaced what the code briefs were missing, because the deleted prose had to go somewhere: - **The Exclusion Criteria had never reached an agent.** The skill states them at the end of the document and tells the orchestrator to "apply" them. The agents do not read the document. The single largest precision control in this review has been governing nobody, in every run, since it was written. - **Nor had the anchor rules.** Agents were asked for a snippet and never told what makes one resolvable: prefer added lines, a removed line cannot be anchored at all, a bare `}` matches everywhere. `resolve-anchors` was downstream of a snippet nobody had given the rules to produce. - **Nor the severity calibration.** `SEVERITY`'s own comment warns that a chunk agent owns test coverage with nothing to calibrate it and will file "zero test coverage" as Critical — and then did not include the calibration. All three are in the briefs now. And two degradations the orchestrator used to be told to add by hand — and can no longer add, because it does not write these prompts — are applied by the builder: in cross-repo lightweight mode there is no tree, so 1b and 1c report at `Confidence: low` rather than asserting a re-establishment is missing. A false Critical blocks a merge. Step 3C (the medium-effort inline pass) now *loads* the briefs it needs rather than carrying them: same text as the high-effort agents get, read when that level actually runs instead of sitting in every review's context. SKILL.md: 171 178 → 153 656 bytes. * fix(review): scope an invariant agent to its own file, and let a blocker's blast radius be part of the blocker Two corrections, both from dogfooding the paths that had never been run. 1. An invariant agent was being handed the whole chunk plan. It owns one heavily-rewritten file. Its brief says so, and gives it that file's own slice of the diff. Its *launch prompt* listed every chunk in the review — on PR QwenLM#6457, all twenty-one reads of a 6 149-line diff, for an agent whose job is one file. The wasted reading is the smaller half. Coverage is computed from the ranges in the launch prompt, so an invariant agent was being credited with having read **every chunk in the review**. One of them could have masked twenty missing chunk agents. It now gets exactly its file's `diffRange`, and nothing else. 2. `permissions: write-all` on a job that runs untrusted code is not a Suggestion. The path rule said it was. Dogfooded against a planted vulnerability, the security agent read that and escalated anyway: "grants maximum token scope to a job that processes untrusted contributor code, amplifying the RCE above". It was right and the flat rule was too coarse. A broad token on a privileged job is not a separate recommendation — it is how far the blocker reaches, and it belongs in that finding, at Critical. On an ordinary job it stays a Suggestion. * fix(review): the six findings this skill filed against its own pull request The repository's own `/review` bot reviewed QwenLM#6892 — this change reviewing the code that changes it — and filed six Suggestions. Every one of them is real, and two are fail-open holes in the gates this pull request exists to build. They are fixed here, each with a test. - **`submit` accepted `state: null`.** `=== undefined` is not `== null`, so the structural check passed it; `compose`'s `?? {}` then collapsed it to an empty state and would have posted a review whose footer named no model and whose caps came from nowhere. - **`wasDeliveredVerbatim` was vacuously true for an empty `built` prompt.** A zero-byte record is what a partial write leaves behind — and `recordPrompt` swallows its write errors by design, so this is reachable. `readRecordedPrompts` stores it as `''`, not `undefined`, so the "no prompt was built" guard did not catch it, and the loop's body never executed. The roster would have credited a required role to whichever transcript it looked at first. It now fails closed. - **A chunk read across two pages got no credit.** The check asked for a *single* range containing the chunk, and reads of 1-200 and 201-400 are two — so it contradicted the paging instruction the same review had just given, on exactly the oversized chunks where paging is not optional. Ranges are coalesced first. - **Agent 7 was handed relative paths it could not resolve.** `worktreePath` and the plan path are repo-relative in the report, and Agent 7's working directory *is* the worktree — so `--worktree .qwen/tmp/review-pr-6457` resolved to `<worktree>/.qwen/tmp/review-pr-6457`, which does not exist. This was already visible and nobody had read it: in the 29-agent dogfood run, Agent 7 spent its time running `find … -name "*6457*fetch*"`, hunting for a plan it had been handed a path to. Absolute now. - **`removePromptRecord` was dead code with a comment claiming a caller it did not have.** `cleanup.ts` sweeps the prompt directory by prefix instead. Deleted. - **`--dry-run` omitted `cappedBy`.** The point of a dry run is to see what would be posted; `"event": "COMMENT"` with no reason leaves the reader to guess why the Approve went away. * docs(review): purge the stale event/body payload examples from Step 7 The reviewer caught a real contradiction it filed as Critical: submit.ts now refuses a payload carrying `event`/`body` (those are computed from `state` and the attached comments), but Step 7's main-path 'Build the review JSON' examples still showed `"event": "REQUEST_CHANGES"` / `"body"` and routed the verdict through a copy-it-verbatim step. An orchestrator following the unchanged instructions would have built exactly the payload submit rejects. The correct `{commit_id, comments, state}` shape existed lower in the section (the no-findings branch), added when submit took over composition — but the main-path examples and the compose-review-then-transcribe bullets above them were never reconciled. They are now: one payload shape, no verdict in it, `state` handed to submit, and the inline counts derived from the comments rather than supplied. Found by the repository's own /review on QwenLM#6892. * fix(review): the three findings from the third self-review The repository's /review passed QwenLM#6892 (no blockers) and filed three Suggestions. All three are real; two are contradictions this PR itself introduced. - **verdictLine printed a dangling colon.** When a would-be Approve was taken away by a presubmit downgrade ALONE — no cap state, `cappedBy` empty, `downgraded` true — the code joined the empty array and produced 'an Approve was NOT available: — downgraded by a presubmit check', a colon over nothing. It now collects the reasons (a cap and a downgrade are both reasons, either can be the only one) and prints the clause only when there is a reason to. The function had no test; it has six now, including this case. - **Step 3D said 'six failures' and listed seven, while check-coverage reports eight.** The count drifted as failure classes were added, and the uncoverable-chunk class had no bullet at all. Now 'eight', with the missing bullet written. - **`submit --review` help still advertised `event` / `body`** as payload fields, which the same command now refuses. Updated to `commit_id / comments / state`. Found by the repository's own /review on QwenLM#6892 — the third pass, the one that turned CHANGES_REQUESTED into no-blockers. * fix(review): a heavy file in a Step-3A diff must not demand invariant agents From a human review of QwenLM#6892 (doudouOUC). `heavy` is decided independently of topology (lib/heavy.ts): a ~300-line source file with ~120 changed lines clears the rewrite-ratio branch while srcDiffLines stays under 500 — a Step 3A review. The invariant-agent loop in requiredAgents ran in both topologies, so it added invariant-a/b/c to the roster of a 3A review that never launches them; check-coverage then reported them as missingRoles and exit-3'd, and compose-review capped the verdict — an otherwise-complete small PR, falsely blocked. Gate the loop on isTerritoryFanOut. Step 3A's dimension agents each walk the whole diff, so one already sees both ends of a rewritten file; invariant agents are a 3B mechanism for when the diff is carved into territories and no single agent holds the whole file. roster.test.ts now pins the 3A-heavy case. Also, same review: merge() in coverage.ts copied its first tuple and pushes copies, so it no longer mutates a tuple owned by rec.diffReads (harmless today, pure now). * fix(review): a downgraded Request changes must not read as a plain Comment Fifth self-review, one behavioural finding among five (the rest are test/doc). verdictLine printed 'Comment — downgraded by a presubmit check' for BOTH a Suggestion-only Comment the presubmit moved and a REQUEST_CHANGES it moved down to Comment. The second is a review with confirmed Criticals posted inline, and 'Comment — downgraded' reads to an operator as 'nothing blocking'. It could not tell them apart from baseEvent alone — a cap may already have softened the RC before the downgrade ran — so ComposeReviewResult now carries downgradedFrom, and verdictLine says 'Request changes, downgraded to Comment … (the blockers are still posted)' for that case. Six verdictLine cases now, including this one. Also from the same review, all confirmed: - agent-prompt.test.ts: the describe block named a function that was renamed (buildRolePrompt -> buildRoleBrief), and the mode-rejection it.each covered 2 of the invalid combinations, not the role-mode ones; now covers all five and drops the stale 'two modes' wording. - SKILL.md Step 7: the review-JSON example used a /* */ comment inside a ```json fence (not valid JSON); switched to ```jsonc with a // comment. * fix(review): eight review-round fixes atop the Step-3A invariant resolution Follows 7c499d1, which resolved the doudouOUC roster finding (a heavy file in a Step-3A diff must not demand invariant agents — gate the loop on the topology) and the merge() purity nit. This carries the rest of the same round: - `roster.ts` requires Agent 0 only for a positive PR number. `!== undefined` let `null`/`0`/`''` through. Note the reviewer's suggested `typeof === 'number'` is wrong for this codebase — `fetch-pr` writes the number as a *string* — so the guard accepts a numeric string too, or every real PR review would lose Agent 0. A table test pins both directions. - `transcripts.ts` matches the diff path as a whole JSON string value, so `…/diff.txt.bak` no longer counts as reading `…/diff.txt`. It also documents why FIFO is right for a chronological transcript. - `agent-prompt.ts` scopes path rules to `--file` only for invariant roles — a whole-diff reviewsCode agent passed `--file` would otherwise lose the rules for every other file — and guards each chunk element in `diffReadingBlock` like `chunkFrom`, so a corrupted chunk errors legibly instead of emitting `offset=NaN`. - `compose-review.ts` stops double-wrapping `cov.missingRoles` / `cov.rewrittenPrompts`, which coverage.ts already writes self-explanatory. - `agent-briefs.ts` JSDoc said "Two do not" read the diff; only Build & Test does not. - The agent-prompt size-bound test now covers `test-matrix`. The empty-prompt guard, the paged-read coverage, the verdictLine dangling-colon and the submit help text were all already handled by earlier commits on the branch; those threads are answered without a code change.
|
The scheduled runs of this patrol are all ending up The cause is not concurrency (the group is In the logs, the Suggested fix:
|
6 participants
What this PR does
Adds a serialized patrol that runs every ten minutes and scans open, non-draft PRs targeting
main. It asks GitHub for currently failing PRs, keeps only currentQwen Code CIfailures that are at least 30 minutes old and no more than seven days old, orders them newest first, and processes a configurable batch of up to five PRs per run.A credential-free skill classifies each sanitized failure as one of three actions: rerun a concrete transient failure, publish a concise English failure explanation with a folded Chinese translation, or take no visible action when evidence is ambiguous. Main-branch failures remain out of scope for this PR.
The trusted driver revalidates the open PR, head SHA, exact workflow/check, run ID, run attempt, current job metadata, and action count immediately before writing, and requires each classifier decision to match the scanned failure fingerprint. Hidden bot comments store per-head state. The same run attempt is handled once, recorded automatic actions stop after three consecutive attempts on a PR head, a successful handled run records a zero-count reset, and a new push starts fresh state. The rerun API call happens before its state marker so a failed mutation cannot create phantom handled state.
The classifier receives no GitHub credentials and only has sandboxed file read/write tools. Scheduled reads use the workflow token, while comments, reruns, and reset markers use
CI_BOT_PAT. Logs and model input are bounded, common credentials/private keys/JWTs are redacted, visible explanations neutralize mentions and HTML, and malformed or stale decisions cause no write.Why it's needed
Maintainers currently inspect and recover stale PR CI failures manually. This patrol automates the safe cases without acting on healthy PRs, repeatedly retriggering CI, or acting on stale model output.
Reviewer Test Plan
How to verify
mainPRs/fresh failures/superseded checks, and processes no more than the configured batch size.<details>, while ambiguous evidence only receives hidden state.Evidence (Before & After)
A local read-only scan against the live repository returned the five newest eligible failed PRs. A final one-candidate scan selected PR #6723 and the exact failed job, and retained the actual
toolFormattingassertion about the missingdeferred_tool_callChinese translation instead of cleanup output or expected errors from passing tests. A fault-injection scan now rejects an attempt-1 job/log when the live run is already on attempt 2. No GitHub mutation was executed during local scan verification.Tested on
Focused Vitest coverage (29 tests), ESLint, Prettier, actionlint,
git diff --check, the full repository build, and the full repository typecheck pass locally.Risk & Scope
CI_BOT_PAT, the existing model API secrets, and the review model variable on the upstream repository before scheduled writes can work.CI_BOT_PAT; a dedicated identity or signed marker protocol remains a maintainer-gated security decision.Linked Issues
N/A