mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-08-21 14:46:19 +00:00
8723 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
叶公
|
4d4e3a0ed2 | fix(web-shell): restore reasoning preview after session clear | ||
|
叶公
|
afd51bdaa5 |
Merge remote-tracking branch 'origin/main' into codex/fix-webshell-new-session-effort
# Conflicts: # packages/desktop/bun.lock |
||
zjgzx1988
|
07405e669c
|
fix(artifacts): expand recorded directories into per-file artifacts (#9395)
* fix(artifacts): expand recorded directories into per-file artifacts A generated folder was stored as one file artifact, so Open and Download hit file APIs on a directory and returned 400. Record each file instead, and treat office documents as downloadable artifacts. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): address review on directory expansion Report depth truncation, keep later files on their own turn, and stop one bad filename or a full-content hash from breaking directory ingest. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): tighten directory expansion after review Use the locator's realpath workspace root, map unlistable directories to validation errors, skip junk roots, and keep walk caps/warnings aligned with what was actually recorded. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): drop unused recorded counter Directory expansion warnings are always surfaced, so the counter tripped no-unused-vars in CI. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): close remaining directory-expansion review gaps Reject non-object metadata and overlong titles before fan-out, wrap symlink-target lstat as validation, skip junk-only over-deep peeks, and stop offering Download for missing files. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): follow up on the latest directory-expansion suggestions Trim derived child titles, hash expanded files like direct records, only flag over-deep trees that actually contain recordable files, and show missing status on download-only cards. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): reject worktree-root expansion and close remaining review gaps Directory expansion treated a worktree cwd as a normal subdirectory, silently dropped unrecordable names, and could overflow the metadata budget when stamping expandedFromDirectory. Align the walker, tool, and store so those cases fail or disclose cleanly, and keep the pathless download test covering the collapsed 3-card cap. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): close junk-path, coalesce, and depth-disclosure review gaps Reject directory expansion when any path component is skip-listed, including symlink aliases into those trees. Treat unreadable over-depth directories as depth-limited, reject trim-unstable child names, and strip expandedFromDirectory when a curated same-batch record wins coalescing. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): use bracket access for expandedFromDirectory metadata acp-bridge tsc fails under noPropertyAccessFromIndexSignature when the test reads the marker via dot access. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(artifacts): keep worktree directory expansion and close round-7 gaps Strip the bound-root worktree prefix before the skip-directory gate so ordinary subdirectories still expand in worktree sessions. Realpath every directory walk root to catch intermediate symlink aliases into skipped trees, validate parent title/description before fan-out, and make the new fixtures Windows-safe. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): refresh desktop bun.lock for Live Host frozen install Qwen Live Host CI runs `bun install --frozen-lockfile` with Bun 1.3.9; the checked-in lockfile drifted and fails on main and any PR that touches Live Host path filters. Regenerate with the CI Bun version so install stays reproducible. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> |
||
Shaojin Wen
|
7e56bd0657
|
docs(autofix): add an operator guide for /takeover from N (#9622)
* docs(autofix): add an operator guide for `/takeover from N` `qwen-autofix.md` records why each line of the workflow is the way it is, indexed by code site — af-007 for the `from N` parser, af-016 for the marker it writes. That answers "why is this code shaped like this", but a maintainer deciding whether to seed a takeover has a different question: what do I type, what number do I pick, and what happens next. Nothing answered that. This adds a task-oriented sibling guide covering the problem the seed solves, how to choose N (with a table of remaining suggestion budget per seed), the three semantics that surprise people — the seed is a floor for an empty window rather than an offset added to every round, it dies with its counting window so `/retry` and a bare `/takeover` both return the counter to zero, and it is clamped strictly below the round cap while the audit record still cites the number that was typed — and the two things it deliberately does not do: seed the growth brake, or change what Critical-only keeps flowing. Every row of the accepted/rejected command table was produced by replaying the merged parser fragment verbatim, so the doc records observed behaviour rather than intent: `from 04` and `from 08` seed 4 and 8 rather than tripping octal, `from 0` engages as the explicit no-seed spelling, and `stop from 4`, a doubled space, a 3-digit number, and a prefixed or suffixed body all fail closed to no label and no seed. Cross-linked both ways: the design record's preamble now points at task-oriented guides, and the parser keeps its af-007 pointer with an operator-guide line beside it. The workflow grows by 64 bytes, which the size gate covers. * docs(autofix): align the round-seed guide with the workflow behavior Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
易良
|
e40263ee55
|
chore(deps): Clear high-severity CVE baseline and harden the security gate (#9584)
* chore(deps): Clear high-severity CVE baseline and harden the security gate - Bump OpenTelemetry stack to 0.221.x (fixes @opentelemetry/core advisories) - Bump @larksuiteoapi/node-sdk to ^1.73.0 and override axios to ^1.19.0 - Bump mobilewright to ^0.0.53 (drops vulnerable sharp 0.34.x) - Bump markdown-it to ^15.0.0 (drops vulnerable linkify-it 5.x) - Update undici/fast-uri/brace-expansion/ip-address within range - Adapt telemetry code to OTel API changes (forceFlush, processor options) - Make security-checks a hard gate now that the high baseline is clean * chore(deps): Refresh mobile-mcp vendored lockfile to drop vulnerable sharp * fix(telemetry): stub sdk-node 0.221 env auto-config helper packages sdk-node 0.221 extracted its env-based auto-configuration into @opentelemetry/configuration, otlp-exporter-base, and otlp-grpc-exporter-base, which it now requires eagerly. The existing esbuild stub only covered the exporter-* packages, so the OTLP protocol chain (grpc-js, protobufjs, otlp-transformer) re-entered the sdk-impl static closure and tripped the serve fast-path bundle guard. Stub the three helper packages when imported by sdk-node only; our own protocol modules keep resolving the real packages. qwen-code never reaches these helpers at runtime (explicit exporters + env scrub). * fix(telemetry): disable metrics fallback without reader * fix(vscode): restore nested dependency notices * fix(deps): declare bundled punycode so its notice survives regeneration The CLI esbuild config aliases punycode to the userland package (esbuild.config.js), so the shipped CLI bundle contains MIT-licensed punycode@2.3.1. Its NOTICES.txt section was lost because the only lockfile paths reaching punycode were dev-only; the notice walker (rooted at vscode-ide-companion) never sees a production declaration. Declare punycode as a direct production dependency of the CLI (the bundle input) and of vscode-ide-companion (which packages the bundled CLI into the VSIX and owns NOTICES.txt), then regenerate the lockfile and notices so the MIT notice is restored. |
||
|
Shaojin Wen
|
94a8c40364
|
docs(autofix): pin the publish-side checks to the patch text (#9652)
Review feedback on #9525 arrived after that PR was merged, so the notes it raised never made it into the doc. Both are things an implementer could get wrong from the text as it stands. Say what makes the re-derived checks sound in their new position: they are computed from the patch text on the trusted side and may never re-read a value the execute job wrote — not a line count it reported, not a footprint list it declared, not a base SHA it echoed back. This is the check class that has repeatedly failed to be forgery-proof here, and an implementation that shortcuts to reading the count the gate already computed reintroduces exactly that. Record the sequencing constraint as a constraint: the issue-autofix follow-up has to land before the deletion pass removes machinery it still relies on, which is why that pass is per cluster. Cross-reference #9556, which asks the same question on the review side. Refs #9089, #9525. |
||
Dragon
|
d0d68c0e8b
|
feat(cli): extend non-blocking slash commands to more builtins (#9495)
* feat(cli): extend non-blocking slash commands to more builtins #8130 opted /about, /help, and /settings in to run immediately while a response streams. Apply the same criteria to eleven more builtins so local UI controls no longer wait for the active turn: - UI-preference commands whose saved changes apply through the existing settings hooks: /theme, /editor, /vim, /voice, and /terminal-setup. - Read-only status commands: /tools, /lsp, /tasks, /hooks, /docs, and /bug. Commands that submit model turns, mutate conversation state, or read state the active turn is writing remain serialized, as documented in the non-blocking slash commands design doc. Each opt-in is pinned by a unit test. * docs(cli): scope extended-command claim to the eleven opted-in builtins |
||
|
Dragon
|
459c04adaa
|
docs: document Session Workflow setting (#8554) | ||
|
Dragon
|
fad5ae0bab
|
docs: document inline terminal image previews (#8656)
* docs: document inline terminal images * docs: clarify inline image behavior * docs: scope terminal image limits * docs: clarify IM image delivery support * docs: separate terminal image guidance |
||
|
Dragon
|
e09399e0fb
|
feat(core): make list_directory opt-in (disabled by default) (#9424)
* feat(core): make list_directory opt-in (disabled by default) glob covers directory listing in most cases, so list_directory is now disabled by default to keep the tool surface lean. Enable it with tools.listDirectory.enabled=true or by listing it in the coreTools allowlist (--core-tools / tools.core). The plan-mode reminder no longer steers the model toward the tool. * fix(core): align list_directory opt-in gate with prompts and allowlist parsing Round-1 review findings on the opt-in gate: - Normalise coreTools entries with parseRule so specifier forms such as list_directory(/src) still re-enable the tool. isLsToolEnabled used resolveToolName, which does not strip specifiers, while PermissionManager admits the same entry via parseRule — so the allowlist accepted the tool and the registry never got it (R1-1). - Drop list_directory from the plan-mode block error. That message is returned to the model as the tool result of every blocked call, so it kept steering the model at an unregistered tool even after the system prompt stopped doing so (R1-2). - Remove the now-inert ToolNames.LS entries and the prompt text that advertised the tool to built-in agents. The gate also guards the forked-registry rebuild path, so these agents were silently stripped of a tool their own prompts told them to use (R1-3). - Say why the tool is missing and how to enable it in the tool-not-found message, instead of offering unrelated Levenshtein suggestions (R1-6). - Cover the alias and specifier forms in the registration tests (R1-4), and drop the stale examples from the sub-agents and Java SDK docs, where the listed entry can no longer register the tool (R1-5, R1-7). * fix(core): address list_directory opt-in round-2 review findings - Reword the skill-review agent task prompt so the inspection guidance only references read_file; the run's tool filter has no list_directory, glob, or shell, so the old `ls` instruction was unsatisfiable for the turn-budgeted background agent. - Resolve tool-name aliases (ListFiles, ListFilesTool, ReadFolder) in the list_directory not-found message so aliased calls get the enablement explanation instead of a Levenshtein suggestion. - Attribute a missing list_directory to the workspace tools toggle when the workspace disabledTools set blocks it, since the opt-in setting cannot lift that state. - Drop the coreTools allowlist advice from the enablement message: setting tools.core to ["list_directory"] alone would exclude every other tool. - Switch the two remaining sdk-java runTransportOptionsExample copies from list_directory to glob; setAllowedTools only adds auto-approve rules and never registers a tool. - Extract the shared fake-server scaffolding in the list_directory integration tests into a local helper, keeping the load-bearing CLI-flag comment. - Pin the skill-review agent tools array and assert the learn-skill prompt steers to read_file / glob, matching the sibling planners. * fix(core): only claim list_directory is opt-in-disabled when it is unregistered The not-found explanation resolved aliases (ListFiles, ReadFolder) before checking whether the tool was actually absent. The registry is keyed by canonical names while the lookup that reaches this path resolves legacy migrations only, so an alias call missed even when list_directory was enabled — and the message then told the user to switch on a setting that was already on, hiding the generic path's "Did you mean list_directory" self-correction. Gate the branch on the canonical name being absent from the registry, and pin the two alias combinations that were unpinned: an alias against a non-empty workspace disabledTools set, and an alias against a registered tool. |
||
|
Shaojin Wen
|
98854a38d4
|
feat(review): fold the one-hop import widening into fetch-pr --since (#9332)
* fix(review): gate the recovered incremental anchor on the model that certified it Incremental scoping is a same-model contract: "clean up to this commit" is one model's verdict. The cache path has always enforced it through lastModelId, but the anchor recovered from the posted review's ledger marker shipped bare, so a round run under a different model would scope sha..HEAD past code the current model never reviewed — permanently, since each clean round re-anchors past the last. The marker now carries the certifying model beside the anchor, riding and falling with it: withheld on fail-closed and truncated rounds, and dropped by the parser when the sha beside it did not survive. The recovered-ledger context section names the model and instructs the gate (absent counts as a mismatch — markers predating the field), and the skill's incremental check requires a model match on both the cache path and the marker-recovery path before scoping to the interdiff. The findings work list still carries across models — every entry is re-asserted against the code — only the anchor does not. * feat(review): rescope — deterministic incremental plans, widened one import hop Incremental review existed only as prose: Step 1 said "compute git diff <lastCommitSha>..HEAD and use it as the review scope" and left the mechanics to improvisation. The improvisable route — re-run plan-diff over a hand-captured interdiff — silently degrades the plan (no worktreePath, no PR identity, no heaviness), dropping Agent 0, the modeled-system lens and every invariant agent from the roster. `qwen review rescope --plan <plan> --anchor <sha>` moves the scope decision into code: it re-validates the anchor against the history, captures the interdiff with the pinned flags, widens it by one import hop — every still-clean source file that imports a changed file re-enters the scope with its full-range hunks — and rewrites the plan in place with the same builders fetch-pr used, identity fields riding through and post-image line counts intact. The plan gains an `incremental` block; chunk briefs annotate each file's class (changed = review in full, interaction = review the seam only), and whole-diff briefs carry the frame once. Failure is directional: any refusal leaves the plan untouched, so the fallback is the full-range review, never a skip; an empty interdiff exits 3 and maps to the same-SHA outcomes. The widening exists because "clean" was certified against the code as it stood: a fix that moves a contract can break an unchanged caller, and an interdiff-only scope never re-opens it. Dependents only, source only, one hop; the scan is a documented heuristic whose misses keep exactly the pre-widening floor. * fix(review): harden rescope and the widening against review findings Findings from the PR's own review rounds, each verified before fixing: - Scoped files now carry FULL-RANGE hunks; the interdiff only chooses which files are in scope. Since-anchor hunks broke inline-comment anchoring: a fix round that restores lines the previous round changed produces hunks that exist nowhere in the PR's own diff, and one such anchor 422s the whole posted review, all-or-nothing. - EXT_MAP maps .js to BOTH .ts and .tsx — under react-jsx a .tsx file emits .js, and 921 of 6,200 relative .js specifiers in this repo named .tsx targets no edge could reach. Root-escape guard is segment-exact (a '..config' directory is not an escape), and the documented dist/ deep-import remap now actually strips the dist/ segment. - rescope refuses an already-rescoped plan (a second pass derived candidates from the shrunk file list and repointed fullDiffPath at the file it was about to overwrite) and a plan with missing or malformed files[] (normalising to [] silently dropped every widening candidate). All git calls are pinned with -C to the plan's worktree: pathspecs resolve against git's cwd, and from a subdirectory an unmatched pathspec exits 0 with empty output instead of failing. - incrementalScopeOf honours its degrade contract: interaction entries whose edges failed validation are dropped, and a block with no surviving scope renders no incremental frame at all. - Whole-diff briefs name each file with its scope class (capped list); chunk briefs state that scope classes override the generic duties for interaction files; heavy INTERACTION files get no invariant agents — their full-range slice is exactly the code the previous round cleared. - incremental.contextFiles (23 KB measured on a 300-file plan, with no reader) is now a count; fullDiffPath is named in the skill prose. SKILL.md states rescope runs from the main checkout, not the worktree. - Test batch from the mutation findings: exit-code literals pinned, --out exercised, diffPathAbsolute asserted, one-hop limit gated, same-sha refusal byte-compared, heaviness preservation asserted, test-file dependents excluded, cross-package widening exercised, fileLineCount covered at the git layer. * fix(review): round-2 findings — slice the fetched diff, cap and reconcile the frames Round-2 review findings, each reproduced before fixing: - The composite is now a BYTE-SLICE of the fetched full-range diff, not a pathspec-scoped re-capture: a scoped re-capture cannot see a rename source, un-pairs the rename, and renders a whole-file add whose hunks exist nowhere in the PR's own diff — the second entrance of the same 422 anchor class the round-1 redesign closed. Slicing also keeps the subset invariant byte-exact. sliceDiffByLines moves to lib/diff-plan. - deltaFiles is reconciled with the sections the composite actually holds (a file restored to its merge-base state names no phantom scope; its importers still widen), a files[] whose entries carry no usable path refuses like an empty one (zero-compared must never read as nothing-changed), and an unwritable --out exits 2 instead of throwing. - The whole-diff frame carries the same scope-class-WINS reconciliation as chunk briefs (agents 1a/1b sweep duties re-opened round-1 findings over interaction hunks), scope lists cap edges per entry (8) as well as entries (30), anchors render inertly, empty-string edges degrade, a chunk with no classed files gets no frame, and the frame wording is flow-neutral (review's base, not PR base). - Roster: interaction paths subtract deltaFiles (a path in both lists is live delta — widening wins), the field is declared on RosterPlan, and heavyFiles' doc is re-attached. - import-graph: dist deep-imports resolve under BOTH emit layouts (dist/src/… and flat dist/…), and the header now states the honest wrong-edge cost of unparsed exports maps (one extra widened file, never a narrowed scope). - Tests: rename-preserving slice, restored-file reconciliation, empty and zero-usable files[], out-of-worktree cwd run, unwritable --out, exact contextFileCount, head-distinct heaviness oracle, .cjs resolution, both dist layouts, list/edge caps, both-lists roster widening, no-frame-for-unclassed-chunks. * fix(review): round-3 findings — follow the lineage, absolute full-diff path Round-3 review findings on the rescope layer: - R3-1 (Critical): a file renamed BEFORE the anchor and deleted in the fix round carries two names — the post-image name in the interdiff, the left-side name on the PR diff's deletion section — so the section holding its unreviewed hunks matched no scoped name and silently vanished (or exited 3 as 'nothing new'). An unmatched delta file is now dropped only when a cheap per-file probe proves it a genuine RESTORATION (identical blobs on both sides of the PR range); any other lineage break refuses to the full range, and the check runs before the empty-sections exit so the refusal wins. - incremental.fullDiffPath is absolute: a cwd-relative path is meaningless to the later step the field exists for (R3-9), and the exit-3 contract in the header now names both of its causes (R3-8). - The roster's interaction-path reader applies the same validation the brief renderer does — anchor present, every entry carrying a surviving edge — and a malformed deltaFiles disables the narrowing entirely rather than just its delta-wins subtraction: with no trustworthy delta list there is no way to tell a seam-only file from a live one, and every malformation here must widen (R3-2, R3-10). - Tests: rename-then-delete refusal, restored-only exit 3, unwritable --out leaves the plan byte-identical, sliceDiffByLines gets a direct suite (parse → slice → parse round-trip, byte-exactness over invalid UTF-8 and lone CR, range ordering and clamping), and the resolver's literal-form candidate is pinned. * fix(review): certify the ledger anchor with the runtime model identity * fix(review): pin the posted marker's model wiring and tighten the anchor-gate spec * fix(review): round-4 findings — chunk-scoped role briefs, resolver gaps Round-4 review findings: - R4-1 (Critical): a chunk-scoped ROLE brief (the reverse auditors, the one role accepting a chunk) received per-file scope classes only from the globally capped list, so on a wide round its own files could be elided past entry 30 — the sole reviewer of that territory left without their class and with no way to recover the tail. Its own chunk's files are now listed in full, and the chunk brief's seam bullet drops the display cap for the same reason (R4-2); the cap stays where it belongs, on the whole-diff frame. - The resolver gains the `.jsx` emit row (a JSX source emits `.js` under the same convention as `.tsx`, R4-4) and normalises bare-package subpaths through the same POSIX rules relative specifiers already get, refusing escapes (R4-5). - A plan file that parses to JSON `null` now refuses instead of throwing a TypeError past the catch (R4-7), `fileLineCount` is `-C`-pinned like every other git call in the module (R4-9), and a `deltaFiles` array of non-string junk disables the roster narrowing exactly as a missing list does (R4-11). - Tests: chunk-scoped role brief listing, `.jsx` and subpath normalisation, junk-deltaFiles widening, JSON-null plan refusal. R4-8 declined with rationale, recorded in the code: a file absent at BOTH ends of the PR range is either a net-zero add-then-delete (safe to drop) or a rename-before-anchor whose deletion hunks sit under its pre-rename name (dropping loses them). This layer cannot tell them apart, and dropping re-opens the round-3 Critical, so the refusal stands. * fix(review): shed the anchor pair first and pin the round-3 findings The marker's byte-cap loop dropped a finding before the anchor pair; `dropped` then withheld the pair in the same render, so a capped clean round lost a ruling it was owed. Shed the pair first — the work list survives and recovery degrades to the full diff. Plus the round's pins: attribution-off withholding of the runtime-injected model, the submit fixture's production filename encoding, the skill's same-model gate clauses, and the differing-SHA gate in the user docs. * fix(review): scope the identity-channel claims and pin the branch-1 gate The boundary comments and DESIGN.md claimed the runtime identity channel delivers what the mechanism cannot: a model-authored command prefixes its env, and the override reaches the child (measured in this repo's bash -c spawn shape), so "the model the session ACTUALLY runs, not the id the state JSON typed" overstated the guarantee. Scope every PR-owned claim to what the wiring delivers — the runtime id supersedes the typed one, and the channel stays forgeable, same posture as the cache path. Plus the revert-guard's missing pin: branch 1's `If SHAs differ **and** model matches` clause was unpinned, so a partial revert dropping only it left every suite green (measured); the pin makes that revert fail and does not misfire on the PR state. * fix(review): shed the dead anchor tie-break and pin the reprieve clauses * fix(review): round-5/6 Criticals — readers for restored files, honest exits Per the posture announced last round, this lands Criticals only. - R6-10: a delta file the fix round RESTORED to its merge-base state fell between both reader classes — no PR-diff section, so no full review, and inside `delta`, so the widening skipped it as a candidate. Its imports of files that are still changing therefore had zero readers. The restoration probe now runs BEFORE the widening and splits the set: every changed file (restored included) still pulls its importers in, because a revert moves their seam too — round 1 cleared them against the pre-revert callee, and (importer@head x callee@base) is a pairing no round has seen — while the restored files themselves become candidates in a second pass keyed on the LIVE delta, since a restored file importing another restored file has no moving side to check. - R5-14: nothing past the plan write may throw. "Only exit 0 rewrites the plan" needs its contrapositive to hold, and a dead stdout (`qwen … | head`, a daemon redirect) made the courtesy reporting raise EPIPE — exit 1 over an already-rewritten plan, sending the caller down the "full-range plan untouched" branch against an incremental one. - R6-16: `fetchedSha`/`mergeBaseSha` were taken on type-check faith. Both ends of the PR range must be object ids: a clobbered plan naming a moving ref would resolve at call time, so the interdiff describes one tree and the worktree reads another while the exit-0 plan claims incremental scope. Each of the three tests was mutation-checked: reverting the fix it pins turns it red. * fix(review): round-6/7 Criticals — whole tree entries, and an async-proof exit Criticals only. - `restored()` compared blob oids (`rev-parse <ref>:<path>` yields nothing else), so a fix round that reverts the content and KEEPS `chmod +x` — or swaps a file for a symlink with the same text — was misclassified as restored and dropped from scope. Its mode-only section is in the PR's own diff (parseDiff emits one, planChunks gives it a chunk), so the incremental path narrowed BELOW the full-range floor it is documented to hold and exited 3 "nothing new" over a change nobody reviewed. The probe now compares the whole tree entry, mode included, via a pathspec-pinned `ls-tree`. - The round-5 EPIPE guard caught only the synchronous throw. A dead stdout also surfaces as an ASYNC 'error' event on the stream, which no try/catch around the write can intercept and which terminates the process with exit 1 — over an already-rewritten plan, sending the orchestrator down the "full-range plan untouched" branch against an incremental one. A persistent no-op 'error' listener makes that shape inert; the test now pins both. Both tests were mutation-checked: restoring the blob-only probe, or removing the listeners, turns them red. * fix(review): stamp the round's model at capture, qualify it by provider Two ways the same-model gate could certify a range under a model that did not review it. 1. Deferred post. compose/submit read QWEN_CODE_MODEL at POST time, which tracks the session's CURRENT model — review under A, /model to B, "post comments" and the marker said B. The next round under B then scoped sha..HEAD past code B never saw. fetch-pr now stamps reviewModelId into its report when the diff is captured, and compose withholds the sha/model pair outright when that stamp disagrees with the runtime posting it: the round cannot name who reviewed the range, so it certifies nobody and the next round reviews in full. The findings still post. 2. One model id, two providers. A bare id is unique only inside one provider configuration; two of them exposing 'qwen3-coder-plus' would pass each other's gate. Config now publishes QWEN_CODE_MODEL_IDENTITY — <model>@<8-hex of authType+baseUrl> — beside the bare id, and the review flow prefers it. A runtime that publishes neither yields '', which reads as a mismatch, not as agreement. The identity slot is process-global while the model is per-session, so shellContextEnv hands it down only while it still describes the model resolved for THIS session; a daemon side-session gets the bare id rather than another session's qualification, since a confidently wrong identity passes a gate the coarse one would have failed. Every new test mutation-checked. * docs(review): correct the absent-stamp and model-cap notes The reviewModelId doc claimed compose reads an absent stamp as "unknown"; it reads it as today's behaviour, and the reason is worth stating — the report is written at the start of a round and read at its end, so a missing stamp means an upgrade landed between the two, and a runtime that publishes no model id empties the other side of the comparison anyway. The ledger cap's note predates the provider qualifier, which adds nine characters to every id it bounds. * style(review): prettier the reapplied round-model helper * fix(review): drop the duplicate `incremental` field the merge left behind main's #9100 declared `incremental?: unknown` on agent-prompt's local PlanReport, and this branch already had one for the rescoped plan; the merge kept both, which is TS2300 and failed the build for every PR in the stack. Kept the documented one. Missed locally because vitest transpiles through esbuild, which drops types without checking them — a duplicate interface member is invisible to the test run and only `tsc --build` sees it. * fix(review): rule the same-model gate in the CLI, key the identity per session Four blockers from round 9, all in the identity plumbing this PR adds. R9-1: the recovery path's gate could never fire. The marker's `model` is the provider-qualified identity (`<model>@<digest>`), but SKILL.md told the orchestrator to compare it against `{{model}}`, which BundledSkillLoader substitutes with the BARE `config.getModel()` — two identity spaces that are never equal, so every same-model continuation round silently re-reviewed the full diff, which is the whole payoff this PR exists for. Read loosely instead, a prefix match would have accepted another provider's same-named model and re-opened the scope-skip the digest closes. The comparison now happens in the process holding both values: `pr-context` renders the verdict — "the same-model contract HOLDS" or "**Do NOT pass the reviewed-at sha as `--since`**", naming both identities either way — and the skill obeys that sentence instead of comparing strings. A section with no verdict is a mismatch. The cache path keeps its bare-`{{model}}` gate: Step 8 writes `lastModelId` from the same bare value, so that path is self-consistent. R9-2: in daemon mode the identity leaked across sessions. The slot is process-global and first-writer-wins, and withholding by OMITTING the key is not withholding at all — every spawn site composes the child env as `{...process.env, ...getShellContextEnvVars()}`, so the stale global rode the spread and session B stamped its marker under A's identity. Now registered per session beside the model (dropped together on unregister) and written as `''` on a miss, the precedent the agent and prompt ids in that file already set. The global slot stays the single-session CLI's fallback, guarded so one that describes another model is dropped rather than mis-qualifying this one. R9-3 (×2): the two wiring tests never cleared QWEN_CODE_MODEL_IDENTITY, which the boundary under test prefers — so an ambient value, which this PR's own Config now publishes into every subprocess, overrode the model they set. Running the suites inside a Qwen Code session is the dogfooding path, so that was the normal case, not the exotic one. Also folds the four inline `?? ` chains into lib/round-model.ts: `roundModelIdFrom` and `certifierMatchesRound`, the latter pinning whole-string equality and every unknown — absent certifier, unpublished runtime, two blanks — as a mismatch. Every new test mutation-checked. * feat(review): fold the one-hop widening into `fetch-pr --since`, drop `rescope` main's #9100 landed anchor validation and scoping inside `fetch-pr`, which is where this work belongs — so the `rescope` subcommand it was built as is gone (612 lines of command, 728 of test), and what was unique to it now runs on the `--since` path. Two changes to what an incremental round reviews. The scoped diff is a SLICE of the PR's own diff, not a re-capture of `since..head`. The delta decides WHICH files are in scope; their hunks come from the full range. Every hunk an agent can anchor a comment on is therefore byte-identical to one GitHub renders, and an inline-comment 422 takes the whole Create Review call with it. It also dissolves a refusal: an "undo per feedback" commit reverts lines back to base content, so a re-captured delta carries hunks the PR's diff does not contain — `hunks-outside-pr-diff`, which cost the round its whole scope. Sliced, that file is simply reviewed at the shape GitHub shows. And the file set is widened by one import hop. A still-clean source file that imports a changed one re-enters: round 1 cleared it against the callee's OLD shape, and (importer@head × callee@head) is a pairing no round has seen. This is only expressible under slicing — an importer is unchanged by definition, so no delta capture can show it. `incremental.scope` names each file's class (deltaFiles, interaction with the edges that pulled each one in, contextFileCount, restoredFileCount) and the superseded full range stays at `incremental.fullDiffPath`. A file restored to its merge-base state owes no review — mode-aware, so a content revert that keeps `chmod +x` is not a restoration — but still pulls its importers in. New refusal `lineage-unfollowable`: a delta file with no section of the PR's own diff under that name (a rename before the anchor) cannot be sliced, and refusing costs a full review where guessing loses hunks. An unparseable delta is `containment-unverified`, never `upToDate` — the empty file list is the parser's, not the tree's, and reading it as 'nothing changed' would stop the round over a failed capture. The scope logic is a pure module with injected readers, so it is unit -testable without a repository. Four new fetch-pr cases cover slicing, widening, restoration and the nothing-new stop; every one mutation-checked, and each of the four mutants (no slice, no widening, no restoration probe, widen on the live delta) turns the suite red. * fix(review): make the blanked identity fall back, and drop the anchor pair whole Round 10 filed no Criticals; these are the deferred items that were defects rather than coverage gaps. The R9-2 blanking silently disabled the bare-id fallback. `??` falls back on ABSENT, not on empty — and the identity slot is deliberately written as '' when a session has none to publish, because an omitted key is not withheld (the spawn-site env spread leaks the parent's stale one). So a blanked slot meant 'this round has no identity at all' rather than 'no qualification, use the bare id': the round certified nobody and every round after it re-reviewed the full diff. Both comments claimed the opposite. Blanking must cost the qualification, never the identity. `stripAnchor` dropped a foreign ledger's `sha` and left its `model` behind — an identity certifying a range that is gone, which every reader would have to know to ignore. They are written together, withheld together by compose-review, and serialized only as a pair; they are dropped as one now. SKILL.md's recovery path is reached from a cache-path WITHHOLD too, not only from an absent or refused anchor: a cache holding another model's anchor stops the round at the cache, and the marker it never looks at may hold one this model certified. Five new tests, each mutation-checked: the blank-slot fallback, the pair-drop, buildMarkdown's identity wiring, the per-session identity registry (write and mid-session re-key), and `certifierMatchesRound`'s engage case — every other case there is a refusal, so `return false` survived them all. * fix(review): repair the build, the retry class, and two import-graph edges R1-1 broke `npm run build --workspace=packages/cli` outright: `mergeBaseSha` is `string | null` and reached `treeEntryUnchanged` un-narrowed (TS2345), because the guard above tested only `fullBytes` / `fullText` and the compiler cannot see that a non-null capture implies a base. Naming the null base in the rejecting conjunct narrows it — and it is the same conjunct R1-2 needs, so the two fixes are one edit. I missed this locally twice, and the reason is worth recording: in a fresh worktree `tsc` bails with TS6305 before checking anything, so the `grep commands/review` I judged by came back empty and read as clean. Building core in the worktree first reproduces it immediately. R1-2: a base-fetch failure was demoted `containment-unverified`, which this skill's own taxonomy files under "deterministic for the same sha and must NOT be retried" — so a CI checkout with a flappy base fetch would pay a full review every round from then on, under a reason that also misnames the cause (the delta read fine). The three causes are now split by what a re-run would repeat: `base-untrusted` for a failed fetch, `capture-failed` for a base that existed and would not read, `containment-unverified` only for a successful merge-base that found no common ancestor. SKILL.md's reason list says so too, and the test that conflated the first and third is split in two. R1-4: `candidatesFor` tried every extension remap BEFORE the literal specifier, and `resolveSpecifier` takes the first membership hit — so in a mixed JS/TS directory where both siblings changed, `./util.js` resolved to `util.ts`. That is not one extra widened file, the cost this module budgets for a wrong edge; it DISPLACES the true one, so the seam brief names a pairing that does not exist while caller × util.js is named nowhere and retires unreviewed under a `scope.interaction` entry claiming the caller was covered. Every existing test used a single-element membership, so none could tell precedence apart. R1-3: the package-subpath escape check was `startsWith('..')` — the exact misclassification `repoJoin`'s comment eight lines above names and avoids segment-exactly. `@q/core/..config/mod.js` is a legal directory, and reading it as an escape drops the edge silently. Four new tests, each mutation-checked. * fix(review): rule the anchor verdict on the sha the side file actually holds R11-3: the section's RULED-FOR-YOU verdict was rendered from the ledger this run RECOVERED, while the sha Step 1 passes comes from the side file — and `persistRecoveredLedger`'s never-lower-round guard deliberately keeps a HIGHER-round file when the recovery walk comes back short (a concurrent lane, a paginated fetch that returned less than it should, a latest review deleted or edited). In that state a HOLDS about the recovered sha is obeyed against a different one, certified by whichever model ran THAT round — so the round scopes past a range only that model reviewed, permanently, since its own clean verdict re-anchors past it. Compose's drift gate cannot catch it: the re-run re-stamps under the running model, so the stamp agrees with the runtime and nothing looks wrong. The verdict now rules on what the file HOLDS, read back off disk after the persist decision rather than inferred from it — the guard's outcome is exactly the thing a caller would get wrong by reasoning about it. A divergence is a no-verdict state: both shas are named and the round reviews the full range, because nothing available here can say who reviewed the span between them. The findings still carry. Two new tests, both mutation-checked: the renderer's divergence refusal (and that agreement, and a file holding no anchor, still rule normally), and `persistedAnchorSha` reading back what the guard actually kept — the second is what fails when the read-back is stubbed out, which the renderer test alone could not see. * fix(review): move the last identity comparison out of prompt text R12-1 and R12-2 are the sixth and seventh findings in one class — two boundaries meaning different strings by the round's identity — so these close the class rather than the two instances. R12-1: the cache-path gate compared BARE ids on both sides. Step 8 writes `lastModelId: "{{model}}"` and the gate compared it to `{{model}}`, both the bare `config.getModel()`, so two provider configurations exposing one model name passed each other's gate — the exact case the recovery path in this PR rejects. Self-consistent is not sound; it was consistently wrong across providers, and I deferred it last round as an asymmetry when it was a hole. The gate moves into `fetch-pr`, beside the one the anchor already goes through: `--since-model` carries WHO certified the anchor, the skill copies both fields verbatim, and `certifierMatchesRound` — the same function the marker-recovery ruling uses — decides. A mismatch reports `cross-model-anchor` and reviews the full range, refused before the history is consulted at all. That leaves ZERO identity comparisons in prompt text. Six rounds have each closed one channel and the next round found another; the reason the class kept regenerating is that a comparison written in prompt text cannot share the CLI's notion of the string, and `{{model}}` is structurally the wrong one — it interpolates the bare id where everything the CLI records is provider-qualified. The SKILL guard now asserts the absence, not just the presence: no `lastModelId equals`, no `model matches`/`model differs`. R12-2: the drift gate disengaged whenever the post-time runtime channel was blank, even with the plan's stamp proving the round STARTED under a published identity — so `certifying` fell back to the model-written `input.modelId`, the channel these docstrings retire. The recovery side already rules an empty running identity a mismatch; the certifying side does now too. An UNSTAMPED round still keeps its old behaviour, because it cannot prove disagreement either. Two new tests, both mutation-checked. * fix(review): keep the merge-base probe's exit status, not just its answer R2-1: `mergeBaseSha === null` conflated the definitive "these histories share no ancestor" (git exit 1) with a probe that could not ANSWER — exit 128, or a kill, which is the 120s timeout a large long-lived PR under CI load reaches. The probe was wired through `gitOpt`, which discards the status, and `lib/git.ts`'s own `gitProbe` doc condemns exactly that collapse. The consequence is the retry class again: the round reported `containment-unverified`, which the taxonomy files under "deterministic for the same sha and must NOT be retried", so a transient merge-base failure cost the PR its incremental scope permanently and named a cause that had not happened. `GitProbe.mergeBase` now returns `{sha, status}`, `resolveMergeBase` reports `probeUnavailable`, and the reason keys on it. The flag is STICKY across candidates: the tracking ref can fail to probe while the local fallback answers a definitive no-ancestor, and a round that heard one unanswerable probe has not established determinism. `probeUnavailable` is required rather than optional on the result type, so a future producer cannot omit it and have the absence read as benign — the shape of the last three findings in this class. Three new tests, all mutation-checked: dropping the status split, and dropping it from the reason, each turn the suite red. * refactor(review): retire what slicing made dead, and pin what the caps cut Suggestions from round 1, all mutation-verified by the reviewer and re-verified here. Two are defects the slicing change introduced. R1-8: `diffBase` still carried the ANCHOR while the published bytes had become sections of `merge-base..head`. Agent 7 welds it into `--base` and recomputes its own diff, so the probe would run over hunks the round never reviewed and miss the ones it did — the exact error the field was added to prevent, arrived at from the other side. The producer stops writing it on a sliced round; the consumer's fallback to `mergeBaseSha` is the correct answer there, and it still honours the field on a plan an older CLI wrote, where a delta-range publish made it true. The seam-crossing test now asserts the published range instead of the anchor. R1-7: `fullDiffPath` was cwd-relative while every agent reads through `read_file`, which rejects relative paths, from inside `worktreePath` where `.qwen/tmp/…` resolves to nothing. Absolute now, and the docstring says NOTHING READS IT rather than naming consumers — the same over-claim #9191's R10-1 caught in the sibling field. R1-5: `containmentRuling` and its ~200 lines of helpers had no production caller left — containment is structural once the published diff is a slice of the PR's own — while a comment still claimed it "runs on every incremental capture" and `hunks-outside-pr-diff` sat in the reason union and the SKILL enumeration with no emitter. All gone, including the integration file that existed only to exercise it. R1-6: `fileLineCount`'s `repoRoot?` was a dead switch no caller set, documented for `rescope`, which no longer exists. R1-10: the capped-lists doc block sat above `chunkScopeBullets`, the function that is explicitly UNCAPPED, so hover read the cap rationale as documentation of its own contradiction. Moved to `scopeFileLists`. R1-11 through R1-15 are test gaps, each named with the mutant that survived. The caps are now pinned by what they CUT (the `(+N more)` arithmetic is independent of the `.slice()`, so both markers stayed correct with the truncation deleted); the malformed-block fixtures reach the field validators, and one carries a bad anchor with VALID lists — the only shape the anchor guard alone can reject; the restoration probe is steered per REF, which pins both "entries differ ⇒ not restored" and the mode half (a `chmod +x` with unchanged bytes is not a restoration); the plan⇔slice pairing is asserted where the slice is genuinely smaller; and the uncapped chunk-scoped path has a fixture that reaches the cap. Every one of those mutants was re-run here and turns the suite red. * fix(review): reconcile the incremental docs with slicing, pin the killed probe * fix(review): keep the scope ruling honest — probe status, rename lineage, two-flag re-run The restoration probe kept its exit status (an unanswerable ls-tree is retryable infrastructure, not a deterministic lineage refusal), a restored rename target carries its deleted source into the lineage check, a lossy capture fails the scope ruling closed, the side-file re-run passes both --since flags, a resolved base sheds the probe taint, and the brief renderer agrees with the roster on what a corrupt delta list means. * fix(review): brief the seams that have no other surface first R4-1: an interaction file that carries a section of the PR's diff is named twice — in `scope.interaction` and, uncapped, in the chunk brief of whichever chunk holds that section. One that carries NONE is named once. Those are the restored files the second pass pulls in: their own content is base content, so no chunk holds them, and the capped whole-diff list is the only place their seam is briefed at all. Insertion order appended them LAST, so on any round past `SCOPE_LIST_CAP` they were the first elided into `(+N more)` — the seam went unbriefed while `scope.interaction` still recorded it as covered. Coverage claimed and not delivered, which is the failure direction this module's header says it does not have. The cap now bites the redundantly-named entries first. It still bites: a round with more sectionless entries than the cap elides some, and that is the honest degradation rather than the silent one. The module is pure but for two injected readers — the property its docstring claims to make the whole decision testable without a repository — and nothing exercised it directly until now; every existing case reached it through `fetch-pr`. This adds that file, with the ordering as its first property. Mutation-checked: restoring insertion order turns it red. * fix(review): measure the decode, and ask the FULL range about a rename R5-2: the lossy-decode guard scanned the decoded TEXT for U+FFFD, which cannot tell a substitution from the code point itself. The code point is ordinary content — this repository carries four literal ones in source — so a delta touching any of them, even as context, demoted the round to `containment-unverified`. That reason sits in the recovery contract's "deterministic for the same sha and must NOT be retried" class, so the affected PR paid a full review every round from then on, under a cause that had not happened. Both documented causes of that reason are false for this arm. Measured on the DECODE now: re-encode and compare byte lengths. A substitution replaces an invalid sequence with three bytes and changes the length; a buffer that legitimately holds U+FFFD round-trips unchanged. Only invalid bytes can collide two names onto one, which is the hazard this guards — the character never could. R6-1: the rename-source ride-along fired only when the TARGET was restored, and that is the wrong question. Rename detection is a similarity threshold and the two ranges compare different pairs of blobs, so the delta can pair a rename the full range renders as a plain deletion beside a plain addition. With a live target and that straddle, nothing rode along, the lineage check passed on the new name alone, and the source's deletion hunks — content no round had seen — dropped out of the slice with the anchor advancing past them. The rule is the direct one: does the FULL range carry a section under the source's name? If it does, that section is unreviewed content the slice would drop, so the source rides and the lineage check keeps it. If it does not, both ranges paired the rename, the net hunks already sit under the new-side section, and riding the source would demand a section that does not exist and refuse the round. Three new tests, each mutation-checked — including an END-TO-END U+FFFD case, because the unit test of the helper alone left the arm that calls it free to revert. * fix(review): ask the decoder whether a capture is valid UTF-8 The byte-length round-trip missed every LENGTH-PRESERVING substitution, which is the shape a truncated capture actually produces: Node emits one U+FFFD per maximal ill-formed subpart, and a 3-byte subpart substitutes to a 3-byte replacement character. `F0 9F 98` — a cut-off 4-byte sequence — decoded to one U+FFFD of exactly the length it replaced, so the guard passed it as clean and scope membership was then decided on collided path strings: a live delta file conflated with a cleared sibling, its sections dropped from the slice, the anchor advancing past hunks no round had read. Asked of the decoder now — `TextDecoder('utf-8', {fatal: true})` — which is the only thing that knows. A literal U+FFFD in ordinary content still decodes cleanly, which is the distinction the guard exists to draw. Four of the reviewer's byte sequences are pinned directly; the byte-length heuristic calls every one of them clean. * fix(review): ride the section the full range paired a deletion under The ride-along asked whether the full range carries a section under the rename SOURCE's name. The two ranges can also pair the same deletion with DIFFERENT targets: base has `a.ts = A`; the anchor round rewrites `a.ts` to `A'` and adds `r.ts ~ A`; the fix round deletes `a.ts` and adds `q.ts` as an exact copy of `A'`. `anchor..head` pairs `a.ts->q.ts` (100% similarity, zero hunks); `merge-base..head` pairs `a.ts->r.ts` and renders `q.ts` as a plain addition. Nothing names `a.ts` in the full range, so nothing rode along, the lineage check passed on `q.ts` alone, and the published slice retired the source's net hunks — which sit under the section labelled `r.ts` — at the next re-anchor. Content no round had seen, gone from every later delta by construction. The rule now asks where the full range put the deletion. A section under the source's name rides as before. Otherwise, when the full range paired the source with a different target, that carrier section rides instead; a rename target of the full range is absent at the base by construction, so the restoration probe cannot misread it as restored and drop it. Otherwise both ranges paired the delta's own rename, the net hunks already sit under the new-side section, and riding anything would refuse the round for nothing — the pinned control for that shape still scopes by the new name alone. One battery test built from the two-range rendering: delta pairs, full does not. It fails at the parent commit (the slice publishes `q.ts` alone) and passes here. * fix(review): widen the restored-file hop in both directions R9-1. A file the fix round reverted plays both parts, and only one was wired. As a change it pulls its importers in; as an importer, its own base-era calls now face whatever the PR still moves — and that second direction is the one a revert makes load-bearing. Round 1 changes `i.ts` (`foo(x)` → `foo(x, y)`) together with its caller `r.ts` and clears both at the anchor. The fix round reverts only `r.ts`. The delta is `{r.ts}`, restored, so `deltaLive` is EMPTY — and the callee it strands was changed BEFORE the anchor and is unchanged since, so it is not in `deltaFiles` at all. Two layers then stopped the round: the second pass resolved `r.ts`'s import against that empty membership and found no edge, and even with the edge `scoped` took only the importer side, so the section that actually moves was never kept and `kept.length === 0` ruled `nothing-new` anyway. `upToDate` does not advance the anchor, so every re-run rules the same and `r.ts@base × i.ts@head` — the base-era call against the new contract — retires reviewed by no round. The membership is now every file the PR still changes, and the edges' targets are scoped with their importers. Restored×restored pairs stay excluded for free: a restored file carries no section, so it is never a candidate. `contextFileCount` follows the same move — "considered and not scoped in" is no longer "not an interaction key", now that a seam can scope a candidate as a target. One regression test in the pure module; each of the three edits is killed by it independently. It also trips one existing fixture, which declared `a.ts` restored while serving a full range that carried a hunk for it — a state git cannot produce, since a file identical at both ends of the PR has no section there. That fixture now serves the honest range, which makes its `not.toContain('a/a.ts')` structural rather than load-bearing; the comment says so. * fix(review): gate the widening's worktree reads on lstat before opening The readWorktree closure fetch-pr hands to widenScope is fed paths the PR's own file list determines — candidate files and every ancestor package.json discoverWorkspacePackages walks up. readFileSync follows symlinks and opens whatever sits there: a planted fifo blocks the synchronous read forever and a device like /dev/zero grows the buffer until SIGKILL. Neither death mode throws, so the catch that releases the worktree lease never runs and every later review of that PR refuses or re-hangs identically. lstat first and treat anything that is not a regular file as unreadable — null already means that to the widening, so an irregular path contributes no edge and the round keeps the unwidened floor. Same gate the pipeline already applies to this hazard class in script-lint's firstLineOf and run-ledger's ledgerOccupant. Witnessed by probe before the fix: the verbatim closure against a modeled worktree whose src/package.json is a fifo blocked the full timeout budget (exit 124); gated, it returns in milliseconds. Regression test pins the gate through runFetchPr — served content carrying a real edge is never read when lstat says the path is not a regular file — and removing the gate fails it. * fix(review): contain the widening's reads, and stop skipping invariants Two Criticals on the widening, one of them a hole in the lstat gate that answered the last one. **The gate defended only the FINAL path component.** `widenScope` hands the reader paths derived from the diff, and an INTERMEDIATE component can be a symlink the PR itself planted — ordinary git content that a standard checkout materializes, needing no platform cooperation. The path stays lexically inside the worktree while the kernel resolves it outside. That is an arbitrary-file read AND a channel out, because what the reader returns is content-derived and reaches `scope.interaction` in the published report. Containment is now by filesystem reality: `realpathSync(abs)` must sit under `realpathSync(resolve(root))`. Same class and same defence as `script-lint`'s `firstLineOf`, whose comment already names it — "SYMLINKED ANCESTOR … lstatSync only spares the final component". The reader moves out of the `fetch-pr` closure into `lib/worktree-reader.ts` so it can be tested against a real filesystem, where the kernel does the resolving; a mocked `fs` would have passed against a fiction, which is how the lexically-inside form got through the first gate. Six cases there, and `fetch-pr` keeps one mocked test for the WIRING — that it reaches the worktree through this reader at all. **A heavy interaction file keeps its invariant agents.** The skip rested on the premise that an interaction file's full-range slice is code the previous round already cleared, which holds only while the merge base holds still. Nothing enforces that: the anchor gate validates `--since` against head history, and neither the round cache nor the posted ledger carries a base identity, so a BACKWARD base move — the author retargets the PR to an older base, an ordinary GitHub operation — is accepted. `newBase..anchor` then carries hunks no round has read, they arrive inside a heavy interaction file's full-range slice, and these three agents are the only ones that would walk them; the chunk agent for the same file is briefed for the seam alone. So the skip is off until an anchor can prove base continuity — recording the base beside `lastCommitSha` and refusing on a change is a ledger schema change, and it belongs in its own PR. Removing it costs three agents on a rare shape (heavy, unchanged since the anchor, importing something that moved) and gives back the direction this design refuses to lose in. `incrementalInteractionPaths` and the `incremental` field on `RosterPlan` go with it. Both fixes are mutation-checked: dropping the realpath containment turns the reader's two escape cases red with the canary content, and turns the `fetch-pr` wiring test red; restoring the interaction skip turns the roster test red. * test(review): unit-test the seam narrowToDelta now composes Round feedback, one of three. `narrowToDelta` is a thin wrapper over `selectNarrowing` + `assembleSections` since the reshape, and the two halves are the surface the widening uses — it runs between them and asks `assembleSections` for a set LARGER than `selection.touched`. Every scenario in this file drives the wrapper, so a change correct for `touched` and wrong for any wider set was invisible here. Two cases, against captures real git produced: the selection reports only the touched paths while carrying every section the full capture does (the state the widening needs in order to consider anything), and the emit answers for whatever subset it is handed — reproducing the wrapper's own bytes for `touched`, adding the other section whole for a wider set, and answering null for a set the capture carries nothing for. Mutation-checked with a mutant shaped like the gap: gating the emit on `selection.touched` as well as `paths` leaves the wrapper's behaviour exactly right, and turns exactly one test — the new one — red. Also rewords a comment in `agent-prompt.test.ts` that read as a standing admission ("deleting the anchor guard left the suite green") when it describes the state BEFORE the case beneath it was added. Deleting `typeof raw.anchor !== 'string'` today is a one-test failure; the comment now says so. --------- 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> |
||
Kun De
|
6b64468445
|
fix(web-shell): show create-group tooltip (#9400) | ||
ytahdn
|
ef86d1f3c8
|
fix(web-shell): settle parallel-agents collapse and unify agent detail transcript (#9640)
- Show the subagent prompt as a user bubble inside the detail panel's transcript and let the whole message area scroll as one unit instead of pinning the prompt above a separate fixed-height box. - Collapse a parallel-agents group as soon as its agents are terminal, even while the main agent keeps responding, and bound the awaited-summary hold with a grace window so a missing narration cannot pin the turn open. - Return a stable empty artifacts array from useSessionArtifacts so a session without an artifacts endpoint stops re-running consumer effects every render, which previously cascaded into an infinite update loop. - Brighten a running subagent's summary text on hover alongside its icons. Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com> |
||
|
Shaojin Wen
|
7142a4fd93
|
docs(autofix): design runner-level isolation for PAT-bearing steps (#9525)
PR #9214 tried to close issue #9089 from inside the job by running the verification gate in an ephemeral container. Eleven review rounds took its diff from 329 to 2,700 lines while the per-round Critical count rose 8 -> 9 -> 15 -> 19 -> 29; rounds 10 and 11 changed no code and round 11 still produced 14 new Criticals on byte-identical files, six of them reporting that a site the diff added lacked the discipline the same diff installed elsewhere. That PR is frozen; its findings are inventoried in issue #9524. Two of its results shape this design. The container cannot help, because the gate's root of trust sits outside it: the digest chain is rooted in step outputs that the runner parses last-wins at step end, from a file under attacker-writable RUNNER_TEMP, with no sweep before the staging step. And the wrapper could never have run post-merge at all, which the pre-merge skip path hid from CI. The design moves the boundary from inside a step to between jobs: an execute job that runs branch code and holds no credentials, and a publish job that runs no branch code, re-derives its own decision from the patch instead of trusting the gate verdict, and holds the PAT. Agent process trees are killed by cgroup lineage rather than by an env marker, and the in-step enumeration machinery the split makes redundant is deleted rather than kept. Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
575e62ee46
|
fix(autofix): bind the sandbox image to its pulled digest (#9527)
* fix(autofix): bind the sandbox image to its pulled digest The sandbox image was exported as a mutable tag. `docker run <tag>` resolves against the local store without re-pulling, so a co-resident process with daemon access can `docker tag` different content under the same name between the resolve step and the consumer. Export the `<repo>@sha256:...` RepoDigests entry that matches both the pulled repository and the digest the pull itself reported: RepoDigests is shared by every tag of the same content, so index 0 can move off the pulled repo under a same-content retag, and retagged foreign content keeps its own repo — only the pair binds the export to what the pull fetched. Pin the daemon endpoint for both spawns. The docker CLI resolves its endpoint from DOCKER_HOST, then --context, then DOCKER_CONTEXT, then `currentContext` in the pool-shared config.json; clearing DOCKER_CONTEXT falls through to that last one, so the context is named explicitly and DOCKER_HOST is dropped from the child environment. An inspect answered by someone else's daemon hands back any digest it likes. Write the step files through a non-blocking, type-checked append. $GITHUB_ENV and $GITHUB_OUTPUT live under the runner-writable temp tree, where a planted FIFO turns a plain append into a block until the step timeout. Extracted from #9214, which is frozen; these were R11-1 and R11-2 there. The inspect timeout is now injectable so the tests can pin it, and the suite covers the endpoint pin on both spawns, the FIFO and directory refusals, cross-chunk stdout accumulation, and the timeout itself. Each new test was checked against a mutant of the code it pins. Refs #9089, #9524. * fix(autofix): bind gate image inputs to the resolver step output (#9527) * fix(autofix): revert repo-hygiene binding outside PR footprint (#9527) The deterministic gate rejected the previous commit because repo-hygiene.yml is CI machinery this PR never touched; review feedback alone cannot authorize changes there. Restore the file byte-for-byte and scope the workflow contract test to the two autofix workflows this PR binds. The repo-hygiene binding is real and is deferred to the review-findings follow-up queue for a maintainer-owned change. * fix(autofix): harden sandbox image consumers per review round (#9527) - R1-2: extract the duplicated spawn guard (endpoint pin, settle-once finish, SIGKILL timer, stdout capture, error/close wiring) into one spawnDockerCapture helper; pullImage and repoDigestOf share it. - R2-1: contract test fails when a workflow detects zero sandbox consumers instead of passing vacuously. - R2-2: success-path e2e test for the digest-bound export; verified it kills the exportImage(image) mutant. - R2-3: pin the daemon endpoint (DOCKER_HOST: '', DOCKER_CONTEXT: default) on every sandbox-consuming step, closing the $GITHUB_ENV and pool-shared currentContext channels past the resolver; contract test enforces the pin. - R2-4: gate the repair step on the resolver outcome so a failed resolver can never relaunch the agent unsandboxed. Also updates the workflow source pin in scripts/tests to the shared helper's literals (required by the R1-2 refactor). * test(autofix): pin repair outcome gate, derive contract set (#9527) - R3-1: the contract test now requires every always()-gated consumer to also gate on the resolver step outcome, pinning the R2-4 fail-closed clause; verified that deleting the guard from the repair step now fails the suite (the mutant shipped green before). - R3-2: route both main() e2e tests through withDockerStub; the refusal test's untouched-file asserts move before the temp-dir cleanup — they previously ran after rmSync, so they passed no matter what the resolver wrote. - R3-3: derive the contract test's protected workflow set from the tree instead of a hand-enumerated list, so a new resolver step cannot land untested; repo-hygiene.yml stays in an explicit, staleness-checked exception set until its deferred binding lands. * fix(autofix): pin resolver binary, make image check digest-aware (#9527) * test(autofix): share resolver e2e scaffold, tripwire stale exemptions (#9527) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
fee826c889
|
docs(ci): the ECS pool does run containers — correct two comments that say it does not (#9575)
* docs(ci): the ECS pool does run containers — correct two comments that say it does not Both files record, as the reason for a decision, that the self-hosted `ecs-qwen` pool has no container runtime. It does. `qwen-autofix.yml`'s `review-address` runs with `sandbox: "docker"` on those labels, behind a `docker info` preflight that fails the job outright if the daemon is unreachable, and it passes there — measured on `ecs-qwen-runner-hk-*` and `ecs-qwen-runner-sg-*`, with autofix's own comment noting that qwen-triage's container jobs prove the same pool independently. What is true, and is what the exit-44 note was really about, is the spelling: `sandbox: true` makes the CLI probe for a runtime and exit 44 when none answers, while `sandbox: "docker"` names one. No behaviour changes. `resolve-pr` stays on `ubuntu-latest` — the ephemerality is a good reason on its own for a job that merges the base branch and pushes to the PR head, and the comment now gives that reason instead of the false one. Triage's missing `sandbox` key stays missing; it is now recorded as a choice rather than as a capability it lacks. This matters because both comments are the first thing anyone picking up #9556 (whether the review pipeline should keep granting code execution as the invoking user) will read, and they price the sandbox option as a runner-capacity decision when it is a settings change plus a mount audit. * docs(ci): attribute the sandbox difference to daemon state, not spelling (#9575) * revert(ci): restore the triage contract test — CI machinery outside this PR's footprint (#9575) Deterministic verification rejected the previous commit because it reworded rationale comments, test names, and assertion messages in .github/scripts/qwen-triage-workflow.test.mjs — CI/verification machinery in an area this PR never touched. Review feedback alone cannot authorize that change, so the file is restored byte-identical to the base. The reverted finding (the test still records "the ECS pool ships no container runtime" as rationale) was verified real and is deferred to the follow-up queue for a maintainer-scheduled change. The two workflow-comment corrections from the previous commit stand: they are inside this PR's own footprint, and the restored contract test passes against them 118/118. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
叶公
|
d20f2b13a9 | chore(desktop): refresh frozen bun lockfile | ||
|
叶公
|
844c5994a2 | Merge remote-tracking branch 'origin/main' into codex/fix-webshell-new-session-effort | ||
|
叶公
|
920a6f51ff | fix(web-shell): harden reasoning preview lifecycle | ||
|
Shaojin Wen
|
2c64ebe980
|
feat(autofix): audit the approach instead of stopping on growth-budget breach (#9262)
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
Security Checks / Dependency CVE audit (push) Waiting to run
Security Checks / Secret scan (TruffleHog) (push) Waiting to run
* feat(autofix): audit the approach instead of stopping on growth-budget breach A growth-budget breach no longer escalates to a maintainer handoff that stops the takeover. The breach now makes the round a growth-audit round: the agent audits the PR's approach on two axes — KISS (name a simpler alternative or prove each piece load-bearing) and minimal change (every hunk traces to the problem, an accepted finding, or a failing check) — and records a machine-readable verdict that the verification gate requires. sound re-arms the counting window at the current size and the loop keeps solving; drift simplifies first, then continues; conflict is the only growth path to a human, parked idempotently until a trusted human responds. The old divergence ladder (over budget for N rounds and not shrinking → stop) terminated takeovers whose remaining work could still fit: the growth it punished was protocol-mandated pinned tests (#9213 stalled at round 5 with two small Criticals left). A size signal now triggers a judgment, never a stop. Design: docs/design/autofix-growth-audit.md * fix(autofix): update the artifact-list pin for the growth-audit.json upload entry * fix(autofix): surface conflict verdicts past the failure.md exits and strip verdict forgery channels (#9262) * fix(autofix): harden the growth-audit verdict pipeline and park wake set (#9262) * fix(autofix): close the verdict-pipeline forgeries and loop-generated wake entrances (#9262) * fix(ci): drop the retired divergence rationale records (af-046/af-047) from qwen-autofix.md --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
ytahdn
|
e443f26bfc
|
feat(web-shell): keep a turn expanded while its background shell runs (#9632)
A turn that launches a background shell stays expanded while the shell is still running and collapses once it completes. Completion is detected from the background shell notification or a terminal snapshot of shell tasks, so a lost notification cannot pin the turn open forever. Promoted shells are also classified as background tasks so polling stays armed for the snapshot fallback. Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
易良
|
a074d3b042
|
chore(ci): Disable install scripts in release CI and guard security-checks workflow (#9577)
* chore(ci): Disable install scripts in release CI and guard security-checks workflow * fix(ci): complete release install hardening * test(ci): pin release install step count Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): scope release PAT to push step Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): export GH_TOKEN so the release-branch push uses CI_BOT_PAT * fix(ci): export GH_TOKEN so the credential helper sees it at push time An inline GH_TOKEN prefix only covers the gh auth setup-git call itself; the helper re-resolves the token when git push invokes it, so the push would fall back to the job token with persist-credentials disabled. * fix(test): anchor setup-git ordering check after the export line A comment in the push step mentions gh auth setup-git before the export, so indexOf found the comment first and the ordering assertion inverted. * style(test): wrap long line to satisfy prettier * fix(ci): address review findings on PAT handling and install comments - Pin gh auth setup-git before the git push it authenticates in both release and finalize workflow tests, so moving credential setup after the push no longer passes. - Correct the replay comment: npm run generate is not a lifecycle script and workspace lifecycle scripts stay disabled. - Drop the overstated push-boundary claim and record why the push needs the bot PAT rather than the job token. * test(ci): pin CI_BOT_PAT out of install steps and the publish job header * style(test): apply prettier's exact re-wrap for the two flagged calls * test(ci): pin CI_BOT_PAT out of the workflow-level headers too --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
54a3a7f669
|
fix(web-shell): bound daemon transcript retention to stop renderer OOM crashes (#9303)
* fix(web-shell): bound daemon transcript retention to stop renderer OOM crashes Watching a busy daemon session (long turn, many subagents) could exhaust the browser renderer: the replay snapshot stayed pinned on the session client for the whole attachment, replay rebuilds ran uncapped and could ratchet the transcript block cap above its configured limit, and a few retention windows were implicitly far larger than intended. Observed as a Chrome renderer abort after multi-GB growth, reproducible on every reload of the affected session. - Release the replay snapshot once it is injected into the transcript store (SSE resumes from lastEventId; older history via pagination) - Rebuild replays under the configured maxBlocks cap and never raise the committed cap above it; trimming keeps the most recent blocks - Cap the debug text embedded for unrecognized session_update kinds, which appended one block per frame with the full payload - Pass the web-shell block cap to the subagent detail provider and lower the provider default window from 200k to 50k blocks * fix(review): address round-1 findings for transcript retention (#9303) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): cap debug payloads at the producer for transcript retention (#9303) * fix(web-shell): slim consumeReplaySnapshot to fit the SDK browser bundle budget The consume-and-drop API landed as a getter plus a private backing field, which pushed the minified browser daemon bundle 51 bytes past the assertBrowserSafeBundle budget. Collapse it to a single mutable replaySnapshot field swapped by consumeReplaySnapshot(), preserving the external behavior while fitting the budget without raising it. * fix(web-shell): bound transcript retention in bytes, not just blocks (#9303) Round-3 review fixes for the renderer OOM: - The capped diagnostic strings no longer retain their oversized parents: capped slices are detached via a UTF-8 round-trip before being kept on transcript blocks (capDetails and the 100KB text-block truncation), so a 4KB visible cap cannot pin a 100KB payload backing store. - Retention is now byte-aware: the transcript state keeps a running estimate of what blocks retain and trimming evicts oldest blocks until the estimate is back under a byte budget (128 MiB default), so the block-count window is no longer mistaken for a memory ceiling while tool blocks carry uncapped raw payloads. - The replay snapshot is released only after the injection dispatch succeeds, keeping the window consistent if normalization throws mid-injection. - The provider DEFAULT_MAX_BLOCKS is exported and guarded by a web-shell drift test instead of a duplicated literal. * fix(review): close round-4 byte-budget accounting gaps in transcript retention (#9303) * fix(webui): reject history pages that overflow the transcript byte budget (#9303) Pagination admission only checked the block cap, so a page could merge over the retention byte budget: idle sessions stayed over budget, and on live sessions the next trim evicted the freshly prepended oldest records, which the exclusive pagination anchor can never re-fetch — a permanent silent gap. Admit pages only when the merged estimate fits the budget, mirroring the atomic block-cap rejection. * fix(review): reconcile transcript eviction with the pagination anchor (#9303) * fix(webui): gate pagination re-open and drop stale in-flight pages (#9303) Round-7 review fixes for the eviction/pagination reconciliation: - The eviction re-open of the load-older affordance now applies the same gates as the sibling paths: it requires the session_transcript_pagination capability and an existing positional anchor (beforeRecordId or cursor). On non-pagination daemons or anchor-less sessions the capacity latch stays closed instead of exposing an affordance that would call a route the daemon does not serve and latch a pagination error. - A retention trim firing while a load-older page is in flight re-anchors the exclusive beforeRecordId; the stale page fetched against the old anchor is now dropped instead of merged, so it can never advance the anchor below the evicted band. Load-older fetches capture a monotonic pagination generation bumped on every block trim and drop the page when it moved mid-fetch (lossless: every record in the dropped page is older than the new anchor and is re-served by the next fetch). Adds regression tests for both: latch stays closed on live trims without pagination support, and an in-flight page is dropped across a mid-fetch re-anchor with the retry fetching against the re-anchored anchor. * fix(webui): gate pagination re-open on admission headroom (#9303) Round-8 review fix for the eviction-triggered capacity-latch re-open: - The re-open now checks whether the rejected page would actually be admitted against the post-trim window occupancy (carried on the truncation detail). A rejected page's footprint is remembered at rejection time; the latch re-opens only once enough count AND byte headroom has been freed for that page, stopping the streaming fetch/reject/re-render churn when count trims restore the window to exactly maxBlocks (zero headroom). - A footprint-less latch (replay saturation) re-opens only on real count headroom: while the count window is saturated, count admission rejects every page regardless of freed bytes. - A page that alone exceeds the whole retention window (block count or byte budget) can never be admitted in any occupancy state; it is now a terminal pagination failure (hasMore=false + paginationError) instead of a re-openable capacity state, so everything older than it is no longer silently unreachable. - Adds a maxRetainedBytes provider prop (mirrors maxBlocks) so the byte budget is configurable and testable; the replay rebuild store uses the same budget as the live store. Adds regression tests: churn suppression under count saturation, latch suppression while the freed hole is smaller than the rejected page, footprint-based re-open once eviction frees room (with re-anchored fetch), and terminal failure for a single oversized page. * fix(webui): reconcile pagination with eviction across rebuild and rewind (#9303) Round-9/10 review fixes for the retention/pagination reconciliation: - impossible is now evaluated across both admission dimensions with >= at the equality boundary: a page that alone fills the whole block window, or whose estimate alone reaches the byte budget, can never be admitted (an anchored window always retains at least one block / some bytes), so it routes to the terminal pagination failure regardless of which branch rejects first. - applyTranscriptHistory merges sentinel-aware: a pagination-resurrected real tool block mapping wins over the current window's TRIMMED sentinel for the same callId (the resurrected block is no longer orphaned), and the resurrected tool's trimmed-notification flag is cleared. - rewind (truncateTranscriptBeforeBlock) now fires the same 'blocks' truncation signal when it drops blocks, so freed capacity can re-open the pagination latch. The detail carries evictedOldest so consumers only re-anchor on oldest-first eviction (a rewind drops the newest blocks and leaves the oldest anchor intact). - The live-trim re-anchor clears a stale cursor atomically, and fails closed (clears the anchor, closes the affordance) when no retained block carries a recordId and the pre-trim replay had several distinct recordIds (a real evicted band) — with zero/one recordId the pre-trim anchor stays valid. - Replay-rebuild reconciliation runs for live-journal repair rebuilds too (both the ordinary and the marker-visible checkpoint-seeded branches observe trimming), and fails closed when the re-anchor is uncomputable with a real evicted band. - The rebuild affordance only opens with real admission headroom: a positional anchor and byte-budget room. Without byte-budget headroom (e.g. a single oversized block whose estimate alone exceeds the budget) no page can ever be admitted, so the affordance closes instead of burning on the first click. - projectChatRecordsToDaemonTranscript (offline/export projection) passes maxRetainedBytes: Infinity — its only documented truncation knob is maxBlocks; the live-window byte budget must not silently evict rendered or exported transcripts. Updates the affected pagination tests to non-boundary footprints and adds coverage for the eviction-direction signal. * fix(webui): harden eviction/pagination reconciliation corners (#9303) Round-11 review fixes: - Mirror the sentinel-aware merge for permissionBlockByRequestId: a page-resurrected real mapping wins over the current window's TRIMMED_PERMISSION sentinel, so a resurrected pending permission can flip to resolved (export isTrimmedPermissionBlockId next to the tool helper). - Snap retention eviction to record boundaries: one persisted record fans out into several blocks sharing a sourceRecordIds entry, and block-granular trimming could land mid-record, orphaning the evicted sibling blocks from exclusive-before pagination (the anchor re-sets to the shared record while the dedup filter drops any page still advertising it). Advance the cut until it no longer lands inside a record, keeping the at-least-one-block floor. - Content-aware dedup for recordId-less blocks: the locally echoed user prompt carries no recordId, so recordId dedup is blind to it; once a trim leaves it as the oldest retained block, a load-older page returning that prompt's persisted record would render it twice and double-count bytes. Drop page user blocks matching a displayed recordId-less prompt by text. - Fail closed when the rebuild trimmed and no retained block carries a recordId: with any pre-trim recordId, that record's blocks were all evicted and exclusive-before pagination never returns the anchor record itself, so anchoring there leaves it silently unreachable. Drop the anchor (affordance closes) instead of paging from a stale anchor. Updates the bounded full-snapshot fallback test to the fail-closed outcome. * fix(webui): close round-12 retention/pagination reconciliation criticals (#9303) Round-12 review fixes: - Scope the local-echo text dedup to the boundary pair (R12-1): the recordId-less echo can only collide with the page's newest user block, the one adjacent to the window's oldest block. Keying on text window-wide instead dropped distinct older prompts sent twice ("yes", a retry), permanently orphaning their assistant replies. Compare only the window's oldest recordId-less echo against the page's newest user block. - Gate observeReplayTrim on evictedOldest !== false (R12-2): a rewind also fires the 'blocks' truncation signal but drops the newest blocks and leaves the oldest pagination anchor valid, so it must not latch the capacity/re-anchor path during a capped rebuild (same gate the live store's onTruncation handler already uses). - Add the pagination generation guard to the loadMore catch block (R12-10): the success path already drops a page when a trim re-anchored mid-fetch, but the catch restored hasMore unconditionally, reviving the load-older affordance in the anchor-less state the fail-closed branch just closed. Leave the fail-closed state intact when the generation changed. - Back the record-boundary snap off the floor (R12-21): when byte pressure evicts to the last block, the forward snap's at-least-one-block floor stopped it from escaping a mid-record cut between the final two sibling blocks. Re-retain the penultimate block when the floor-pinned pair shares a record, trading at most one extra retained block against the budget to keep the record whole. Adds a reducer regression test. * fix(webui): close round-13 retention/pagination reconciliation criticals (#9303) Round-13 review fixes: - Count binary payloads in the retention estimate: Blob/File, ArrayBuffer, and typed-array/DataView values carry their content in non-enumerable slots, so the record walk only charged the fixed object overhead — an 8 MiB attachment counted as ~64 bytes and the byte budget never fired for media-heavy transcripts (the OOM class this PR targets). Charge Blob.size / byteLength for these shapes. Adds a budget-eviction regression test. - Loop the record-boundary floor back-off (R12-21 follow-up): the single back-off only re-retained one block, still cutting a 3+-sibling record mid-record. Loop while the boundary pair shares a record, keeping the record whole; the removeCount===0 guard retains the whole window when nothing is evictable. Grows the snap test to a 3-block record. - Offer the load-older affordance on a live trim for sessions that loaded unlatched: eviction re-anchored beforeRecordId but the re-open path only ran under capacityReached, so a session whose replay fit under the caps could never surface older persisted history after the window trimmed. Mirror the replay path's olderHistoryReachable gates in the re-anchor branch. - Key the boundary-echo dedup on echo presence and fold media into the comparison (R12-1 follow-up): an empty-text gate skipped image/file-only echoes entirely, and a naive empty-text match would collapse two distinct media-only prompts. Compare text + image/file counts for the boundary pair. * fix(webui): close round-14 retention/pagination reconciliation criticals (#9303) Round-14 review fixes: - Clamp the trim floor to keep at least one block (R14-1): a non-positive or non-finite maxBlocks made the count floor evict the whole window, and the record snap then read one past the end of the block array, throwing on every dispatch. Treat maxBlocks < 1 / non-finite as 1 so the window always keeps a block. Adds a regression test. - Drop the rebuild-trim fail-closed anchor unconditionally (R14-2): scanning only the fresh replayEvents missed recordIds trimmed from the repair checkpoint in a marker-visible live-journal repair, leaving a stale anchor with the affordance still on. Mirror the live store's fail-closed branch — when no retained block carries a recordId any pre-trim anchor is stale, and dropping an already-undefined anchor is a no-op. - Grow the latched rejectedPage footprint by a re-anchoring trim's evicted band (R14-3): the daemon re-serves the evicted band on the next exclusive-before fetch, so the page is larger than latched; a stale footprint would churn fetch/reject or misclassify a now-larger page as terminal. The re-open gate now measures the grown page. Reworks the re-open test to the faithful daemon behavior (the latch stays closed once the evicted band re-joins the page). - Refuse anchor-less forced retries (R14-4): after a fail-closed trim drops both cursor and beforeRecordId, a forced retry fetched with neither anchor, and the daemon defaults that to the journal's oldest page — prepended below the window and re-stamping a bogus anchor. Return early until a later trim re-establishes an anchor. * fix(sdk): integral-ize a fractional maxBlocks in the trim floor (#9303) Round-15 review fix (R15-1): the R14-1 degenerate-maxBlocks clamp floored non-positive and non-finite values to 1 but passed positive fractional values through unchanged, leaving removeCount fractional so the record-boundary snap indexed blocks[removeCount] one past the end and threw on later dispatches — the same crash class R14-1 was added to prevent, re-entering through a sibling input. Floor the clamped value so the window always trims by a whole block count. Math.floor is a no-op for the integer maxBlocks every in-repo caller passes. Extends the degenerate-maxBlocks regression test with a fractional case. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
9f2342d323
|
fix(ci): stop the fallback comment from denying a review it already posted (#9462)
* fix(ci): stop the fallback comment from denying a review it already posted The review job can fail AFTER posting its review — the CLI exiting silently, a cleanup step dying — and both fallback sites then announce that review as one that could not be posted, retry instruction attached. Measured on PR #9342: the review posted at 11:56:34Z, review-pr failed at 12:00:53Z ("Qwen review completed but produced no output"), and the comment landed at 12:01:00Z saying the pipeline "failed before a review could be posted. … retry with @qwen-code /review" — a fresh ~3-hour review, asked for beside the review that had just landed. The autofix takeover loop reads the same feed a human does. Both sites now check, before composing a body, whether a review this run posted is already on the PR. The check is scoped three ways so a stale review can never buy silence on a genuinely dead pipeline: the bot's own account, the head this run reviewed, and a submission at or after this run started. Where the proof is unavailable — no start time, no head, a failed listing — the guard declines to fire and the comment posts, the same call the head-moved guard already makes. The job-level step now reads state and headRefOid in one `gh pr view` (the in-job step already did), which is where its head value comes from. Tests run the steps' real bash over review fixtures, because the guard IS a filter: silence when this run posted the review, and posting for each near-miss on its own — an earlier run's review at the same head, another account's, one of a different head, a PENDING one, none at all, an unavailable start time, and a failed reviews listing. One existing assertion tightened: "no `gh run view`" was the proxy for "no head comparison on comment runs", and the new guard asks that same command for startedAt on every event, so it now pins the head lookups themselves. The stub's state,headRefOid branch learned the pr_closed scenario its state-only sibling already knew. * fix(ci): anchor the already-posted guard on the run's creation, and say when it cannot run Round 1's two blockers, both re-verified against this repo's own run data. The time anchor reset on job re-runs. `gh run view --json startedAt` returns the LATEST attempt's start while the run id stays the same — the dedup above relies on that stability — so a re-run pushed attempt 1's review outside "this run": runs 32219268680 (created 05:23:57Z, startedAt 05:51:26Z) and 32218596441 (05:13:04Z → 05:22:05Z) both show the ~9-28 minute shift. Attempt 1 posts its review, the job fails after the post, someone re-runs it, attempt 2 fails before posting — and the guard, anchored on attempt 2's start, lets the contradictory comment through. Exactly the shape this PR exists to stop, on the path most likely to reach it. Both sites now anchor on `createdAt`, which is attempt-stable; a review submitted after the run was created still cannot belong to an earlier run, so the stale-review protection is unchanged. The guard also swallowed its own lookup failures. A transient failure in either call emptied the value, the guard declined, and the false comment posted with nothing in the log separating "the guard ran, nothing matched" from "the lookup died" — while every sibling lookup in these steps announces its failures. Both unavailable paths now emit a `:⚠️:` and a step-summary line before posting. No behavior change: posting was, and remains, the fail-open direction. Tests: a re-run fixture per site, where the stub answers `createdAt` and `startedAt` with DIFFERENT values and attempt 1's review sits between them — reverting either site to `startedAt` fails exactly these two; and a per-site assertion that both unavailable paths announce themselves. Also from round 1, both verified before taking: the stub's standalone `*state*)` branch is dead (no `--json state` call remains in either extracted step) and is removed, so its scenarios cannot be edited into a no-op; and the harness now substitutes `${{ vars.* }}` before running the in-job script, which bash rejected as a bad substitution — the assignment was skipped, `MAX_TIMEOUT_MINUTES` stayed unset, and eight error lines rode every suite run, so "the step's real bash" was not quite true for that line. * fix(ci): read the head this run reviewed, and claim only what the guard proved Round 2's six, all taken. The fallback JOB compared review commit ids against the PR's head at fallback time, not the head the run reviewed. On every trigger but pull_request_target the head-moved guard above deliberately does not run, so a push landing between the post and this step leaves that value pointing at bytes no review ever covered: the match fails and the contradictory comment posts anyway — the #9342 shape, re-opened for the trigger + post + push + fail-after-post interleaving. `review-pr` now publishes the head its review step recorded as a job output, and the guard reads it, falling back to the fresh head only when the job died before that step (a run that posted nothing either). The in-job twin needs none of this — its unconditional head-moved check exits first — and that asymmetry is now pinned per site rather than left to be rediscovered. Both skip messages claimed "this run already posted a review". Reviews carry no run id, so the window (bot account + head + submitted at or after this run was created) also matches an overlapping sibling run's review, which this workflow's own concurrency note says can happen. The suppression is right either way — a review IS sitting above the comment — but the oncall reading the summary was told something the guard never proved; both now say what it did. The guard's opening paragraphs still described the round-1 `startedAt` anchor while the code (and the paragraph below it, and the runtime warning) said creation. A maintainer reading top-down got the anchor that re-runs break — the defect round 1 removed. Test stub: `gh run view` now answers by running the caller's own --jq over an object carrying both timestamps, instead of a `case` on "$*" that matched substrings in order. A combined `--json createdAt,startedAt --jq '.startedAt'` was answered from the createdAt branch, leaving the re-run pin green for a guard reading the attempt-scoped field — the exact regression it exists to catch. * fix(ci): attribute the guard by time alone — the head is not a stable run attribute Round 3's blocker, and the second time the head clause re-opened the contradiction this PR exists to close. Two entrances this round, both after a "Re-run failed jobs": attempt 2 dies before the review step writes its head, so the guard falls back to a head attempt 1 never reviewed; or a push lands and attempt 2 records the NEW head — in both, attempt 1's own review no longer matches `.commit_id`, and the fallback posts "failed before a review could be posted … retry" beneath the review the same run had posted. Rather than patch the head lookup a third time, the head clause is gone. What the guard proves is now narrower and stable: a bot review of this PR was submitted while this run was alive — bot account plus the attempt-stable `createdAt` window. That closes both entrances at once and takes the round-2 cross-job wiring with it (review-pr's `expected_head_sha` output and the env line that read it), so there is no untested chain left whose silent breakage would restore the fresh-head comparison. The job-level step no longer needs the PR head either and reverts to its state-only query; the test stub's state-only branch, removed in round 1 as dead, has a caller again. The comment blocks now state the guarantee the concurrency model actually supports. They claimed a review inside the window "cannot belong to an earlier run", but per-run concurrency groups deliberately allow overlapping runs on the same head, so an earlier-created run's review can match and this run's failure then goes unannounced. That is accepted, and said plainly: the silence coincides with a bot review a reader can see — the very state that makes the comment's claim false — while the bot-author and creation-time clauses still rule out silence with no review at all. Tests: the moved-head case flips from "posts" to "silences" and is pinned per site (a review on ANY head inside the window silences); re-introducing a head clause fails exactly that test; and a structural pin asserts the wiring is absent rather than merely unused. * test(ci): skip the guard's jq-driven cases where jq is absent, instead of failing them The stub answers the guard's reviews and run-view lookups by running the caller's own `--jq` filter — that filter IS the thing under test — so those cases need jq on PATH. A reviewer running the suite on Windows without jq saw them as failures of the guard rather than as untested, which is the wrong signal in the wrong direction. Probed once per run and skipped honestly. Measured with a jq that exits 127: the file goes from 31 failures to 26 failures plus 13 skips — the 26 are the retry-loop cases, which have parsed the review log with jq since long before this change and are equally untestable without it. GitHub's windows-latest image ships jq, so CI coverage is unchanged either way; what changes is what a jq-less machine reports. * docs(ci): remove the head-keyed leftovers the guard no longer has Round 5's four, all leftovers of the round-3 design change rather than new behavior. The job-level block still explained why it compared against the head this run reviewed — naming `pr_head`, "the reviewed head's review" and a `review-pr` job output, none of which survive: the shipped filter is author scope plus the creation-time window, and the wiring was deleted with the head clause. A maintainer reading it would look for a comparison that is not there. The in-job block stated the createdAt-not-startedAt rationale twice, once with the measured run ids and once without; the measured one stays. Same in the tests: the stub's comment listed a head clause the filter deliberately does not have (`attributes by TIME, not by head` is the test that pins its absence), and the harness still declared and injected `reviewedHead`/`REVIEWED_HEAD_SHA`, which nothing reads since the wiring went — a knob that looks live and cannot be. * docs(ci): drop the duplicated anchor rationale and the last stale-head leftovers Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): exclude foreign same-account approvals from the already-posted guard * fix(ci): attribute the already-posted guard by composed-review markers The foreign-approval exclusion list shipped incomplete: the triage skill's commit-pinned APPROVE body also posts under the same account, matches the guard's author and window clauses, and silenced the fallback for a genuinely dead run — the failure shape this guard exists to stop. The producer set is open, so no exclusion list can be finished; every miss fails in the dangerous direction. Match positively instead: a review silences the fallback only if its body carries what only this pipeline's composed reviews carry — the "via Qwen Code /review" attribution footer or the invisible qwen-review-ledger marker. Every composed body carries at least one (a zero-findings APPROVE included); no foreign approval carries either. A marker that ever changes shape stops the guard firing and the comment posts — the pre-guard status quo, not a masked dead run. --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
375ff49752
|
feat(review): tell the author why a review loop is not settling (#9461)
* feat(review): tell the author why a review loop is not settling The convergence advisory design's diagnosis half (#9278), reading the telemetry #9413 started recording. A push-triggered review plus an agent addressing its findings is a feedback loop whose gain can exceed 1, and measured on this repository the loop has carried PRs to hundreds of open threads — one closed unmerged at ~500. The posting floor damps the volume; nothing told anyone WHY a particular loop was not settling. Two signals fire the observation, both comparisons between this pull request's own rounds. Recurrence: a file that carried a finding in an earlier round and carries more now, joined by FILE and deterministically, with the prior rounds read off the carried ledger ids so the rendered sentence is checkable against the PR's own history (title similarity was considered and dropped — model-written, 80-char-capped titles are noise at exactly the length where a match would matter). And volume that is not shrinking, from round 3, because two rounds give one step and a step is not a trend. There is no threshold anywhere in it. A volume bar is somebody's policy, and a policy the tool owns is one it would have to defend on repositories it knows nothing about: a PR diverging at 40 comments deserves the reading a threshold of 100 would have delayed, and a large review whose findings are shrinking deserves no interruption at all. It decides nothing. No finding is withheld, no verdict capped, no cap state added — the paragraph says so in its own last sentence. The recommendations stay at the process level (triage the cause, split an independent cluster, batch the fixes, drop the floor) and never prescribe how code should be restructured, because this module cannot verify such a claim and an unverifiable claim is what the rest of this pipeline refuses to post. A round that looks healthy produces no diagnosis at all rather than an empty section. * fix(review): a loop that posted nothing is settled, not stalled (triage round 1) Both triage nits, and the second was a real inversion. `0 >= 0` is arithmetically "not shrinking" and semantically its opposite: a round that posted nothing is precisely where a settling loop lands, so the volume signal narrated "the volume is not falling" at the moment it had finished falling — flagging the settled state as the unsettled one. The signal is about a loop still producing comments at an undiminished rate, so it now requires this round to have produced some. Both zero shapes are pinned: flat at zero, and the fall to zero from above. The Approve branch's separator condition now counts the convergence block. That combination is unreachable — an APPROVE is composed from zero findings, so neither signal can fire there — and it is listed anyway, with the reason recorded: a condition that is right only because another rule makes its input impossible is a trap for whoever changes that other rule. (A test asserting it was written and then deleted rather than shipped: it could only have passed vacuously.) * fix(review): harden the convergence diagnosis against round 1 review Round 1 on #9461 found two blockers and fifteen suggestions. The two blockers: - The diagnosis received an unclamped `prevRound + 1` while every other public round surface in the composer clamps to `LEDGER_MAX_ROUND`, so at the cap the prose named round 10001 beside a marker stamping 10000. - Cluster paths are PR-controlled and were spliced into the posted body inside hand-spelled backticks. A filename carrying a backtick or a newline broke out of the code span and rendered as live Markdown — a working @mention, a forged body line — in a body this bot posts under its own identity. `mdField` moves to `lib/md-field.ts` and both templates go through it. Signal correctness: - Carried re-posts of still-standing findings no longer count as fresh activity. Step 6 re-posts every unfixed ledger Critical under its original id, so one unfixed blocker fired both signals every round at exactly the steady state they exist to distinguish from divergence. - The volume trend needs a non-zero predecessor: `N >= 0` is true for every N, so restarting from a settled round fired on the healthiest shape there is. - Drafted paths are canonicalised to `LEDGER_MAX_FILE`, the cap the other side of the join already applies, so the signal is no longer blind to paths past 200 characters. - The previous work list is gated on its own round, like the volume already was; a side file with no usable round no longer seeds citations. Evidence and advice are now qualified rather than presented bare: a truncated work list (`dropped > 0`) and one recovered from another account's marker are both disclosed in the rendered sentence, and the floor recommendation is dropped when the round already resolves to `--severity-floor critical`. `pr-context` persists that provenance. Also: the block carries `trim: 1` so an advisory paragraph yields before the disclosures that qualify the verdict; the path tie-break compares code units instead of consulting the runtime locale; the English closing claim is scoped to the observation the way its Chinese twin already was; the `(body)`/`(unknown)` pseudo-paths and the carried-id readback each get one shared statement instead of two; and the diagnosis is composed beside `postedInline` so its count and the marker's cannot drift. Seventeen mutations, one per load-bearing decision, each verified to turn a named test red. * fix(review): close the round 2 findings on the convergence diagnosis Three blockers: - The overflow ladder's notice surfaces all key on the trim RANK, not on what actually dropped, so sharing rank 1 with the deferral list made a round that shed only the convergence paragraph announce a "deferred-findings list" that never existed and point the author at artifact entries that do not exist. The block gets its own rank, shed before every other, named for itself and carrying no artifact pointer. - `parseLedger`'s squat filter is anchored and untrimmed while every reader downstream trims, so an id like ` R9999-1` passed the filter that exists to stop it and then took full effect — pre-claiming the next round's id prefix and citing a round no account ever ran. Admission is now the whole grammar, at the single entry point; `LEDGER_ID_SHAPE` moves to ledger.ts and presubmit imports it rather than restating it. - The foreign-provenance caveat was gated on the recurrence reading, but the previous round's VOLUME comes from the same marker and the volume-only branch cites it as this loop's baseline. A foreign winner now loses `posted`/`prevPosted`/`floor` at the recovery seam the way it already loses the anchor, and the caveat attaches to any reading that cites previous-round evidence. The volume trend also stops reading a POSTURE change as loop divergence: the round's resolved floor is stamped beside the volume it qualifies and sheds with it, and a previous round under a different floor makes the trend unevaluable rather than true. A floor never recorded is not a floor that differs, so pre-field markers evaluate as before. Also: the advice names an auto-resolved floor the way the enforcement note in the same body does, instead of claiming a `--severity-floor critical` flag nobody passed; the pseudo-paths become a structural flag on the finding, so a real file named `(body)` clusters like the file it is; the re-post exclusion fails toward "carried" where the id space collides at the round cap; and `mdField` no longer emits a bare backtick run for a value that strips to empty, which two planted filenames could pair into a span around the bot's own prose. Fourteen mutations, one per load-bearing decision, each verified to turn a named test red. * fix(review): close the maintainer review of the convergence diagnosis Nineteen findings on the diagnosis and the ledger it reads. The ones that changed a judgement rather than a line: **The trend was measuring the wrong number.** Step 6 re-posts every unfixed ledger Critical under its original id, so the re-post floor only ever rises: a loop whose NEW findings collapsed from five to one still posts more comments than the round before, and a trend on the totals called that convergence "not falling" — permanently. The marker now records `fresh` beside `posted`, the trend runs on it, and a predecessor that recorded no fresh count leaves the trend unevaluable rather than measured on the wrong number. A carried id must also NAME a standing entry: models emit stray ids at the head of a claim line, and trusted on the token alone a genuinely new finding vanished from both signals. **The sort key inverted the premise of the advice it ranked.** The previous round's ledger is a POSTING set, so `priorRounds` deepens only where nothing is being fixed — and the paragraph then explained the top-ranked file as "a cluster that keeps producing siblings", about a file where no fix happened. It is also the key a stranger could set: one marker holding fifty legal ids on one file put a fabricated cluster in the top slot and evicted a genuine one. This round's new-finding count leads now. **Two readings, one branch.** The renderer discriminated on the clusters alone, so on the shape this feature exists for — recurrence and a flat trend together — the volume sentence and the entire floor recommendation were unreachable. Both are reported when both fire. **Trust seams.** `parseLedger`'s admission test is now shared with compose-review's side-file read, which restated two of its checks and skipped the rest; `dropped` counts what the filter rejected, so a shortened list no longer reads as complete and still certifies its anchor; the serializer re-validates an id after the length cap, which could cut a token mid-grammar; an unrecognised `k` is normalised rather than dropping the finding that carries it; an unknown identity is no longer read as "another account", which was stripping this account's own telemetry on a `gh` blip; foreign provenance is sticky while the work list can still carry foreign-minted ids, and records whether the list was MERGED over own entries, so the caveat stops over-claiming; and the union that protects own findings from a foreign winner now protects the own volume beside them. **Records that were false.** The convergence paragraph is returned on the result and printed as `CONVERGENCE:`, because the ladder sheds it first and the trim notice promised a terminal copy that did not exist; the floor is stamped only when the state named one, since `Ledger.floor` reserves absence for "not recorded"; and an absent floor folds to `auto` for what the round REPORTS while enforcement keeps its strict reading, so a posture the module had to guess at can still not move a finding out of the posting set. Also: the recurrence signal is held to round 3 like its sibling; the join matches a truncated ledger entry by prefix instead of truncating the drafted path, which was creating the prefix collisions it meant to avoid and posting a 200-character prefix as a real path; unmarked drafts are not counted as findings; `draftedFindingsOf` guards a non-array like its siblings; one shared projection replaces the hand-kept volume-group field lists at both persist seams; and the paragraph's REQUEST_CHANGES path — the verdict a diverging loop actually produces — is pinned. Twenty-two mutations, one per load-bearing decision, each verified to turn a named test red. * fix(review): close the bot's round 3 on the convergence diagnosis Four blockers, three of them introduced by the previous round's own fixes: - The marker's `fresh` count called `isFreshDraft` without the carried-id set while the paragraph ten lines away used the real one, so one body stated two volumes for one round — and the marker's undercount persisted as the next round's `prev.fresh`, where the trend's own guard reads it. The `EVERY_ID` default's premise was simply wrong: `convergence` is in scope at that call site. - The anonymous counter-advance branch hard-wrote `foreign: false` while keeping the work list verbatim, destroying a flag vouched under a known identity and making the sticky clause unable to ever re-fire. Both provenance flags now ride with `...kept`, like the list they describe. - The sticky `merged` term overrode a recovery that did not merge, so a pure-foreign winner inherited a claim that own-certified entries exist when every entry was a stranger's — the shape the union guard one function up refuses for the same reason. - `isLedgerFinding` bounded the id round only from above, so `R0-1` was admitted while `birthRound` rejects round 0 and `isFreshDraft` reads that rejection as "fresh": a re-posted `R0-*` counted as first-time work every round and narrated divergence at a settled steady state forever. Of the thirty-three suggestions, the ones that changed behaviour: - The side-file read counted only the marker's `dropped`, so entries its own admission test rejected shrank the work list while it still read as complete — the invariant this branch wrote into `parseLedger` two rounds ago, violated on the second read path. - Truncation qualifies BOTH readings, not just recurrence: the work list IS the carried-id set that defines freshness, so re-posts of shed entries read as first-time work and can manufacture the very trend that went undisclosed beside them. - The depth tiebreak was still a number a stranger sets, and ties are the ordinary shape — fifty planted ids on one file still evicted a genuine cluster. Depth is dropped entirely when the work list is foreign. - Carried ids are deduped the way `idFor` dedupes them, so a duplicated re-post is not read as carried here while the ledger mints it fresh. - `isLedgerFinding` gained the `LEDGER_MAX_ID` bound (admitted then sliced, an entry changes identity between rounds); `dropped` is clamped through the shared reader (it renders into the model-facing PARTIAL line and cannot be re-ruled); the anonymous seam drops `model` beside `sha`; the artifact validator carries `convergence` and `postedFresh`; and `pickVolume` makes the union's restore derive from the same list `withoutVolume` strips by. The rest were test gaps and comment drift from the previous round — a vacuous round-3 guard, a same-round comparison no test reached, a "terminal copy" test that never triggered the ladder, the `k` flag with no round-trip, and several comments still describing the design they replaced. All corrected. Fourteen mutations on the load-bearing changes, each verified to turn a named test red. * fix(review): close round 4 on the convergence diagnosis Three blockers, all on the write side of the ledger: - `serializeLedger` validated ids AFTER the length slice, and a cut can still match the grammar — `R3-` plus twenty-two nines slices to a well-formed twenty-four — so the marker carried a DIFFERENT id under the same entry. The next round's readback of the posted claim returns the full id, matches no ledger entry, and the finding retires with no ruling while the list reads as complete and the anchor still scopes past it. Admission now runs before anything is sliced, as it does on the parse side. - The `rejected` term added last round re-opened the unbounded-count hole the `declared` clamp beside it closes: `raw.findings.length` is attacker-chosen, and the sum is interpolated verbatim into the model-facing PARTIAL line. The SUM is clamped now, not one of its terms. - `idFor` kept a claimed carried id without checking it names a standing entry, so a model-emitted stray id was recorded under a fabricated mint round that the next round then CITED in a posted paragraph. Last round's fix corrected the counting side and left the recording side asymmetric. A claimed id absent from a COMPLETE work list is re-minted; over a shortened list, or with no recovered predecessor at all, it is kept — a non-member there may be an entry the byte budget shed, and continuity wins where the two cannot be told apart. Also: the own-volume restore no longer inherits the union's non-empty-list gate, so a clean own round's true-zero baseline survives a stranger's marker; the depth-key drop's `foreign`-only keying, the sticky `merged` clause's clear-on-empty, the volume group's survival through the ordinary recovered write, and the unknown-identity guard are each pinned; and the nine unasserted Chinese halves of the convergence paragraph now have assertions. One correction to a reply I posted last round: the round-3 volume-gate test was NOT fixed — the edit aborted on a failed pattern match and I reported it as landed. It is fixed here, and the mutation removing `input.round >= 3` now turns it red. Nine mutations, each verified to turn a named test red. * fix(review): close round 5 on the convergence diagnosis Four blockers, three of them asymmetries between two sides of a rule this branch already states: - The freshness determinations never received the work-list COMPLETENESS signal that `buildLedger` receives, so over a shortened list the marker kept a re-post's id (continuity wins) while the fresh count called the same comment first-time work. One marker said two things about one comment, and a loop doing no new work posted "the rate of new findings is not falling" every round. `isFreshDraft` now takes the flag and applies the same rule. A `findings` field that is not a list at all also stops reading as a COMPLETE empty list — it is a read that knows nothing, not a round that held nothing. - The floor-change guard was blind under the DEFAULT configuration: the marker recorded a posture only when the state NAMED one, while every consumer folds an absent floor to `auto` and resolves it determinately from the round and the context state. A real transition — round 6, or one transient context failure — left the guard nothing to compare and read as loop divergence, the misreading the field exists to prevent. The resolved posture is recorded now, from the same fold the advice and the enforcement backstop already use. (This reverses half of the round-3 fix, which took both alternatives that finding offered; only the fold was needed.) - The truncation caveat named the round undercount but dropped the overcount clause when no previous volume was cited — while the facts clause cites this round's fresh count unconditionally, and a shortened carried list inflates exactly that number. The overcount clause is unconditional now. - `buildLedger` stamped `line` under a looser guard than the admission test it feeds, so a model-written `12.5` made the serializer refuse the WHOLE entry: a posted finding retired unruled, the round was mislabelled budget-truncated, and the anchor was withheld. The line alone is dropped. Also closes the two bilingual parity gaps still standing from round 4 — the Chinese advice had no negative-direction assertion, and the already-at-floor branch's Chinese batching clause was pinned by no test on that branch. Seven mutations, each verified to turn a named test red. * fix(review): close round 6 — five blockers, all on rules this branch states - `isCarry`'s continuity keep admitted ids the serializer's own admission test refuses (over the length cap, round 0, a round ahead of this one). Kept, the entry is then refused WHOLE: a posted finding exits the work list owing no Step-6 ruling, the round is mislabelled budget-truncated, and the anchor is withheld. Continuity covers ids a shortened list may have shed; it cannot cover ids no list this pipeline wrote could hold. Re-minting costs the entry its cross-round id and nothing else — the same trade the integer-line guard makes. - The truncation caveat named the direction truncation mechanically DISABLES. The stray-id rescue is gated on the list being whole, so over a shortened one a genuinely new finding written under an earlier round's id cannot be rescued and reads as a re-post: the count is UNDERstated, not overstated. Both languages and the two comment blocks corrected. - An unrecognised (present, non-empty) `severityFloor` folded to `auto` in the reporting reading only, so the body claimed the round "already resolves to a critical posting floor" while its own deferral-licence clause said the floor carried no recognisable value and the enforcement backstop moved nothing. Only genuine absence folds now. - Restoring the own marker's volume onto a foreign winner re-paired own round-M counts with the winner's round N, so the paragraph attributed own numbers to a round this account never ran — "the previous round posted 0 (0 new)" beside a cluster citing a round that plainly did post. The restore applies only when the two markers describe the same round; absence already reads as "not recorded", which beats a wrong pairing. - The sticky `foreign` clause keyed on the NEW list's length, so one stranger's empty LGTM marker adopted before this account's first finding stamped a provably all-own work list foreign forever — and dropped the cluster sort's depth key over a list with zero fabrication risk. Both ends are required now: a list that could carry an id, and one that can still cite it. Six mutations, each verified to turn a named test red. * fix(review): close round 7 — two blockers on values this pipeline re-reads - `mdField` neutralised the code-span grammar and left the ledger marker's. The span makes Markdown and HTML inert to a RENDERER; this pipeline's own readers scan the raw body, and `stripLedgerMarker` takes the FIRST `<!-- qwen-review-ledger` it finds. A path named `a <!-- qwen-review-ledger .sh` — git permits it, script-lint records it verbatim, and this sanitizer is what renders it — makes the next round's strip swallow everything from the forged opener to the real marker's `-->`, deleting that round's prose AND its marker from the model-facing rendering. A complete forged pair additionally parses as the recovered ledger on any round the real marker is missing, which this account then trusts as its own. The delimiters are broken in the strip now. - The anonymous whole-write branch persisted the winner's volume verbatim under `foreign: false`. Without a `me` every marker walks as foreign, so the upstream strip never fires and `ownMax` is 0 — any marker inside the headroom wins. Kept, a stranger's counts became this loop's baseline: the trend evaluated against them, the paragraph cited them as own history, and they were stamped into this account's own next marker as `prevPosted`, which later recovery trusts. It takes the same "not recorded" degradation the counter-advance branch already accepts, through the same projection. Two mutations, each verified to turn a named test red. |
||
|
Shaojin Wen
|
5715782279
|
feat(review): post --comment reviews to Aone Code via the a1 CLI (#9491)
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
npm cache producer / Save npm cache (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
Security Checks / Dependency CVE audit (push) Waiting to run
Security Checks / Secret scan (TruffleHog) (push) Waiting to run
* feat(review): post --comment reviews to Aone Code via the a1 CLI The Aone chain of /review was read-only: a review of an Aone MR ran fully but `--comment` refused. This lands the Phase 3 submit slice — an authorised run now posts through `a1`: one comment per inline finding, then the summary comment, and `a1 repo mr approve` on an APPROVE. Aone has no native request-changes state, so that verdict posts a blocking summary header and leans on the discussion merge gate; the terminal names the difference. Writes ride a no-retry transport (a transient retry after an accepted write would double-post). The commit_id gate GitHub enforces server-side lives in the provider as a pre-write head-drift refusal, and a mid-batch failure reports exactly what landed with exit-3 do-not-re-run advice instead of a retryable throw. The recorded-but-hostless refusal stays fail-closed, now between two writable platforms. * fix(review): count an accepted-but-unreadable Aone answer as posted The triage review of #9491 flagged the gap: if `a1` ACCEPTED a comment but answered unparseable JSON, the read-back threw before the post was counted — the partial-post report undercounted by exactly that comment, and if it was the first, the do-not-re-run advisory did not fire at all, so a retry would double-post it. Split the read-back semantics: an exec failure still propagates (the write genuinely failed), but a succeeded exec whose answer fails to parse now degrades to "landed, result unreadable" — counted as posted, only the id dropped. The fail-open empty-sourceBranch drift gate is left as the deliberate, tested trade-off it was reviewed as; it is now named in the Phase 3 design-doc note beside the Q4 follow-ups. * docs(review): record the Aone write-path trade-offs in the Phase 3 note Names the two deliberate trade-offs the triage review of #9491 flagged for the Q4 era: the head-drift gate is fail-open on an empty sourceBranch, and the created-comment id read-back is best-effort — plus the tolerant read-back semantics the follow-up fix introduced. * fix(review): close five write-safety holes in the Aone submit path The maintainer review of #9491 found five ways the new Aone write path could post to the WRONG platform or double-post; all five closed: 1. The target-platform-unbound refusal never read the --host flag it names as the remedy, so the --user-authorized re-run refused again forever. An explicit flag on the re-run is platform proof and now lifts the refusal. 2. The write gate compared hosts with raw equality, but Aone is one platform under two names (the CR URL records the web host, the skill's --host rule carries the git host). Hosts now bind through hostsEquivalent, like every other host comparison in remote-match. 3. Platform routing keyed on the family wildcard and the ambient GH_HOST, so a ghe.alibaba-inc.com GHE export selected a1, and a recorded Aone host outranked an explicit --host github.com (the opposite of the registry's documented precedence). Write routing now keys on the canonical Aone pair (isAoneCanonicalHost), never consults the ambient GH_HOST (reads never detect from it), and lets an explicit --host outrank the recorded binding in both directions. 4. a1 takes a comment body as one argv element; Linux caps that at 131072 bytes while compose-review's cap counts characters, so a long bilingual CJK summary died with E2BIG only after every inline had landed. A size gate now refuses the whole batch before any write. 5. An accepted-then-died write (timeout after the POST committed, a reset mid-response) read back as not-landed, suppressing the do-not-re-run advisory and inviting the double-post a1Once exists to prevent. Exec failures now count as possibly-landed (ambiguous), so the advisory fires even when the count is zero. * fix(review): harden the Aone submit path per the verify-lane review The sandboxed-verification review of #9491 (8 Critical, 24 Suggestion) caught the next layer of the write path; every finding addressed: Platform selection: - The unbound refusal now also fires when NO recording exists at all: a --user-authorized publish from another directory finds nothing, and the cwd probe alone must not pick the platform of an irreversible write. Tests that modeled the old cwd-fallback now model evidence: args() seeds a recording, session-scoped recordings are seeded where a session id is set. - The gh write rebinds its routing host to the evidence that selected it (explicitHost ?? recordedHost) — a recorded GHE host no longer posts wherever the ambient env pointed. - Host comparison is normalised ONCE (case/port/trailing-dot) and shared by hostsEquivalent and isAoneCanonicalHost, so the write gate and the router cannot normalise differently; a port-bearing CR-URL host no longer dies at the gate after the whole review ran. - The fast-path repo axis binds case-insensitively (GitHub resolves owner/repo case-insensitively server-side). - The cross-session recording scan is last-writer-wins by mtime; the NEWEST same-PR recording decides (its host, or unbound) instead of an older session's stale host masking the newest recording's hostlessness. - recordedSeverityFloor binds through hostsEquivalent too — the floor recovery no longer silently discards the operator's floor on the web-host/git-host alias. Reporting: - a1Cause reads the captured stderr, not the execFileSync message: the message embeds the full argv — the entire multi-line comment body — so parsing it surfaced the operator's own review text, never a1's error. - The REQUEST_CHANGES terminal note is conditioned on the inline Criticals actually posted — a body-only Critical posts no discussion threads, so nothing mechanically blocks the merge and the note says so. - The summary skip-guard keys on the posted summaryMessage, not the raw body: an empty-body REQUEST_CHANGES still posts its blocking header, the verdict's sole carrier on Aone; the size gate measures the same message. SKILL.md: the mid-batch bullet no longer commands hand-posting the remainder (it contradicted the write-monopoly rule); it names the oversized-comment refusal as the third Aone-specific shape; the relay-the-link fallback stops assembling Aone links from meta's collapsed owner/repo (a nested-group hazard) and relays the target's coordinates. Tests: 24 mutation-driven hardening cells (ordering via invocationCallOrder, exact argv pins, boundary cells at 131071/131072, RC-header accounting, stderr-over-message, summary-create failure, accepted-then-unreadable counts, attribution-off passthrough, url-absence arm, positive read-retry). 3861 review tests green. * test(review): pin hostsEquivalent's alias equivalence across spelling variants * fix(review): close the third-layer holes in the Aone submit path The third review round of #9491 found the layer under the last one; every finding addressed: Platform selection: - The cwd arm of the write gate probes the origin through the CANONICAL predicate itself instead of delegating to the registry's family-wildcard detection: a ghe.alibaba-inc.com origin no longer takes the a1 path. - submit FORCES context-unavailable into the compose input on the Aone path: the cap no longer rides the model-written state, so an omitted contextUnavailable cannot compose an APPROVE that the a1 path turns into a real platform approval. The docs now say the native approve does not fire this phase. - The floor recovery's host axis binds to the host the write routes at (explicit ?? recorded ?? gh fallback): a flagless Aone post no longer drops the operator's recorded severity floor. Failure shapes: - A mid-batch failure emits "partial": true with the landed counts and comment ids: posted:false alone invited a wrapper retry that double-posts what landed. The summary's fate is stated when it was the write that died. - A deliberate pre-write refusal (drift, oversized) reads as aone-post-refused; an UNEXPECTED pre-write error rethrows, gh-parity — nothing landed, a re-run is safe, a recoverable blip no longer loses the authorised review. - The batch re-reads the head once after posting and discloses a mid-batch amend (headMovedDuringPost) instead of claiming the pins held. Text: - The approve-failure WARNING and the oversized refusal name the USER as the manual actor — "by hand" is never an agent action, and Step 7's ban now says so. - SKILL.md: restored the exact subcommand enumeration, qualified the cleanup tripwire as GitHub-only, taught the completion contract the partial/approved shapes, and documented the repeat-round caveats (no dedup backing yet, no self-PR detection on Aone). Tests: cwd canonical-arm cells (GHE family origin falls to gh), the forced-cap wiring, structured partial JSON, refusal-vs-failure reasons, the rethrow cell, missing-sourceBranch key, mid-batch drift disclosure, and the a1JsonOnce transient-no-retry invariant. 3870 review tests green. * fix(review): close the residual holes the sandboxed verify report found The follow-up-2 sandboxed verification of #9491 (155 scripted assertions, 153 pass / 2 fail) re-measured everything at the new head and surfaced three findings; all three closed: - F1R: a failed write exiting with EMPTY stderr (the 120 s deadline kill, SIGKILL/OOM, an a1 crash before writing) made a1Cause fall back to parsing the exec message — and Node embeds the FULL argv in it, so the "cause" quoted a line of the operator's own review body. The fallback now reports exit facts only ("a1 failed without stderr (exit N) / (signal X)"), never the argv-bearing message. The dominant shape (real a1 error on stderr) is untouched. - F3: the forced context-unavailable wiring on the Aone path survived 3870 tests unpinned — submit's compose now has a cell asserting the compose input carries contextUnavailable: true on the Aone path and false on the gh path. - F2: createdCommentId's result/data nestings were correct but unpinned (a key-drop mutation survived). A new cell pins ids read back from {result:{id}} and {data:{id}}. * fix(review): close the round-5 platform-binding holes in the Aone write path - Order the cross-session recorded-args scan by each recording FILE's mtime (writeSkillArgs rewrites in place, so the directory mtime never advances); fold the publishing session's own recording and the sessionless root recording into the same newest-wins ordering instead of pinning them ahead of (or behind) the sorted siblings. - Refuse an explicit --host that contradicts the recorded host (target-platform-conflict): the flag fills a gap in the recorded evidence, it does not retarget the recorded review at another platform's same-named repo. The Aone web/git alias still passes through hostsEquivalent. - Fail closed on a hostless recording read via the --skill-args override: the submission cwd's origin probe names submit's clone, not the review's, and must not stand in for the missing platform evidence; the --host remedy lifts the refusal. - Bind the gh routing host to the cwd origin when the cwd arm selected the platform (and mirror the chain in the floor recovery's host axis), so a cwd-selected post no longer restores ambient env inheritance and routes past the clone that chose the platform. - Hand the GitHub path's contextUnavailable claim through raw so compose-review's deliberate shape check still refuses a malformed non-boolean instead of silently coercing the cap away. - Serialize the partial-post ambiguous flag in the stdout JSON and give the partial shape its own completion-line disposition in the skill — never the `not posted` form a retry-on-not-posted wrapper acts on. |
||
|
叶公
|
52e2f01366 | fix(web-shell): show reasoning effort before session creation | ||
qwen-code-dev-bot
|
3e99b71669
|
fix(ui): collapse duplicate in-flight tool_group rendered from history + pending (#9421)
* fix(ui): collapse duplicate in-flight tool_group rendered from history + pending (#9420) The latest tool call rendered twice: the same in-flight tool_group was present both in committed history and the live pending list (or twice within pending), so MainContent's combined virtualized list showed it twice until the next tool call finalized. Collapse consecutive tool_group rows with an identical callId signature, keeping the later (live pending) copy so status keeps updating. Adds a regression test asserting a duplicated tool_group renders once. Fixes #9420 * fix(ui): dedup duplicated tool_group by live pending copy, not adjacency (#9420) * fix(ui): skip tool_group dedup pass when no pending copy is live (#9420) * fix(ui): drop only the latest committed tool_group copy on callId collision (#9420) * fix(ui): collapse duplicate tool_group copies by scheduler batch identity (#9420) * fix(ui): make tool batch ids unique per mount (#9420) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ui): preserve next batch id when a callId is reused (#9420) --------- Co-authored-by: qwen-code-dev-bot <269191875+qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
易良
|
f700f8e345
|
fix(web-shell): fall back to execCommand copy in non-secure contexts (#9540)
* fix(web-shell,webui): fall back to execCommand copy in non-secure contexts The async Clipboard API is only exposed in secure contexts (HTTPS or loopback). The daemon serves the Web Shell over plain HTTP, so opening it through a non-loopback address left navigator.clipboard undefined and every copy entry point failed (issue #9485). Add writeClipboardText() helpers that try navigator.clipboard first, fall back to the legacy hidden-textarea document.execCommand('copy') path, and reject with an actionable secure-context hint when neither works. Route all web-shell copy entry points (code blocks, assistant message copy, session id, timestamps, table copies, git SHA, local-control URL, /copy command) and the webui copy paths (toolcall CopyButton, MessageMeta) through it. * chore: drop webui changes — package will be deprecated Per maintainer feedback, webui is slated for deprecation; keep the clipboard fallback scoped to web-shell only. * fix(web-shell): keep the execCommand copy fallback reliable inside overlays Address the round-1 review findings on the clipboard fallback: - append the temporary textarea inside the enclosing Radix dialog/popper layer so a trapped FocusScope cannot refocus the copy button mid-select and a popover focusin cannot dismiss the overlay during copy - restore focus to the previously focused element after the fallback copy - skip the doomed async write when clipboard-write permission is already denied, keeping transient user activation alive for execCommand - attach a rejection handler to the LocalControlSettingsCard copy button - assert the fallback textarea actually holds the requested text and add regression tests for the layer container and focus restore * fix(web-shell): keep clipboard writeText dispatch synchronous and log copy failures * refactor(web-shell): centralize clipboard write-failure warning in one reporter |
||
|
易良
|
e501628ea0
|
fix(cli): persist skipped duplicate tool results (#9593) | ||
Bob.qwencode
|
808c9c9f3d
|
chore(release): v0.21.14 (#9594)
* chore(release): v0.21.14 * docs(changelog): sync for v0.21.14 * docs(changelog): sync for v0.21.14 --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: 易良 <1204183885@qq.com> |
||
|
易良
|
d6732eaa5d
|
fix(ci): author the release PR with a third bot PAT (#9592)
GITHUB_TOKEN cannot create the release PR because the org disables GitHub Actions from creating or approving pull requests. The repo-level switch is rejected with 409 and the org-level switch requires org admin. Author the PR with CI_REVIEW_BOT_PAT (a third identity) instead, so ci-bot and dev-bot can both approve without self-approval blocking. |
||
|
Shaojin Wen
|
c59910ba3f
|
fix(ci): make autofix finding replies idempotent (#9463)
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
npm cache producer / Save npm cache (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
Security Checks / Dependency CVE audit (push) Waiting to run
Security Checks / Secret scan (TruffleHog) (push) Waiting to run
* fix(ci): make autofix finding replies idempotent A crash-and-rerun of an address round, a same-run repair that regenerates the dispositions, or a later round re-declining the same finding all reproduce the same comment-replies.json entry — and the reply step posted it again, landing identical bot replies on one thread (observed 2026-08-16: one identical reply posted three times, #9296). The thread fetch now also reads each comment's author and body, and the reply step skips posting when the thread already carries a comment by the autofix bot whose body equals the neutralised body about to be posted. A changed body — new information from a later round — still posts; a threads view without author/body, or a stale/empty one, degrades to the old post-always behavior. The replies API itself is already the no-review-event path, so this PR only adds the missing idempotence (the P1 replies item of #9296). Refs #9296 * fix(ci): restore inner pageInfo in autofix threads query and pin reply-gate contracts (#9463) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
027772d2ab
|
test(ci): stage on-disk session state in the serve A/B (#9444)
* test(ci): stage on-disk session state in the serve A/B The serve A/B drives every scenario against a freshly started, empty daemon, so the entire session-admission surface — case resolution, transcript integrity, active/archive conflicts, reserved sources — is unreachable and a PR that rewrites it diffs as "no response changes". #9341 is the worked example: the posted A/B reported no change across 4 scenarios while the same build pair, driven with transcripts on disk, answers differently on six requests. Scenarios can now stage transcripts before their request and capture a reduced projection of the response, and the HTTP status is recorded on every capture so a status-only difference is visible. Six session-admission scenarios use that: a healthy restore, the legacy uppercase spelling, case-only twins, an unreadable transcript, one id in both the active and the archive directory, and creation carrying a source type. The staged fixtures depend on the on-disk project layout, which the harness mirrors rather than imports. If that mirror ever drifts the transcripts land nowhere and every staged scenario would quietly answer 404 on both arms, so the healthy restore doubles as a canary that fails the drive instead of publishing a reassuring all-clear. * test(ci): address the R1 review round on the serve A/B harness Clears the capture directory before a drive writes into it, so a re-run can never let an earlier run's files stand in for scenarios this run did not capture, and writes a completion marker once every scenario is captured. A baseline without that marker is now reported as partial, because a base drive that stopped part-way leaves the scenarios it never reached rendering as "this PR adds these responses" — the same shape a genuinely new scenario produces. Both arms are driven by the head checkout's harness, so a capture pair always carries the status field on both sides and the compatibility shim for a base that predates it was unreachable; it and its tests are removed rather than left to teach a transition the wiring cannot produce. Non-object response bodies are now nested instead of spread, which dropped scalars and re-keyed arrays. The source-type scenario probed a type today's daemon does not reserve, so it never reached the refusal branch it was named for. It is split: one scenario pins the source the daemon actually reserves, the other keeps an ordinary type that a future reservation would move from admitted to refused. A second canary covers the archive directory, which nothing certified before — a drifted archive name would have left the conflict scenario loading from the active copy on both arms and diffing clean. The remaining inert request body key is gone too; the client id is read from a header, never the body. The harness tests were passing under mutations they appeared to cover: the staging routing, the projection guards and the fixed-id requirement are now pinned by assertions that fail when those are inverted. * test(ci): close the R2 gaps in the serve A/B harness The completion marker was declared twice, once by the writer and once by the reader, with nothing pinning the copies together: renaming one side left both suites green while CI would either flag every complete baseline as truncated or stop noticing truncated ones. The drive now owns the constant and the diff imports it. Two invariants the code asserted in comments were not enforced. A response body carrying its own status key overwrote the status the harness saw, so a status-only regression on such a route would have diffed as an unchanged body; the harness value now wins. And the canary check — the harness's only drift alarm — had no test at all: inverting it so it could never fire left every test passing. It is now a named helper with tests on both branches. The archive canary pinned an exact status, which conflates its precondition with the product's decision: if an archived-only load ever becomes loadable, the precondition still held, but the drive would abort and suppress the very row the captures already contained. It now fails only on the one answer that means the staged file was never seen. Finally, nothing pinned that a staged scenario probes an id it actually staged. Staging the wrong id answers 404 on both arms, captures identically, and drops that branch out of coverage with every test green. * test(ci): close the R3 gaps in the serve A/B harness The completion marker proved that some drive finished, never that this run's did. The only reset lived inside the drive script, which does not run when an arm is skipped before it starts — no merge-base resolved, the base checkout failing, or its build dying — and on the persistent pool the capture paths outlive a run. An inherited baseline then arrived complete, marker included, so neither degraded-baseline warning fired and the comment would have diffed this head against another run's base. The workflow now clears both capture paths in an unconditional step, which is the only place that covers a skipped arm. The in-script reset also turned a write-only script into an unguarded recursive delete of a path taken straight off the command line, which the documented local usage invites a reader to mistype. It now refuses any directory that holds something other than captures. The healthy canary's premise was wrong: the product validates transcripts record by record and fails open, so a fixture whose records stop validating restores as an empty session and still answers 200. Measured against a real daemon, a wholly drifted fixture passed the canary and left every staged scenario probing an empty daemon — the false all-clear this harness exists to prevent. The canary now keeps a replay-size witness in its capture and fails when it is zero. Three test gaps behind the same theme: the marker's writer, the comment subcommand that CI actually invokes, and the mixed-case scenario's existence were all unpinned, and the staged-id check asserted against the union of every scenario's staging rather than the one under test. The capture loop is extracted so its ordering is testable without a daemon. * test(ci): cover the setup-failure abort in the serve A/B capture loop The capture loop was extracted so its ordering could be pinned without a daemon, and three of its four abort branches were covered — but not the one that fires when a scenario's setup request fails. Dropping that throw left the whole suite green while a capture would be recorded against a daemon where the setup never took effect, which is the masked diff the branch exists to prevent. * test(ci): close the R5 gaps in the serve A/B harness * fix(ci): send an admitted source in the serve A/B unreserved-source witness --------- Co-authored-by: wenshao <nigolaschao777@gmail.com> |
||
|
Shaojin Wen
|
4807d06b31
|
fix(autofix): mutation-probe new guards before a round commits (#9578)
* fix(autofix): mutation-probe new guards before a round commits
Provenance analysis of six multi-round takeover PRs found roughly a
third of post-initial review findings were introduced by the
immediately preceding fix round, and the dominant shape was guards
and branches added with no test of their own: the deterministic gate
re-runs only the tests that exist, so an unwitnessed guard passes
every gate and its hole resurfaces as a new finding in a later
round. Require the agent to prove each newly added guard or branch
kills a test (mutation probe) before committing, and record the
probe in the round summary.
* fix(autofix): pin the mutation-probe mandate, not just its intro
The original pin stopped at the preposition "before", so a future
edit that kept the intro phrase but gutted the remove/confirm-FAIL/
restore procedure would leave the contract test green. Pin both
halves of the mandate. Mutation-verified: gutting the mandate in
SKILL.md now fails the skill contract test.
* fix(autofix): also pin the mutation-probe remediation clause
The procedure pins cover what the probe does, but not what to do
when it exposes an unwitnessed guard: dropping the remediation
sentence ("write a test that pins it (or drop the guard)") left the
whole suite green while the rule told the agent to probe without a
verdict path. Pin the remediation clause too; mutation-verified.
|
||
|
Shaojin Wen
|
099a71c936
|
fix(ci): heal a symlinked workspace instead of wedging the runner on it (#9498)
* fix(ci): heal a symlinked workspace instead of wedging the runner on it The hardened wipe guard refuses any workspace that canonicalizes outside the runner workspace. That refusal is correct, and it created a permanent failure: when a previous job leaves the workspace replaced by a symlink pointing outside — or by any non-directory — the guard resolves it to the target, refuses, and exits 1 having removed nothing. Nothing else clears that state, so every later job on the runner dies at the same line, forever. The pre-guard code wiped through the link and self-healed by accident. Reproduced against main's own step text before this change. Heal it: the link itself lives inside the runner workspace and is safe to unlink, and only once it is gone can a legitimate wipe proceed. The layer has to sit before canonicalization — afterwards the path has already resolved to the target and the allowlist refuses before any repair can happen — which means it judges a raw path, and that is where the first attempt at this (closed with #9369) went wrong. A raw `"$RWS"/*` match accepts `$RWS/link/sub` as a string while the kernel resolves it through an intermediate symlink to a file outside the runner workspace, so the unlink and the mkdir landed outside and only then did the allowlist refuse the wipe. Here the containment is judged on the canonicalized PARENT — never on $WS, which would resolve through the very link being removed — and the unlink then acts on the raw path, so it takes the link and never follows it. Four more constraints the same review surfaced: the raw trailing-slash strip moves ahead of the predicates (both `[ -L "$WS/" ]` and `[ ! -d "$WS/" ]` resolve through a link and report its target, so one slash hides the corruption); the allowlist root is prepared before the heal, since it bounds it, and an empty $RUNNER_WORKSPACE would degenerate the containment pattern to the match-all `/*`; both the unlink and the mkdir fail closed, because under `-e` a failure that is not the last command of an && list is swallowed and would leave the wipe running on a corrupt path; and the heal logs what it found and where the link pointed, since this incident otherwise leaves no trace at all. All three copies get it — the two triage wipes and the A/B wipe — with per-suite fixtures: the wedge healed (link gone, directory recreated, target's contents intact), the intermediate-symlink attack refused with the outside file unmutated and zero rm calls, the non-directory half, the trailing-slash spelling, the fail-closed unlink, and the ordinary workspace where the heal must not fire at all. Mutation-checked layer by layer; each has a fixture that fails when it is removed. One pre-existing test changes meaning: the canonicalization pin used a symlinked workspace and asserted refusal, which is now the healed path. It moves to a vector the heal does not touch — an intermediate symlink whose far end is a directory — and keeps its mutation strength: with the canonicalization deleted, find resolves the link and hands the outside directory's entries to the rm recorder. Closes #9480 * fix(ci): keep the heal's log out of the workflow-command channel Three findings from the first review round on this layer. The heal logged the symlink's target inside a `:⚠️:` line. The target is bytes a PREVIOUS job chose — on the verify lane that job may have run a contributor's code — and the runner parses `::` at the start of any stdout line as a workflow command, so a target of $'…\n::error::forged' let the step reporting the corruption forge an annotation. The annotation now carries no untrusted bytes: the target is stripped of line breaks, capped, and printed on its own prefixed line, where a leading `::` cannot begin a command. Verified against the real step text — the forged line lands as data, and no output line starts with `::error::`. The mkdir leg's refusal had no executed fixture while its `rm -f` sibling had one. It does not need a permission trick: `rm -f` returns 0 for a path whose parent is not a directory (it reads as "already absent"), and the mkdir that follows cannot succeed — so the branch is reachable, and a swallowed failure there would run the wipe against a path that does not exist. Fixtures in both suites, and it runs as root too. And the post-run triage copy's header still said this copy "predates the checkout-heal hardening and never received it" while carrying the whole guard plus the heal directly underneath. That header is the in-code inventory the eventual convergence of these copies will read; understating it is how a sync strips layers in the wrong direction. * test(ci): drive both wipe copies in the remaining single-step heal fixtures * fix(ci): keep the Serve A/B job from timing out on slow runners --------- Co-authored-by: Qwen Autofix <autofix@qwen-code.dev> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |
||
|
Shaojin Wen
|
6fe9ce4886
|
fix(ci): stop counting wedged queued runs as in-flight in the shepherd (#9518)
* fix(ci): stop counting wedged queued runs as in-flight in the shepherd When GitHub refuses to start a workflow run it still CREATES it: the run sits `queued` forever with zero jobs and cannot be cancelled or deleted through the API. On 2026-08-19 an oversized qwen-autofix.yml produced a run like that from the shepherd's own liveness dispatch, and the watchdog counted it as in-flight for the next 18 hours: last scan signal: 2026-08-19T05:01:14Z (1107m ago), in-flight: 1 The age gate said "dispatch a scan", the in-flight gate said "one is already running", and nothing ever completed the run that would clear it. The loop stayed dark until a human looked. Treat a run still `queued` past ZOMBIE_QUEUED_MINUTES (30, overridable via the QWEN_SHEPHERD_ZOMBIE_QUEUED_MINUTES repository variable) as wedged rather than live. One `wedged` predicate is defined once and reused by the in-flight count, the conflict lever's busy-set, and a new census, so the three readers cannot disagree. Only `queued` runs wedge — a review-address run legitimately runs for hours — and a missing createdAt reads as brand new, so unknown age never licenses a duplicate dispatch. The wedge is now visible instead of silent: a :⚠️: names the count and the oldest one, the tick heartbeat carries `wedged-queued:`, and the dashboard carries a banner. Invisibility is what made this expensive — PR-event runs kept reporting success while every scheduled scan was dead. Verified against the real run list from the incident: the old predicate returns in-flight=1 (starved), the new one returns 0 with a census of 2. Behavioral tests replay both jq programs and the busy-set walk verbatim from the workflow. * fix(ci): keep shepherd busy-set job-verified and bound wedge re-dispatch (#9518) * fix(ci): reject degenerate zombie threshold and name the paused liveness gate (#9518) * fix(ci): name the recorded liveness run in the shepherd wedge remedy (#9518) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> |
||
|
Shaojin Wen
|
a20a724ec0
|
refactor(review): build the incremental scope from the PR's diff, not a check (#9267)
* refactor(review): build the incremental scope from the PR's diff, not a check The containment oracle proved, after the fact, that a separately captured `anchor..head` carried no hunk the PR's own `base..head` diff lacked — because a comment anchored on a line GitHub does not display answers 422 and takes the whole all-or-nothing Create Review call with it. That proof was a hand-written match over two rendered unified diffs, and six review rounds each closed the reported entrances while the next found new ones: count-less headers, deletion junctions, lossy UTF-8 decodes that collapse distinct bytes onto U+FFFD, a budget spent across hunks, content matched without position. Every one was the same shape — something the delta carried that the PR's diff did not display, arriving through a gap in the match. The surface is unbounded because it is a match over arbitrary content. So the scope is no longer checked against the PR's diff; it is built from it. The delta is read only for the post-image ranges it touched, and the published text is assembled out of the full capture's own hunks. Every line the review sees is a line GitHub displays, by construction rather than by proof. The oracle goes, and with it the two refusal reasons that existed to report its verdicts: `hunks-outside-pr-diff` and `containment-unverified`. One reason replaces them, and it names a fact rather than a failed proof — `nothing-to-narrow`, when the PR's diff has no hunk left in the range that changed since the anchor. That is the "undo per feedback" round, where the commits since the anchor put lines back the way the base had them: there is genuinely nothing there to re-review, and the round keeps the full range, which is the review it would have done anyway. The invariant is asserted directly against real-git captures rather than argued per shape: every line of the narrowed output must appear in the full capture, checked over the whole output on each scenario, including the undo-per-feedback history that defeated the oracle six times. A mutant that assembles from the delta's bytes instead fails three of the four. Net effect on the tree: -255 lines of production code and the ~765-line battery that existed to pin it, against +29 and a four-case integration suite. * docs(review): retire the oracle's refusal reasons from the skill #9100 has landed, so the paragraphs this would have conflicted with are settled and the deferred half of this PR can go in. SKILL.md's recovery taxonomy still enumerated `hunks-outside-pr-diff` and `containment-unverified` — reasons the report can no longer carry, since the oracle that produced them is gone. It names `nothing-to-narrow` now, and says what actually produces it: an "undo per feedback" revert, which puts lines back the way the base had them so the PR's diff no longer shows that region, and a capture whose bytes do not survive a UTF-8 round trip. The retry classification moves with it. The old sentence said "the containment reasons re-rule identically"; the new one says the narrowing re-narrows identically, and why — the same two captures select the same hunks, and a capture that failed a round trip fails it again. Both remain deterministic for the same sha and outside the retryable set. Pinned in SKILL.test.ts beside its siblings: moving the reason into the retryable set fails that test rather than shipping green. * fix(review): keep narrowed rounds displayed-only across rename, mode, and huge-hunk shapes - Refuse to narrow when a delta path does not cross into the full capture's keys: git's rename detection can resolve differently across the two ranges, and the unmatched section is a displayed change that would silently drop from the published scope while the round still certified head. The round keeps the full range instead. - Emit a full section whole when the delta touches it without hunks: a since-anchor mode change, pure rename, or binary replacement lives in the section header, and the old guard dropped the whole section while reporting `effective: true`. - Weld `incremental.diffBase` to the merge base, not the anchor: the published hunks are byte-identical hunks of `mergeBase..head`, so Agent 7's test-efficacy probe must recompute that range; the anchor range can carry undo hunks the PR's diff does not display at all. - Assemble the narrowed text without spreading selected hunks into a single `push`: a hunk past the ~125k-line argument ceiling crashed the whole fetch-pr round with a RangeError instead of degrading. - Validate the full capture with a fatal UTF-8 decode instead of re-encoding a full-size copy to compare, removing ~N of peak memory on the large long-lived PR workload. Pinned by real-git integration scenarios for each shape, including the deletion-acceptance control and an unconditional null fallback; the producer→consumer weld test now asserts the merge base end to end. Each new test was verified to fail against the pre-round code. * fix(review): close the rename-key divergence in incremental narrowing - Fail closed on a rename the full capture does not key as the SAME rename: when round 1 rewrites a file below git's rename threshold and round 2 renames it, `base..head` nets the chain to an addition plus a deletion while `anchor..head` carries a 100%-similarity rename keyed on the new path. The path guard passed — the new path is in the full capture, as the addition — while the rename's deletion half sat under the old path and dropped from the published scope under `effective: true`. `parseDiff` now exposes `rename from`, and the narrowing join refuses unless both captures key the same rename; the round keeps the full range, which still displays the deletion. - Fatal-decode the delta symmetrically with the full capture: `narrowToDelta` takes the delta's raw bytes and decodes them itself. A lossily pre-decoded delta folded an invalid path byte onto U+FFFD, which could collide with a legitimate U+FFFD path in the full capture and publish an unchanged file's hunks. - Record the executable bit through git itself in the mode scenarios (`git update-index --chmod=...` beside the filesystem chmod): `chmodSync` is invisible to git on Windows — libuv cannot set the exec bit and `core.fileMode` is false — so the two mode tests failed on the Windows merge-queue leg. Verified against a Windows-git model (`core.fileMode=false`): 2 failed before, 16/16 after. - Assert the narrowing outright where the scenarios are constructed to narrow: the null guards on the undo-per-feedback and post-anchor-file tests let an all-or-nothing refusal ship green with zero assertions executed. - Pin the unpinned emission shapes: a whole-file deletion riding the `+0,0` clamp and inclusive `overlaps`, a mode-only full section the delta touches with content hunks, and both hunks of a two-region file surviving the join — each verified red against the corresponding mutant. - Enumerate all four null shapes under `nothing-to-narrow` in the report's union doc, the demotion arm, and the skill's reason bullet; the routing (deterministic, never retried) already held for all four. The rewrite-then-rename regression test fails against the pre-round code (the probe published [new.ts, other.ts] with the deletion absent); the mode scenarios fail under the Windows model before the index-native recording. * fix(review): keep header-level changes in the narrowed incremental scope A delta section whose hunks all miss the full capture used to be dropped whole, taking a post-anchor mode flip or rename out of the published scope while the round still reported effective. Track header-level delta changes per path and emit such sections whole, the hunk-less treatment. Also close the measured battery-power gaps: the rename guard's pass-through arm, rename-plus-hunks emission, a single delta hunk overlapping two full hunks, the multi-file section drop, the capture-failed disk assertion, the diffBase seam's division of labor, and the skill's reason taxonomy plus retryable set. (#9267) * test(review): close the narrowing battery's mutation holes (#9267) The headerTouched miss-branch tests never asserted the section's surviving content hunks, the delta-side UTF-8 refusal test stayed green with its guard removed, and the battery carried no no-trailing-newline marker pin and no binary delta section. Add the missing assertions and the two scenarios, and rebuild the delta-side refusal test around the U+FFFD collision its comment describes, so each shape now fails under the mutant it is meant to catch — all four mutants probed against the battery. Also name what the merge-base clamp actually prevents in the skill's `base-untrusted` clause, whose "those" lost its antecedent when the containment reasons were retired. * fix(review): carry position-divergent hunks through the narrowing join (#9267) Myers aligns a change inside a run of identical lines against whatever surrounds it, and the two captures' old sides differ — so the same post-anchor change can sit at disjoint head-side ranges in `base..head` and `anchor..head`. The range join then dropped a change the PR's diff displays while the round still reported `effective: true`, and the ledger certified head over it — the change never re-entered any later scope. Fail closed per hunk: a missed delta hunk whose changed lines the full section also changed emits the section whole — every line of it is displayed, and clean siblings still narrow. A netted-out undo contributes no line the full section displays, so the deliberate section drop is untouched. Covers both probe-confirmed shapes: the whole-section miss and the partial miss where a matched sibling hunk kept the file visible. * fix(review): fail closed when a delta hunk lacks a corroborating full hunk (#9267) * fix(review): key changed-line corroboration by new-side junction (#9267) * fix(review): narrow between files, not within them Four consecutive rounds reported the same class and each fix was defeated by the next round's entrance: whole-section miss, partial miss, then two shapes defeating the position-divergence guard's conjuncts, then content-only corroboration, then junction-keyed corroboration defeated by a delta hunk carrying two changes. The reviews were right about the cause each time and right about the pattern: matching hunks across the two captures is a heuristic over arbitrary content, which is what the containment oracle this file replaced also was. The two captures are independent Myers alignments over overlapping content, so which HUNK a change lands in is not stable between them — a run of identical lines lets the same edit be attributed to the run's front in one and its back in the other. Every guard here tried to recognise that divergence; none could, because it is a property of the alignment and not of the change. What IS stable is which FILE a change belongs to, and the path and rename guards already fail closed on the one way that could differ. So the unit of narrowing is the file: a section the delta touched is emitted whole, a section it did not touch is dropped. Nothing the delta performed can fall out of a section emitted entire, and the whole position-divergence family stops existing rather than being caught. That failure direction is the reason this could not stay as it was. A dropped hunk left the round reporting `effective: true`, and the ledger then certified head as the next anchor, so the change was never reviewed by any round. The deleted oracle failed toward more review; this failed toward silently less, recorded as complete. Cost, stated plainly: within a touched file the round now reviews all of that file's PR hunks, not only the ones that moved since the anchor. The saving incremental review exists for is the untouched files — a round touching 2 of 40 reviews 2 — and that is unaffected. Also: a base-free round no longer reports `capture-failed`. The fetch succeeded and `git merge-base` found no common ancestor, so nothing threw; naming an infrastructure fault put a deterministic state into the class the recovery flow retries. It reports `nothing-to-narrow`, and the two pins that asserted the old reason move with it. * fix(review): split a base-free round by why the base is missing The R11-1 fix keyed on `mergeBaseSha === null` alone, but that null has two causes and only one is deterministic. A base that could not be FETCHED — a fresh CI clone with no local base ref, hitting a transient fault — is infrastructure: something did fail, and the re-run re-runs exactly the component that failed. Reporting `nothing-to-narrow` there put a retryable state into the never-retried class and pointed operators at "nothing to narrow" instead of a fetch failure. The arms are split by `baseFetchFailed` now: fetch failed keeps `capture-failed`, no-common-ancestor keeps `nothing-to-narrow`. This is the same distinction SKILL.md's recovery paragraph already draws for a planless `partition-failed`, applied where the code makes the same choice. The pin that was supposed to cover this asserted `nothing-to-narrow` over a `{sha: null, baseFetchFailed: true}` fixture while its comment said "the fetch succeeded" — a fixture contradicting its own comment, which is what kept the gap invisible. It is one test over both fixtures now, each with the reason its cause implies. * fix(review): split merge-base surface failures from deterministic refusals (#9267) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
d0cd5520d8
|
test(review): single-encode the diff-read fixture and pin the failed-read gate (#9539)
* test(review): single-encode the diff-read fixture and pin the failed-read gate Two follow-ups to the diff-read pin added in #9484: - The fixture's launch line was `JSON.stringify(...)` before the trailing `.map((r) => JSON.stringify(r))` encoded it a second time, so `parseTranscript` parsed a bare string and dropped it — `launchPrompt` was silently `''`. Make it a plain object literal like its siblings, and assert `launchPrompt` so the encoding can't regress unseen. - Add a FAILED (`response: { error }`) read of the diff to the fixture and assert `diffToolCalls` stays 1 and `diffReads` stays `[[1, 40]]`. Hoisting the counter out of the `!isErrorPart` branch — which otherwise ships green across the whole suite — would credit a denied read as a diff read. * test(review): pin the failed-read gate on the evidence arg lists (#9539) |
||
qqqys
|
2870eeedf9
|
feat(web-shell): adopt canonical Goal v3 controls (#9393)
* feat(web-shell): adopt canonical Goal v3 controls
Route WebShell Goal lifecycle through the canonical Goal v3 control
plane instead of model-bound chat commands. Goals can be created before
the first message, then inspected, edited, paused, resumed, replaced and
cleared directly.
- core: extend the Goal reducer/runtime and protocol with the v3 control
actions and a revisioned snapshot.
- serve/acp: expose typed Goal state and controls over the daemon HTTP
routes and the ACP bridge, with a shared error taxonomy.
- sdk/webui: surface the same revisioned snapshot and control actions so
every consumer reads one source of truth.
- web-shell: render the active Goal as a compact composer row sized to
match the queued-message row, drop the clear confirmation, and hold
ordinary messages in a local FIFO queue while a Goal runs — only the
explicit Insert action enters the active turn.
The TUI keeps its existing presentation and command flow. Token-budget
UI and desktop-shell adoption are intentionally out of scope.
* test(serve): count the two Goal routes in the telemetry guards
The PR registers `POST /session/:id/goal` and `GET /session/:id/goal`,
both `handler_resolved`, taking the legacy session telemetry catalog from
59 routes to 61 and the attribution split from 57/2 to 59/2. Three
hardcoded totals in telemetry.test.ts and one in the drift guard still
asserted the old counts, so the Test job failed even though the guard's
real check — registered Express routes equal the catalog — passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(webui): keep the Goal snapshot authoritative across replace, stale reads, and load
Three ordering defects let a stale Goal frame win over the daemon's actual
state, each of which then flips `holdQueuedPromptsLocally`, the Goal strip,
and the manual-run gate off a goal that is not really there.
- `selectGoalState` recorded an ordering identity only for a cleared goal, and
`goal-runtime` attaches `clearedGoal` to a clear but not to a replace — so a
pre-replace frame of the replaced goal passed every guard and reinstalled it
over its replacement. Carry a bounded ledger of superseded goal identities
(cleared and replaced alike) forward onto each accepted snapshot and reject
frames at or behind one.
- `getGoal()` reconciled a bare-null READ snapshot against whatever state
existed at resolution time. A read the daemon answered while goal-less can
land after a concurrent create, and with no `clearedGoal` tombstone it read
as "clear whatever is current", wiping the new goal. Stamp the read with the
goal observed at issue time: a bare-null response may only clear that goal.
- The session-load path installed its snapshot behind a reference-equality
guard instead of `selectGoalState`, so a frame arriving inside the load
window discarded the authoritative response — and when none arrived, the raw
install registered no tombstone and a later stale frame resurrected a
cleared goal. Reconcile instead, and keep the synthesized empty snapshot for
a failed fetch only while no state is known.
Each fix is pinned by a test that fails when the fix is reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(webui): pin the known-Goal-state branch of a failed session-load fetch
Only the fresh-connection branch (synthesize an idle snapshot) was covered, so
a simplification that always synthesizes on rejection would ship green and
replace a live goal with idle on a transient `goal()` failure.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(web-shell): keep Goal controls and drafts owned by the surface that started them
Four Goal-surface defects, all of the same shape — state applied to whatever
form, latch, or composer exists when an operation settles rather than to the
one that started it:
- `GoalEditDialog` live-synced its textarea to the `objective` prop, so a
concurrent edit from another client (or the refresh a failed save triggers)
silently overwrote the user's typed draft — the only copy. Adopt prop
refreshes only while the field is still pristine.
- `GoalsDialog` left its form dismissible while a submit was in flight, so
closing it mid-request handed that request's `resetForm()`/`setFormError` to
the next goal's form. Pass `dismissible={!submitting}`, matching
`GoalEditDialog`.
- ChatPane's `/goal` branch ran before the broken-connection guard applied
further down the same `handleSubmit`, so a control typed while the pane was
disconnected was consumed, written to the transcript, and then failed with
only a toast. Apply the guard inside the branch and keep the text.
- ChatPane's goal-control busy latch was session-keyed, so a server-side goal
replacement released it mid-operation and a second control could dispatch
against the same expected revision (one loses with a 409). Key the latch to
the operation and release it only from its owner.
Each fix is pinned by a test that fails when the fix is reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(web-shell): fail Goal gates closed while hydrating, and stop losing held prompts
- The `goalSnapshotRef` gates (`promptBlocked`, retry, language change,
fast-model select) failed OPEN while `connection.goalState` was still
hydrating: the session load clears `loadingTranscript` before its `goal()`
fetch resolves, so a prompt typed in that window was submitted straight into
a Goal the client had not learned about — while the queue-hold gate,
`enqueueManualRun` and `tryFireBoundRun` all failed closed on the identical
state. They now share one `isGoalGateBlocked()` predicate that treats an
unknown Goal state on a real session as blocked.
- Creating a Goal in an allocated session went through the workspace-scoped
control, which — unlike `sessionActions.controlGoal` — never wrote
`connection.goalState`; the only compensation was a conditional re-sync one
round trip later. Until it landed the hold gate read false and no Goal strip
rendered. A new `applyGoalSnapshot` session action installs the create
response into the connection state, reconciled like any other snapshot.
- Locally held Goal prompts were stashed under `(workspaceCwd, sessionId)` and
could only be relocated when the session just left was the same one, so a
workspace resolving while the user was on another session orphaned the stash
under a key nothing looks up again — silently losing typed text. Any stash
whose session half matches is now relocated and restored in queue order.
Each fix is pinned by a test that fails when the fix is reverted. The App test
harness now defaults to a hydrated (goal-less) snapshot, matching a loaded
session; the tests that exercise the hydration window set it back to undefined.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(acp): publish turn_complete when a Goal turn ends
Goal turns are driven inside the ACP child via its own prompt() call, so
the daemon bridge never sees a session/prompt RPC boundary for them and
published no turn terminal on SSE. Web Shell (and any SSE client) only
settles its streaming state on turn_complete/turn_error/prompt_cancelled,
so after a Goal turn the spinner spun forever and locally queued messages
never drained — even though the daemon itself was already idle.
The child now sends the existing `_qwencode/end_turn` notification with
`source: 'goal'` and a turn-scoped promptId when a Goal turn settles, and
the bridge translates that into a real `turn_complete` frame — the same
contract the Web Shell goal e2e mock already assumed.
* fix(web-shell): serialize held-prompt release, drop the dead insert abort registry
- Releasing several locally held Goal prompts fired every submission at once,
so a prompt awaiting media uploads could be overtaken by a later plain one
and reach the daemon's queue out of order. `submitPendingPrompt` now returns
its admission promise and the release chains them; the first release stays
synchronous.
- `explicitInsertAbortControllersRef` was populated and cleaned up but never
read: nothing aborted its controllers, so the signal plumbing and the
`abort.signal.aborted` recovery branches were unreachable. An explicit insert
is meant to outlive an owner rotation and settle into the queue of the
session it was started from (two tests pin exactly that), so the registry,
the signal and the dead branches are gone rather than given a consumer.
- Both mount sites rendered the Insert button enabled while streaming was idle
and a Goal was active — precisely the state `insertQueuedPrompt` no-ops in.
`canInsertMidTurn` now tracks the hook: the affordance appears only while a
turn is running.
Test coverage the review probes found missing, each verified by reverting the
line it gates: the images and slash-command insert guards, the `isInserting`
reset on both settle-unaccepted paths, the release order, the Insert
affordance, and the held-prompt stash handoff (a prompt already dealt with must
not come back from a stale owner key).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(web-shell): localize Goal command errors and stop Goal surfaces swallowing input
- `parseWebShellGoalCommand` returned an English sentence that `formatError`
prefers over any localized fallback. It now returns the offending keyword and
both composers render `goals.error.requiresObjective` from the dictionaries;
the manual-run rejection gets `scheduledTasks.error.goalActive` for the same
reason.
- `handleGoalSlashCommand` returned true — wiping the composer — before the
preconditions it checks asynchronously held, so a `/goal clear` typed without
a session lost its text to a toast. The cheap preconditions are checked first
and refuse the submit instead.
- `enqueueManualRun` treated an unknown Goal state as "Goal is active", which
also fires when no session is attached and no Goal can exist, so Run now on a
fresh workspace always failed. It shares the hydration-aware gate now.
- ChatPane consulted the host slash handler after its `/goal` intercept, so a
host override applied in the main composer but not in a pane; and a bare
`/goal` in a pane without a Goals view consumed the text silently. The pane
now matches the composer's ordering and refuses what it cannot open.
- GoalsDialog offered Edit on a completed Goal, which the reducer rejects, and
fell back to the stale snapshot when the edited session left the list —
turning the friendly "no longer available" path into a raw conflict error.
- The versioned control request is built by one shared helper instead of two
copies that had already drifted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* refactor(web-shell): retire the dead Goal footer paths and restore the stranded-session rationale
- `GOAL_STATUS_ACTIVE_EVENT` lost its only production listener in this PR while
`GoalStatusMessage` kept dispatching it and two test files kept asserting the
dispatch — a contract that looks live but is dead. The dispatch, the
`activateFooter` prop, the `isLatest` plumbing that fed it, and the
assertions are gone.
- The built-in StatusBar now received `activeGoal={null}` permanently, so its
goal pill and `onOpenGoals` wiring could never fire while StatusBar.test.tsx
kept them green. The pill, its props, its elapsed-time ticker and that
pill-only test file are removed; the composer status stack is the goal
surface. Custom footers keep their `activeGoal` prop, which is still fed.
- `/language ui` skipped its daemon sync when `promptBlocked`, silently
switching the chrome while the agent kept answering in the old language for
the rest of a Goal run. It now refuses with the same feedback the language
picker gives for the identical condition.
- The `@container` block in GoalStatusStrip styled `.root`, which cannot match
its own container query, so half the responsive rule never applied. Trimmed
to the descendant rules that do, with the reason recorded.
- Restored the rationale comment above `strandedGoalSessionRef` — the mechanism
it documents is unchanged and still live.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(sdk): expose limitKind, drop the unused expected-version type, cover the Goal wire seams
- `GoalRecord` in the SDK omitted `limitKind`, the field that decides whether a
stopped Goal can be resumed. The daemon sends it, the webui mapper now parses
it (rejecting unknown values), and both Resume controls hide for an
evidence-limited Goal instead of offering a click that always 409s.
- `GoalExpectedVersion` was added to the published SDK with zero read sites —
the request type inlines the two fields — so it is removed before release
turns it into a shape we must support forever.
- Tests for the wire seams the review found uncovered, each verified by
reverting the line it gates: the goal error-kind → HTTP mapping and its
`current` forwarding, `GET /goals` listing paused/blocked/usage_limited and
filtering complete, the untrusted-workspace gate for every work-expanding
action (not just create), and `bridge.controlSessionGoal`'s `{ request }`
envelope — the only producer of the shape the agent's handler reads.
- acpAgent's core mock now carries the real `GoalConflictError` /
`GoalInvalidTransitionError`, without which every `instanceof` branch in
`mapGoalControlError` throws before it can be asserted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(web-shell): close the Goal coverage gaps the review probes found
Each test below fails when the line it gates is reverted (probe-verified):
- App: the direct-submission `onSubmitBefore` rejection keeps the draft; a
session-less stranded session is forgotten when the user leaves the Goals
page; a create after a successful one starts a fresh session; the allocated
create is dispatched only after the allocation completes; the Goal edit
dialog closes when the same session's goal is replaced; a stale edit
resolution does not install its snapshot over the session now displayed; the
control busy latch stays owned by the session that started it; and the
allocated-session create is observed through the strip the App renders rather
than through the value the test's own mock wrote.
- ChatPane: the control request is built from the freshly fetched Goal (both
the pause payload and the `/goal set` → replace mapping); the edit dialog
closes when the pane's goal changes; the "no longer available" message is
pinned instead of `expect.any(Error)`; and a control dropped because the pane
moved to another session no longer reports a failure toast — matching how the
edit-save and main-composer paths already treat that race.
- e2e/mock: the mock's `GET /session/:id/status` returns the flat
`DaemonSessionSummary` the daemon really sends (the envelope left every field
the client reads undefined), its clear response carries the `clearedGoal`
tombstone so the anti-resurrection path is reachable, and the first Goal
scenario re-checks that no prompt was sent after the create resolves.
- Restored the one-shot run-now rationale comments in ScheduledTasksDialog and
its tests; the behavior they explain is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(web-shell): drop the imports the retired Goal paths stranded
`ed422f9a6c` removed the dead `GOAL_STATUS_ACTIVE_EVENT` contract and the
StatusBar goal pill together with the tests that covered them, but left four
imports behind with no remaining reference. `lint:ci` runs eslint with
`--max-warnings 0`, so `@typescript-eslint/no-unused-vars` failed the required
`Test (ubuntu-latest, Node 22.x)` gate on this head.
- `StatusBar.tsx`: `useEffect`/`useState` fed only the goal pill's elapsed-time
ticker, which went with the pill.
- `SystemMessage.test.tsx`: `TranscriptRenderModeProvider` and
`serializeGoalStatusMessage` were used only by the removed
"goal status activation" describe block.
No behavior change and no test weakened — the covered production paths were
deleted in the same commit that stranded these imports.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(acp): surface Goal-turn activity in hasActivePrompt summaries
Goal turns never flip the bridge's promptActive flag because they run
inside the ACP child without a session/prompt RPC, so live-state reported
hasActivePrompt: false while a Goal was executing and the Web Shell
sidebar showed no activity indicator for that session.
The child now sends `_qwencode/start_turn` (source `goal`) when a Goal
turn begins; the bridge tracks it as a separate goalTurnActive flag and
ORs it into every hasActivePrompt summary. The existing goal end_turn
signal clears the flag, the start/end pair now lives in the goal queue
drain so a prompt that rejects early cannot leak the flag, and an RPC
prompt start self-heals a stale flag since the child serializes the two.
* fix(acp): let a mid-turn insert reach a running Goal turn
A Goal turn runs inside the ACP child via `prompt()` directly, so the
bridge never sees a `session/prompt` RPC for it and `pendingPromptCount`
stays 0 for its whole duration. `POST /session/:id/mid-turn-message`
passes `rejectIfIdle: true` and the admission gate reads only that
counter, so every explicit Insert during a Goal turn was answered
`accepted: false` — while the Web Shell enables the affordance precisely
because `c9597f6e` made a Goal turn non-idle in `hasActivePrompt`. The
client returned the row to its hold and reported `insertFailed`; the
media variant deleted the uploaded attachments with it. The e2e mock
answers `accepted: true` unconditionally, so nothing caught it.
The session is genuinely busy during a Goal turn — the child drains this
same queue between tool batches from inside it — so admission now treats
`goalTurnActive` as busy and the message is queued for that drain.
A Goal turn owns no prompt slot, so nothing settled what its last drain
missed: the goal `end_turn` signal now closes that window the way the
prompt terminal already does (`queueOnly` callers get
`onSettledWithoutDrain`, everything else is promoted). Promotion is the
supported path while a Goal is still active — the child's `claimGoalTurn`
makes the promoted prompt wait for the permit and run as the next Goal
turn.
Both tests fail when their line is reverted (probe-verified).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(webui): stamp the session-load Goal read the way `getGoal` does
The R1-10 fix landed only in `getGoal()`: the session-load path still
routed a fulfilled bare-null `goal()` read straight through
`selectGoalState`. The load issues that read while the session is
goal-less, so the answer carries no `clearedGoal` tombstone — and
`selectGoalState` then derives the clear target from whatever the store
holds when it is applied. A goal created inside the load window (the Web
Shell allocates a session, then creates the goal on it, and the load's
`Promise.allSettled` is gated on its slowest sibling request) was
therefore accepted as the thing being cleared, and its identity was
written into `clearedGoalOrder`. From there `isSupersededGoalFrame`'s
`<=` tie rejected every later frame of that goal at the same revision —
including the `refreshGoal()` that follows the create. The daemon held a
live goal while the store reported goal-less, `holdQueuedPromptsLocally`
read false so ordinary prompts bypassed the Goal queue, and the tombstone
survived reloads.
`getGoal`'s guard moves into a shared `selectGoalStateFromRead`, and the
load path stamps its read with `goalStateAtLoadStart` — the goal it
observed when the read was issued, which it already captures.
The provider test mirrors actions.test.ts's stale-read case and fails
when the stamp is replaced by an apply-time read (probe-verified).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web-shell): close two Goal-queue leaks in the composer and queue
Two Goal-queue leaks the review found on this head.
`ChatPane`'s composer submit read `goalState?.goal?.status !== 'active'`,
which is TRUE while the snapshot is still hydrating — the session load
clears `loadingTranscript` before its `goal()` resolves, and the daemon
has no server-side prompt gate for an active Goal, so a prompt typed in
that window went straight to the daemon and bypassed the queue. Every
other gate in the client already fails closed on the same window. The
predicate now lives in `utils/goalGate`, shared by App's
`isGoalGateBlocked` and both of ChatPane's gates, so they cannot drift
apart again. The pane fixture gains the Goal snapshot a loaded session
always carries — without it the fixture models a hydrating session, not a
Goal-less one.
`useQueuedPrompts` captured the stash owner key once when an explicit
insert started, but the workspace half of that key can resolve mid-flight
and the owner-change effect relocates the whole stash onto the new key,
deleting the old one. The accepted `midTurnState: 'queued'` write then
hit a key nobody holds and was silently dropped: the row came back from
the stash still `isInserting`, and release/edit/delete/clear all skip
such a row — bricked until a reload. The key is now resolved at use time
by the session half (the same uniqueness invariant the relocation itself
relies on), and the four inline owner-match copies collapse into one
helper that compares that half rather than the whole key.
Both new tests fail when the line they gate is reverted (probe-verified).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web-shell): re-check the Goal hold on every release-chain link
The chain that drains locally-held prompts is built synchronously when the
hold lifts, but each link runs only after the previous admission settles, and
`submitPendingPrompt` never consults the hold. Pausing a Goal (which starts
the drain) and resuming it inside the chain-length x admission-latency window
left the remaining links POSTing against an active Goal — the queue emptied
against the user's change of mind, which is the contract this PR exists to
hold.
Each link now re-checks the hold (and the write block) and returns its row to
held instead of sending it; the next inactive transition re-drains in order.
The revert is inline rather than through `setQueuedPromptFlags` because that
callback is declared below this effect, so naming it in the dep array would
read it before its initializer.
Also pins `mapGoalControlError` at the ext-method layer, which had no
coverage: `sessionGoalControl`'s only tests were the success path and the
untrusted gate, and the gate throws before the mapping is reachable. The new
case drives real `GoalConflictError` / `GoalInvalidTransitionError` /
persistence rejections through `extMethod` and asserts the code plus intact
`data.errorKind` and `data.current` — the payload the client's 409 resync
depends on.
Both fail when the line they gate is reverted (probe-verified).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor(web-shell): stamp GoalsDialog's CAS fields through the shared builder
`versionedRequest` was a private second writer of `expectedGoalId` /
`expectedRevision` — the exact drift `buildGoalControlRequest` was introduced
to prevent. The dialog's two call sites now go through the utility; behaviour
is unchanged for them (both always hold a goal, so the builder's
`goalUnavailable` guard is unreachable there, and `edit` falls back to the
goal's own objective as before).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web-shell): stop offering Goal controls the reducer will reject
Clears the three Critical findings from round 2 of the review on #9393.
R2-1 / R2-2 — both resume gates (`GoalStatusStrip.tsx`, `GoalsDialog.tsx`)
keyed off `goal.limitKind === undefined` alone, while core's
`isEvidenceLimited` (`goal-reducer.ts`) also treats a Goal as evidence-limited
when `lastReason` is one of the two sentinel strings. Those sentinels shipped
before the `limitKind` field did, so a Goal persisted in that window restores
as `usage_limited` with no `limitKind` at all: the UI offered a Resume button
that `reduceGoalControl` is guaranteed to answer with
`GoalInvalidTransitionError`.
Both gates now go through one `canResumeGoal` in `utils/goalGate.ts` — the
module whose documented invariant is that every client Goal gate runs through
a shared predicate so none can drift. It states the reducer's rule directly
(complete/active refuse; `usage_limited` refuses when evidence-limited) rather
than approximating it, and it keeps the evidence check scoped to
`usage_limited` exactly as the reducer does, so a `paused` Goal carrying stale
sentinel prose is not stranded without a Resume control.
The Web Shell client bundles for the browser and cannot import
`@qwen-code/qwen-code-core`, so the two sentinel strings are duplicated. A
comment asking the next person to keep them in sync is not a mechanism:
`goalGate.test.ts` reads `packages/core/src/goals/goal-protocol.ts` and fails
if either literal moves, or if core grows a third sentinel branch.
R2-3 — a session switch mid-drain silently destroyed queued prompts. The
serial release chain stamps the whole batch `serverState: 'submitting'` up
front and then releases it one link at a time (a prompt carrying media awaits
its uploads, so that window is seconds, not microseconds). Two things went
wrong inside it: the chain carried no owner token, so remaining links called
`submitPrompt` against the old session, which throws before POSTing and was
swallowed by the chain's own `.catch`; and the owner-change stash only saves
rows matching `isLocallyHeldPrompt` (`serverState === undefined`), so every
chain-marked row was excluded from it. Both typed prompts were gone with
nothing restored to the editor.
Each link now bails when the owner token it was built for is no longer
current, and a new `unreleasedPromptIdsRef` tracks exactly the rows the chain
stamped but never handed to `submitPendingPrompt`. The owner-change effect
stashes those alongside the genuinely held rows, dropping the optimistic stamp
on the way in so the next drain re-releases them in order.
Deliberately narrower than the finding's suggested fix on one point: the row
whose admission is actually in flight is NOT stashed. Restoring it would flip
the behaviour pinned by 'ignores an old submit response after an S1 to S2 to
S1 owner change' and 'fences an old submit before the replacement owner
rerenders', which deliberately fence and drop an in-flight submission across
an owner change — that POST may well have landed, so resurrecting the row
risks a duplicate. The rows the chain never POSTed have no such ambiguity:
nothing exists for them on the daemon, so stashing them cannot duplicate
anything. Reversing the in-flight contract is a design call for the reviewer,
not a drive-by.
Also fixes R2-12 in passing, since the new tests need it: `MockGoal`'s
snapshot `status` union omitted `'complete'` (a TS2322 against this file's own
later usage) and had no `limitKind`.
Verification: `npx vitest run client/hooks/ client/utils/
client/components/GoalStatusStrip.test.tsx
client/components/dialogs/GoalsDialog.test.tsx
client/components/ChatPane.test.tsx` in `packages/web-shell` — 1011 passed
(994 on the stashed tree, so all 17 new tests pass), with an identical 17
pre-existing failures in the untouched `hooks/useMessages.test.ts` on both
trees. eslint clean on all eight touched files.
Every fix is mutation-verified. Dropping the sentinel fallback, widening it to
any `lastReason`, or unscoping it from `usage_limited` each turns a distinct
test red at both the unit and the component level; removing the chain owner
guard, the unreleased-row half of the stash filter, or the stamp reset each
turns the new mid-drain test red.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(web-shell): stop Goal controls and the release chain racing each other
Four round-3 Criticals, each reproduced first and then mutation-verified.
R3-4 / R3-19 — `goalControlBusy` was a shared latch with no owner. A create
in an allocated session released it unconditionally in `finally`, and the
composer `/goal` path never consulted it at all, so two controls could read
the same snapshot, stamp the same expectedGoalId/expectedRevision, and let
the daemon reject the loser with a 409. Ported ChatPane's twin pattern —
`goalControlOpSeqRef`/`goalControlOwnerRef`, released only by the operation
that still owns it — to both `controlCurrentGoal` and
`createGoalForAllocatedSession`, and made `handleGoalSlashCommand` refuse
(keeping the composer text) while a control is in flight.
R3-24 — a chain link deleted its id from `unreleasedPromptIdsRef` before the
owner check. The owner token is replaced in the render body while the stash
is a passive effect flushed after commit, so a link firing in that window
left a row that was neither locally held nor unreleased, and the stash
dropped it: the typed prompt vanished with no POST, no abort, no toast.
Delete moved below the owner check, as the review suggested.
R3-20 — a queue clear mid-drain aborts only the in-flight link's controller;
the chain's pending links have no controller yet, so they went on to POST
prompts the user had explicitly cleared. The link now bails when its row has
left the queue.
Verification: 775 passing across App / useQueuedPrompts.dom /
useQueuedPrompts.midTurnReconcile / ChatPane / GoalStatusStrip / GoalsDialog;
full web-shell 3906 passing with the same 18 failures the branch has without
this change (useMessages background-agent reconciliation, build-artifact —
confirmed by stash-and-rerun). tsc unchanged at 66 pre-existing errors from
unbuilt workspace deps. ESLint and Prettier clean on the touched files.
Mutation-verified: each of the four fixes reverted in isolation turns exactly
its own new test red.
* test(cli): align merged replay expectations
* fix(web-shell): keep a prompt typed mid-drain behind the release chain
R3-2: the serial release chain preserved order only inside the batch it
drained. It exists because the prompt at its head may await media uploads
for seconds; a prompt typed inside that window went straight through
`submitPendingPrompt`, so the daemon admitted it ahead of the older held
rows it was typed after -- and while link 1's upload was still running it
could overtake link 1 itself and start the turn.
The chain is now published on `releaseChainRef` (owner-pinned, retired
once its newest tail settles), and `enqueuePrompt`'s ordinary path appends
to that tail instead of POSTing past it. The waiting row is registered in
`unreleasedPromptIdsRef`, so it is stamped-but-not-POSTed exactly like the
chain's own undrained rows and the owner-change stash saves its text
rather than losing it.
The per-link guards (owner pinned at build time, row-still-present,
hold/write-block revert) move into a shared `releaseChainedPrompt` so both
the drain and the appended send carry identical semantics. A re-drain for
a live owner now extends the existing chain instead of racing it.
Test: `holds a prompt typed mid-drain behind the release chain` -- two held
prompts with link 1's admission hung, a third typed mid-drain; asserts it
does not POST while the chain is in flight and that final order is
['first with media', 'second plain', 'typed during drain'].
Mutation-verified: disabling the append arm turns the test red with the
reported symptom (the mid-drain prompt POSTs immediately, order inverted).
Verification: `npx tsc --noEmit` clean; `npx vitest run` in packages/web-shell
-> 190 files / 3986 tests passed.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>
|
||
callmeYe
|
4839935e55
|
feat: register toggle-only Qwen reasoning (#9574) | ||
|
ytahdn
|
bf0dbb1ae1
|
feat(web-shell): support mid-turn file attachments (#9570)
Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com> |
||
|
Shaojin Wen
|
5832c49035
|
feat(review): rule on contract documentation, and matrix layered guards (#9448)
* feat(review): rule on contract documentation, and matrix layered guards Add a path-scoped checklist for consumer-facing contract documentation — wire protocols, API references, SDK guides — that reads the prose a diff adds and asks whether the code makes it true. Every existing dimension reads the code and asks whether the code is right; the documentation item that already exists asks whether a doc is present. Nothing checked the document a change ships against the behaviour it ships, and for a wire contract that document is what an integrator builds against without ever seeing the implementation. The rule is deliberately narrow. Proposals, plans and RFCs are excluded because describing behaviour the tree does not have yet is the point of the genre; user guides and changelogs are excluded because a rule that fires on every documentation change is a rule that gets skimmed. Only reference material an outside consumer builds against is governed, only prose the diff adds or falsifies is in scope, and wording, silence and roadmap text are never findings. Also correct the deep-verification mutation matrix for defence in depth. A one-row-per-guard matrix cannot see two hunks that close one hazard from different directions: reverting either alone leaves the other holding the line, so both rows read as survivors and the report claims two coverage gaps that do not exist. The matrix now carries a combination row for such a set and a third survivor class for the guards it explains. The positive control a survivor already owed is tightened to the file it was measured in — a control that turns a test red somewhere else proves the runner runs, not that the chosen command collects anything exercising the mutated file. * fix(review): take contract-doc governance from location, not from a name Review found the filename branch both unbounded and unbounding. It backtracked quadratically — a failing extension scan once per keyword occurrence, 403 ms on a 96 kB path git accepts, synchronously inside every agent-brief build, which is the shape the Java checklist in the same file grades a denial-of-service hole. And because a keyword in a name fires anywhere in the tree, it also governed a user guide, a changelog entry and a flat-layout RFC, each of which would then have been read under a checklist whose blockers are Critical. Adding exclusions did not converge; every audit round found another form. Both problems have one cause, so both get one fix: governance now comes only from where a document lives — a reference section or an SDK package — and the keyword branch is gone. The genre exclusion is built once from a shared list and applied in both its directory and its filename form, so a changelog directory and an `rfc-0001.md` are excluded by the same rule that already excluded `rfcs/` and `CHANGELOG.md`. The recall this costs is deliberate and now stated as a test: a wire reference kept outside a documentation or SDK tree is not governed, and a project that wants the checklist there has its own review rules. Every alternative of both the location branches and the genre list is pinned in the matcher table in both spellings, the changelog exclusion is pinned by paths where it is the deciding clause rather than by ones no branch reaches anyway, and the timing block gains four arms that end in a documentation extension — every existing arm short-circuits at the extension gate, which is how an unbounded matcher shipped past it. * fix(review): make the contract-doc pins and timing arms measure what they name Round two found the arms and the completeness claims weaker than they read. The two timing arms matched their target expression at the first anchor, so neither ever reached the failing scan it was named for: two quadratic mutants — one in the genre exclusion, one in the location branch, both behaviour-identical on newline-free paths, so no behavioural test could see them — shipped with the whole suite green. Every arm is now a near miss that matches the prefix at each anchor and fails on the last character, and both mutants die on them. The genre-silence test had the same shape one level up: none of its fixtures sat inside a governed location, so location alone silenced them and the test stayed green with the entire genre exclusion deleted. It now carries fixtures where the exclusion is the deciding clause. The matcher itself was asymmetric or under-pinned on eight axes that each shipped green as a one-edit narrowing — section-name anchoring, the SDK suffix quantifier and its character class, two separator members, the shared genre list, and the case-insensitivity of three separate expressions. Each now has a row that flips. Four behaviour changes came with them. Section names are plural-tolerant on all four words rather than two. A genre is a whole token wherever it sits, so a numbered or dated genre filename is the genre its name says it is, while a word that merely begins with those letters is not. Per-version release notes join changelogs as history rather than contract — this repository ships one inside a governed location. The markdown extension family is admitted in full, since governance is by location and not by which spelling a file uses. Two entrances that contradicted the location rationale are closed: the SDK branch is anchored to a package root, because an `sdk` segment at any depth also governed user guides and fixture trees, and a closed set of repository-meta and agent-context filenames is excluded whole. What remains — non-contract prose inside a reference tree that no closed set describes — is accepted and pinned rather than enumerated: measured here, 44 of the 55 tracked documents under the developer tree are integrator references, and deciding document kind inside a reference tree is the shape that took the keyword branch out last round. * fix(review): pin the contract-doc members and boundaries that flip nothing Round three found the completeness claim still short in three directions, each a one-edit change that moved real paths with the whole suite green. Members: the genre list's newest entry was pinned only in the singular, and seven of the eleven repository-meta filenames plus the SDK branch's `libs` prefix had no row at all. Every alternative now has one, in both numbers where the expression allows both. Boundaries: the trailing separator that makes each location branch match a section directory was pinned by nothing, so dropping it revived governance by filename — the entrance the keyword branch was withdrawn for two rounds ago, returning through the terminator rather than the alternation. Rows now pin both sides. The same was true of the SDK branch's case-insensitivity and of the space in the genre separator class. Ownership: the section branches matched at any depth, which is right for a package-local reference section and wrong for a vendored library or a doc-shaped fixture that carries one. Anchoring the branch would have dropped both, so the distinction is drawn where it belongs — a closed set of trees whose prose the diff's author does not own, alongside the meta filenames it sits next to. Findings against third-party documentation are noise the author cannot act on. Also taken, though the review deferred them as non-blocking: the assertion on the diff-scoped blocker stopped one word short of the negation it exists to pin, and two of the five never-a-finding bullets had no assertion at all. * fix(review): drop the contract-doc table row the ownership block duplicated * fix(review): exclude separator-less thirdparty trees, and pin the NOT_OURS set --------- Co-authored-by: wenshao <nigolaschao777@gmail.com> |
||
|
ytahdn
|
5f3165f17e
|
fix(daemon): avoid custom attachment upload header (#9567)
Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com> |
||
jinye
|
50d2700797
|
fix(web-shell): Stop repeated session title catalog refreshes (#9563)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
313f191150
|
fix(autofix): make the brake's BLOCKED handoff a first-class round outcome (#9297)
* fix(autofix): make the brake's BLOCKED handoff a first-class round outcome When the growth brake fires, feedback.md tells the address agent to stop BLOCKED with a handoff — but the output contract only accepted address-summary.md or no-action.md, so a round that followed the instruction died as 'finished without required output file(s)', the brake's decision text was buried under a generic failure.md, the report said 'could not produce a passing fix', and the job left a red review-address check that the next scan counts as new feedback. Observed on #9222 rounds 6/7. The handoff becomes a first-class verdict end to end: run-agent.mjs honors an agent-written handoff.md (with no fix verdict) as a graceful exit the way it already honors failure.md, and shields it from the API-error retry reclassification; the verification gate reports outcome=handoff for a no-commit round with a handoff and no failure.md; finalize lets handoff pass without failing the job; the report step runs for this outcome, posts the handoff note with the eval marker (watermark advances — the feedback is consumed as evaluated), and names the stop honestly instead of reporting it as a failed fix. The skill now tells the agent exactly which file to write when the brake fires. A coexisting spec output still outranks the handoff, and failure.md coexistence keeps the failed classification, so crash paths are unchanged. * fix(autofix): align the handoff outcome's consumers and pins with its contract (#9297) Review found the new handoff outcome breaking two pinned helper tests (stale breaker-headline wording, unclassified headline in the fleet-shepherd contract test), misreporting handoff rounds in the status-comment finalize step, and leaving the whole handoff chain unpinned against mutation. - Update the breaker headline pin to the PR's reworded headline. - Classify the handoff headline as transient in the shepherd contract test and drop its "AutoFix stopped" prefix so the shepherd's terminal-only REASON regex cannot capture a transient stop (the shepherd workflow itself stays outside this round's footprint). - Include handoff in the Finalize-status published-report branch. - Give deliberate stops their own takeover-digest census bucket instead of the residual crash/infra bucket (EN + ZH). - Neutralize :: workflow commands at the two new handoff echo sites. - Use the runner's non-empty missing() convention for handoff.md so an empty file cannot read as a verdict in one layer and not the other. - Correct the run-agent.mjs precedence comment: when a handoff coexists with a spec output, the gate (handoff branch first) decides the round, matching the documented "handoff + no-action -> handoff" contract. - Pin the handoff chain where its siblings are pinned: finalize replay, POST_HANDOFF replay, mark/headline replays, the gate's no-commit decision table, the stub-runner handoff/empty/API-error cases, the report-step if-clause, and the census needle-to-emit cross-pins. * fix(autofix): classify a no-commit handoff before the gate's structural checks (#9297) Review proved the new handoff classification unreachable exactly where the brake fires: the structural pre-checks (core rebuild, settings schema, contracts) judge the PR's own diff and reject before the no-commit fork, and the growth brake fires on precisely the red PRs whose diff trips them. A compliant handoff (no commit, only handoff.md) then classified as a retryable failure, so the repair pass deleted handoff.md and could commit against the brake's explicit stop — the self-feeding loop the handoff exists to prevent. Reproduced with the real gate script: schema-check-fail + no-commit handoff exited 1 with no outcome=handoff. Move the no-commit handoff classification above the structural checks (right after the failure.md exits, which keep their precedence). A handoff claims nothing — acted=false, deferred to a human — so the checks' false-no-action rationale does not apply, and the retryable/ repair machinery must never engage on a round the brake told to stop. The no-op fork reverts to no-action-only classification. - Add a gate test: stale schema + no commit + handoff.md classifies outcome=handoff, exit 0, no retryable (fails on the pre-fix gate). - Pin the handoff-note :: workflow-command neutralization in both layers (the gate's sed and the runner's replaceAll), which review showed were surviving mutations. * fix(autofix): reject a no-commit handoff written over a dirty workspace (#9297) * fix(autofix): report a dirty-handoff rejection honestly, not as a failed fix (#9297) * fix(autofix): reject a handoff written beside a round commit, non-retryably (#9297) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): classify the committed handoff shape as its own non-retryable outcome R7-1 on this PR: a round that HAS a commit beside handoff.md skipped both brake-violation guards (clean tree misses the dirty guard; committed ref misses the no-commit branch) and fell through to the structural checks, where reject_fix defaults to retryable and the repair pass deletes handoff.md and may commit again against the brake's stop. Classify it before the structural checks under its own outcome committed_handoff, sibling of dirty_handoff: non-retryable, its own honest report headline (reusing dirty_handoff's wording would claim nothing was committed when a commit exists), listed among the report-publishing outcomes in the status classifier, and never routed through finalize's pass list. Pins updated in the same pass: the committedWithHandoff gate case now expects committed_handoff with no retryable, the shepherd contract test classifies the new headline as transient (loop stays engaged), the status-classifier pin names all five outcomes, and the handoff-contract gate test gets an explicit subprocess budget (eight fixture arms outgrew the 5s default). * fix(ci): count committed-handoff rounds in the milestone census rejected bucket (#9297) * fix(autofix): publish brake violations green and preserve handoffs across crashes (#9297) Two Critical review findings on the handoff output contract. Brake-violation rounds (dirty_handoff / committed_handoff) ended with a red review-address check: the eval marker stamps ts=NEWEST, strictly before the check completes, and the scan counts failed checks completed after the watermark — including this workflow's own review-address checks — as new feedback. The next scan re-selected the PR and burned a full agent round on the item the posted headline promised not to retry, once per violation. Admit both outcomes to the green finalize arm the way the clean handoff already is (the diff's own comment names this self-feeding loop as the reason handoff went green), and key the report step's routing and POST_HANDOFF trigger on the outcomes themselves so the green rounds still publish their honest headline, handoff note, and eval marker instead of going silent. A crash, budget kill, or loop guard after the agent wrote handoff.md synthesized a failure.md that shadowed the note: the gate reads failure.md first (outcome=failed), the report preferred it, and the timeout sentinel re-handed the item the brake stopped. Preserve the agent-written handoff in the crash branch (exit 0, mirroring the agent-written-failure.md arm), and never let writeHandoff overwrite a non-empty agent verdict. Both findings reproduced against this commit's verbatim code before fixing: the case/jq replay showed the violation check red and counted as new feedback, and a stub run showed the synthesized failure.md shadowing the handoff. New behavioral tests fail pre-fix and pass post-fix. * test(ci): give four subprocess-heavy replays explicit budgets The milestone digest, stale-duplicate revalidation, deny-by-default footprint, and recoverable-API-render tests spawn multiple bash replays of the real workflow/gate scripts each; the files those replays parse grew with this PR's handoff chain, and all four outgrew the 5s default (each verified to pass with an explicit 30s budget, matching the suite's convention for subprocess-heavy tests). * fix(ci): mirror the handoff outcome consumers into the recovery clone (#9297) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
易良
|
0baaec2b32
|
chore(ci): Drop NPM_TOKEN in favor of npm Trusted Publishing (#9552)
* chore(ci): Drop NPM_TOKEN in favor of npm Trusted Publishing * chore(ci): Pin npm 11 for Trusted Publishing in release jobs * test(ci): Cover Trusted Publishing requirements |
||
|
易良
|
a07f52acfe
|
fix(tests): retry acp-cron cleanup rm on transient ENOTEMPTY (#9559)
The recurring cron job re-fires every ~5s under QWEN_CODE_TEST_CRON_FAST. A second fire can race the cleanup's recursive rmSync and drop a fresh file into the fake HOME after the walk has drained it, making the final rmdir fail with ENOTEMPTY. Add maxRetries/retryDelay so the transient write settles before the removal gives up. |
||
|
jinye
|
2e6151aa15
|
fix(serve): Harden standalone conversation primitives (#9512)
* fix(cli): pass ConversationDirectoryIdentityError cause through native Error options * fix(cli): re-inspect raced standalone directories and report first creation as created * fix(core): measure JSONL head integrity against a line budget * fix(core): Preserve plain JSONL record budgets Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> |
||
|
Shaojin Wen
|
a8a855914b
|
fix(review): run verifier probes in a private scratch worktree (#9207) (#9221)
* fix(review): run verifier probes in a private scratch worktree (#9207) Step 4's verifier is the review's one writing agent: it writes a probe, runs it, applies the one-line fix its flip-check needs, and restores. All of that landed in the shared review worktree — the tree `working_dir` pins every other agent to — and the pipelined loop launches a round's verifiers alongside the NEXT round's reverse auditors, so those writes are live exactly while the auditors read. "Leave the tree as you found it", which the brief has always said and verifiers do obey, cannot close that window: the exposure is *during* the probe. Measured on a live run, a round-5 auditor read a probe's mutant plus a leftover probe test and came within a step of filing a Critical against code no commit contains; it recovered only by improvising `git show HEAD:`, a fallback no brief mentions. Three parts, because isolation alone is a guarantee one regression away from being false: - `qwen review scratch-tree` gives each verifier shard a throwaway sibling worktree at the commit under review, with the review worktree's node_modules linked in so a unit harness starts without an install. Every call hands back a pristine tree (a previous finding's mutant surviving into the next probe would be a wrong verdict carrying a deterministic source tag); the label is the shard's record key, because the shards of one round run concurrently; and a tree it cannot create makes the probe inconclusive rather than falling back to the shared worktree. This is the isolation the test-efficacy probe has had since #6832 and the A/B's base tree has, extended to the last step that writes. - Every code-reading brief — dimension agents, chunk agents, reverse auditors and the verifier itself — now carries the rule that auditor had to invent: the worktree is shared, code that is not in the diff and not in the commit is not a finding, and anything surprising is judged against `git show HEAD:`. - `agent-prompt` reads the tree once per call, and every wave of agents is built by it immediately before launch: residue is named in each brief it builds and warned about on stderr, so a contaminated tree is announced to the agents about to read it instead of being discovered as a phantom Critical. `exposeDependencies` moves to `lib/worktree.ts` beside the other disposable-tree machinery, and now farms each workspace member's own node_modules as well as the root's. Measured on this repo, a tree with 1560 root packages linked still could not resolve `@testing-library/react` for a UI probe, because npm could not hoist that copy out of `packages/cli` — which also silently cost the efficacy probe the same class of test. `cleanup` sweeps the scratch family by prefix; the label half is the shard's key and cannot be reconstructed by the sweeper. * fix(review): address the scratch-tree review — two criticals and the honesty gaps Both Criticals were real, and both had the same shape: a claim the code made that one machine class or one leftover state falsified. - The welded `scratch-tree` command interpolated `--worktree` unquoted, so on any checkout under a path with a space or an apostrophe every shard's isolation was silently unavailable and every probe fell back to a reading. Quoted with `shellQuotePath`, like every other path this file prints into a command. - The reuse gate checked only that the scratch path EXISTS. A bare directory there — the leftover of a crashed `worktree add`, or of a cleanup whose `rmSync` failed — has no `.git`, so git walked up and ran `checkout --force --detach` against the user's own checkout: their uncommitted work discarded, their HEAD detached onto the PR's commit, and `rev-parse HEAD` then returning the sha that made the reset report success. Gated on the tree being a registered worktree, with the regression pinned by a test that fails on the un-gated code. The rest are the same defect class at lower stakes — something stated more broadly than it holds: - The residue probe hand-parsed porcelain's rendered form, so a path with a space, a non-ASCII byte, or a literal ` -> ` came back as a name matching nothing on disk, `--untracked-files=normal` collapsed a whole probe directory to one unactionable `dir/` entry, the 1 MB `maxBuffer` default answered the dirtiest trees with "clean", and the cap truncated silently while both renderers presented the list as complete. Now `-z --untracked-files=all` with a 64 MB buffer, and a `{paths, total}` result both renderers disclose. - `git checkout -- <path>` restores from the INDEX, so the advised recovery left staged residue in the tree; it now says `git checkout HEAD --`. - `git show HEAD:<path>` cannot produce an untracked path — the prototypical residue — so the rule now says what that answer means instead of handing the reader a command that exits 128. - Agent 8's `--whole-diff` block reads the same shared worktree and got neither the rule nor the residue paths; it is built outside `buildLaunch`, which is exactly how it was missed. - The stderr warning claimed every agent had been told; the block is gated on `reviewsCode`, and Agent 7 — which builds and tests that tree — is not. - A label that flattens to no path-safe character fell back to a shared `agent` tree, `git clean -fd` left a nested repo standing while the report said the tree was pristine, `dependencies: null` said "no node_modules" about a farm that had failed to link, `{0,0}` read as "already in place" for a `node_modules` holding nothing linkable, and `--out` was validated after the tree and its farm already existed. - The docs stated the isolation unconditionally: local-diff and file-path reviews have no worktree and no scratch tree, and SKILL.md/DESIGN.md and the user page now say so. Tests moved with the code (`exposeDependencies` and `worktreeCreateFailureDetail` now live beside `lib/worktree.ts`), and the plumbing that was pinned at both ends but not in the middle — that a verify shard's recorded brief carries ITS record key as the scratch label — is pinned by a test that fails when the key is dropped. * fix(review): close the round-2 review — Windows guard, Agent 7's blind spot, unpinned wording The Critical is a test that would only fail where this PR's CI does not look: the new chmod-based case guards on `process.getuid`, which is undefined on Windows, and `chmodSync` on a directory there sets a read-only attribute that does not stop `git worktree add` from creating a subdirectory — so the merge_group-only Windows leg would go red for every PR carrying the file. Sixteen of the seventeen chmod-permission tests in this repo already skip win32; this one now does too. The rest are the same class as round 1 — a claim wider than the code: - **Agent 7 had no protection for residue that PREDATES the round.** The exclusion's justification ("its own commands are the writes it sees") is only true for residue a round creates. A tree that starts dirty reaches Agent 7's compile and test run, where a `[build]`/`[test]` finding is pre-confirmed and skips verification — a merge-blocking phantom Critical of exactly the class this machinery exists to prevent. The residue paragraph now goes to EVERY brief (with "a defect confined to these paths is not a finding"); only the reader rule stays scoped to the roles that review code. The stderr warning says to restore before launching the wave, not before the next round. - **Residue paths reached two sinks unflattened.** `inertPath` moved to `lib/paths.ts` and now covers the scratch-tree note and the orchestrator's stderr line as well as the briefs — the `-z` format this PR introduced is precisely what lets a control byte in a filename arrive intact. - **The recovery wording could not clear two shapes the probe reports.** A path staged as NEW and a rename destination are in the index but not in HEAD, so `git checkout HEAD --` cannot match either; both renderers now name `git rm --cached` for those and `rm -rf` for untracked residue (including the nested- repo directory entry `--untracked-files=all` still cannot expand). - **The "full set" command was the one this PR calls unusable.** Both notes now say `git status --porcelain --untracked-files=all`, since the default collapses the probe directory whose files the count came from. - **`alreadyPresent` believed an empty farm dir.** The dir a previous call creates when the source holds nothing linkable is gitignored, so the reset spares it, and the second call flipped "no harness will start here" into "already in place" with nothing changed. It now requires a non-empty farm. - Doc corrections: `resetScratchTree`'s header still said `clean -fd`, and the residue probe's comment claimed `--untracked-files=all` ends directory-shaped entries — it does not for a directory holding its own `.git`. The invalid- UTF-8 limit (`encoding: 'utf8'` maps a bad byte to U+FFFD, and no string form of that name resolves) is documented rather than papered over. Pinned, each verified by reverting the fix and watching the test go red: the apostrophe ESCAPE in the welded command (the fixture had no apostrophe, so a naive `'…'` wrap passed), the `git checkout HEAD --` wording in both places, the capped-note arithmetic, the farm-failure note, stdout-before-side-file ordering and the exit-1 arm it also covers, and `sharedTreeResidueTotal` on the creation-failure return. * fix(review): close the round-3 review — hooks, hidden mutants, planted farms, suppression after restore Four of the seven Criticals are the same discovery from four directions: a scratch tree is a LINKED worktree and a `clean`/`checkout` reset is not the guarantee it reads as. - **Hooks resolve to the user's repository.** `git worktree add` and `checkout --force` both fire `post-checkout` from the common dir — the user's own `.git/hooks` — so creating or resetting a scratch tree executed whatever that repository holds. Every git call this command makes now runs with `core.hooksPath` pointed at a path holding no hooks, and the report says plainly that hooks, config and refs are shared rather than isolated. - **skip-worktree hid a mutant through the reset.** `checkout --force` silently skips a file carrying the bit and `clean` never touches tracked files, so a probe that set it (directly or via `sparse-checkout`) left a mutant that survived with `git status` reading empty and the sha still matching. The reset now refuses when `ls-files -v` still shows a hidden entry, which routes the caller to discard-and-rebuild. - **The farm was certified by existence.** `clean -ffd` spares ignored paths to keep the dependency farm — and equally spares whatever a probe installed or planted there. `node_modules` is now the one ignored path a reuse does not inherit: it is cleared and re-linked, and `exposeDependencies` marks the farms it builds so a directory it did not build is never certified as one. - **A broken leftover could not be rebuilt over.** A `.git` gitfile whose admin entry survives makes `worktree remove` fail and the next `worktree add` refuse "missing but already registered"; `discardWorktree` now prunes. The fifth is about the instruction rather than the tree: the residue paragraph is baked into every brief at build time, so restoring the paths and then launching the already-built wave tells every agent to drop findings in a file that is by then exactly the PR's code. Both the stderr warning and SKILL.md now say to rebuild the wave after restoring. The remaining two are the three new real-git fixtures missing `isolateHostGitConfig()`, which a polluted host gitconfig (`commit.gpgsign` with no key, a `core.hooksPath` hook) turns into suites that fail for reasons the branch never touched. The suggestions, in one line each: a rename now reports BOTH of its names (the restore needs the one that is gone); a `git status` that dies is reported as UNMEASURED rather than clean, in both renderers; `inertPath` covers `\p{Cf}` and `\p{Zl}`/`\p{Zp}` — bidi overrides and zero-width characters passed through the sanitizer that exists to defuse hostile filenames; residue paths and tree paths are shell-quoted where the notes prescribe commands over them (`rm -rf my probe.ts` deleted `probe.ts`); the verifier brief no longer says "then move on" after applying a candidate fix, because one tree serves every finding in a shard; `scratchLabel` strips a leading dash, which yargs read as a flag; the member-farm loop guards per member rather than around the loop; the unguarded `mkdirSync`/`readdirSync` in the farm now count as failures instead of throwing out of a best-effort contract; cleanup discloses a `readdir` failure instead of reporting "nothing to clean", and sweeps a dangling symlink `releaseWorktree` cannot see; the briefs' restore recipe gained the staged-only branch the scratch-tree note already had; and both "pristine" claims now say that gitignored paths survive. Also corrected: "the one agent whose job requires writing" — Agent 7's efficacy probe writes too, and has had its own tree since #6832. New tests, each verified by reverting its fix: the hooks suppression, the skip-worktree refusal, the planted-farm replacement, the prune, staged-residue detection, the unmeasured state, `inertPath`'s character class, and the residue reaching every launch class rather than the one role the earlier test inspected. * fix(review): close the round-4 review — untrusted workspace paths, member farms, locked leftovers The sharpest two are about treating the reviewed PR's own manifest as data rather than as input, in code that DELETES: - `exposeDependencies` fed workspace dirs from the root manifest of the code under review straight into `join()` and then into the farm's opening `rmSync`. A PR setting `"workspaces": [".."]` — or a committed SYMLINK at a workspace path, which `readWorkspacePackages` follows deliberately because npm does — pointed that delete at a directory outside both trees; in this pipeline's layout, at the reviewer's own checkout. Every member is now resolved through `realpathSync` and required to be contained in the tree it belongs to, which closes the string and the symlink vector together, and a member that escapes is counted as failed rather than silently skipped. - The reuse path wiped only the ROOT farm, so `<tree>/packages/<member>/ node_modules` survived with its marker and was certified as-is — the same hole round 3 closed at the root, one level down, where Node resolves a member's imports FIRST. `exposeDependencies` now takes `rebuild`, and the reuse path distrusts every farm rather than the top one. Two more that would have wedged a review: - `git worktree prune` never drops a LOCKED admin entry, and probe code has a shell inside these trees: one `touch` in the admin dir the tree's own gitfile names, and every later `worktree add` for that PR fatals "missing but locked" — permanently, since cleanup prunes too. `discardWorktree` now unlocks and retries with the second `--force`. - `ls-files -v` and `clean` ran under `spawnSync`'s default 1 MiB buffer, which a large repo passes; Node kills the child, the reset reads that as failure, and every reuse rebuilds forever. And a Windows one of the same class as round 2's: the new fixture wrote a file named `a -> b.ts`, and `>` is reserved on NTFS — the merge-queue-only Windows leg would have failed at fixture setup. Split, with the arrow half skipped there (the shape it pins cannot exist on NTFS). The rest: the residue restore recipe was WRONG for a staged rename's original name (`git rm --cached` stages a deletion there; `git checkout HEAD --` is what clears it) and the test that certified it never staged anything — both fixed, and the test now builds all four real shapes; the reader rule was gated on `reviewsCode`, which left Agent 0 and the test matrix reading worktree source without it (the gate is now "every role that judges code", i.e. all but Agent 7); the stderr warning did not actually carry the rebuild-after-restore instruction SKILL.md attributes to it; `ScratchTreeReport` dropped the `unmeasured` state, so a failed `git status` read as clean to a script; the scoped-package branch linked non-directory entries the top-level branch skips; `farmDependencies`' catch and its note branch were unreachable once round 3 guarded the fs calls, so they are gone rather than pretending to be a net; cleanup's family-read failure now fails the run instead of letting it announce "Nothing to clean", and its dangling-symlink branch no longer swallows a throwing `rmSync`; the residue note says the names are flattened for display and where the exact bytes are. Disclosed rather than enforced, with the reason in the code: the farm's links are read-write and point into the shared worktree, so a probe that writes THROUGH one (an `npm rebuild`, a package that writes into its own directory) lands outside its tree where the residue check cannot see it. Copying the farm would cost the minutes it exists to save; the verifier's block now says to replace a link with a copy before modifying a dependency. New coverage: workspace escape and symlinked-member containment, the self-farm guard, rebuild-on-reuse at member level, assume-unchanged beside skip-worktree, the unmeasured state through its renderers, the dangling-symlink sweep, the family-read failure, and the cleanup mock gaps that made two of those branches unreachable by construction. * fix(review): close the round-5 review — scoped prune, real pristine, submodules, symlink gadgets The findings that hold without assuming an attacker already has a shell: - **The prune added in round 4 was repo-wide.** `git worktree prune` drops any admin entry whose directory is momentarily absent — another shard's `worktree add` mid-flight (this pipeline runs discards and adds concurrently against one common dir), or the user's own worktree on a volume that happens to be unmounted. It now removes the one entry whose `gitdir` file names this path, and nothing else. - **"Pristine" spared ignored paths, and a probe's state lives there.** Its own `node_modules` at any depth, a `.tsbuildinfo`, a `dist/` it built and then mutated — all survived a reset the report called pristine, and the farm-level wipe could not reach them (a member farm whose source has none was skipped before the target was touched; the root rebuild was skipped entirely when the review worktree had no `node_modules`). The reset is now `clean -ffdx` and the farm is re-linked, so pristine means pristine. - **An initialized submodule was untouched by all of it**: `checkout --force` without `--recurse-submodules` leaves its working tree, `clean` never touches a tracked gitlink, and `rev-parse HEAD` is the superproject's. A tree holding one is now rebuilt rather than reset — a fresh `worktree add` leaves submodules uninitialized, so the rebuild is both correct and cheap. - **test-efficacy reuses one probe tree across every suite run**, and the code running in it is the PR's own test code. Round 4 answered this with the positive control; the control only covers the green-forcing direction, so the farm is now re-linked before each run instead — about a second against a 540-second budget. The remaining Criticals all presuppose code execution that this pipeline grants before any of this code runs (Agent 7 installs and tests the PR; a verifier's probe is arbitrary code by design), so they do not change what an attacker can do — but three of them describe gadgets that cost nothing to remove, and they are removed: - `cleanup` no longer hands ANY symlink at a family path to `git worktree remove`, which would follow it and delete whichever registered worktree it points at. It unlinks the link and reports that. - `resetScratchTree` validates that the tree IS the tree — not a symlink, and `rev-parse --show-toplevel` resolving to itself — before running a reset that would otherwise land wherever the path resolves. - `farmNodeModules` lstats its SOURCE root, so a symlink at the review worktree's `node_modules` cannot redirect every probe tree's dependencies. Also: the `dependencies: null` contract now says what it means (no `node_modules` to link from; a link failure arrives counted, not as null), the `alreadyPresent` branch of the note is gone with the reuse path that could reach it, and the residue note carries the flattened-names disclosure its sibling renderer already had. New tests: the symlinked scratch path, ignored-state clearing, the submodule rebuild, and the unmeasured pass-through through the command's own report. * fix(review): close the round-6 review — committed farms, foreign repos, deinit'd submodules One finding crosses the line the round-5 reply drew, and it is the important one: `node_modules` is gitignored by convention, not by rule, so a pull request can force-add `node_modules/.qwen-review-farm` beside its own module stubs and `git worktree add` checks both out. The fresh-create path then found a marker and certified the PR's own modules as the farm this code built — reachable from PR CONTENT, with nothing executed. Two changes: the marker records the dependency root it was built from and is only believed when it names this one, and both scratch-tree paths (fresh and reuse) now rebuild rather than trust anything found at that path. The other three that hold on their own: - **The identity check added in round 5 was incomplete.** `rev-parse --show-toplevel` prints the directory the `.git` file sits in, whatever that file points at — so a gitfile naming another repository, or a whole repo planted at the predictable scratch path, passed it while every command below would have run against someone else's objects, refs, hooks and config. The tree must now share a common dir with the review worktree. - **`git submodule deinit` walked through the round-5 submodule gate**: the status line goes back to `-` while the submodule's gitdir — its hooks, its config, its objects — survives under the common dir and is resurrected by the next `update --init`. The gate now reads the commit's gitlinks instead of the submodule's state, so a repo with submodules rebuilds rather than resets. - **The round-5 anti-planting rebuild never reached the positive control or the hunk probes.** They defaulted `dependencyRoot` to the probe tree, where `exposeDependencies` returns before clearing anything — so the control, the run that decides whether ANY mutant verdict is trusted, resolved through whatever the baseline's test code had planted. Smaller, from the same review: a dangling symlink at the target skipped the rebuild wipe (`existsSync` follows links); a dangling top-level link in the source was dropped without counting; a stray file under a scope directory was linked as a package where the top-level branch skips one. Disclosed rather than fixed, with the count now in the report: the farm mirrors npm workspace SELF-links, so an import that goes through a package NAME resolves back into the review worktree's copy and a mutation made in the disposable tree is invisible to it. Re-pointing them would break resolution outright for any package whose entry point is a build artifact a fresh checkout does not have, so `DependencyFarm.selfLinked` carries the number instead of the layout carrying it silently. Not fixed, and this is the third round of stating it: the remaining findings need a prior arbitrary-code-execution foothold (a background process racing the containment check between resolve and use). The pipeline grants that capability several steps earlier and by other code — Agent 7 runs `npm ci` and the PR's test suite — so hardening this command changes which line appears in the trace, not what is possible. The place to change it is sandboxing the review's command execution, which is its own design. Tests: the locked-leftover recovery, the scoped registration drop (a sibling worktree whose directory is absent survives it), the foreign-repository gitfile, the deinit-proof submodule gate, and the scope-directory stray file. The foreign-repo fixture is a CLONE on purpose — with an unrelated repo the reset fails for the wrong reason and the test passes without the guard it pins. * fix(review): close the round-7 review — walk-up escapes, identity gates, farm wipe Five Criticals, each probe-verified on real git before the fix and mutation-checked after: - worktreeResidue verifies the path IS a worktree before measuring: with the .git file gone, git discovery walks up into the user's checkout and answers with the user's own dirty state — fail closed instead (R7-4). - runScratchTree applies the same --show-toplevel check to the trusted --worktree argument before resolving HEAD (R7-3). - The reuse identity gate rejects a tree whose gitdir equals the common dir: a .git symlinked or hand-edited to name it passed every prior check while checkout --force detached the user's main HEAD onto the PR sha and rewrote the main index (R7-2). - farmNodeModules detects the rebuild target with lstatSync: a dangling symlink a PR committed as node_modules read as absent, skipped the wipe, and died EEXIST on every rebuild (R7-1). - The residue probe's node_modules/dist exclusion is enforced by a pipeline-controlled excludes file rather than borrowed from the commit's .gitignore (R7-5). Plus the production-shaped UNMEASURED fixture (R7-19) and the three remaining stale contract texts on the reuse reset (R6-7). * fix(review): close the round-8 review — tripwire identity, GIT_DIR redirects, symlink writes Round-8 Criticals, each probe-confirmed and pinned by a test that goes red when the fix is reverted: - The residue tripwire certified a CLEAN status for a repository planted over the contamination (`rm .git && git init && commit`): no local check can tell a planted repo from the tree it replaced. It now fails closed unless `.git` is a gitfile, and reports unmeasured for what `git status` cannot see inside committed gitlinks (a non-empty submodule directory) instead of clean. - An inherited GIT_DIR (or GIT_WORK_TREE/GIT_INDEX_FILE/...) redirected every identity check at once — both sides of each comparison see the same override, so none could detect it. Every git call in the scratch-tree lifecycle and the tripwire now drops the redirect variables. - The scratch-tree reuse gate accepted a planted gitfile naming a SIBLING worktree's admin entry and reset the sibling: the gate now requires the admin entry's gitdir backpointer to resolve to this tree. - Probe writes followed PR-committed leaf symlinks (mode 120000) into the shared review worktree — the positive-control injection, every mutant and every hunk restore. Each write site refuses a symlinked target as inconclusive. - discardWorktree handed a symlink at the tree path to `git worktree remove --force`, which resolved it and force-deleted the registered victim it pointed at; a symlink is unlinked instead. Plus the record-key comments (chunk id separates --all-chunks shards) and the orphaned marksOurFarm doc block. * fix(review): close the round-9 review — gitlink residue, env redirects, probe escapes Round-9 Criticals, each probe-confirmed on real git before the fix and pinned by a test that goes red when the fix is reverted: - worktreeResidue's gitlink blind set was parsed from RENDERED `ls-files -s`: default core.quotepath quotes a non-ASCII gitlink name into a spelling that never resolves on disk, the entry dropped from `blind`, and a contaminated gitlink was certified CLEAN. It now reads `-z` (R9-1). - An exported GIT_DIR redirected every inheriting git call at once: test-efficacy's head-sha read, probe resets and revert checkout, and base-tree's add and reuse check. Every git spawn in both files now drops the redirect variables (R9-16). - cleanup's symlink guard covered only the scratch family; the three named family paths still reached releaseWorktree, whose existsSync follows a LIVE link and whose `worktree remove --force` resolves it — deleting the registered victim while reporting success, and silently skipping a dangling link. The guard moved into the shared report path all four families go through (R9-18). - exposeDependencies' rebuild wiped only the farm-owned node_modules; anything planted at another path — packages/node_modules resolves before the root farm in a packages/* workspace — survived between probe runs and decided their verdicts. Rebuild now wipes every node_modules the farm does not recreate, following no links (R9-37). - NO_HOOKS covered hooks only, not config-driven content FILTERS: a planted `filter.<name>.smudge|clean` plus one attributes line — both writable into the common dir — executed on every reset and rebuild checkout. runScratchTree detects repo-local filter config and refuses rather than run it (R9-38). - The harness never WRITES probe test files, so no write guard ever saw one committed as mode 120000 — vitest collected it through the link and scored mutants against code the tree never mutated. Probe files are checked against the committed index and dropped as not-run before any suite collects them (R9-40). - The probe write guards lstat-checked the LEAF only; lstat resolves intermediate components, so a relinked ancestor in a reused tree read ordinary and every later write followed it out of the tree. The guard walks every component (as safeRmWithin does for deletes), and each restore write re-validates immediately before writing — a mid-run relink stops the phase instead of writing through the link (R9-17). * fix(review): close the round-10 Criticals — env config injection, sweep scope, probe-set holds Taken back from the autofix loop, and scoped to Criticals from here: the findings that hold without assuming a foothold, plus the ones whose fix is a few lines and removes a step from a chain. Reachable from the environment or from PR content, with nothing executed: - **`sanitizedGitEnv` dropped only the discovery redirects.** `GIT_CONFIG_COUNT` with `GIT_CONFIG_KEY_<n>`/`GIT_CONFIG_VALUE_<n>` sets any config key for the run — `core.fsmonitor` and the `filter.*` pair are command execution — and `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM`/`GIT_CONFIG_PARAMETERS` reach the same place by other routes. A gate on the front door with the window open. - **`lib/git.ts` never had the gate at all**: `gitOpts()` spread `process.env` into every shared helper, `releaseWorktree`'s `worktree remove --force` included — the commands that run against the user's own repository, so they need it more than the disposable-tree ones that got it first. - **The residue probe could be steered by the tree it measures**: `core.fsmonitor` runs a command on `status`, so the tripwire was an executor. Emptied for that invocation, the same discipline the checkouts' `core.hooksPath` already had. - **`probeTargetEscapes` treated a backslash as a separator on POSIX**, where it is an ordinary name character: `x\y.test.ts` split into phantom components, the first `lstat` died ENOENT, and the catch reported "no escape" for a leaf nobody looked at. It also never checked the tree ROOT, which defeats every per-component check at once. - **The symlink drop reassigned the probe list before the collocated-test hold ran**, so a mutant whose own test was dropped answered "there is no such test" and was scored `survived` on the strength of the other probes passing — the hold exists to refuse exactly that. - **The rebuild sweep matched `node_modules` case-sensitively** on the two filesystems this file already special-cases: `Node_Modules` resolves like the real one on APFS and NTFS and survived every rebuild. - **A tracked path in the residue list suppressed real findings.** The brief said "none of it is the PR's code, so a defect confined to those paths is not a finding" — false for a file the commit contains and this PR changes: only the uncommitted DIFFERENCE is foreign. It now separates the two, and a defect present in the committed version stays a finding. Cheap chain removal, though each needs a prior foothold: - The excludes file moved from a constant name in the shared temp dir to a `mkdtemp` directory written `wx` — git re-reads its CONTENTS on every later `status`, so a writable fixed path could blind the tripwire. - `discardWorktree` now clears the registration named by the TREE's own `.git` pointer, read before anything is removed, instead of scanning `<common>/worktrees/<id>/gitdir` files — which any same-user process can rewrite to aim this cleanup at a live sibling. The scan remains only as the fallback for a tree whose pointer is unreadable, which is the case the step exists for, and it now refuses entries whose tree still stands. - The scratch sweep refuses to run at all when its parent is not a directory: a symlinked `.qwen/tmp` redirects every delete below it, and no per-leaf check can see an ancestor. - `removeUnownedNodeModules` counts a wipe it could not perform, instead of letting a planted directory survive every rebuild undisclosed. Left as recorded decisions, not re-litigated: R4-5, R6-2 and the R6-5/R9-14/ R9-38/R9-40/R10-25/R10-27 class, which need probe code already running as the user — a capability Agent 7's install and test step grants several steps earlier. The remaining Suggestions stay on the deferred queue for a follow-up, per the triage gate's own call that seven rounds was the moment to land this. * fix(review): close the round-11 Criticals — the half-fixes from round 10 Five of the eight are residue from the previous round's own fixes, which is the honest reason to take them: - **The fsmonitor neutralization covered one spawn of two.** `git status` got `-c core.fsmonitor=`; the sibling `git ls-files -s -z` that builds the gitlink blind set did not, so the same planted command still fired during the measurement. - **The gitlink blind-set filter failed OPEN.** An absent directory is the clean shape `worktree add` leaves; an UNREADABLE one is a place neither `git status` nor this probe can see, and both answered "nothing to hide there". Only ENOENT is clean now. - **The wipe walk skipped symlinked directories entirely**, so a `node_modules` behind a PR-COMMITTED symlink directory survived every rebuild with nothing said. The walk still refuses to follow a link — the farm's own entries point out of the tree — but a link resolving back INSIDE the tree that hides dependencies is now counted as a failure the caller reports. - **The sweep's ancestor check stopped at its own argument.** `lstat` refuses to dereference only the last component, so a symlink one hop above (`.qwen` over `.qwen/tmp`) resolved silently and redirected the same deletes. Every ancestor up to the filesystem root is checked. - **Detection was followed by the damage it detected.** When the mutation phase aborts because the probe tree was relinked mid-run, the revert phase ran anyway — `git checkout base -- …` and `safeRmWithin` with a cwd resolving through the link into the shared review worktree. The phase now re-validates the tree first, and `safeRmWithin` lstats its own ROOT, which its docstring always promised and its loop never did (a root link resolves the whole prefix in the kernel, so every component below it looks ordinary). Left as recorded decisions: R11-1 (`refs/replace` in the shared common dir), R11-2's residual fallback path, and R11-3 (content filters during `status`) — each needs probe code already running as the user, the capability Agent 7's install and test step grants several steps earlier, and the scan fallback exists precisely for the corrupt-pointer recovery the earlier rounds pinned. * fix(review): close the round-12 Criticals — sanitizer gaps, sweep scope, false farm failures Nine of the twenty-four hold without a foothold, or are defects in the previous rounds' own fixes: - **`residue.unmeasured` rode raw into two sinks** while the paths beside it went through `inertPath` — and that string is built from `ls-files -z` gitlink names, so a PR that commits a gitlink with ESC bytes puts a control sequence on the orchestrator's terminal and in the verifier's note. - **The branch delete never got the env the check did.** `refExists` resolves the real repository through the sanitized helpers, while `git branch -D` in cleanup and fetch-pr still inherited `process.env`: with `GIT_DIR` exported the pipeline verified a branch in one repository and deleted it in another. - **The ancestor refusal guarded one sweep of three.** It announced that `.qwen/tmp` hangs off a symlink and the same function went on to delete the base-tree lock and every side file underneath it. The check moved to the top of `runCleanup` and now refuses the whole clean. - **A family symlink that would not unlink released the lease**, unlike the three sibling failures that hold it, leaving the next `fetch-pr` to pass a gate over an occupied path. - **The symlink disclosure added last round produced FALSE failures**: it never consulted the `owned` set, so a PR-committed `alias → .` counted a failure for the farm this very call had just re-linked, and it judged before the walk had wiped the target it was complaining about. It now runs after the walk and skips owned farms. - **Probe names were passed to `git ls-files` as raw pathspecs**, so a probe committed as `:(literal)x.test.ts` — a legal filename — was parsed as magic and its symlink never found. Every pathspec is `:(literal)`-prefixed now, and a refused pathspec drops every probe rather than answering "no symlinks". - **The residue oracle could be blinded by index bits.** `skip-worktree` and `assume-unchanged` make `status` answer clean for an edited tracked file — the hazard the scratch tree's reset already refuses to certify around, missing from the reader-side probe that tells auditors the tree is pristine. - The excludes file is created 0600 rather than 0644. Left on the recorded boundary: the filter-gate coverage findings (R12-6, R12-24), the hardlink and runtime-relink shapes (R12-31, R12-32, R12-72). Each needs the PR's own test code running as the user — the capability Agent 7's install and test step grants several steps earlier — and the last three are properties of executing untrusted tests at all, not of this command. * fix(review): close the round-13 Criticals — spelling, empty probe sets, raw error text Five hold without a foothold, and four of them are defects in the previous round's own fixes: - **`owned` mixes path spellings by construction** — the tree root as the caller spelled it, each member as `containedIn` resolved it — and last round's disclosure compared only one of them, so a farm reached through a link counted a failure for a directory that same call had just re-linked. Both spellings are asked now, through one helper the wipe and the disclosure share. - **A suite run with an EMPTY probe list runs vitest with no filter**, which collects whatever the repository holds and scores it as this probe's evidence. After the committed-symlink drop empties the set, the mutation and revert gates now skip rather than run. - **Git's own stderr rode raw into the verifier-facing note** through `Error.message`, as did the filter KEY NAMES in the refusal (a git subsection name carries any byte but newline and NUL). Same sink class as last round's `unmeasured`, flattened the same way. - **Every refusal that fires before the residue is measured answered with the empty list a measured-clean tree produces.** A consumer could not tell "the tree is clean" from "this call never looked"; the refusals now carry the unmeasured reason. Left on the recorded boundary, re-checked and unchanged: R12-6, R12-22, R12-24 (config planting), and the R13-9/R13-10/R13-13 family, which need probe code already running as the user. * fix(review): close the round-14 Criticals — farm containment, residue blinds, release guard Five hold without a foothold, and three of them are channels a pull request controls outright — no same-user foothold anywhere: - **Farm entries were mirrored link-or-not.** A committed symlink under `node_modules` (force-add defeats gitignore) pointing out of it became a write channel from the disposable tree to wherever it points — into the shared worktree's tracked files — recreated on every rebuild. Entries now resolve through realpath before linking; only a borrowed `node_modules` or an npm workspace self-link passes, everything else is counted and disclosed. (R10-18) - **The residue probe's untracked view came from `status` alone**, which honors ignore rules the contaminator controls: a committed whitelist-form `.gitignore` (`*` with `!`-negations) blinded it to contamination. The probe now merges `ls-files --others` without `--exclude-standard` and filters the pipeline's build artifacts in code. (R10-19) - **A gitlink named in bytes UTF-8 cannot decode dropped from the blind set**: the mangled spelling never resolves on disk, the readdir read absent, and a contaminated gitlink certified clean. Such names now fail closed into unmeasured. (R11-4, second entrance) - **The `owned` set the rebuild disclosure asks held one spelling** while the disclosure loop presented another — on a host whose tree path carries a symlinked ancestor (macOS's `/var` vs `/private/var`) every rebuild counted a phantom failure for the farm the same call had just re-linked. The set is normalized once, at build time. (R13-1 remainder) - **`releaseWorktree` followed a symlink standing at the path**: `existsSync` resolved it and `git worktree remove --force` deleted whichever registered worktree it named — and a dangling one wedged the next `worktree add` while invisible. The lstat-first guard cleanup's family sweep applies to every path now lives at this choke point, which is where fetch-pr's `cleanStale` releases. (R13-3) Pinned against real git and real filesystems; all seven new tests fail on the pre-round sources. * fix(review): skip the invalid-UTF-8 gitlink fixture off Linux APFS and NTFS both refuse a filename that is not valid UTF-8, so the fixture cannot be created there — `mkdir` fails ENOENT on macOS, and the round-14 test reddened for every maintainer on a Mac while the repo's `pull_request` CI, which runs the unit legs on Linux only, stayed green. The shape it pins cannot exist on those filesystems either, so the skip loses no coverage. Same class as the round-2 and round-6 guards (`chmod` semantics, NTFS reserved characters): a fixture that cannot exist on a platform is skipped there rather than left to fail in a leg nobody watches. * fix(review): close the round-15 Criticals — a tripwire that was firing on every healthy run The important one is a regression this PR introduced two rounds ago, and it is measurable rather than hypothetical. Round 14 answered "a `.gitignore` can hide a probe's leftovers from `status`" by merging the ignore-INDEPENDENT untracked listing into the residue set. Measured on a healthy review worktree of this repo, after the `npm ci` and build the pipeline itself runs there: `git status` reported NOTHING and the tripwire reported 3 957 paths — coverage HTML, `.tsbuildinfo`, husky's installed hooks. Every one of them reached every verifier and every reverse auditor as contamination, with `rm` and `git checkout` recipes attached, and real residue would have been three lines inside that. A tripwire that fires on every healthy run is not read. What separates the two sets is not the pattern, which is unbounded, but WHO WROTE THE RULE. An ignore file the commit under review carries is ordinary repo hygiene that every reader of that tree sees identically; a rule from anywhere else — `info/exclude`, a `.gitignore` written after the checkout — is the plant. `check-ignore -v` names the source file per path, so the reconciliation asks git that question instead of inferring it from the name. One exception, because it is the whitelist form's whole trick: a rule from the commit that hides EVERYTHING (`*` with `!`-negations) vouches for nothing and is not believed. Measured again through the real function afterwards: 3 957 → 0, with the two files that were genuinely modified at that moment still named. Also: - **The index-bit oracle failed OPEN.** Its guard asked for `status === 0`, and a spawn that dies leaves `status` null — so the one tree it could not read the skip-worktree/assume-unchanged bits of was certified clean, which is precisely the tree that can be carrying a mutant `status` cannot see. It now fails closed like its three siblings, and takes `-z` so `core.quotepath` cannot octal-expand a non-ASCII name past the buffer. - **The efficacy probe tree carried tracked writes between runs.** Re-linking `node_modules` before each run (round 5) covers half of what a run leaves behind; tracked files are the other half and the more direct one — a suite that rewrites a probe file after vitest has collected it stays green for its own run and hands every later run a file of its choosing, and the verdict that buys is `killed`: "a test catches this", asserted for statements no test covers. Every run now opens by putting the tree back to its commit, guarded by the same identity gate the residue probe uses so a directory that is not a checkout is skipped rather than having the enclosing repository checked out into it. Two fixtures had to change with it, and both were relying on the hole. The control-setup test made its point by having the fake runner DELETE the probe file; a deleted probe file now comes straight back, so it relinks it instead (the read-failure path itself is pinned directly in the unit suite). The wrong-line-mutant test wrote its mismatched content without committing it, which is not a state a probe tree can be in — a probe tree is a detached checkout — so it commits it. Each fix was verified by reverting it and watching the new test go red: the provenance filter, its trust direction, the catch-all exception, the `.husky/_` footprint entry, the fail-closed oracle, and the tracked-file restore. * chore: keep main's package-lock.json A local `npm install` during the merge dropped six `"peer": true` markers from the lock. Unrelated to this branch, so it goes back to main's copy. * fix(review): close the round-16 Criticals — catch-alls, the exec env family, untracked plants Two of these are holes in last round's own fix, which is the right way round for them to be found: - **`hidesEverything` recognised only the `*`/`**` spellings.** `?` matches any single character, so `?*` is `*` with extra steps — a committed `.gitignore` whitelist written that way vouched for everything it hid, with no execution anywhere: it is committed content. The check is now shape-based rather than a list of spellings: every segment wildcard-only, at least one `*`. Enumerating the spellings one at a time is how the hole got made. - **The probe tree's pre-run restore failed OPEN on an unresolvable `.git`.** The gate skips a directory with no `.git` because there is no commit to restore FROM — but `.git` is untracked, so nothing ever restores IT, and a guest that overwrites it once bought "proceed, nothing to put back" from every later phase. Present-but-unresolvable is now a failure, which is the same discipline round 15 applied to the residue probe's index-bit oracle. And three that stand on their own: - **`safeRmWithin` split the PR-controlled path on backslashes unconditionally.** On POSIX a backslash is an ordinary filename character, so ONE committed name — `\..\review-pr-42\package.json`, legal on POSIX — became three components, two of them `..`, which `join` normalises away silently. The revert phase's delete then reached the sibling shared review worktree. `probeTargetEscapes` already split platform-dependently with a comment naming this hazard: the write side had the fix, the delete side did not. Both now, and a `..` component is refused outright rather than resolved. - **`sanitizedGitEnv` closed redirection and config injection and left execution open.** `GIT_SSH_COMMAND` and `GIT_EXTERNAL_DIFF` are a command, `GIT_EXEC_PATH` moves git's own subcommand and remote-helper lookup, `GIT_TEMPLATE_DIR` plants hooks for the next `init`. Not a new judgement call: `config/shared-env-keys.ts` blocks exactly this family for session subprocesses, with the rationale written out there. The setter need not be an attacker — a reviewer's shell profile exporting `GIT_EXEC_PATH` silently changes which `git-remote-https` every fetch in this pipeline runs. - **Nothing removed UNTRACKED files between probe runs.** Round 15 restored the tracked half; a `vitest.config.ts` — untracked, because no zero-config project commits one — is what a suite reaches for to decide the next run's collection. `clean -fd` and not `-fdx`, so the borrowed farm and the ignored build output the probes need survive. Also, the live-symlink test in `git.integration.test.ts` compared git's CANONICAL worktree path against an un-canonicalized `mkdtemp` path. It passes on macOS by accident — `/private/var/…` contains `/var/…` as a substring, so `toContain` succeeds — and would not on a fixture reached through a symlinked ancestor. It now realpaths, so it passes on purpose. Each fix was verified by reverting it alone and watching its test go red. * fix(review): close the round-17 Criticals — provenance, absent .git, ignored plants, ancestor links Three are holes in the two previous rounds' own fixes: - **"Tracked" is not "unchanged".** The round-15 provenance test asked `ls-files` whether the ignore file's PATH is in the index, so a `.gitignore` the commit carries went on vouching for rules appended to it after the checkout. The status set the same function already computed answers this: a source that appears there has been edited away from the commit, and its rules are the writer's. - **An ABSENT `.git` was read as "nothing to restore".** Round 16 made the probe tree's restore fail closed on a `.git` it cannot resolve and left the cheaper state open — `.git` is an untracked pointer file inside the tree the PR's own suite runs in, so one `rm` bought "proceed" from every later phase. Running the restore anyway is not the alternative: with no `.git`, discovery walks UP and checks the enclosing repository out into the tree. Refusing is the only answer that is neither. - **The between-run sweep honored the ignore rules.** `clean -fd` skips what the commit's own `.gitignore` names, and those rules are the PR's to write, so a plant named to match one survived every restore. It is `-ffdx` now, with `-e node_modules`: the borrowed farm is the one ignored thing in that tree the probes cannot run without, and everything else ignored goes. The two restore spawns also empty `core.fsmonitor`, which both of them execute. And four that stand on their own: - **`releaseWorktree`'s symlink guard was leaf-only.** `lstatSync` dereferences every component except the last, so a link at `.qwen/tmp` left every path under it looking ordinary while `git worktree remove --force` landed in whatever checkout it named. `runCleanup` refuses its whole sweep for this; `cleanStale` releases with no guard of its own, so the refusal now lives at the choke point every caller inherits. - **`runCleanup`'s own ancestor guard ran before a network-bound audit** and nothing re-checked it afterwards, though the lease condition beside it gets exactly that re-check for exactly that window. - **The scratch tree's filter screen read the wrong tree's config.** It runs against the review worktree, while the checkout it authorises runs in the SCRATCH tree, whose own `config.worktree` is honored once `extensions.worktreeConfig` is on. The screen now reads every entry under the common dir's `worktrees/`. - **`runOneMutant` and `runControlMutant` had no pre-write escape re-check**, while `runOneHunkProbe` — in this same diff — carries one with a comment explaining the threat. `redirectedAncestor` is now one shared function rather than two, and its walk STOPS at the checkout instead of climbing to `/`: `/var` is a symlink on every macOS box, so the unbounded version refused every sweep there while reporting that it had found a redirect. Two fixtures became real checkouts, because a probe tree is one in production and a bare `mkdtemp` no longer reaches the behaviour they pin. Each fix with a behavioural test was verified by reverting it alone and watching that test go red. The pre-mutation re-checks are window-narrowing on a check-then-use race and are not pinned by one. * docs(review): drop the JSDoc left stranded when redirectedAncestor moved Round 17 moved `redirectedAncestor` into `lib/worktree.ts` and deleted the copy here, but not the block above it — which then sat on `scratchWorktreesOf`, describing a function this file no longer has and, worse, describing the OLD behaviour: "the walk stops at the filesystem root" is exactly what round 17 changed, because `/var` is a symlink on macOS and an unbounded walk refuses every sweep there. * fix(review): close the round-19 Criticals — PR-content channels, and the guards this PR wrote for itself Two are reachable from a PR's committed content with nothing executed, which is the line this branch draws everywhere else: - **A committed `vendor -> ../stash` was never counted.** The rebuild walk discloses a `node_modules` symlink resolving back INSIDE the disposable tree and silently skipped one resolving OUT of it — so the farm reported `{linked: n, failed: 0}` while a `node_modules` under the link's target survived every rebuild. Node realpaths the importing file, so imports under the link resolve in the stash and decide later verdicts. Outside state cannot be wiped from here; it is COUNTED, which is what the contract promises. - **`workspaces: ["."]` widened the self-link whitelist to the whole shared worktree.** npm accepts a root manifest declaring itself a workspace and creates the self-link itself, `containedIn(root, '.')` answers the root, and every `node_modules` link resolving anywhere inside the review worktree then passed the verdict and was mirrored into the disposable tree as a read-write channel back. A member that resolves to the dependency root is not a member. Four more are this PR's own guards not being applied to the code beside them: - **`resetScratchTree` lstat'd only the immediate parent**, on the stated premise that `.qwen/tmp` is the one component above the leaf anything here can replace. False one hop higher: a link at `.qwen` redirects the whole path, and then every check in that gate agrees with every other because they all resolve through it — toplevel self-equality, common-dir equality, gitdir ≠ commondir, and even the backpointer round-trip. It walks every ancestor now, bounded at the repository the common dir belongs to. - **`restoreProbeTreeTracked` had none of the gates its sibling reset has.** A rewritten gitfile pointing at another repository passed `--show-toplevel` self-equality and the "restore" wrote that repository's content in; a skip-worktree bit made `checkout --force` skip a tampered file silently while the function answered "as the commit left it". Both are checks `scratch-tree` documents for the identical reset. The backpointer and ancestor checks apply to the linked-worktree shape a probe tree actually has — a standalone checkout has no admin entry to round-trip and IS the repository root. - **The revert phase collected probes screened once, from the index, before the baseline.** Every run since executes the PR's own test code, which can replace a probe with a link; the mutation writers re-check immediately before they write and this collection had no equivalent, so a relinked probe was collected THROUGH the link and scored against code the revert never touched. - **`runCleanup`'s symlink branch never pruned.** It returns before `releaseWorktree`, which is where the pipeline's only other prune lives — so the family paths were unlinked and reported swept while their registrations stayed behind to wedge the next `worktree add`. And two cheap ones with no argument against them: `sanitizedGitEnv` deleted by exact case, which removes nothing on Windows where env lookup is case-insensitive (the `shared-env-keys.ts` list it is modelled on folds case for this reason); and it now sets `GIT_NO_REPLACE_OBJECTS=1`, because one `git replace <sha> <evil>` in the common dir makes every `checkout --detach <sha>` here materialise someone else's tree while `rev-parse <sha>` still answers the original. Each fix is pinned by a test that goes red when that fix alone is reverted. * fix(review): close the round-20 Criticals — an empty probe list, a dropped refusal, a symlinked root The first one is a bug this branch shipped yesterday, and the worst kind: it turns a screen into a wider run. - **The revert phase could call `runProbeSuite` with an EMPTY list.** Round 19 added a screen that drops probes relinked out of the tree after the baseline; the `probes.length > 0` gate the phase opens with was taken before that screen could empty the list, and `vitest run` with no file argument collects the WHOLE suite — so "every probe was tampered with" became "score everything", with the verdicts attributed to files the phase never selected. It now stops the phase and says which of the two happened, once rather than per file. - **`cleanStale` discarded `releaseWorktree`'s refusal.** The guard added in round 17 declines to release through an ancestor symlink and reports why; `fetch-pr` called it as a bare statement, so the sweep looked successful and the next `worktree add` wedged at a path nobody was told about. The result is read and the reason printed, like every other failure on that path. - **The probe tree's own root was never lstat'd.** The ancestor walk added in round 19 starts above the leaf, and every identity comparison realpaths both sides — so a probe tree that IS a symlink into the shared review worktree agrees with itself all the way down, and the restore's `checkout --force` and `clean -ffdx` would have run in the tree every other agent is reading. The first is pinned end-to-end by the fixture that already relinks its probes mid-run: before round 19 it scored that relinked probe `inert` — a fabricated verdict read through the link — and the assertion now names the phase-level refusal instead. Reverting the guard alone turns it red. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> |