mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-01 20:34:36 +00:00
222 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Shaojin Wen
|
46a16de5c6
|
fix(autofix): state the primary agent budget and use the step's headroom (#8257)
* fix(autofix): budget the whole round, not just the agent step The primary attempt took run-agent.mjs's 50-minute default while its step capped at 80, so a third of that step was unreachable and every "ran out of time" round ended for a reason this file never named. Raising it surfaced the larger problem: the budget was never checked as a whole. Measured on one scan (run 30646547838): setup 5-7m in earlier steps, so it never competes with the agent step; Triage and address 50m03s on #8005 round 9 (its own timer) and 12m45s on #8211; the Verification gate 22m48s on #8211 — the largest consumer in the job, and unbounded; push, report and finalize 3-4s. The old arithmetic ("80 + 20 leaves ~50 for setup, two verification passes, and reporting") predates that measurement: the real worst case was 7 + 80 + 23 + 20 + 23 = 153 against a 150-minute job, and a JOB timeout cancels the always() reporters, which is the silent round the design exists to prevent. Sized for the PRs that actually exhaust the budget (47 and 35 changed files): agent 120m under a 130m step cap, each verification gate bounded at 60m — 2.6x the measured pass, and a graceful degrade because both gates already carry continue-on-error, so a bound turns a job kill into the ordinary verification-failure path that reports. Worst case 7 + 130 + 60 + 20 + 60 + 3 = 280 against a 300-minute job, itself under the 360-minute ceiling ubuntu-latest imposes regardless. Raising the budget does not make a doomed round cheaper — each exhausted budget still pushes nothing, which is why TIMEOUT_WINDOW_CAP stops a PR after three in one window. It buys rounds that were close to finishing. The test now asserts the SUM against the job cap and the count of bounded steps, because asserting the numbers individually is exactly what let 153-against-150 pass. Mutations checked: budget over its step cap, either verification bound removed, job cap back to 150, job cap over the runner ceiling, and continue-on-error dropped are each caught. * fix(autofix): bound every long step and cap the timeout override (#8257) * fix(autofix): enforce the timeout ceiling and trim review feedback (#8257) * fix(autofix): force base-10 clamp and align stale bound with job cap (#8257) * fix(autofix): close the clamp int64 escape and pin it with a bash replay (#8257) * fix(autofix): give the timeout clamp a floor, not only a ceiling The review's Finding 1: the guard clamped only the ceiling, and the uncovered side is the likelier typo. Every comment in this file, the PR body and the operator message speak in MINUTES; this one variable wants MILLISECONDS. A maintainer told to "raise the agent time budget" who sets QWEN_AUTOFIX_TIMEOUT_MS=120 armed a 120 ms timer — every round SIGTERMs instantly, writes agent-timeout, and reports "ran out of time (timeout (120ms))" until TIMEOUT_WINDOW_CAP trips and AutoFix stops on the PR, advising the human to raise the budget they just raised. No warning anywhere in that loop, which is the exact misreport the clamp exists to prevent, reached from the other direction. A 60000 ms floor rejects every minutes-shaped value, and it also closes the `0`/`000` hole the review noted alongside it — those passed the bare regex while the message asserted the value had to be positive. The message now names the units, because a units confusion is the whole failure mode. Replayed the review's own table against the extracted block, stdout and stderr separated: 7200000, 3600000 and the floor itself pass untouched; 120, 60, 0, 000 and 59999 all clamp with a warning, alongside the over-cap, malformed, octal and int64 cases the previous round closed. The test pins both boundaries from each side (59999 clamps, 60001 does not) and asserts the warning names MILLISECONDS. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Autofix <qwen-autofix@users.noreply.github.com> |
||
|
Shaojin Wen
|
cd88149145
|
feat(verify): check the accepting end, and separate empty from unmeasured (#8295)
Three rules from maintainer rounds on #8132 and #8261, chosen over the larger set those rounds produced because this file is already 2.4x its size six days ago and nothing gates its growth. A capability has two ends. #8132 found a cookie-to-Authorization bridge gated to the desktop shell where it was minted and mounted unconditionally where it was accepted, so every server treated that cookie as a bearer. The tell generalises: the tests were named after the gated end, which is what made the ungated end look covered. "Nothing found" and "could not measure" must be different values, and the damage is done by the consumer rather than the flag. #8261 set `emptyDiff` both for a genuinely empty PR and for a diff capture that FAILED, and the skill answered that flag by recommending the PR be closed as superseded — a transient fetch error closing live work. The verdict contract already applies this rule to our own report; the code under test gets it too. A validity control must run before the artifact it invalidates is built. #8261's re-classifier demoted findings from a dead harness after the findings list was assembled, so a harness proven dead still filed `mutant-survived` against the author. A control that runs late is not a weaker control, it is not a control. The review also asked for two overlapping pairs to be consolidated. One resolved itself when the observability-ranking bullet moved next to the concurrency rule it cites. The other I deliberately left, with a note saying why: merging the type-boundary bullet into the sibling-sweep bullet would edit the one rule in this file with a measured before/after behind it, and that text is byte-identical to what the treatment arm read (verified against the staged copy). Editing the instrument is a change to make with a fresh measurement, not on the way past. Each new assertion was mutation-checked: neutering any of the three pinned phrases turns the suite red. Co-authored-by: verify <verify@local> |
||
|
Shaojin Wen
|
ce8eb830d7
|
fix(triage): say what the re-run summary measured, in both languages (#8273)
* fix(triage): say what the re-run summary measured, in both languages The re-run summary told maintainers the bot had "no review of its own" whenever it had not VOTED. On #7948 and #8141 the bot had reviewed and deliberately deferred, leaving a COMMENTED review visible on the page — so the comment said the opposite of what the reader could see. It was also the one composed body in this workflow that shipped English only, sitting directly under the bot's own bilingual stage-3 review. Classify the head-commit state three ways instead of two and name what is actually there. A COMMENTED-only review now reads as "carries no vote" and lists both things that produce it: the skill deferring on purpose at 3/5 (a fork `refactor` on the approval guardrail, or a core change escalated for maintainer awareness) or an approval a push dismissed. The stage-3 comment says which; the reviews API cannot. That indistinguishability is why the warning still fires on both. My first pass silenced the defer case as routine, which would have muted the guard for the incident the existing test encodes — a dismissed approval leaving only a COMMENTED behind. Either way the PR is one approval short with the bot not supplying it, so the operator signal stays and only the wording differs. DISMISSED and PENDING are explicitly not deferrals: a push voids the bot's approval, and that is precisely when a fresh one is required. Verified by running the extracted step under its own `set -euo pipefail` with a stubbed gh: the real review payloads from #7948 and #8141 both classify as deferred; ten synthetic arms separate own / deferred / none, including human-approval-only and a vote on an older commit, which must stay `none`. The composed body was rendered through GitHub's own `POST /markdown` — 8 code spans, 6 bold runs, 2 links, no live mentions, Chinese intact. * fix(triage): address review feedback on re-run summary tests (#8273) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
bc382c3ff9
|
feat(verify): sweep sibling shapes, calibrate replays, measure suggested fixes (#8242)
* feat(verify): sweep sibling shapes, calibrate replays, measure suggested fixes
The verify-pr skill produced a `merge-ready` report on a head that leaks
live HTML. Measured on #8147 at
|
||
|
Shaojin Wen
|
0b36e597d4
|
fix(triage): render the verify report as sanitized markdown, not an escaped pre dump (#8147)
* fix(triage): render the verify report as sanitized markdown, not an escaped pre dump The sandboxed-verification comment embedded report.md inside <details><pre><code> with full HTML escaping. Safe, but unreadable: the report is a curated bilingual document — tables, headings, nested <details> folds — and it displayed as a wall of raw markdown source (#8140's verify comment was the exhibit: literal asterisks, table pipes, and <details> tags shown as text). report.md now renders as markdown through emit_report, which holds the same security floor with four line-independent guarantees: every & < > is escaped and only the structural tags the report legitimately uses (details/summary/pre/code/br) are un-escaped back to live tags, so no other tag can form; the comment-open token is broken (the autofix-proven neutralizer), so no forged qwen-triage:* marker can appear in the raw body the upsert logic greps; @ becomes @, which renders identically but can never fire a mention; and unbalanced <details> opens are counted and closed, so a malformed report cannot swallow the footer. An oversized report falls back to the escaped-pre embedding wholesale (truncated markdown dangles fences and folds), as does any sanitizer failure. The tmux lane's raw-log embedding is untouched — escaped pre remains right for logs. The zero-match grep in the fold balancer carries || true: under the step's pipefail, a report with no folds would otherwise kill the whole composer. Tests: a behavioral replay drives the real emit_report — structure survives (tables, folds, no pre/code), the security floor holds (no live marker/mention/tag, entities escaped, folds balanced), and the oversize fallback produces the escaped shape; the full-render ordering pin follows the new heading. * fix(triage): cap sanitized report size and use portable ERE sed (#8147) * fix(triage): budget fold-closer overhead against the report size cap (#8147) * fix(triage): annotate emit_report fallbacks with distinct warnings (#8147) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(triage): sanitize the verify report code-region-aware and re-collapse it (#8147) Replace the unconditional sed escaping in emit_report with a node sanitizer that tells code regions apart from prose. CommonMark does not decode entities in code spans/fences, so escaping & < > @ there showed && / <T> / @pkg in the commands, types, and paths a report is read to copy. Prose is still escaped (< only; & and > are not security controls and mangling them killed && and blockquotes), code is left inert, the <!-- break stays global so no forged marker survives in the raw body the upsert greps, and folds are balanced over prose only so a fenced </details> can no longer defeat guarantee 4 (surplus closers dropped, unclosed opens closed). Wrap the rendered report in a collapsed <details> so it costs one line again instead of expanding up to 45 KB inline, narrow the tag allowlist to details/summary, bound the whole wrapped section against the size cap, and make every fallback label say "truncated". * fix(triage): close dangling code fences at EOF and test the sanitize-failure fallback (#8147) * fix(triage): defuse mentions with ZWSP and track HTML blocks in sanitizer (#8147) * fix(triage): prose-escape code spans inside HTML blocks and widen inHtml entry (#8147) * fix(triage): degrade to escaped fallback when a code fence is open at EOF (#8147) * fix(triage): fail closed on paragraph code-span and container-fence divergence (#8147) * fix(triage): fail closed on escaped-backtick and entity-forgery sanitizer holes (#8147) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
易良
|
eabf312a06
|
feat(autofix): unify local and CI flows in one skill (#8121)
* feat(core): add current PR autofix controls * fix(core): align autofix ci status wording * feat(core): add current PR Autofix watcher * feat(cli): route Autofix watcher ticks * fix(autofix): fail closed on invalid watchers * fix(acp): reject malformed autofix ticks * fix(autofix): stop malformed watcher jobs * fix(autofix): preserve unrelated cron jobs * fix(autofix): separate watcher command from workflow skill * test(autofix): cover headless watcher delivery * test(autofix): cover watcher safety edges * test(autofix): cover watcher failure paths * fix(autofix): preserve non-watcher cron jobs * fix(cli): fail closed on malformed autofix ticks * test(cli): type autofix child process mock * test(autofix): align malformed watcher coverage * test(autofix): assert detached head skips gh * fix(autofix): close watcher control gaps * fix(autofix): preserve ordinary cron queue semantics * fix(autofix): extract shared constants, validate job id, broaden off filter * test(autofix): cover malformed watcher cleanup * refactor(autofix): keep current PR controls minimal * feat(autofix): reuse project skill for local runs * fix(autofix): harden local review boundaries * fix(autofix): enforce nested review containment * fix(autofix): require local repository trust |
||
|
Shaojin Wen
|
e447cbe0dd
|
feat(triage): start the verify lane alongside /triage on a pull request (#8249)
A `@qwen-code /triage` comment on a PR now starts the sandboxed verify lane too, in parallel. The two answer different questions — triage reads, verify builds and runs — and neither waits on the other; they were already sibling jobs on `needs: [authorize]`, so this is a trigger change, not a new pipeline. The trigger cannot be a second copy of the command patterns. authorize publishes `verify_lane`, and the verify job's `if` and concurrency group both read it, because re-matching the strings in the job predicate is exactly what would let the trigger and the trust classification drift: the classifier keyed on `/verify` while the predicate answered to `/triage` as well, and a `/triage`-started run would then have executed an external author's code with `verify_trust` empty — skipping the head-OID pin, the risk screen and both workspace wipes, the four controls that exist for untrusted code on a reused pool. Routing the new trigger through the same classifier is what keeps them on. The lane fails closed differently from an explicit `/verify`. An unreadable author permission or a failed head-OID snapshot still denies `/verify` outright, since the commenter asked for exactly that; on `/triage` it closes the lane only, so a flaky permission API cannot cost a reviewer their triage. Automatic per-PR triage (`pull_request_target`) deliberately does not pull the lane: the constraint is the agent budget, not runner capacity — a verify run is up to 110 minutes of model time, and every `synchronize` would spend one. Commenting is what says the PR is worth it. On a plain issue the lane stays off and no author lookup is spent. The verify lane's trigger turned out to be unpinned: deleting the gate from the job predicate left the suite green. The new test pins both predicates, and it took two tries — asserting over the whole job text could not tell `if:` from `concurrency:`, so the first version survived its own mutation, and so did the positive control written to vouch for it. Both are now sliced per predicate. Mutation results: dropping the gate from `if`, from `concurrency`, from authorize's `/triage` branch, and making the piggybacked lane deny triage are all caught; the unmutated control and the ECS kill-switch control behave as expected. Verified by executing the real authorize step with a per-user permission stub across twelve branches: /verify and /triage on a PR classify identically (trusted for a write author, external plus a pinned OID otherwise), /verify still denies on a lookup failure where /triage keeps should_run with the lane off, /tmux and plain comments are unchanged, and a commenter without write is denied throughout. actionlint, yamllint, eslint clean; the prettier warning on this file is pre-existing (fails identically with the change stashed). Co-authored-by: verify <verify@local> |
||
|
Shaojin Wen
|
2ad15a9fbc
|
fix(autofix): Extend suggestion handling to ten rounds (#8247)
* fix(autofix): extend takeover suggestion window Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): use ten-round suggestion threshold Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
8efdf749ad
|
fix(autofix): guard review thread resolution (#8231)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
qwen-code-dev-bot
|
0d3c8641f1
|
ci: cache npm downloads for verify and tmux build steps (#7885)
* ci: cache npm downloads for verify and tmux build steps The "Install and build PR app" step in both the verify and tmux jobs runs `npm ci` from scratch every time, taking ~5m40s out of a 15-minute verify run. Add an `actions/cache@v4` step before each build that restores the npm download cache keyed by `package-lock.json` hash. Security model: the cache restore runs as root (with full Actions credentials) in a separate step. The build step itself still strips ACTIONS_RUNTIME_TOKEN/URL/CACHE_URL before running PR lifecycle scripts as the `node` user, so untrusted code cannot read or write the Actions cache. The restored cache directory is chowned to `node:node` and passed via `npm ci --cache` so the build user reads packages from the local cache without touching the cache API. Expected improvement: npm ci drops from ~4min to ~1min on cache hit, cutting total verify time from ~15min to ~10min. * ci: pin actions/cache to SHA for supply-chain security (#7885) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * ci: add npm cache comment to verify job matching tmux job (#7885) * fix(ci): use actions/cache/restore to prevent cache writes from PR code (#7885) * test(ci): pin actions/cache/restore as restore-only invariant in both lanes (#7885) * test(ci): assert npm ci consumes the restored cache directory (#7885) * fix(ci): align prepare log with npm ci cache flag and harden cache tests (#7885) * fix(ci): harden npm cache tests and document missing save step (#7885) * fix(ci): add npm cache producer and clear stale cache before restore (#7885) * test(ci): harden npm cache guards per review (#7885) * fix(ci): make npm cache test robust to prettier YAML quoting (#7885) Prettier reformats the hashFiles() key value from single-quoted YAML (with '' escaping) to double-quoted, breaking the raw-string comparison in the cache producer test. Compare parsed scalar values instead. * fix(ci): run npm cache producer on the consumer runner so restores hit (#7885) actions/cache scopes an entry by a hash of the literal cache path plus the compression method. The producer ran on ubuntu-latest (host path, zstd) while the verify/tmux consumers run in a node:22-bookworm container (container path, gzip), so the versions never matched and every restore was a guaranteed permanent miss. Move the producer onto the same runs-on + container so path and compression match by construction, give the restore step an id and report cache-hit to the job summary so any future miss is visible, and point the stale-cache clear step at $RUNNER_TEMP so it removes the container path rather than the inert host path. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen Code CI <qwen-code-ci@users.noreply.github.com> |
||
ytahdn
|
7918717ade
|
fix(ci): avoid verify capture color conflict (#8236)
Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
|
Shaojin Wen
|
f66bfaad57
|
fix(release): keep notes anchored and cap the release body (#8199)
* fix(release): keep notes anchored and cap the release body
The v0.21.2 publish failed at "Create GitHub Release and Tag" with
HTTP 422 "body is too long (maximum is 125000 characters)", after every
npm package had already been published.
Stable releases are tagged on their own release/* branch and merged back
to main only afterwards, so the previous stable tag is never an ancestor
of the branch being released. The ancestor guard therefore dropped
--notes-start-tag on every stable release, and without an anchor GitHub
generates notes across the entire branch history (8000+ commits), which
overruns the body limit.
Always pass the previous tag instead: GitHub diffs it through the merge
base, which is how v0.21.1 produced a 27KB body from a tag that was
equally divergent. Generate the body through the generate-notes API
first so an oversized changelog is truncated on a UTF-8 boundary, and
degrade to an unanchored body and then a minimal one, rather than
aborting a release whose packages are already on npm.
* test(release): pin the anchored release-notes contract
The workflow test asserted the ancestor guard that dropped
--notes-start-tag on every stable release. Assert the replacement
instead: the previous tag is always passed to generate-notes, the body
is capped, and ancestry no longer decides whether notes are anchored.
* refactor(release): extract release-notes capping into a tested helper
The degradation chain lived inline in the workflow bash, so nothing
pinned that a capped body plus its footer stays under GitHub's 125000
character limit, that truncation never splits a multi-byte character, or
that the chain always yields a non-empty body. Move it to
.github/scripts/cap-release-notes.mjs with a collocated node:test suite,
matching the other workflow helpers.
Capping on code points rather than bytes drops the head/iconv dance and
makes the surrogate-pair case testable. The helper also absorbs the
empty-body fallback, which caught a real defect: gh writes the API error
payload to stdout when generate-notes fails, so a doubly failed call
would have published `{"message":"Not Found",...}` as the release body.
Discard a failed attempt's output instead.
* test(release): exercise the surrogate-pair cut and footer-overflow branch (#8199)
---------
Co-authored-by: Qwen Code Bot <qwen-code-bot@alibabacloud.com>
|
||
|
Shaojin Wen
|
3ff8892a82
|
feat(ci): fail the startup bundle check when the CLI entry is hoisted into a chunk (#8203)
`packages/cli/src/cli.ts` is the esbuild entry point and bootstraps only under a
main-module guard:
if (process.argv[1] !== undefined &&
import.meta.url === pathToFileURL(process.argv[1]).href) {
void runCliEntryPoint();
}
The bundle is built with `splitting: true`. If any module the entry loads lazily
(e.g. `gemini.tsx`, reached through `await import('./gemini.js')`) adds a static
`import ... from './cli.js'`, esbuild moves the entry module's body into a shared
chunk and leaves `dist/cli.js` as a re-export stub. Inside a chunk
`import.meta.url` is the chunk's own URL, so the guard never matches and the
bundled CLI exits 0 without running anything.
Nothing catches that today: tsc, eslint and every src-based unit test stay green,
because the breakage only exists in the bundle. The single CI step that executes
`dist/cli.js` is the no-AK integration smoke test, which reports it as
`daemon exited with 0 before listening` from three unrelated serve suites — a
symptom that points nowhere near the import that caused it.
Assert instead that the entry output still compiles the entry module. When the
entry is hoisted, `dist/cli.js` keeps no inputs of its own, so the metafile the
existing closure checks already read is a precise signal, and the diagnostic can
name both the cause and the fix.
Co-authored-by: verify <verify@local>
|
||
|
Shaojin Wen
|
0d6f3d42e6
|
feat(verify-pr): ship a one-command capture helper (#8114)
* feat(verify-pr): ship a one-command capture helper Fourth attempt at the same failure, and the first one aimed at the real cause. The score so far: #8016 captures were "Optionally … when text cannot carry the oracle" -> 0 images / 14 runs #8104 captures became budget item 4 in Scope selection -> 0 images / 1 run That last run is the one that settles it. Same PR (#7975), browser installed and working ("Install evidence browser: success"), and the verification got DEEPER — 64 assertions against 40, 53 tables against 31 — while still producing zero images. An agent reading the instruction, doing more work than before, and still not capturing is not an agent that missed the instruction. The cause is one I should have checked when I wrote #8016: the skill sent the agent to build node-pty -> xterm.js -> Playwright itself, and `node-pty` is not a dependency of this repo. It needs a native build. The `playwright` package is not a declared dependency either. So the documented route did not exist, and the incentive was entirely against trying it: authoring that pipeline risks failing and eats budget, while skipping costs nothing and is invisible. `scripts/verify-capture.mjs` makes a capture one command using deps that are already installed: node scripts/verify-capture.mjs --out evidence/01-ab.png \ --title 'A/B: the gate flips' -- node my-harness.mjs Command (or stdin) -> @xterm/headless parses the ANSI into a cell grid with colour and bold -> SVG -> sharp rasterises. No browser, no pseudo-terminal. A non-zero exit from the captured command still produces an image, because capturing a failing base arm is the normal case. The skill's dead route is removed and replaced with that command, and the budget line drops from ~5 minutes to ~2 because there is no pipeline to author. QWEN_VERIFY_CHROMIUM and its install stay for a future web-UI capture, but the terminal route no longer depends on them — gating on a browser the route does not use is how an absent browser turns into a skipped capture. Mutation-verified 7/7 against the real helper and the real PNGs: bare LF (staircase render), no event-loop turn (blank capture), dropped bold, dropped colour, no blank-row trimming, tolerating an empty capture, and dropped geometry validation. Two of those initially SURVIVED. The colour test compared "coloured and bold" against plain and asserted the bytes differ — which passes while EITHER attribute survives. That is the wrong-reason trap this skill warns about, met in the skill's own test file. Each attribute is now isolated against the same plain baseline (green-no-bold, bold-no-colour), and both mutations kill. 119/119 across both suites; prettier and eslint clean. * fix(verify-pr): harden capture helper per review (#8114) - Strip U+FE0F before rasterising: the emoji variation selector made Pango abort() in native code (SIGTRAP, no PNG, no diagnostic) when no colour-emoji font exists; the base codepoint renders. Add a non-ASCII regression test. - Await xterm's write callback instead of a fixed 120ms sleep, so a large capture is not read mid-parse and silently come out blank. - Name a signal-killed child ("killed by SIGKILL") rather than "exited null". - Warn on stderr when input is taller than --rows and the top is dropped. - Correct the falsified "route did not exist" rationale: the browser pipeline's deps do resolve from this repo; the real fragility is that integration-tests/terminal-capture is not a root workspace. Keep a skill pointer to terminal-capture for TUI/web-UI captures this helper cannot do. - Qualify the colour claim (16 base ANSI colours; 256/truecolor fall back). - Tests: feed both --cols and --rows to the geometry guard, exercise escapeXml, flatten the SKILL.md assertion against reflow, and replace the platform-fragile PNG byte-length check (flaked at 846B on Linux vs >1000B on macOS) with the deterministic canvas geometry. * fix(verify-pr): exercise colour fallback and fix wrap-aware truncation warning (#8114) * fix(verify-pr): separate stdout/stderr join and pin next() guard (#8114) * fix(scripts): correct verify-capture truncation guard and SGR 30 colour (#8114) The truncation guard compared wrapped rows against --rows, but newline-terminated output needs one row beyond its last line (the final CRLF scrolls it off the scrollback-less viewport), so input of exactly --rows lines — including the default 40 — lost its top line with no warning, and taller input under-reported the drop by one. Compare against a capacity of rows - 1 for newline-terminated input. Also lift SGR 30 foreground to the default grey: it mapped to #1e1e1e, identical to the canvas background, so black-foreground labels (e.g. vitest's project badge) vanished as black-on-black. * fix(scripts): address review feedback on verify-capture helper (#8114) - Declare sharp as a root devDependency so the script does not rely on workspace hoisting from packages/core - Guard against TTY stdin hanging silently: check process.stdin.isTTY before readFileSync(0) and print usage immediately - Fix phantom blank row when stdout already ends with a newline: only insert a separator between stdout and stderr when stdout lacks a trailing newline - Strengthen 256-colour/truecolor test to decode pixels and assert the #d4d4d4 fallback grey is present - Add test for non-newline-terminated input that fits exactly --rows - Add test for the phantom blank row fix (console.log + stderr) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
cc508dd7ba
|
fix(autofix): salvage race-lost pushes by merging the moved head and retrying (#8042)
* fix(autofix): salvage race-lost pushes by merging the moved head and retrying The review-address push is one-shot: when anything pushes to the PR head during the agent's ~50-minute window, the final push dies 'fetch first' and the entire verified agent run is discarded. The per-PR head-write concurrency group cannot prevent this — it serialises this repo's workflows, not the PR author or the fork side. Observed twice in one day (#7983 after a 56-minute run, #7985 after 43 minutes). On rejection, fetch the moved head, merge it into the local line, and retry (bounded at 3 attempts). Merge rather than rebase: the agent's own conflict-resolution rounds create merge commits, and a rebase would flatten them and can silently re-introduce the conflicts they resolved. The merge result descends from the remote head, so the retried push is a fast-forward and rewrites nothing. A genuine content conflict aborts the merge and falls through to the existing failure path unchanged. When a salvage merge happened, the round report discloses that the round's verification predates the merge so mid-run commits get re-checked by a human. * fix(autofix): address salvage-loop review findings - Gate the PUSH_RACE_MERGED disclosure on HEAD actually advancing: a transient push failure (upload timeout, 503) on an unmoved branch no-ops the merge ('Already up to date') and must not tell the reviewer to re-check mid-run commits that never existed. - Annotate the salvage fetch failure with ::error:: like the two adjacent failure paths, so a deleted fork branch or network error does not kill the step with an unannotated exit 128 under bash -e. - Re-pin the same-repo push URL construction in tests: it lost its old 'origin "${BRANCH}"' pin in this rework, leaving a ${REPO}→${HEAD_REPO} mutation (malformed remote in the same-repo case) unkillable. * test(autofix): restore dropped mutation-killing pins and add structural assertions (#8042) * test(autofix): pin exit 1 in the give-up guard regex to kill the deletion mutation (#8042) * test(autofix): pin exit 1 in the fetch-failure and merge-conflict salvage paths (#8042) * test(autofix): strengthen salvage-test pins to kill init-value and capture-order mutations (#8042) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
f6ae9202b1
|
feat(autofix): per-source feedback budget in Critical-only mode (#8071)
* fix(autofix): never defer maintainer feedback in Critical-only mode Critical-only mode (after 5 change-producing rounds) classifies feedback lexically: only a literal **[Critical]** tag or a CHANGES_REQUESTED review survives; everything else is deferred before the agent reads a word of it. That rule was built to stop the review bot's suggestion ping-pong, but it catches maintainers too. Observed four times in two days (#8037, #7944, #7885, #7799): a maintainer's review with explicit merge-blocking findings — #8037's said 'I'd fix before merge' on a correctness bug and a security-adjacent one — was wholesale-deferred as one 'non-Critical item', and the bot then reported 'No Critical feedback. The Issue-level comments sections are empty', which was lexically true and substantively false. The bot's own advertised definition ('correctness bugs, security issues, or formally requested changes') is exactly what the deferred comments contained; the agent that could have applied that definition never saw them. The lexical test now applies exclusively to the review bot's output: - All three actionable filters (reviews, inline, issue-level) pass anything not authored by the review bot straight through in Critical-only mode — the agent judges maintainer feedback on content, as everywhere else. - All three deferred-list builders keep only review-bot items, so a maintainer comment can never appear as an 'audit record'. - The deferral note says what is actually deferred (the automated reviewer's non-Critical suggestions), states that maintainer feedback is never deferred, and names the exit (@qwen-code /retry opens a fresh counting window). - SKILL.md's Critical-only policy now marks everything rendered in the actionable sections as in scope, so the agent does not re-refuse what the filter passed through. Behavioral test updated both ways: maintainer comments/reviews stay actionable in Critical-only mode across all three sources, bot suggestions still defer, and structural pins hold the bypass and the bot-only select in all six filters. * feat(autofix): per-author feedback budget in Critical-only mode Follow-up to the author-based split, prompted by the obvious counterexample: a human account can host an automated reviewer loop with the exact regeneration property the review bot has — feedback re-generated after every push at zero marginal cost — so 'not the bot' cannot mean 'never throttled'. An account is an accountability unit, not a throttle; the brake has to key on measured regeneration. Unified model: once Critical-only engages, every source has a bounded budget of untagged feedback batches per counting window. The review bot's budget is zero (all deferred, as before). A human's is CRITICAL_ONLY_HUMAN_BATCHES (2) CONSUMED batches: feedback items are bucketed into the (prev marker ts, marker ts] span that evaluated them, only spans from Critical-only rounds count, and an author needs K distinct consumed spans before their new untagged feedback defers. Fresh unevaluated feedback never counts against its own author, and the census is window-scoped, so /retry resets the budget with the window. The observed cases (#8037/#7944/#7885/#7799 — one or two late verification reports each) stay fully served under K=2; a looped reviewer is throttled after 5+K driven rounds instead of grinding to the 100-round cap. Past the budget, continuing requires one conscious act — **[Critical]**, a Request changes review, or /retry — which is precisely what separates intent from automation. Over-budget authors are named in the deferral note with those exact escapes. Tests: the six filter replays gain over-budget cases both ways (the tagged/CR escapes survive even over budget), and the budget census itself is replayed over fixture files — two consumed critical-tail batches list the author; one batch, pre-Critical batches, unconsumed feedback, untrusted authors, and command comments never count. * fix(autofix): fix deferred-feedback bash quoting and drop a dead jq binding (#8071) * test(autofix): exercise census window-isolation guard with a stale-window fixture (#8071) * test(autofix): make census command-exclusion observable; surface census stderr (#8071) * fix(autofix): exclude never-deferrable feedback from the budget census (#8071) The Critical-only per-author budget census counted every trusted review, inline comment, and issue comment, including feedback the deferred renderer would never defer: **[Critical]**-tagged comments, Request changes / APPROVED reviews, inline replies rooted at a Critical comment, and inline comments attached to a Request changes review. A maintainer who followed the documented escape hatches (tag Critical, request changes) thereby spent their own budget and had later untagged feedback silently deferred — the exact bug this PR fixes, re-created one level down. Mirror the three deferred-builder predicates in the census item filter so a batch is counted only when it is actually deferrable. Extend the census replay test with protected authors (Critical-only, Request changes, APPROVED, Critical-rooted replies, Request-changes-review inlines, the review bot as a trusted MEMBER, and a sentinel-ts marker probe) that each carry two consumed-span batches yet must stay absent, so dropping any one exclusion now fails the suite. Also fold bash's stderr into the bash -n guard assertion so a future quoting regression reports the syntax error, not just a non-zero exit. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
4dab39c8d7
|
fix(autofix): answer round-cap refusals on the PR instead of only in logs (#8067)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 1/2 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 2/2 (push) Waiting to run
E2E Tests / channel-plugin E2E (nightly) (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
* fix(autofix): answer cap-gate refusals on the PR instead of only in logs Observed on #7836: the fleet shepherd detected a merge conflict, posted 'dispatched the autofix loop to resolve it', and the dispatch died at the scan's round-cap gate with only a log line — the PR page showed a promise, the run showed green, and the conflict sat unhandled for hours. Three silences stacked: the standard-management cap itself is silent (the pause notice was takeover-only, so #7836 hit 10/10 with zero PR-visible notice), the forced-dispatch refusal is silent, and the shepherd dedups per head SHA — a capped PR gets no pushes, so its head never changes and conflict handling froze permanently. Two scan-side changes (the shepherd stays untouched — the windowed round computation lives in the scan and duplicating it would drift): - A FORCED dispatch (shepherd conflict lever or a human) refused at the cap gate now answers on the PR: cap value, what stays unhandled, and the two recovery commands (/retry for a fresh window, /takeover for the raised cap). No dedup — the shepherd sends at most one dispatch per head, and a human asking twice deserves two answers. - The cap pause notice covers ALL managed PRs: the takeover variant keeps its wording, standard bot PRs get their own (/retry or /takeover). Same marker, same once-per-window dedup, same consent and PAT-identity checks — skip wins everywhere, and only the takeover variant requires the label to still be present. After a re-arm the next scheduled scan picks the PR up normally (conflict targets are label-independent), so the frozen-head loop resolves without any shepherd change. * test(autofix): replay the cap-notice consent gate across label/takeover permutations (#8067) * fix(autofix): gate the loud cap-refusal on workflow_dispatch (#8067) FORCED_PR is populated for every trusted pull_request_review (route emits pr_number for those), not just workflow_dispatch, so on a capped PR each review submission landed in the un-deduped refusal branch — 7 "Dispatch refused" comments on #7836 where 2 carried the information. Answer only workflow_dispatch (the shepherd lever or a human); review submissions stay covered by the once-per-window pause notice. Adds a verbatim behavioral replay of the guard so a dropped EVENT_NAME condition fails the test. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
b64a6c4f13
|
fix(verify-pr): budget evidence capture in scope selection (#8104)
Post-merge measurement of #8016, on the first two live runs after it landed (#7975 and #8066): the browser installed successfully — "Install evidence browser: success" in both — and both reports carried **zero images**. #7975 has 31 tables, #8066 has 407 assertions. Neither has a witness. The cause is the third instance of one structural bug, and it is the same one twice already fixed: #7917 the /verify recommendation sat inside a section headed "local invocation ONLY" -> 1/16 reach #8016 captures were "Optionally ... when text cannot carry the oracle" -> 0/14 now captures are a required deliverable in the ARTIFACT CONTRACT, but the plan the agent executes is the Scope-selection budget list, and that list had zero mentions of evidence, png, capture or screenshot An agent that budgets by that list never allocates time for a capture, however firmly a later section says to produce one. So captures are now budget item 4 alongside the A/B, the harnesses and the gates, with the time reserved (~5 min), the gate named (QWEN_VERIFY_CHROMIUM=1), the count bounded (normally two, at most a handful), and the measured failure stated so the rule carries its own reason. The report structure gets the matching half: the A/B table item now says to reference the capture of those cells by filename. A produced image with nowhere to go is as dead as an unproduced one. Mutation-verified 4/4: dropping the budget item, the time allowance, the count bound, or the report-side reference each turns the test red. Not changed, and worth stating rather than implying: the seven techniques from #8010 do not appear in either report. I do not read that as failure — every one of them is conditional ("when one fix bundles two changes", "when the oracle is an instrument", "when a PR adds a writer to a shared store") and neither PR met those conditions. #8010 is not yet falsifiable from these runs. What both reports DO show is older skill rules working: a naive-matcher differential in #8066, and #7975's realpath check finding node_modules/@qwen-code/qwen-code-core resolving into the HEAD tree and then reasoning about whether that contaminates the control. 107/107 tests; prettier and eslint clean. Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
f4cd6e1d8b
|
fix(ci): gate the attachment guard before it allocates a runner (#8095)
Measured on a congested pool: 88 active jobs, 72 hosted and 15
self-hosted. The self-hosted 15 were all running with zero queued; the
hosted 72 were contending, and 20 of them were the SAME job —
`remove-suspicious-attachments`, all queued, none running.
Its real cost is not the work. Recent completed runs:
queue=629s run=5s queue=568s run=2s
queue=518s run=2s queue=340s run=3s
Two to five seconds of API calls behind up to ten minutes of queueing.
And almost none of it needed to happen. The trust check lived INSIDE the
github-script, so a runner was queued, allocated and started before the
job could decide it had nothing to do. Over the 200 most recent comments
on this repo: 184 from trusted associations, 9 from bots, 7 actually
needing a scan. 96.5% of these runs existed to print "Trusted author;
skipping".
Two changes:
- Hoist the association and bot checks into the job `if:`. GitHub
evaluates `if:` BEFORE allocating a runner, so a trusted comment now
costs nothing. The script keeps its own copies: the gate is an
optimisation, not the control, and the two must be able to disagree
without becoming unsafe. Every ambiguity therefore resolves toward
RUNNING the scan — an unrecognised payload yields an empty
association, which is not in the trusted list, so the job runs.
- Add a per-comment concurrency group with cancel-in-progress. The
workflow listens on `edited` as well as `created`, and the bot PATCHes
its own comments constantly, so repeated edits of one comment stacked.
The scan reads the comment's CURRENT body, so a queued earlier scan is
already stale and cancelling it loses nothing. (Contrast the verify
lane, where cancel-in-progress is deliberately false because a
cancelled run destroys evidence.) The key falls back to run_id so an
unexpected payload gets its own group instead of serialising every
scan into one.
Deliberately NOT moved to the self-hosted pool, though it would fit
technically (no checkout, no PR code, API calls only): the 20 stacked
jobs were duplicates, so relocating them just fills the ECS pool
instead — and that pool is what /verify and /triage depend on. It also
holds issues:write while processing untrusted comment bodies, which
belongs on ephemeral hardware rather than reused machines.
The `if:` semantics are verified against all payload shapes — 12 cases
covering both `comment.*` and `review.*` associations, bots, and
missing/empty payloads, each asserting which direction it resolves.
CONTRIBUTOR is deliberately NOT trusted: a merged PR does not make
someone's links safe.
Mutation-verified 6/6: dropping the review payload path, dropping the
bot check, adding CONTRIBUTOR to the trusted list, turning off
cancel-in-progress, collapsing the group to a global key, and inverting
the gate so untrusted comments are the ones skipped — each turns a test
red. The last is the one that matters; it is the only mutation here that
would be a security regression rather than a cost regression.
148/148 tests across both suites; actionlint exit 0; prettier and eslint
clean.
Co-authored-by: wenshao <wenshao@example.com>
|
||
|
Shaojin Wen
|
6b0d2bbca0
|
feat(triage): make /verify evidence screenshots actually possible (#8016)
* feat(triage): make /verify evidence screenshots actually possible
Measured across the 14 real verify reports on this repo: tables appear
in 14/14 (4 to 31 rows), images in 0/14. The hosting machinery has been
complete the whole time — pr-assets branch, PNG magic check, 8 images,
2 MB cap, sanitised names — and it has never once been fed.
Two independent causes, both fixed:
1. The agent physically could not produce a PNG. It runs as `node`
under `env -i` with HOME pointed at a per-run scratch dir, so
`playwright install --with-deps chromium` needs an apt it has no
rights to and would re-download ~170 MB into a directory deleted at
the end of every run. Chromium is now installed by ROOT in the tools
step into a shared, world-readable PLAYWRIGHT_BROWSERS_PATH, and the
agent env carries that path plus QWEN_VERIFY_CHROMIUM=1 — but ONLY
when the install actually succeeded, so the variable's absence is a
real signal rather than a stale promise. The install is best-effort:
a failure warns and the run continues to a text-only report, which
is what every run has produced anyway.
2. The skill discouraged it. Captures were "Optionally … use them when
text cannot carry the oracle", with TUI rendering as the only
example — so an agent verifying a daemon or a CLI correctly
concluded text carried the oracle and skipped. It now says produce
them whenever a harness ran, and says WHY: a table is the agent's
claim about what happened; a capture of the run is a witness that
the numbers came from a real execution, which is the part a reviewer
cannot get any other way. It names the three highest-value shots
(A/B cells side by side, the mutation matrix as printed, raw harness
output behind a headline number) and forbids `playwright install`
explicitly, so the failure mode of cause 1 cannot be rediscovered by
an agent burning budget on it.
Affordable now because #8014 raised the agent budget to 120m; at 20m a
browser download alone was most of the run.
Mutation-verified 4/4: handing the agent the variable unconditionally,
failing the job on a chromium install failure, reverting the skill to
"Optionally", and dropping the do-not-install warning each turn the
test red.
90/90 tests; prettier, eslint, actionlint and shellcheck clean.
* fix(triage): match browser to lockfile and fix pr-assets branch conflict (#8016)
Split the chromium install into system deps (tools step, as root) and
browser binary (new post-checkout step, as node using the checkout's
own Playwright). This eliminates the version pin that produced an
unusable browser tree.
Change evidence hosting from a bare pr-assets leaf branch — which
cannot coexist with the existing pr-assets/* namespace — to per-PR
branches (pr-assets/pr<N>-verify) with orphan-init fallback.
Update terminal-capture skill to warn CI agents away from the
forbidden playwright install command, and close test blind spots
for conditionality (M1b), version matching (M5), and marker
ordering (M6).
* fix(triage): unpin Playwright install-deps so apt list tracks lockfile browser (#8016)
* fix(triage): assert the browser path reaches the agent, not just the flag
Review finding on #8016, verified by mutation before accepting it:
deleting `"PLAYWRIGHT_BROWSERS_PATH=$CHROMIUM_PATH"` from the agent env
left the test green. That is the nastiest arm of this feature — the
agent is TOLD chromium is available via QWEN_VERIFY_CHROMIUM=1, then
Playwright looks in the default ~/.cache/ms-playwright instead of the
shared install and every capture fails, so a successful install still
degrades to a text-only report.
The test now asserts both variables and that BOTH sit behind the
success guard. Mutation-verified 2/2: deleting the path line fails on
the containment assertion, and hoisting it outside the guard fails on
the ordering one (`expected 21928 to be less than 21834`).
Not changed, because the other two findings do not describe this
codebase — replies on the threads carry the evidence.
90/90 tests; prettier and eslint clean.
* fix(triage): cover orphan-init delivery and delete verify asset branches
Two review findings on #8016, both about the per-PR branch scheme the
autofix bot introduced on this branch (
|
||
|
Shaojin Wen
|
cfcc711a70
|
feat(autofix): back off scan inspection of idle candidates (#8049)
* feat(autofix): back off scan inspection of idle candidates The scheduled scan inspects every candidate every tick. The takeover pool doubled in two days (28 open takeover PRs, 8 of them idle in 'nothing new' state for 10+ hours), and idle candidates crowd the two SHARED budgets: MAX_CANDIDATE_INSPECTIONS (60) and the 10-target cap. Observed on #8002: freshly engaged, admitted by the 09:03 scan, then deferred by the target budget while long-idle PRs re-confirmed their idleness yet again. Candidates whose list-provided updatedAt (no extra API call) is older than 24h are now inspected on roughly every 4th scan, on a deterministic slot keyed by PR number and UTC hour so no PR waits forever. The skip is free — it sits with the busy skip before the inspection-budget increment. Safe by construction: every real wake-up bumps updatedAt (reviews, comments, labels, pushes) or routes in real time anyway, so the only thing deferred is the scheduled re-confirmation of idleness plus worst-case a few hours of base-conflict-detection latency for a PR nobody touched in a day. The forced-dispatch path never builds the list files, so a forced PR is always inspected. * feat(autofix): idle-backoff review follow-ups - Corrected the comment's cost model: idle candidates hit 'continue' before the TARGETS append, so they never contend for the 10-target cap — the real win is the shared inspection budget plus the serial scan-walk latency (the walk is what delayed #8002's pickup by ~6 minutes), and the comment now says exactly that. - Slot quantum changed from the hour to the scan tick (600s, the same quantum as ROT_OFF): an hourly slot against the */10 cron meant 6 back-to-back inspections then a ~3h blind window per PR — same 25% average, terrible shape. The gap is now bounded at ~30 minutes, which is what the operator-facing strings promise ('gap ≤30m'). - The two scan-only signals updatedAt cannot see (a base conflict appearing when main moves; still-red checks awaiting the redcheck marker) are named in the comment instead of papered over. - The per-candidate jq fork became a single precomputed set + a bash substring test, matching the busy skip's idiom and the 'free' claim. - Tests: the skip predicate and set builder got a behavioral replay (idle+out-of-slot defers, idle+in-slot inspects, fresh inspects, missing-from-lookup inspects); the two byte-distance assertions became a loop-head slice (comment growth cannot red-light CI, and budget-consuming code between the skips and the increment fails); the --json field pin is order-independent; the 3600 quantum is pinned OUT. * test(autofix): pin the null-updatedAt defensive guard in idle-backoff replay (#8049) * fix(autofix): extract idle-backoff predicate from workflow, fix gap bound 30→40m (#8049) * test(autofix): pin takeover-prs.json in idle-backoff replay (#8049) * fix(autofix): state idle-backoff gap probabilistically, unpin quantum (#8049) Round-2 verification showed the scheduled scan lands every ~40-70 min on this repo, not every 10 min, so the (epoch/600)%4 slot is an independent ~25% draw per scan, not a deterministic 1-in-4 rotation. The gap is geometric (measured median ~2h, p90 ~6h), not bounded at ~40m. Reword the operator-facing strings (skip echo, fleet row) and the workflow comment to state the behaviour probabilistically and drop the false "bounded" / "no PR is unlucky forever" claims; correct the misattributed #8002 latency (queue/startup, not the serial walk). Relax the tests so a truthful number is not a CI failure: pin the mod-4 time-quantum shape instead of the exact 600s constant, and drop the /3600 exclusion that forbade the better-tailed quantum. Mechanism logic is unchanged. --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
d5ac7a9642
|
fix(autofix): post the takeover engage ack from the command itself (#8043)
* fix(autofix): post the takeover engage ack from the command itself The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (#7999 — the author read the silence as failure and removed the label; #8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on #7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck. * fix(autofix): ack command-driven releases directly and key the scan grace on the label actor Review follow-up: the engage-side fix left the release side on the fragile round-trip — a loud add next to a mute stop re-creates the exact 'did it work or did the event get lost?' ambiguity this PR set out to remove, now on release. And a fork or non-main release could never ack at all (fork unlabeled events carry no secrets; the route ignores non-main releases). - takeover-command now posts the release ack directly after removing the label, mirroring the engage side, with the same three variants as the ack job (plain release / bot-authored / bot-authored+skip) chosen from the same PR_INFO the gates used. The route suppresses the unlabeled-path ack when the label sender is the bot. - The scan's first-pickup grace is keyed on the label event's actor: a bot-applied label defers only 45s (the command's own write is seconds behind the label — fork or in-repo alike), so the immediate scan still heals a failed command ack instead of slipping to the next scheduled tick, and an ic.json snapshot taken between the label write and the command ack cannot double-post. A human in-repo label keeps the 3-minute ack-job grace; a human fork still posts right away. - The fork note now says 'usually within minutes', matching the */10 cron instead of contradicting it by 6x. - Tests kill the two surviving mutations from review: the EN/ZH fork-note swap (per-language-half assertions) and the warning fallback downgraded to '|| true' (both fallback strings pinned). * test(autofix): behaviorally pin the LAST_LABELED_BY jq extraction (#8043) * fix(autofix): correct stale comments about scan heal timing and command blast radius (#8043) * test(autofix): pin release-ack body identity across the command and ack jobs (#8043) --------- Co-authored-by: verify <verify@local> Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com> |
||
|
Shaojin Wen
|
c631c0448e
|
fix(autofix): cumulative timeout breaker, narrowed retry prompt, truthful handoff wording (#8044)
* fix(autofix): stop repeat-timeout PRs with a cumulative breaker and narrow the retry prompt The consecutive-failure cap resets on every pushed round, so it never fires on the observed failure shape: timeouts interleaved with successes. #7929 burned three 50-minute agent timeouts (with pushed rounds in between) and #7846 two — each a full agent budget with nothing to show, invisible to the breaker by construction. Three changes: - Cumulative timeout breaker (TIMEOUT_WINDOW_CAP=3): time-budget exhaustions are counted over the whole counting window, successes in between included. At the cap the PR goes terminal with a skip-until-/retry headline, exactly like the consecutive breaker. The census reuses PRIOR_HEADS, so it is window-scoped and a re-arm clears it. - Prompt narrowing from the second attempt: when the current window already contains a timeout marker, the prepare step appends a budget warning to feedback.md — smallest blocking subset first, commit early, defer the rest explicitly — instead of re-running the identical address-everything prompt into the same wall. - Truthful gate-rejection handoff: 'A human should take over this PR' read as a full release, but the loop keeps managing the PR (new feedback, base conflicts) — #7929 posted it and then kept pushing rounds. The headline now says exactly which half is over: this item will not be retried automatically, management continues. * fix(autofix): timeout-breaker review follow-ups All six review findings addressed: - The narrowing prompt now restates the SKILL deferral contract instead of pointing at 'your summary': deferred findings stay out of resolved-comments.txt and get a comment-replies.json entry so every open thread carries its reason — the summary-only shortcut is exactly what a budget-pressured agent would otherwise take. 'skip refactors entirely' became 'decline with a one-line reason' for the same never-drop-silently rule. - The narrowing census counts timeouts SINCE THE LAST SUCCESSFUL ROUND, not cumulatively: a push falsifies 'not converging', and one old timeout must not degrade every later round of a 100-round window. The breaker stays cumulative (a push does not make the next timeout cheaper in budget terms). - The breaker headline states what the census measured ('this counting window now contains N time-budget exhaustions') instead of 'stopped after N' — the tripping round can itself be a gate rejection, which is the exact rollout state of #7929/#7846. - Both census needles anchor on the verbatim emitted headline ('AutoFix ran out of time before finishing') — first lines can embed provider error text, so the loose phrase could count a model error as a timeout. - The precedence assertion got teeth: 'consecutive' alone matched both branches; it now asserts the consecutive breaker's own phrase and the absence of the timeout one (the if-true guard mutation now fails). - The narrowing census got a behavioral replay over fixture ic.json: push-resets, trailing counts, legacy no-win markers under 'none', old-window exclusion, author filtering — plus a pin on the -ge 1 trigger so a threshold mutation cannot leave the feature inert. * fix(autofix): state what the timeout breaker measured, pin the inherited guarantees Second-round review follow-ups: - The breaker headline no longer infers 'is not converging' — a window can carry 40 pushes and still trip the (deliberately cumulative) cap, so the sentence now states only what the census measured: N full agent runs that pushed nothing. - Two guardrail tests pin behavior that held only by construction: a non-timeout failure landing on an already-capped window still trips the breaker (the documented rollout state — an 'only count when this round timed out' cleanup would silently delete it), and the transient-API-error exemption inherited from the outer guard (a refactor hoisting the block out of it must not mass-terminate every in-flight PR during a provider outage). * fix(autofix): name a gate rejection only when the gate ran; pin the timeout breaker's stale-base exemption Third-round review follow-ups: - The handoff headline no longer claims 'the verification gate rejected the attempt' for every outcome=failed verdict: that branch is reached for five distinct failures (failure.md abort, dirty tree, unchanged branch, missing summary, and a real gate rejection), only one of which is a gate decision. The clause is now gated on gate-rejection.md, which reject_fix is the sole writer of — so the other four paths keep the neutral framing instead of repeating the very wording-doesn't-match-behaviour bug this PR fixes. A replay test pins both halves: the clause appears when gate-rejection.md exists and is absent when it does not. - The timeout breaker's stale-base exemption is now pinned by a replay (five in-window timeouts plus a stale-base retry current round stays retryable), matching the existing transient-API-error pin: a refactor hoisting the timeout block out of the outer guard would otherwise delete this exemption silently. - The prepare-step narrowing census jq now fails open (2> /dev/null || true) like its report-step sibling, so a malformed ic.json drops the budget warning instead of aborting prepare under errexit and turning the round into a terminal 'could not start evaluation' handoff. - The decay comment now matches the code: the warning resets on a push/no-op round but fires on every failing round until then (gate rejections included), which is correct since nothing has converged. * fix(autofix): clear stale gate-rejection.md in repair cleanup and pin headline template (#8044) * test(autofix): cover no-op reset branch in timeout census (#8044) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
25f5147677
|
feat(autofix): post a takeover milestone digest every tenth pushed round (#8046)
* feat(autofix): post a takeover milestone digest every tenth pushed round The takeover round cap (100) bounds runaway but carries no signal about when a human should step in: #7469 ground to round 12 over seven days of takeover with fifteen pushes, and the only place that trajectory was visible was the Actions logs. Every 10th pushed round under takeover, the report step now posts a window-scoped census on the PR itself — pushed fixes, no-change reviews, timeouts, rejected attempts, base updates — plus the three options (keep going / split or reduce / release takeover). The digest is a separate comment with its own autofix-milestone marker and no autofix-eval marker, so every census (round, consecutive-failure, watermark) ignores it and the feedback filters keep it out of the agent's prompt. Posting is best-effort: a digest failure never fails a good push. The rejected-attempt count matches both the current and the reworded gate-rejection headline so the census cannot silently zero; base updates carry no win= field and are windowed by timestamp instead (the window key is the engage ack's created_at). * feat(autofix): milestone digest review follow-ups - Residual bucket: every outcome the four buckets missed (crash, model error, gate error, infra) now lands in 'other round(s)' — a window that burned 80% of its budget on crashes was rendering as four zeros, QUIETER than a healthy window, the inverse of the digest's premise. - Crossing trigger: fire on the first pushed round once 10+ rounds have accumulated since the last digest in this window, instead of an exact %10 hit — failure rounds advance the counter too, so push@9/crash@10/ push@11 skipped the digest forever on exactly the failure-heavy PRs it exists for. - The success log is chained to the post; a failed comment no longer logs 'posted' after its own warning. - WINDOW=none says 'since the PR opened (no counting window yet)' instead of claiming a current window while counting all time. - A census that parses zero window markers at round 10+ skips the digest instead of posting a fabricated all-zero one. - autofix-milestone joins BOT_COMMENT_FILTER (marker inventory). - The timeout needle anchors on the verbatim emitted headline. - The pin-only test became a behavioral replay: the digest block runs under bash with a stubbed gh over fixture ic.json histories — bucket counts, residual loudness, crossing suppression/resumption, old-window isolation, none-window phrasing, non-pushed/non-takeover gating, and the empty-census skip. The OUTCOME == fixed conjunct is pinned. * test(autofix): cross-pin digest census needles to headline emission sites (#8046) * test(autofix): widen rejected-headline cross-pin to match #8044 reword (#8046) * test(autofix): behaviorally cover the digest comment-failure branch (#8046) --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
36fe53d421
|
feat(triage): raise the /verify agent budget from 25m to 120m (#8014)
* feat(triage): raise the /verify agent budget from 25m to 120m
Maintainer decision: give /verify enough time to do what a maintainer's
local round does, and add machines if the pool gets tight.
Three values encode this one budget and each has its own silent failure
when edited apart, so all three move together:
- agent kill 25m -> 120m (the graceful budget; ships a partial
report on expiry)
- watchdog secs 1500 -> 7200 (distinguishes that kill from an OOM;
left low, a late 137 is mislabelled
`timeout` and publishes "partial
evidence" for a crash)
- job timeout 60m -> 150m (guards infra hangs only; it must clear
agent + install/build + overhead or it
kills the container mid-run and the
ship-what-ran path never executes)
150 rather than 130: measured install+build is ~6m (run 30284341325,
npm ci 3m00 + build 2m40), budgeted at 15m for a cold cache, plus ~5m of
tools/checkout/pin/upload/cleanup — worst case ~140m, leaving 10m of
headroom.
The skill's advertised budget moves too, and this is the part that
actually changes behaviour: it read "≈ 20 minutes (hard 25-minute
kill)", and an agent obeying that would have self-limited no matter what
the workflow allowed. It now reads 110/120 and says what the extra time
is FOR — bisecting a threshold through the real code path, compiling an
intermediate build to separate the halves of a bundled fix, adjudicating
mutation survivors, driving a real daemon end to end. It also says
plainly that spending it on breadth is the one way to waste it; the
rule that one proven load-bearing claim beats ten unverified
observations does not relax because the clock did.
Pinned by a new test that asserts the RELATIONSHIPS rather than the
numbers: watchdog == agent budget in seconds, job >= agent + 20m, and
the skill's advertised hard kill == the workflow's, with the soft budget
strictly below it. Mutation-verified 4/4 — leaving the watchdog at 1500,
the job at 60, the skill at 25m, or raising the soft budget above the
hard kill each turn it red.
Cost, stated so it is a decision and not a surprise: a verify run now
holds one ECS slot for up to 2.5h instead of 1h. Concurrency is
unchanged (per-PR group, cancel-in-progress false), so this reduces how
many distinct PRs can verify at once, not how many runs a PR can queue.
90/90 tests; prettier, eslint and actionlint clean.
* fix(ci): pin soft-budget lower bound in verify consistency test (#8014)
* fix(ci): harden verify budget guard and clarify budget is a ceiling (#8014)
* fix(ci): measure elapsed before artifact copy and tighten budget guard (#8014)
* test(ci): pin elapsed-time chain in verify budget drift guard (#8014)
* refactor(ci): derive verify watchdog threshold from the agent budget (#8014)
The watchdog threshold that tells a graceful timeout (137 at the budget)
from an OOM kill (137 before it) was a bare `7200` coupled to the `120m`
agent timeout only by a comment. Define the budget once as AGENT_BUDGET_M
and derive both the timeout (`${AGENT_BUDGET_M}m`) and the threshold
(`$((AGENT_BUDGET_M * 60))`) from it, so editing one cannot silently
desync the other. The consistency test now asserts that derivation rather
than re-extracting two independent literals.
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
---------
Co-authored-by: wenshao <wenshao@example.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
|
||
|
Shaojin Wen
|
0232e73811
|
feat(verify-pr): add seven techniques from maintainer verification rounds (#8010)
* feat(verify-pr): add four techniques from maintainer verification rounds Two hand-written maintainer rounds contained methods the skill could not have produced. Checked each against the current text before adding it; these four had no coverage at all. From #7914 (live daemon A/B on the artifact-recording change): - Run every control on BOTH arms. That round's sharpest finding came from a control whose only job was to validate the BASE probe — "the empty list is a real absence, so have the model call record_artifact and watch an entry appear". Run on head as well, it showed the curated title being silently discarded. The control was not hunting for a bug; running it symmetrically is what found one. - A new writer into a shared store is an ordering change. The PR added write_file as a second writer into the artifact list; the bug was not in the new writer but in the collision, where a pre-existing first-writer-wins merge began discarding record_artifact's curated title and description while still reporting success. Enumerate the other writers, exercise the collision in both orders, and check what the loser is told — and separate the pre-existing cause from the PR's contribution so the author is not blamed for the policy. From #7998 (ink cursor fix, real-terminal A/B): - When the oracle is an instrument, corroborate it with a mechanism that does not use that instrument. The hardware cursor row came from `tmux display-message -p '#{cursor_y}'`, then from a marker printed after the TUI exits — which lands wherever the cursor actually was. Two agreeing instruments turn a measurement into evidence; one tool's report about the system is not the system. - Re-run the generator on committed generated artifacts and diff. That round re-ran `npx patch-package ink` and found byte-different hunk headers, proving the .d.ts hunks were hand-written rather than regenerated as the description claimed. Also strengthens Not covered: proving a limitation is environmental requires an A/A control (boot base and head identically, show both fail the same way), because "seems environmental" and a real regression look identical in a report. Mutation-verified 4/4, each with landing proof. Two initially reported `landed: False` — the assertions match the whitespace-normalised text while the rules wrap across lines in the source, so the replace never fired and the green result proved nothing. Re-run against line-accurate anchors, both kill. 89/89 tests; prettier and eslint clean. * feat(verify-pr): teach the timing-race and scenario-arrival checks Third maintainer round mined for method (#7934 R4). The blocker it found had zero coverage in the skill — `timer`, `wall-clock`, `flake`, `retry`, `duration`, `deterministic` all returned 0, and the one `race` hit was a substring of "trace". - **Timing-triggered assertions have a threshold — measure it, do not sample it.** A new guard (`expect(false).toBe(true)` after an abort loop) turned a vacuous pass into a deterministic failure, because the case triggers its abort from `setTimeout(..., 1000)` while the query's duration is set by CLI startup rather than the server. Natural completion measured 730-2151 ms, so every box on the fast side of 1000 ms fails. The rule says to measure the operation's natural duration with the trigger disabled and compare it to the timer, because a green run only proves this box was slow enough. - **A speed-correlated failure is not flake, and a retry budget does not absorb it.** Random flake becomes a pass under `retry: 2`; this failed 5/5 runs on all three attempts. The two get opposite verdicts, so the kind has to be established before the verdict is written. Stated plainly in the skill: the verify job runs on a shared, loaded runner — the regime where such a test PASSES. Repetition cannot reproduce a fast-machine failure there; only computing the margin can. A rule that said "run it more times" would be useless in this lane. - **The failure one level before vacuity: the scenario never reached the code under test.** The vacuity check asks whether an assertion can fail; this asks whether the code ever ran. Four abort cases fired during CLI process startup, so the fake server saw zero requests and a suite named for mid-stream aborts never streamed — with every assertion green. Instrument the seam and assert the count is non-zero. Mutation-verified 5/5, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. Skill is 472 lines, up from 392. * feat(verify-pr): six more techniques, from three maintainer rounds Mined #7836 R2, #7885 and #7899 for method. Checked each candidate against the current text first; six had zero coverage, the rest were already there (harness teeth-checks, pin dereferencing, boundary probing, and the follow-up round's "re-measure, never diff"). The one that corrects the skill's own core method, from #7836: - **Before calling a survivor vacuous, escalate to a finer mutation.** A whole-file revert is blunt enough to remove the PRECONDITION a test depends on, so a good test goes green because its scenario no longer occurs — from the outside, identical to a test that asserts nothing. A `finally`-cleanup test survived reverting four production files and died to deleting one line. Coarse survived + fine killed ⇒ the test is fine and the mutation was wrong. A false "your test is vacuous" costs the author more than a missed survivor does. From #7836, the root cause shared by both of its blockers: - **When the same predicate is checked in two places, verify they see the same state.** A guard duplicated across a process boundary is two implementations of one question that diverge when their INPUTS differ. One settings key made a route ask sessionExistsInAnyState() with an unpinned runtime dir while the child asked with a pinned one, turning a clean 409 into a 500 plus a process.exit(1) that killed every session on the channel. Includes the temporal half: lazily-created backing files leave a window where a just-created entity is invisible to any on-disk existence check. - **Measure the blast radius on bystanders.** The caller's own error code understates a shared-state failure; the number that matters is an unrelated session going 200 -> 404 and a workspace list going 2 -> 0. From #7885, which took a performance claim apart: - **Isolate the slice the mechanism can actually affect.** A speedup claim is two claims: the mechanism works, and the thing it speeds up matters. `--ignore-scripts` isolated what an npm download cache can touch — 36s of a 226s install — so the ceiling was 20s and the real saving 15%, not the claimed 75%. Then check it against the whole job: 33s off 14m37s. - **A mechanism that persists something has a cost — price it.** 219 MB per lockfile hash into a pool at 9.98 GB of a 10 GB cap, with 39 distinct lockfile states in 30 days: at the cap every entry evicts by LRU, including entries other jobs need and its own. - **Test the scarier consequences and report which do NOT hold.** The write-path finding was real; code injection was disproved (npm integrity-checks a tampered cache and refetches) and privilege escalation was disproved (chown -R does not follow symlinks). A finding that names what it is not is harder to wave away. - **Verify third-party actions from their own manifest.** The PR said the cache dir was discarded after the job; `action.yml` declares `post: dist/save/index.js` with `post-if: success()`, which uploads it as root with credentials intact — the opposite of the claim, and the whole finding. From #7899: - **To exercise real production data safely, interpose a refusing proxy on the write path.** Wrap the client so every mutating call hard- fails, then run the shipped script verbatim: real counts, mechanical guarantee of no side effects. Mutation-verified 9/9, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. The skill is now 546 lines, up from 392 — the growth is deliberate and called out in the PR body's risk section. * feat(verify-pr): decomposed fixes, contextual limits, destination counts From #7862 R4. Three additions, and a deliberate stop. - **When one fix bundles two changes, build the intermediate variants.** An A/B against base proves the pair works and says nothing about what each half does. That round compiled a third build with only the ordering change reverted, and the three-row table showed the halves do different jobs: moving `initialized = true` after the fallible work converts a 2,999-and-climbing backlog flood into a fail-safe retry, while `reduce()` restores liveness. Either alone leaves a channel that floods or wedges — a conclusion the two-cell A/B cannot reach. - **A limit measured in isolation does not transfer to the real call site.** The same `Math.max` spread threw between 110k and 130k elements inside a deep async stack, well below a standalone micro-benchmark. Bisect thresholds through the real code path and quote the harness; a limit taken from documentation or a toy loop is a guess about the system under test. - **Count at the destination, not at the component boundary.** The mirror of the scenario-arrival rule added earlier: envelopes the adapter emitted and prompts that reached the agent are different numbers, and every gate lives between them. A count taken at the seam can be right while the feature is silently dropped downstream. Declined from the same report, to protect prompt budget rather than because they are wrong: siblings-as-convention-oracle (the lockfile version table across five channels), degenerate fixtures that cannot distinguish two sort keys, and naming the condition under which a cosmetic finding becomes real. Each is a good technique; none is worth another rule competing for attention with the ones already here. The skill is now 578 lines, up from 392 on main (+47%) across this branch. That growth is the main risk on this PR and further additions should wait until a live round shows the current set changes behaviour. Mutation-verified 3/3 with landing proof. One mutation initially SURVIVED — it deleted text sitting AFTER the asserted phrase, so the assertion still matched and the green proved nothing; re-run against the phrase itself, it kills. 89/89 tests; prettier and eslint clean. * test(scripts): drop stale technique count from verify test name (#8010) * fix(triage): correct verify-skill worked examples and verdict path (#8010) Address review feedback on the verification-techniques skill: - Make the npm-cache worked example's numbers close: separate the 20 s download-slice ceiling (36 s to 16 s) from the 15% end-to-end saving (226 s to 193 s) rather than conflating them. - Stop overstating the tarball experiment: one tarball was poisoned, and the 2262-entry integrity coverage is a separate static fact. - Give the speed-correlated-failure rule a contract-legal verdict path by encoding the margin as a scripted assertion, and mark the load/idle sweep as the local-mode variant. - Fix the one bullet that broke its 2-space list continuation. - Pin the new contract-encoding clause in the workflow test. --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
易良
|
c55e63c42a
|
fix(release): bump preview base past published stable (#7978)
* fix(release): bump preview base past published stable When the nightly tag base (e.g. 0.21.0) is already published as stable, getPreviewVersion() now bumps the patch (→ 0.21.1-preview.0) instead of deriving a preview for the released version. This prevents the scheduled Tuesday preview release from hitting npm E403 on channel packages that were already published at that version. Fixes #7969 * fix(release): address preview version review feedback * fix(release): reject divergent preview baseline * fix(release): skip already published packages * test(release): cover publish skip guards * fix(release): bump preview past newer stable * fix(release): address review feedback on preview version guard - Expand doesVersionExist to check all 10 published packages instead of only @qwen-code/qwen-code, so the auto-increment loop detects versions taken on sibling channel packages. - Use the rollback-aware getAndVerifyTags lookup for the latest stable instead of the raw dist-tag, preventing a retrograde preview base when the dist-tag has been rolled back. - Emit :⚠️: in the channel publish loop when every package was already published, making a fully-skipped release visible. - Move preview stable-guard tests to Advanced Scenarios, add npmTag and previousReleaseTag assertions, add a non-bump boundary case, and assert the subshell wrapper and all-skipped warning in the workflow test. * test(release): cover channel package version conflicts --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |
||
|
易良
|
ff5c639e26
|
fix(release): raise model timeouts and shrink batch size for slow networks (#8007)
* fix(release): raise model timeouts and shrink batch size for slow networks The AI release-notes generator timed out on every batch in the v0.21.1 finalize run on a GitHub-hosted runner (US, Azure westus3) hitting a remote LLM endpoint. The 60s per-request timeout was too close to the edge: two successful requests took 54.2s and 56.7s, and every other batch hit the 60s abort. The 12-minute total budget was consumed by retries on those timeouts, and the circuit breaker opened after 3 consecutive failures, skipping highlights entirely. - timeoutMs: 60s -> 180s — give the model enough headroom to generate a full JSON response over a high-RTT cross-region connection. - totalTimeoutMs: 12min -> 30min — ~140 PRs at 8 per batch is ~18 batches; at ~90s each that's ~27 minutes of model time. - batchSize: 12 -> 8 — fewer entries per prompt means the model generates less per request, returning faster and reducing the chance of a single slow request dragging the whole batch. - workflow timeout-minutes: 15 -> 35 — match the new 30min budget plus a margin for git/npm setup. No changes to retry logic, circuit breaker, or prompt shape. * fix(release): sync timeout workflow test * test(release): assert step timeout exceeds script budget The workflow `timeout-minutes` and the script's `totalTimeoutMs` are defined in separate files with no shared constant. If they drift apart silently and the budget exceeds the step timeout, the runner SIGKILLs the step and even the fallback release notes are lost. Add a cross-file assertion so any future change to either value that breaks the invariant fails the test immediately. Suggested-by: wenshao in PR review. * test(release): parse release notes timeout expression |
||
|
Shaojin Wen
|
ea4c25e25f
|
feat(triage): run external /verify on ECS behind a workspace wipe (#7985)
* feat(triage): sponsored /verify for external PRs on ephemeral runners
/verify used to require the PR AUTHOR to hold write access, because the
lane executes the author's code on the persistent ECS pool — which
excluded external contributors' PRs entirely (16 of 38 open PRs
eligible when measured on 2026-07-28), and those are the PRs where
sandboxed evidence is worth the most. The practical alternative was a
maintainer running the verification on their own machine, which is
strictly worse: personal credentials, SSH keys, and no isolation at
all. The risk was not avoided, only relocated.
The author's permission now ROUTES instead of gating, using the same
conditional runs-on pattern the authorize/triage jobs already use for
fork PRs:
- write-access author -> the persistent ECS pool, unchanged;
- external author -> a SPONSORED run on an ephemeral GitHub-hosted VM
(destroyed after the job), free on a public repository.
The commenter gate is unchanged: only a write-access maintainer can
trigger either lane, so runs stay maintainer-metered. Their comment is
the sponsorship, and authorize snapshots the head OID at that moment;
the resolve step refuses to run any other commit, so code pushed while
the job queues is never executed on the sponsor's approval. On the ECS
lane the execution-time re-check keeps its old meaning (the author's
write permission is the control there, and it can be revoked while
queued); its refusal now names the sponsored alternative.
Before anything executes on the hosted lane, a pre-execution risk
screen reads the PR diff as data: mechanical checks (added npm
lifecycle scripts, dependencies resolving outside registry.npmjs.org,
package-manager config changes, long opaque single-line content) and a
model screen, every arm failing CLOSED — an unfetchable diff, missing
screen config, a model error, and an unparseable reply each refuse the
run. Reasons posted to the PR are fixed strings chosen by the
workflow, never diff text, so a crafted diff cannot steer the bot's
comment. The screen is defense in depth, not the control: what bounds
a malicious PR is the ephemeral VM. The model can be fooled;
the machine being destroyed cannot.
The ECS kill switch no longer strands the hosted lane (an ephemeral
runner does not depend on the pool), and the authorize-side denial
notice for external authors is gone — the case it explained no longer
denies. The 2b-bis skill guidance now offers the sponsored run instead
of declaring the lanes unavailable, with an explicit warning that an
external author's report is adversarial input to be read like the
fork's own CI logs.
Mutation-verified 8/8, each with landing proof: dropping the commenter
gate, inverting the routing, dropping the OID snapshot, letting a
permission-API failure route instead of deny, dropping the head-moved
check, dropping the lifecycle-script check, tolerating an unparseable
model reply, and pinning the hosted lane back to ECS each turn a test
red. The screen tests run the real resolve-step text against a stubbed
gh and a live local HTTP server standing in for the model endpoint —
in a separate process, because spawnSync blocks the test runner's
event loop and an in-process server would deadlock into the fetch's
120 s abort instead of testing anything.
actionlint category diff vs main: -3 SC2016:info (the removed denial
step), +3 SC2129:style (new GITHUB_OUTPUT groups); both non-gating.
* docs(triage): persistence, not lateral movement, is the ECS threat
The maintainer confirmed the ECS runners are network-isolated, so the
comments justifying the hosted routing now name the decisive risk
correctly: machines are reused and the next run arrives carrying
credentials that planted artifacts could harvest. Lateral movement was
never the load-bearing argument; persistence is, and it is the one
threat a destroyed VM removes without depending on sweep completeness.
* feat(triage): run external /verify on ECS behind a workspace wipe
Maintainer decision (2026-07-29): external contributors' PRs run on the
persistent ECS pool like everyone else, not on ephemeral GitHub-hosted
VMs. The pool is network-isolated, and the realistic alternative — a
maintainer verifying on a personal machine with real credentials and no
isolation — is worse than a hardened shared runner.
That removes machine destruction from the design, so the author's
permission now selects CONTROLS rather than a machine:
trusted -> unchanged: write permission is the control, re-checked at
execution time, head may move freely.
external -> pinned head OID (the sponsoring comment approves the code
that existed then), pre-execution risk screen, and a full
workspace wipe.
The wipe is what replaces destruction. The residual ECS risk for
untrusted code is PERSISTENCE: machines are reused, and the next run
starts by handling credentials (CI_BOT_PAT in the resolve step, the
model key in the proxy). The existing sweep is an allowlist of vectors
we have thought of, and an adversarial author gets unlimited attempts to
find one we have not — so external runs delete the workspace outright
before checkout instead of sweeping it. Deny-by-default costs one cold
clone and removes hooks, worktree config, planted dotfiles, staged
artifacts and out-of-tree symlinks in one move, known or not. Scope is
stated in the step: RUNNER_TEMP and /tmp also persist, and the steps
reading them already rm -rf their own subtrees; the container is
recreated per job, so /home/node and its npm cache are not cross-run
state.
The screen's status changes with the machine: on ephemeral hardware it
was a cost-saver, here it is load-bearing — but still not sufficient
alone, and the comment says so, naming the wipe and the sweep as what
actually bounds a screened-but-hostile diff.
Mutation-verified 6/6: removing the wipe, moving it after checkout,
applying it to every run, tolerating a non-empty workspace, dropping the
suspicious-path guard, and classifying an external author as trusted
each turn a test red.
Also fixes a live hazard in the guard's own test. It passed real paths
(/, /usr, /root) to a live `rm -rf`, which is safe only while the guard
exists — so it detonates precisely when the guard is removed, which is
when it is meant to protect you. It did: a mutation run that deleted the
guard spent six minutes attempting to delete / before being killed.
macOS permissions absorbed it and nothing was lost, but that outcome was
luck, not design. `rm` is now stubbed to a recorder on PATH, so the
destructive primitive cannot fire from the test suite under any edit,
and the assertion is on the decision — with the guard gone the recorder
shows the attempted delete and the test fails, having deleted nothing.
* fix(triage): close two screen bypasses and a vacuous routing assertion
Review round on #7985 found four issues; all four were real.
1. npm lifecycle alternation stopped at the install lifecycle, but npm
runs pre/post hooks for ANY `npm run <script>`. This job runs
`npm run build` and the agent runs the PR's suites, so a `prebuild`
or `pretest` executes exactly like a `postinstall` and walked past
the mechanical screen. Added build and test to the alternation.
2. The off-registry check excluded any line CONTAINING
`registry.npmjs.org`, so a lookalike host — registry.npmjs.org.evil.com,
or an @-userinfo form — read as npmjs and passed. The tarball's own
postinstall lives inside the tarball, never in the diff, so this arm
was the only mechanical defense against it. The exclusion is now
anchored to scheme + host + '/'.
3. A routing assertion read `.runner`, a key from the earlier
ephemeral-VM design that `gate()` never returns, so it was
`expect(undefined).toBeUndefined()` — green no matter how badly the
trust level leaked into /triage or /tmux. Asserts the real keys now,
on both non-verify paths.
4. Three screen arms had no scenario at all: package-manager config,
the long-opaque-line awk, and the unconfigured-model fail-closed
path. All three are covered, each with a negative control so the arm
is not merely refusing everything — the genuine registry URL and a
700-char lockfile integrity line must still pass.
Mutation-verified 6/6, each failure naming the payload it let through:
reverting the alternation (`prebuild was not flagged`), un-anchoring the
registry match (`registry.npmjs.org.evil.com was not flagged`), breaking
the package-manager grep (`.yarnrc was not flagged`), raising the awk
threshold, letting the unconfigured path fall through to clear, and
leaking verify_trust into /triage — which is exactly what the vacuous
assertion could never see.
93/93 tests; prettier, eslint and shellcheck clean.
* fix(triage): close post-run persistence gap and screen bypasses (#7985)
Maintainer review found the wipe only ran BEFORE external code, leaving
the next pool job to meet the allowlist sweep alone. Add a post-run wipe
with `if: always()` so a cancelled or timed-out external job still
cleans up.
The model screen silently truncated at 200 KB — a fail-open in a
fail-closed step. Refuse oversized diffs mechanically before the model
call.
The screen prompt still described an "ephemeral, credential-free
sandbox" from the deleted ephemeral-runner design, calibrating the
model for leniency the persistent pool does not warrant. Describe the
actual environment. Also fix the stale "hosted lane" comment.
Three cheap mechanical-screen hardenings from the review: drop the
root anchor on the package-manager config grep (a nested .npmrc is
just as dangerous), match `rename to` headers (a rename-only hunk
emits no `+++ b/` line), and relax the opaque-line awk from
"no space in the line" to "no space-free field >= 600" so a single
space anywhere is no longer a bypass.
94/94 tests; build, typecheck, lint clean.
* test(triage): cover the unfetchable-diff fail-closed screen guard (#7985)
---------
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@service.alibaba.com>
|
||
|
Shaojin Wen
|
d0481ad884
|
feat(triage): lead the verify comment with a qualitative verdict, fold the Chinese (#7974)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 1/2 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 2/2 (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
SDK Java / ubuntu-latest / Java 11 (push) Waiting to run
SDK Java / ubuntu-latest / Java 17 (push) Waiting to run
SDK Java / macos-latest / Java 21 (push) Waiting to run
SDK Java / ubuntu-latest / Java 21 (push) Waiting to run
SDK Java / windows-latest / Java 21 (push) Waiting to run
SDK Java / Real daemon E2E / Java 11 (push) Waiting to run
* feat(triage): lead the verify comment with a qualitative verdict, fold the Chinese Maintainer feedback on the first 14-run day of the lane, pointing at the report on PR #7836: give a plain pass/no-pass, and make the layout English-by-default with the Chinese folded (the repo's PR-body convention), instead of doubling every head line. Three changes: - every headline leads with a qualitative call — ✅ passed / ❌ not passed / ⚠️ inconclusive / ⚠️ incomplete — ahead of the lane taxonomy. The mapping is honest about what each outcome can claim: only agent verdicts judge the code; a crashed, timed-out, or verdict-less run renders as incomplete/inconclusive, never as a pass or a fail, and a test pins that no process-outcome arm carries ✅ or ❌. - the Chinese version folds into one <details> whose SUMMARY carries the qualitative verdict — collapsed, a Chinese reader still sees 通过/不通过 in one line. English scope and assertion count stay unfolded; both language assertion lines are built from the same validated triple, so inconsistent JSON still suppresses both. - the #7836 root cause gets a Hard rule in the verify-pr skill. That report said "merge-ready — the 7 failures are all expected A/B base-cell failures proving the tests load-bearing" while assertions.json said fail:7, so the publisher's trust rule (merge-ready requires fail==0) correctly refused it and the headline degraded to "no usable structured verdict". Both sides told the truth about different questions. The rule fixes the semantics: an A/B control cell is an assertion that the base arm FAILS — when it does, that assertion PASSED. fail counts only unexpected outcomes, which is what makes a qualitative headline trustworthy at a glance. Mutation-verified 6/6: dropping the qualitative prefix, collapsing all Chinese quals onto one string, unfolding the Chinese scope, giving timeout a ✅, dropping the folded Chinese assertion line, and deleting the new Hard rule each turn at least one test red. The fifth mutation initially reported "landed: False" — the regex never matched and the file was untouched, so the green result proved nothing; re-applied as a line filter with a removed-block count, it kills two tests. One new shellcheck SC2016 info finding (backticks inside a single-quoted Chinese printf), same shape as the 11 pre-existing ones; info-level, does not gate. * test(triage): pin the dropped verdict arms and prepare-failure glyphs (#7974) Restore rendering coverage for the reachable infra-error and unknown catch-all case arms in the qualitative-headline bijection, and assert the prepare-failure path's qualitative glyphs, Chinese fold summaries, and the Chinese retry clause so a swapped emoji or dropped PREPARE_ATTEMPTS_ZH interpolation can no longer ship undetected. * fix(triage): migrate all weak headlines, explain merge-ready mismatch (#7974) * fix(triage): fold Chinese mismatch explanation, differentiate distrust warnings (#7974) --------- Co-authored-by: wenshao <wenshao@example.com> Co-authored-by: Qwen Code CI Bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
qwen-code-dev-bot
|
324b5fba38
|
fix(release): skip notes-start-tag when previous release diverges from target (#7969) (#7970)
* fix(release): skip notes-start-tag when previous release diverges from target (#7969) * style(release): fix prettier formatting in release workflow (#7969) * fix(release): warn when notes-start-tag is skipped for a divergent tag (#7969) --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> |
||
|
易良
|
6b9b861398
|
fix(ci): update the Qwen binary used by ECS runners (#8000)
* fix(ci): install Qwen into system npm prefix * test(ci): update ECS runner workflow guard |
||
|
Shaojin Wen
|
f56303e3e1
|
feat(triage): make the not-verified sentence a mechanical 2b-bis trigger (#7965)
Post-merge measurement of #7917, one day in: 9 eligible PRs, two considered-and-declined mentions (both correct calls), zero positive recommendations. The one clear behavioural candidate — #7947, bounded reads of large text files — wrote "Not verified: Windows and Linux manual runs (author tested on macOS only)" in its own Stage 2 comment and never named a lane. That is the failure shape worth fixing: the judgement-based rule ("when neither static review nor 2b substantiates it") failed exactly where the comment had already written the gap down in so many words. The model judged that pending CI would cover it; a green suite proves the tests pass, not that the untested behaviour holds. So the trigger is now textual, not judgemental: before posting, grep your own draft. A sentence of the shape "not verified", "author tested on one platform only", or "author's claim, not independently re-run" IS the trigger — the 2b-bis line is that same sentence with the remedy attached, and omitting it means telling the maintainer what is missing while withholding the one command that would supply it. Pending CI does not lift the trigger. The two legitimate skip cases (nothing behavioural to settle; author lacks write) are unchanged, and the rule is ordered before them so they read as outs from the requirement, not the requirement as an out from them. The trigger phrases are verbatim from real comments: "not verified" and "author tested on macOS only" from #7947, "author's claim, not independently re-run" from #7951. Mutation-verified 3/3: dropping the trigger paragraph, moving it after the skip cases, and dropping the pending-CI sentence each turn the test red. n=1 is thin evidence for a behavioural rule change — but this rule is text-matching, not probability-weighing, so it cannot overfit to the sample that motivated it. Co-authored-by: wenshao <wenshao@example.com> |
||
qqqys
|
0c0ca5fed0
|
feat(autofix): defer suggestions after five change rounds (#7913)
Some checks are pending
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:docker - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 1/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 2/3 (push) Waiting to run
E2E Tests / E2E Test (Linux) - sandbox:none - shard 3/3 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 1/2 (push) Waiting to run
E2E Tests / E2E Test - macOS - shard 2/2 (push) Waiting to run
E2E Tests / cron-interactive E2E (nightly) (push) Waiting to run
E2E Tests / web-shell Browser Regression (push) Waiting to run
SDK Java / ubuntu-latest / Java 11 (push) Waiting to run
SDK Java / ubuntu-latest / Java 17 (push) Waiting to run
SDK Java / macos-latest / Java 21 (push) Waiting to run
SDK Java / ubuntu-latest / Java 21 (push) Waiting to run
SDK Java / windows-latest / Java 21 (push) Waiting to run
SDK Java / Real daemon E2E / Java 11 (push) Waiting to run
* feat(autofix): defer suggestions after five change rounds * test(autofix): execute deferred jq queries against fixture data (#7913) * fix(autofix): scrub control markers from deferred feedback reports (#7913) * test(autofix): execute actionable reviews and issue-level filters against fixtures (#7913) --------- Co-authored-by: qwen-code-dev-bot <269191875+qwen-code-dev-bot@users.noreply.github.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@service.alibaba.com> |
||
|
Shaojin Wen
|
ddf4b8875d
|
fix(triage): make the build-process guard diagnosable and zombie-aware (#7858)
* fix(triage): make the build-process guard diagnosable and zombie-aware
The first real /verify run to reach the agent step (job 30267953352) was
stopped by the guard I added to stop detached lifecycle children from
outliving their step:
::error::Processes owned by the build user survived; refusing to start
the agent.
That is all it printed. No pid, no state, no command line — so a genuine
threat and a harmless leftover were indistinguishable, including to the
person who wrote it. The control had no observability of its own.
Two changes, both in the verify and tmux lanes:
- name the survivors. The error now lists each one as pid, state and
command line, so the next occurrence can actually be diagnosed.
- disregard zombies. A zombie has already exited and released everything
except its exit status: it cannot re-plant an artifact or touch the
agent's inputs, and it cannot be killed either — so counting one means
the check can never clear, no matter how many times SIGKILL is sent.
The container runs no init to reap orphans, so they are expected here.
The platform difference is the reason the old form could hang: Linux
pgrep reports defunct processes ("Defunct processes are reported." —
pgrep(1), procps-ng), while macOS pgrep does not list them at all
(verified directly: ps shows our zombie, pgrep does not). So this failure
mode was unreachable in local replay and only appears on the runner.
That same difference shapes the tests. The behavioural arm constructs a
real zombie, confirms it survives SIGKILL, and shows the state filter
drops it — but it cannot discriminate the old implementation from the new
one on macOS, because pgrep never saw the zombie there in the first place.
So the portable guarantee is asserted structurally: the filter must read
process state and exclude Z, and must not be a bare pgrep. Mutation-
verified 3/3 — removing the state filter, dropping the survivor names, or
reverting the message each turn one test red.
Also gives both proxy-watchdog tests an explicit timeout. They stream 20
chunks at 200 ms — 4 s before the stall arm even starts — so they cannot
fit vitest's 5 s default and were timing out on main.
* fix(triage): tolerate zero-process exit in build guard (#7858)
---------
Co-authored-by: wenshao <wenshao@example.com>
Co-authored-by: Qwen Code Autofix <qwen-code-autofix@users.noreply.github.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
||
|
Shaojin Wen
|
25e357a279
|
feat(triage): make the verify report readable in Chinese (#7918)
* feat(triage): make the verify report readable in Chinese
The scope disclaimer under the verify headline has been bilingual since
the lane shipped. The verdict above it was not, and neither was the
assertion count — so a Chinese reader of the unfolded comment got the
caveat ("advisory evidence, not a review") and not the conclusion.
Three changes, all on the parts a reader reaches first:
- every headline carries a Chinese twin, across all eight arms —
merge-ready / findings / blocked / inconclusive from the agent, and
completed / fail / timeout / infra-error from the process outcome;
- the assertion count renders in both languages, off the same validated
object, so an inconsistent assertions.json still suppresses both and
the comment cannot grow a number no gate checked;
- in the report itself, 中文摘要 moves from last item to second, right
after the verdict. The whole report is already inside a <details> on
the PR, so leaving the Chinese summary at the bottom meant expanding
that fold and scrolling the entire English report — about 90 lines on
a real one — to reach the one section written for that reader. It
stays collapsed, so it costs everyone else exactly one line.
The headline test pins the pairing as a bijection, not as containment:
asserting only that each arm renders some Chinese leaves a single
hardcoded string — or one that echoes the English — passing every arm.
Mutation-verified 4/4: dropping the Chinese headline, collapsing all
arms onto one Chinese string, dropping the Chinese assertion sentence,
and moving 中文摘要 back to the end each turn one test red.
* fix(ci): correct cross-reference direction and cover all verdict branches in bilingual headline test (#7918)
* fix(ci): narrow bilingual headline comment to match actual scope (#7918)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
---------
Co-authored-by: wenshao <wenshao@example.com>
Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
||
|
易良
|
bfd4c8e519
|
fix(scripts): slim release-note model prompts and log request timing (#7941) | ||
|
Shaojin Wen
|
b6b55c598c
|
feat(triage): surface the sandboxed lanes on the CI path (#7917)
Measured on 2026-07-28 across 22 open PRs: of the 16 whose AUTHOR had write access and that already carried a triage comment, exactly 1 mentioned `/verify`. The lane it recommends has produced real evidence on four PRs (#7829 2565 assertions, #7821 1347, #7830 27, #7881 17), so the gap is not that the lane is useless — it is that almost nobody is told it exists. The instruction was there the whole time. It was a conditional clause inside a section headed "2c. Real-Scenario Testing — local invocation ONLY" whose first words are "Never in unattended CI." An agent running in CI reasonably skips that section, and the assembly order at the end of Stage 2 only allotted 2c a slot "when one was driven locally" — so on the CI path there was no place for the recommendation to go even if it had been read. Split the CI-path half out into its own section, 2b-bis, sited immediately after the CI-evidence step it follows from: 2b tells you the suite is green, and cannot tell you the suite pins the change. It is a required element of the Stage 2 comment when the central claim is behavioural, with two explicit skip conditions — nothing behavioural to settle, or the author lacks write (both lanes execute the author's code, so recommending them on an external contributor's PR is a guaranteed denial). It must name the specific unsubstantiated claim, because a bare "you could run /verify" is noise and noise is why the line got skipped. Stage 1e carried the same dead pointer, and worse: on the high-risk paths — the strongest triage-time signal in the skill, 10 of 31 reverted PRs against 5 of 60 controls, p = 0.006 — it recommended tmux alone and never named /verify. The PRs most likely to be reverted were the ones never offered the lane that proves a change is load-bearing. 1e now points at 2b-bis and names both lanes. The fix is positional, so the tests are positional: asserting that the file mentions `/verify` would have passed throughout the entire period the recommendation was dead. Mutation-verified 4/4 — moving 2b-bis back below the local-only heading, dropping it from the assembly order, dropping the author-write carve-out, and reverting 1e to the tmux-only pointer each turn a test red. Prose assertions are whitespace-normalised and shown to survive a maximal re-wrap, since prettier reflows this file. Co-authored-by: wenshao <wenshao@example.com> |
||
|
Shaojin Wen
|
43c35e533b
|
fix(ci): keep the post-merge E2E signal on main alive (#7795)
* fix(ci): stop cancelling in-progress E2E runs on main Merges land on main roughly every 18 minutes (median) while a full E2E run takes about 40, so cancelling the in-flight run on every push starved the suite: across the last 100 push runs, 67 were cancelled and only 25 ever reported a result. The nightly regression was the only reliable signal, and each cancelled run still burned roughly 10 minutes on three runners before dying. Turning cancellation off for main hands the coalescing to GitHub's concurrency queue, which keeps at most one pending run per group and cancels the previously pending one. The queue therefore collapses to the newest tree on its own while the in-flight run always finishes, so every result covers the batch of commits merged since the last one — bisect that range when it goes red. Dev branches keep cancelling superseded runs, where only the latest push matters. * fix(ci): dedupe main CI failure issues by failing test The autofix issue for a red main was deduped on the commit SHA, so a standing failure opened a brand new issue on every merge: one broken E2E test on 2026-07-26 produced six duplicate issues in twelve hours, each pointing the autofix agent at an unrelated commit. The failing tests are now identified from the logs of the failed jobs and used as the dedupe key, with one marker per test so a failure set that grows still matches the issue that already tracks part of it. Later commits hitting the same failure are appended to that issue as recurrences (bounded, newest first) instead of opening another one, and notes written by a human or the agent are preserved when the machine-owned trailer is refreshed. Runs with no identifiable test — an install or build break — keep the previous per-commit behaviour. * fix(ci): keep the failure analysis out of the bot-PAT job Identifying the failing tests means running a helper from the repository, and the workflow deliberately checked out nothing so that the job holding the bot PAT could never execute repository code — an invariant its own test enforces. Rather than weaken it, the work is split: a read-only job checks out the tree, reads the failed run's logs, finds any issue that already tracks the failure and renders the title and body, handing both over as outputs. The job with the PAT keeps checking out nothing and only writes what it was given. The invariant test now asserts that separation — the PAT job runs no repository code and holds only issue write — instead of banning checkout everywhere in the file. * fix(ci): harden main CI failure dedupe per review (#7795) - Match the `[run <id>]` link text instead of the run URL when deduping recurrences: `/301` is a substring of `/3010`, so the URL match silently deleted an unrelated run's line. - Rebuild the machine-owned "## Also failing" section from the live failure set on every merge so a test that has since been fixed drops out instead of being listed forever. - Assert the analyze checkout is SHA-pinned with persist-credentials disabled and pin the failed-job log-download paths in the workflow test. - Add an e2e workflow test guarding the cancel-in-progress expression that keeps in-progress runs on main from being cancelled. * fix(ci): bound the issue body and randomize the heredoc delimiter (#7795) * fix(ci): test the runCli --existing merge path (#7795) * fix(ci): address review feedback on e2e signal PR (#7795) - Assert the full cancel-in-progress expression including && so a mutation to || is caught by the e2e-workflow test - Filter the capped-summary line from missingTests so it is not rendered as a fake bullet under Also failing --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.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@service.alibaba.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> |
||
|
易良
|
45a6a69cf0
|
feat(triage): add revert-pattern high-risk path detection (#7414)
* feat(triage): add revert-pattern high-risk path detection Replace the behavior-neutral PR filter (PR #7414 v1, ~2% hit rate) with a data-backed triage gate based on revert-history analysis of 111 revert commits and 46 unique reverted PRs in this repo. Stage 1e checks three signals identified by the analysis: - touches_high_risk (66.7% precision, 32.3% recall) - contested-merge pattern (50.0% precision, 19.4% recall) - non-maintainer + high-risk (58.3% precision, 22.6% recall) The gate escalates review depth and recommends maintainer sign-off; it never blocks or closes PRs. Design doc and analysis scripts included. * fix(triage): avoid stale-exempt hold label * fix(triage): address review risk detection feedback * fix(triage): tighten high-risk path patterns * fix(triage): address review feedback on Stage 1e revert-pattern gate (#7414) * fix(triage): address round-2 review feedback on Stage 1e gate (#7414) - Fix APPROVE → APPROVED state name (GitHub API enum) - Use gh api --paginate for file listing (fixes 100-file truncation) - Anchor shell/relaunch/sandbox patterns with (^|/) to avoid false positives - Append || true to grep (exit 1 on no match is the 92% case) - Scope E2E recommendation to write-access authors per Stage 2c - Add bot author filter to contested-merge query - Define core paths explicitly in contested-merge condition - Wire Stage 1e do-not-auto-approve into Stage 3 guardrail - Replace precision percentages with p-values/raw counts in skill text - Add sampling caveat and statistical significance notes to design doc - Fix design doc errors: 71%→61.5%, 10→8 PRs, Rule 3 attribution, ~20% baseline→10% prevalence, Area field, no_e2e inconsistency - Make test assertions specific to Stage 1e (not vacuous) - Add Risk: template field assertion - Revert drive-by prettier reflow - Note need-discussion label removal by maintainer * fix(triage): address round-3 review feedback on Stage 1e gate (#7414) - Separate gh api call from grep so API failures are visible instead of being masked by || true (rc:3660753982) - Include author identity in contested-merge jq output and require different reviewers for the disagreement check, avoiding false positives from same-reviewer iteration (rc:3660753990) - Add Stage 1e to the approval summary checklist so it is not omitted from the pre-approval conditions (rc:3660753994) * fix(triage): address round-4 review feedback on Stage 1e gate (#7414) * fix(triage): use portable ERE grep for test-file exclusion (#7414) * fix(triage): guard deferred approval on discussion label * fix(triage): keep only supported revert signal --------- Co-authored-by: qwen-code-bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
易良
|
47118be482
|
feat(ci): Deduplicate E2E failure issues by commenting on existing issue (#7792)
* feat(ci): deduplicate E2E failure issues by commenting on existing issue * fix(ci): address failure issue review feedback * test(ci): cover CI issue dedup invariants * fix(ci): include comments in failure marker dedup * fix(ci): include issue comments in failure marker search * fix(ci): share failure issue title prefix * fix(ci): filter dedup issue titles * test(ci): assert SHA dedup runs before workflow-name dedup (#7792) * fix(ci): limit SHA dedup search to issue bodies * fix(ci): verify bot ownership before reusing title-matched issue (#7792) --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
ec62797c1c
|
fix(review): give the review retry the remaining time budget (#7852)
* fix(review): give the review retry the remaining time budget A transient API failure on the first review attempt was retried with a hardcoded 5-minute timeout while the first attempt got the whole 180-minute budget. The retry does not resume the failed review, it re-runs it from scratch, so on a large PR it spends those five minutes re-fetching the PR worktree, re-chunking the diff and re-launching review agents and is then killed on the clock before producing a single finding. Every retry after a transient failure therefore reported a timeout, and the resulting comment advised raising --timeout, which does not affect the retry cap at all. All attempts already share the single-review budget, so that budget is the only bound the retry needs. Give every attempt the remaining budget and gate the retry on having enough of it left to plausibly finish; below that, report the transient failure so the next run starts over with a full budget. * test(review): pin the per-attempt review timeout budget The retry-loop harness stubbed timeout with `shift; shift; exec "$@"`, which discarded the duration argument, so no test could observe how much budget each attempt was given. That is the invariant the retry fix turns on: a reintroduced per-attempt cap, or a miscalculated remaining budget, would have left the suite green while making every retry unusable. Record the duration the stub receives and assert on it. Cover the retry gate from both sides as well, since every existing scenario runs with a 180-minute budget and none of them exercise the threshold: a budget just under it must not start a retry that cannot finish, and one just over it must still retry. |
||
|
Shaojin Wen
|
203093c53f
|
fix(triage): retry a transient npm ci before blaming the PR for it (#7884)
Run 30319209722 posted `Sandboxed verification: fail` on PR #7856 with "The PR could not be built ... treated as a PR failure verdict rather than an infrastructure failure." The PR changed six source files. The install died here: Error: spawnSync .../web-templates/node_modules/esbuild/bin/esbuild ETXTBSY at validateBinaryVersion (esbuild/install.js:99:28) ETXTBSY is npm writing a dependency's binary and that package's own install script exec'ing it before the write is closed — a race, and one the PR had no part in. The comment accused its author anyway. Both sandbox lanes now retry `npm ci` once. `npm ci` removes node_modules before installing, so the second attempt cannot inherit the half-written file; the transient class is simply absorbed instead of being classified. Retrying rather than classifying is the point. The obvious fix — match ETXTBSY in the log and downgrade to infra-error — would read text the PR's own lifecycle scripts can print, which is exactly the forgeable signal the verdict logic was rewritten to stop consulting: it let a PR launder its own deterministic breakage into "infrastructure, please re-run". A retry consults nothing. A genuinely broken tree fails twice and still earns `fail`, and the second attempt is only ever paid on a path that is already failing. The build is deliberately not retried: a compile error is deterministic, so a second run would only double the cost of an honest failure. Because the install now gets two chances, the sentence that blames the PR says so — "failed twice in a row". PREPARE_ATTEMPTS is initialised rather than defaulted at the point of use, or an inherited value would claim a single-shot `npm run build` had failed twice, which is the same false accusation pointed the other way. Mutation-verified 5/5: an unbounded retry, no retry at all, a retry on the build, dropping the initialisation, and reverting the wording each turn at least one test red. The retry itself is covered behaviourally — the real step text runs against a stubbed `runuser`/`npm`, so the tests count actual install attempts instead of asserting that a loop exists, which would pass on both a loop that never retries and one that never stops. Co-authored-by: wenshao <wenshao@example.com> |
||
|
易良
|
923e5ab425
|
fix(scripts): harden retry classification and preserve deadline error context (#7854)
* fix(scripts): harden retry classification and preserve deadline error context Follow-up to #7535 addressing remaining review suggestions: - isRetryableModelError: content-validation errors ('Model response did not contain message content.') are now non-retryable. These are deterministic failures from our own code — retrying the same prompt reproduces the same failure. Network-level errors (no HTTP status) remain retryable. - Deadline-expired errors now include the original error message instead of the generic 'budget exhausted', so oncall can see the root cause. - Added 4 tests: HTTP 429 retry, network error retry, content-validation non-retry, deadline error preservation. * fix(release): preserve last model error across retry budget * test(release): cover retry budget expiry after backoff * refactor(release): extract deadline error helper and preserve cause Consolidate the three identical budget-exhausted throw sites into a single deadlineError() helper so they share one message format and source variable, and attach the last model error as the Error cause so its stack trace survives into CI logs. Also conform the release-notes tests added by this series to Prettier. |
||
|
qqqys
|
c994524a2d
|
feat(autofix): retry deterministic rejection once (#7796)
* feat(autofix): retry deterministic rejection once * fix(autofix): use repaired verification result * fix(autofix): preserve repair safety context * test(autofix): cover empty verification outcomes |
||
Yufeng He
|
e5df5b22e3
|
fix(scripts): retry model calls and surface degraded release notes (#7535)
* fix(scripts): retry model calls and surface degraded release notes The stable-release notes generator fell back to pull-request titles on any model error and the finalize workflow stayed green with no visible degradation. Four recent stable releases lost every summary batch and the highlights call to 60s timeouts without anyone noticing. The completer now retries each request (default 2 retries, exponential backoff with jitter) on timeouts, network errors, 429, and 5xx, while 4xx fails immediately. After three consecutive failed summary batches a circuit breaker stops paying the timeout for the rest and skips the highlights call, since the model side is down rather than slow. Fallback warnings are emitted as :⚠️: workflow commands so they render as run annotations, and a degraded-run block is appended to GITHUB_STEP_SUMMARY when any fallback occurred, distinguishing fully degraded (titles only) from partially degraded runs. Refs #7523 Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> * ci: retrigger after flaky fork-dispatch test failure * fix(scripts): create the step-summary parent dir before appending The ubuntu CI shard of this PR failed the summary test with ENOENT on readFileSync after appendFileSync: the append reached the runner fine, but the parent of the summary path can be missing there. Create it recursively first, which is what the function's contract (the note must land in the summary file) already implies. * fix(scripts): assert the degraded summary through the fs mock test-setup.ts no-op mocks appendFileSync for the whole scripts project, so the new test's real-file read could never see the write under test:scripts. Assert on the mock call instead. * fix(scripts): keep summary-write failures from costing the notes and make the reset test real appendDegradedStepSummary now runs through tryAppendDegradedStepSummary so a filesystem error on the auxiliary summary cannot kill the release before the notes are written, and the circuit-breaker recovery test's mock returns valid summary payloads so the reset path is actually exercised. * fix(scripts): escape model-controlled text in workflow commands Warnings can carry model output (parse errors, PR-derived fields) into :⚠️: commands. Percent-encode %/CR/LF so a forged ::error:: line cannot emit a second runner command, and collapse newlines in the step-summary markdown list so a multi-line warning does not break the list. Closes the P1 from PR review. * fix(scripts): bound release-note AI generation time * fix(scripts): log model request retries to stderr Retry attempts in createOpenAiCompleter were silent, so an oncall engineer could not distinguish slow model responses from exponential- backoff retries in the CI log. Emit a one-line stderr log on each retry with the attempt count, the triggering error, and the backoff delay. Plain stderr (not :⚠️:) so transient retries that recover do not become run annotations. * fix(scripts): escape retry log workflow commands --------- Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: yiliang114 <effortyiliang@gmail.com> |
||
|
易良
|
ad2252401b
|
ci(release): comment released-in version on merged PRs (#7814)
* ci(release): comment released-in version on merged PRs After a stable release is published, comment on each merged PR in the release range with a link to the release tag, so contributors can find which version shipped their change directly from the PR timeline. Collects PR numbers from squash-merge commit subjects between the previous and current stable tags, posts a marker-tagged comment, and skips PRs that already carry the marker (re-run safety). Nightly and preview releases are out of scope, since finalize only handles stable tags. * fix(ci): narrow release PR extraction * test(ci): cover release comment safety checks |
||
|
Shaojin Wen
|
2e08486b52
|
fix(triage): carry the /verify lane's hardening across to /tmux (#7753)
* feat(triage): add sandboxed /verify deep-verification lane @qwen-code /verify on a PR now runs a local-verification-style evidence round in the isolated /tmux sandbox contract (container, token-free agent env, loopback model proxy, author-write gate) and publishes the report via a separate PR-code-free job: - new verify job: merge-ref checkout at depth 2 (base tip + PR head for A/B), skills pinned from base so the tree under test can never rewrite its own verifier, PR-planted tmp/*-verify-* artifacts dropped, git exec-vector sweep for the persistent workspace, agent verdict allowlisted before it reaches workflow outputs - new publish-verify job: upserts one marker comment (running status -> final report), HTML-escapes the untrusted report, reports skip/na/ prepare-fail/infra outcomes explicitly since /verify is always an explicit request - new verify-pr skill: A/B load-bearing proof, vacuity check on new tests, mock-free wire-oracle harnesses, targeted gates, fixed report/ verdict/assertions artifact contract, counts-are-sacred rules - triage skill Stage 2c now names /verify (not just /tmux) as the trigger to recommend when a PR's central claim needs behavioral evidence The verify check-runs ride the issue_comment event, which the finalize workflow's event == "pull_request" universe structurally excludes, so they cannot pollute the CI table or the deferred-approval gate. * feat(triage): teach /verify round continuity and artifact-matched methods Fold two more hand-verification patterns into the verify lane: - round continuity: the resolve step snapshots the previous verify report (if any) into the agent context before the status upsert overwrites it, and the skill re-checks each prior finding at the new head (fixed/stands/superseded), scoping new probes to the delta - harness quality: prefer configuration seams over module interception, encode the upstream's real semantics in the fake peer, add decoy targets - artifact-matched methods: per-commit load-bearing tables for multi-commit PRs; workflow/CI PRs get embedded-script replay against real data, repo lint gates, and day-one trigger cost math from real event history; every new config knob must trace to an observable effect, and default-path dispatch combinations get probed - findings quality: blockers enumerate blast radius, demonstrate the sharpest consequence end-to-end when budget allows, and carry a collapsed minimal suggested fix preserving the original commit's intent * feat(triage): host /verify evidence images and encode quantified-A/B rules Borrow the image-evidence and quantified-verification patterns from hand-run rounds (#7265, #7471, #7686 r2 and the pr-assets convention): - publish-verify now hosts agent-produced evidence/*.png on the pr-assets branch (verify/pr<N>-<run>-<attempt>/) and appends them below the escaped report. Untrusted-payload discipline: strict filename allowlist, 8-image / 2 MB caps enforced in the find predicates, racing-push retry, and every failure degrades to a text-only comment. VERIFY_ASSETS_REMOTE is a test seam; the block was dry-run against a local bare remote covering hosting, hostile filenames, oversize files, dotfiles, missing branch, and no-image runs - skill: evidence images are named as kebab-case captions binding image to claim, before/after pairs over lone after-shots; follow-up rounds lead with a previous-finding status table (fixed/stands/superseded/declined, with adjudication) and re-measure instead of diffing the old report; size/perf claims get measured-metric Δ tables with residual deltas accounted for; unreachable branches get the configuration that reaches them constructed; defensive guards get their accept path checked against real production artifacts, not just mocked rejects * fix(triage): address /review suggestions on the verify lane - skill: local invocation resolves --repo and passes it to every gh call - skill: call out the dependency confound when the base A/B side reuses the PR-installed node_modules and the PR touches package.json/lockfile - workflow: document the pin step's bootstrap logic — issue_comment jobs run the default branch's YAML, so base always carries the verify-pr skill by the time this job exists * fix(triage): harden /verify gate, comment budget, and evidence hosting per review Address review round 5078770575 items 1-3 plus the cheap follow-ups: - authorize: /verify now requires write from BOTH the PR author (whose code runs) and the commenter (who spends a scarce runner slot + model budget) — a drive-by account can no longer burn 45 minutes of ecs-qwen on someone else's PR; duplicates check once; /tmux and /triage gates unchanged. Replayed 8 principal scenarios against a stubbed gh - authorize acks /verify with the eyes reaction from the always-hosted job, so a queued/saturated sandbox pool no longer means total silence - publish: emit_block escapes FIRST and caps the escaped size (45 KB for the report) — a raw-side cap let dense <>& content inflate past GitHub's 65,536-char comment limit, 422 the post, and strand the running status with no report at all; iconv -c keeps a UTF-8 sequence split by the byte cut (likely, given the mandated 中文 summary) from shipping broken; replayed: 50 KB dense report -> 45,873-byte body - publish: image cap is byte-exact (-size -2097153c; find's -2M rounds sizes UP to MiB, silently making the documented 2 MB cap 1 MiB), bytes must carry the PNG magic (extension is attacker-choosable), duplicate sanitized names dedupe instead of overwriting + double-rendering, and dropped images are reported in the comment instead of vanishing - publish: weak terminal notices (cancelled/infra/skipped/n-a) only replace this run's own running status; a previous round's real report survives as the marker comment and the notice posts fresh - publish: report.md/assertions.json lookups pin the artifact-dir shape and sort (bare find -name order is filesystem-dependent); the verify job's verdict.txt lookup sorts likewise - verify: global npm install runs from RUNNER_TEMP (the persistent workspace still holds the PREVIOUS run's tree, whose .npmrc would apply to a root install); both cleanup passes remove leftover tmp/ worktrees (git worktree prune alone only drops metadata); the run step no longer re-chowns 50k node_modules files; pr-assets clone sets its committer identity once so the racing-push rebase retry can commit - skill: worktree guidance now tells the agent to remove its base tree itself, with the workflow sweep as backstop only * fix(triage): close runtime-plant and stale-RUNNER_TEMP channels in /verify Address review round 2 (comment 5079157987) and the CHANGES_REQUESTED round on the verify lane: - run step re-sweeps tmp/*-verify-* AFTER npm ci/build and before the agent starts: the pin step's sweep runs before PR lifecycle scripts (postinstall etc.), which could re-plant a fake artifact dir whose zeroed timestamp deterministically wins the sorted collector. From the sweep on, only the agent writes those dirs; a steered agent forging its own artifacts remains the documented advisory-report residual - RUNNER_TEMP verify-results/verify-context are rm'd before mkdir: the pool is persistent and runner temp hygiene is runner-managed — a stale report or previous-report.md from ANOTHER PR must never ride along - symlinks are stripped from verify-results before upload: actions/upload-artifact dereferences them, so a node-planted link would exfiltrate whatever it points at into the artifact - a trusted commenter invoking /verify on a PR whose author lacks write now gets an explanation comment from the hosted authorize job instead of total silence (the commenter is checked first; drive-by accounts and API errors still get nothing); job timeout 45->60 so a slow install can never let the JOB limit kill the agent past its own graceful 25m budget - stale tmp/base-tree (skill's canonical scratch worktree) is removed by name at job start — a plain dir isn't git-registered, so the worktree sweep alone misses it and the next worktree add would fail - scripts/tests/qwen-triage-workflow.test.js gains a verify-lane describe block: an 8-arm stub-gh replay of the dual principal gate (drive-by deny, author-without-write deny + explain flag, self-comment dedupe, 404 fail-closed, /tmux and /triage unchanged) plus guards for the post-prepare sweep placement, the symlink strip, and the RUNNER_TEMP resets — the replay found this commit's sweep edit had silently not applied, which is exactly the regression class it exists to catch * fix(triage): close proxy-hijack, gate-bypass, and false-verdict paths in /verify Address the Codex /review round (19 findings) and the bot's follow-up. Each fix was replayed locally; the proxy fix has a decisive A/B. Gate and routing: - the shell command match is case-insensitive: GitHub Actions expression comparisons ignore case, so `@QWEN-CODE /VERIFY` reached the step and fell through to the commenter-only branch — running the PR author's code with the author never checked - the verify ack and denial notice require github.event.issue.pull_request: /verify on a plain issue was acknowledged but could never report - publish-verify joins the verify job's per-PR concurrency group, and a failed PATCH falls back to posting fresh instead of going silent Untrusted-input paths: - the model proxy binds an EPHEMERAL port, reports it through a root-owned file, and its health check must echo a per-run nonce with the recorded PID alive. A/B with a squatter on 8787: the old code's proxy dies EADDRINUSE yet still reports enabled and points qwen at the squatter; the new code comes up unaffected on an ephemeral port - worktree-scoped git config is deleted before hooksPath is resolved: `extensions.worktreeConfig` is allowlisted and .git/config.worktree is invisible to `git config --local`, so a prior run could set core.hooksPath=/ and make the hook sweep's recursive delete walk / as root (verified locally). The sweep now also refuses any hooks path outside the repository's git dir - marker-comment lookups accept only bot-owned comments that START with the marker: any user can paste the marker and divert the bot into PATCHing a stranger's comment - the upload staging dir is re-flushed after npm lifecycle scripts Honest verdicts: - the docs-only classifier no longer uses a pipeline (grep -q made the writer take SIGPIPE, so under pipefail a long file list with an early code file classified a code PR as docs-only and skipped verification), and executable markdown/YAML (.qwen, .github/workflows, scripts) is classified as behavioral before the extension rule - tee's status is checked alongside qwen's: a full results volume made a truncated evidence stream publish as pass - 137 is split by elapsed budget into watchdog timeout vs crash/OOM - the agent's verdict is honored only for VERDICT=pass with a report and zero failed assertions; otherwise the process outcome headlines and the scope paragraph says the run did not complete - verdict.txt is read through a bounded prefix (SIGPIPE under pipefail) Skill contract corrections: per-commit tables only when the commits are reachable at depth 2 (else aggregate + Not covered); internal workspace symlinks must have their realpaths asserted before a base control is trusted; repo lint gates and event-history cost math are qualified to what the token-free container can actually run; --repo is never inferred from `origin` (a fork holds a different PR under the same number). Tests: 9 new guards, all mutation-verified (reverting each fix turns one red), including an executable escaping/size-cap/UTF-8 test for the publisher's own emit_block and a fix to the earlier command-file test, which matched the tmux job's identically named step. * fix(triage): re-establish the /verify trust boundary after PR code runs Third review round (31 findings). The unifying defect: everything the lane pinned or swept happened BEFORE npm ci/build executed PR-authored lifecycle scripts as node, so each control could be undone afterwards. Trust boundary, rebuilt in order before the agent starts: - kill every process owned by the build user and fail the step if any survives — a detached postinstall child could otherwise wait out each one-shot sweep and re-plant afterwards - re-pin .qwen from the base commit again, now root-owned and read-only: the prepare step chowns the workspace to node, so a lifecycle script could rewrite the very skill that defines /verify-pr - give the agent a fresh HOME/QWEN_HOME: qwen loads user-scope file commands from $HOME/.qwen, and /home/node belongs to the build user, so a planted commands/verify-pr.toml could shadow the pinned skill - the model proxy now requires a per-run bearer token, closing the blind-localhost-scan path to an unauthenticated signer for the real model credential (a command the agent itself launches still inherits it — documented residual, not closed) Authorization and lifecycle: - re-verify the PR author's write permission at execution time and pin the authorized head OID; refuse if the checked-out HEAD^2 differs, so a push during the runner wait cannot smuggle in unreviewed code - validate each principal separately: an empty author vanished in word splitting and left only the commenter checked - honor MAINTAINER_ECS_RUNNER_DISABLED with an explicit notice instead of queueing forever against a disabled pool - status comments carry a machine state marker; inferring 'running' from prose let a report quoting that sentence be overwritten - previous-report.md snapshots the newest substantive report, never a weak/cancelled notice, so prior findings survive into the next round - bot-identity lookup failures fail closed instead of widening the ownership filter to every user's comments - publish-verify uses a per-run concurrency group: a per-PR group holds only one pending job, so a second /verify could cancel a completed run's pending publisher Correctness: - install/build failures are classified: signals, ENOSPC, registry and network errors are infra-error, not a PR verdict - watchdog classification measures the child's own elapsed time, not shell-global $SECONDS which includes proxy setup - assertions.json must be three non-negative integers with a positive total and total == pass + fail before it counts as evidence - the proxy keeps its upstream deadline armed until the body ends and aborts upstream when the client disconnects - cleanups remove .qwen/tmp itself: PR code can make it a symlink, and globbing below it deleted the target's contents as root (verified) - emit_block materializes the escaped text and truncates on a character boundary via node — iconv -c passes an incomplete trailing sequence through on BSD (measured), which the new test caught Skill: local mode requires the same isolation CI provides and must not assume HEAD^1/HEAD^2 on a plain head checkout; shallow boundaries make rev-list counts unreliable for per-commit claims; never run scripts/lint.js with no arguments (it runs prettier --write and rewrites the tree under the harnesses); a vacuity check must fail the intended assertion, not the import. pr-workflow.md now says both sandboxed lanes need the author to have write, so triage stops recommending a guaranteed denial on external PRs. Tests: 9 more guards, all mutation-verified, including executable replays of the docs-only classifier (SIGPIPE + executable-markdown cases), the uppercase-command gate, the empty-principal deny, and the untrusted-image hosting path against a bare pr-assets remote. * test(triage): pass the classifier fixture through a file, not argv The new docs-only classifier replay passed on macOS and failed on CI with `Cannot read properties of undefined (reading 'trim')`: its 60,001-entry fixture is ~889 KB and was passed as a single argv element. Linux caps one argument at MAX_ARG_STRLEN (128 KB), so the spawn failed with E2BIG and stdout was undefined; macOS has no per-argument limit and only a ~1 MB total, so the same call succeeded locally (verified both). Write the list to a temp file and pass the path. The harness now also asserts the spawn succeeded, so a future spawn failure reports itself instead of surfacing as a TypeError on undefined output. * fix(triage): make the /verify report match what the run actually produced Three publisher findings, all introduced by my own previous round: - an artifact download failure (the step is continue-on-error) let the full-report path run with no results: the headline read 'completed' and the scope paragraph claimed the A/B, the harnesses and the gates had run when nothing had been delivered. The download outcome is now an input, and its failure gets its own body saying the results could not be retrieved - the prepare-failure branch ignored the verdict the prepare step had just computed, so an install killed by a registry outage or OOM (classified infra-error) still told the author 'this is treated as a PR failure verdict rather than an infrastructure failure' — the exact opposite. It now branches on the verdict, and an infra-classified prepare failure is a weak body that cannot overwrite a real report - weak notices were being snapshotted as the follow-up round's previous-report.md: they lack the running marker, so 'newest non-running comment' selected them. Bodies that carry findings now mark themselves (qwen-triage:verify-substantive) and the snapshot selects on that marker. A/B on the real jq: report A then cancelled B now snapshots A (101), the old filter picked B (102) Tests: 4 more guards, all mutation-verified — the publisher is rendered for each outcome with a stubbed gh and the assertions read the body it would post, and the snapshot test runs the workflow's own jq program verbatim against a paginate-shaped fixture. * fix(triage): stop PR build output from masquerading as an infra failure Two review findings plus a test-helper hazard: - classify_failure grepped the prepare log for bare words like ENOSPC and ETIMEDOUT, but that log is written by PR-controlled code: a genuine build failure that merely prints 'expected ETIMEDOUT to equal ok' would be published as an infrastructure incident, telling the author to re-run something that fails identically. The patterns are now anchored to lines only npm's reporter or the kernel emits ('npm ERR! code E…', 'npm ERR! network …', kernel OOM, bare 'Killed'); a signal exit still needs no log evidence. Replayed 10 cells: four PR-authored logs quoting infra words stay 'fail', five real diagnostics and one signal exit are 'infra-error' - the two execution-time controls added last round — re-verifying the author's permission after the runner wait, and refusing a head that moved since authorization — had no tests. Both are now executed: the re-auth snippet against a stubbed permission API (write proceeds and pins head_oid; read skips with a publishable reason), and the pin step against a real git repo with a real merge commit (matching head proceeds, moved head exits non-zero) - add a stepIn(job, step) test helper. Several step names exist in both the tmux and verify jobs, and the unscoped step() returns the first match, so a verify-lane assertion silently tests the tmux copy — that has now bitten this suite three times, including in this commit. * docs(triage): teach verify-pr test-only PRs, differential oracles, gate liveness Fold techniques from the round-2 verification on #7620 (an ANSI parser PR) that the skill had no equivalent for: - test-only PRs get their own method: a mutation A/B across TEST FILES (same mutants of the unmodified production file, only the test file swapped), reporting killed/total on both sides, requiring that no mutant regressed from killed to survived, checking that the killing assertion is the one the commit claims to have strengthened, and adjudicating every survivor as coverage gap or defect with independent evidence rather than by inspection - when the code emulates a known implementation, that implementation is the oracle: feed identical input to both and report disagreement counts per side, lift reference tables verbatim out of the shipped dependency, and build the corpus from bytes captured off a real producer alongside synthesized sweeps - prove a gate is live before citing it: plant a violation the linter must catch, confirm it is reported, remove it — a linter that matched no files exits 0 exactly like one that passed - attribute pre-existing failures by byte-identical failing file AND test names on both sides, with deltas, not just totals - when the base is far behind, verify the merge: trial-merge into current main, confirm it is conflict-free, and re-run the affected suite on the merged tree - round continuity gains its one legitimate shortcut: a production file proven byte-identical (sha256 quoted at both heads) carries prior evidence forward by construction * style(triage): reflow verify-pr skill to prettier's markdown wrapping The previous commit's added paragraphs were hand-wrapped and prettier --check flagged the file; the repo runs prettier over all of it. * test(triage): cover the disabled-runner-pool notice The kill-switch path had no test: a refactor could drop the notice and leave a /verify request acknowledged with 👀 but permanently unanswered, since the verify job refuses to start and publish-verify skips with it. Fold the step into the existing PR-guard loop (now scoped through stepIn, so it cannot match a same-named step in another job) and assert the parts that make the answer useful — the kill-switch and permission conditions, both languages, the alternative it points at, and the verify job's own exclusion of the disabled pool. All three mutations turn it red: removing the step, dropping its PR guard, or letting the verify job queue against the disabled pool. * fix(triage): repair a step-killing PIPESTATUS read and six forgeable controls Sixth review round, 12 findings. Several are regressions from my own two previous rounds; the first would have broken every single run. - `AGENT_STATUS=${PIPESTATUS[0]}` is itself a command and resets PIPESTATUS, so the next line's ${PIPESTATUS[1]} was unset and `set -u` aborted the step immediately after the agent finished — before artifact collection, the verdict, or anything else. Verified by replaying the exact structure: 'PIPESTATUS[1]: unbound variable'. Both elements are now snapshotted in one command - concurrency predicates were broader than the job conditions they guard, and GitHub evaluates concurrency BEFORE the job `if`: a /verify comment entered the triage job's shared per-PR group (where it could displace a pending /triage and then skip), and a /verify queued while the runner kill switch was on did the same to a real verification. Both predicates now match their job's runnable set exactly - an outward-resolving .git/hooks entry was only warned about and left in place, so the next root-owned git command would run it. It is now unlinked without traversing its target, a root-owned hooks directory is restored, and core.hooksPath is unset - the second .qwen pin re-derived HEAD^1 from git metadata after the workspace, including .git, had been handed to the build user. The base OID is now recorded while .git is still root-owned and the re-pin archives that content-addressed OID - classify_failure took both of its inputs from PR-controlled sources: a lifecycle script can exit with a signal status and can print any line the log patterns matched, turning its own deterministic breakage into 'infrastructure, please re-run' — which hid the failure and preserved a stale report. No infra verdict is derivable there, so the prepare step reports `fail` and lets the embedded log speak for itself - cleanups descended through PR-writable parents: `.qwen` itself can be a symlink, and the worktree sweep trusted git metadata with only a lexical prefix check. Symlinks are unlinked without traversal and worktree paths must canonicalize inside the workspace. Replayed all three escapes - skipped and docs-only outcomes upload no artifact, so the new download-failure branch pre-empted them and made their real reason unreachable; they are answered first now - a run that crashed before writing report.md still claimed the substantive marker, letting a headline overwrite the previous round's evidence. The marker now requires a report Skill: the byte-identical shortcut needs the whole input closure, not one file hash; the credential-free local path cannot call `gh` at all (fetch the metadata outside and mount it read-only); and the A/B base is `baseRefOid` in local mode, not `HEAD^1`. Tests: 7 new guards plus 4 updated to the new shapes, all mutation-verified (50/50). * fix(triage): answer dropped /verify requests and prove the proxy rejects Maintainer review (yiliang114), 7 items: - a third /verify while two runs are in flight is dropped by the concurrency group with no job and therefore no comment. The hosted authorize job now counts this workflow's other in-flight runs and says so; an API hiccup leaves the request alone rather than denying it - the proxy's bearer check had no executable test. It now starts the real proxy against a real upstream and issues real requests: no header and a wrong token are 401, this run's token is 200, and a route other than /chat/completions is 403 — with the health endpoint echoing the nonce - the 502 path forwarded the raw upstream error, which can name resolved hosts and TLS detail to PR code. It logs server-side and returns a generic failure - publish-verify inherited the 360-minute default; it downloads one artifact and posts one comment, so it is bounded at 10 - removing the log classifier last round left the comment block it replaced, which still said failures are classified from the exit status and the log. Deleted - that removal also left every install failure reported as the PR's fault, including a registry outage. There is exactly one signal here PR code cannot write — asking the registry ourselves, as root, with the container's resolver — so an install failure is downgraded to infra-error only when that probe fails. It proves reachability now rather than at failure time, so it can only ever downgrade, never confirm; a build failure has no equivalent and stays the tree's problem - the skill's local-invocation warning ran into the preceding sentence, which GFM renders as one paragraph Tests: 5 new guards, all mutation-verified (55/55). * fix(triage): resolve hooks hermetically and mirror symlink guards at job end Maintainer review round (doudouOUC), 6 findings. Two were Critical and both reproduced: - the hooks sweep resolved its path with the ambient git config in play. With a global core.hooksPath set — which the reviewer has and I do not, which is why my earlier replay showed a false pass — `git rev-parse --git-path hooks` returns that global path, the in-git-dir guard reads 'outside', and a planted `.git/hooks` symlink survives untouched. A/B: old code leaves the symlink under a global hooksPath, new code removes it in both environments and never touches the link target. Resolution now runs with GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM pointed at /dev/null - the END-of-job cleanup still used the bare `rm -rf .qwen/tmp` that the start-of-job cleaner was hardened against two rounds ago. The agent executes PR code between the two, so the end is no safer than the start: it now unlinks symlinks without descending and canonicalizes worktree paths inside the workspace before deleting Plus four suggestions, all valid: - the saturation notice counted this workflow's in-flight runs across every PR while the concurrency group is per-PR, so a run on another PR would trigger a warning about a queue that does not exist. It now matches on the PR title (the only per-PR handle an issue_comment run record carries) and stays silent when that cannot be resolved - the skill recommended `require.resolve` for the workspace-realpath check; these packages are ESM-only with import-only exports, so it throws ERR_PACKAGE_PATH_NOT_EXPORTED and reads like a missing module. Verified, and replaced with `readlink -f node_modules/@qwen-code/...` - the symlink-escape test inherited the developer's git config, which is what hid the first finding. It now runs with global/system config neutralized AND repeats the case with a global core.hooksPath planted - the publisher's build-phase arm was never rendered by any test (every case used 'install'), so a typo in that command name would have shipped. Now covered, along with an unrecognized phase Mutation-verified 4/4. The hooks guard needed a discriminating assertion: git's own `*.sample` files must survive the sweep, because the outward-path fallback removes the whole directory and would otherwise satisfy a bare 'planted hook is gone' check. * fix(triage): count only /verify runs for saturation, and test the PATCH arm Bot review round, 2 suggestions, both valid: - the saturation notice matched runs by PR title, which narrowed to this PR but not to /verify. /triage and /tmux live in their own concurrency groups, so two of those in flight would warn about a verify queue that is actually empty. It now also requires the run to have a job named 'verify' — the run record carries no command, but its job list does. Replayed: two non-verify runs stay silent, two verify runs warn - every publish fixture returned an empty comments listing, so the PATCH arm was never executed: a broken PATCH would have stranded the running status comment and posted a duplicate below it, with the suite green. The publisher now runs against a stubbed listing and the test asserts which verb went to which comment id — bot-owned live status is PATCHed in place, an absent comment posts fresh, and a marker comment owned by someone else is left alone and posted around Mutation-verified 3/3: counting every command, never PATCHing, and accepting foreign-owned markers each turn one test red. Two stub bugs found while writing these, both mine and both silent: ${*#pattern} applies per positional parameter rather than to the joined string (yielding a wrong run id), and the paginate fixture needs one array per page, not an array of pages. * fix(triage): fix the real silent drop and drop the step built on a wrong premise Review round 4. The blocker was mine twice over: the saturation notice I added last round had GitHub's concurrency semantics backwards, and the silent drop it claimed to cover was somewhere else entirely. - GitHub cancels the OLDER pending run in a group and admits the new one (confirmed against the workflow-syntax reference). My step told the person who had just typed /verify that their request might be dropped, when theirs is the one that runs — and said nothing to the person whose queued run actually died. This PR already had it right in publish-verify's own comment, so the file contradicted itself and the user-facing copy followed the wrong half. The step is removed rather than reworded: with the fix below there is nothing left for it to warn about, and it cost 2+N API calls on every /verify. - the actual drop: a verify job cancelled while still PENDING never reaches a runner, so its outputs block — where the "|| github.event.issue.number" fallback lived — is never evaluated. publish-verify then read an empty PR_NUMBER, hit its own guard and exited 0, making the cancelled branch unreachable in exactly the scenario that produces cancellations. The fallback now lives where the value is read. Reproduced both arms by executing the real step: with a number the cancelled notice posts, with an empty one it only warns. - same one-line class in publish-tmux, fixed alongside. Two copy defects from the classifier removal, both mis-attribution pointed the other way: - the infra-error body still named a signal/OOM kill and a full disk, none of which the current prepare step can produce — infra-error now requires npm ci to fail AND the registry probe to fail. It names that condition only, and offers a re-run instead of asserting it is the fix. - the code comment above it still described the deleted classifier. Also fixes the indentation break an earlier scripted edit left in the publish body builder, and replaces the saturation test with one that executes the cancelled path. Mutation-verified 2/2; the copy needed its own guard, since reverting the wording alone left every test green. * docs(triage): teach verify-pr survivor accounting and observability regressions Fold techniques from the re-verification on #7709 that the skill had no equivalent for: - the mutation matrix must report the mutations that changed NOTHING, not only the ones that failed. Each survivor gets classified as an ordinary coverage gap or as dead code — a guard whose deletion leaves every test green is one of those two, and the difference is what the author needs. Survivors mirroring a pre-existing gap are labelled as such, and the set is framed as completeness reporting rather than merge conditions - the sharper case that report demonstrates: a test that passes for the WRONG REASON. If deleting the new guard leaves its own new test green, that test is pinned by an earlier early-return, not by the change, and asserts nothing about it. Name what actually pins it - and do not generalize from one dead guard to its siblings: the same report shows a clause that is unreachable on one path while being the only protection on another. Check each, report the contrast - observability regressions: when a change suppresses output, follow the value before calling the suppression correct. A bare catch on the path plus a field with no readers anywhere in the repo means the cause is now unobservable even in devtools — a real loss that no behavioural assertion can see - report structure gains a Corrections section: when an earlier round or bot comment described the code inaccurately, state the correct fact with evidence and label it as a correction to the description, not a request to change code. A wrong description left standing costs the next reader more than the original finding did * fix(triage): carry the /verify lane's hardening across to /tmux The /tmux job executes the same untrusted PR code, as the same user, on the same persistent self-hosted pool as the /verify lane that #7710 hardened. Five of those controls had no equivalent here. Each was found on the verify side by reproducing an attack or a failure, not by reading the code, so the same evidence applies unchanged. - the model proxy bound a FIXED port (8787). PR lifecycle scripts run before it, so a detached child can squat that port: the real proxy then dies with EADDRINUSE while the health probe succeeds against the squatter, and the agent takes its chat completions. Now an ephemeral port published through a root-owned file, a per-run nonce the health endpoint must echo, and a liveness check on the PID we started. Replayed with 8787 occupied: the proxy comes up on an OS-chosen port and answers with the nonce. - nothing swept planted artifact directories. npm ci/build run the PR's lifecycle scripts, which can create tmp/<name>-tmux-<ts>/ holding a report.md and a transcript; the collector globs *-tmux-* and the publisher takes the first match, so a planted directory could supply the comment's contents. Swept after the last PR-controlled process and before the agent. - the global npm install ran with the workspace as cwd, where the PREVIOUS run's checked-out tree still sits. npm reads a cwd .npmrc, and a --registry flag does not override script-shell or hooks, so that config reached a root-privileged install. It now runs from RUNNER_TEMP. - the end-of-job cleanup globbed below .qwen/tmp. PR code ran in this workspace, so either .qwen or .qwen/tmp can be a symlink out of the tree — verified on the verify lane, where the glob deleted the link target's contents as root. Symlinks are unlinked without descending. - emit_block capped the raw log then escaped it. Escaping inflates every & < > by 4-5 bytes, so dense content can push the assembled body past GitHub's 65,536-character comment limit, 422 the post, and leave no comment at all. It now escapes first, caps the escaped bytes, and truncates on a character boundary via node — BSD iconv -c passes an incomplete trailing UTF-8 sequence through unchanged. Tests: a tmux-lane-parity suite, all six mutations verified (restoring the fixed port, dropping the sweep, moving the install back, dropping the symlink guard, reverting to a raw-side cap, and dropping the character-boundary truncation each turn one test red; a no-op control correctly changes nothing). One pre-existing assertion updated: it pinned emit_block's old inline-capture shape, and the guarantee it protects — a render failure is caught — is asserted in the new form. Also adds the regression guard for the publish-tmux PR_NUMBER fallback that landed in #7710 without one: a job cancelled while pending never evaluates its outputs, so without the fallback the result comment silently does not post. * fix(triage): address review — symlink guard, artifact strip, bearer auth (#7753) * fix(triage): address R2 review — proxy parity, bearer wire tests, process kill (#7753) * fix(triage): address R3 review — publisher parity, dedup ownership, cap budget tests (#7753) * fix(triage): address R4 review — drop redundant tmux-lane .mjs guards (#7753) * fix(triage): address R5 review — tmp symlink sweep guard, proxy timer clear (#7753) * fix(triage): address R6 review — hoist proxy timer out of try, dead-upstream 502 tests (#7753) * fix(triage): address R7 review — make proxy watchdog idle, end stalled response (#7753) --------- 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-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
c003e17181
|
fix(autofix): answer every review thread, resolve the ones actually fixed (#7758)
* fix(autofix): answer every review thread, resolve the ones actually fixed Two gaps made a handled finding look unhandled on #7731. A finding the bot declined, deferred, or escalated keeps its thread open, but its reason was recorded only in the round summary — a separate comment. The reviewer who opens that thread sees their finding answered by silence. The agent now writes comment-replies.json and the push step posts each reason as a reply on that finding's own thread, leaving the thread open. Replies are neutralised like the summary body, since a reply is model output posted verbatim under the bot identity. Resolution was also keyed on "did I edit a file this round", so a Critical an earlier commit had already fixed stayed open and read as unaddressed. Key it on the finding being resolved in the code, which covers a prior commit's fix the agent re-verified still holds. * style: apply prettier to the new review-reply test * test(autofix): update stale escape-site comment from five to six sites * fix(autofix): reply at thread roots and guard the reply id, per review (#7758) Address the maintainer review on the in-thread reply mechanism: - Map each reply to its thread's top-level comment before posting. The feedback step lists every review comment, replies included, so an rc:<id> can be a reply id; GitHub rejects a reply aimed at another reply, which would have left escalated findings answered by silence. Hoist the threads GraphQL fetch above both the resolve and reply blocks so a reply-only round still has it, and fall back to the id as given past the page cap. - Skip any id already present in resolved-comments.txt so a finding is never both resolved and replied to; the match tolerates the rc: prefix and a trailing CR like the resolve block's own parsing. - Mutation-verify the previously untested ^[0-9]+$ id guard (the boundary between a model-authored id and an arbitrary API path), the -f body= field name, the type=="array" skip, and the new cross-check. --------- Co-authored-by: wenshao <wenshao@example.com> |
||
|
易良
|
e8e15e3fb0
|
fix(ci): rename triage status marker to avoid duplicate-guard collision (#7723)
* fix(ci): rename triage status marker to avoid duplicate-guard collision The lifecycle status comment used <!-- qwen-triage stage=status --> which the triage agent's duplicate guard sometimes matches as a prior stage marker (stage=N), causing it to exit without posting the actual triage analysis. Rename to <!-- qwen-triage-lifecycle --> so the guard never matches infrastructure comments. Fixes the probabilistic silent-triage gap seen in #7713. * fix(ci): update test assertions for renamed triage lifecycle marker * fix(ci): sync triage finalize status marker * test(ci): cover triage status marker parity * fix(ci): preserve triage marker consumers * test(ci): pin triage lifecycle marker checks * fix(ci): add bot-author filter and startswith to triage status lookups qwen-triage.yml's two status-comment lookups matched any comment containing the marker — a human reviewer quoting the marker would have their comment overwritten by the bot PATCH. Add select(.user.login == $bot) (already present in qwen-triage-finalize.yml) and resolve BOT_LOGIN via gh api user. Both workflows used contains() for marker matching; startswith() is a strict improvement since every status comment body begins with the marker. This prevents the demonstrated defect where the finalize step resolved EXISTING_ID to a bot-authored Stage comment that merely quoted the marker. * fix(ci): guard triage status bot lookup --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> |