mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-06 07:10:14 +00:00
424 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Shaojin Wen
|
4e87922504
|
fix(autofix): refuse a non-main takeover out loud instead of only in the job log (#7382)
Applying autofix/takeover to a PR that targets another branch left no visible trace: the label stuck, the pull_request:labeled route run went green, and the only record was one line in a job log. A stacked PR then looked exactly like a managed one while nothing was managing it. The route now emits a 'base-refused' ack, and the ack job posts a bilingual explanation naming the base it was refused against. The refusal deliberately reads no live PR state, so the one ack whose whole purpose is to explain silence cannot itself be silenced by an unrelated API failure. Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
8065355ac3
|
feat(autofix): render the managed fleet into the scan's run summary (#7355)
* feat(autofix): render the managed fleet into the scan's run summary Seeing whether the loop was healthy meant reconstructing it by hand: list the bot's PRs, fetch each one's comments, regex the autofix-eval markers for round and watermark, then cross-check gh pr checks and the fork/takeover state. That is how today's triage of #7246, #7259, #7329, #7333 and #7336 was done, and it is why a stalled PR stayed invisible until somebody went looking for it. The scan already computes every one of those facts while deciding what to process — it just wrote them to a job log nobody reads. Each per-PR terminal decision now also records a row, and the step renders one markdown table into the run summary: | PR | State | Detail | | #7329 | SELECTED | 1 review + 5 inline new (round 0/5) | | #7333 | idle | nothing new since 2026-07-20T13:54:18Z | | #7262 | waiting | active checks in flight | | #7208 | round-capped | round 100/100 - needs a human or @qwen-code /retry | States cover every branch that ends a PR's inspection: busy, skipped, unknown, waiting, round-capped, idle and SELECTED — so a PR cannot drop out of the table by returning early, which is exactly the invisibility this fixes. No new API calls (the data is already in hand), no writes outside the run summary, and the helper is defined at the top of the step so it stays clear of the BUSY_PRS/INSPECTED proximity guard that keeps the free busy-skip from consuming the inspection budget. Tests: the real helper and render block are replayed over fixtures (table structure, one row per state, and an empty fleet still rendering a table), plus each decision branch is pinned to its fleet_row. Mutation-verified: dropping one branch's row turns it red. * fix(autofix): use temp file for fleet test replay; cover fork-head skip (#7355) * test(autofix): assert each skipped fleet_row call site individually (#7355) * fix(autofix): record fleet rows for both budget-break paths (#7355) The candidate-inspection budget break incremented INSPECTED but never called fleet_row, so the PR that tripped the budget was silently absent from the fleet table. The target-budget break left all remaining candidates invisible with no truncation signal. Add a per-PR deferred row before the inspection-budget break and a summary deferred row before the target-budget break so the fleet table stays complete in both cases. * fix(autofix): harden fleet summary render and clean up temp file (#7355) Address review feedback: - Escape '|' in detail values to prevent broken table columns - Render budget summary row (PR '-') as em dash instead of '#-' - Add trap for FLEET_FILE cleanup on early exit paths - Document deferred summary row semantics in test comment * fix(autofix): use summary row for candidate-inspection budget break (#7355) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
96a39f4cb9
|
fix(autofix): retry a verification-gate crash instead of burying the agent's fix (#7351)
* fix(autofix): retry a verification-gate crash instead of burying the agent's fix A gate failure had two very different meanings collapsed into one outcome. When the gate DECLARES a verdict (outcome=failed) it evaluated the agent's attempt and rejected it, so advancing the watermark is right — the same feedback would reproduce the same rejection, and MAX_ROUNDS bounds it. But when the gate dies WITHOUT a verdict it never judged the work at all, and advancing buries a fix the agent had already written: the next scan sees "nothing new" and the PR sits until a human deletes the marker by hand. That is exactly how the nested-package ENOENT stranded #7329 and #7336. Both agents had implemented the review feedback — the handoff even quoted the implemented changes — but the gate crashed on its own bug while resolving packages/channels/*, the commit was discarded, and the PRs read as "Could not address the latest feedback automatically". Two halves: - The review-address gate now declares every rejection it can legitimately reach: build, typecheck, lint and the per-package tests each call a `reject_fix` helper that writes outcome=failed before exiting. (The resolver call is deliberately left undeclared — a resolver error IS a gate bug.) - The handoff treats an EMPTY outcome on a non-success job as the gate's own crash and routes it to the existing sentinel/retry path, so the feedback stays live and the next scan retries. The round still increments, so a persistently crashing gate is bounded exactly as before, and the headline names the real cause ("hit a verification-gate error before reaching a verdict") and, on the final attempt, points at the gate logs. Unchanged: a declared rejection still advances and reads as before, a no-output crash keeps its own wording and retry, and a crash before the feedback was read stays terminal. Tests: the real extracted decision block is replayed under bash across declared rejection (advances to NEWEST), gate crash (sentinel + retry + round+1), no output (sentinel, original wording), the round cap (operator fix), and a successful job (never a crash); plus the reject_fix helper is driven for real to prove a rejection writes outcome=failed. Both mutation-verified — dropping the crash arm, or unwiring one known rejection, turns them red. * fix(autofix): clarify retry-branch comments per review nits (#7351) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
c42b940685
|
feat(autofix): resolve the review threads whose findings it implemented (#7364)
* feat(autofix): resolve the review threads whose findings it implemented A human re-reviewing a managed PR currently has to re-read every thread to work out what the bot already handled. #7308 shows the cost: 17 review threads, 13 still open, with no way to tell which of those were fixed and which were declined. The agent already decides per finding — it records each one in address-summary.md as implemented or declined with a reason. What was missing was a way to act on that: the agent's sandbox carries no GitHub token, so it cannot resolve anything itself, and feedback.md gave it no stable handle to point at even if it could. Three small pieces close that: - feedback.md now renders each inline comment with its id (`- [rc:<id>] …`), giving the agent a handle it can echo back. - The SKILL asks the agent to write resolved-comments.txt: one id per line, for findings it IMPLEMENTED only. A declined or deferred finding must stay unresolved so its recorded reason actually gets read. - After a successful push, the step that already holds the PAT maps each id to its review thread and resolves it. Deliberately narrow: only threads the agent claims it implemented, only ones not already resolved, and entirely best-effort — a resolve failure warns and never fails a good push. Tests: the real extracted block is driven with a stubbed gh over fixture threads — an implemented finding's open thread is resolved, a DECLINED finding's thread is left open, an already-resolved thread is skipped, and an unknown id matches nothing. Mutation-verified: dropping the isResolved guard turns it red. * fix(autofix): harden review-thread resolution per review feedback - Guard --jq against null pullRequest (// {nodes:[]}) so a transient API inconsistency cannot crash the step after a successful push - Add pageInfo{hasNextPage} and emit :⚠️: when threads exceed the first-100 page cap - Tolerate rc: prefix and trailing CR in resolved-comments.txt - Emit :⚠️: when a valid numeric id matches no open thread - Match production set -euo pipefail flags in the extracted-block test --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: Qwen Code <qwen-code@users.noreply.github.com> |
||
|
Shaojin Wen
|
91d60a9812
|
fix(ci): stop a slow patrol classifier from killing every flaky rerun (#7358)
* fix(ci): stop a slow patrol classifier from killing every flaky rerun The CI Failure Patrol has been effectively offline. Across the last 30 scheduled runs, 28 were cancelled and only 2 succeeded — and both survivors ran at 00:0x, in ~2 minutes, when the model was idle. Step timings show why. Setup, checkout, node and the scan finish in 28 seconds; the model step then runs 9m39s and is killed by the job's 10-minute timeout. Because the job dies there, Validate and Upload never run, the `act` job is skipped on `classify.result != 'success'`, and nothing is ever re-run. One slow step was taking down the whole patrol, every cycle, for hours. That is why #7333 still carries a red `web-shell E2E Smoke` whose failure is `No space left on device` — a textbook infra flake, inside the patrol's own TARGET_WORKFLOW, that the patrol never got far enough to re-run. - The classifier step is now bounded at 5 minutes, below the job's 10, and marked continue-on-error: a slow model costs one patrol cycle instead of the patrol, and the next tick simply tries again. - An empty classifier result is reported (has_decisions=false) rather than failing the job, so the run finishes cleanly instead of looking like a broken patrol. - Upload and the `act` job are gated on decisions actually EXISTING, not merely on the classify job having survived — otherwise a no-decision cycle would look actionable. Tests: the step's timeout is asserted to be strictly below the job's, plus the continue-on-error and the has_decisions gating on Upload and `act`; and the Validate step's bash is replayed for real with and without a decisions file, asserting exit 0 and the right flag in both. Mutation-verified — removing the step bound, or the decisions step id, turns it red. * fix(ci): validate JSON syntax in patrol decisions before acting (#7358) --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
0717a2a0b6
|
feat(autofix): pick up managed fork PRs in real time instead of waiting for the throttled schedule (#7350)
* feat(autofix): pick up managed fork PRs in real time instead of waiting for the throttled schedule The `pull_request_review` trigger already routes feedback straight to the PR it arrived on, but it admitted ONLY in-repo bot PRs — every fork under takeover had to wait for the scheduled scan. That schedule is far slower than it looks: the cron says `*/10`, but GitHub throttles scheduled events on this repo hard enough that the observed interval is 40-70 minutes (and in the same window `pull_request_review` fired 17 times while `schedule` fired once). So the takeover PRs a maintainer is actively iterating on were the ones waiting longest for their feedback to be picked up. Real-time pickup now applies the SAME admission the scheduled scan uses for a fork: allow-edits on, and either the bot's own fork or an explicit autofix/takeover label. Nothing about *what* may run changes — this event runs in base-repo context, and review-address independently re-verifies allow-edits, a live write+ author and a matching live head repo before it touches the branch. Only *when* the same gated work happens changes. Unchanged: non-main targets, untrusted senders, human in-repo PRs and forks that are neither the bot's own nor takeover-labelled are all still ignored, and only `pull_request_review:submitted` triggers (not per-comment events). Tests: a behavioural replay drives the extracted route block with a stubbed gh across eight cases — in-repo bot admitted, in-repo human rejected, bot fork and takeover-labelled fork admitted and routed to that PR, and no-allow-edits, unlabelled fork, non-main base and untrusted sender all rejected. Mutation- verified: restoring the blanket fork rejection turns it red. * fix(autofix): admit real-time fork PRs in review-scan's forced predicate (#7350) The route step now admits managed fork PRs for real-time review pickup, but review-scan's forced-PR predicate still required `.isCrossRepository == false`, so every fork was rejected there: targets=[] / has_targets=false and review-address never started — the feature was silently discarded for the very PRs it added. Admit forced fork PRs under the scheduled scan's OWN fork rules (allow-edits on, plus a live write+ author check mirroring the scan's per-candidate gate); in-repo PRs keep the fail-closed `.isCrossRepository == false` test. review-address still re-verifies allow-edits, a live write+ author and a matching head repo before pushing. Also exercise the route step's metadata-read-failure branch (fails closed) in the workflow tests. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
2493248858
|
fix(review): make agent launches and cleanup resilient (#7259)
* fix(review): make agent launches and cleanup resilient Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): address worktree cleanup feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): harden cleanup follow-ups Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): sync autofix workflow assertions Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): close cancellation cleanup gaps Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): cover remaining review-cleanup feedback suggestions (#7259) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
eb53ebed5b
|
feat(autofix): re-arm a stranded PR with @qwen-code /retry instead of deleting a marker (#7354)
* feat(autofix): re-arm a stranded PR with @qwen-code /retry instead of deleting a marker Recovering a stranded managed PR meant running `gh api -X DELETE` against the bot's own autofix-eval marker comment. That needed raw API access and the comment id, erased the audit trail, and was undiscoverable unless you had read the workflow — it came up twice while triaging #7246, #7329 and #7336. `@qwen-code /retry` now posts a single `<!-- autofix-rearm -->` marker, which does both halves of what the deletion did: - The scan's watermark ignores eval markers written BEFORE the newest re-arm, so the feedback those markers buried is read again. The watermark stays global otherwise — this is an explicit, maintainer-issued exception, which is exactly what the deletion was, only recorded instead of destructive. - The marker also opens a fresh counting window (it joins the engage ack in REARM_KEY), so the round counter resets and a terminal round stops skipping the PR. That also means the existing "a re-arm supersedes queued old-window jobs" guard covers /retry for free. The address job's live recheck mirrors both, so a run selected before a re-arm still discards itself instead of stamping an old-sequence marker. Authorization is the takeover command's, unchanged and reused rather than reinvented: exact body match, live permission lookup, in-repo-only author privilege. The route prefilter now admits the second command. The job verifies CI_DEV_BOT_PAT authenticates as the bot before commenting, because both scanners only count markers authored by it. The marker is registered as a control comment so the agent never sees the re-arm as feedback to address. Tests: the real extracted scan block is replayed over synthetic comment fixtures — stranded (watermark held, round 2), after /retry (watermark released, window reset, round 0), a marker written after the re-arm counting again, and a re-arm from a non-bot author correctly ignored. Both halves mutation-verified. * test(autofix): add behavioral test for address-side re-arm stale check (#7354) * fix(autofix): generalize remaining command-ignored messages and assert all filter sites (#7354) * test(autofix): add behavioral test for the retry-command re-arm marker job (#7354) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
eca654f365
|
fix(autofix): resolve owning package for nested paths; report verify-failed handoffs as not pushed (#7330)
* fix(autofix): resolve owning package for nested paths; report verify-failed handoffs as not pushed The verify gate mapped each changed file to a flat `packages/<dir>` and read `<dir>/package.json`, which ENOENT-crashed on nested packages such as packages/channels/base — the container packages/channels has no package.json. Walk each changed file up to its nearest package.json in both the issue-fix and review-address verify steps, and skip any candidate that still has none. When such a verify failure follows an agent commit, the review-address handoff rendered the agent's optimistic address-summary.md (which can cite a commit SHA) under a neutral "what I found" heading, so a maintainer chased a commit that was discarded with the runner workspace. An EXIT trap now records any post-commit non-zero exit as outcome=failed, and the handoff states plainly that the change did NOT pass the gate and was NOT pushed. Tests: walk-up detection over a nested package tree, the outcome=failed trap, and the not-pushed handoff wording — each mutation-verified. * refactor(autofix): extract owning-package resolver to a shared staged script Addresses review on #7330. Extract the changed-file → owning-package walk into .github/scripts/resolve-owning-packages.sh, staged to RUNNER_TEMP from the trusted base alongside check-settings-schema.sh and invoked from both verify gates, so the two gates cannot drift into resolving packages differently (the 8-line walk was otherwise duplicated verbatim in each). Updates the package-scripts test that pinned the old inline grep. Narrow the verify-failed handoff lead-in to "This change was NOT pushed": four paths set outcome=failed BEFORE the deterministic gate runs (agent abort via failure.md, dirty tree, unchanged branch, missing address-summary.md), so the previous "did NOT pass the verification gate" claim was factually wrong for them. The specific reason stays in the headline and the quoted summary. * style(autofix): brace variable references in resolve-owning-packages.sh The repo's shellcheck gate runs --enable=all --severity=style, under which bare $f/$d references trip SC2250 (prefer ${var}). Brace them to match the convention already used in check-settings-schema.sh, and update the script content assertions accordingly. Verified with shellcheck 0.11.0 using the exact CI flags: clean. * fix(autofix): resolve owning workspace via npm query; key unpushed-handoff on commit existence Addresses the deeper review on #7330. Blocking issue: the "nearest package.json" resolver mapped a change under a workspace's fixture/example package (e.g. packages/cli/src/commands/extensions/examples/starter) to that fixture, whose test script is not Vitest — silently SKIPPING packages/cli's own tests, a coverage regression invisible in the log. Resolve against the authoritative `npm query .workspace` set instead and take each file's longest-prefix workspace: nested workspaces (packages/channels/base) match exactly, fixtures and non-workspace paths (packages/sdk-python, packages/README.md, the excluded packages/desktop) drop. Also harden the resolver against a final line with no trailing newline and against an unmatched last line, which under `set -o pipefail` would otherwise abort the script. Handoff wording: keying "was NOT pushed / commit discarded" on outcome=failed was wrong for the abort paths (failure.md, dirty tree, unchanged branch, missing address-summary.md), which set outcome=failed before ever making a commit. Record committed=true right after checkout — before any gate can fail — and key the wording on that; the abort/no-op paths keep the neutral framing. This removes the EXIT trap entirely (its only observable effect was that wording), so it no longer mislabels pre-commit failures either. * fix(autofix): expand workspaces on-disk so branch-added packages are tested; harden resolver Addresses the re-review on #7330. The resolver sourced its workspace set from `npm query .workspace`, which reads node_modules — installed from the BASE checkout. A workspace the PR branch ADDS (a new channel adapter, a new sdk — the issue-fix job's whole purpose) was invisible, so its tests were silently skipped, and for a nested new package the ENOENT crash this PR fixes turned into a silent skip. Expand the set from the on-disk root package.json `workspaces` globs instead (shallow `dir/*` + literals, honouring `!` negations, keeping dirs with a package.json): it reflects the branch, matches what `npm run --workspace` accepts downstream, and needs no install. Verified to reproduce `npm query`'s set exactly on the current tree. Also from the review: - Fail the gate loudly on an empty/unreadable workspace set instead of the silent "no package changes" skip, and drop the now-unneeded `|| true` at both resolver call sites (the resolver already exits 0 on legitimate no-match). - Record committed=true at the TOP of the step (ref-only diff), covering an agent that commits then aborts, and count only `git diff --quiet` exit 1 as a commit (128 is a git error, not a discarded commit). - Correct the two call-site comments that still described the superseded nearest-package.json approach. Also hardens the resolver against a final changed-path with no trailing newline and an unmatched last line under `set -o pipefail`. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
3cd9561c8b
|
fix(ci): tighten API error detection to avoid false positive on review prose (#7328)
* fix(ci): tighten API error detection to avoid false positive on review prose The result-text classifier matched *"[API Error"* which hits review summaries that quote the pattern in prose (e.g. reviewing PR #7247 whose summary mentions "[API Error: ...]" and "quota … limit"). The quota grep then fired on the coincidental "quota … limit" substring, falsely reporting quota exhaustion on a successful review. Require a digit after "[API Error: " so only real API error messages (e.g. "[API Error: 429 …]") trigger the failure path. * fix(ci): anchor API error detection on tail position, not status-code shape The status-code glob from the previous commit would silently miss real aborts whose message has no leading digit (Connection error, Status: suffix, Chinese rate-limit text) — trading a loud false positive for a silent false negative. Anchor on position instead: an aborted run renders the API error as (or at the very end of) the result text, while a successful review that discusses API errors quotes them mid-prose. Checking only the last 600 bytes separates the two without constraining the error message format. Also adds fixtures for the non-digit-leading shapes and for prose quoting a real status code mid-body, and removes a stale comment. * fix(ci): catch aborts longer than the tail window; grep full text for quota Add a whole-result check (case "$RESULT_TEXT" in "[API Error: "*) alongside the tail check so errors whose body exceeds 600 bytes are still detected — the prefix falls outside the tail window but the result starts with it. Move the quota grep back onto the full RESULT_TEXT so a long error with quota wording early in the message is still classified as quota (not downgraded to retryable). * fix(ci): anchor abort detection on trailing shape, not byte window Replace the prefix+tail-window split with an ends-with check: the stream-json adapter appends the formatted API error last, so an aborted run's result ENDS with "[API Error: …]" optionally followed by a rate-limit guidance suffix. Strip the three known suffixes, rtrim, then match *"[API Error: "*"]". This catches the production abort shape (partial review + appended error) at any error length, without a byte-window constant to tune or leave untested. Prose that quotes the pattern mid-body keeps writing afterwards and does not end with "]". Adds fixtures for the production shape (appended error, long appended error, rate-limit suffix after the bracket). * test(ci): pin suffix sync with errorParsing.ts; document ]-ending trade-off Add a sync test that reads RATE_LIMIT_MESSAGE_BY_AUTH from errorParsing.ts and asserts all three suffixes appear in the workflow — prevents silent drift if someone rewords one. Add a KNOWN-limitation fixture documenting that prose ending with ] after quoting the pattern is a false positive (accepted trade-off; the durable fix is checking that the bot comment landed). |
||
易良
|
5e183dda18
|
test(autofix): sync workflow assertions with split model vars (#7297)
The autofix workflow now plumbs QWEN_AUTOFIX_MODEL (with a QWEN_PR_REVIEW_MODEL fallback) into the report steps, and the prepare step documents the verification gate's git diff --quiet check in a comment. Update the two stale assertions so they match the workflow again without dropping their original intent. |
||
|
Shaojin Wen
|
0f1700b8a4
|
feat(autofix): auto-manage the bot's own fork PRs without a label (#7243)
A fork PR the autofix bot itself opened (its codex flow pushes to qwen-code-dev-bot/qwen-code) is the bot's own generated work — same author, same code provenance, and the bot holds write+ — so it is trust-equal to an in-repo bot PR. Requiring a manual autofix/takeover label on it was redundant: in-repo bot PRs are auto-managed with no label, and the takeover label exists to authorize EXTERNAL (human) fork authors, not the bot's own. Now a fork authored by AUTOFIX_BOT with 'Allow edits from maintainers' is admitted and managed WITHOUT a label: - Scan: fork candidates are unioned from bot-prs.json (the bot's own forks — --author AUTOFIX_BOT, so no label needed) AND the takeover-labeled list (non-bot forks, explicit opt-in). Both still require allow-edits and pass the per-candidate live write+ gate. - Eligibility: the fork chain no longer demands the takeover label when the author is the bot (the author check already exempts it); it still demands allow-edits + a live write+ author + a matching live head repo. autofix/skip still opts any such PR out. Non-bot forks are unchanged — they still need the explicit label. Tests: the fork-candidate union admits a bot fork (no label) + a labeled human fork, dropping no-allow-edits/in-repo/skip; the eligibility replay makes a bot fork with allow-edits eligible without a label and discards it without allow-edits. 62/62 + 12/12. Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
076427650d
|
feat(ci): auto-open a deflake fix issue for confirmed flaky tests (#7231)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none (push) Waiting to run
E2E Tests / E2E Test - macOS (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
* feat(ci): auto-open a deflake fix issue for confirmed flaky tests
The CI Failure Patrol reruns flaky failures but never fixes them, so
the same tests flake forever on a rerun treadmill. This closes the
loop: when the patrol classifies a rerun as a nondeterministic TEST
(not infra), it now also opens ONE deflake issue that the existing
autofix issue pipeline develops into a reviewable stabilization PR.
- ci-flaky-patrol SKILL: a rerun decision whose cause is a specific
named flaky test carries an optional flakyTest {file, name}; infra
reruns (ENOSPC, network, runner death) never do.
- ci-flaky-rerun.mjs: validates flakyTest (malformed → the whole
decision is rejected, so a bad classification can't open a bogus
issue); after a rerun, ensureDeflakeIssue upserts a deflake issue
deduped by a stable (file, name) marker — one open issue per flaky
test across all PRs — labeled status/ready-for-agent + autofix/
approved so the scheduled autofix scan picks it up.
- .qwen/skills/deflake/SKILL.md: constrains the fix to four
assertion-preserving patterns (raise timeout/poll budget, stabilize
timing/waiting, make randomness/time deterministic, isolate
interference) and forbids skipping/deleting/loosening the check;
write failure.md if none applies or the failure looks like a real
bug. The produced PR is reviewable, never auto-merged.
Tests: deflakeKey stability/collision-freedom, the bilingual issue
body, one-issue-per-test dedup, no issue for infra reruns, and
malformed-flakyTest rejection. 34/34 across both patrol suites.
* fix(ci): deflake review hardening — rerun survives bad metadata, no markup injection
Addresses the two Criticals + suggestions on #7231:
- **Critical: a malformed/over-length flakyTest no longer kills the
rerun.** flakyTest validation is removed from validDecision (which
gated the PRIMARY action on secondary metadata — a >200-char nested
test name or a null silently dropped a valid rerun). Well-formedness
is now checked in ensureDeflakeIssue, which simply skips the deflake
issue when the metadata is bad; the rerun always stands.
- **Critical: markup/mention injection via the test path/name.** file
and name are code-span-stripped of backticks (which cannot be escaped
inside a span and would break out into live Markdown, turning
into a mention in a bot-created issue) and both now sit in
code spans. safeReason alone did not close this (it does not touch
backticks).
- Best-effort deflake: ensureDeflakeIssue is wrapped in try/catch so a
transient createIssue failure — after the marker is already posted —
no longer surfaces as a misleading "skipping PR" and permanently
suppresses the deflake; it retries on the next flaky occurrence.
- Run link uses the patrol's own repo (client.repo) instead of the dead
target.repo, so deflake issues on a fork don't 404.
- Body reworded: it no longer claims the rerun already passed (it runs
right after the rerun is triggered) — it says a real deterministic
failure is NOT flakiness and must not be stabilized.
- SKILL: bound file/name to 200 chars, and note a malformed one is
ignored (never drops the rerun).
Tests: malformed flakyTest keeps rerun (no createIssue); long title
truncates ≤240; backtick path/name cannot inject; run link honors the
repo; a throwing createIssue leaves the rerun intact. 38/38.
---------
Co-authored-by: wenshao <wenshao@example.com>
|
||
|
Shaojin Wen
|
a7bac6a433
|
test(autofix): exercise the SKILL stage↔resolve contract end-to-end (#7227)
* test(autofix): exercise the SKILL stage↔resolve contract end-to-end Follow-up to #7225, implementing the reviewer's non-blocking suggestions. The staging guard #7225 added pins the mirrored LAYOUT but re-implements run-agent.mjs's `<dir>/../SKILL.md` convention in the test. If that coupling ever moves in the RUNNER (e.g. ../../SKILL.md), the string test stays green while prod breaks again — the same class of blind spot that let #7165 ship. This adds the one check that exercises the contract for real: stage the actual runner into a mirrored tmp layout, run it with --print-prompt, and assert it reads the staged SKILL (sentinel body + resolved skill dir). The negative case — the flat layout #7165 shipped — is asserted to crash with ENOENT, proving the test catches that regression. Also replaces the brittle fixed-width `[\s\S]{0,200}` bound between `core.hooksPath .husky` and the runner invocation with a direct ordering assertion (indexOf), so adding a comment between the two lines can no longer fail the test spuriously. 61/61 + 12/12. * test(autofix): harden the stage↔resolve integration test per review Applies all four inline suggestions on #7227: - spawn process.execPath, not the bare 'node' string, so a version-manager shim or a PATH without node can't turn the test into an opaque 'null !== 0'. - nest the flat-layout runner under dir/flat/ so its ../SKILL.md resolves to dir/SKILL.md (never created) instead of a shared tmpdir()/SKILL.md that a concurrent job could leave behind and make the negative case pass spuriously — a real flake in the deflake-test itself. - reuse the existing withRunnerDir helper instead of duplicating its mkdtemp/try/finally/rmSync. - bound each spawnSync with timeout: 10_000 so a hung runner fails the test instead of the whole CI job (spawnSync blocks the event loop, so vitest's async timeout can't fire). 61/61 + 12/12. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
1e28b9a089
|
feat(review): retry transient API failures once; surface quota clearly (#7233)
* feat(review): retry transient API failures once; surface quota clearly The PR-review workflow failed permanently on any single API hiccup — a 502/503, a dropped connection, a rate limit — with only a fallback comment, so an idle PR sat without a review until someone re-ran `@qwen-code /review` by hand. Now the qwen invocation runs in a budget-guarded retry loop: - A transient outcome (non-quota API error, dropped/aborted run, empty output, error result) retries ONCE after a 60s backoff. - All attempts SHARE the review timeout budget, and a retry is capped at 5 minutes — a cleared transient succeeds fast, and a still-failing retry can't burn another hour (the observed quota run took 61 min, so an unbounded second attempt could blow the job timeout). - A quota-exhausted 429 is NOT retried in-run: its reset is typically hours out. It fails with kind=quota and the fallback comment now states the reset time and how to re-run once it resets — a clear recovery instead of a dead end. - A real timeout or a hard/config failure never retries (unchanged). Detection is exactly as before; only the disposition (retry / quota kind) is new. Behavioral test spawns the extracted loop under bash with a scripted stub qwen: success→1 try, transient→2 then success, persistent-transient→2 then fail, quota→1 try + quota kind + reset time, error-result→retry, hard-exit→no retry. 7/7. * fix(ci): guard the quota-detail grep, sync the timeout assertion, harden the retry tests Review follow-through on #7233's two Criticals and every suggestion: - The quota-detail grep ran unguarded in an assignment under `set -euo pipefail`: a 429 whose message lacks "reset at" exited the step before fail() wrote failure_kind, so the quota-aware fallback never fired. Guarded with `|| true`, and a quota_noreset scenario reproduces the exact message shape that died. - qwen-resolve-workflow.test.js still asserted the pre-refactor literal `fail "… ${QWEN_TIMEOUT} minutes." 1 "timeout"`; it now pins the OUTCOME/REASON pattern the loop actually uses. - The timeout REASON reported the total budget even when a 5-minute retry cap fired; it now names the attempt's own timeout beside the budget, so the fallback's --timeout advice matches what actually expired. - Quota matching tightened to quota-plus-context (exhaust/exceed/limit/ reset) so a transient "quota configuration" style error keeps its retry; the new test file uses the root `yaml` dependency instead of a hoisted js-yaml, carries the license header, anchors the loop extraction on the retry-budget comment instead of lastIndexOf, and gains the two uncovered scenarios: a real timeout is not retried, and an attempt with under 30s of budget never starts. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
58103b614a
|
fix(autofix): a no-output crash must not advance the review watermark (#7229)
* fix(autofix): a no-output crash must not advance the review watermark When a review-address run crashes AFTER prepare (so NEWEST is set) but BEFORE the agent writes any verdict — no address-summary.md, no-action.md, or failure.md — the handoff stamped the marker with ts=NEWEST, advancing the feedback watermark as if the feedback had been evaluated. It hadn't. The next scan then saw 'nothing new since <NEWEST>' and never retried, stranding the PR on a purely transient crash. That is exactly what happened to #7219 during the #7165 SKILL-staging outage: the run crashed at promptFor (ENOENT) at 09:50, the handoff advanced the watermark to 09:50:56, and even after #7225 fixed the crash the loop considered all prior feedback 'addressed' and would not re-engage. Fix: on a no-output crash (NEWEST set, DETAIL_FILE empty) stamp the sentinel ts instead — it is excluded from EVAL_WM, so the watermark does not move and the next scan retries the same feedback. The round still increments, so a PERSISTENT crash is bounded by MAX_ROUNDS and ends in a terminal handoff rather than looping forever. Agent-produced handoffs (verify failed after real output) keep advancing the watermark as before. Replay test extended to assert BOTH MARK_TS and MARK_ROUND across all three shapes: output+verify-fail → advance; no-output crash → sentinel (retry); pre-prepare crash → terminal. 60/60 + 12/12. * fix(autofix): correct the final-attempt crash headline per review Two review findings on the no-output-crash handoff: - The headline promised 'it will retry on the next scan' even on the final attempt, but at MARK_ROUND == MAX_ROUNDS the scan's round-cap gate skips the PR and the cap-reached notice is takeover-only — so a maintainer was told a retry was coming that never comes. The headline now branches: 'it will retry' only while MARK_ROUND < MAX_ROUNDS, otherwise 'this was the last automatic attempt; a human should take over'. - It embedded a Run log URL that the report block already appends to every handoff, duplicating it in the comment. Removed from the headline. Replay test extended: mid-attempt headline promises retry and carries no Run log; final-attempt headline says human-takeover and never 'retry'. 60/60 + 12/12. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
4998cb16d6
|
feat(autofix): surface the running model in every autofix report (#7226)
* feat(autofix): surface the running model in every autofix report Every visible autofix comment now carries a footer naming Qwen Code and the model it ran — for diagnosis (which model produced this) and as a small attribution for our own model. Four surfaces: the review-address fixed report, the no-action report, the handoff report, and the issue-phase PR's E2E comment. The model comes from the QWEN_PR_REVIEW_MODEL repo variable — already the agent's OPENAI_MODEL, a variable not a secret, so it is safe to echo into a public comment. Each reporting step plumbs it in and computes MODEL_DISPLAY with a 'default' fallback so an unset variable never renders a bare backtick pair. The footer sits with the report body (before the eval marker), and on the E2E path it is appended after the model's file, never injected mid-generation. Contract test pins the env plumbing and the footer on all four surfaces (twice in push-and-report, which carries both bodies), plus the append-after ordering on the E2E path. 61/61 + 12/12. * Update scripts/tests/qwen-autofix-workflow.test.js Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> |
||
|
Shaojin Wen
|
68c9032cd5
|
feat(autofix): direct takeover of maintainer-fork PRs (#7213)
* feat(autofix): direct takeover of maintainer-fork PRs Maintainer-approved v2: many maintainers work from personal forks, and adoption-snapshotting breaks their local workflow. A fork PR is now directly manageable when three live conditions hold — the takeover label, 'Allow edits from maintainers' (org-owned forks cannot enable it; adoption remains their path), and a fork author who holds write+ RIGHT NOW (the same live-privilege rule as the comment command, so an ex-member's fork can never summon secret-bearing runs). Plumbing: - Scan: fork takeover candidates are admitted per candidate (allow- edits + no-skip filtered in jq; the author's live write+ gate is one permission call each — a rare set); every matrix target now carries its head repo. - Address: prepare fetches the fork branch (origin has no copy) and checks out FETCH_HEAD with hooks already severed; the eligibility gate re-verifies takeover + allow-edits + author write+ live; the report step pushes back to the fork via the allow-edits grant. - Triggers: fork pull_request label events carry NO secrets, so the route notes them and the next scheduled scan engages (≤10m); the comment command now toggles fork PRs too (write+ senders only — fork authors stay silently dropped) and refuses only when allow-edits is missing, with the actionable ask. The scan posts a first-pickup engage ack (identity-verified, deduped on any existing ack, ic.json re-fetched so the same scan counts under the fresh window key) — closing the fork/manual-label ack gap and anchoring the round window. Behavioral coverage: fork-candidate admission jq (allow-edits, skip, in-repo exclusion, tsv rows), eligibility across fork+takeover+allow-edits+write / no-allow-edits / read-author, the toggle's fork split (refusal vs managed), plus plumbing pins (fork fetch/push forms, head_repo threading, first-pickup ack dedup). 60/60 + 12/12. * fix(autofix): strip stray patch-artifact quotes after two fi keywords Two inserted blocks ended 'fi"' — the quotes balanced against each other inside the same script, so bash -n stayed green while runtime would have lexed 'fi' as a command word and swallowed the span between them (the fork head-repo resolution tail and the engage-ack block) into one string. Removed both, and pinned the artifact class in the suite: a lone fi/done/esac followed by a quote now fails the tests. 61/61 + 12/12. * fix(autofix): author-filtered, re-armable first-pickup engage ack Reverse-audit findings on the scan-side ack: - Dedup was a raw grep over ic.json — a forged human comment carrying the engaged marker would have suppressed the real ack (and with it the window anchor). Dedup now selects bot-authored engaged acks via jq, same author rule as the window key itself. - Fork PRs get NO ack job (label events carry no secrets), so the documented re-arm gesture — remove and re-add the label to reset the round window — silently kept the old window: any historical ack blocked a fresh one. When a bot ack already exists, the scan now compares it against the takeover label's latest application time (issue events, fetched only in that rare case); a newer application posts a fresh ack, resetting window and cap as documented. Coverage: verbatim jq replays for both selections (forged-marker and released-marker exclusion, label-name filter, sort|last) plus the lexicographic re-arm gate pin. 61/61 + 12/12. * fix(autofix): review round 1 — ack ordering, dry-run, ghost-engage gate Addresses the maintainer review on #7213 (all findings confirmed): - Critical: the first-pickup ack read ic.json BEFORE the per-PR fetch — the first takeover candidate killed the whole scan step (missing file under -eo pipefail; every in-repo label-forced scan regressed), and later candidates dedup'd against the PREVIOUS PR's comments (bot PR ahead → fresh ack every 10min → window reset → cap never binds). The block now sits directly AFTER the fetch; its post-ack re-fetch keeps the downstream MARKERS/window-key reads fresh. A contract pin asserts the fetch precedes the first ack-timestamp read. - Medium: the ack now honors DRY_RUN (log only, window key untouched). - Medium: the command refused forks only for missing allow-edits — a below-write fork author was a silent ghost engagement (label sticks, no ack, nothing ever manages it). The command now mirrors the scan's author write+ gate with an actionable bilingual refusal. Found while fixing it: PR_INFO never fetched maintainerCanModify (or author), so EVERY fork toggle refused regardless of allow-edits — the test stub carried the field and masked the gap. Both engage-side fork gates are now also scoped to 'add': release is never blocked. - Low: the two new paginated jq reads are slurped (add-merged) so >100 comments/events cannot scramble the timestamp comparisons; replays now feed two concatenated page-documents. Fork fetch pins refs/heads/ (tag shadowing); HEAD_REPO_FULL guards each component (deleted fork = owner XOR name empty); fork-rotation caveat documented; forced-path refusal mentions the scheduled fork path. - Security caveat adopted: prepare proves fork push access with a --dry-run push right after checkout (allow-edits rides the classic-PAT grant only) and discards gracefully instead of 403ing after a full agent round. 61/61 + 12/12; YAML parses; every run block passes bash -n. * fix(autofix): fork targets keep base/branch invariants + 3 hardening follow-ups Blocking (yiliang114): the last fork elif ends the eligibility ladder for every eligible fork, so the LIVE_BASE/LIVE_BRANCH re-checks were unreachable for exactly the PR class the loop fetches and pushes — a labeled fork retargeted off main (or head-renamed) between scan and address would have had conflicts resolved against the wrong base. The base/branch invariants now sit ABOVE the fork chain (comment explains why the order is load-bearing), with replay cases pinning a retargeted and a renamed fork to the discard path. Follow-ups from the same review, all adopted: - PR_LIVE re-reads headRepositoryOwner/headRepository; a fork renamed or transferred since the scan discards at the live re-check (moved or unresolved, fail-closed) instead of fetching and token-pushing a stale path. The replay's fork fixture now carries its head repo and the harness provides the matrix HEAD_REPO to compare against. - The fork fetch failure (force-push/rename race) discards through the standard no-action path instead of a red run. - The first-pickup engage ack defers to the in-repo label event's DEDICATED ack job within a 3-minute grace after the label lands, so a concurrent ack job is never double-posted (which would shift the round-window anchor); a failed ack job is still healed by the next scan, and forks (no ack job) keep immediate pickup. Events are read once, before the branch split. Both hooks-order regex windows widened to span the new fork-arm guards (the assertions are about order; one hooksPath site genuinely covers both checkout arms). 61/61 + 12/12. * test: raise timeout ceiling for I/O-bound tests flaky under CI contention The self-hosted CI runners are heavily oversubscribed (core runs maxThreads: 16), and a recurring class of tests blows vitest's 5s default timeout purely under that contention — not from any logic fault. Observed repeatedly across unrelated PRs (#7213, #7219, and noted in prior sessions): - packages/core/src/utils/shell-ast-parser-lazy.test.ts — fully mocked, but the dynamic import + async coordination exceeds 5s when 16 threads contend. - packages/cli/src/serve/workspace-registration-store.test.ts — tempdir round-trip. - packages/core/src/extension/github.test.ts > extractFile — its waitForFileData helper polled a FIXED 1_000 setImmediate turns, which elapse in <100ms while the tar extraction I/O is still catching up, throwing 'Timed out waiting for extracted data'. Fixes: - testTimeout: 15000 in the core and cli vitest configs — 3x the default. Assertions still fail instantly; only the timeout ceiling grows, so this masks no logic bug (a real hang still fails, just later, and the job timeout still bounds it). - waitForFileData now polls a real ~10s wall-clock budget (2_000 x 5ms) instead of a fixed iteration count, so a slow extraction is awaited rather than raced. Stays under the 15s ceiling. These are the deterministic root-cause fixes for the flake class the autofix loop and CI Failure Patrol were papering over with reruns. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
5c7a5a84e0
|
fix(scripts): allow multiple dev:daemon instances by probing Vite port (#7212)
* fix(scripts): allow multiple dev:daemon instances by probing Vite port The Vite dev server port was hardcoded to 5173 with --strictPort, so a second npm run dev:daemon would crash immediately. Now the launcher probes for an available port (starting from 5173) and passes it explicitly via --port, matching how the daemon port is already handled. * fix(scripts): fail cleanly when the dev Vite/daemon port probe is exhausted The daemon and Vite port probes were bare top-level awaits, so an exhausted range (e.g. 5173–5182 all taken by other dev:daemon instances) rejected as an unhandled promise rejection — a raw stack instead of an actionable message. Wrap both probes and print the launcher's usual `[daemon-dev] <message>` before exiting 1, matching the validateLauncherArgs handling. findAvailablePort already rejects with the exhausted range, so the message names exactly which ports to free. * fix(scripts): let Vite handle port selection atomically Remove the probe-then-bind approach for the Vite port. The probe had a TOCTOU race (two launchers could pick the same port before either binds) and a host mismatch (probed 127.0.0.1 while Vite binds localhost which may resolve to ::1). Instead, drop --strictPort and let Vite auto-increment from its configured port 5173. Vite's --open uses the actual bound port, so the token URL is always correct. * fix(scripts): don't print a potentially wrong Vite URL in banner The banner hardcoded http://localhost:5173/ but with --strictPort removed the actual port may differ. Point at Vite's own output instead, which always prints the real bound port. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
易良
|
4e21f82c28
|
fix(ci): consolidate issue triage ownership (#7180)
* fix(ci): consolidate issue triage ownership Resolves #4786 * chore(vscode-ide-companion): regenerate NOTICES.txt to fix CI drift * fix(ci): close triage ownership review gaps * fix(ci): restore need-info label cleanup * docs(ci): document issue retriage triggers --------- Co-authored-by: Shaojin Wen <szujobs@gmail.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
|
Shaojin Wen
|
89db3d6aa0
|
fix(autofix): stage SKILL.md beside run-agent.mjs so review-address boots (#7225)
P0 regression from #7165. The review-address agent invokes a trusted staged copy of run-agent.mjs to avoid executing the PR branch's version on the host with the model key. run-agent.mjs resolves its instructions as `<own dir>/../SKILL.md`, but the staging was flat (${RUNNER_TEMP}/run-agent.mjs), so ../SKILL.md pointed at ${RUNNER_TEMP}/../SKILL.md = /home/runner/work/SKILL.md — which does not exist. Every review-address run since #7165 merged crashed with ENOENT before reading any feedback: the entire feedback-addressing and takeover path was down on main. Fix: stage the runner AND its SKILL in a mirrored layout (${RUNNER_TEMP}/autofix-skill/{SKILL.md,scripts/run-agent.mjs}) and invoke the staged runner from there, so ../SKILL.md resolves to the staged SKILL. This also closes a latent gap — the model's instructions now come from the trusted base too, never the checked-out PR branch. Regression guard: the suite pinned the cp string and the invocation but never checked SKILL.md was resolvable from the staged location. The test now derives the staged runner path from the invocation, computes <dir>/../SKILL.md, and asserts a cp stages exactly that — a flat re-stage fails. 60/60 + 12/12. Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
6acfc9a22f
|
feat(autofix): label-driven takeover and release; fix forced-dispatch green no-op (#7165)
* feat(autofix): label-driven takeover and release; fix forced-dispatch green no-op Takeover, exactly as designed: applying autofix/takeover (GitHub triage+ only — the permission gate is GitHub's own) summons the loop onto a PR, human-authored included; removing it releases the PR. The autofix/skip label opts any PR out at every engagement path — the autofix scan, the forced path, and the Fleet Shepherd walk — and wins when both labels are present. Every toggle gets a bilingual ack from the PAT-verified bot identity. The comment-command surface stays CLOSED: the pinned no-comment-commands contract test is untouched. Plumbing: pull_request labeled/unlabeled triggers; label events share the per-PR route group (the event class is triage-gated); the group expression also carries the #7163 payload trust prefilter so the two open PRs converge on the same final text in either merge order; scan candidates become bot PRs ∪ takeover PRs minus skip PRs (deduped); fork label events are logged and dropped (no secrets to even post a rejection). The forced-validation replay immediately caught a latent bug live since #6528: '(.isCrossRepository // true) | not' is false for EVERY input (jq's // treats false as empty), so every workflow_dispatch pr_number=N run — including the shepherd's conflict dispatches — validated to targets=[] and exited GREEN having done nothing. Fixed to '.isCrossRepository == false' (fail-closed on a missing field) with a replay case pinning the trap. Tests: autofix 55/55 (label engage/release pins, candidate-selection replay with skip-wins and fork cases, forced-validation replay across author/takeover/skip/closed/fork/missing-field); shepherd 12/12 (skip-filter replay). * feat(autofix): '@qwen-code /takeover' comment sugar over the takeover label Maintainer-mandated reopening of the comment surface, in the narrowest possible form: '@qwen-code /takeover' applies TAKEOVER_LABEL and '@qwen-code /takeover stop' removes it — nothing else. The label stays the single source of truth: engagement and release happen exclusively via the pull_request label events, so a manual label edit and the command are one mechanism with two entry points, and the command's whole blast radius is one label toggle. Gates: an expression-level startsWith prefilter keeps ordinary comments from ever starting a job; the body must match the constant EXACTLY after trimming (no parsing, no arguments); allowed senders are the PR author (who may lack label access — this is who the sugar is FOR) or a write+ collaborator via the same permission API used by review routing; closed PRs, non-PR comments, and the bot itself are ignored. The toggle job is PAT-verified and presence-aware (no-op toggles are explicit, since they fire no label event). The 'does not expose comment-triggered autofix commands' contract test is REWRITTEN into pinning this gated design, documenting the deliberate reversal. New behavioral replay drives the extracted command branch with a PATH-stubbed permission API across eight author/write/read/ exact-match/non-PR/closed/self cases. 56/56. * feat(autofix): raise the round cap to 50 while a PR is under takeover Large managed PRs routinely need dozens of feedback rounds — that is the point of takeover — so the unattended MAX_ROUNDS=5 would strangle exactly the PRs the label exists for. While TAKEOVER_LABEL is present the effective cap is TAKEOVER_MAX_ROUNDS=50: the circuit breaker stays (a bot/review-bot ping-pong is still bounded and every round still requires new trusted feedback or a conflict), it is just sized for explicitly delegated work. Removing the label restores the strict cap on the next scan. The scan computes the effective cap from the candidate's live labels and stamps it into the matrix target; the address job shadows the workflow-level MAX_ROUNDS with the matrix value, so every round message, marker, and cap gate uses the same number consistently (including #7163's address-time cap discard once both merge). 57/57 with a verbatim cap-selection replay (labeled → 50, plain → 5). * feat(autofix): re-armable round windows, cap raised to 100, visible cap pause The round counter is DERIVED state, stored nowhere but in the bot's eval-marker comments on the PR — and counting is now windowed by the latest '<!-- takeover-ack engaged -->' comment. Re-engaging (label off→on, or repeating the takeover command on an already-managed PR, which now posts a re-arm ack instead of a silent no-op) starts a fresh window: a PR that exhausted its rounds continues under management with one human action, auditable in the PR timeline. The WATERMARK stays global across windows — feedback already addressed is never replayed — and a PR never taken over has no ack, so strict lifetime counting is unchanged. The prepare-side live round is windowed identically, so pre-reset markers can neither trip the cap nor look like same-ts round-advance duplicates (replay-proven). TAKEOVER_MAX_ROUNDS rises to 100 per maintainer sizing, and pausing at the cap is now VISIBLE on managed PRs: a bilingual notice with re-arm guidance, once per counting window (marker-deduped past the latest re-arm; a failed post retries next scan). 58/58: rearm windowing replay (no ack → lifetime; ack → round 0 with watermark preserved; new rounds count from 1; latest ack wins), the stale-gate re-arm interplay case, cap-selection at 100, and cap-notice dedup pins. * feat(autofix): collapsed-Chinese bilingual takeover comments; fix ESLint regex-spaces Every takeover-flow comment — engage ack, release ack, re-arm ack, and the cap-pause notice — now follows the project convention: English body plus Chinese collapsed under <details><summary>中文说明</summary> (pinned at exactly four sites). Bodies are built via printf so no workflow indentation leaks into the markdown: the previous literal multi-line strings embedded 10 leading spaces, which would have rendered the trailing marker comment as a visible code block. Also fixes the CI failure at |
||
|
Shaojin Wen
|
030e335651
|
ci(autofix): harden the address path against stale targets and untrusted route events (#7163)
* ci(autofix): harden the address path against stale targets and untrusted route events Follow-up to #7127 addressing the five Critical findings of its post-merge review (4728499913): - Route group trust prefilter: the per-PR concurrency group is entered before any step runs, so an arbitrary commenter's review could cancel a queued legitimate route and then die in Decide phases. Reviews whose payload does not already look trusted (repo association or the review bot) now get a run-unique group — cancel nothing, still fully authorized inside. Decide phases remains the real permission gate. - Address-time eligibility recheck: a matrix job can start hours after its scan; a PR closed/merged meanwhile (or with changed author/repo/base/branch) is discarded BEFORE the PR branch checkout — no secret-bearing agent run, no push, no comment, no marker. A failed fetch discards too (unknown is not eligible). - Non-stale duplicates adopt the live watermark and round: a sibling may have evaluated F1 through T1 while this job carried watermark W; rendering from W would replay handled feedback to the agent, and reusing the matrix round would double-write a marker round. Both reporters consume the effective round. - Live round cap: when live markers already sit at MAX_ROUNDS the run discards (the scan itself skips capped PRs before conflict checks) — no round MAX+1 work, no second capped marker. - Stale discard suppresses the failure-path handoff: a late always() step failure after stale=true no longer converts a deliberate no-comment/no-marker discard into a handoff that consumes a round. Contract tests 53/53: W/T1/T2 replay proving only-F2 rendering via the adopted watermark, live-round adoption and cap-discard replays, sentinel-ts non-adoption, stale-suppressed handoff replay, and eligibility ordering/coverage pins. * ci(autofix): honor engagement labels in the address-time eligibility gate Forward-compatible with the incoming label-takeover feature so the two changes commute in any merge order: the eligibility recheck now reads labels live — autofix/takeover exempts a human-authored PR from the bot-author requirement, and autofix/skip (which wins over takeover) withdraws consent even if applied while the job sat queued. Both are enforced at the moment the secret-bearing run starts, not at scan time. Inert until the labels exist. * test(autofix): behaviorally replay the eligibility recheck Review suggestion, adopted: the discard path was only string-pinned — a future edit dropping the stale=true echo would leave every toContain green while STALE arrived empty downstream, letting a late always() failure post a spurious handoff for a discarded job. The recheck now runs VERBATIM under a PATH-stubbed gh across nine states: healthy bot PR proceeds writing nothing; closed-while-queued discards AND writes all four outputs later gates read; live takeover label exempts a human author; live skip label withdraws consent even for the bot's own PR; fork head, renamed branch, and a failed fetch (unknown is not eligible) all discard. 54/54. * fix(autofix): scan-side skip filter, honest labels comment, infra-distinct discard Review round on the hardening PR (six Suggestions, all adopted): - The scan candidate list now excludes skip-labeled PRs: the address-gate discard writes no marker, so an unfiltered scan would re-emit a skip-labeled PR into a full address job (checkout, npm ci, build) every tick forever. - The engagement-labels env comment no longer overpromises: in THIS change the labels are honored at the eligibility gate and scan filter; the scan-side widening that makes takeover summon human PRs ships with the takeover feature PR (the two commute either way). - A failed eligibility fetch now discards with an infra-distinct message (metadata fetch failed (API error) — fail-closed) instead of masquerading as state='unknown', mirroring the scan-side wording. - Tests: the non-main-base discard is now exercised behaviorally (not just pinned); the fetch-failure case asserts its distinct message; the full parenthesized route trust expression is pinned as ONE string (Actions binds && tighter than ||, so dropped parens would invert the grouping); and the terminal-sentinel adoption guard is exercised on a path that actually reaches the adoption block (live conflict skips the stale gate) instead of passing via the discard. 54/54. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
jinye
|
86ad532deb
|
perf(cli): Defer TUI runtime from ACP startup (#7182)
* perf(cli): Defer TUI runtime from ACP startup Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): Preserve state on lazy import failures Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): Colocate API error classifier coverage Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
1cacbb1bcc
|
ci(shepherd): add Fleet Shepherd — automated unblocking of the bot-PR fleet (#7142)
* ci(shepherd): add Fleet Shepherd — automated unblocking of the bot-PR fleet
A scheduled janitor that applies, per open bot PR, the smallest lever that
unblocks it — each lever was validated by hand across the fleet before being
automated here:
- conflict → dispatch the autofix loop for that PR (its --conflict path
merges base and resolves), deduped per conflicted head SHA via a marker
comment, with a bilingual notice on the PR
- stale base → update-branch when ≥25 commits behind main (fresh CI signal
and propagates workflow/skill fixes to the branch tree; self-limiting
because behind_by resets to zero after the sync), never while checks run
- known-flake red → rerun failed jobs only when EVERY failing test parsed
from the job log matches .github/known-flakes.txt (one unknown failure
blocks the rerun), only after the run fully concluded, max 2 attempts per
run id; the registry seeds the three repeatedly-observed flakes
- scan liveness → if no autofix full scan ran in 60 minutes and none is in
flight, dispatch one (GitHub cron was observed silent for 16h on a */10
schedule)
A single "Fleet Shepherd Dashboard" issue is edited in place each tick for
observability. Safety rails: bot-authored in-repo main-targeting PRs only,
per-tick action caps (3 syncs / 2 dispatches), dry-run dispatch input,
FLEET_SHEPHERD_DISABLED repository-variable kill switch, PAT identity
verified before any write; dispatches/reruns ride the workflow token while
comments/update-branch/dashboard use the bot PAT so synced branches still
trigger CI. Contract test (8 cases) pins triggers, scoping, credential
split, idempotency markers, caps, liveness, dashboard, and validates every
registry line as a compiling test-file regex.
* ci(shepherd): review fixes — gate reruns across ALL failed jobs, behavioral tests
Addresses the /review findings:
- The flake gate now enumerates EVERY failed job of the run via the jobs API
and parses failing tests from all their logs before deciding — matching the
header's "every failing test" promise, since `gh run rerun --failed` reruns
them all. A flaky Ubuntu test can no longer green-light a run that also has
a genuine Windows failure. Failed jobs exposing no parseable failing tests
(e.g. a coverage comment failing downstream of the test job) neither allow
nor block on their own — documented tradeoff. This also removes the URL
job-id parse entirely, mooting the /job-vs-/jobs concern (for the record:
statusCheckRollup detailsUrl is the web URL and uses singular /job/, as the
campaign's live extractions confirmed — but not parsing URLs is better).
- Behavioral tests: the log parser and the flake-gate pipeline are extracted
VERBATIM from the workflow (the test fails if either drifts) and executed
under bash — parser fixture → exact file list; gate matrix all-known→RERUN,
mixed→BLOCK, unknown→BLOCK, empty→SKIP. 10/10.
* ci(shepherd): review round 2 — atomic markers, dashboard lookup fix, hardened plumbing
Two criticals and the standing suggestions from review:
- act() now propagates the wrapped command's real exit status (still set -e
safe at the if-wrapped call sites), and every dedup marker is posted ONLY
when its primary action succeeded — a transient dispatch/rerun failure no
longer plants a marker that freezes the PR at that head or burns phantom
rerun attempts.
- Dashboard lookup used `--jq --arg`, which gh does not support (single
expression only) — DASH_NUM was always empty, creating a new dashboard
issue every tick. Fixed to an exact-title search with a plain expression.
- Scan-liveness signal now counts SCHEDULE runs plus the shepherd's own
liveness dispatches (recorded as a watermark in the dashboard body), so a
conflict dispatch — also a workflow_dispatch — can no longer satisfy the
watchdog and silently starve full scans; one run-list call now feeds both
the age and in-flight computations.
- Fleet metadata comes from ONE gh pr list call (no N+1 pr view loop);
actions reads (run state, jobs, logs) ride the workflow token whose
actions scope is guaranteed; update-branch passes expected_head_sha as a
compare-and-swap against races with the loop's own pushes; PENDING now
counts WAITING/REQUESTED; RUN_ID extraction is pipefail-guarded; the
registry filter is a trichotomy — rc≥2 (invalid pattern) BLOCKS reruns
instead of falling through to the rerun branch.
- Behavioral replay extended: invalid-ERE registry → ERRBLOCK (fail-safe
direction proven under bash). 10/10.
* ci(shepherd): review round 3 — cede rerun ownership to CI Failure Patrol; consumer-parity gates
The decisive review finding (raised independently by two reviewers): the
flake-rerun lever created a SECOND scheduled owner for the same failed CI
runs, unserialized against the existing Qwen CI Failure Patrol
(qwen-ci-flaky-rerun.yml) — exactly the rerun-vs-rerun race class observed
live during the fleet campaign. Single-owner principle applied: the lever,
its registry (.github/known-flakes.txt), and its tests are removed; the
shepherd now only REPORTS red CI on the dashboard, and the header documents
the non-goal. This also moots the sibling findings against the lever's
internals (stale first-FAILURE selection, unparseable-job neutrality, missing
fleet-wide rerun budget).
Remaining findings fixed:
- Conflict dispatch now mirrors its consumer's predicate: the autofix scan
skips PRs with checks in flight, so the shepherd defers the dispatch (and
its dedup marker) until checks are quiet instead of wasting the dispatch
and freezing the head behind a marker for a scan that never ran.
- A failed marker read now SKIPS the PR for the tick — an empty comment
history must never masquerade as "no markers" and double-fire actions.
- New behavioral test: act() is extracted verbatim from the workflow and run
under bash, proving a failing primary action returns nonzero (marker
skipped) and a succeeding one returns zero (marker posted). 9/9.
* ci(shepherd): honest budget accounting on marker failure; dry-run behavioral proof
Review round 4 (two suggestions):
- A successful dispatch now counts against MAX_DISPATCHES_PER_TICK the moment
it happens, with the marker posted afterwards; if the marker post fails the
note says so honestly and the possible next-tick re-dispatch is absorbed by
downstream dedup (the autofix scan's busy-PR skip and nothing-new gate).
The reviewer's ordering — counting only after a successful marker — would
let a marker outage overspend the per-tick dispatch budget.
- The act() behavioral replay now also proves the dry-run branch: with
DRY_RUN=true and `false` as the primary command, act() returns 0 without
executing it (DRY-OK), so an inverted condition or dropped return can no
longer pass on string presence alone. 9/9.
* ci(shepherd): scope-named conflict cap, wider liveness window, no empty-fleet overwrite
Review body-level confirms addressed:
- MAX_DISPATCHES_PER_TICK renamed to MAX_CONFLICT_DISPATCHES_PER_TICK: the
cap budgets conflict dispatches only, BY DESIGN — the liveness dispatch is
separately self-limited (dashboard watermark + 60-minute age + in-flight
gate ⇒ at most one per tick and usually one per hour), and sharing one
budget would let a liveness fire starve conflict handling. The name now
states the scope instead of inviting the bypass reading.
- Liveness run-list window widened to 50 so a review-event storm can't push
schedule runs or an in-flight scan out of view; the residual worst case
(one unnecessary dispatch) stays bounded by the watermark and the autofix
scan's own busy/nothing-new gates.
- A failed fleet enumeration now skips the walk AND the dashboard update —
the previous dashboard body is preserved instead of being overwritten with
a misleading empty table. 9/9.
* test(shepherd): rename to qwen-fleet-shepherd-workflow.test.js per convention
Every other workflow contract test in scripts/tests/ carries the -workflow
suffix; match it so workflow-test globs include this file.
* ci(shepherd): if-wrap every act(), exact-title dashboard, drop vestigial checkout
Review round 7:
- act() propagates real exit codes, so under the runner's set -eo pipefail a
BARE failing call aborts the whole tick — the reviewer's sharpened framing
of the marker-call issue (that call was already if-wrapped in the previous
round; the residue was the two dashboard writes). Both are now if-wrapped
with retry-next-tick warnings, and the test asserts no bare act call
remains anywhere.
- Dashboard lookup: exact-title equality via standalone jq --arg (in:title is
a substring search — a bystander issue merely containing the title must
never be hijacked and overwritten), and a FAILED lookup now skips the
dashboard for the tick instead of minting a duplicate via create-on-failure
(same fail-closed rule as the fleet and marker reads).
- Removed the vestigial checkout step: with the rerun lever ceded to the
Patrol, the run step reads no repo files — every read goes through the API.
10/10.
* ci(shepherd): fail-closed run snapshot, liveness-scoped in-flight, shepherd-own busy-set
Review round: three Criticals against the run-snapshot section.
- A failed autofix run-list read sets SCAN_RUNS_OK=false and skips the
liveness lever AND every conflict dispatch for the tick, instead of
falling back to '[]' — an empty snapshot both zeroed the in-flight
count and blanked the schedule signal, so the watchdog could stack a
duplicate scan on top of a live one.
- SCAN_INFLIGHT counts only SCHEDULE runs plus workflow_dispatch runs
whose createdAt sits within 180s of our recorded liveness watermark.
Forced conflict dispatches can hold a run in progress for up to two
hours; counting them starved the watchdog and froze full scans exactly
when the fleet was busiest.
- The shepherd builds its OWN busy-set from live runs' review-address
matrix jobs and defers a conflict dispatch while one is live or
queued (unknown snapshot => every dispatch deferred). The
marker-failure retry note claimed 'downstream dedup' that lives in an
unmerged PR; the dedup now travels with this workflow.
Contract tests: 11/11, including a verbatim-extracted jq replay proving
a foreign forced dispatch (created far from the liveness watermark)
neither satisfies nor starves the watchdog.
* ci(shepherd): declare set -eo pipefail explicitly
The act() contract, every if-wrapper, and the behavioral replay all
assume strict-mode semantics. Actions' default bash gives -e (bash -e
{0}) but NOT pipefail — declare both so production matches the tested
contract and a future bare act() call fails loudly instead of silently
posting its marker. The one real pipeline in the tick (the liveness
watermark grep) already carries || true.
* ci(shepherd): platform-blind CI-red on the dashboard; document first-tick edge
Review notes (non-blocking, adopted): the dashboard's ci-red state
matched only 'Test (ubuntu' — a Windows- or macOS-only regression
stayed invisible on the health view. Widen to any Test platform
(reruns stay with the Patrol either way) and pin it. Also record the
first-tick liveness edge at the in-flight computation: with no
watermark yet a manual dispatch in flight is not counted and one
duplicate scan may go out; the scan's own busy/nothing-new gates
absorb it, and every later tick has the watermark.
* ci(shepherd): unknown watermark and partial busy-set fail closed
Review round: two remote-read failure paths still turned unknown state
into actionable empty state.
- The liveness watermark lives in the dashboard body, so a failed body
read (not just a failed lookup) now flips DASH_LOOKUP_OK: the
dashboard is not overwritten (that would destroy the stored
watermark) and the liveness lever is skipped — otherwise, with cron
already stale, a PAT outage would dispatch a duplicate full scan
every tick for its duration. The lever now requires known watermark
state, a good run snapshot, expired age, and zero in-flight.
- Every busy-set jobs read is tracked: BUSY_OK inherits SCAN_RUNS_OK
and flips false on the first failed gh run view or jq parse, because
a partial enumeration is unknown busy-state, not a smaller busy-set
(a gh failure inside process substitution never trips the parent
shell). Conflict dispatches now gate on BUSY_OK.
New behavioral replay (PATH-stubbed gh over the verbatim-extracted
walk): failed jobs read → BUSY_OK=false with an empty set; successful
read → the queued review-address PR lands in the busy-set; no live
runs → known-empty set with dispatches enabled. 12/12.
* ci(shepherd): fetch markers only for conflicting PRs
Review suggestion, adopted: MY_MARKS was fetched (paginated) for every
PR in the fleet but consumed only by the conflict lever's dedup —
~100 wasted comments fetches per tick on a 50-PR fleet — and a failed
fetch on a NON-conflicting PR dropped the PR from the dashboard,
stale-base sync, and CI-red reporting via the shared continue. The
read now lives inside the CONFLICTING branch behind MARKS_OK: a failed
read defers just the conflict dispatch (empty history still never
masquerades as no markers), and every other lever keeps working.
* ci(shepherd): link the dashboard ci-red state to the failing job
Review suggestion, adopted: FAILED_TEST_URL was extracted per PR and
then only tested for non-emptiness — the dashboard said 'ci red' with
no way to reach the failure, forcing exactly the per-PR navigation the
dashboard exists to eliminate. The state cell is now a markdown link
to the failing Test job.
* ci(shepherd): attribute in-flight liveness by run id, not timestamp proximity
Review round: the ±180s attribution window necessarily miscounted a
conflict dispatch fired later in the SAME tick — created seconds from
the liveness watermark, its two-hour address run then suppressed the
age>=60m dispatch on every later tick, the exact starvation the header
claims to prevent. And the behavioral test only exercised a foreign
dispatch 40 minutes outside the window, masking the reachable case.
The liveness dispatch now records its run id: captured right after the
dispatch (a correlation window in which no other dispatch can exist —
conflict dispatches fire later in the walk), persisted in the
dashboard marker as 'run=<id>', and matched by id equality next tick.
No proximity window exists at all. Unknown id (capture failure or a
pre-id marker) attributes nothing, so the failure mode is one absorbed
duplicate scan — never starvation. The replay now proves the
reviewer's in-window case: a conflict dispatch created 5s after the
watermark counts 0, and our completed run plus that live conflict run
also counts 0. 12/12.
---------
Co-authored-by: wenshao <wenshao@example.com>
|
||
|
Shaojin Wen
|
ee0cc79739
|
ci(autofix): fan out review targets and stop route-scan starvation (#7127)
* ci(autofix): fan out review targets and stop route-scan starvation
Two throughput fixes for the review loop, both observed live:
- review-scan emitted ONE newest-first target per scan ("single-target
worker"). With sparse cron ticks this starves older armed PRs for hours —
an armed PR sat unprocessed for 16h while newer PRs took every tick. Emit
EVERY eligible target instead: the address matrix's max-parallel (3) bounds
simultaneity and the per-PR concurrency groups already prevent duplicate
same-PR runs, so one surviving scan drains the whole backlog.
- route used a single shared concurrency group with cancel-in-progress. Under
runner backlog a route job sits QUEUED for minutes, and any newer event
(review submissions arrive constantly) cancelled it — five consecutive
dispatched scans died this way; during event storms no full scan survived
at all. Cron ticks keep deduping through a shared 'route-cron' group, but
dispatches and review/issue events now get unique per-run groups: route is
a seconds-long job, so never cancelling it costs nothing and every trigger
is guaranteed to route.
Contract test updated: fan-out asserted (no single-target break, matrix
max-parallel), new route concurrency expression pinned. 50/50.
* ci(autofix): cap targets emitted per scan (review defense-in-depth)
Review note on the fan-out: bound the scan's output for a pathological
backlog. Clarifications recorded in-thread — the loop lives in review-scan
(timeout 15m), not route (5m), and the pre-change worst case already walked
the full candidate list (break fired on the first ELIGIBLE PR, not the first
candidate) — but an explicit bound is good hygiene: emit at most
MAX_TARGETS_PER_SCAN (10) targets, LOG the deferral (never a silent cap),
and let the next scan pick up the remainder since their signals persist.
Contract test pins the cap, the deferral log, and the slice.
* ci(autofix): review round 2 — per-target route coalescing, busy-PR skip, in-loop budget
Both criticals and the suggestion from review, each verified against live
campaign observations:
- Route concurrency is now keyed by TARGET: cron ticks still coalesce with
each other; review events coalesce PER PR (near-simultaneous reviews on one
PR route once — the one useful side effect of the old shared group,
restored — without events on other PRs cancelling this one); issue events
coalesce per issue; dispatches stay unique and are never cancelled. This
keeps the starvation fix while closing the duplicate-forced-scan window the
per-run_id grouping had opened.
- The scan now skips any PR whose review-address job is RUNNING OR QUEUED in
a live autofix run (one runs-list plus a jobs-view per live run). A
fanned-out matrix holds queued jobs past a 10-minute tick and
schedule/dispatch runs never surface in the PR's checks, so without this
the next scan re-emitted the same PRs and per-PR groups accumulated
duplicates that later replayed stale watermarks — the exact duplicate-round
behavior observed live on the fleet.
- The per-scan target budget now BREAKS the candidate loop instead of slicing
after it, so it genuinely bounds scan runtime and API usage (each candidate
costs several serial reads); the deferral is logged and the remainder keeps
its signals for the next scan.
Contract test updated for all three (route expression per target, busy-skip
message + capture regex, in-loop budget break). 50/50.
* ci(autofix): discard stale duplicate targets via live-watermark revalidation
Review: the busy-set closes the queued-matrix window but not the pre-matrix
one — two near-simultaneous same-PR triggers can both scan before either has
emitted a matrix job, so both emit the PR with the same stale watermark, and
the per-PR address group QUEUES (not discards) the duplicate.
That queueing is exactly what makes revalidation sound: address jobs for one
PR run strictly one at a time, so when the duplicate reaches prepare, the
first job's eval marker is already posted. Prepare now recomputes the
watermark from LIVE markers; if it advanced past the matrix watermark and
nothing (reviews / inline / issue comments / failed checks) is newer — and
there is no conflict — the run marks itself stale and the address + verify
steps are skipped entirely: no agent run, no marker, no comment, no push.
Contract test pins the revalidation, both step gates, and the now-three
shared address-carve-out sites. 50/50.
* ci(autofix): filter live runs server-side in the busy-set listing
A client-side status filter over the 15 newest runs loses a long-lived
fanned-out run once cron traffic (~6 runs/hr) pushes it past the
window — its queued review-address PRs silently stop looking busy and
the next scan re-emits them with a stale watermark. Query in_progress
and queued server-side instead, so the limit applies to LIVE runs only
(at most a handful) and the window cannot be starved by completed runs.
One status query failing does not hide the other (|| true per query);
an empty set stays fail-open by design — the address-side live-marker
revalidation is the second line of defense.
* ci(autofix): document route-group cases and busy-set fail-open contract
Review notes (non-blocking, adopted): a four-case summary above the
chained route-concurrency ternary for the next reader hitting it in a
blame, and an explicit contract at the busy-set listing — a double
status-query failure is deliberately fail-open because the skip is an
optimization and the address-side live-marker revalidation is the
correctness gate; if that revalidation is ever removed, this read must
become fail-closed.
* ci(autofix): discard conflict-only duplicates; fix dead recount in the stale gate
Review round: the ts-only revalidation missed a conflict-only
duplicate. Two overlapping scans emit the same conflicted PR with
watermark W; the first serialized job resolves the conflict and — with
no newer feedback — its marker keeps ts=W while its round advances.
The second job then sees CONFLICT=false live but LIVE_EVAL_WM == W, so
the strict > gate never fired and the agent re-ran against resolved
work. The gate now also extracts LIVE_MAX_ROUND and treats
same-ts-with-newer-round (conflict cleared) as a duplicate signature,
still subject to the nothing-newer recount.
The behavioral replay the reviewer asked for immediately caught a
latent bug in the previous fix: the recount jq opened with '((' and
never closed it, so it failed to compile, LIVE_NEW stayed empty, and
the whole stale gate was dead code in production. Fixed to a single
paren; the replay now proves five transitions (conflict-only duplicate
discards, first conflict job proceeds, live conflict always proceeds,
ts-advanced duplicate discards, round-advanced-with-new-feedback
proceeds).
---------
Co-authored-by: wenshao <wenshao@example.com>
|
||
|
Shaojin Wen
|
582fb49603
|
feat(web-shell): git status chip, visual working-tree diff, and sidebar git status (#7054)
* feat(web-shell): git status chip, visual working-tree diff, and sidebar git status
Bring working-tree Git awareness to the Web Shell (browser daemon session UI):
- Toolbar branch chip becomes a live status indicator: dirty (staged/unstaged/
untracked), ahead/behind upstream, stash count, detached HEAD, in-progress
operation (merge/rebase/cherry-pick/revert/bisect), and conflict count, each
with a non-color cue.
- Read-only "Changes" dialog: working-tree-vs-HEAD file list with per-file,
line-level, per-side syntax-highlighted diffs; opens via /diff or a dirty
chip; untracked files expand as fully-added and deleted files still diff.
- Per-workspace git status in the sidebar: a compact icon-only chip per trusted
workspace (status dot + hover tooltip); click opens that workspace's dialog.
All git access goes through the daemon REST API with per-workspace trust
gating; new SDK status fields are optional and additive (v2).
* fix(web-shell): themed tooltips and git-chip review follow-ups
Tooltips now render on the themed popover surface (bg-popover /
text-popover-foreground / border + fill-popover arrow) instead of the
inverted bg-foreground default, so they read dark-on-dark rather than a
bright box on the dark theme. Fixing the shared primitive corrects the
git branch tooltip in the composer toolbar and sidebar, plus every other
tooltip, at once.
Also addressing review feedback on the git integration:
- Replace the hand-drawn detached/conflict/stash SVG icons with
lucide-react (CircleDot / TriangleAlert / Layers) per the web-shell
icon convention.
- Gate the tooltip "Working tree clean" message on an enriched status
(computedAt) so a branch-only status no longer asserts clean.
- Include the file path in the diff dialog row aria-label so screen
readers can distinguish files.
- Reset the toolbar git chip on workspace switch so it never shows the
previous repo's branch/counts while the new fetch resolves.
- Log a sidebar git poll failure only on the success->failure transition
to avoid spamming a long-lived tab.
- Correct the SDK doc for DaemonWorkspaceGitDiffFile.added/removed
(0, not undefined, for binary files).
* fix(web-shell): address git-integration review suggestions
Follow-ups from the /review pass on the git integration:
- GitBranchIndicator: include the short SHA in the detached-HEAD tooltip
title, and add the "Working tree clean" status to the aria-label (gated
on an enriched status, matching the tooltip) so the two never drift.
- WorkspaceSection: keep the last known git status on a transient poll
failure instead of blanking the chip for a whole interval.
- App: surface a toast for `/diff` when no workspace is available instead
of silently consuming the composer input.
- Tests: cover the diff dialog's list-load and per-file load error paths,
and detectGitOperation's revert/bisect branches.
- Design doc: align the getGitWorkingTreeStatus spec text with the
decision (transient states return status with `operation`; null is
reserved for non-repo / git failure).
* fix(web-shell): focus-visible ring for git chip button; align doc poll interval
- Add a :focus-visible outline to .gitBranchChipButton so keyboard users
get a visible focus indicator (the chip resets UA button chrome).
- Design doc: align the active-workspace poll-interval references at 30s
to match the implementation.
* fix(web-shell): surface capped diffs, catch row-build failures, cover degradation paths
Address the remaining review findings on the git integration:
- Truncation is no longer silent: fetchGitDiffHunksForFile now returns
{ hunks, truncated } — the parser records files that actually lost
lines to MAX_LINES_PER_FILE (tracked path), and the untracked
synthesis reports its byte/line caps. The route forwards an additive
`truncated` flag on the hunks response (absent when not truncated, so
older clients and daemons are unaffected), and the Changes dialog
renders a "Diff truncated" note under the visible window.
- DiffHunks catches an unexpected buildRows rejection (e.g. malformed
hunk lines) and shows the per-file error instead of leaving an
unhandled rejection and a silently empty diff area.
- New tests: untracked and tracked truncation at the core caps, the
route's truncated passthrough (and its absence when clean), the
branch-only degradation when the working-tree summary throws, the
malformed-hunks error path, and the Shiki success path (a fake
tokenizer proving add rows pull new-side tokens and del rows pull
old-side tokens, not the plain-text fallback).
* fix(web-shell): drop dialog backdrop-blur that froze the page on open
The dialog and alert-dialog overlays applied `backdrop-blur-xs`, which
forces the browser to rasterize and blur the entire content behind the
overlay when a dialog opens. With a long transcript behind it, that
main-thread paint+blur froze the whole page — e.g. clicking the git
branch chip to open the Changes dialog. Keep the bg-black/10 scrim for
separation and drop the blur.
* fix(core): guard synthesizeUntrackedHunk against non-regular files
synthesizeUntrackedHunk opened an untracked path before checking its
type, so an untracked FIFO (listed by `ls-files --others`) would block
on open() forever waiting on a writer — hanging the daemon's event loop
and leaving the Web Shell Changes dialog stuck on a permanent loading
state. lstat-gate on regular files before opening, matching the existing
guard in countUntrackedLines. Adds a FIFO regression test.
* fix(web-shell,core): rename expansion, no-newline marker, chip measurement
Round-5 review Criticals:
- core: key renamed diff entries by the real (post-rename) path and carry
the old path for display, so renamed rows can be expanded — the synthetic
`old => new` key was sent to git as a nonexistent literal path. The diff
dialog renders the rename as `old → new`.
- core: preserve Git's `\ No newline at end of file` marker through the hunk
parser so a trailing-newline-only edit isn't shown as identical
removed/added lines (the viewer already renders it as a meta row).
- web-shell: the toolbar's hidden git-chip measurement replica now renders
the full chip content via the extracted GitBranchChipContent, so the
expanded width includes the status indicators and the compact/expanded
toggle no longer oscillates near the responsive threshold.
* fix(build): generate git-commit info even when prepare build is skipped
The review tooling runs `npm ci` with QWEN_SKIP_PREPARE=1 (to skip the
heavy prepare build) and then builds only the changed workspaces. Because
`prepare` exited before generating the gitignored git-commit.ts, a
per-workspace build of packages/cli failed at the unchanged systemInfo.ts
on the missing `../generated/git-commit.js` module. Generate the git-commit
info in the skip path too — it is cheap and never fails hard — so a later
per-workspace build or typecheck finds the module. The non-skip path still
generates it via `npm run build`.
* fix(web-shell,cli): address round-6 review suggestions
- cli: carry the pre-rename path (oldPath) through DiffRenderRow and show
renamed files as `old → new` in both the Ink and plain-text renderers.
The rename-keying fix updated the daemon and web-shell dialog but not the
CLI `/diff` renderer, which silently dropped the old path.
- web-shell: key DiffFileRow by workspace + path so switching workspace
remounts the row instead of reusing another workspace's hunks/open state
for a path both workspaces share.
- web-shell: show a loading placeholder in DiffHunks while rows are (re)built
(e.g. after a theme switch) instead of an empty, jumpily-resized box.
- web-shell: cover the /diff local intercept in App.test.tsx (opens the
Changes dialog and is not forwarded to the agent).
* fix(web-shell,cli,core): address round-7 review suggestions
- cli: sanitize the rendered filename (and pre-rename oldPath) in the Ink
DiffStatsDisplay via sanitizeFilenameForDisplay, matching the plain-text
renderer so a crafted path can't inject into the interactive view.
- cli: apply the read headers before awaiting the per-file diff fetch (as
handleDiffList does) so error responses also carry no-store/nosniff.
- cli + web-shell: strip Unicode bidi embedding/isolate controls
(U+202A-202E, U+2066-2069) in the filename/control-char sanitizers so a
crafted filename can't visually spoof its extension.
- core: guard countStashEntries with an lstat type check before readFile, so
a symlink-to-FIFO at logs/refs/stash can't block the event loop (the same
hazard already guarded in the untracked-file readers).
- core: cover fetchGitDiffHunksForFile's transient-state guard with a test
(the sibling helpers already had one).
* fix(web-shell,cli,core): address round-8 review suggestions
- core: pass --no-optional-locks to the ls-files call in
fetchGitDiffHunksForFile, matching the other runGit calls so it doesn't
contend for an optional index-refresh lock alongside concurrent git
add/commit.
- cli: add a route test asserting a rename's oldPath survives serialization
end-to-end (keyed by the new path, old path carried alongside).
- web-shell: add a GitDiffDialog test for the hiddenCount>0 "N more files
not shown" note (every payload previously used hiddenCount: 0).
- web-shell: drop the nonexistent primaryLabel prop from the WorkspaceSection
test (it is not a WorkspaceSectionProps member).
- docs: correct the plan doc — large-diff virtual scrolling was explicitly
descoped (core caps + per-file lazy loading), not implemented in Phase 2.
* fix(cli,web-shell): address round-9 review findings
- cli: propagate the pre-rename oldPath through DiffDialog's
perFileToUnified and render renamed files as `old → new` in the
interactive diff viewer (the rename-keying fix had updated the daemon,
the web-shell dialog, and the /diff stats, but not this viewer).
- cli: cover DiffStatsDisplay's rename (`old → new`) rendering and the
sanitizeFilenameForDisplay path for hostile filenames carrying control
characters.
- web-shell: guard the GitBranchIndicator test afterEach against
double-unmounting an already-unmounted root (the localization tests
assert on getTranslator without calling render()).
* fix(core,cli,web-shell): rename-aware single-file diff (old→new)
fetchGitDiffHunksForFile pathspec-limited the diff to the new path, which
defeats git's rename detection — a renamed file was reported as fully
added (every line +) instead of its actual edit. Thread an optional
pre-rename path through the single-file endpoint (core → route → SDK →
dialog) and diff old→new with -M when it is present, so expanding a
renamed file shows its real content change.
* fix(cli): address round-10 review suggestions
- DiffDialog: split the path-width budget between old and new paths for a
rename (reserving the " → " separator) so the combined width stays within
maxPathChars instead of overflowing the row layout.
- textUtils: extend MULTILINE_CONTROL_CHARS_REGEX with the Unicode bidi
ranges (matching FILENAME_CONTROL_CHARS_REGEX) and add a test that
sanitizeFilenameForDisplay strips bidi embedding/isolate controls.
- workspace-git-diff route: add a test that ?oldPath= is parsed and
forwarded to fetchGitDiffHunksForFile.
* test(sdk),docs: cover diff client methods; align design doc
- sdk: add DaemonClient unit tests for workspaceGitDiff() and
workspaceGitDiffFile(path, oldPath?) — URL construction (incl. urlEncode
on path/oldPath, with and without oldPath, plus the workspace-qualified
route) and response deserialization, mirroring the existing workspaceGit()
test.
- docs: add the oldPath? param to the workspaceGitDiffFile API spec; record
that the diff client methods now have unit tests (correcting the claim
that workspaceGit() had none); attribute the bundle-limit bump to
packages/sdk-typescript/scripts/build.js; clarify ahead/behind are relative
to upstream (0, and ↑N/↓N not shown, without one).
* fix(web-shell,core): address round-11 review suggestions
- GitBranchIndicator: count conflicted entries as dirty — a merge where every
changed file is conflicted (staged=unstaged=untracked=0) is still
uncommitted, so the expanded chip's dirty dot / data-dirty now reflect it.
- core: split the status branch line at the last "..." (the branch/upstream
separator) so a dotted branch name isn't truncated at the first "...".
- GitDiffDialog: guard DiffFileRow's in-flight fetch against unmount via a
cancelled ref, matching DiffHunks / GitDiffDialog.
- tests: forward oldPath when expanding a renamed file in the web-shell
dialog; bidi-strip coverage for the web-shell sanitizeControlChars;
untrusted-guard coverage on the single-file diff route; conflicted-only
dirty; branch-line "..." split.
* fix(web-shell,cli): address round-12 review suggestions
- DiffDialog: only render the rename "old → new" when there's room for both
sides (≥19 cols, so each gets ≥8); otherwise fall back to the new path
alone, so a narrow terminal no longer overflows the row (the Math.max(8,…)
floor could exceed maxPathChars).
- GitBranchIndicator test: guard afterEach container.remove() for non-render
tests run in isolation, and make the compact-mode ↑-suppression assertion
non-vacuous by giving the fixture an ahead count.
- App: compute the active workspace once (useMemo) and share it between the
git-status effect and the Changes-dialog entry point, so the chip and the
dialog can't drift onto different repos.
* fix(core,docs): address round-13 review suggestions
- core: add a rebase-apply detection test (git am / an interrupted
`rebase --apply` creates rebase-apply, which detectGitOperation also maps
to 'rebase'); previously only rebase-merge was exercised.
- docs: correct section 5 to describe the actual diff-dialog mechanism
(diffWorkspaceCwd state, not the stale activePanel design).
* test(core): cover stray no-newline marker before any hunk header
parseGitDiff's pre-hunk guard already skips a "\ No newline at end of
file" marker that appears before any @@ header, so a malformed/truncated
diff can't throw on a null currentHunk and lose subsequent files' hunks;
add a regression test pinning that behavior.
* fix(web-shell): unstick per-file diff loading and skip non-path git poll
- DiffFileRow: reset the cancelled-fetch flag on mount so StrictMode's
mount/unmount/mount replay no longer leaves it latched at true, which
dropped the fetched hunks and froze the row on "Loading changes…" despite
a 200 response.
- WorkspaceSection: skip the git status poll when the workspace cwd is not an
absolute path. A synthetic fallback workspace carries a display name there,
which the cwd-qualified route rejects with a 400.
* fix(web-shell,cli): address review suggestions on the git diff surface
- GitDiffDialog: highlight each diff side independently so a small side
keeps syntax highlighting even when the other side exceeds the size cap
(the old guard dropped both as soon as either was too large).
- ChatEditor: complete the .gitBranchChipButton reset (font/color/padding/
margin) so the clickable dirty-tree chip matches the read-only output chip
instead of picking up UA button styling.
- DiffDialog: cover the interactive rename display (old to new on a wide
terminal), mirroring the rename tests DiffStatsDisplay and GitDiffDialog
already have.
* test(web-shell,cli): cover git chip clean/reload/traversal paths, fix doc
- GitDiffDialog: add the missing expect(header).not.toBeNull() guard to the
three expand-file tests that lacked it, matching the others in the block.
- GitBranchIndicator: cover the known-clean aria-label branch (computedAt set
and every change counter zero).
- WorkspaceSection: verify a reloadToken change re-fetches git status instead
of waiting for the next 60s poll.
- workspace-git-diff route: verify a traversal oldPath is forwarded to core
and surfaced as available:false rather than escaping the workspace.
- Design doc: /diff is handled via setDiffWorkspaceCwd, not setActivePanel.
* fix(core): allow literal `..foo` paths in diff normalization
- toRepoRelativePath: reject only a real climb-out (`..` or `../…`), not a
literal `..foo` filename at the repo root, which the bare startsWith('..')
over-rejected, leaving the diff viewer unable to render such a file.
- parseGitDiff: cover the truncatedPaths output set directly (it was only
exercised indirectly through fetchGitDiffHunksForFile).
---------
Co-authored-by: wenshao <wenshao@example.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
|
||
|
jinye
|
b67506b70b
|
feat(daemon): Profile ACP channel initialization (#7145)
* feat(daemon): Profile ACP channel initialization Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#7145) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
Dragon
|
a516040775
|
fix(core): align system prompt with interaction mode (#7089)
* fix(core): align system prompt with interaction mode
* refactor(core): centralize interaction-mode resolution; document override boundary
Extract the ACP > interactive > headless precedence into a shared
resolveInteractionMode(config) helper in prompts.ts and use it from both
client.ts and the /context command, so the two call sites can no longer
drift apart. Add a comment documenting that a QWEN_SYSTEM_MD override is
intentionally not augmented with interaction-mode guidance (the override
is a full, user-owned prompt). Update the client prompts mock to keep the
pure helper real while still spying the prompt builders.
* fix(core): resolve stream-json sessions to ACP interaction mode
resolveInteractionMode only checked getExperimentalZedIntegration() for
ACP, so a --input-format stream-json session (without Zed) resolved to
headless and the prompt told the model to never ask a question. The
runtime question/permission sites (askUserQuestion, enterPlanMode,
coreToolScheduler) treat stream-json as ACP-capable, so the model was
denied a tool the host can actually relay. Align the prompt with the
runtime by treating stream-json as ACP, and harden the headless negative
test to reject affirmative ask_user_question guidance.
* test(core): cover resolveInteractionMode; use InputFormat enum
Address review feedback on the interaction-mode resolver:
- Replace the hardcoded 'stream-json' literal in resolveInteractionMode
with the InputFormat.STREAM_JSON enum, matching the runtime sites
(askUserQuestion, enterPlanMode, coreToolScheduler) and keeping the
check rename-safe.
- Add direct unit tests for resolveInteractionMode covering the Zed,
stream-json->acp, interactive, and headless branches, ACP precedence,
and the optional-getInputFormat fallback.
* fix(core): restore exhaustiveness check in getInteractionModePrompt
Replace the combined interactive/default fallthrough with an explicit
interactive case plus a never-typed default guard, so adding a new
SystemPromptInteractionMode without handling it fails typecheck instead of
silently receiving the interactive prompt. The default still returns the
interactive prompt at runtime to keep behavior safe for unexpected values.
* refactor(core): use ToolNames.ASK_USER_QUESTION in interaction prompts
Replace hardcoded 'ask_user_question' literals in the headless, acp, and
interactive interaction-mode prompt strings with interpolated
${ToolNames.ASK_USER_QUESTION}, matching how the rest of prompts.ts
references tool names. Keeps the rendered prompt identical while making
the strings track the constant if the tool is ever renamed.
* test(core): cover headless mode in interaction-mode prompt test
Add the headless row to the parameterized client.test.ts case so the
isInteractive=false, zed=false -> headless mapping is asserted deliberately
alongside interactive and acp.
* test(core): assert arena workers use the headless system prompt
Add a test that verifies ArenaManager builds the in-process worker
system prompt in headless interaction mode, guarding that 'headless' is
passed to getCoreSystemPrompt. A regression dropping that argument would
fall back to the interactive prompt and instruct arena workers to ask
questions no one can answer.
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Shaojin Wen
|
25bef232e1
|
fix(review): report what the transcripts prove; build the roster in one call (#7033)
* fix(review): name a rewritten launch as itself, and leave nothing to hand-assemble
Dogfooded on a real 3A review of a live PR, and the run talked its way past the gate:
compose-review printed: Verdict: Comment — an Approve was NOT available: a
dimension nobody reviewed
the run's next thought: "the compose-review flagged reverse audit as unreviewed
(transcript visibility issue — the reverse audit did run substantively with two
dry rounds). Let me proceed."
the run then reported, and saved: Verdict: Approve
The gap was right and its wording was wrong, and the wording is what let the run
dismiss it. Two auditors HAD run — 16 and 23 tool calls each — and both HAD opened
their brief. What had actually happened is that the orchestrator skipped `--findings`
and hand-wrote their launches, keeping only the brief pointer, so no agent was
launched with the prompt the CLI built. The gap said "no agent was launched with it
that opened its brief", which is false as written, and "a transcript visibility
issue" is what a reader concludes from a message that does not describe what
happened.
So the floor tells the four shapes apart instead of collapsing them into one
boolean, and each says what happened and what to do:
not-built — the step was skipped; `agent-prompt --role <r>` never ran
not-launched — the prompt was built and nothing was launched with it
rewritten — an agent ran and opened its brief, but no agent got the built
prompt: the launch was written by hand instead of pasted
brief-unread — an agent got the built prompt and never opened the brief
`rewritten` is the one that just happened, and it is now un-dismissable: it concedes
the agent ran and read its brief, and names the orchestrator's own edit as the defect.
And the path that produced it is gone: `--findings` is now REQUIRED for a role that
takes findings. There is no bare-block-plus-hand-assembly path left — the command
refuses, and prints one block to paste. An early reverse-audit round with nothing
confirmed yet passes an empty file, which the command renders as "Nothing is
confirmed yet".
SKILL: Step 4/5 say `--findings` is required. Step 6 gains the second half of the
lesson — you may not overrule the line compose-review gives you; a cap you can
explain is still a cap, and the fix is to make the step verifiable and re-run, not to
keep the verdict you preferred. Step 8's report interpolates the verdict out of the
composed JSON (`jq -r .event`) instead of typing it, because the terminal is prose
and the archive is forever.
* fix(review): call the CLI that is running, not whatever `qwen` PATH finds
Reported from a real session: `npm run dev:daemon`, `/review 6998 --comment` in the
web shell, and the run died on
Missing required argument: chunk
with a help screen for a command that has no `--role` at all. The daemon was running
the checkout — the skill it loaded is the current one, and says `--role 0` — but the
skill shells out to `qwen review agent-prompt …`, and `qwen` on that machine is
`/usr/bin/qwen` → a v0.19.10 global install whose `agent-prompt` predates #6892
entirely. The skill and the CLI it was talking to were different programs.
The skill assumed `qwen` on PATH is the build running it. That holds for a single
install and breaks for exactly the people most likely to run a dev daemon. It is also
invisible when it breaks: the error names an argument, not a version.
So the entry is passed down instead of rediscovered. `scripts/cli-entry.js` is the
executable entry and the one thing that knows its own path, so it publishes it as
`QWEN_CODE_CLI` (`||=`, so the relaunch into dist/cli.js keeps pointing callers back
at the wrapper with the shebang, not at itself). `daemon-dev.js` sets it too — the dev
daemon is started as `node scripts/dev.js` and never passes through the wrapper, which
is why this bit there first. `getShellContextEnvVars` passes it to every shell
subprocess, beside the session and project-dir vars that are already handed down for
the same reason. The skill's 23 command sites now read `"${QWEN_CODE_CLI:-qwen}"
review …`; the fallback keeps hosts that do not export it on the old behaviour.
PATH was the other candidate and was rejected: prepending a shim dir means writing an
executable at spawn time and overriding `PATH` in an env that
`normalizePathEnvForWindows` has already normalised — a `Path`/`PATH` collision on
Windows in exchange for saving one variable.
The env var is isolated in `shellContextEnv.test.ts` the way the session id already
is: the CLI now exports it to every shell it spawns, so `npm test` run from inside a
qwen session inherits it, and the exact-equality assertion would have failed on a
variable the test never set. Verified by running the suite with it set.
* fix(review): point the dev daemon's CLI at the source it is running, not dist
Verifying the previous commit on a real `npm run dev:daemon` caught it doing a
smaller version of the bug it fixes. The daemon runs the TypeScript **source**
through tsx; `cli-entry.js` runs `dist/cli.js`. Pointing QWEN_CODE_CLI there traded
"the subprocess is a whole major version behind" for "the subprocess is however
stale the last build was" — measured on the box that reported this, dist was **105
source files** behind the daemon. Same bug, smaller hat.
`scripts/dev.js` is the entry that runs what the daemon itself runs, so the dev
daemon points there. It gains a shebang and the exec bit, which is what lets a
caller invoke it as `"${QWEN_CODE_CLI}" review …` without knowing it needs node —
the same shape `cli-entry.js` already has for the published path.
Verified end to end on a headless box: started the dev daemon, read
/proc/<pid>/environ (QWEN_CODE_CLI=<repo>/scripts/dev.js, -rwxr-xr-x), and ran the
command that started this whole thread. Before: `Missing required argument: chunk`.
After: `agent-prompt: --role 0 needs a plan with prNumber and ownerRepo` — the role
is understood, and the complaint is about the fixture, which is the correct answer.
* fix(review): say what a missing brief proves, once, to the reader who can act on it
A role with no recorded prompt proves one thing: the brief never reached an
agent. The roster check claimed more than that — "no prompt was built for it
(`agent-prompt --role 0` never ran)" — and on #7012 it said that about all
twelve dimensions of a review that had just posted two Criticals with line
numbers. The agents were in the same comment the gate was calling empty.
Both failures are real and neither is the other. An orchestrator that writes the
launch by hand gets an agent that runs, reads the diff and finds things, having
never seen the severity bar, the finding format or this project's rules — all of
which live in the brief it was never given. That is worth blocking on. It is not
"nobody looked", and a check may not report the reading it cannot see.
Three changes, one shape:
- The per-role text says the brief never reached an agent, and that the
dimension was reviewed "if at all" from a prompt the run wrote for itself.
It no longer speaks for the agent's existence.
- Every role briefless collapses to one line. It is one failure — the run did
not use the prompt builder — and saying it twelve times buries the fact that
explains all twelve.
- The public body drops the internal command. `agent-prompt --role 2` is not
something a PR author can run; on #7012 fourteen lines of it were the whole
CHANGES_REQUESTED while the findings sat inline below the fold. The call
survives in check-coverage's stderr, where the orchestrator reads it, and the
role number is already in each label.
check-coverage no longer leads with a count: the collapsed line covers the whole
roster, so "1 required brief" would undercount it by the size of the review.
Behaviour is unchanged — the gate fires on exactly the same runs and still caps
the verdict. Only the sentence changes, and only where it was overclaiming or
talking to the wrong reader.
* fix(review): name the directory the missing briefs were missing from
"The prompt builder never ran" and "the prompt builder ran against a different
--plan" arrive at this check as the same thing — an absent file — and they are
fixed differently. Nothing in the error told them apart.
The record directory hangs off the plan path as given, so a relative --plan
resolves against the caller's cwd, and the skill runs Steps 2-6 from inside the
worktree it just created. Two cwds, one relative path, two directories. Proven
locally: the same `--plan .qwen/tmp/p.json` from a repo root and from a worktree
under it yields two record dirs.
That is not a reason to resolve the path differently — resolving a relative path
against the cwd is what a relative path means, and the mismatch mostly fails
loudly, because the plan is not in the worktree either and the read errors. It
is a reason to print where it looked. One line, on stderr, where the
orchestrator reads it; the PR author gets no path to a temp directory.
* feat(review): build the whole roster in one call, because compliance decays per call
The launch prompts are already small — a role line, the brief pointer, the diff
reads — and it did not save the run that stopped building them. Dogfooded on one
PR, the same environment went from a clean review to "no prompt was built for
any of twelve roles" over three reviews in a day. The per-agent form asks the
orchestrator for ~30 build-then-launch round trips on a large review, and that
is a compliance cost paid per agent, per review, forever; what decays under
repetition eventually decayed.
`agent-prompt --roster` builds every prompt the plan requires — chunk agents,
dimension agents, invariants — in one call: one labelled block per agent, each
recorded under the key `check-coverage` will look it up by. The list is
`requiredAgents(plan)`, the same list the coverage gate reads, so what gets
built is exactly what gets checked; a key the two derive differently is refused
at build time rather than surfacing later as "brief never reached an agent" on
a compliant run.
The blocks are separated by lines that are visibly not prompt text, and a block
copied lazily — separator included — still passes the add-only delivery check.
That is load-bearing: if honest-but-sloppy copying read as a rewrite, the gate
would punish exactly the behaviour this call exists to buy.
The per-agent forms stay, for rebuilding a single prompt after Step 3D names a
gap. Step 4/5 verify and reverse-audit are untouched: they are built per round,
with the findings folded in.
SKILL.md's Step 3A and 3B now ask for the roster once instead of one call per
agent, and check-coverage's missing-brief error names the one-call fix first.
* fix(review): close the review's five consistency gaps in the CLI-pinning story
Review feedback on this PR found five places where the fix stopped short of its
own thesis. All five, addressed:
1. Four copyable SKILL.md commands had missed the QWEN_CODE_CLI sweep —
`pr-context` (lightweight mode), `cleanup` (cache hit), `capture-local
--file` (file-path reviews), `agent-prompt --whole-diff` (Agent 8). On a
skewed host those modes died exactly the way the motivating run did. All
four now carry the prefix; the remaining bare mentions are prose.
2. check-coverage's own stderr recommended recovery with a bare `qwen` — the
message is the interface the orchestrator acts on, and on a skewed host the
recommended recovery reproduced the skew. All four recommendation sites now
print the prefixed form, and the rebuild hint covers `--chunk <id>`, which a
missing chunk agent needs and `--role` cannot express.
3. Ambient inheritance could silently re-point an entry at another session's
CLI. A dev daemon started from inside another qwen session's shell — the
usual dogfooding flow — inherited that session's QWEN_CODE_CLI through
`??`/`||=` and called the OUTER build: the same skew, one level up, and
silent. Every entry now stamps itself unconditionally; nested sessions each
call their own build. The `||=` comment in cli-entry.js also claimed a
relaunch hazard that does not exist (the relaunch child runs dist/cli.js and
never re-executes the wrapper) — the comment now states the real reason.
4. The third dogfooding entry point was still unpinned: `npm run dev` and
`npm start` published nothing, so a /review from a plain dev TUI fell back
to PATH. `scripts/dev.js` now stamps the variable in the env it spawns with
— which also covers the daemon, since daemon-dev launches serve through it,
and the daemon's own deferring copy is gone (one writer, not two).
`scripts/start.js` does the same and gains the shebang and exec bit that
make it callable as the entry it now names.
5. `"${VAR:-fallback}"` is POSIX parameter expansion, which cmd.exe passes
through literally and PowerShell rejects. The skill was already POSIX-bound
(Step 0 pipes through `tee`); the requirement is now total, and SKILL.md
says so where the variable is introduced: on Windows, run the review from
git-bash.
The unconditional stamp is pinned by a test that inherits a foreign
QWEN_CODE_CLI and asserts the spawned child gets this checkout's dev.js;
flipping the assignment back to `??` turns exactly that test red.
* fix(review): finish the two-register split, and pin the last unpinned entry
Round-2 review feedback: five more places where this PR's own rules were not
yet applied to itself.
The Agent 7 brief handed its subagent a bare `qwen`. Its two fenced command
blocks (`build-test`, `test-efficacy`) are the one call site where a SUBAGENT
shells out to the review CLI — reachable by neither the SKILL.md sweep nor the
stderr hints. Its shell gets QWEN_CODE_CLI exactly as the orchestrator's does,
so the standard prefix works verbatim; without it, an old PATH global likely
lacks these subcommands entirely, wedging the agent between its mandate (no
hand-run builds) and a command that does not exist. A test now rejects any
line-initial bare `qwen review` in that brief.
The Step 4/5 gap texts and the blind-agent line carried remediation commands
into the posted body — the register §4 stripped from missingRoles, surviving
in the sibling paths, and partly ADDED by this PR (the rewritten texts). Each
gap is now two sentences for two readers: `gap` (author-facing, no internal
commands, rendered under `Not reviewed:`) and `fix` (orchestrator-facing,
printed by compose-review to stderr as `FIX:` lines, carried on the result as
`remediation`). The four-shape precision is intact — it moved channels, not
content — and tests pin both directions: the body may not contain
`agent-prompt`/`--findings`, and the remediation must.
Pinning start.js exposed a stdout contamination: check-build-status.js printed
"Checking build status..." to stdout ahead of every child, and start.js is now
an entry whose stdout callers consume — `review parse-args --stdin | tee`
would write a plan file whose first line is not JSON. The checker's status
lines go to stderr with its warnings; `./scripts/start.js --version` now emits
the version alone.
Also from review: the all-briefless hint no longer points at role labels the
collapsed line does not carry, and start.js's stamp gets the same test dev.js
has — inherit a foreign QWEN_CODE_CLI, assert the spawned child gets this
checkout's entry.
* fix(review): isolate the env var this PR exports, and give every gap its FIX
Two findings from the bot review of the previous commit.
The shellContextEnv suite isolated QWEN_CODE_SESSION_ID and QWEN_CODE_CLI but
not QWEN_CODE_PROJECT_DIR — the third variable the CLI exports to every shell,
and the one this suite's own per-session tests assign without cleanup.
Reproduced: run the suite with it set, as any `npm test` from inside a qwen
session does, and exactly the two `.toEqual()` exact-match tests fail on a key
the test never set. Same isolation, same shape, and it retires the in-file
leak too.
The remediation channel covered blind agents and the Step 4/5 gaps and stopped
there: missing briefs, rewritten launches, unread briefs and never-opened
diffs still reached the body with no FIX line beside them. A body disclosure
with no repair command is how #7012's orchestrator got to "the agents clearly
did their job" — the whole reason the channel exists. Each category now pushes
one remediation line (missing briefs point at `--roster`; the relaunch-shaped
ones say relaunch with the same printed prompt), and a test pins the pair for
a roster gap: the body says "brief never reached an agent" with no command in
it, and the remediation names the roster call.
* fix(review): retire the last two overclaims the round-3 review found
Two sentences, same class, both this branch's own thesis applied to itself.
A chunk agent that ran on a hand-written prompt while its chunk was never
built landed in the body as "no prompt was built for it (`agent-prompt` never
ran for this chunk)" — an internal command on the author-facing surface, one
line per chunk on a 3B replay of the #7012 shape. The label now says what
happened in the author's register (ran on a prompt the run wrote itself; the
brief never reached it); the rebuild command already rides the
rewritten-launches remediation line on stderr.
And the Step 4/5 `not-built` texts still said "no auditor ran" / "no verifier
ran" — the one residual of the overclaim this branch exists to retire.
`not-built` is decided before the transcripts are consulted: a run that
skipped the builder and hand-wrote the launch leaves no brief on disk whose
open could be looked for, so such an auditor is invisible to the check, and
"no auditor ran" claims sight it does not have. Both texts now use the roster
wording: what a missing record proves (no agent was launched with a prompt
this skill builds), then what it costs ("ran, if at all, without the method
its brief carries"). The Delivery docstring records why.
Tests pin the new sentences positively and negatively; the register pin
(no `agent-prompt`/`--chunk` in a body label) guards the first one.
* test(review): make the every-gap-has-a-FIX claim true, and pin the partial stderr shape
Round-5 review caught a test whose title outran its body: "every coverage gap
… has a FIX" exercised only the missing-roles path, so dropping the
remediation push for unread briefs — or rewritten launches, or never-opened
diffs — failed nothing. That is the exact disclosure-without-repair state the
channel exists to prevent, asserted by a test that could not see it.
The title now claims what the test covers, and a sibling test covers the rest:
one plan, three defects — a chunk agent on a hand-written prompt, one that
never opened its brief, one that never opened the diff — asserting each
category's FIX line and that none of the three drags a command into the body.
Between the blind-agent test, the missing-roles test and this one, every
category that discloses is now asserted to repair; mutation-checked by
deleting each push in turn, one red test each.
Also from the review: the missing-briefs stderr had handler coverage only for
the all-briefless collapse. The partial shape — one role missing, the rest
briefed — reached stderr through no test, so a formatting regression there
(a broken join, a lost --roster hint, a garbled Looked-in path) would ship
unseen. A second handler test pins it: the per-role detail, the rebuild
hints, and the record-dir line, with the collapse text asserted absent.
* fix(review): close the round-5 findings — entry contracts, gap reach, repair loops
A GPT-5 review pass filed twenty-eight findings against this branch. Nineteen
were real and are fixed here; two were refuted with evidence (the scripts test
suite IS in CI: `test:ci` runs `npm run test:scripts`); the rest are recorded
follow-ups of documented floor designs.
Entry contracts. The standalone package launches through a shim that carries
the bundled Node and announces itself via QWEN_CODE_LAUNCHER_PATH — stamping
cli-entry.js there handed subprocesses a `#!/usr/bin/env node` script on hosts
that may have no system Node; the shim is now preferred, with a test. The
variable also predates this branch with a second meaning: desktop tooling sets
it to a vendored dist/cli.js — a module path, no shebang — which a POSIX shell
would run as a shell script; getShellContextEnvVars now drops a shebang-less
script (and only a script: a native binary needs none), restoring the bare
`qwen` fallback for those hosts. And both dev launchers read a signal-killed
child (`code === null`) as exit 0 — a killed gate command reported green; both
now re-raise the signal, with close(null, 'SIGKILL') regressions. The
production entry's stamp gets the test only the dev entries had.
Gap reach. `not-launched` said the pass "did not run" — but a hand-written
launch that never opened the brief lands in that shape too, so it now uses the
certification language the other shapes got. The roster check judged only the
FIRST transcript matching a built prompt, so a failed attempt masked the
compliant relaunch that the remediation itself prescribes — all matches are
consulted now. An agent flagged rewritten is no longer also flagged unopened
(contradictory repairs for one agent), and the all-briefless collapse no
longer coexists with one "none was built" line per chunk transcript.
Repair loops. Every rebuild command the run prints is now executable as
written — plan, selector, and `--rules` included, because a rebuild without
the rules file writes a rules-free brief that every delivery check still
passes; the verify variant stops inviting the empty findings file that is only
legitimate for a reverse-audit round. check-coverage prints exact selectors
beside the human labels. Idle agents and unread chunks get FIX lines too, and
a handler test pins the boundary: every FIX on stderr, before the verdict,
never in the JSON. SKILL.md Step 6 now says what FIX lines are for: one
bounded repair round, recompose, then the cap stands.
Roster integrity. The output is self-checking against the 30 000-character
shell truncation the skill itself documents — numbered blocks, an
end-of-roster line, and SKILL.md redirects it to a file read back paged. A
PR-controlled filename can no longer forge a block boundary: control
characters flatten to spaces in the label and the launch prompt, and a test
pins the separator count.
The jq interpolation in the report template is gone — the verdict line is
copied from Step 6's output, not recomputed by a binary the host may not have.
The findings read-error no longer advises omitting a flag another guard
requires.
* fix(review): filter by overwriting, not omitting — the spread carries what the record drops
The shebang filter fixed the wrong layer. It omitted QWEN_CODE_CLI from the
record getShellContextEnvVars returns — but every spawn site composes the
child env as `{...process.env, ...vars}`, so a key omitted from the additive
record arrives anyway, inherited through the spread. On exactly the hosts the
filter was written for (desktop tooling setting the variable to a shebang-less
vendored dist/cli.js), the value leaked through and every
`"${QWEN_CODE_CLI:-qwen}"` in the skill died on exit 126 — where before this
branch those hosts ran bare `qwen` and worked.
The fix is the pattern this same function already documents for the
agent/prompt IDs: write an EMPTY string, which overwrites the inherited value
through the spread, and which the consumer's `:-` expansion treats exactly
like unset. The test comment that justified omission — "an empty string would
shadow the fallback" — was true only of the colon-less `${VAR-qwen}` form and
is corrected where it stood, so the reasoning that produced the bug does not
outlive it.
The tests now assert on the channel the bug lived in: composing
`{...process.env, ...getShellContextEnvVars()}` and reading the child env —
for the shebang-less case, the unreadable-path case, and the pass-through
case. Reverting the overwrite to an omission turns exactly the two filter
tests red. Verified end-to-end: with the desktop shape in the parent env, a
child shell resolves `"${QWEN_CODE_CLI:-qwen}"` to the PATH `qwen` again.
Also from the same review: the two adjacent `missingReceipts` blocks in
compose-review are one block now (disclosure and repair cannot drift apart),
and the `Exact selectors:` line says a rebuild of an already-built role is
idempotent, so the over-prescription cannot make an operator hesitate.
* fix(review): reunite roleLabel with the doc comment the selectorOf insertion orphaned
The insertion left roleLabel's one-line JSDoc stranded above selectorOf,
stacked on top of the new function's own — a maintainer chasing a wrong-label
bug would have edited the rebuild-flags function. Each doc sits on its
function again.
* fix(review): close the round-9 findings — convergence, injectivity, and the claims a record can carry
Fourteen findings from a GPT-5 review of the previous head; twelve fixed here,
one was already fixed in the commit the review missed, one re-recorded as the
standing roster-design follow-up.
Repair loops now converge. Coverage accumulated every historical failed
transcript, so the relaunch its own FIX line prescribes ADDED a transcript
while the failed one kept its flag — ok stayed false, the same FIX printed
forever. A failed attempt is now superseded by a compliant attempt at the same
target (same chunk served verbatim with the diff opened; same built prompt
delivered to an agent that opened its brief), and a rewritten agent is not
also told to relaunch the prompt that was the defect.
One transcript, one credit. Pasting the whole roster output to a single agent
produced one transcript that verbatim-contains every block, matched every
requirement independently, and certified an N-agent fan-out with one reader
(reproduced upstream: roster 8, agents 1, ok true). Requirements now claim
distinct transcripts; the paste-all run fails with a sentence that names the
mistake.
Records claim only what they prove. "Its brief never reached an agent" said
more than a missing record can see (the builder may have run against another
--plan spelling); it now reads "no record shows its brief reaching an agent".
The rewritten texts claimed the brief's method never arrived — but that shape
is DETECTED by the brief being opened; they now state exactly that, and that
the launch was not the built one. A zero-byte record (a torn write) no longer
counts as built anywhere: one predicate serves the collapse, the roster loop
and the chunk lookup.
Entries the shell can actually run. The shebang filter now also requires the
execute bit (a 0644 script passes the header check and dies on EACCES), and
cli-entry consumes QWEN_CODE_LAUNCHER_PATH at stamp time — the serve/mcp fast
path never reached the branch that deleted it, so a standalone daemon leaked
the outer shim into every child, where a different checkout would republish it
as its own entry.
Inputs a PR cannot weaponize, commands an operator can run. The invariant
brief interpolated the raw PR-controlled filename into the file the agent is
told is the whole of its instructions — display sinks now flatten control
characters and the functional read argument is JSON-quoted. Agent 7 no longer
receives the review rules its own workflow forbids it (SKILL.md: deterministic
commands, not code review). The verifier refuses an empty findings file — a
vacuous pass that cleared the delivery floor while ruling on nothing — while
the early reverse-audit round keeps it. FIX lines carry the run's real plan
path instead of a `<plan>` placeholder that pastes as a shell redirection, the
roster truncation hint names --file and --rules, and composed.json persists
the exact verdictLine so the archived report copies rather than reconstructs
it — event and cappedBy alone cannot express a presubmit downgrade.
Every new behaviour is pinned: convergence, paste-all refusal, and the
zero-byte collapse are mutation-checked (disabling each turns exactly its test
red); the exec-bit, brief-injection, launcher-consumption and verdictLine
contracts each carry a direct test. 1 236 tests across the affected suites.
* fix(review): close the three paths the round-11 review found still open
The Step 4/5 FIX lines still carried a literal `--plan <plan>`. Round 9
substituted the real path into compose-review's own remediation strings and
check-coverage's hints, and left the one builder both Step 4/5 gaps flow
through — `rebuildFix` — untouched: its output reached stderr through
verificationGaps with the placeholder intact, and a literal `<plan>` pasted
into a POSIX shell parses as input redirection, so the one repair round Step 6
prescribes could never run there. The push sites now substitute the plan path
verificationGaps was handed, and the test that pins the fix text asserts no
literal `<plan>` survives anywhere in the remediation.
A lightweight cross-repo review can now be REQUIRED to run Agent 0. plan-diff
takes `--pr <n> --repo <owner/repo>` — passed only after pr-context succeeds,
so the pair's presence doubles as the context-availability signal — and
writes the identity into the plan; the roster requires role 0 wherever the
full identity is present, not only in worktree mode (fetch-pr always writes
both fields, so PR-worktree behavior is unchanged). Half an identity is
refused: a roster demanding an agent nobody can brief would wedge the run.
SKILL.md's lightweight capture block carries the flags and the
when-not-to-pass-them rule.
And the path-inertness boundary is one function with a wider net: `inertPath`
now flattens every control character (a terminal escape in a filename must
not reach a terminal), the separator glyph, and the backtick — which could
close the Markdown code span the path is rendered inside and let the tail of
a PR-controlled filename run as markup in the brief the agent treats as
authoritative. The roster label and launch-prompt sites that had their own
narrower regexes now share it. The injection test's hostile filename gained a
backtick and an ESC sequence, and asserts the rendered heading carries
exactly the span's own backtick pair and no control bytes, while the
JSON-quoted functional read argument still round-trips the raw path.
Each fix is mutation-checked: reverting the substitution, re-gating the
roster on worktree mode, and narrowing inertPath each turn exactly one test
red.
* fix(review): bind the receipt to what was delivered, and match what actually assigns
Three review-integrity holes from the round-12 review, each with a
reproduction, each fixed at the layer the reproduction named.
The verify receipt could be satisfied by a partial delivery. The record was
deliberately the findings-free launch block, so one key could serve every
shard by the add-only rule — and that same rule let a caller build with a real
findings file, launch the agent with only the recorded tail, and clear the
gate while no verifier ever saw a finding. The record is now the EXACT printed
prompt, findings folded in, keyed per findings-content digest
(`verify--<sha>`, `reverse-audit--chunk-N--<sha>`); the delivery side collects
the whole key family with the documented floor of one. Tail-only delivery
matches nothing; each shard verifies against its own list; shard records no
longer share a key, so none clobbers another.
The injective roster matching was greedy, and greedy rejects valid
assignments. With transcript T1 containing blocks A+B and T2 containing only
A, first-come claiming took T1 for A and reported B missing — a compliant
repair permanently capped by transcript filename order. The claim set is now a
maximum bipartite matching (Kuhn's augmenting paths), seeded on the edges
where the transcript also opened the requirement's brief and extended over all
verbatim edges, so a requirement reports missing only when no injective
completion exists at all.
A rules-free rebuild could silently strip the brief. The launch prompt only
points at the brief, so rebuilding a rules-bearing role without --rules left
the recorded launch byte-identical while the project rules vanished from the
one file the agent treats as authoritative — every delivery check kept
passing. writeBrief now refuses the downgrade at the single choke point both
build paths pass through, with the escape hatch named (delete the record dir
to start over deliberately).
All three are mutation-checked: regressing the record to findings-free, the
matching to greedy, or disabling the downgrade guard each turns its own test
red. 704 review tests green.
* docs(review): let the docs and comments claim only what the new record design does
The round-13 review caught the drift this branch's own thesis forbids: two
SKILL.md sentences still described the findings-free record the previous
commit retired — an orchestrator reasoning from them would conclude a
findings-less delivery still matches, precisely the bypass that commit closed.
Both now state the new contract: the record is the exact printed block, keyed
per findings digest, and a launch that drops the list matches no record.
And the matching comment claimed more than Kuhn guarantees: phase-2
augmentation can displace an opened match onto an unopened edge to enlarge the
matching, so an unread flag describes the assignment, not an impossibility.
The comment now says so, and why cardinality is the right thing to maximize.
* docs(review): finish retiring the findings-free record from every sentence that described it
Round 15 found the three survivors round 13 missed — all in code, not
SKILL.md: the findingsSection docstring (all three of its clauses false since
the digest-key commit), the findings field doc ('Printed, not recorded'), and
the --findings --help text, which told an operator the exact opposite of what
the command now does. Each now states the new contract: the findings are part
of the recorded prompt, keyed per digest, and a launch that drops them matches
no record.
Also from the same review: the plan-path substitution uses a function
replacer, so a path containing $& or $` cannot be misrendered as a
replacement pattern. Practically unreachable for .qwen/tmp paths; closed
because it costs four characters.
* docs(review): the actually-last sentence describing the findings-free record
Round 16 counted one survivor of the sweep the previous commit's title
claimed complete: the acceptsFindings jsdoc in agent-briefs.ts, present-tense,
whose '(see runAgentPrompt)' pointed at a function whose own comment says the
opposite. It now states the digest-key contract like its siblings, and a
whole-tree grep for present-tense descriptions of the retired design comes
back empty.
* test(review): pin the idle and missing-chunk FIX lines to the remediation channel
Round-18 review: the two remediation pushes added for the every-gap-has-a-FIX
rule had no test of their own — deleting either failed nothing, leaving a body
disclosure whose repair could silently vanish, the exact state the channel
exists to prevent. The idle-plan test now asserts the relaunch FIX; the
blind-plan test, whose chunks nobody reads, now asserts the chunks-nobody-read
FIX beside the blind one. Both mutation-checked: deleting each push turns
exactly one test red.
* fix(review): quote the plan path in every printed repair, and test the executable shebang-less shape
Round-21 review, three items. The plan path is now single-quoted at all seven
sites that print it into a repair command — a workspace path containing a
space split the copy-pasted FIX at the space, exactly the operator moment the
lines exist for; the earlier uniformity deferral ends here, uniformly.
PlanDiffResult declares prNumber/ownerRepo so a refactor away from the
conditional spread cannot silently drop the fields the roster's Agent-0
requirement reads. And the filter gains the test its primary target deserved:
an EXECUTABLE shebang-less .js (the desktop vendored bundle shape) is rejected
by the header read itself — the existing 0644 fixture never reached that
branch, so a regression in the byte read would have passed every test.
* fix(review): shell-quote the plan path properly — an apostrophe is not rarer than a space
Round-22 review: the bare '…' wrap from the previous commit closed at an
embedded apostrophe, so ~/Documents/John's Projects broke where it had worked
unquoted — one breakage class traded for another instead of both closed. A
shared shellQuotePath (the same '\'' dance as utils/standalone-update.ts)
now serves all six repair-printing sites, and a test drives verificationGaps
from a plan under an apostrophe directory, asserting the escaped form and
rejecting the naive wrap.
* fix(review): quote the --file selector, un-dead the spawn guard, test the half-identity
Round-24/25 reviews, four small items. selectorOf now shell-quotes the --file
path — the same copy-paste contract the --plan quoting just earned, on the one
selector that carries a path. RULES_MARKER moves above writeBrief's JSDoc,
which it had been silently stealing. The check-build-status test's reject
guard was dead (execFile always delivers string stdout, so an ENOENT resolved
and the empty-stdout assertion passed on a script that never ran) — it now
rejects on spawn-level errors, which carry string codes, while non-zero exits
still resolve. And the roster's ownerRepo guard gets the independent test it
never had: a plan with prNumber but no ownerRepo requires no Agent 0, since
the brief builder cannot serve half an identity.
|
||
|
Shaojin Wen
|
8652789471
|
docs(autofix): make bot PR comments bilingual with collapsed Chinese (#7137)
Files the workflow posts verbatim as PR comments (address-summary.md, no-action.md, e2e-report.md) must now end with a complete collapsed Chinese translation (<details><summary>中文说明</summary>…), mirroring the repository's PR-body convention, so the bot's review reports and E2E reports read natively for both audiences. failure.md/handoff.md stay English-only without a details block: handoff comments embed a byte-truncated excerpt, and a severed <details> tag would swallow the rest of the rendered comment. Contract test pins the rule (bilingual instruction present, the three files named, the truncation-safety exclusion stated). 51/51. Co-authored-by: wenshao <wenshao@example.com> |
||
|
易良
|
bf12fed31c
|
fix(ci): notify silent triage re-runs (#7079)
Refs #7073 |
||
|
Shaojin Wen
|
9be2750d5b
|
ci(autofix): treat Suggestion-level review findings as actionable per AGENTS.md (#7094)
* ci(autofix): treat Suggestion-level review findings as actionable per AGENTS.md AGENTS.md's review policy: Suggestions ARE addressed during a PR's first ~5 review rounds; only past that are they deferred with a recorded reason. The autofix loop's QWEN_SUGGESTION_FILTER contradicted this by unconditionally hiding /review **[Suggestion]** inline comments from both the scan's feedback count and the agent-facing feedback rendering — at every round. Since the loop's MAX_ROUNDS cap (5) is the same boundary the policy names, every round the loop actually runs is within the address-Suggestions window: drop the filter from both sites, and align the SKILL's Optional triage with the policy (implement valuable suggestions; decline only with a recorded per-finding reason). Contract test pins the filter's absence. 50/50. * chore: retrigger CI (rerun attempts wedged in queue) --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
62ad5b3ad3
|
ci(autofix): run the schema gate from a trusted staged copy, not the branch tree (#7076)
* ci(autofix): run the schema gate from a trusted staged copy, not the branch tree Live-run failure on the first post-merge address run (PR #7072): the verify gate invokes `bash .github/scripts/check-settings-schema.sh`, but by then the working tree is the PR BRANCH (switched in "Prepare branch and feedback"), and a branch that predates the script's merge does not contain it — bash exits 127, the gate dies with no outcome, and only the always-handoff kept the loop from going silent. Every bot branch created before the script's merge hits this. Stage the script from the trusted-base checkout into ${RUNNER_TEMP} before any branch switch (both jobs, for symmetry) and invoke the staged copy in both verify gates. This also means the gate logic always comes from the trusted base, never from the branch under verification. Test asserts: staged invocation in both gates (and no working-tree invocation), exactly two staging cp lines, review-address staging ordered before the branch switch, and the structural before-no-op ordering unchanged. 49/49. * test(autofix): also assert issue-job staging precedes its branch checkout Review: the lastIndexOf ordering assertion only protected the review-address job. The issue job's agent commits can touch .github/scripts, so staging after the verify gate's checkout would copy the agent's gate instead of the trusted base's. Assert the first staging occurrence (issue job) precedes the first git checkout "${BRANCH}" (also the issue job's). --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
5a35435df1
|
ci(autofix): recover from generated-artifact CI gates and stop silent stalls (#6998)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none (push) Waiting to run
E2E Tests / E2E Test - macOS (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
* ci(autofix): teach the review loop about generated-artifact gates and stop silent stalls
The autofix bot stalled on PRs that edit settingsSchema.ts without
regenerating settings.schema.json (e.g. #6984): a CI-only freshness gate
it could neither run, see, nor recover from.
- Give the agent the tool and the instruction to regenerate: add
`npm run generate:settings-schema` to the develop-issue and address-review
coreTools allowlists, and a SKILL rule to regenerate + commit a source's
generated artifact.
- Mirror CI's "Check settings schema is up-to-date" step in both verify
gates so a stale artifact fails locally instead of red-on-CI after push.
- Inject the actual failing STEP name + a log excerpt into feedback.md so
the agent diagnoses from the real failure instead of guessing from local
test runs. SKILL now forbids "pre-existing"/environment excuses without
evidence.
- Decouple the feedback watermark from base-sync pushes: use the last eval
marker (what the agent evaluated), not the head commit date, so an
"Update branch" merge can no longer bury unaddressed maintainer feedback.
Use PR createdAt as the pre-first-eval floor.
- Bound the pending-check skip so a check wedged pending can't strand a PR
forever; always post a handoff comment + eval marker on failure so the
loop never goes silent; add an issue_comment trigger so an @-mention from
a trusted maintainer re-triggers the review pass promptly.
* ci(autofix): drop comment-trigger and raw-log injection; keep them within existing safety guards
Respects two deliberate, tested design decisions that the first cut collided
with (caught by scripts/tests/qwen-autofix-workflow.test.js):
- Drop the issue_comment @-mention trigger and its route branch: the workflow
intentionally does not expose comment-triggered autofix (only
pull_request_review:submitted) to avoid redundant runs and comment-command
surface. The scheduled scan plus the watermark fix already re-target a PR
after maintainer feedback.
- Drop the raw CI-log injection into feedback.md: feedback fed to the model is
deliberately sanitized and must not pull in URLs / raw context (a
prompt-injection surface). Keep only the sanitized check-name rendering
(.name // .workflowName, still gsub+truncated).
Update the assertions that the retained improvements (watermark decoupling,
pending-check staleness bound, always-post-handoff-on-failure) legitimately
changed. Workflow test: 48/48 green.
* ci(autofix): address review — handle cancellation, fold a PR fetch, cover schema commands
Addresses the three inline /review suggestions on the PR:
- Handoff on any non-success end, not just "failure". A 120-minute job-timeout
cancellation sets job.status = "cancelled", which the "== failure" check
missed — leaving no marker and no comment, so the next scan re-targeted the
same feedback with the same round (an invisible loop). Use "!= success";
the step only runs on failure()/cancelled()/dry-run and dry-run is excluded.
- Fold the createdAt fetch into the existing statusCheckRollup gh pr view call
(statusCheckRollup,createdAt), removing one GitHub API round-trip per PR
scanned.
- Add test coverage the earlier diff lacked: assert generate:settings-schema is
in both agent allowlists and that both verify gates run the schema-freshness
check, so a future edit can't silently drop the guard this PR adds.
* ci(autofix): fix handoff/staleness design flaws from review (4 critical + 2 suggestions)
Addresses the CHANGES_REQUESTED review of the handoff (E-4) and pending-check
staleness (E-3) logic:
- Suppress the handoff once a run published a result (OUTCOME fixed/noop), so a
later always() step failing the job (e.g. artifact upload) can no longer post
a contradictory acted=false handoff over a reported success.
- Bound the agent step at 80m, well under the 120m job timeout, so a runaway
agent fails the STEP (not the job) and the always() report step still runs and
hands off — a job-level timeout would cancel that step too and go silent.
- On a pre-prepare crash (empty NEWEST) the watermark can't advance, so write a
terminal marker (round = MAX_ROUNDS) and skip on the highest marker round
(not last-by-ts), so the scan stops re-handing-off instead of repeating until
MAX_ROUNDS.
- Raise the pending-staleness bound from 30m to 240m so an active check
(review-pr ~50m, review-address up to 120m) is never aged out mid-flight and
the same feedback double-processed; only truly-dead checks are ignored.
- Prefer the agent's detailed failure.md over the generic handoff.md wrapper.
Adds a bash-replay test that extracts the actual POST_HANDOFF decision and
MARK_ROUND logic from the workflow and exercises the state transitions
(published+late-failure, dry-run, verify failure, pre-verify crash,
cancellation; terminal vs incremental round). Workflow test: 49/49.
* ci(autofix): address review nits — symmetric schema-gate outcome, softer SKILL wording
Two non-blocking review suggestions:
- The issue-phase verify gate's schema-freshness check now writes
outcome=failed before exit 1, matching the address-review gate, so the
issue-phase step summary shows outcome=failed instead of outcome=unknown.
- Reword the SKILL rule from "do not invent environment excuses" to "do not
skip a failing check by attributing it to the environment without evidence,"
which keeps the intent (no hand-waving a real failure as an env issue)
without discouraging the agent from reporting a genuine infra failure.
* ci(autofix): close review edge cases — structural schema gate, immutable floor, robust handoff
Review round 3 (2 critical + 3 suggestions):
- Run the review gate's settings-schema freshness check BEFORE the no-op/
unchanged return, so a stale-schema PR the agent wrongly no-ops fails
(outcome=failed) instead of being reported as evaluated while CI stays red —
the exact motivating bug. Single check now covers every path; ordering is
asserted in the test.
- Never fall back to the mutable head commit date for the pre-first-eval
watermark floor: if the PR metadata query fails, use an empty (over-inclusive,
never-buries) floor. A base-sync HEAD as the floor would recreate the burial
bug. Removes the now-unused HEAD_SHA lookup.
- Guard the terminal handoff marker's timestamp (MARK_TS=${NEWEST:-${WATERMARK:-unknown}})
so a cascading API failure that blanks WATERMARK can't emit an unparseable
`ts=` that the scan regex skips, defeating the terminal-round guard.
- Truncate failure.md through `iconv -f utf-8 -t utf-8 -c` so a byte-level
head -c can't split a multi-byte sequence and corrupt the comment body.
- A pre-prepare crash (empty NEWEST) now says "could not start evaluation"
instead of "round 5/5", which would imply MAX_ROUNDS attempts were made.
Workflow test: 49/49.
* test(autofix): assert regression-catching invariants flagged in review
Four review suggestions, test-only: assert the else-branch floor
(EFF_WM=${CREATED_WM}, not the old PUSH_WM), the staleness jq filter
(.startedAt // ... // $cut), the JOB_STATUS env declaration (else it is always
empty → over-eager handoffs), and the .name // .workflowName feedback format —
so a regression on any of these is caught rather than passing silently.
* ci(autofix): fix iconv silent-abort, fold branch fetch, tighten staleness filter
Review round with a real regression in my own round-3 UTF-8 fix:
- CRITICAL: `iconv -c` exits 1 whenever it discards a byte split by `head -c`,
and under the step's `set -eo pipefail` that aborts before the eval marker +
gh pr comment run — a silent stall, the exact failure this block prevents.
Add `|| true`; the cleaned text is already emitted, so the handoff continues.
- Fold headRefName into the PR_META fetch (headRefName,statusCheckRollup,
createdAt) and derive BRANCH from it — one fewer API call per scanned PR.
- Simplify the pending-staleness clock to `.startedAt // $cut`: statusCheckRollup
has no updatedAt and pending checks have no completedAt, so those fallbacks
were dead and contradicted the comment. Now a check blocks only if it actually
started within the bound; comment matches the code.
- Tests: assert `.startedAt // $cut) > $cut` (the comparison, not just the
constant) and the `|| true` guard, so a flipped comparison or a dropped guard
is caught. 49/49.
* ci(autofix): review round — skip empty branch, hoist staleness vars, robust sentinel
Six review suggestions (no criticals):
- Skip a candidate PR when the metadata fetch fails (empty branch) instead of
falling through to an address job that fails on `git checkout -B "" origin/`
and posts a misleading handoff. This also means CREATED_WM is only reached
with populated metadata (subsumes the empty-floor warning suggestion).
- Hoist the invariant PENDING_STALE_MIN / PENDING_CUTOFF out of the per-PR loop
(one `date` fork instead of one per candidate).
- Replace the MARK_TS "unknown" sentinel with a far-future ISO-8601 date, so it
is non-empty AND sorts above real timestamps without relying on an
undocumented lexicographic quirk of a bare word.
- Cross-reference comment on the positional eval-marker regex noting it must
stay in lockstep with every write site (ts= acted= round=).
- Tests: document OUTCOME="" + JOB_STATUS=success → no handoff, and assert the
empty-branch skip guard.
* ci(autofix): DRY schema check via --check, widen handoff detail, honest terminal recovery
Three review suggestions:
- Replace both duplicated schema-freshness blocks with the generator's in-process
`--check` mode (verified: exit 1 when stale, 0 when fresh; no disk write, so
the review gate's later no-op git-diff is unaffected). Single source of truth
with CI; a future change to the check lives in one place.
- Widen the handoff DETAIL_FILE search to address-summary.md/no-action.md: when
the agent succeeds but a post-agent verify gate fails (e.g. the schema gate),
OUTCOME=failed with only the success outputs present, and "Push and report" is
skipped — so this was posting a false "crashed or timed out" and dropping the
agent's real summary.
- Correct the terminal-crash headline: the marker makes the scan skip forever
(even forced dispatch), so "re-trigger if transient" was misleading; the
headline now states the real recovery — delete the terminal autofix-eval
marker comment, then re-trigger. (Keeps the terminal design a prior review
asked for; only the advertised recovery is fixed.)
* ci(autofix): revert schema gate off --check (removed from main by #7031); jq replay
- CRITICAL: `--check` was reverted from main's generator by #7031 (
|
||
callmeYe
|
3d4601489e
|
revert: remove local PR verification gate (#7031)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none (push) Waiting to run
E2E Tests / E2E Test - macOS (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
Reverts QwenLM/qwen-code#6873 and QwenLM/qwen-code#7025. |
||
|
callmeYe
|
441006b0e1
|
feat(scripts): add local PR verification gate (#6873)
* feat(scripts): add settings schema check mode * feat(scripts): add local PR verification runner * fix(scripts): harden local PR verification * docs: document local PR verification gate * fix(scripts): isolate local verification tools * fix(scripts): scope PR formatting checks * fix(scripts): skip symlinked PR paths * fix(scripts): preserve verification gate integrity * fix(scripts): canonicalize verification temp paths * fix(scripts): stabilize local PR verification * fix(scripts): clear built-in test credentials * fix(scripts): enforce isolated test environment * fix(scripts): serialize local verification tests * fix(scripts): address PR verification review * fix(scripts): preserve review git wrapper environment * refactor(scripts): avoid step helper shadowing * fix(scripts): distinguish forwarded child signals * fix(scripts): preserve relayed signal exit codes |
||
|
易良
|
d4c15f05c5
|
feat(ci): add automated PR failure patrol (#6766)
* feat(ci): add stale failure patrol * fix(ci): harden failure patrol * refactor(ci): simplify flaky rerun patrol * docs(ci): clarify flaky patrol skill boundary * feat(ci): patrol stale PR failures * fix(ci): prefilter failed PRs * fix(ci): isolate patrol classification * fix(ci): revalidate stale patrol actions * fix(ci): verify main before branch update * fix(ci): classify all stale PR failures * fix(ci): bound patrol batches * fix(ci): persist patrol failure state * fix(ci): harden patrol state transitions * fix(ci): harden stale failure patrol * fix(ci): continue patrol after expired logs * fix(ci): tighten patrol guardrails * fix(ci): preserve failure context in patrol logs * fix(ci): harden stale patrol closeout * fix(ci): paginate patrol marker comments * fix(ci): harden patrol action guards * test(ci): cover patrol guard rails * fix(ci): harden patrol marker parsing * fix(ci): address patrol review followups * test(ci): cover patrol review edges * fix(ci): record patrol rerun marker first * fix(ci): harden patrol review edge cases * fix(ci): tighten stale failure patrol markers * refactor(ci): simplify flaky rerun patrol (2838→1258 lines) - Remove classification guards from actOnDecision (confidence check, action enum validation, boundedReason, update_branch multi-guard chain) - Move classification rules to SKILL.md prompt - Delete 40 source-code text matching tests, keep 20 behavior tests - Merge identity job into classify, remove SHA verification - Change scan sort order from oldest-first to newest-first - Remove unused functions: writeSkillInputs, failureKey, boundedReason, canAct, skillCandidate, mainRunSucceeded * fix(ci): show gh stderr in top-level error output When gh CLI returns non-zero exit, execFile rejects with an error whose .stderr contains the actual GitHub API diagnostic. Previously only one of stderr or message was shown; now both are printed. * fix(ci): address patrol review findings * fix(ci): make stale patrol actions recoverable * refactor(ci): simplify flaky rerun patrol * fix(ci): close flaky patrol review gaps * fix(ci): restore PR failure patrol actions * fix(ci): harden failure patrol scanning * fix(ci): address patrol review follow-ups * fix(ci): harden patrol parsing and coverage * fix(ci): classify failures against PR changes * fix(ci): harden patrol script input handling * fix(ci): remove unsafe auto branch update * test(ci): exercise patrol action limit * fix(ci): bind patrol actions to current evidence * fix(ci): count patrol actions per PR * fix(ci): redact quoted secret labels --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
|
易良
|
d7e2892a7c
|
fix(cli): avoid updating active CLI processes (#6874)
* fix(cli): avoid updating active processes * fix(cli): close update relaunch gaps * test(cli): fix standalone update source path * fix(cli): reset deferred update per relaunch |
||
|
易良
|
13d735bd71
|
ci(release): finalize stable releases asynchronously (#6868)
* ci(release): finalize stable releases asynchronously * chore: remove redundant release workflow docs * ci(release): skip prerelease finalization * fix(ci): block release PR when changelog fails * fix(ci): reject invalid release finalization tag * fix(ci): restrict release finalization token |
||
|
易良
|
6692289455
|
fix(ci): skip empty SDK release PR (#6861)
* fix(ci): skip empty SDK release PR * fix(ci): skip no-op sdk release branch push * fix(ci): target sdk release from ref on no-op * fix(ci): guard sdk release tagging |
||
|
易良
|
344c006122
|
fix(ci): avoid apt on self-hosted Playwright smoke (#6865)
* fix(ci): isolate web-shell Playwright smoke runner * fix(ci): avoid apt on ECS Playwright smoke * fix(ci): enable manual ecs smoke validation * fix(ci): repair manual runner input parsing * docs(ci): clarify self-hosted playwright deps * test(ci): fix Playwright runner assertion |
||
|
易良
|
ae9e7782d4
|
feat(release): generate AI-assisted release notes (#6756)
* feat(release): generate AI-assisted release notes * docs(release): remove AI release notes planning docs * fix(release): preserve complete AI release notes * refactor(release): trim AI notes generator surface * ci: Open autofix issues for main CI failures * fix(release): harden AI notes review feedback * fix(release): close AI notes review gaps |
||
Nothing Chan
|
b19ebd8fc6
|
fix(packaging): bundle clipboard addon in standalone builds (#6708) | ||
|
易良
|
55eb8325e5
|
fix(tests): sync qwen-resolve-workflow test expectations with PR #6706 timeout changes (#6720)
PR #6706 updated review-pr workflow timeout values (job timeout to 260m, default to 180m, max to 240m) but did not update the corresponding test expectations, causing CI failures on all subsequent PRs. |
||
qwen-code-dev-bot
|
bcf5b7bfdd
|
fix(release): raise prepared package size limit to 96 MB (#6687) (#6691)
The assertPreparedPackageSize check introduced after v0.19.8 set an 80 MB ceiling, but the current package is 80.58 MB — 597 KB over — causing the Docker sandbox build to fail during the v0.19.9 release. Bump to 96 MB to accommodate normal growth with ample headroom. Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
han
|
a130ce430e
|
fix(cli): localize approval mode UI labels (#6592)
* fix(cli): localize approval mode UI labels * fix(cli): address approval mode i18n review * fix(cli): stabilize approval mode i18n key * test(cli): cover approval mode i18n follow-up * test(cli): cover localized auto indicator * test(cli): address approval i18n suggestions --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
qqqys
|
ebd83f1d2d
|
fix(release): raise package size budget to 85 MiB (#6688) | ||
|
易良
|
3bf2d45403
|
ci: add suspicious comment attachment guard (#6599)
* ci: add suspicious comment attachment guard Resolves #6597 * ci: reduce attachment guard false positives * ci: harden comment attachment guard * ci: tighten attachment extension matching * ci: avoid false attachment removal summary * ci: avoid markdown link attachment false positives * ci: avoid country-code attachment false positives * ci: harden attachment URL parsing * ci: reduce attachment guard false positives Updates #6597 * ci: harden malformed attachment URLs Updates #6597 * ci: reduce attachment guard overmatching Updates #6597 * ci: scan attachment path segments Updates #6597 * ci: cover review attachment summaries * ci: harden attachment link detection * fix: address critical bypass vectors in comment attachment guard - Remove break in decodeTarget catch block so malformed percent sequences (e.g., %ZZ) don't prematurely exit the decode loop, allowing double-encoded extensions to be fully detected - Strip zero-width characters (U+200B-U+200D, U+FEFF, U+00AD, U+2060, U+180E) before NFKC normalization to prevent invisible-character evasion of extension matching - Add protocol-relative URL (//) support to linkPattern and highRiskTarget, normalizing to https: before parsing - Add tests for all three bypass vectors (39 total, all passing) --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
|
易良
|
6d966d2917
|
test(core): stabilize file history eviction test (#6637)
* test(core): stabilize file history eviction test * ci: clean package build artifacts before fast-path check * ci: preserve web build outputs during fast-path check |