366 commits

Author	SHA1	Message	Date
callmeYe	f1b1305a76	feat(models): support dual-role image generation models (#9650) ... * feat(models): support dual-role image generation models * fix(models): address dual-role image selector review * test(cli): cover image model resolver rejection * fix(models): preserve legacy vision image routes	2026-08-23 05:02:32 +00:00
Shaojin Wen	509226260c	feat(review): back comment-status and presubmit for Aone Code targets (#9627) ... * feat(review): back comment-status and presubmit for Aone Code targets A second `--comment` round on an Aone MR re-posted every still-valid finding as a new comment and never downgraded a self-MR review — both flows were skipped for lack of a1 backing. Route Aone targets at the a1 reads (mr view / mr status / mr comment list / auth whoami) through the same pure classification cores the GitHub path pins, so the report schemas and the Step-7 downgrade semantics stay one contract: parentNoteId threading, closed → resolved, outdated → stale (a rewritten line stays re-postable), no commit anchors (code facts degrade to unknown), and drift with no compare API fails safe. The context-unavailable verdict cap stays until pr-context lands. Closes #9613 * fix(review): harden Aone runners' pr_number guards and null gate payload Address round-1 review findings on the Aone backing of comment-status and presubmit: - extractStatusChecks no longer throws a TypeError when a1 answers a bare null to `mr status`; the payload now reads as the designed unreadable gate state (undefined), capping the verdict like a still-running check instead of crashing presubmit with no report. - comment-status and presubmit validate pr_number with fetch-pr's /^[1-9]\d*$/ grammar before Number() coercion, refusing '012'/'1e3'/ '0x1f'/' 12'/'12.0' tokens that would query a different MR than the caller's label carries. - Pin the two subject_type combinations no test covered (pathless comment WITH outdated:true; the live path+line shape) with mutation-probed assertions. - Align the --host describes with the sibling commands' detection wording (omission no longer promises github.com), name the real bucket (`resolved`) in the review skill's Aone dedup note, and scope the design doc's remaining-unbacked claim to its own section. * test(review): pin the Aone dedup seams the round-2 review named (#9627) Four mutation-verified pins on the existing Aone backing, each closing a round-2 Suggestion: - classifyAoneChecks: the continue-scan cell of aoneCheckState — an unrecognized value in an earlier key beside a recognized verdict in a later key reads the verdict, not pending (a first-present-key mutant now fails) - classifyAoneChecks: a context-keyed FAILED gate carries its name — the passing context-keyed case pinned nothing because passing gates never collect names - both comment mappers: `note` beats `body` when BOTH keys are present (`??` does not coalesce `body: ''`, so an inverted priority would blank every recognition signal and re-post the whole review) - aoneCommentToPresubmitComment: parentNoteId maps onto in_reply_to_id, including the absent-stays-unset half No source changes; each pin fails under its named mutant and passes on the current code. * test(review): pin the five Aone seams the round-3 review named (#9627) * fix(review): align Aone comment reads with measured a1 facts (#9627) * fix(review): read fully-dropped Aone checks array as pending, not all-clear (#9627) * fix(review): match SKILL.md self-PR wording to the revert-guard test The merge resolution reworded the self-PR note to "matched against the 'a1 auth whoami' account", but SKILL.test.ts's revert guard (#9616, #9627) pins the exact phrase "the MR author is matched against 'a1 auth whoami'". Restore the pinned wording (semantics unchanged) so the bundled-skill test passes. * fix(ci): record qwen-autofix.yml's actual size in the workflow ratchet The workflow-size ratchet failed on this PR: qwen-autofix.yml is 397656 bytes but .size-baseline recorded 392111 (5545 over, allowance 4096). The oversize was inherited from main, not introduced here: main's ratchet commit (a5d77eb8, #9677) shrank qwen-autofix.yml to 397656 but set the baseline to 392111 — 5545 bytes below the file's actual size at that very commit. This branch carries main's file unchanged (byte-identical), so its CI is the first to trip the mismatch. Growth is real in the sense that the file genuinely is 397656 bytes; per the ratchet's own guidance ("if the growth is real, bump the number and say why"), record the actual size so the ratchet measures future drift from reality. The Post Coverage Comment failure is downstream of this (the Test job exits before uploading the coverage artifact). * fix(review): keep the pipeline's own pathless Aone summary out of the blocker index (#9627) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-23 04:50:14 +00:00
Shaojin Wen	ec8a8a1a97	feat(review): back pr-context on Aone Code targets (#9621) ... * feat(review): back pr-context on Aone Code targets pr-context was the one read subcommand still gh-direct, so every Aone run was forced context-unavailable: the verdict capped at COMMENT (the wired a1 approval could never fire), Agent 0 skipped, and the machine ledger never recovered from posted summaries. Route it through the platform reader with a normalized context bundle; Aone serves it from mr view + the flat comment list (thread comments carry the ledger), GitHub's implementation is an extraction of the existing calls — its output stays byte-identical. The forced cap leaves the Aone write path for parity with GitHub's state-claim handling, and the refetch commands a context file emits bake --pr on Aone, where comment bodies are addressed per-MR. * fix(review): keep Aone ledger carriers out of the blocker re-check (#9621) On Aone this pipeline's own round summaries are path-less comments, so they ride pr-context's issue channel, where their visible **[Critical]** lines self-promoted every prior Critical-bearing summary into "Blockers to re-check" — rendering each prior Critical three times (beside the ledger section and the inline roots that own the same findings) and spending the section budget on the pipeline's own prose until genuine human blockers degraded to snippets. Exclude bodies carrying the ledger marker from issue-channel promotion and the stdout count, strip the marker out of the settled snippet, and switch the pr_number guard to the canonical isPositivePrNumber so 0x10/5. spellings cannot fragment side-file continuity. Pin the witnesses the round's findings name: the guard, args.host forwarding, the issue-kind --pr refetch branch, the account-first author keying, and the GitHub test suites' independence from the cwd-origin probe. * fix(review): refuse pr_number spellings that do not round-trip (#9621) isPositivePrNumber alone admits two spellings whose Number() value does not round-trip to the raw string: leading zeros (007 fetches 7 but the raw string labels the heading and the prev-ledger side file, so a later 7 run reads a different side file and the round counter restarts) and digit strings above Number.MAX_SAFE_INTEGER (Number() silently rounds them, fetching a different PR than the labels announce). Add the safe-integer and no-leading-zero conjuncts — matching fetch-pr's [1-9]\d* rule — so every admitted input satisfies String(Number(x)) === x. Also pin the witnesses the round-2 review names: the commit_id round-trip through the GitHub reader and toRawReview into the persisted side file (both spreads were unwitnessed), the stale force-applies comment in submit-aone.test.ts the cap removal outdates, and the setup batch's Aone carve-out for the unbacked comment-status call. * docs(review): align Aone docs with the landed no-ancestry anchor rule and comment-status skips D6 described the AGit-Flow anchor as inert until the incremental rule landed, but that rule (#9630) merged while this branch was in flight — anchors now delta-scope Aone re-reviews. SKILL.md's comment-status section and Step 6's report-existence guard now name the Aone skip the setup batch already carries, so no path sends an Aone run at the unbacked command or at a report that was never written. * docs(review): annotate #9616 as landed and define the report-less re-check rule The out-of-scope list still read self-PR detection as open work although #9629 shipped it into this branch's merge base — annotate it like the sibling #9618 entry. Step 6's report-existence guard pointed report-less runs at a re-derivation the skill never defines; replace it with the explicit rule: no per-thread status routing, no hand-derived substitute, rule from the code at the reviewed commit, cannot-tell over a guess. * fix(review): route the context head through aoneHeadSha and close the round-5 findings getReviewContext read sourceBranch raw while every other head read trims — a padded server value diverged the context file from the rest of the run (phantom-drift shape). getCurrentUser now honors the seam contract on the anomalous whoami shapes instead of leaking untagged throws and non-string accounts. Step 6's report-less rule no longer contradicts the comment-status failure contract: runs where the command ran and failed keep the "re-derive if needed" fallback. The Aone paragraph names comment-body among the backed reads, and witness tests pin the identity gate's carriers key and the head normalization. * fix(review): shape-check the Aone comment listing in getReviewContext a1 can answer repo mr comment list with an exit-0 a1.error/v1 error object (backend auth failure or client timeout — measured by cleanup's a1CommentList on the identical payload). Without a guard the object survives the ?? [] coalesce and .filter throws an untagged TypeError, losing the envelope's actionable message at exactly the recoverable moment. Guard as the provider family already does and surface the cause; witness tests pin both envelope shapes (mutant-checked). * test(review): pin getCommentBody's body-field fallback (mutant-checked) * fix(review): union resolved comments into the Aone context bundle The default comment list excludes resolved comments (measured by the cleanup audit) while GitHub's REST fetches include them, so a resolved blocker/marker root never reached the re-check walk or the fail-closed identity gate. Union the default and --resolved listings as the audit does, dedupe by id, fail closed on either listing's error envelope, and disclose the residual that resolved replies stay invisible; witness tests mutant-checked. * fix(review): serve resolved comments and guard the envelope in getCommentBody getCommentBody queried only the default comment list while the context bundle it serves refetches for unions in resolved comments — a resolved id named by a truncation note threw "not found" every time, and an exit-0 a1.error/v1 envelope threw an untagged TypeError that lost the actionable message. Extract the shape-checked default+resolved union helper and read both sites through it; witness tests mutant-checked. * ci: correct qwen-autofix.yml size baseline to its actual post-migration size #9677 shrank qwen-autofix.yml from 431526 to 397656 bytes (prose moved to the design record) but recorded the baseline at 392111, 5545 below the file's own post-change size, so the first PR to run the ratchet tripped it. This branch introduces zero growth to the file (byte-identical to main); the bump aligns the baseline with reality. No workflow content changes. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-23 01:10:32 +00:00
qqqys	98fa2e9770	feat(cli): enable dynamic workflows from a settings key (#9098) ... * feat(cli): enable dynamic workflows from a settings key `ConfigParameters.workflowsEnabled` is declared, defaulted, and read by `Config.isWorkflowsEnabled()` — but `loadCliConfig` never writes it, so no setting has ever reached it. The only way to turn dynamic workflows on is the undocumented `QWEN_CODE_ENABLE_WORKFLOWS=1`, which has to be exported in every shell that launches qwen. AGENTS.md names this shape directly: an optional field that is declared and read but never set by any caller is a dead switch. Add `tools.workflowsEnabled` to the settings schema and populate the field from it. Precedence is unchanged and still resolved in core: `QWEN_CODE_DISABLE_WORKFLOWS` beats everything, then `QWEN_CODE_ENABLE_WORKFLOWS`, then the setting. Because `settings.merged` already folds the System scope, an operator gets a fleet-wide force-off with no extra code. `requiresRestart` is load-bearing rather than decorative: the Workflow tool is registered once while the tool registry is built, `/workflows` is gated when commands load, and keyword steering resolves at startup — so a mid-session toggle would leave the dialog claiming the feature is on while the tool is absent from the registry. The setting description also disambiguates it from the unrelated `experimental.sessionWorkflow` plan-and-review view, which shares the word "workflow" and would otherwise be easy to confuse in the settings dialog. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(cli): clarify workflow feature controls * test(cli): cover workflow command gating * fix(cli): restrict workflow opt-in scope * fix(cli): keep workflow opt-in user-owned * test(cli): cover workflow system scopes * refactor(cli): drive workspace-restricted settings from one list R4-3: the restricted set was hand-maintained in three parallel places — a per-key warning block, the condition in `stripWorkspaceRestrictedSettings`, and that function's destructure. Adding one restricted setting needed three synchronized edits, and either omission is silent: forgetting the warning discards a workspace value with no diagnostic, forgetting the strip honors a value the warning says is ignored. `WORKSPACE_RESTRICTED_SETTINGS` is now the single source, and the warning loop and the strip both derive from it. It lives in `settingsUtils.ts` rather than `settings.ts` because `settings.ts` already value-imports that module — defining it there and importing it back would close a runtime import cycle. R4-2: `tools.workflowsEnabled` is the first setting that is both `showInDialog: true` and stripped from Workspace scope, so the dialog offered a toggle that silently never took effect — it renders from the raw scope file, so it kept showing the value it wrote while the feature stayed at its merged value, leaving a dead entry in the repo's .qwen/settings.json. `getDialogSettingKeys` gained `excludeWorkspaceRestricted`, which the dialog passes when the selected scope is Workspace. The scope comparison stays in the component so settingsUtils keeps its type-only dependency on settings.ts. Unlike `showInDialog: false` (what the two pre-existing restricted settings use), the setting stays visible and editable under the scopes that honor it. Verified: settings 169/169, settingsUtils 85/85, BuiltinCommandLoader 13/13; packages/cli typecheck clean. Mutation-checked both ways — forcing the filter off fails 1 test, dropping a key from the list fails 3. SettingsDialog.test.tsx's 23 failures are pre-existing and environmental: identical counts on upstream/main and on this branch before the change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): reject workspace-restricted settings at the daemon API too R8-1. The workspace restriction stopped at the TUI dialog. `stripWorkspace RestrictedSettings` drops these keys before every merge, so a workspace-scope write through the settings API persists a committable dead entry into the repo's `.qwen/settings.json` and answers 200 + `requiresRestart: true` while the feature never turns on — GET then reports `workspace: true` beside `effective: false`, and the warnings channel carries only `corrupted`, so the client never learns the write was inert. Exactly the trap the SettingsDialog comment in this same PR says it eliminates, one layer over. `tools.workflowsEnabled` is the first workspace-restricted key with `showInDialog: true`, which is what puts it in `getDialogSettingKeys()` and therefore in `getAllowedKeys()` — the two pre-existing restricted keys are `showInDialog: false` and never reached the API. Both POST handlers now call one shared `rejectWorkspaceRestrictedWrite`, answering 400 `workspace_restricted_setting`. One helper rather than two copies, for the reason the previous commit collapsed the warning/strip pair. User scope is untouched — that scope honors the key, and a guard that reached it would kill this PR's whole enablement path. Verified: workspace-settings 22/22, settings 169/169, settingsUtils 85/85. Mutation-checked three ways — dropping either call site fails a test, and widening the guard past workspace scope fails the user-scope test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-23 00:33:51 +00:00
Shaojin Wen	2a99e84169	fix(review): clear the deferred Round-5 findings from the Aone write path (#9604) ... * fix(review): clear the deferred Round-5 findings from the Aone write path The full cleanup of #9579 — the 29 Suggestions deferred from round 5 of the /review bot on #9491 under the ~5-round rule (Criticals-only from that round on). One item (the GH_HOST setGhHost assertions) was already landed with the round-5 Critical fixes; the rest are implemented here. Write-path fixes: - A shaped-but-empty --host refuses with its own shape (host-flag-empty) instead of collapsing to the unbound refusal the flag was the remedy for — the agent re-run loop the refusal wording exists to break. - An invalid host (recorded verbatim or flag-typed) refuses in the exit-3 shape naming the offender and its origin, instead of setGhHost's TypeError escaping runSubmit as a failed command. - A flagless gh post whose nothing-bound routing would inherit an ambient GH_HOST pointing at canonical Aone refuses actionably (ambient-gh-host-aone) instead of failing opaquely after compose ran. - The shared authorisation gate no longer reads an absent host as a github.com claim for callers whose routing follows the recorded binding (submit): the ordinary flagless publish of a GHE-recorded review passes, while publish-assets keeps the strict comparison. - Mid-batch drift disclosure rides the partial-post shape too (headMovedDuringPost on AonePartialPostError, warned from submit's partial branch), and the post-batch re-read is tri-state: a failed re-read leaves headMovedDuringPost undefined and submit discloses "could not re-verify" instead of a false all-clear. - The Aone success JSON surfaces postedCommentIds/summaryCommentId — the audit the partial shape carries and the gh receipt records. Docs and contract fixes: - The context-unavailable cap wording now says what it does (keeps an Approve verdict at Comment; a Request-changes verdict still posts) in the user docs and both SKILL.md sites. - The head-drift bullet is qualified by the per-review restart bound — spent on Aone there is no submit-at-reviewed-SHA fallback; report and leave the rest to the user. - Step 9's Posted: contract admits the no-link note the Aone fallback prescribes. - The --host help text spells both canonical Aone hosts out. - The provider design doc's Phase-3 "refuses" sentence is marked superseded. Test hardening (unfalsifiable pins made falsifiable): - ensureAoneAuthenticated ordered before the writes; setGhHost ordered before the gh write; the a1 path never touches the gh host state. - Live-probe cells for the explicit-flag precedence, the unbound refusal, and the fast-path hostless refusal; the recorded-binding- outranks-probe fixture driven through submit's real gitOpt seam. - submit.test.ts mocks ./lib/git.js (no real git spawned in the vitest cwd), isolates the cross-session suite's recording store via chdir, and pins the newest-wins ordering when two recordings of one PR carry different hosts. - Producer-side 'refusing to post:' prefix pins, the RC-Note count source pin, the contextUnavailable:true gh-path pin, and the floor recovery's callerHost pin. * fix(review): address round-1 findings on the Aone write path (#9604) * fix(review): address round-2 findings on the Aone write path (#9604) Extract one refuse helper for submit's seven exit-3 refusal shapes (sibling publish-assets precedent), align the Aone pre-write refusal prefix with the other refusal paths, and pin the invalid-host remedy of the flag/origin arms positively — the recorded arm's absence pin alone let a ternary-collapse mutant ship green. * fix(review): address round-3 findings on the Aone write path (#9604) Make submit's exit-3 refusal terminal: refuse now throws a SubmitRefusal that runSubmit's single catch renders into the refusal shape (stderr line, posted:false JSON, exit 3), so a gate that says no cannot fall through toward the write — the helper previously returned and relied on every call site adding its own `return;`. Also extract the post-batch MR-head re-read, duplicated between submitAoneReview's partial-post and success paths, into one helper. * fix(review): address round-4 findings on the Aone write path (#9604) * fix(review): address round-5 findings on the Aone write path (#9604) * fix(review): address round-6 findings on the Aone write path (#9604) * fix(review): resolve merge-conflict residue in the review skill (#9604) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-22 13:27:51 +00:00
Shaojin Wen	0c36e5093a	feat(review): close Aone residual gaps — composeUrl, test-plan routing, a1 version floor (#9624) ... * feat(review): close Aone residual gaps — composeUrl, test-plan routing, a1 version floor The three residuals #9619 tracks together, one pass: - composeUrl joins the platform reader: GitHub composes the PR-page URL from the routed host (deterministic grammar, no API call); Aone is reader-backed — the platform's own detailUrl, never assembled, since the nested-group owner/repo collapse can name a different repo. submit fills a receipt that carries no url through it on both platforms, so the skill's prose fallback shrinks to the coordinates relay for the one case the reader cannot serve. - test-plan's body fetch routes through the platform reader: the MR description on Aone (already in the reader's fetch metadata — no new API surface), so the Test Plan check runs on Aone targets instead of going unchecked on every run. - ensureAoneAuthenticated enforces the a1 version floor design-doc Q1 asked about — 0.1.90, the version the platform facts were probed against — in presence → floor → auth order, each with its own remedy message; unreadable versions are disclosed on stderr and fail open. Verified: ~590 targeted unit tests, tsc/eslint/prettier clean, build + bundle green, and a CLI smoke that refuses a fake stale a1 at the floor while a fake fresh one passes the gate. * fix(review): apply round-2 review on the Aone residuals All six round-2 suggestions on #9624, probe-verified and pinned: - R1-1: the version-probe fail-open now discloses the CAUSE — the extraction mirrors the whoami catch (first non-empty line past the execFileSync preamble), so segfault / unsupported flag / permission failures stay distinguishable instead of one constant preamble line. - R1-2: aoneReader.composeUrl discloses a failed lookup on stderr before degrading to '' — every other fail-open in the provider discloses, and the coordinates-relay case must stay distinguishable from an environment fault. - R1-3: one home for the PR-page host spelling — normalizeGhHostForUrl in lib/gh.ts, shared by compose-review's comment anchors and the reader's composeUrl, so a `--host GHE.Corp:443` run can no longer print two textual spellings of the same PR page; non-default ports survive. - R1-4: submit no longer re-queries the reader when the Aone receipt carries no webUrl — detailUrl is a stable MR attribute and the pre-write drift-gate read already carried it, so the second fetch could only block on the flaky state that lost the field. The empty receipt rides the coordinates relay; the reader keeps composeUrl as the canonical seam. - R1-5: the Aone body-fetch route runs the same ensureAuthenticated gate every other a1-backed flow runs first — a standalone test-plan on a missing/stale/logged-out a1 now fails exit 1 with the install/upgrade/login message instead of exit 0 with the generic note. The GitHub arm keeps its historical degrade. - R1-6: the handler wiring (the Aone fix's integration point) is pinned by handler-level tests — an Aone --host must route the body through the reader with the gate first, and a refused gate fails the command before any fetch. SKILL.md's Posted paragraph, its revert-guard pins, and the design-doc bullet follow the R1-4 semantics. Verified: tsc/eslint/prettier clean, 759 targeted cli tests + 23 SKILL guards green. * fix(review): apply round-3 review on the Aone residuals * fix(review): apply round-4 review on the Aone residuals * fix(review): fail closed on unknowable host in composed receipt link (#9624) * fix(review): route explicit GHE-family hosts to the GitHub reader Platform detection selected Aone on ANY *.alibaba-inc.com host for explicit --host/--remote signals, but a family host that is not the canonical pair (ghe.alibaba-inc.com is the live example) is a GitHub Enterprise instance — such a review authenticated against a1 and read an unrelated same-numbered Aone MR instead of the GitHub PR body (and test-plan additionally gated on a1 auth first). Explicit signals now select Aone only via the canonical pair (code./gitlab. alibaba-inc.com) — the same canonical-only rule the write gate has always applied — while the family predicate survives on the no-explicit-signal cwd-origin fallback. parse-args stops refusing /pull/ URLs on GHE-family hosts (the same predicate misapplied: those are real GHE PR URLs), and the five --host describe texts now name the canonical pair. Pins: registry.test.ts flips the GHE explicit-host expectation to github, adds the explicit-remote and canonical-port arms, and keeps the family cwd fallback; detection-side 592 + write-side 322 tests green. * fix(review): fail closed on family-only /codereview/ URLs AONE_CR_URL_RE captures the whole *.alibaba-inc.com family (shape-first grammar), but a family-only host is a GHE instance that serves no /codereview/ page: accepting its URL as a live target would let detection route the explicit GHE host to GitHub and aim fetch/submit at GHE PR #<id> — a target the supplied URL never named as a valid GHE resource. The classifier now gates the aoneMatch branch on isAoneCanonicalHost, so non-canonical /codereview/ inputs stay invalid-url, mirroring the /pull/-on-canonical-Aone refusal. The mirror arm is pinned too: a /pull/ URL on a family-only host parses as the real GHE PR target it is. --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-22 12:55:25 +00:00
Shaojin Wen	f1d05b79fc	feat(review): detect self-MR on Aone targets in presubmit (#9629) ... * feat(review): detect self-MR on Aone targets in presubmit The self-PR verdict downgrade existed only for GitHub targets: the Aone read path skipped presubmit entirely, so a review of one's own MR silently carried the weight of an independent review (#9616) — exactly the wrong direction for the most common local Aone flow, re-reviewing one's own CR before the next amend. presubmit now routes by platform. On an Aone target it compares the authenticated account (a1 auth whoami) against the MR author from one mr view fetch — case-insensitive, fail-soft on a deleted author, fail-closed on an unreadable MR — and emits the same report shape with the unbacked slices neutral (CI classification and comment dedup have no Aone backing yet). The same fetch backs head drift via sourceBranch, and a malformed pr_number/owner_repo stays a usage error rather than a metadata blip. SKILL.md runs presubmit on Aone targets instead of skipping it, and the "self-PR detection has no Aone backing" caveat is gone from the skill and the user docs. * fix(review): unify Aone live-head reads and the presubmit whoami gate The round-1 review of the Aone presubmit found four seams the new path had hand-derived a second time; each is now stated once: - The self-PR comparison (including the load-bearing `author !== ''` guard) existed as two inline copies in presubmit.ts; isSelfReview states it once for both platform paths so a future normalization rule cannot diverge one platform silently. - "An Aone MR's live head is mr view's sourceBranch" was hand-derived in five places in aone.ts, two of them untrimmed: a padded server value then manufactured a phantom "PR head advanced during review" (and a submit-time refusal) against the trimmed reads, for an MR that never moved. aoneHeadSha states the fact once; all five sites route through it, repairing the two untrimmed copies. - The "same report shape as GitHub" invariant was convention only; both presubmit result literals are now typed against one PresubmitReport interface, so a field added to the envelope is a compile error on the path that forgets it instead of a silent toBool(false) at the consumer. - The Aone path spawned `a1 auth whoami` twice per run (plain gate + JSON account read). The gate now runs the JSON whoami once and returns the account: one spawn per run, and no account fetch remains after the MR fetch that could throw uncaught and orphan the graceful metaUnavailable report — the fail-closed path pays no a1 work after a thrown mr view. The padded-head regression cells for getPrMeta/getFetchMeta fail on the pre-round code; the empty-guard, single-spawn, and report-shape witnesses each fail under mutation probes. 4143 review tests green. * test(review): pin Aone presubmit auth-gate throw path (#9629) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-21 14:15:27 +00:00
Shaojin Wen	04886c4354	fix(review): make the incremental cache work for Aone AGit-Flow CRs (#9630) ... * fix(review): make the incremental cache work for Aone AGit-Flow CRs * docs(review): qualify the Aone no-ancestry claims in comments and docs The D7 comments described the ancestry gate as unconditional and both ancestry tests as failing for every AGit-Flow update; the head test alone fails for every amend (the clamp fires only on amend-plus-rebase), and the narrowing join never lets a drift byte reach the published scope. Qualify the ledger.ts SHA_RE block, the resolveIncrementalAnchor docstring, the clamp-skip and call-site comments, the test-block comments, and the design/user docs accordingly. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-21 11:06:59 +00:00
Shaojin Wen	0dd518f950	feat(review): disclose that Aone posts join the discussion gate only (#9625) ... * feat(review): disclose that Aone posts join the discussion gate only Aone has a dedicated ai_comment merge gate for AI-posted review comments. A controlled probe on a scratch CR (issue #9614) resolved the design doc's open question Q4: `a1 repo mr comment create` does NOT auto-set isAiComment for the posting identity (a general and an inline probe both read back false, re-checked against an async classifier), and a1 v0.1.90 exposes no flag to request it — so qwen-posted comments sit in the generic discussion gate only, and the ai_comment gate never tracks them. The same probe re-confirmed Q3: still no native reject/request-changes on the a1 mr surface. Until a1 ships a marking flag (feature request to the a1 CLI), the write path discloses the gate split instead of silently implying participation: the Aone REQUEST_CHANGES note names the posted comments as unflagged and the discussion gate as the only mechanical block, and SKILL.md / the user docs carry the same fact. createMrComment documents the constraint and is named as the seam where a future marking flag wires. * test(review): pin the directional ai_comment gate claim in the Aone disclosure note Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(review): bind the Aone gate-disclosure pins to content, call, and count source --------- 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>	2026-08-21 08:17:23 +00:00
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	2026-08-21 07:24:20 +00:00
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.	2026-08-21 07:24:05 +00:00
Shaojin Wen	5715782279	feat(review): post --comment reviews to Aone Code via the a1 CLI (#9491) ... * 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.	2026-08-20 16:27:16 +00:00
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>	2026-08-20 07:50:44 +00:00
qqqys	36c77ff803	fix(dingtalk): attach media from quoted messages (#9347) ... * fix(dingtalk): attach media from quoted messages * fix(dingtalk): keep the reply text when attaching quoted media R1-1: `attachMedia`'s placeholder cleanup was written for the direct-media path, where `extractContent` generates `(audio)` / `(video)` / `(file: name)` itself. This PR made the quoted-media path reach it, and there `envelope.text` is the user's own reply — so a reply reading exactly like one of those placeholders was blanked and the agent got an attachment with no prompt. A group `@Bot (audio)` arrives here as exactly `(audio)`, the mention having been stripped upstream. `attachMedia` now takes the placeholder to erase as a parameter; only the direct-media call site passes one. R1-2: the same path newly routes text-only replies through the unguarded `mkdirSync`/`writeFileSync`/`basename` block. Those are synchronous throw sites — ENOSPC on a write of up to 50 MB, ENAMETOOLONG from a quoted fileName over 255 bytes (`basename` does not truncate), a TypeError from a truthy non-string fileName. An escape rejects `processMessage`, whose catch sends the generic error reply and never calls `handleInbound`; the msgId is already in `seenMessages`, so DingTalk's retry is deduped and the prompt is lost for good. The block now degrades the way a failed download already does: log, skip the attachment, deliver the text. This also covers the pre-existing direct-media path. Verified: dingtalk 310/310. Both mutation-checked — restoring the caller-blind cleanup fails 3 tests, letting the fs block throw fails 1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(dingtalk): address quoted-media review findings Unify the msgType->mediaType mapping in a shared helper, make extractContent the single source of truth for the placeholder text cleaned on attach, and remove the store directory when a media write fails so failed stores no longer leak into tmpdir. Merge the stacked attachMedia JSDoc blocks, document quoted media downloads, and pin the previously uncovered paths: unmapped quoted msgTypes with a downloadCode, own-media + quoted-media combinations, direct placeholder cleaning, and the degraded-store attachment shapes. * fix(dingtalk): file-back a quoted image colliding with the own image * fix(dingtalk): give generated media store names a mime-derived extension (#9347) --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot[bot]@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-20 03:18:32 +00:00
Shaojin Wen	1eb8a0c7f8	feat(review): wire --resume through /review and the review run subcommand (#9153) ... Surface the local resume feature (PR #9092) on the paths a user reaches it from: - `parse-args.ts`: `/review <pr> --resume` parses to `resume: { requested, effective }`, gated on PR targets (a local review's diff comes from a live working tree with no stable interrupted state). A `--resume` on a non-PR target warns and is inert. - `run.ts`: the `qwen review run` headless wrapper takes `--resume` and passes it through to the `/review` prompt. - `SKILL.md` Step 1 gains a "Resuming an interrupted run" branch: on `resume.effective`, append `--resume` to `fetch-pr`, branch on its `resumed` JSON, run `recover-findings`, re-enter the audit loop at `latestReverseAuditRound + 1`, and read the restart bound back from `restartsSpent`. - `DESIGN.md` / `docs`: document resume as a LOCAL convenience. The CI review workflow runs FRESH — it does not pass `--resume`. A CI attempt runs no-sandbox on the reviewed PR's own code and its worktree is deleted the moment it exits, so there is no interrupted state on disk for a retry to continue; a resume would refuse `worktree-gone` and start over anyway. The retry loop and its test assert the fresh-only wiring.	2026-08-19 05:13:09 +00:00
Shaojin Wen	081a96d864	feat(cli): plain-prose /review comments; severity markers follow review.attribution (#9027) ... * feat(cli): Add review settings for attribution, default effort, and default comment * fix(cli): resolve review settings from operator scopes and close gate gaps (#8994) Address review feedback on the review settings: - Resolve review.attribution/effort/comment from operator-controlled settings scopes only (system defaults, user, system); a repository's .qwen/settings.json is content under review and must not control whether findings publish, whether the review names its model, or how deeply the pipeline verifies. - Normalize the configured review.effort through the same case- insensitive validation as the --effort flag, so "Low" cannot miss the exact comparisons the forcings run and invalid values cannot leak into the verdict. - Gate the modelId requirement and footer-safety validation on attribution: with the footer gated off, the field has no consumer and must not refuse the run. - Pass the standing review.comment setting into publish-assets' call of the shared authorisation gate, so both callers agree on what authorises a run. - Make presubmit's self-comment detection footer-independent by also matching the reviewing account's own top-level comments, so attribution-off posts still dedup. - Align SKILL.md's Step 7 gate and every --comment branch on comment.effective, and add handler-level wiring tests for all configured defaults. * test(cli): pin the review-settings operator defaults with unit tests (#8994) * fix(cli): share the guarded footer strip and pin the gate audit text (#8994) * fix(cli): raise the repository-context array bound to 256 (#8994) * fix(cli): validate review setting values and tighten the review gates (#8994) * feat(cli): drop the AI template tells from unattributed /review posts review.attribution: false already drops the footer; the posted text still read as machine output. With attribution off, inline comments now post without the **[Critical]**/**[Suggestion]** prefixes and are written as plain reviewer prose, the review body loses its fixed template markers (LGTM! ✅, the ⚠️ glyph, the **[Critical]** bullets in body lists), and the Step 1 verdict carries the attribution flag so the orchestrator can pick its register. The severity strip happens in the final post object only — counting, the unmarked gate, and the ledger all still run on the marked payload, so verdict semantics are unchanged and the default mode is byte-identical to before. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): align presubmit dedup with severityOf and normalize auto effort (#8994) * feat(cli): make plain prose the only register for posted /review text The template voice is worse prose for every audience, not just the unattributed one, so the phrasing now goes plain unconditionally: comment bodies drop the '— Failure scenario: trigger → outcome' label and arrow notation (the evidence rule is unchanged — trigger and wrong outcome must be in the sentences), and the fixed review-body copy loses LGTM! ✅ and the ⚠️ glyph in both modes. What still follows review.attribution is the machine-readable layer — the severity prefixes and the footer — because qwen-autofix.yml's Critical-only mode greps posted bodies for the literal **[Critical]** marker. With prose unconditional there is no register to branch on, so the parse-args verdict's attribution field goes away again; submit keeps stripping prefix and footer at post time when the operator turned attribution off. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): keep the copy humans actually write — restore LGTM! and the warning glyph Readability, not concealment, is the criterion: reviewers type LGTM! and reach for the ⚠️ glyph on a warning line every day, and both aid scanning. The earlier commits stripped them along with the real scaffolding, which overshot. What goes is only the labelled failure-scenario template; the fixed review-body copy is now byte-identical to before in both attribution modes, and the pr-context LGTM filter needs no change after all. * fix(cli): close the review-found gaps in the unattributed post path CI review on the PR found three real defects and four test/doc gaps in the first two commits; all addressed: - presubmit dedup went blind to attribution-off posts: the authorship fallback gated on severityOf, but submit strips exactly that prefix, so a later round re-posted its own findings as duplicates. Attribution-off comments now carry an invisible <!-- qwen-review --> marker and presubmit matches on it — from any account, which also closes 8994's documented other-accounts gap. - The attribution-off body-Critical branch quoted entries verbatim, leaking a model-written **[Critical]** marker into the posted body; it now strips like the inline path does. - The SKILL.md payload example still showed the labelled template the rewritten body-format paragraph forbids; both examples now show plain prose. - A comment that is nothing but its severity marker is refused at the consistency gate instead of posting the bare marker. - Forged footers followed by text survived the trailing-anchored strip and were the only attribution an unattributed post carried; the off leg now strips footer-shaped lines regardless of position. - The setting's description now names both stripping consequences (autofix Critical-only deferral; the invisible marker), and the loosened clean-approve test assertions are exact again. * test(cli): pin the reply guard with a finding-shaped reply fixture The unmarked reply body was excluded by the severityOf gate even with the reply guard deleted, so nothing pinned the guard itself (mutation-verified by CI review). * fix(cli): close the round-3 review findings on the unattributed post path Five Criticals and twelve Suggestions from the third CI review round, all addressed: - presubmit dedup: the invisible-marker branch was ungated — any account could plant the public marker string on a line expecting a blocker and have the next round silently withhold it. The branch now requires authorship by the reviewing account plus the exact trailing shape submit posts; adversarial and quote-reply fixtures pin both guards. The 'other accounts escape dedup' disclosure from 8994 stands again. - The marker-only gate was dead under attribution ON (the canonical footer was appended before the check) and stacked markers bypassed it: the strip is now iterative, delegates the classification to severityOf, and the gate refuses when the footer-and-marker-stripped remainder is empty or still marker-led. - bodyCriticals and cannot-tell entries now get the forged-footer strip on the unattributed leg (a surviving mid-entry footer was the post's only attribution), the cannot-tell parse trims before matching, and ledger titles strip the marker (the ledger rides the body as an HTML comment the autofix grep reads). - stripForgedFooterLines rewritten line-based: closing underscore optional (looping-model truncation), CRLF tolerated, 400-char line bound, fence- and indented-code aware, and byte-identical when nothing matches. - The comment marker now carries severity (<!-- qwen-review critical -->); pr-context's blocker promotion reads it, so an unresolved unattributed Critical re-enters the re-check section every round — including past the ledger's horizon. - Tests: stripForgedFooterLines unit coverage, grouped cannot-tell strip, ledger leg under a prNumber plan, and the adversarial presubmit shapes; loosened assertions re-tightened. Docs and the settings description now match the shipped behavior. * fix(cli): close the round-4 review findings — marker read/write hardening Seven Criticals and four Suggestions from the fourth CI review round: - commentMarkerSeverity now reads only the trailing posted shape, and submit strips pre-existing bare marker lines before appending the canonical marker — a marker string quoted or planted in a reviewed file can no longer choose the severity the classifiers see. - The marker disjunct in the blocker classification is gated on the reviewing account, via one shared predicate (isBlockerBody) now used by BOTH pr-context and comment-status — an empty planted 'critical' comment no longer becomes a permanent irrefutable blocker, and the two consumers can no longer diverge on the posted shape. - The ledger's drafted-comments leg strips like the bodyCriticals leg (iterative markers, forged footer lines first, footer spans off the title), and stripSeverityPrefix now strips to empty for marker-only bodies — the submit gate refuses exactly that shape, in both modes. - The fence scanner is a faithful model now: ~~~ fences count, a fence opener indented 4+ spaces does not open one, and lines inside a simple HTML block never toggle fence state. - Producer/consumer roundtrip tests pin the marker shape (the drift class the module header exists to prevent); the iterative strip, the attribution-on marker-only gate, and the strip order each carry the assertion the mutations showed missing. * fix(cli): make the unattributed strip a fixpoint, closing the round-5 escapes Seven Criticals from the fifth CI review round, all probe-verified escape hatches in the strip chain, closed by restructuring it: - One shared stripForUnattributedPost iterated to a fixpoint now serves every attribution-off leg (submit's post transform and gate, compose's body lists, both ledger legs), so the sites cannot drift on order: forged footer lines, severity prefixes (leading AND paragraph-initial, via a new fence-aware stripParagraphMarkers), bare marker lines, and footer spans interleave arbitrarily in a looping draft and only the fixpoint posts none of them. - The marker-only gate runs the full chain: a prefix over a bare marker line no longer posts an empty visible comment carrying a live marker. - Marker-only body Criticals and cannot-tell entries are refused at compose (both modes), mirroring submit's gate — an empty-stripped entry no longer counts toward REQUEST_CHANGES while rendering nothing. - The version-parens truncation (the natural mid-character cut) is admitted by all three footer regexes; blockquoted forged footer lines strip; HTML blocks stop shielding footer lines (their content renders visibly) while still not toggling fence state. - The design doc's definitional line now says what ships: no VISIBLE attribution — the machine contract moves to the invisible severity marker. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): drop the dead bare-marker arm and tolerate whitespace before the colon - submit's post transform no longer references COMMENT_MARKER: the unmarked gate runs first, so every body reaching the transform has a known severity and posts the severity-carrying marker unconditionally. - stripSeverityPrefix tolerates whitespace before the colon after a marker and treats a whitespace-only remainder as empty (a trailing newline no longer survives as a phantom non-empty body). * fix(cli): close the round-6 findings — faithful fences, render-nothing gates Seven Criticals from the sixth CI review round: - mapLinesAware now applies the map inside HTML blocks (the round-5 fix updated the comment but not the code — the shield stood), tracks the opening fence by delimiter character and run length with no info string on the closer (CommonMark), and the chain's final span pass is line-aware so fenced quotations survive the full strip. - The emptiness gates (submit + both compose lists) project through a new rendersAsNothing — Cf characters, HTML comments, hollowed fences, and forged-footer residue are not content — and run the full post-transform chain, so a scaffolded-but-invisible comment can no longer post, count toward REQUEST_CHANGES, and re-promote as an unanswerable blocker. - stripCommentMarkerLines admits blockquoted marker lines, matching its sibling regexes. - buildLedger keeps the carried id and title when the finding text starts on the line after the severity marker (trimStart before titleOf) — a regression from routing titles through the new chain. * fix(cli): close the round-7 findings — faithful quotes, code spans, render-nothing classes * fix(cli): close the round-8 findings — one displayed projection for every strip * fix(cli): close the round-9 findings — fail-closed identity, one shape per leg * fix(cli): close the round-10 findings — rendered-text signals, bounded spans, fence-safe entries - blocker prose scan reads only rendered text: an HTML comment renders as nothing, so a planted `<!-- [critical] -->` can no longer promote an invisible, irrefutable blocker through the ungated channel - the footer-span version group admits only the version shape footerVersion validates — a span truncated inside the parens can no longer swallow the prose after it - the marker strips admit the full-width colon, closing the marker-only refusal's ASCII-only hole in bilingual drafts - entries containing a code-fence delimiter line are refused for redraft: the one-line collapse turns them into an unclosed fence that swallows the rest of the posted body - the identity fail-closed trigger narrows to what identity actually gates — critical markers on root comments — so a planted reply cannot convert a transient identity blip into a repeating refusal - the ledger's carried-id anchor reads through render-nothing residue left between the marker and the id, ending the silent renumbering - the marker-only contract covers trailing Cf/comment residue * fix(cli): close the round-12 Criticals — bounded footer version, drop-scoped blank cleanup, quote-preserving markers, gate-matched post leg * fix(cli): close seven review sanitation entrances from round-20 review (#9027) - CR-aware line model: scanLines and rendersAsNothing split on CR/CRLF; entry lists normalize line endings on ingest, so a bare CR can no longer hide a forged footer, a hollow fence, or a fence delimiter from the refusal and emptiness gates (R20-1) - empty-login identity lookups fail closed like thrown ones in both pr-context and comment-status while a critical marker is posted (R20-2) - whitespace-only body-list entries fail the renders-nothing gates instead of vanishing before them; the dead raw sha check drops (R20-4) - drop-collapse never touches blank runs around an HTML-block content drop — quotation blanks render and survive (R20-6) - attribution-off posts refuse drafts whose post-strip shape leaves a fence open at the appended invisible marker (R20-9) - carriedClaimLine slices on the classifier's projection and both colon widths; presubmit reads carried ids off the attribution-off posted shape (R18-1) - duplicates disclosure routes through the attribution-off fixpoint chain like every other body leg (R15-1) --------- Co-authored-by: qwen-code-autofix[bot] <qwen-code-autofix[bot]@users.noreply.github.com> 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>	2026-08-18 14:47:42 +00:00
Shaojin Wen	da26cffc36	feat(review): Aone Code read path (second review-platform provider) (#9226) ... * docs(design): /review Aone Code read path (Phase 2) * feat(review): Aone Code read path (Phase 2) Adds an Aone Code provider so /review can review a MaxCompute CR locally. The read path works end to end against a real odps_src CR (verified E2E): fetch-pr fetches `refs/merge-requests/<global-id>/head` and builds the worktree + diff (stats computed locally, Aone advertises none); meta / issue-context / fetch-diff resolve identity, Aone workitem evidence, and the diff via the a1 CLI. The four reader-backed subcommands and fetch-pr select the provider from the clone's remote (or an Aone host), so a GitHub clone is unchanged. Read-only this phase: pr-context / comment-status / presubmit have no Aone backing yet (the run degrades to context-unavailable), and --comment is refused on an Aone target. SKILL.md + code-review.md document the Aone target and the degradations. See docs/design/2026-08-15-review-aone-provider.md. * fix(review): address PR #9226 round-1 Aone review findings Critical: - match-remote: Aone CR URLs use the WEB host (code.alibaba-inc.com) while a clone's remote uses the GIT host (gitlab.alibaba-inc.com) — treat them as one equivalence class (hostsEquivalent) so a codereview URL matches its clone's remote and the worktree flow is reachable; nested-group remotes (group/subgroup/project) now collapse to the last two segments instead of failing to match - registry: an explicit non-Aone host/remote now beats the cwd probe, so an explicitly-GitHub subcommand run from an Aone clone is not hijacked to Aone; hint host is trimmed; the four reader-backed subcommands thread --host into detection (previously dropped); dropped the unwired --platform dead switch - aone parseRemoteUrl: user-less scp remotes (ssh-config/insteadOf), nested groups, a trailing slash after .git, and empty segments now parse; the parse-failure message redacts a user:token@ origin (no credential leak) - aone getCommentBody throws on a missing id (was an indistinguishable empty string); fetchDiff uses gitRaw (512 MiB buffer, no CRLF rewrite, latin1) instead of git() (1 MiB ENOBUFS, CJK byte loss) Suggestions: - aone-client: 120 s timeout, ENOENT branch in the auth check ("install the a1 CLI"), TRANSIENT_RE anchored to HTTP 5xx (bare 502/503/504 misfired on command lines containing those digits), one stderr trace line per retry - fetch-pr: validate pr_number before Number() coercion (1e3 fetched PR 1000), trim --host before detection; submit guard trims --host; comment-body --pr help notes the Aone per-MR requirement - parse-args: nested-group codereview URL grammar; invalid-url warning names both grammars - SKILL.md + code-review.md: Aone paragraph corrected (clone-origin trigger, Agent 0 skipped, test-plan/publish-assets unbacked, pass --host), design doc updated (detection, Agent 0 gating) - tests: aone.test.ts, registry cwd-mock + precedence + parseRemoteUrl cases, remote-match hostsEquivalent + nested collapse, parse-args nested codereview, submit-aone refusal * fix(review): address PR #9226 round-2 Aone review findings Critical: - parse-args: the Aone CR URL grammar is now constrained to Aone hosts (*.alibaba-inc.com) — a /codereview/ URL on any other host hits the fail-closed invalid-url refusal instead of becoming a live PR target (unlike …/pull/<n>, which any GHE host legitimately serves) - aone fetchDiff: merge-bases against a fetched target branch (not a present-but-stale origin/<target>), and the MR-head refspec is now force-fetched (+) so a stale throwaway ref from an interrupted run does not fail the fetch when the head was rewritten (normal AGit-Flow iteration) Suggestions: - fetch-pr: countDiffChangedLines now delegates to the single hunk-state walker in computeDiffStats (the two could not disagree silently); the changedFiles count is pinned on `diff --git` via a binary-file fixture - aone parseRemoteUrl scheme case made explicit + pinned (RFC 3986) - submit.test.ts pins the platform registry to GitHub so the Aone refusal guard neither spawns a real git in the vitest cwd nor couples to the machine's clone origin * fix(review): address PR #9226 round-3 Aone review findings Critical: - registry: hostOfRemoteUrl now makes `user@` optional in the scp branch (user-less scp remotes from ssh-config/insteadOf no longer misroute an Aone clone to GitHub); the token-bearing scp userinfo parses to the host, not an owner - parse-args: the Aone CR-URL host group now requires a REAL subdomain dot boundary (`(?:[A-Za-z0-9-]+\.)+alibaba-inc.com`), so lookalikes (`evilalibaba-inc.com`) hit the fail-closed refusal; a `/pull/<n>` URL on an Aone host is refused too (Aone serves no /pull/ pages) - meta: on a non-GitHub platform an explicit `--repo` without `--host` is refused (no default host off GitHub) instead of emitting the contradictory `platform:aone` + `host:github.com` - aone fetchDiff: spreads PINNED_DIFF_CONFIG/PINNED_DIFF_FLAGS (an un-pinned color.diff=always zeroes computeDiffStats), discloses a failed target-branch fetch via a stderr WARNING, and refuses to diff from a clone of a different repo; scp-form userinfo is redacted in the parse-failure message Suggestions handled: - comment-body: the Aone per-MR `--pr` requirement is enforced before the auth gate (usage errors precede auth) - aone-client: the auth-failure diagnostic surfaces a1's real first stderr line (not the execFileSync preamble) and reports a timeout/kill distinctly - remote-match docstring + registry precedence comment updated to the implemented behavior Deferred to follow-up #9194: the 16 test-gap patterns, headRefOid dead-field removal, MAX_SAFE_INTEGER digit guard, and the refusal-message host branch. * fix(review): address PR #9226 round-4 Aone review findings Critical: - aone fetchDiff + fetch-pr merge-base fetch: the server-controlled target/base branch reached `git fetch` bare — a dash-leading branch name (creatable by full-refname push) parses as an option, so `--upload-pack=<payload>` executed attacker-named code with the reviewer's credentials. Pass `--` to end option parsing and refuse dash-leading values outright on both providers - meta: the no-default-host guard now gates on the FLAG, not the resolved value — a GH_HOST export no longer bypasses it (and an empty-string --host counts as missing); the whole --repo branch's pure resolution moves above the auth gate (usage errors precede auth) - skill: pass --host for EVERY pr-url target including github.com — an omitted hint falls back to the cwd origin probe, which hijacked a github.com review run from an Aone clone (and vice versa); lightweight fetch-diff/pr-context carry the host too - submit: the Aone refusal moves BELOW the authorisation gate and takes the exit-3 + {"posted": false} shape instead of throwing — an unauthorised Aone run now ends as the skill's contract defines, and detection reads the effective host (flag → GH_HOST), so an Aone-pointing GH_HOST export is refused instead of dying opaque inside gh Suggestions handled: - parseRemoteUrl: strip query/fragment (credential channel into repo identity), fix the cleaning order for two-plus trailing slashes after .git, and discard an explicit port instead of folding it into the path - registry: isAoneHost normalizes the trailing-dot FQDN spelling; the cwd probe delegates to lib/git's gitOpt (shared git policy) - aone: the MR-head refspec is stated once (mrHeadRefSpec); resolveRepo quotes git's real error line, not the execFileSync preamble - aone-client: the auth fall-through message is neutral (covers non-auth failures the login hint cannot fix) - fetch-pr: pr_number guard tightened to ^[1-9]\d*$ (no PR zero, no leading zeros, no side effects before the refusal) - the five detection-consuming subcommands' --host describes now state the implemented semantics; SKILL.md/code-review.md read-only phrasing corrected and the false "detection reads the clone's remote, not the URL" claim fixed Tests: dash-leading refusal on both providers, meta guard flip tests (GH_HOST bypass, empty flag, pre-auth), submit exit-3 shape (authorised, unauthorised, padded host, GH_HOST), trailing-dot and /pull/-on-Aone parse refusals, port/query/slash parse cases, fetch-pr zero-number and base-ref refusals. Deferred to follow-up #9194: the single-branch merge-base disclosure (R3-9), cleanup audit skip-in-code (R3-13), URL-form --remote hint (R3-19), publish-assets refusal parity (R3-22), and the data-path deadline translation (R3-25). * chore(review): re-push to re-link PR head after branch recreation * test(review): repin SKILL.md host-rule wording in SKILL.test.ts The round-4 fix rewrote the skill's --host notes (pass --host for every pr-url target, github.com included); three revert-guard tests pinned the old 'add --host <host> for Enterprise' phrasing and reddened the core suite in CI. Repin them at the new wording. * fix(review): address PR #9226 round-5 Critical findings - aone resolveRepo: redactUrl now strips the query/fragment channel too — a ?private_token=… origin carries no @ for the userinfo redaction, so the parse-refusal message echoed the secret the success path strips (test pins the refusal message secret-free) - aone fetchDiff: the merge-base fallback (base = ref~1) DISCLOSES via a stderr WARNING — previously silent, a multi-commit MR got only its last commit served as the complete diff (shallow/single-branch clones hit this; the GitHub path is loud about the same class) - submit: the Aone write-refusal binds the platform in BOTH directions — the authorisation gate now surfaces the recorded target's host, so a recorded Aone host refuses whatever the runtime-effective host resolves to (an ambient GH_HOST export can no longer steer an Aone review into posting at a same-named repo), while a recorded non-Aone pr-url binding is no longer vetoed by the cwd probe from an Aone-origin clone Tests: refusal-message redaction, fallback disclosure (spy calls captured before mockRestore — vitest's restore clears them), bidirectional refusal arms (recorded-Aone + GHE env refuses; recorded-github + Aone cwd posts). * fix(review): address PR #9226 round-6 Critical findings - authorization: the --user-authorized fast path now surfaces the recorded target's host too (best-effort read of the recorded args) — it returned before the args file was read, so recordedHost was always undefined on that path and the 'a recorded Aone host always refuses' invariant leaked: a user-authorised post of a recorded Aone codereview review from a non-Aone cwd with no --host/GH_HOST posted at github.com's same-named repo. Tests pin the fast-path host through the REAL gate and the end-to-end refusal (the witness scenario) - aone: the query/fragment strip now uses [\s\S]* in both redactUrl and parseRemoteUrl — git stores newline-bearing remote URLs, and a plain . stopped at the first \n, letting ?private_token=SECRET\nx smuggle the token past the strip into the parse-refusal message. Tests cover both the parse-success and refusal paths of the smuggle Round-6 is Critical-only per the ~5-round policy (user-confirmed for convergence); the 13 Suggestions are deferred to follow-up #9194. * fix(review): address PR #9226 round-7 Critical findings - fetchDiff's throwaway ref now carries a pid suffix — two concurrent runs for the same MR in one clone shared the name: one session's finally- delete killed the other mid-review (unknown revision), and a pre-existing local branch of the reserved name was force-moved then deleted, reflog and all (race probe: 12/60 failures → 0 with the per-run unique name) - the target/base-ref guards close the refspec channel the dash-only check left open after `--`: a leading `+` parses as a force refspec (fetches the wrong head — stale evidence, no WARNING) and a colon as src:dst (force-moves the throwaway ref or a reviewer-local branch). Both providers now refuse '-', '+', and ':' shapes (probe-confirmed on real fetchDiff incl. the served-wrong-diff and local-branch-overwrite witnesses); tests pin the new channels on both guards - redactUrl and parseRemoteUrl clean userinfo BEFORE the query/fragment strip: a userinfo that itself contains '?' or '#' was truncated mid-credential, leaking the username+secret prefix into the refusal message and making parseable origins unparseable (flip-verified on the witness shapes) Round-7 is Critical-only per the convergence directive; the 8 Suggestions (incl. the 4 bot findings) are deferred to #9194. * fix(review): address PR #9226 round-8 Critical findings - the server-controlled branch-name guards now validate ALLOWLIST-style on both providers (aone.fetchDiff's target, fetch-pr's baseRefName): the denylist admitted HEAD (silent fetch + merge-base through the stale clone-time symref), rev-parse metasyntax (wrong base under a misdescribing warning), ranges, and the empty string (garbled diff-less fallback) — a plain-branch-name shape closes every channel - parseRemoteUrl/redactUrl consume userinfo GREEDILY up to the last @ of the authority — multi-@ and :-/-bearing token userinfo no longer leaks cleartext residue through the refusal messages or folds into the parsed host (take() fails closed on any surviving @); the scp strip admits only a removal that leaves a host: shape behind - fetch-pr's Aone stats backfill moves AFTER the plan/rescue, where diffText is final — the partition-rescue republishing the full range no longer leaves delta-scoped numbers beside a full-range diffPath — and isCollapsedFromUpstream is skipped when the stats are locally derived (one source, not two: the disclosure needs an independent advertised fact, and a delta-scoped round beside the full-range count fired a false collapse) - remote identity is injective again: Aone nested-group targets carry the full group path (parse-args → match-remote --group-path → matchRemotes compares every segment when both sides have three or more), and fetchDiff's origin guard adds the origin's host (Aone family) — a same-named repo in another group or on another platform can no longer pass either gate; SKILL.md passes --group-path for nested targets - meta's discovery branch drops GH_HOST inheritance off GitHub — an ambient GHE export beside an Aone-origin clone no longer vetoes the valid invocation at HOSTNAME_RE; only an explicit --host steers routing Round-8 is Critical-only per the convergence directive; all five findings fixed, no deferrals this round. * fix(review): address PR #9226 round-9 Critical findings - redactUrl is fail-closed BY CONSTRUCTION: split at the last @, redact everything before it — the per-regex redaction kept missing shapes (round-9: URL userinfo with a / in the secret, scp userinfo with a newline, residues with no host: shape all leaked verbatim through the parse-refusal message) - parseRemoteUrl cleans per form and fails CLOSED: URL-form userinfo is bounded to the authority (greedy within it — multi-@ and ?/# inside secrets consumed whole, /-bearing secrets left to fail closed in take), scheme inputs never fall through to the scp grammar (a malformed https://user:pa/ss no longer parses host user); the round-8 scp-strip firing on scheme URLs fabricated coordinates from query-borne and path-borne @ witnesses — all witnesses now parse correctly or refuse - registry hostOfRemoteUrl consumes token-bearing userinfo (':' AND '/' in the secret) on both branches, mirroring aone.parseRemoteUrl — detection no longer parses the credential prefix as the host and misroutes Aone clones to GitHub; detectPlatformKind ranks an explicit --host above the remote-URL hint in BOTH directions (an Aone origin can no longer hijack an explicitly-GitHub invocation into fetching a global MR id from the wrong remote) - nested-group identity is injective in both directions: matchRemotes compares the full group path exactly whenever the target carries one (any length — a 3+-segment target no longer matches a two-segment remote sharing its tail, nor the reverse); Aone CR targets carry the path even at two segments and the canonicalized URL keeps the full path; fetchDiff's origin guard compares the origin's full path against the MR's own detailUrl path (authoritative repo identity, where the seam's ownerRepo is collapsed); the rescue pool keys on the full path and same-id cross-group CR URLs are refused as ambiguous Round-9 is Critical-only per the convergence directive; the 8 Suggestions (R8-6..R8-13) are deferred to follow-up #9194. * fix(review): address PR #9226 round-10 Critical findings - aone.fetchDiff's host arm keys on the CANONICAL Aone-family predicate (new remote-match isAoneHostFamily: port/trailing-dot/case normalized; registry.isAoneHost now delegates to it) — a trailing-dot FQDN clone that detection accepts as Aone can no longer be refused by the diff gate with a misdirecting remedy - the URL cleaning/redaction class is closed structurally, not per shape (sixth consecutive round a new entrance was found): parseRemoteUrl's URL-form userinfo is consumed whole WITHIN the authority (span between // and the first /), and the scp-form userinfo strip + its lookahead are bounded at ?/# — an @ inside a query or fragment value is the credential's own character and can no longer fabricate coordinates from the query tail; redactUrl fails the DISPLAY closed with a constant when the last @ sits after a ?/# marker — the token tail can no longer reach the refusal message (URL/scp/fragment witnesses all pinned) - isPlainBranchName rejects git's pseudo-ref set (FETCH_HEAD/ORIG_HEAD/ MERGE_HEAD/…) on both guards — FETCH_HEAD resolves to the just-fetched PR head (empty diff beside full-range metadata), ORIG_HEAD to an arbitrary ancestor; both shape-legal, both silently wrong - fetch-pr's merge-base probe requires the fetch to have produced the tracking ref — a tag-only baseRefName exits 0 writing only FETCH_HEAD, and the bare-name fallback once merge-based against the reviewer's local tag with baseFetchFailed falsely false; the tag shape now lands in the disclosed state - parse-args: the repo-qualified CR URL outranks a same-number bare spelling as the target in BOTH the rescue pool and positional order — the bare number carries no host, and letting it win flipped detection onto the cwd fallback (a loud refusal at the merge base had degraded to a silent wrong-platform retarget); bare restatements of the URL target are skipped silently, matching the rescue loop's restatement handling Round-10 is Critical-only per the convergence directive (the bot's own ledger is at its round cap); the 6 convergence-posture deferrals named in the review body join follow-up #9194. * fix(review): address PR #9226 round-11 Critical findings - the URL cleaning/redaction surface is closed STRUCTURALLY: one parser, one source of truth — registry.hostOfRemoteUrl now delegates to the canonical aone.parseRemoteUrl (detection and the identity parser can no longer disagree), and the scp branch reads GIT'S OWN grammar (GIT_TRACE-probed: hostinfo ends at the FIRST ':', userinfo carries no ':' or '/') — the last-'@' consumption once parsed a different host than git connects to, letting fetchDiff's same-repo guard pass while git fetched from another server; token-bearing scp shapes now fail closed, and the round-8 detection tests are re-blessed onto shapes git reads that way - the pseudo-ref allowlist is CASE-INSENSITIVE on both twins: on case-insensitive filesystems (macOS/Windows defaults) fetch_head folds onto FETCH_HEAD, resolving the merge-base to the just-fetched MR head (empty diff beside full-range metadata); lowercase spellings refused, pinned - submit.test.ts's file-level setup now saves/clears/restores GH_HOST — the Aone refusal reads the ambient env, and the org's standard intranet export pattern (an Aone-family host) turned 50 of 69 posting tests into refusals - the --user-authorized fast path binds the recorded host to THIS write (same-PR number only — a stale recording of another PR must not supply a host) and scans SIBLING session recordings when the session-scoped args file is absent — the characteristic cross-session publish shape otherwise lost the host and posted a recorded Aone review at github.com's same-named repo (real-gate witness: exit 0, COMMENT filed); tests drive the real gate through a sibling-session fixture - the tracking-ref requirement and both merge-base sites are FULLY QUALIFIED (refs/remotes/…): git resolves unqualified origin/<name> in refs/tags and refs/heads first, so a tag or branch literally named origin/<baseRefName> — a PUSHABLE, server-controlled refname a plain clone auto-carries — shadowed the just-fetched tracking ref and moved the merge base with no disclosure; shadow-tag tests pinned on the resolveMergeBase probe, the fetch-pr seam, and aone.fetchDiff Round-11 is Critical-only per the convergence directive; the bot's own ledger is at its round cap and this round still produced findings — recommend freezing the bot loop and moving to human security review. * fix(review): address PR #9226 round-12 Critical findings - the recorded-args host lookup is HARDENED — the store lives under .qwen/tmp/ beside review worktrees checked out from the PR's own tree, so its content is attacker-influenceable: only s-* session directories are scanned (a malicious PR can no longer plant a root-level args file that binds a host), symlinks are skipped at both the directory and file levels (mirroring writeSkillArgs' O_NOFOLLOW write-side policy), reads are size-bounded, and the host binds only when the recording names the same PR number AND the same repo - the canonical Aone invocation shape (bare global MR id, no URL) can no longer post cross-session without host evidence: a same-number recording with no host binds the recorded --host flag when present (parse-args now records it), and without one the write gate FAILS CLOSED with the exit-3 shape and names the remedy — instead of posting the review at github.com's same-named repo (the probe-verified witness once exited 0 and POSTed) - parse-args: the URL-outranks-bare-number invariant now holds for MIXED shapes — a positional bare number restating the rescue pool's single PR is carved out of hasValidCandidate, so --effort <cr-url> 7 (and both orderings/equals-form) target the CR URL instead of silently retargeting onto the cwd clone's same-number PR; a different number still outranks - aoneReader.resolveRepo refuses an origin outside the Aone host family — an explicit --host can steer detection onto this reader while the cwd clone is a GitHub mirror (the dual-remote migration setup), which once emitted {platform:'aone', host:'github.com'} and queried a1 with the mirror's coordinates; same predicate fetchDiff's origin guard applies - isPlainBranchName (both twins) rejects refs/-prefixed names: legal branch names (check-ref-format --branch) that resolve qualified refs the server controls as fetch/merge-base arguments (refs/remotes/origin/ HEAD is the clone's default-branch symref — wrong base, misdescribing WARNING) - the ref-dwim class is closed at the verified sites: fetch-pr's base probe fetches an EXPLICIT branch refspec (bare names dwim onto same-named tags — exit 0, tracking ref untouched, stale base passing the freshness guard it never refreshed), its fetchedSha/merge-base head reads are refs/heads-qualified (a planted same-name tag can no longer shadow the real head), and aone.fetchDiff's target fetch + merge-base + diff-range reads are qualified the same way Round-12 is Critical-only per the convergence directive (the bot's own ledger is past its round cap).	2026-08-18 09:19:52 +00:00
Shaojin Wen	30366b5faa	fix(review): gate the recovered incremental anchor on the model that certified it (#9184) ... * 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. * 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): 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): 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): 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. * 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): 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. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-18 07:43:40 +00:00
易良	18c9763f46	feat(cli): add /advisor command for second-opinion conversation review (#7567) ... * feat(cli): add /advisor command for second-opinion conversation review Adds a manual /advisor slash command that asks a reviewer model for an independent second opinion on the current conversation. The review runs as a read-only forked side query sharing the main conversation context (runForkedAgent cache path, NO_TOOLS), so the main session is never mutated. A new advisorModel setting selects a dedicated reviewer model, falling back to the main model when unset. Refs #6542 * fix(cli): translate advisor command description * fix(cli): address review feedback for /advisor command (#7567) - Fix prompt to acknowledge transcript may be truncated (F1) - Add empty history guard so fresh sessions get a clean error (F2) - Add getModel() guard consistent with /btw - Trim advisorModel to reject whitespace-only values - Add cross-provider disclosure to advisorModel description - Add ADVISOR_MAX_FOCUS_LENGTH constant - Add i18n entries for advisor-specific strings (en/zh/zh-TW) - Strengthen tests: section headings, abortSignal forwarding, empty history, whitespace model, no-override assertion * fix(cli): refine /advisor abort handling, cache sharing, and docs (#7567) * fix(cli): address /advisor review feedback — docs wording, i18n, test coverage (#7567) * fix(cli): address /advisor review feedback — rendering, tools, guard (#7567) Render the advisor review as a boxed markdown block (new MessageType.ADVISOR + AdvisorMessage) instead of a flat INFO line, so the four fixed sections display as real headings. Always strip tools on the forked query (matching /btw and the "no tools" prompt) rather than declaring them on the default path. Tighten the busy guard to /recap's (isIdleRef + pendingItem) and return an error message instead of addItem. Surface the resolved reviewer model in the header (ForkedQueryResult.model) so a mistyped advisorModel that falls back to the main model is visible. Move buildAdvisorPrompt and the input limit to core advisor-utils for reuse, skip session recording like /btw, and document the blocking-vs-/btw UX difference. * test(cli): cover advisor inline code fences * test(cli): cover advisor model setting schema * fix(cli): address /advisor review feedback — docs accuracy, shared fence normalizer, test pins (#7567) * test(cli): register advisor i18n keys as must-translate (#7567) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address /advisor review feedback — fence-aware normalizer, i18n keys, test pins (#7567) * fix(cli): repair advisor test type cast that broke tsc build (#7567) * fix(cli): address /advisor review feedback — parser-mirroring fence tracker, test pins (#7567) * fix(cli): address round-4 /advisor review — fence tracker parser parity, raw-mode math gate (#7567) * fix(cli): address round-5 /advisor review — normalizer corruption guards, wiring test pins (#7567) * fix(cli): address advisor review gaps * fix(cli): harden advisor reviews * fix(cli): preserve ACP slash command cancellation * fix(acp): preserve slash command lifecycle * test(acp): verify advisor output without recording * fix(cli): scope advisor ACP behavior * fix(cli): require structured advisor output * docs(acp): correct slash result support * fix(cli): honor advisor context and JSON mode * fix(cli): preserve sibling ACP slash command cancellation * fix(cli): use shared recording skip set * fix(cli): close advisor ACP telemetry gaps * fix(cli): keep advisor JSON paths compatible * fix(cli): close advisor telemetry compile gaps * fix: resolve advisor transcript gate + response_format endpoint compat R18-6: classify the /advisor recording gate by the resolved command's kind instead of the raw input string, so user-defined commands shadowing the name still record their prompt while the built-in /advisor stays out of the transcript. handleSlashCommand now returns the resolved command's name+kind alongside every result. R19-1: gate buildResponseFormat on the official OpenAI endpoint, matching the prompt-caching precedent; third-party OpenAI-compatible endpoints (DeepSeek, older vLLM, validating gateways) reject the unknown response_format field and this pipeline never sent it before. * fix: key advisor recording skips on command identity + pin fallbacks R18-10: the TUI recording-skip gate now matches the built-in /advisor by kind+name instead of the bare name in SLASH_COMMANDS_SKIP_RECORDING, so a user-defined command shadowing the name is recorded like any other custom command. Regression test covers the FILE-kind shadow. R18-3: pin the normalizeOpenAIStrictSchema -> json_object fallback with the goalJudge-shaped partial-required schema and a typeless property. R18-4: assert logConversationFinishedEvent fires on the fully-handled non-advisor ACP slash-command path in the existing /btw test. * fix(cli): record /clear user-turn before the session switch R20-9: /clear (and its session-switching aliases) swaps in a fresh recorder inside its action, so its user-turn record must land before the action runs. Restore pre-resolution recording for every slash command except /advisor, which alone defers to after resolution so a user-defined command shadowing the built-in name keeps its record (R18-6). --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Code Bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-17 08:54:18 +00:00
qqqys	2bbaafbf2f	feat(core): let a workflow agent pin a directory and outlive the default bounds (#8972) ... * feat(core): let a workflow agent pin a directory and outlive the default bounds Three gaps that together keep workflow subagents to short, in-place work. **`agent({workingDir})`.** A script had no way to run an agent inside a directory. `isolation: 'worktree'` is not a substitute: it CREATES a worktree from the current tree and refuses to run when the parent tree is dirty — the opposite of pinning an agent to a directory whose uncommitted state is the point (a review worktree, a checkout a previous step provisioned). `workingDir` is the same contract `AgentTool` already exposes as `working_dir`: an existing, caller-owned worktree that the harness neither creates nor removes. Two details are easy to get wrong and both are covered: - The fast path hands `config` to `AgentHeadless` untouched and cannot honour a rebind, so `workingDir` forces the override path. Left on the fast path it would be dropped in silence and the agent would run in the parent tree — the failure the option exists to prevent. - `canonicalizeAgentOpts` now projects `workingDir`. The same prompt run against two worktrees is two different questions; without the projection a resume that changed only the directory would replay the previous tree's answers as this one's. The validation moves to `agents/worktree-pin.ts`, shared with `AgentTool` rather than duplicated: the path comes from a model either way, and pinning replaces the child's `WorkspaceContext` wholesale, so it must resolve inside the repository and be a registered linked worktree. The caller passes the parameter name so errors say `workingDir` to a script and `working_dir` to a tool call. **Tunable per-subagent bounds.** `max_turns: 50` / `max_time_minutes: 10` were hard-pinned at both dispatch sites with no override, while the three other workflow bounds all have one. A build-and-test agent, or an analysis of a 2 000-line file, exceeds them routinely — and under the GOAL-terminal contract being cut off surfaces as a `null` element, an agent that silently went missing rather than one that visibly failed. Both are now env-tunable and clamped, and the doc comment states how `stallMs`, `max_time_minutes` and the run wall clock differ, since raising one without the others just moves which limit kills the run. **Headless regression test.** A foreground `Workflow` call must complete with no interactive session and no completion channel: `qwen --prompt` has no TUI, no approval bridge and a closed stdin, so anything reaching for interactivity inside the tool or runner would hang on a prompt nobody can answer. The background half was already refused explicitly; this pins the foreground half. Part of #8769. * fix(workflows): harden agent worktree pinning * fix(workflows): anchor worktree-pin containment at the main working tree Address the remaining review findings on the agent workingDir pin. Containment anchored at `--show-toplevel`, which from inside a linked worktree answers with the worktree's own root — spuriously refusing registered sibling worktrees, the documented review-pipeline setup. Resolve the repository's main working tree via the first entry of `git worktree list --porcelain` (new GitWorktreeService helper) and anchor there, keeping the toplevel answer as fallback. Add dispatch-site wiring tests for the env-tunable subagent bounds at both the fast and the override path: with a clean env the DEFAULT_* constants and the resolvers are indistinguishable, so a revert mutation at either call site kept every existing test green. Cover the fs.realpath half of the containment guard with a real symlink fixture (plain-string stubs made both realpath calls reject, so the canonical-path logic never executed), and the sibling-anchor fix with a unit case. Correct the bounds doc comment: only QWEN_CODE_MAX_WORKFLOW_AGENTS and the subagent bounds clamp to a ceiling; the stall and wall-clock env overrides apply valid values verbatim. * fix(workflows): refuse truncated main-tree anchors in worktree pinning (#8972) A main working tree whose path contains a newline splits the porcelain first entry of `git worktree list`, and the truncated prefix could resolve inside a different repository — re-anchoring the pin's containment and registration checks against that repo's worktree registry. Detect the malformed first record (a path remainder where a record attribute belongs) and fall back to `--show-toplevel`, whose single-value answer keeps interior newlines intact. Also pins down round-2 review findings: direct unit and real-git coverage for `getMainWorktreePath()` (whose semantics were only exercised through a stub), the symmetric journal-key HIT direction for `workingDir` resumes, and the model-facing `workingDir` eligibility description (the main checkout is not a valid pin target even though it appears in `git worktree list`). * fix(workflows): harden worktree pinning per round-3 review (#8972) - Round-trip-validate the porcelain main-tree anchor (git-common-dir must agree) so attribute-shaped or trailing-newline truncations cannot aim the pin gate at a different repository's worktree registry - Preserve legitimate path whitespace when parsing the anchor and the --show-toplevel fallback (terminator-only strip, untrimmed raw output) - Thread one canonical realpath through both pin gates and the rebind so a re-pointed symlink cannot land the child where neither gate looked - Canonicalise both containment sides or neither, so an absent target reaches the registration gate's accurate message instead of a manufactured outside-the-repository refusal - Name the degraded anchor in the containment refusal when the main working tree could not be determined - Throw on agent({workingDir, isolation}) at the orchestrator entrance (revived plain object — not evadable by the sandbox getter trick) - Trim-based blank check for workingDir at both workflow entrances - Document stallMs in the workflow schema and extend the tool-level capability enumeration to workingDir and stallMs * fix(workflows): type worktree-pin test stub as nullable per service (#8972) The round-3 degraded-anchor test passes null to the getMainWorktreePath mock, but the vi.hoisted stub inferred Promise<string> from its default implementation while GitWorktreeService.getMainWorktreePath() returns Promise<string | null> — tsc --build failed on the clean rebuild of packages/core. Annotate the stub with the real service signature. * fix(workflows): harden stallMs gate and test portability per round-4 review (#8972) - Reject non-numeric agent({stallMs}) loudly in the sandbox gate instead of silently dropping it to the default 60s watchdog, which contradicted the advertised "0 disables the watchdog". - Compare worktree paths via path.resolve/path.normalize in the new tests so the windows-latest unit lane stops failing on separator differences. - Correct the model-facing workingDir/stallMs descriptions to match the registry-only gate and the first-progress-event arming semantics. - Restore the Agent tool's historical "a sub-agent" refusal wording. * test(webui): deflake same-session refresh transcript assertion (#8972) The failing CI annotation named keeps-the-attachment-live (load variant): the live agent_message_chunk was asserted after a single-macrotask flush, but under runner contention the batched setTimeout(0) dispatch can land one tick after that window, so the chunk reads as missing even though it is delivered. Replace the fixed-depth read with a bound-wait (vi.waitFor) for the exact same blocks, preserving the assertion. Verified with 10 repeated full-file runs under load (all failing before the fix, all passing after). * docs: note the workflow workingDir opt is stricter than working_dir (#8972) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): sanitize workingDir echo in workflow refusal errors (#8972) The workingDir refusal throw interpolated the model-authored path through JSON.stringify only, which escapes C0 but leaves DEL and the C1 range (incl. NEL U+0085) raw in the error message. Route the echo through sanitizeForErrorMessage like the sibling agentType path, and add a regression test that fails without the fix. Also address R7 review suggestions: name the QWEN_CODE_WORKFLOW_STALL_SECONDS override in the stallMs description to match every other env knob there, anchor the new workingDir/stallMs prose in the description regression test, and make the bounded runConfig test hermetic against the two new env knobs. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): name the subagent turn/time env caps in the workflow tool description (#8972) * fix(core): preserve trailing CR in git worktree path answers (#8972) --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.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>	2026-08-17 08:34:03 +00:00
qqqys	a1e046eb6c	feat(core): add a live-session registry and qwen sessions ps (#8969) ... * feat(core): add a live-session registry and `qwen sessions ps` Records each interactive session at `~/.qwen/sessions/<pid>.json` while it runs, so "which Qwen Code sessions are on this machine right now" is one readdir instead of a walk over every project's transcript directory. This is the discovery surface cross-session messaging needs (QwenLM/qwen-code#8724), landed on its own because it is useful by itself and changes nothing about how a session behaves. Why not extend the existing runtime.json sidecar: it lives under `<projectDir>/chats/<sessionId>.runtime.json` and is never unlinked, so presence carries no liveness signal. The cost of asking it this question is visible in `isSessionRuntimeActive` — ~150 lines of candidate-directory guessing plus a recursive scan, and that only answers whether one *known* session is alive. Enumerating every live session that way is that cost times N. The two coexist: runtime.json stays the kimi-compatible "which session is PID X serving" sidecar for external observers. Staleness is decided by PID liveness plus a start identity of `<boot_id>:<starttime>` read from /proc, so neither a recycled PID nor a reboot can resurrect a dead session's record. `session-writer-lease.ts` composes the same Linux identity and is deliberately left alone — its token is a persisted format with takeover semantics. The new `process-liveness` helpers replace the private copy in teamHelpers. Registry hygiene worth calling out, each one a real failure mode rather than defensive habit: the directory is chmod 0700 on every register (mkdir's mode is umask-masked and does nothing for an existing directory); records are 0600 and written `noFollow`, so a pre-planted symlink cannot redirect a registration write; only `<digits>.json` is ever considered a record, because a lenient prefix match would read `2026-planning-notes.json` as PID 2026 and delete a file this code never wrote; and a record that fails validation is skipped without being swept, since we cannot reason about what we cannot parse. `qwen sessions ps` prints the live sessions; `--json` emits JSON Lines. Record fields come from other processes, so the table renders them through `sanitizeTerminalText` — ANSI, control bytes, and bidi overrides (CVE-2021-42572 class) all matter when DIRECTORY is the column a user relies on to tell two sessions apart. Registration happens after first paint: nothing on screen depends on it, and it is an mkdir plus an fsync'd write. `/clear` and `/resume` patch the record's session id, and a directory switch patches its cwd, but never its name — that name is the handle a user just read out of `ps`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(core): harden the session registry per review feedback (#8969) * fix(core): guard session registry identities per machine and boot (#8969) * fix(core): harden session registry identity guards under boot-id and schema outages (#8969) Boot-id unreadability now degrades the write paths (register/patch/ unregister) to accepting only tokenless records and never disables the reader-side cross-machine guard, so a foreign machine's live record can no longer be overwritten, merged into, unlinked or swept during an outage. readRecord discriminates newer-schema records from torn files and the write paths refuse them like foreign-identity records instead of treating them as unowned. Registration on Linux retries the start token once and refuses rather than writing an impersonable tokenless record. The /cd refresh queues the sidecar write and the registry patch as separate entries so a sidecar failure cannot skip the patch, and patch/ unregister reuse the record path captured at registration so a relative QWEN_HOME resolving against a moved cwd keeps working. deriveSessionName NFC-normalizes, keeps combining marks, and truncates by code point. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): keep the review-context manifest under the resolved-file bound (#8969) The committed-manifest tripwire failed once main's coordinate skill landed: the every-rule-co-matched relatedPaths expansion grew from 128 to 129 resolved files, one past the wire bound. Narrow the core-skills rule's relatedPaths to the infrastructure files directly under packages/core/src/skills/ — bundled skill content is self-contained, arrives in the diff itself when it changes, and grows with every new bundled skill, so leaving bundled/** in the glob would spend the bound's headroom on each addition. All fail-closed bounds stay pinned. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): close session registry race and outage gaps per round-4 review (#8969) Refuse registration when the Linux PID-namespace id stays unreadable (the record would be unreclaimable litter poisoning its PID slot), retry once like the start token. Treat non-ENOENT read failures as intact-foreign ("read-error") in register/unregister instead of unowned. Require a readable matching start token before patch merges. Re-read a record before the sweep unlinks it so a registration winning the window is not deleted. Tolerate ENOSYS/ENOTSUP on the registry-dir chmod. Move registry patches off the sidecar write chain onto their own never-awaited chain so a rejecting or hanging sidecar write can neither skip a patch nor hang /cd on the HOME write. * fix(core): serialize session registry lifecycle * test(cli): update session registry mocks --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> 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> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-17 03:53:12 +00:00
Shaojin Wen	7091b8c761	fix(review): lock the PR review worktree lease against concurrent sessions (#9211) ... * fix(review): lock the PR review worktree lease against concurrent sessions The /review worktree lives at a fixed path per PR number, and the lease recording its owning session was only consulted by the end-of-session crash sweep. A second session reviewing or finishing the same PR deleted the first session's worktree, branch, and side files mid-run (#9205). Make the lease double as a lock: fetch-pr refuses with an actionable error before touching anything when another session holds it, and cleanup skips the whole target with a note. Ownership is per session, so drift restarts and later rounds of a multi-prompt review are not locked out. A missing worktree now fails repo-context with a re-run-fetch-pr message instead of a bare ENOENT. * fix(review): roll back the lease on fetch-pr failures and scope the missing-worktree remedy Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): shield live leases from the cleanup sweep and roll back on any fetch-pr failure * fix(review): re-check the lease after cleanup's audit to close a TOCTOU (#9211) - Re-read the lease after the network-bound bypass audit and before any destructive step, so a session that acquires the lease during the audit is skipped, not destroyed (R2-10). - Narrow the cleanup lease-skip guard to the real lease shape so a target named 'lease' still has its own side files swept (R2-1). - Make the fetch-pr lease rollback best-effort via tryRemove so an un-removable lease file cannot mask the original failure (R2-5). - Pin the lease-lock wiring and success/rollback invariants in tests (R2-2, R2-7, R2-8, R2-11). * fix(review): validate fetch-pr's number and release leases off side-file residue (#9211) The lease gate only engaged `pr-\d+` targets while cleanStale destroyed worktreePath(prNumber) for any input, so a malformed number bypassed the lock and deleted a live holder's worktree; refuse non-positive-integer pr_number before the gate like the sibling commands. Cleanup now releases the lease once the worktree and branch steps succeed instead of holding it on an un-deletable side file, which wedged every later review of the PR. The lease-file grammar is one shared predicate (isReviewLeaseFile) across the writer, the sweep guard, and the finalizer scan, and the lease tests pin the arguments and ordering the mocks previously left blind. * fix(review): acquire review leases atomically and fail closed on identity (#9211) Close the round-5/6 lease-lock findings: - Create the lease with `flag: 'wx'` so two concurrent fetch-prs that both pass the gate's read cannot clobber each other's lease; on EEXIST, same-session re-fetch rewrites, a foreign holder refuses (R6-1). - Roll the lease back on failure only when this run created it, and compare ownership before deleting so a re-fetch keeps the session's live lease and a lease acquired during a stuck run survives (R6-2). - Refuse fetch-pr before any state when QWEN_CODE_SESSION_ID / QWEN_CODE_PROMPT_ID are absent instead of running lease-less (R6-3). - Register the lease inside the rollback try (R6-5). - Track the platform separator in the lease assertion (R6-4) and gate the POSIX-only ENOTDIR test off Windows (R5-1). - Pin the `Number(prNumber) <= 0` validation disjunct (R5-2) and arm the side-file sweep in the lease-skip test (R4-3). --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-autofix[bot] <qwen-code-autofix[bot]@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-17 00:48:00 +00:00
Shaojin Wen	2610f6ed6d	feat(review): apply the huge round reduction only when the run has a clock (#9203) ... * feat(review): let an operator lower the reverse-audit round cap The round cap is chosen from the diff topology, which is right for the cost question it answers but leaves no way for someone paying for reviews to say "spend less on this loop". `review.reverseAuditRounds` is that knob: a standing operator policy that lowers whichever tier applies. It can only lower, and the asymmetry is the point rather than caution about configuration. A single operator-chosen count is exactly what tiering removed — a round is one agent on a small diff and about ninety minutes on a huge one, so one number is wrong for at least one topology, and most wrong for the one whose cap exists to stop six-hour reviews that post nothing. Lowering carries no matching hazard: it can only end the loop sooner. The floor stays at the huge tier's three, for the reason the plan reader already refuses one and two — a cap below that pre-empts the two-consecutive-dry rule and buys a capped verdict rather than a cheaper review. The two things an operator means by "let it run longer" both have direct expressions that a round count only approximates: a ceiling longer than the huge tier assumes is a deadline, which the admission gate already prices a round against, and "keep going while it is still finding real defects" is a property of the findings rather than of a number chosen before the review starts. It is a setting, not a flag, and it resolves in the capture command rather than at the gate — so it lands in the plan and every reader sees one number without learning a setting was involved. That satisfies the module's standing rule that a budget the caller passes is a budget the caller can inflate, rather than making an exception to it. The reader needs no new code: a lowered value is inside the tier's band, which the existing clamp already honours. Operator scopes only, like the other review policy settings — a repository must not choose how deeply the pipeline verifies it, which the new tests pin by putting the setting in a workspace file and asserting it does nothing. Two things fall out of adding a fourth caller, both fixed here: - The plan builder now takes the ceiling as a REQUIRED parameter. Three capture commands build a plan, an optional one is a parameter a call site can quietly omit, and a setting that applies to two of the three review entry points is worse than one that applies to none. Passing undefined is how a caller says "no ceiling", visibly. - Reading the settings can throw: loading raises a fatal error when any settings file fails to parse, and this is now read while the diff is being captured — the review's first step. A stray comma in a file none of these settings had to come from would have ended the whole review. It degrades to the defaults and discloses instead, and every default is the conservative side: attribution on, no auto-posting, no effort or round-cap override. * chore(review): regenerate the settings JSON Schema for the new review setting The schema file is generated from the settings schema and checked in; adding a setting without regenerating it fails the CI check that keeps the two in step. No behaviour change — the file is derived output. * fix(review): stop the settings degrade from being killed by its own announcement The try/catch added here exists so a corrupt or unreadable settings file cannot end a review: loading throws a fatal error when any settings file fails to parse, and this is read while the diff is being captured, the review's first step. It then announced the degrade through the *throwing* stderr helper, so the announcement could end the review the degrade was written to save. Both halves are reachable together, and the second is ordinary rather than exotic: `process.stderr.write` throws on EPIPE or a closed fd — whenever the reader goes away (`qwen … | head`) or a daemon redirects its stderr — which is why the safe sibling exists and says so in its own docstring. With a broken settings file and no reader, the throw propagates out of the catch and all three capture commands crash before writing a plan. Switched to the safe writer, with the reason recorded at the call site. The test that covers it needed the mock repaired first, because the mock was hiding the bug: it mapped both writers to one non-throwing spy, which makes the throwing and safe helpers interchangeable and mocks away the entire distinction the degrade depends on. The safe one is now a spy that swallows what the underlying write throws, matching the real contract — so a test can make the write fail and see which helper the code chose. Reverting to the throwing helper turns the new test red; before this, it turned nothing red. * fix(review): correct two rationales that contradict the code they sit beside Both are claims about mechanism, both false, and both contradicted by documentation already in the same file — which is what makes them worth fixing past the round where only correctness fixes land: in this codebase the comment is the design record, and a wrong one outlives the round that shipped it. **Why a cap of one or two is refused** was stated in four places as "it forces a non-converged stop where two-consecutive-dry would have converged on its own". That is false for two: the convergence check runs before the cap gate, so an all-dry loop reaches CONVERGED under any cap of two or more — which the huge-tier constant's own docstring, two functions away, already said. The reasons are real but different for each value. One refuses the convergence pair's second member, so the loop can never produce the two dry audits convergence is defined by. Two lets an all-dry loop converge but leaves no round for a loop that reports anything, so the first finding makes the stop non-converged. Both end in a capped verdict rather than a cheaper review; only the mechanism was wrong. **When loading settings throws** was described as "any settings file fails to read or parse", with a stray comma as the example. Malformed JSON is the one case that does not throw: it is copied aside and recovered, under a comment that says "Never crash due to a corrupted settings file". The throw comes from a file that cannot be read, which is enough on its own to justify the degrade — this is read while the plan is being captured — so the correction narrows the claim without weakening the reason for the guard. * feat(review): apply the huge round reduction only when the run has a clock Three is the one tier lower than the topology beneath it, and read as a statement about auditing it is backwards. A huge diff has more defects and more territory than a chunked one, converges later, and on recall deserves more rounds rather than fewer — the standing counterexample is a 5,801-line PR that took eight review rounds and was still surfacing Criticals in code present since its first commit. It was never a statement about auditing. It is a statement about a wall: a reverse-audit round on a 4,000-line PR is about ninety minutes, five of them are 450, and a six-hour CI ceiling does not hold that plus the fan-out and the tail. The survey behind it measured absent reviews rather than slow ones — twenty-six timed-out jobs in one window, about 122 hours of compute, nothing posted. Three rounds reported beat five rounds lost. That argument is sound exactly where the wall is. A local run exports no review deadline, nothing kills it at six hours, and the reduction there trades recall away to fit a ceiling that does not exist — on the tier where recall matters most, and by a number calibrated against somebody else's CI. So the reduction now applies only when the run has a deadline at all. With a clock a huge diff caps at three, as before; without one it is simply a large chunked diff and caps at five. The clock is read where the settings ceiling is read — in the capture command, passed into the budget, recorded in the plan — so the two facts the cap depends on arrive the same way and the budget module keeps its property of touching neither the environment nor the settings. Both now travel as one context object rather than a growing parameter list, still required at the plan builder so a capture command cannot silently omit them. The admission gate asks the same question through the same parse the deadline gates already use, so "has a deadline" and "a deadline will be enforced" cannot come apart. All four capture/gate clock combinations are safe and covered: a plan captured without a clock records five and is honoured at five; read later under a clock its band closes to three and it is cut to three, which is the conservative direction when a wall turns out to exist after all. Two things this does not pretend to fix, both recorded in the design note. The deadline gate falls back to a flat thirty-minute round estimate until it has measured one, so on exactly the runs that time out it under-prices the first two rounds threefold and cannot refuse them — that, not the round count, is why a static reduction was needed on top of a working gate, and a size-aware first estimate is what would retire the reduction entirely. And chunk retirement can only begin at round three, so under a three-round cap only one round can ever shrink: the arithmetic that justifies the cap is an arithmetic the cap guarantees stays true. * fix(review): correct the claims this stack got wrong, and cover its untested seams Round-4 review of the three stacked changes, all of it comment-and-coverage rather than behaviour. Grouped by what was actually wrong. **A doc comment detached from what it documents.** The context interface was inserted between `reviewBudget`'s doc block and the function, so the whole block — including the input-laundering contract that opens it — attached to the interface and the function was left undocumented. The interface moves up beside its sibling with its own doc; the function keeps its contract. Its new paragraph also called both context fields environment values, when only one is — the other is resolved from settings. **Four test comments that argued for their assertions with false reasons.** The assertions were right and the rationales were not, which in a codebase whose comments are its design documentation is the more durable error. The integer guard is tested before the floor, not after, so the fractional value cited says nothing about what the floor would have caught alone. Not every value in the coercible-garbage list becomes zero — a negative stays negative and two strings become real counts, one of them large enough to land on the huge tier rather than the fallback. A single global bound of ten honours only two of the three clamp assertions, not all three; the third is an edge case, not a discriminator. And one comment justified its cases by naming a writer that does not exist on that branch. **A guarantee that is not guaranteed.** The settings fallback was documented as always degrading toward more work; that holds for three of the four fields and not for effort, where losing an operator's `high` returns the built-in rule and a local review drops to medium — less work, not more. Named as the exception it is, which is also why the fallback discloses on stderr. **Five prose sites that never learned the cap is conditional.** The step's stop rule, the clock module's own header, the large-diff cost model, the setting's schema description and the user-facing feature doc all still enumerated the cap unconditionally, contradicting the change one file away. The schema description additionally advised setting a deadline "to let a productive loop run longer", which is backwards: on a huge diff a deadline lowers the cap from five to three. **Two prose sites that overstated scope.** The setting is honoured from three operator scopes, not user only, and a value below the floor is ignored rather than clamped to it; and it cuts the cap for high-effort reviews only, since medium skips the reverse audit and low runs none. A cross-reference named a heading that does not exist. **The two seams nothing covered.** The environment boundary — whether this run has a deadline — was never exercised with the variable actually set: the budget tests pass the flag as a literal and the gate tests delete it. The operator ceiling's write path was likewise untested end to end; every builder call in the unit tests passes it as absent. Both now run through the real capture handler against a real environment and a mocked settings source, and both were mutation-checked: inverting the environment predicate and ignoring the ceiling each turn the new tests red. * fix(review): finish propagating the clock, and close the seams round 5 found Round-5 review of the stack. One real defect, one unsafe writer, and the rest is narration that stopped short plus coverage that could not see the change it was meant to cover. **A mock that leaked into every test after it.** The handler test added last round set an operator ceiling on a module-level settings mock and never restored it, so every later test in that file — including the whole trailing block — ran the real handler with an undeclared ceiling of twenty in play. Inert today because nothing downstream asserts on it, which is exactly how it would have survived to matter. Reset in the file's `beforeEach`. **The degrade announced itself with a writer that can destroy the degrade.** The settings fallback wrote its NOTE through the throwing stderr helper, whose safe sibling exists for precisely this case and says so in its own docstring: the write is incidental to the work in hand, and failing it would take down the fallback the guard exists to provide. Switched, with the reason recorded. **Five narration sites still stated the huge reduction unconditionally** — the field doc, the cap reader's own bullet, the enforcement-site comment above the round gate, the skill's version-skew paragraph, and the design note's "what an operator means by let it run longer". The last one was the most wrong: it named a deadline as the way to say "my ceiling is larger than six hours", but the check is for a deadline's *presence*, so any deadline however generous reads as three, and the cap is evaluated before the deadline arithmetic — setting one lowers the cap rather than raising it. Saying that would need the tier to read the deadline's size, which is now written down as the missing capability rather than implied to exist. **The setting was documented everywhere except the reference table.** The hand-maintained settings reference lists the review section exhaustively and did not list this one; the generator only writes the JSON schema, so it does not self-heal. Its schema description also never mentioned that only whole numbers are honoured, which matters because JSON Schema has no integer type here — a fraction validates in an editor and is discarded at runtime. **Two coverage gaps where the change was invisible to mutation.** Every gate test used the unsized fixture, whose tier is the fallback whatever the clock says, or forced a cap by storing one — so hardcoding the clock argument at all four call sites left the suite green. A sized huge plan is the only shape where the flag decides anything, and it now drives the gate on both sides. And two of the three capture commands had no budget assertion at all, so dropping either context field from their call sites compiled clean; the local one now asserts both. Both new tests were mutation-checked against exactly those edits.	2026-08-16 03:26:59 +00:00
Shaojin Wen	dc7e234876	feat(review): absorb prose gh commands into platform-backed subcommands (#9096) ... * docs(design): /review platform provider abstraction (GitHub + Aone Code) * feat(review): absorb prose gh commands into platform-backed subcommands The skill prose and agent briefs carried raw gh commands for the model to execute (repo resolution, head-SHA fetches, issue evidence, lightweight diffs, truncated-body refetches) — the prose-carried class that keeps shipping parsing bugs and drops the Enterprise host unless a prose rule remembers GH_HOST. Four new subcommands absorb them, built on a review-platform reader seam (lib/platform) whose first provider is GitHub over lib/gh.ts: - meta: repo identity + live headSha/webUrl (was gh repo view / gh pr view) - issue-context: closing-issue evidence file for Agent 0, incl. cross-repo issues and --issue for referenced-but-unlinked targets - fetch-diff: lightweight-mode diff to file (was gh pr diff redirects) - comment-body: one comment body by kind; pr-context truncation notes now name this command (with --host baked in) instead of a gh api route SKILL.md, the Agent 0 brief, and the role-0 generated prompt no longer contain model-executed gh calls; the GH_HOST prefixing prose rule is gone. * test(review): pin the bare-number host source as review meta Step 1 now derives a bare PR number's owner/repo/host with the meta subcommand instead of a prose gh repo view; the pin follows. * fix(review): address PR #9096 review findings Critical: - tests: resolve() expectations on Windows-asserted --out paths - issue-context: same-repo-keyed closing/extra dedup, extras self-dedup, and a failed single-issue fetch degrades to an explicit section instead of aborting the whole evidence file - meta: apply the URL-discovered host to gh routing before the PR call, validate --repo without requiring a number, usage errors exit 2 - agent-prompt: shellQuotePath the welded --out evidence path; plan-diff gains --host so a lightweight run welds it into the Agent 0 command - lib/gh: ghRaw (no trim) for diff/comment-body payloads whose edges are content; resolveGhHost normalizes an empty --host flag - SKILL.md: restore the constructable Posted:-link fallback; scope the no-model-run-gh-calls claim (Step 4 scratch-repo carve-out named) - issue-context: actionable error when gh < 2.72.0 lacks closingIssuesReferences Suggestions: drop dead host fields from run-function arg interfaces, exit-2 consistency, pin the previously unpinned contracts (setGhHost ordering x4, buildMarkdown host baking, welded GHE command, mkdirSync guards, no-comments placeholder, GH_HOST save/restore, 422 meta pin), fetch-diff handler tests, ClosingIssueRef dead fields removed, design-doc corrections (D1 subset note, D2 cell, D7 amend-delta rule, testing strategy wording, carve-out exemption), code-review.md --out fix. * fix(review): address PR #9096 round-2 review findings Critical: - lib/gh: ghRaw now returns bytes untouched — the unconditional CRLF rewrite would strip blob-content \r from every hunk of a CRLF-file diff (heavy mode's raw-bytes policy; the justification comment was wrong) - SKILL.md: Step 7's head-SHA fallback meta call carries the Enterprise --host annotation like every sibling call site - SKILL.md: the render-adjudication carve-out runs in a verifier subagent's shell — the Enterprise note now says exported-GH_HOST only, otherwise adjudication is unavailable (a --host note here cannot reach the subagent); the GHE enumeration also names submit Suggestions: - agent-prompt welds the plan's pr/ownerRepo/host only after re-validation (the plan is a file on disk; compose-review already re-validates) - plan-diff validates --host against HOSTNAME_RE before recording it - pins: full emitted-command prefix at all three sites, setGhHost ordering now includes ensureAuthenticated (x4), ghRaw no-trim/no-rewrite, plan-diff host write side, closing-ref repository-less fallback, --issue handler wiring, comment-body --out JSON marker + malformed-repo exit 2 + usage-error preempts auth, meta cwd-branch flag precedence, buildMarkdown host baking for inline/issue kinds - agent-briefs: --issue extras fetch from the PR's own repo — disclosed - pr-context: fix the resolveGhHost comment (env host IS baked) - docs: design doc corrections (gh.ts not-unchanged note, plan-diff in the inventory + D8, Phase 1 is new-implementation-not-refactor note, carve-out row/phase-3 ownership), review DESIGN.md issue-fetch path * fix(review): address PR #9096 round-3 review findings Critical: - lib/gh: HOSTNAME_RE now requires an alphanumeric first char and REPO_SEGMENT rejects a leading dash — flag-shaped values (--help, -evil/repo) no longer pass validation only to be misparsed as CLI options downstream of the unquoted weld Suggestions: - agent-prompt weld: the plan re-validation (digit prNumber, isOwnerRepo, HOSTNAME_RE-gated host) is now pinned by tampered-plan tests - setGhHost trims once so raw and resolved --host inputs agree - all four subcommands validate --repo before the auth gate (usage error exit 2, never preempted by an auth failure), pinned with ensureAuthenticated-not-called assertions - issue-context: bodies render untrimmed (leading-indent log pastes keep their code block); closing/extra dedup compares repos case-insensitively; a failed closing-issue discovery degrades into a named section (with the gh >= 2.72.0 hint) while --issue extras still fetch; numeric args get positive-integer validation with exit 2 (also --issue, id, --pr) - agent-briefs: retry-once guidance extended — unfetchable sections mean re-run with --issue before declaring evidence unavailable - SKILL.md: Step 5's lightweight block no longer re-fetches the diff Step 1 already wrote (one fetch, no head-advance race); SKILL.test.ts gains the revert guard for the lightweight capture + host note - meta: env-GH_HOST label for explicit --repo pinned * fix(review): address PR #9096 round-4 review findings Critical: - agent-briefs: the retry rule no longer sends unfetchable CLOSING refs through --issue (extras resolve in the PR's own repo — a cross-repo closing number would fetch the same-numbered unrelated issue); a plain re-run is the retry, closing refs are re-fetched every run - fetch-diff: an empty PR diff writes a 0-byte file, not a one-blank-line file that plan-diff dies on with a coverage error instead of taking the designed empty-plan branch Suggestions: - setGhHost: only genuinely-absent input resets; a non-empty all-whitespace --host now fails validation instead of silently restoring the default - agent-prompt weld trims the plan host before re-validating (fetch-pr records the raw flag); pr-context validates the resolved host against HOSTNAME_RE before baking it into emitted refetch commands - empty --out is a usage error (exit 2) classified before any fetch, in comment-body/fetch-diff/issue-context; plan-diff's handler maps the new --host usage error to exit 2 instead of an uncaught crash - issue-context: extras section header no longer claims NOT-in-closing when the closing set is UNKNOWN (discovery failed) - SKILL.md: Step 1's lightweight item spells out the fetch-diff failure stop rule; the Enterprise enumeration now lists every --host subcommand (adds plan-diff, test-plan, publish-assets); code-review.md matches - design doc: D1 names the ensureAuthenticated gate; the D2 carve-out row describes the shipped behavior (exported-GH_HOST only), not a welded prefix that never existed - pins: full-wrapper assertions extended, numeric usage gates at all three remaining handlers, --pr success-path plumbing, --out JSON marker, setGhHost TypeError class + trim/whitespace behavior, ghRaw byte fidelity, unfetchable extras in the JSON, cross-repo ownerRepo in the JSON, untrimmed body rendering, extras-section absence, discovery-failed header wording, runPrContext-level host baking (flag + env + rejected alias), SKILL revert guards for Step 7's meta rewiring * fix(review): address PR #9096 round-5 review findings Critical: - fetch-pr records the TRIMMED host into the fetch report, so the two downstream readers that re-validate it (compose-review's plan identity, the agent-prompt weld) see the canonical form — a padded-but-valid GHE host no longer drops to github.com anchor links - a non-empty all-whitespace --host no longer silently falls through to the env/default in resolveGhHost (it is returned as '', not swallowed), and publish-assets validates the raw flag via setGhHost before resolving — the Contents-API write can no longer be retargeted at github.com by a whitespace-only flag; match-remote now fails closed (exit 6) on the same input instead of matching github.com Suggestions: - plan-diff: drop the doubled `plan-diff:` prefix from the two thrown TypeErrors (the handler prepends it once); reject a whitespace-only --host instead of dropping it from the plan - new shared assertWritableOutPath (lib/paths): empty/whitespace AND directory --out targets are classified as usage errors BEFORE any fetch in comment-body/fetch-diff/issue-context (the directory case previously died EISDIR after the fetches and exit-coded as a runtime failure) - resolveRepo fetches `parent` and prefers it when the resolved repo is a fork — gh's default-repo preference is a remote literally named `upstream`, not an API fork check, so an origin-only fork clone no longer targets a fork's same-numbered PR - scope the comment-body exit-2 comment to the handler-level guards (yargs -layer missing-arg / invalid-choice failures exit 1 — a known gap) - SKILL.test revert guards: rule-4 issue-context weld + absence of the pre-absorption `--json closingIssuesReferences` syntax; the 422 `commit_id` comparison clause and the `fetch-diff`-output rename; the Step 6 tail-fetch `--out` sentence and the Posted: fallback grounding; the lightweight-capture host note - pins: malformed-host handler exit-2 in fetch-diff/issue-context/meta; issue-context exit-1 auth branch; padded-host weld trim; pr-context setGhHost routing (flag + env); whitespace-only --out in all three; numeric-gate tests reset process.exitCode between invocations and add non-integer cases; plan-diff asserts the metacharacter host is never recorded into the plan * fix(review): address PR #9096 round-5 findings (meta host guard, plan-diff stderr) - meta's discovery branch validates the routed host against HOSTNAME_RE before setGhHost: a host gh tolerates but the subcommands reject (underscore intranet aliases, IPv6 literals) is an environmental condition, so it now names the actual source (--host flag vs discovered repo-URL host) and fails exit 1, never as a usage error blaming a flag the caller never passed - plan-diff's handler catch uses writeStderrLineSafe (a broken stderr must not let the throw escape and lose the exit-2/exit-1 classification) * fix(review): address PR #9096 round-6 Critical findings - lib/gh: split the byte/text raw modes. execGhWithRetry gains a mode ('default' | 'bytes' | 'text'); the bytes mode runs with encoding 'buffer' and decodes latin1, so a diff of a non-UTF-8 (Latin-1/Shift-JIS) file no longer loses every invalid byte to U+FFFD. ghRaw is the bytes mode (fetch-diff writes it back with latin1 — byte fidelity end to end); new ghRawText is UTF-8-with-edges-preserved, which comment-body uses (comment bodies are always valid UTF-8 from the API; the leading-indent code-block fidelity holds, but bytes are not corrupted into mojibake) - lib/gh: split the leading-dash ban per segment — owners cannot start with a hyphen but REPO names can (yezhaodan/-Git exists), so a leading dash on the repo half is no longer rejected (the ban only protected against the flag-shaped OWNER half anyway) - github resolveRepo: take the host from the resolved repo's OWN url — gh's `parent` field carries no url (only id/name/owner), so reading target.url crashed every origin-only fork clone with TypeError; the meta.test fork fixture now matches the real gh shape - publish-assets: the round-5 raw-flag validation guarded on `trim() !== ''`, which skipped exactly the whitespace-only host it exists to refuse — guard on presence instead so setGhHost(' ') throws the documented TypeError (exit-3 refusal, no silent Contents-API retarget at the env/default host) * fix(review): address PR #9096 round-6 gpt-5.6-sol Critical findings - agent-prompt weld: a present-but-invalid plan host now fails closed (throws) instead of being silently dropped to null — a tampered host can no longer quietly reroute the evidence fetch to github.com's same-named repo (a missing host stays optional) - comment-body: read `.body` off the JSON-parsed response instead of `--jq '.body // ""'` — the jq form appends a trailing newline (a body not ending in one gained a byte; an empty body became "\n"); JSON parse returns the exact bytes GitHub stores - issue-context: --issue now accepts `owner/repo#123` as well as `123`, so a referenced issue living in a DIFFERENT repo is fetched there instead of silently reading the PR repo's same-numbered unrelated issue; dedup is by (repo, number) pair, case-insensitively, which also fixes the cross-repo-closing-shadows-same-repo-extra edge uniformly - lib/gh: drop the now-unused ghRawText text mode (comment-body moved to the JSON parse) * fix(review): address PR #9096 round-7 review findings Critical: - R7-1: the Agent 0 brief, SKILL.md rule 4, and code-review.md still taught "issue-context cannot fetch a referenced issue in a different repo — declare it unavailable", contradicting the cross-repo `--issue owner/repo#123` capability shipped in round 6. All three carriers now teach the qualified form, and the wrong-issue warning / retry ban is narrowed to bare numbers (a qualified retry is a correct retry) Suggestions (all directly pin or harden this PR's changes): - agent-prompt weld fails closed on a present-but-NON-STRING host and on a present-but-whitespace-only host (both were silently dropped to null, rerouting the evidence fetch), matching the sibling identity fields - gh.test.ts: the ghRaw byte-fidelity test now returns a real Buffer with an invalid-UTF-8 byte (0xE9) — the latin1 decode genuinely executes (the previous string mock made String.prototype.toString an identity call) - meta: the explicit-`--repo` branch gates the emitted host with HOSTNAME_RE, same as the discovery branch (an unroutable GH_HOST env value no longer emits a host label every sibling rejects) - publish-assets: pin the round-6 whitespace-host refusal (exit 3, no gh call, `(from --host)` in stderr) - issue-context: pin the documented `--issue owner/repo#n` grammar end to end through the handler regex - code-review.md: the GHE `--host` enumeration adds match-remote (the pipeline's first host-sensitive step) * fix(review): address PR #9096 round-8 review findings Critical: - R8-1: the round-7 non-string-host guard threw on `host: null` — which fetch-pr writes unconditionally into every same-repo github.com plan (`args.host?.trim() || null`), so every ordinary review would have failed at the roster build. null is now tolerated (only a present non-null non-string host throws); regression test added - R8-2: comment-body validates `--kind` is a single admitted token before any platform call — a duplicated `--kind` arrives as an array that passes yargs' element-wise choices, and String() would coerce it to 'review,inline' into the wrong API collection Suggestions: - assertWritableOutPath rejects a trailing-separator --out (the POSIX directory spelling that resolve() normalizes away) - comment-body prints the body via process.stdout.write (byte-exact, no invented trailing newline) - agent-prompt weld prNumber guard strengthened (rejects 0 and unsafe integers, matching the welded handler's contract) - pins: meta explicit-branch HOSTNAME_RE gate, isOwnerRepo dash asymmetry both directions, ghRaw retry with buffer stderr, whitespace-only and null plan hosts, issue-context qualified-grammar rejection side, corrected the misleading case-insensitive dedup test, fs mocks no longer consult ambient /tmp state (existsSync/statSync overridden) - R8-13 (extras-header double-render assertion) deferred to #9194 per the reviewer's own note	2026-08-15 09:16:48 +00:00
qqqys	8517fa9d47	feat(web-shell): redesign Channel policy and workspace management (#8848) ... * feat(web-shell): expose channel access policies * test(cli): cover shared Channel management fields * feat(web-shell): clarify channel policy controls * feat(web-shell): select channel workspace * feat(web-shell): redesign channel management * fix(web-shell): align channel manager with shell tabs * fix(web-shell): prioritize conversation settings * fix(web-shell): preserve legacy channel defaults * fix(channels): address management review blockers * fix(channels): address editor review blockers * fix(channels): preserve workspace action and route state * fix(web-shell): prevent stale channel editor state * fix(channels): preserve stored group settings * fix(channels): preserve group behavior settings * fix(web-shell): reset channel workspace UI state * test(web-shell): assert restored channel scope * fix(web-shell): preserve legacy channel scope * fix(web-shell): preserve inherited channel defaults * fix(channels): preserve compatible legacy settings * fix(web-shell): keep workspace navigation available * fix(channels): default new channels to pairing --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-14 07:41:44 +00:00
Shaojin Wen	97ec96ec54	feat(cli): Add review settings for attribution, default effort, and default comment (#8994) ... * feat(cli): Add review settings for attribution, default effort, and default comment * fix(cli): resolve review settings from operator scopes and close gate gaps (#8994) Address review feedback on the review settings: - Resolve review.attribution/effort/comment from operator-controlled settings scopes only (system defaults, user, system); a repository's .qwen/settings.json is content under review and must not control whether findings publish, whether the review names its model, or how deeply the pipeline verifies. - Normalize the configured review.effort through the same case- insensitive validation as the --effort flag, so "Low" cannot miss the exact comparisons the forcings run and invalid values cannot leak into the verdict. - Gate the modelId requirement and footer-safety validation on attribution: with the footer gated off, the field has no consumer and must not refuse the run. - Pass the standing review.comment setting into publish-assets' call of the shared authorisation gate, so both callers agree on what authorises a run. - Make presubmit's self-comment detection footer-independent by also matching the reviewing account's own top-level comments, so attribution-off posts still dedup. - Align SKILL.md's Step 7 gate and every --comment branch on comment.effective, and add handler-level wiring tests for all configured defaults. * test(cli): pin the review-settings operator defaults with unit tests (#8994) * fix(cli): share the guarded footer strip and pin the gate audit text (#8994) * fix(cli): raise the repository-context array bound to 256 (#8994) * fix(cli): validate review setting values and tighten the review gates (#8994) * fix(cli): align presubmit dedup with severityOf and normalize auto effort (#8994) * fix(cli): show the review settings in the settings dialog (#8994) * fix(cli): bound the footer strip tail and match refusal advice to its class (#8994) --------- Co-authored-by: qwen-code-autofix[bot] <qwen-code-autofix[bot]@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com>	2026-08-13 13:27:39 +00:00
易良	8858d4340b	feat(cli): add native multi-agent coordination (#8804) ... * feat(core): add native multi-agent coordination * feat(cli): add agent view pty workers * fix(cli): harden agent view pty workers * fix(cli): harden agent view pty host teardown * fix(cli): fail fast on pty host auth rejection * test(cli): cover pty host remote exit polling * fix(cli): address agent view pty worker review nits * fix(cli): harden pty host socket fallback * fix(cli): harden agent view pty workers * fix(cli): harden agent view pty workers * test(cli): cover pty host spawn contract * fix(cli): guard agent view pty host socket takeover and races * fix(cli): fit agent view pty logs under wire cap, strip host token after merge * feat(cli): manage agent view session lifecycle * fix(cli): preserve agent view lifecycle state * fix(cli): harden agent view lifecycle persistence * fix(cli): harden agent view lifecycle * fix(cli): harden agent view lifecycle recovery * feat(cli): expose agent view commands * fix(cli): wire agent view command safeguards * fix(cli): resolve agent view command integration * feat(cli): add agent view roster ui * feat(cli): add durable multi-agent coordination * fix(cli): harden multi-agent coordination * fix(cli): complete native coordination flows * fix(cli): wait for agent view host cold starts * fix(cli): allow coordination startup time * fix(cli): persist missing coordination results * fix(cli): enforce exact Agent View answers * refactor(cli): reuse existing agent team coordination * docs(cli): clarify homogeneous coordination * feat(core): complete native team coordination * fix(agents): enforce teammate coordination boundaries * fix(agents): close teammate lifecycle gaps * fix(agents): align empty teammate names with routing * fix(core): close coordinator review gaps * fix(core): keep read-only teammates off writer tasks --------- Co-authored-by: 俊良 <zzj542558@alibaba-inc.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>	2026-08-12 17:39:03 +00:00
jinye	4980a2c20d	fix(cli): bound headless tool result content (#9012) ... Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-12 16:22:36 +00:00
易良	70672f8fb8	fix(cli): avoid duplicate context usage in footer and status line (#8749) ... * fix(cli): avoid duplicate context usage in footer and status line The built-in default status line preset includes `context-used`, and the footer renders its own context indicator unless `hideContextIndicator` is set, so context usage was shown twice out of the box. Treat `ui.statusLine.hideContextIndicator` as tri-state: an explicit boolean still wins in both directions, and when it is unset a preset status line containing `context-used` or `context-remaining` hides the footer indicator. Command status lines are unchanged — their output is opaque, so it is never inspected for context information. Fixes #8695 * fix(cli): preserve status line context override * fix(cli): preserve status line context semantics * fix(cli): keep context visible in narrow footers * fix(cli): keep context visible when status line clips * fix(cli): preserve context indicator visibility * fix(cli): match status line wrap layout	2026-08-11 07:18:48 +00:00
Shaojin Wen	af372e5a21	perf(review): guarantee compose survives a reverse-audit budget stop (#8791) ... * perf(review): guarantee compose survives a reverse-audit budget stop PR #8687 — a 4,269-line cross-worktree git guard — timed out after six hours and posted nothing, holding ~20 E2E-confirmed Critical bypasses. The deadline gate worked: it refused round 3 correctly with ~110 minutes and the whole reserve in hand. The tail after the stop was the killer — a single hand-rolled verification agent re-running a 15-family shell/git bypass battery with real filesystem E2E consumed all of it, and the wall hit mid-verification before compose-review ever ran. The reserve was one number covering "verification + compose + submit", which is right for a normal per-finding re-trace but wrong for a security PR where verification cost is unbounded (real E2E per finding) while compose and submit stay bounded. So a distinct, smaller compose FLOOR is carved out and the VERIFIER — not the reverse-audit builder — is gated on it: below the floor `agent-prompt --role verify` refuses to build (VERIFY BUDGET, exit 4), the findings keep their `— [unverified]` tag for compose-review to cap, and compose runs. The floor is strictly below the reserve, so a healthy run reaches the reverse-audit gate first and never sees it; it is the cover for the one span the reserve cannot bound. The prose closes the bypass the gate cannot see: the post-stop tail verifies only through the gated builder, never a hand-rolled agent, and invents no fresh re-verification pass for findings already confirmed — compose and submit are non-negotiable. DESIGN.md records the incident; the RA budget message and SKILL Step 5 tail are rewritten to match. * fix(review): close the round-1 gaps in the compose-floor gate - R2-2 (Critical): the documented `0` escape hatch did not disable the verify gate past the deadline — `remainingSeconds` goes negative there and `negative >= 0` is false, firing the supposedly-disabled gate. verifyBudgetExhausted now returns null the moment the effective floor is 0, before the comparison. Pinned with a past-deadline case. - R2-1 (Critical): the gate bounds prompt CONSTRUCTION, not the wall time of an already-admitted verifier that then runs a long E2E past the floor — and agent-prompt builds prompts, it cannot cancel a running agent. The SKILL tail now tells the orchestrator to bound the WAIT: when the deadline is within the compose floor and a verifier batch has not returned, stop waiting on it, keep its findings unverified, and compose. The remaining execution-time cancellation is a harness capability, noted as such (same layer boundary as the hand-rolled-agent caveat). - R2-3: the agent-prompt exit-code help now documents both the BUDGET and VERIFY BUDGET exit-4 refusals. - R2-4: the reverseAuditBudgetMessage test now pins the new tail rules (gated verifier only, no hand-rolled agent, no re-verification). - R2-5: docs/users/features/code-review.md documents the compose floor — default, env var, reserve nesting, exit-4 behaviour, zero hatch. * fix(review): round-2 fixes for the compose-floor gate - R3-1 (Critical): the verify gate admitted at exactly the floor, where the first work crosses below it — the floor is compose-only with no margin, so it now refuses at equality (`> floor`, unlike the RA reserve which admits at exact cover). Exact-boundary test flipped. - R3-2 (Critical): the refusal message and SKILL claimed unverified findings "post as needing human review", but the confirmed-only rule keeps tagged details terminal-only. Reworded to the true contract: compose-review caps the verdict and discloses the verification gap; the tagged details stay terminal-only; what posts is the earlier rounds' confirmed findings plus that gap. - R3-5: extracted readDeadlineSeconds / readNonNegativeSeconds, shared by both gates so the fail-open contract lives in one place. - R3-3: pinned the verify gate's fail-open branches (malformed/non-positive deadline, past-deadline negative remaining, negative-floor fallback). - R3-4: pinned the floor-minutes rendering (a field swap to remainingSeconds would misstate the protected floor). - R3-7: pinned that a refused verifier writes no budget-stop marker and no admission stamp. - R3-8: pinned validation-before-gate (a malformed verify call under the floor throws, not exit 4). R3-6 needs no change: the SKILL.test pointer<->heading gate already covers the DESIGN section (a dangling pointer fails it). * fix(review): round-3 cheap fixes for the compose-floor gate Low-risk corrections; the two edge-case Criticals (R4-1 broken-plan masking, shared with the RA gate; R4-2 compose-review relaunch FIX) are left as follow-ups — noted on the threads. - R4-4: the readDeadlineSeconds extraction stranded reverseAuditBudgetExhausted's contract JSDoc above the helper; moved it back onto the function. - R4-5: the round-2 "terminal-only, never posted" wording contradicted compose-review's own verdict line ("posted, disclosed as unverified") — a pre-existing contract ambiguity this PR should not relitigate. Reworded the message and SKILL to the invariant both readings share: an unverified finding is never treated as a confirmed blocker; the verdict is capped. - R4-7: "below the N-minute floor" contradicted the exact-equality refusal (the gate admits on `> floor`); now "at or below", in the message and the user docs. - R4-3: pinned that a blank/whitespace floor override falls back to the default (only explicit 0 disables). - R4-6: pinned the negative-remaining clamp in verifyBudgetMessage. --------- Co-authored-by: verify <verify@local>	2026-08-10 02:50:45 +00:00
qqqys	88a325bce9	feat(workflows): add cooperative pause and resume (#8320) ... * feat(workflows): add cooperative pause and resume * fix(workflows): restrict pause to background runs * fix(cli): clarify foreground workflow pause errors * fix(core): preserve dispatch errors across cancellation * test(core): cover late workflow state callbacks * fix(workflows): address review suggestions (#8320) - Rename misleading `terminal` local to `presentation` in BackgroundTasksDialog - Fix vacuous `toContain('p')` assertion to `toContain('Background tasks + p')` - Fix vacuous gate assertion with macrotask yield in scheduler test - Add over-count cap test for `onAgentCompleted` past dispatched count - Add pausing-state approval parking test - Remove dead `concurrencyLimiter` module (no production consumers) * test(workflows): pin review-flagged mutation-surviving branches (#8320) * test(cli): use valid agent status in detail-view reset test (#8320) * test(workflows): harden pause-gate settle probes with a full flush (#8320) * fix(workflows): address round-5 review findings (#8320) * test(ci): sync review timeout assertions with repository variables (#8320) * fix(workflows): address round-6 review findings (#8320) * fix(workflows): address round-7 review findings (#8320) * fix(workflows): address round-8 review findings (#8320) * fix(workflows): address round-9 review findings (#8320) * fix(workflows): address round-10 review findings (#8320) * fix(workflows): address round-11 review findings (#8320) * fix(workflows): address round-12 review findings (#8320) * fix(workflows): address round-13 review findings (#8320) * fix(workflows): address round-14 review findings (#8320) * fix(workflows): address round-15 review findings (#8320) --------- Co-authored-by: qwen-code-dev-bot <269191875+qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: qwen-code-bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-08 04:21:21 +00:00
destire-mio	b34a08d16f	fix(core): separate hook context from transcript display (#7948) ... * fix(core): separate hook context from transcript display * test(ci): gate desktop transcript projection * revert: keep desktop CI scope unchanged * test: cover transcript display fallbacks * fix(transcript): address review feedback * fix(transcript): reconcile post-merge provenance paths * fix(webui): preserve legacy transcript concatenation * test(transcript): cover projection consumers * fix(transcript): consolidate hook context projection * fix(transcript): support single-field display provenance * fix(transcript): strip hook context with invalid metadata * test(acp): cover empty replay display text --------- Co-authored-by: destire-mio <248462155+destire-mio@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-08 02:03:20 +00:00
jinye	26352fcc6a	feat(external-context): Add optional Mem0 memory writes (#8507) ... * feat(external-context): Add optional Mem0 memory writes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(hooks): Preserve confirmation content visibility Render PreToolUse confirmation reasons literally and keep long confirmations accessible through the virtualized TUI. Add unit and interactive regression coverage for Mem0 write confirmations. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Address memory write review findings Align Hook and MCP argument handling, distinguish definitive Provider rejections from ambiguous outcomes, improve deployment diagnostics, and document the write-back trust boundary. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(hooks): Refine plain-text confirmations Render URLs consistently, avoid persistent virtual viewport gaps, and document the literal-rendering and managed deployment boundaries. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Support Auto Edit write confirmation Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Harden write confirmations Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): Measure virtual row height directly Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): Preserve YOLO Hook confirmation content Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-07 16:52:33 +00:00
BaboBen	028747aa41	feat(feishu): enrich observed contact labels (#8569) ... * docs: design feishu observed contact enrichment * docs: add Chinese Feishu enrichment design * feat(feishu): enrich observed contact labels * fix(feishu): preserve enriched contact labels * fix(feishu): harden observed-contact label enrichment lifecycle * fix(feishu): bound label caches, honor observation recency, silence enrichment token failures (#8569) - hydrate runtime label caches from the newest observation per contact so stale group membership labels cannot overwrite more recent ones - cap the user/chat label, in-flight lookup, and write-dedup maps at 500 entries (matching the persisted registry) and evict oldest entries - route best-effort label lookups through a silent token refresh path so enrichment failures no longer write to stderr - add tests for silent token refresh, newest-label hydration, cache cap, and the persisted-observation reject path in hook ordering * fix(feishu): address observed-contact label review feedback (#8569) * Track core (non-silent) waiters on the shared tenant-token refresh so a silent-initiated refresh still logs token errors for joined delivery callers. * Short-circuit label lookups on the resolved names cache so evicted lookup entries do not trigger redundant API requests. * Re-hydrate label caches from the persisted registry after an in-lifetime cache eviction so the next initial write cannot clobber a persisted label with the raw ID. * Add mutation-proof regression tests for the channel-isolation filter, the list-failure swallow, the silent HTTP-error branch, and the 'unknown' label guard. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com>	2026-08-07 16:41:37 +00:00
BaboBen	edb420393e	fix(channels): manage DingTalk interactive card config (#8517) ... * fix(channels): manage DingTalk interactive card config * test(cli): cover nested channel object validation * fix(channels): harden nested management metadata * fix(channels): isolate invalid management descriptors * fix(channels): isolate invalid management metadata from channel runtime * fix(channels): reject reserved unknown keys in management config upserts * fix(channels): harden channel management validation and editor checks Reject management descriptors that lack a fields array at registration so broken plugins are stripped to unmanageable instead of being advertised as manageable and failing every upsert with an unmapped TypeError. Reserve the top-level "type" field key and require enum fields to declare at least one option, both of which the settings store could never accept. Treat whitespace-only number drafts as empty in the channel editor, consistent with the module's other emptiness checks. Also give the SDK descriptor mirror test a runtime wire-shape walk over the built-in catalog, add the parser's timeout rejection boundary, and restore the exact built-in catalog membership assertion. * fix(channels): validate management field shapes and editor bounds (#8517) * fix(channels): align management validation layers and pin gate behavior (#8517) Read envResolvable by truthiness in the settings store so it matches the registration gate and the editor, instead of rejecting the advertised environment references of untyped plugins. Fail closed at registration on non-finite exclusiveMinimum values, empty object property lists, and async validateConfig functions, all of which would otherwise advertise a field or save path that can never succeed. Strip invalid management metadata over a prototype-preserving copy so class-instance plugins keep their createChannel implementation. Move the unchanged-value preservation exemption ahead of the object shape rejection so a stored non-record value (for example a hand-written null) no longer locks every unrelated management edit of that channel. Clamp DingTalk question-card timeouts at the maximum setTimeout delay, since Node treats larger delays as one millisecond and would expire cards instantly. Pin the previously untested load-bearing behaviors: per-key previous threading in the recursive validation, the preservation exemption's precedence over nested required enforcement, nested "type" properties, depth-2 nesting rules, and the nested-only constraints of the daemon descriptor wire contract. * fix(channels): close reserved-key preservation gaps and pin gate behavior (#8517) * test(cli): tolerate IPv6-less hosts in serve ::1 bind tests (#8517) The self-hosted CI containers can have no IPv6 loopback, where the two runQwenServe tests that bind ::1 fail with EADDRNOTAVAIL. Probe the interfaces once and skip only the IPv6-dependent binds there; every assertion still runs on IPv6-capable hosts. * fix(channels): align descriptor type contracts with runtime validation (#8517) The registry already rejects object fields without a non-empty properties array and enums without unique options, but the descriptor types still admitted both, so TS-authored plugins only learned about it when registration stripped their management surface. Make `properties` required, give enums a dedicated descriptor member with required `options`, and drop the never-honored `envResolvable` flag from number descriptors, in both channel-base and the SDK mirror, and export the descriptor sub-types through the webui barrels. Also map a throwing `validateConfig` to the usual invalid-config error and pin the store contracts that had no distinguishing tests: omitting a parent object drops the stored object without checking its nested required, writes replace nested values wholesale, unchanged stored scalars are still re-validated, and valid plugins register by original reference. * fix(channels): defuse validateConfig rejection leak and close descriptor gate gaps (#8517) 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-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-07 15:47:20 +00:00
qqqys	3edecac116	feat(channels): support group pairing (#8440) ... * feat(channels): support group pairing * fix(channels): address group pairing review * fix(web-shell): show group pairing management * fix(channels): recheck group pairing before history backfill * test(channels): verify group approval isolation * fix(channels): address group pairing review findings and pin behaviors (#8440) - Grandfather the group allowlist file in PairingStore legacy migration - Offer the pairing groupPolicy option in github/gitlab descriptors - Re-export DaemonChannelPairingSubject from the webui barrels - Refresh the GroupGate doc comment and channel docs rows - Pin the unpinned group pairing behaviors called out in review: subject dedup, trigger matrix, notification content/cap/failure/ thread routing, DM negative space under groupPolicy pairing, stored DM loop authz, pairing-enabled guard negative space, approval/revocation HTTP bodies, descriptor-driven gate branch, and the web-shell group approval mirrors - Add a compile-time assertion for the revocation request union * fix(channels): address group pairing review findings (#8440) - Accept 'pairing' in the GitLab connect warning, descriptor help text, and gitlab.md: todos dispatch after one-time group approval. - Model group approvals in the web-shell e2e mock daemon (approve by subject type, GET returns senderIds+groupIds, DELETE accepts groupId) and exercise the group pairing flow in the channels spec. - Add 'pairing' to the groupPolicy enumerations in the plugins and per-channel docs (telegram, feishu, dingtalk, qqbot, wecom). - Update the channel pairing CLI help to cover group requests. - Cap pending pairing requests at one per sender so a single member cannot occupy every shared pending slot. * fix(channels): address group pairing review findings round 7 (#8440) * fix(channels): address group pairing review findings round 8 (#8440) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(channels): address group pairing review findings round 9 (#8440) --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-07 09:20:18 +00:00
Shaojin Wen	7edc16ba11	feat(review): say so when the bundle is older than the review it runs (#8390) ... * feat(review): say so when the bundle is older than the review it runs Every `qwen review …` step runs the BUILT bundle, not the working tree. So editing a review command, or switching to a branch that contains one, changes nothing about the run until someone rebuilds -- and the failure is silent and total: the run behaves like the last build, and every conclusion drawn from it is a conclusion about that build. Measured on 2026-08-02, dogfooding /review against #8368 from a checkout whose bundle was fourteen hours old. Three things were invalidated at once and none announced itself: `drive` and `mock-provider` had merged that morning and were absent from the binary, so "the agent never reached for them" measured nothing; and #8345's guard against scoring a mutant `survived` when its own collocated test was red had merged too, so the run reproduced the bug it fixed and filed three findings the current code holds as `inconclusive`. The round was discarded and re-run after a rebuild. `parse-args` is the first command of every review, which makes it the only place a notice reaches a reader before they act on a result. It names the file that is ahead, by how much, what actually runs from the bundle, and the command to rebuild -- "rebuild" without evidence is advice nobody can check. mtime, not git: the question is whether this bundle was built from this source, and a git comparison answers a different one. A margin absorbs a checkout, which writes everything at once in no guaranteed order. An installed package has no sources beside it, finds nothing to compare, and stays silent -- a check that cannot see the files must not accuse the build. Also documents `findings --test-delta` for users: it can lower a severity, and therefore change what the verdict is computed from, so it belongs beside `--outcomes` rather than only in the skill. * fix(review): watch the file every subcommand is registered in `packages/cli/src/commands/review.ts` is where all 30-odd subcommands are imported and registered, and it sits beside the directory rather than in it -- so a new command, or a changed dispatch, was exactly the change this check could not see. A root may now be a single file, which is what that one is. Confirmed end to end: with `review.ts` three hours ahead of a fresh bundle, the warning names it. Also two comments that did not match the code: symlinks of every kind are skipped, not only directories (`isFile()` is false for a symlinked file too), and the module now says what `QWEN_CODE_CLI` already covers -- talking to a different program -- so it is clear this guards the other half, the right program built before the change. * fix(review): compare content, because a timestamp check cried wolf The first version compared the bundle's mtime against the newest review source, and it was wrong in the direction that matters most. `git checkout` rewrites every file that differs between two commits, so returning to the branch a bundle was built from re-stamps exactly those files and the check calls a byte-for-byte correct bundle stale. Measured: with the sources untouched and the bundle two minutes older, it warned. A line that fires when nothing is wrong teaches its reader to skip the line, which would have made this worse than absent. The build now stamps a digest of the review sources it bundled into `dist/review-sources.sha256`, and the check re-derives that digest from the tree and compares. No margin to tune, no clock to trust, and no answer but the true one. Verified end to end across all five cases: a clean tree is silent, a source touched but unchanged is silent, and a real change under any of the three roots -- the command directory, the `review.ts` that registers them, the bundled skill -- warns. The digest is now one rule stated twice, since the build script cannot import the package it runs before building. `scripts/tests/review-source-digest.test.ts` holds the two equal, on this repo and on a synthetic tree that exercises the file-shaped root; a package test may not reach into `scripts/`, so it lives on the side of the boundary that may. Paths are folded relative to the repo root with separators normalised, and the file list is sorted -- `readdir` order is a property of the filesystem, so without it a bundle built in CI and a tree cloned locally would hash the same source differently and every run would warn. * fix(review): a diagnostic must not kill the run, and tests are not the bundle Two Criticals and five suggestions from review, all verified before changing anything. `writeStderrLine` throws on EPIPE, so stderr piped to `head` would have killed the review before it parsed a single argument -- a warning that destroys the run it was warning about, and the opposite of this change's own invariant. `writeStderrLineSafe` is the convention for diagnostics in this subsystem and is what it calls now. `reviewSourceRoots` builds paths with the platform `join`, and the test asserted forward-slash literals, so all three elements would have failed on the merge queue's Windows leg -- which the pull_request event never runs, so the green CI here proved nothing about it. Test files left the digest. esbuild follows imports from the CLI entry and no test is reachable that way, so folding them in fired the warning for an edit that cannot change a byte of the bundle -- the false positive this module already rejected once. 112 files became 61, and a test-only edit is now silent while a production one still warns. The handler wiring is tested at last, against a real temp tree rather than a mock of the reads under test: the derivation from `process.argv[1]`, the stamp read, and the warning. All three mutations the review named -- dropping the call, reading the stamp from the wrong directory, collapsing repoRoot to distDir -- now redden it. Also: the stamp's filename is pinned across the boundary it crosses (the build wrote a literal while the check read `DIGEST_FILE`, so a one-sided rename would have silenced the feature with every test green); the digest is computed only when there is a stamp to compare it against, instead of hashing a hundred files for a value the first guard discards; the `rebuildCommand` parameter no caller ever set is gone; and the build script's comment no longer claims a code-sharing relationship that does not exist. * fix(review): fixtures are not in the bundle either The same false positive, a third time and one directory over. Excluding tests from the digest was right and incomplete: `review/__fixtures__` holds four files — three responder modules and a captured comment — that a test loads at runtime, from no import the bundler follows. Measured against `dist`: none of the four appears in it, so editing one changed the digest while the bundle stayed byte-identical and the warning claimed a review command had changed. Both walks skip the directory now, and the parity test's synthetic tree grows a fixture and a `.spec.tsx` so the two implementations are held equal on the whole exclusion, not just the part the first case exercised. Reverting one side reddens the local case AND both parity cases, which is what that guard is for. Verified the other direction too, since an exclusion can overshoot: every review source that reaches `dist` is still covered. `DESIGN.md` and `SKILL.md` both ship and both remain in the digest — checked, not assumed, after two rounds of this exact mistake. Six cases end to end after a rebuild: a clean tree, a test edit and a fixture edit are silent; a production edit, a `review.ts` edit and a `DESIGN.md` edit each warn. * fix(review): allowlist the stamp, and stop guessing what the bundle holds The Critical first: `create-standalone-package.js` fails on any top-level dist entry outside its allowlist, and `review-sources.sha256` was on neither list. The next release would have aborted the standalone archive on all five targets, and no PR-time job runs the packager, which is why this suite is green. Allowlisted -- shipping it is harmless, since a standalone install has no `packages/` to compare against and the check stays silent there. `lib/test-utils.ts` was in the digest: test support with a production-looking name, imported by two test files and nothing else. That is the fourth patch to one rule -- `.test.ts`, then `__fixtures__/`, then this, plus `.DS_Store` -- and each was found by a reviewer after it shipped. So the rule stops being a list somebody remembers to extend: a new test asserts the property the list approximates, that every file the digest folds in is reachable from production code and nothing reachable is left out. Dropping `test-utils.ts` from the exclusion reddens it, which is the fifth instance failing in CI instead of in a review. Three branches that no test reached, each with a mutant the review measured surviving the whole suite: the walk's symlink skip (a directory cycle would send the first command of every review into unbounded recursion), the read-failure path (hashing the survivors of a concurrent checkout would accuse a tree that is merely mid-change), and the build's stamp call site (removing it left the scripts suite green while `npm run bundle` silently stopped writing the stamp). All three now redden. And `unmeasured` had no reader, so the one edge this check cannot measure but can see -- sources present, stamp absent -- passed in silence. That is the state of every existing checkout the moment this ships, and it is exactly the silent failure the change was written to end. It now says so, while an installed package, which has no sources either, still says nothing. * fix(review): the guard was shallower than the property it claimed The guard added last round asserts that every file in the digest is reachable from production code. It did not: a file imported by nothing passed, because the filter also required some test to import it; only `.ts` was inspected, so a test-only `.tsx` or `.mts` helper walked through; and it read static imports only, while this directory has nine `await import('./…')` edges. It asserts the property now — every extension, orphans included, dynamic edges seen — and the tree has no violators, so the strictness cost nothing today and is there for the next file. `__snapshots__` joins the exclusions. `vitest --update` regenerating a snapshot would have moved the digest with the bundle byte-identical; none exists under the review roots today only by chance, and 120 `toMatchSnapshot()` calls live elsewhere in this package. Three couplings that no test held: - the allowlist entry that fixed the release-breaking R2-1 -- reverting those five lines left the whole scripts suite green, and the next failure would have been a release aborting on all five targets. `isAllowedDistEntry` is exported and the stamp's own name is asserted against it, so a one-sided rename fails here instead; - the `.DS_Store` member of `NOT_BUNDLED_FILE`, absent from the repo and so from the parity tree -- one-sided removal stayed green while a macOS checkout would digest differently on the two sides forever; - each `unmeasured` reason. Swapping the two arguments at the single call site kept all 76 tests green while telling a pre-stamp checkout its sources were missing. And two comments that said the opposite of the code beneath them: the digest is computed unconditionally on purpose (the pre-stamp notice needs it), and `NOT_BUNDLED_FILE` helpers are deliberately not importers, since nothing reaches the bundle through a file the bundle does not contain. The two stderr diagnostics are documented for users, beside the sibling paragraph this PR already added. * fix(review): measure only the layout that can carry a stamp `npm start` launches `node <root>/packages/cli`, and node sets `argv[1]` to that directory -- so the derivation found sources under `<root>` with no stamp beside them and printed "could not check" on every review, forever, with advice that could never make it stop. That is the fires-when-nothing-is-wrong failure this change argues against, on the path `start.js` sets `QWEN_CODE_CLI` to precisely so reviews reach that build. Only a `<root>/dist/cli.js` layout is measured now; anything else has no stamp to find and no way to grow one. The build-side digest could kill `npm run bundle` where the check side degrades gracefully: a file vanishing mid-walk threw out of the hash loop, and the stamp is the copier's last step, so the build would fail with every asset already in place. Caught and skipped -- a missing stamp is `unmeasured`, which the runtime already treats as an acceptable answer. The skill now says what to do with the warning, which is the half that makes it reach a human: `parse-args` runs inside an agent's shell tool, the user reads the agent's summary rather than raw stderr, and a line nobody repeats is a line nobody sees -- which is how the 2026-08-02 round went wrong in the first place. It also records that the instruction cannot help the run that needs it, since the skill comes from the same bundle. And the scope is stated where silence could be over-read: the digest covers the review commands, the file that registers them, and the bundled skill -- not the shared helpers those import. A quiet run means the review code matches the bundle, not that the tree does. * fix(review): refuse to certify a bundle the copier may not describe The stamp described the tree as the COPIER saw it, and the copier runs after esbuild -- so a source edited in between, or `copy_bundle_assets.js` run on its own (it self-executes), wrote a digest certifying a `cli.js` built from something else. Silence then means "verified fresh" when it is not, and that is the only direction here where a quiet run is affirmatively wrong rather than merely uninformative: every other gap degrades to `unmeasured`. Timestamps are the wrong tool for judging staleness and the right one for judging whether this stamp can be honest at all, so the build refuses when any source is newer than the bundle it would attest to, and says why. Driven for real: touching a review source and running the copier alone now prints "skipped the source digest rather than certify a bundle it may not describe". `it('counts the same files')` compared nothing -- it asserted `> 50` on the build side while the check side exposes no count, so the title claimed a parity the body never checked, and the margin over the real 56 made it a future false alarm in `scripts/` for an unrelated change. Removed; the digest parity already holds the file set. "Root is a file" was inferred from `readdirSync` raising ENOTDIR, an assumption about every platform's libuv on the one root that is a file -- `review.ts`, where "a new subcommand was registered" lives. `statSync(root).isFile()` says it instead. And the check itself moves out of the handler into `bundleStalenessNotices`, which is where the rest of it already lived. `parse-args` is about parsing arguments again, the wording is testable without the yargs harness, and a second caller -- an agent resuming a review never runs step 1 -- is one line. * fix(review): align the twin walk, and stop a test from passing on nothing The build side still inferred "this root is a file" from `readdirSync` raising ENOTDIR, one commit after the check side stopped doing exactly that and said why. A platform that maps the case differently would drop `commands/review.ts` from one digest and not the other, and a byte-for-byte correct bundle would warn on every review forever, on that platform alone, with rebuilding reproducing the same one-sided walk. Both sides ask `statSync(...).isFile()` now. Fixing one half of a pair and not the other is the mistake this file keeps making. The filename parity test had been passing on nothing since the previous commit: it matched `writeFileSync(join(distDir, '…'))` against the script's source, the literal moved into a `stampPath` variable, and the regex returned `undefined` so the assertion compared against nothing. It runs the build against a fixture now and reads the name off `dist/`, so it measures what the build does instead of what its source looks like. Renaming the stamp on one side reddens it. Also from review: the duplicated comment block in `parse-args`; an unreadable source now says the check could not run rather than passing in the same silence as an installed package, which is what the docstring already promised; the "could not check" line no longer asserts that the checkout predates the feature, since the build has three refusal paths and one of them means the opposite; every refusal removes an existing stamp, because leaving an older attestation beside a newer bundle is a weaker form of the certifying it refuses; and `drive` calls the check, which the module comment argued for and the diff had not done -- a resumed review never runs step 1, and that is where the long work starts. * fix(review): pin the regex group the parity tree missed, and say source, not command * fix(review): allowlist what the bundle holds, and cover the drive notice (#8390) * fix(review): treat unreadable review sources as unmeasured (#8390) * test(review): pin the stamp guard mutations that survived the suite (#8390) * fix(review): close staleness-check gaps and pin the round-4 survivors (#8390) * fix(review): close round-5 staleness gaps for parity, refusals, and partial checkouts (#8390) * fix(review): close round-6 gaps in the clause classifier, symlink layout, and pin honesty (#8390) * fix(review): close round-7 gaps in the closure oracle, parity pin, and refusal pins (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): close round-8 gaps from the maintainer review (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): close round-9 nits from the maintainer review (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): pin the lease root in the synthetic digest parity case (#8390) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen Autofix <autofix@qwen-code.dev> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-07 03:21:26 +00:00
qqqys	03eb5043cc	fix(dingtalk): keep status cards continuous and attributable (#8565) ... * fix(dingtalk): keep status cards continuous during runs * fix(dingtalk): render attributable markdown replies * fix(dingtalk): harden status card attribution and fallback paths (#8565) * fix(dingtalk): deliver boundary content reliably and harden card fallbacks (#8565) * fix(dingtalk): halt refreshes on dead cards, keep delivered content (#8565) The per-second status chain kept pushing metadata updates after the content stream latched failed, and once the 3-failure breaker tripped an idle card could never revive. Stop the chain on a latched stream failure and keep a low-frequency probe so a recovered metadata API revives it. Also re-send boundary content declared delivered via the card when a failed/cancelled terminal overwrites the continuity card, give a re-latched status context a fresh segment id after input_requested so a later failure still reaches the card failure UX, dedup inline sender echoes, and require the 'I' of IMAGE in the partial-marker regex so a bare trailing '[' survives in final fallback text. Consolidate the duplicated content-cap constants and pin the reviewed delivery, drain, breaker, and attribution behaviors with tests. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-06 22:42:42 +00:00
Shaojin Wen	e76dff1c6b	feat(review): add declarative repository-context manifest (#8401) ... * feat(review): add OpenJDK repository context Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * refactor(review): extract repository context foundation Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): repair CI type guard and add manifest repository context Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): harden repository context per maintainer review Address both maintainer reviews on the repository-context PR: - repo-context: a PR plan whose merge base never resolved (mergeBaseSha: null) now degrades to a null artifact without consulting the worktree, instead of throwing a misleading "invalid plan" error or falling back to the PR head. - Identity reads return the same shape in PR and local modes (CRLF->LF, trimmed) and fail closed: absence yields null, a present-but-unreadable file throws. - Context-required roles can no longer override the roster's effort, topology, and mode gates. - The relatedPaths scan bound rises from 1024 visited entries to 16384 and is documented, so honestly scoped manifests no longer abort reviews. - A present-but-invalid repositoryContext now fails closed in every consumer; the gate no longer silently drops the disclosure. - The duplicated validators and bounds are shared between the wire format and the manifest provider; the context role allow-list is derived from a single const; manifest arrays no longer require hand-sorting (uniqueness only). - Nits: dead mkdir removed, output message names the provider, escape-message fix, unsafe changed paths skip instead of aborting, segment-glob regexes memoised, list helper hoisted. - Docs: user-facing manifest section, trust-boundary residuals and foundation status in the design doc, fail-closed exit guidance in the skill. * fix(review): skip unsafe related paths in manifest context (#8401) * fix(ci): align review timeout helper test with externalized variables (#8401) * fix(review): harden repository context bounds and base identity reads (#8401) * fix(review): bound manifest matching work and pin round-2 review gaps (#8401) * fix(test): isolate serve streaming suite from stray workspace settings (#8401) * fix(review): cap identity reads, bill match work by length, pin round-3 gaps (#8401) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-06 15:13:31 +00:00
Shaojin Wen	477859bb3f	feat(channels): support local gh authentication (#8461) ... * feat(channels): support local gh authentication Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(channels): align registry catalog test and visuals with optional GitHub token (#8461) * fix(channels): address review feedback for GitHub local gh auth (#8461) Treat a blank replacement of an optional secret as a clear so an existing GitHub channel can no longer ship an empty or whitespace-only PAT to the daemon. Reuse the shared missing-field predicate in the editor's GitHub credential validation, wrap malformed baseUrl failures in an actionable channel error, and surface sanitized gh stderr in local authentication failures. * fix(channels): address second-round review feedback for GitHub local gh auth (#8461) Pin the whitespace-only token gate, the bounded gh stderr sanitization, and the required-secret blank-replacement guard with mutation-resistant tests. Log the authenticated account identity on channel connect so an out-of-band gh auth switch is visible to operators. Align test secret-source fixtures with the SDK union and complete the design doc's change footprint. * fix(channels): address third-round review feedback for GitHub local gh auth (#8461) * fix(channels): address fourth-round review feedback for GitHub local gh auth (#8461) * fix(channels): address fifth-round review feedback for GitHub local gh auth (#8461) * fix(channels): address sixth-round review feedback for GitHub local gh auth (#8461) * fix(channels): address seventh-round review feedback for GitHub local gh auth (#8461) * fix(channels): address eighth-round review feedback for GitHub local gh auth (#8461) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-05 14:05:00 +00:00
Shaojin Wen	a123d0030a	ci(review): prepare evidence-image tooling for GitHub-triggered reviews (#8454) ... * ci(review): prepare evidence-image tooling for GitHub-triggered reviews Reviews triggered on GitHub cannot attach images today for three reasons; this wires the two that live in the workflow: - Install tmux and freeze (pinned, checksum-verified) before the review runs. Both are optional by contract — the evidence ladder degrades honestly without them (png -> ans-only -> refused, recorded in the capture manifest) — so the step never fails the review; it only decides which rung the runner can reach. tmux mirrors the tolerant install qwen-autofix.yml already uses; freeze falls back to ~/.local/bin when passwordless sudo is absent. - Pass QWEN_REVIEW_ASSETS_REPO from a repository variable to the review step. Publishing stays OPT-IN by design: with the variable unset the env is empty and publish-assets refuses (parseAssetsRepo trims and rejects empty), so nothing changes until a maintainer sets the variable. When set, evidence images land on commit-pinned pr-assets/<pr>-review branches — already covered by the visuals cleanup workflow — pushed with the same CI_BOT_PAT the step uses. The third reason is release lag: the capture producer (capture-tui, #8388) has to merge and ship in a release before rendering claims can generate images on CI at all. This change is inert until then. * fix(ci): capture-tools step review fixes — enforced tolerance, version pin, cached fallback R1-1: the never-fails contract is now enforced twice — continue-on-error at the YAML level (the belt) and set +e with a trailing exit 0 inside (the suspenders); under the runner's default bash -e several statements (mktemp, install, sudo install with an empty path) could previously abort the step and fail the review the comment promised never to fail. R1-6: probe the VERSION, not just the binary — on a persistent self-hosted runner an installed freeze made any FREEZE_VERSION/SHA bump a silent no-op; the pin now forces a refresh when the cached binary does not match. Cached-fallback fix: put ~/.local/bin on PATH (and GITHUB_PATH) before the probe — a sudo-less runner otherwise re-downloads the tarball on every review run forever. R1-3: the step comment says capture-tui is UPCOMING (#8388, not in the released CLI) and names qwen review drive as today's tmux consumer, so the step cannot be mistaken for stale dead weight and deleted from under the follow-up. R1-4: the retention comment scopes the cleanup-workflow claim to the same-repository designation; a fork or scratch destination manages its own retention (docs updated to match, plus a note documenting the repository VARIABLE a maintainer sets to enable publishing). R1-5: the step's real bash now runs in the workflow behavioural harness under bash -e with stubbed sudo/apt/curl/sha256sum/tar/uname: worst-runner and checksum-reject scenarios exit 0 installing nothing, the no-sudo happy path pins the ~/.local/bin + GITHUB_PATH pairing, and the version-pin probe is pinned from both sides (wrong version re-downloads, matching version skips). Real freeze/sudo on a developer machine are shadowed so the tests are deterministic and can never install to /usr/local/bin. Nit: both sudo guards now check sudo -n true. * fix(ci): capture-tools step review fixes — step-owned tool dir, anchored probe, honest failures Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): capture-tools test harness — shadow tmux, don't blank its PATH dir The harness dropped every host PATH directory that ships a tmux so the step's apt branch would depend on the scenario, not on the machine hosting the suite. On GitHub-hosted ubuntu runners tmux lives in /usr/bin, so the filter blanked /usr/bin wholesale — bash, grep, mkdir, and tar included — and execFileSync('bash') died of ENOENT: all seven capture-tools tests failed in the Test (ubuntu-latest Node 22.x) job while passing on tmux-less dev machines. Replace the directory-level drop with an entry-level shadow: each tmux-bearing directory is mirrored (symlinks) into a scratch dir minus the tmux entry, in place, preserving PATH order and the empty-entry stripping the old filter did. Hosts without tmux take the map through unchanged, and Windows (no tmux in its PATH, no symlink branch) keeps its current behavior exactly. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): capture-tools test review fixes — faithful stubs, env shape pins, missing-branch scenarios Round-3 review findings: the harness executed several of the step's branches but asserted nothing about them, so probe-verified mutants (dropped tmux guard, deleted warning/degradation messages, malformed or missing FREEZE_VERSION/FREEZE_SHA256, wrong hash variable, dropped URL `v` prefix or curl `-L`, severed tarball paths, broken regex boundary, leaked mktemp dir) all shipped green. - Make the curl/sha256sum/tar stubs model their real contracts: exact pinned URL, pinned checksum over a file curl actually wrote, existing -xzf operand - Pin FREEZE_VERSION/FREEZE_SHA256 shape in captureToolsSource - Pin the full curl flag set and the three-site tarball path agreement - Assert the stale-renderer warning (fires on degraded re-download, silent on the happy path) and the tmux-unavailable message - Pin TMPDIR and assert the mktemp cleanup leaves it empty - Add the two missing scenarios: tmux-present skips apt, cached version extending the pin with a leading digit re-downloads Verified by 13 mutation probes: every named mutant now turns the suite red (13/13 killed), baseline 34/34 green. * fix(ci): capture-tools step review fixes — hash-verified cache, per-run PATH promotion * fix(ci): capture-tools step review fixes — verified-bytes-only installs, step timeout Review findings on the capture-tools step: - Drop the PATH-trust branch: a freeze already on PATH was accepted on its own --version and executed to probe it — exactly the self-report the FREEZE_BIN_SHA256 comment declares attacker-controllable, from dirs writable between jobs on both runner classes. The checksummed download always runs now; the cache makes it free after the first run. - Guard $tools_bin in the download branch: with mktemp failing, the unguarded install resolved to /freeze — harmless unprivileged, but a root-in-container self-hosted runner writes it and reports success with nothing on PATH. - Copy-then-verify the cache: install into the fresh per-run dir FIRST, verify THOSE bytes, delete both copies on mismatch — the verified bytes are the bytes later steps execute, closing the check-then-copy race for free. This makes the separate pre-verify block redundant; it is deleted. - Add timeout-minutes: 5 — continue-on-error bounds failure, not duration, and a stalled `sudo apt-get update` mirror had no other bound under the 300-minute job cap. - Report block: say the resolved freeze is likely broken when its --version produces nothing, instead of echoing a blank line and calling it stale; the mismatch wording is direction-neutral now. Tests: replace the PATH-trust scenario with a planted-PATH one (marker outside the scenario dir proves the plant never executes), add the mktemp-failure scenario (the install stub succeeds like root would, so the unguarded mutant is caught) and the promoted-dir 0700 assertion; re-anchor the two digit-boundary tests on the report's warning. 41/41 green; both fix mutants verified killed. * fix(ci): capture-tools review fixes — stale-dir cleanup, pinned guards Address round-5 review: - R5-1 (Critical): the per-run qwen-review-tools.* dir under RUNNER_TEMP was never removed; RUNNER_TEMP survives across jobs on the shared pool, so every review run accumulated one dir + one Go binary, unbounded. 'Clean stale agent state' now removes stale dirs before the install step creates the current run's dir, matching the qwen-triage.yml convention. The harness comment claiming the dirs were runner-cleaned is corrected. - R5-6: the cache re-verification rejection branch now logs why it deletes the cached binary instead of degrading silently. - R5-7: bump-checklist note beside the freeze pins — the harness stubs key on the same env values, so a transposed hash pair must be caught against the real release artifacts at bump time. - R5-2/R5-3/R5-4/R5-5: four unpinned step properties now pinned (the if: guard, the sudo -n probe flag, install-after-context ordering, and the cache branch's tools_bin guard via a new mktemp-fails scenario); six mutation probes confirm each pin kills its mutant. * fix(ci): capture-tools review fixes — curl budget, swept scratch dir, wiring pins * fix(ci): capture-tools review fixes — harness mutation pins, pin-pair self-check Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): capture-tools review fixes — shadow-farm cleanup, backoff budget term * fix(ci): capture-tools review fixes — report probes only installed freeze, age-gated sweep --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-05 13:16:56 +00:00
Shaojin Wen	1a2bb10298	fix(review): admit evidence images by content, not by name — magic-byte sniffing (#8459) ... * fix(review): admit evidence images by content, not by name — magic-byte sniffing The publish-assets allowlist was extension-based, and an extension is a claim anyone can make: combined with a prompt-injected review run, whatever could name a file evidence.png could host up to the size cap of arbitrary bytes at a github.com URL through the evidence push (raised in #8454's review as an enablement consideration). sniffImageFormat reads the four admitted signatures (PNG, JPEG, GIF87a/89a, RIFF+WEBP — RIFF alone is not enough, AVI and WAV share the container prefix) and validateAssetContent rules the content against the format the extension claims, fail-closed: an unrecognized signature refuses even when the extension is allowed. publish-assets applies the ruling to every file's first bytes before anything is uploaded — all-or-nothing, same refusal contract as the other gates. Pinned: the four signatures, truncated/empty headers, the RIFF/AVI near-miss, extension-format mismatch, fail-closed unknown extensions, and end-to-end: a shell script named evidence.png refuses with exit 3 and nothing pushed. * fix(ci): re-pin review timeout tests to vars externalization (#8459) * fix(review): single-source the asset allowlist and pin sniff depth (#8459) Address round-1 review suggestions: - ASSET_EXTENSIONS now derives from EXTENSION_FORMAT, so admitting a format is a one-place change the batch gate and the content gate cannot drift on; the duplicated extension extraction moves into one claimedExtension helper shared by both gates. - Pin what the mutation probes showed unpinned: the GIF87a branch (sniff + admission), uppercase extensions at the content gate, the refusal message direction, the full depth of every signature (near-miss negatives), and the publish-time 16-byte slice end to end (a WEBP publishes through runPublishAssets; verified the test fails when the slice shrinks to 8). * fix(review): pin every sniff check and guard the allowlist lookup (#8459) * fix(review): pin every sniff byte and the two-gate format coupling (#8459) * fix(review): align the two-gates comment with the pin that enforces it (#8459) * fix(review): sharpen evidence-gate diagnostics and pins (#8459) - Content refusals now name the JSON-quoted full path, so two same-named files from different directories are tellable apart (the sibling read-error refusal already spent the path). - Export ASSET_HEADER_BYTES from the lib that owns the sniff depth; the publish call site uses it and the two-gates pin slices canonical headers to it, so a future longer signature fails the pin, not real publishes. - One shared refusal builder keeps the allowlist message identical in validateAssetFile and validateAssetContent. - Narrow the sniffImageFormat threat model to what magic bytes buy: binds the claimed type to the leading bytes, does not stop prefixed payloads. - One-byte-off matrix becomes a labeled it.each table (failures name the exact corrupted byte); imports re-alphabetized. * test(review): pin the shared extension refusal across both asset gates (#8459) * test(review): pin lastIndexOf extension parsing for multi-dot asset names (#8459) * fix(review): admit WEBP by its fourcc and name refused files once (#8459) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-05 02:23:25 +00:00
Shaojin Wen	ac67de2e46	fix(review): stop the reverse-audit loop while there is still time to report (#8468) ... * fix(review): stop the reverse-audit loop while there is still time to report Measured on CI run #8368 (+1699 lines): the iterative reverse audit ran to its 5-round cap, each round a per-chunk fan-out whose findings then went back through verification, and the loop consumed 3.5 of the job's 4 budgeted hours. The outer GNU-timeout kill arrived while round 5's findings were still being verified. The review died holding every confirmed finding it had; nothing reached the pull request. The loop's rounds are driven by the orchestrator, but every round begins at the same place: agent-prompt building the round's prompts. So the builder becomes the loop's clock. When the environment carries a review deadline (QWEN_REVIEW_DEADLINE_EPOCH, exported per attempt by the review workflow) and the remaining time is inside the reserve kept for the last verification, compose-review and submission (default 60 minutes, QWEN_REVIEW_DEADLINE_RESERVE_SECONDS to override), a reverse-audit round is refused: a BUDGET line on stderr, exit code 4, no prompt built and no record written. The message carries the exact unreviewedDimensions entry to file, so the disclosure that caps the verdict is the CLI's text, and Step 6 proceeds with the findings already confirmed. Local runs have no deadline and are untouched. A malformed deadline fails open — the outer kill still bounds the run, and a broken variable must degrade to today's behaviour rather than wedge every budgeted review at round 1. The verifier is deliberately not gated: the reserve exists so it can run. * fixup: scale the deadline reserve to the externally-chosen budget The budget is not this workflow's to assume: it arrives from a repository variable, a workflow input, or a /review --timeout=N comment. A fixed 60-minute reserve would consume most of a 70-minute budget and refuse the audit loop outright on a 30-minute one. The workflow now passes a reserve of a quarter of the attempt, floored at 10 minutes and capped at 60; the CLI constant remains only the fallback for a caller that sets a deadline without a reserve. * review feedback: admit the round only if IT fits, and cap deterministically Three findings from review, all taken: 1. The gate budgeted for the tail but not for the round it admits — the terminal round is by construction the one that starts closest to the boundary, so the killed-mid-verification failure survived one round wide. The gate now requires remaining >= round + reserve, where the round's cost is the previous round's, measured admission-to-admission from a stamp the builder writes (one per round; a same-round rebuild is not a round), falling back to a 30-minute constant for round 1, which starts with the most headroom. 2. The refusal was deterministic; the disclosure that caps the verdict was prose the orchestrator had to carry. The builder now records a budget-stop marker beside the prompt records and compose-review synthesizes the unreviewedDimensions entry from it — deduped against a relayed copy — so a run that drops the sentence still cannot approve past a truncated audit. 3. Exit code 4 is documented in the command's describe. Also restores the Step 5 bullet the previous commit's edit displaced (new findings merge into the cumulative list before the next round). * review feedback: pin the budget gate's all-chunks refusal and ordering Cover the two behaviours the review noted were only asserted on the bare --findings form: an exhausted budget refuses the loop's real --all-chunks round before ANY of the per-chunk records is written, and a malformed call (--round 0) still gets its validation error first — exit 4 is for a well-formed round the budget refuses, never a replacement error. Also name what the code already does: reserve=0 is the deliberate escape hatch (the gate shrinks to the round estimate alone), and the workflow's 3600s cap mirrors DEFAULT_RESERVE_SECONDS. * docs(review): describe the soft-deadline env vars for time-budgeted runs The review noted the two new variables appeared in no user-facing doc; the reserve in particular is an operator-facing knob. State what each does, the fail-open posture, and how the refusal surfaces in the verdict. * fix(cli): align budget-stop disclosure with the gate's refusal (#8468) A round-1 budget refusal left no reverse-audit records, so the Step 4/5 floor reported the deliberate stop as a rogue/unlaunched audit with a rebuild FIX the same gate deterministically rejects; the refusal's own disclosure was swallowed by the caller-echo dedup. The floor now stands down when the budget-stop marker exists, and compose-review renders the disclosure structurally, bilingually, from the marker. Also: `--role reverse-audit` requires `--round <k>` (an unlabeled admission stamps an entry no estimate can attribute), the budget gate runs after the plan/findings reads (a broken plan or unreadable findings deserves its own error, and nothing is stamped ahead of a buildable call), and the gate's admission boundary, measured-cost behaviour, and the workflow env contract are pinned by tests. * review: a budget stop excuses only the round it refused The budget-stop suppression keyed on the marker's existence alone, so every reverse-audit gap shape went silent once any round was refused — including the shapes that describe rounds which RAN before the budget hit. A hand-written round-1 launch is exactly as undelivered when round 3 later hits the budget, and suppressing its disclosure let 'stopped before round 3' imply the rounds that did run were faithful. Exactly one shape is by design under a marker: not-built — the refusal writes no record, so an audit with no records IS the audit the gate stopped, and its FIX (rebuild the round) would be refused by the same gate. The suppression now names that shape and no other; a rewritten, unlaunched or brief-unread round keeps its disclosure and its repair. The new test pins the operative halves: the verdict stays capped, the marker's disclosure posts, and the operator channel carries the rewritten round's exact repair. (The posted body collapses same-subject disclosures — both say 'reverse audit' — so the author sees the stop; repairs are acted on from stderr, where the rewritten fix rides.) * fix(review): fence budget state per run, and let gate errors beat budget stops Address the round-2 review threads on the reverse-audit budget gate: - Fence budget-rounds.json and budget-stop.json by the plan's own mtime. Every run rewrites the plan at its Step 1 capture, so records older than the plan belong to a previous run of the same PR: a run killed before cleanup no longer prices the next run's rounds off stale stamps (an hours-old stamp read as an hours-long round refused round 1 of a fresh budget) and no longer caps a later run's verdict on a stop that did not happen in it (R2-1, R2-2). - Refuse a structurally unbuildable plan (no chunks[], duplicate or non-integer ids) with its own error ahead of the budget gate, so the same corruption gets the same diagnosis whatever the clock says, and no budget-stop marker is written over a corrupt plan (R2-5). - Stamp a round admitted only after its build succeeds: a build that throws leaves no stamp, so the next round's cost is never measured from a build that produced nothing and floored to 600s (R2-6). - Keep the budget entry's 'reverse audit' subject out of the caller-echo prefix filter: other reverse-audit scopes the orchestrator disclosed (a twice-whiffed chunk from the rounds that DID run) are no longer silently dropped in the marker's shadow; the marker's own relays stay deduped by the phrase splice (R2-7). - Render --round unbracketed in the reverse-audit rebuild fix — the CLI refuses a round-less reverse-audit call, so the paste-and-run repair must not present the flag as optional (R2-14). - Document the deliberate one-verification overlap between the measured round estimate and the tail reserve, at both definitions (R2-13). - Test hardening, each assertion mutation-probed to fail its named mutant: a reshaped relay only the marker-phrase splice dedups (R2-8); the stamp's round label and the verifier's no-stamp invariant (R2-9); whole-line, unit-arithmetic and reserve-cap pins on the CI wiring contract (R2-10); the first-wins stamp survivor (R2-11); the reserve=0 escape hatch (R2-12). --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-04 13:58:15 +00:00
Dragon	48d37cdf70	docs: document headless Goal workflows (#8503)	2026-08-04 03:30:26 +00:00
Shaojin Wen	7dfc554dff	feat(review): Add structured Web Shell review results (#8402) ... * feat(review): add Web Shell review artifacts Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(web-shell): add code review artifact visual scenario (#8402) * fix(review): address Web Shell review artifact feedback (#8402) * save-artifact: document why paths resolve against the daemon workspace root (QWEN_CODE_PROJECT_DIR) instead of cwd, and cover the relative-path form the skill documents with a test where the two roots differ. * CLI/renderer contract: the renderer hand-duplicates the findings vocabulary and fails closed on unknown values, so name the renderer as a second consumer beside the CLI's lists and check in a contract fixture generated through the real pipeline (validateFindings -> buildReport -> save-artifact) that exercises every source, severity, confidence and outcome. Exporting the vocabulary through the SDK stays deferred: it is a public cross-package API change beyond this PR's seam. * resolve-anchors now validates `line` exactly like `findings` does (positive safe integer); the two validators in one pipeline no longer disagree. Note: an in-flight `.qwen/tmp` findings file carrying `line: 0` fails where it previously did not. * The renderer validates markdownReportPath (relative, no ".." segments, .md suffix) before it becomes a readWorkspaceFile call, resets the severity/confidence filters when switching artifacts, and surfaces heldByMeasurement so a nonzero Held count is attributable. * save-artifact refuses low effort structurally (choices and library guard) instead of by prose, stats the Markdown report before reading it so a directory reports "not a file", and the component no longer shadows the DOM `document` global. * The case-insensitive alias test now skips visibly on case-sensitive filesystems instead of passing vacuously. * Comment the kept `turnOutputs.review` key and document the JSON companion in the user docs. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): address second Web Shell review artifact feedback round (#8402) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-03 16:08:13 +00:00
jinye	0cb109f513	fix(core): Avoid replaying unsafe MCP tool calls (#8387) ... * fix(core): Avoid replaying unsafe MCP tool calls Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): Revalidate MCP replay after reconnect Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-03 11:04:38 +00:00
Shaojin Wen	186812694c	feat(review): publish evidence images to a user-designated assets repo (#8351) ... * feat(review): publish-assets — evidence images for PR review comments GitHub's API cannot attach images to review comments (the web UI's drag-and-drop upload has no API equivalent), so a review whose evidence is a screenshot — a TUI rendering, a before/after comparison — had no way to show it. New `qwen review publish-assets` hosts evidence images in a user-designated repository and hands back URLs a comment can embed. Grew from the maintainer's manual workflow (screenshots pushed to `pr-assets/<PR>-verify` branches over HTTPS), and inherits the shape of the skill's only other public write (`submit`) deliberately: - Designated destination: writes only to QWEN_REVIEW_ASSETS_REPO, an owner/repo the user set by hand — the reviewed repo for maintainers, a fork or scratch repo otherwise (fork-vs-in-repo becomes a configuration difference, not two code paths). A separate variable from QWEN_REVIEW_SCRATCH_REPO on purpose: that contract forbids PR-derived content, and evidence screenshots are exactly that. Unset → exit 3. - Authorised run: the same args-file re-parse and target binding as submit, now extracted to a shared lib/authorization.ts so the two gates cannot drift (the target-binding lesson lives in one place). Since an effective --comment forces high effort, low/medium runs can never publish. - Images only, capped, all-or-nothing: extension allowlist (SVG excluded — a script container), per-file and per-batch size caps, one refused file refuses the batch before anything is pushed. - Immutable references: files land on pr-assets/<pr>-review via the Contents API (HTTPS via gh; no clone, no SSH), content-hash-named so re-runs are idempotent, and every URL is pinned to the commit — a posted comment's evidence cannot be changed from under it. The web-host /raw/ URL form works unchanged on GitHub Enterprise. - Auditable: a manifest names every file pushed and the landing commit, swept by cleanup with the other review artifacts. The findings artifact gains per-finding `assetFiles` (local evidence paths) and `assets` (published URLs); `publish-assets --findings/--findings-out` publishes everything referenced and weaves the URLs back in, so the comment builder reads the artifact rather than hand-carrying URLs. What the command cannot check is stated in SKILL.md instead: image content. Publish only evidence the review itself produced — never a capture of the user's own terminal, which can hold an env dump in the scrollback. Tests: 45 files / 1394 assertions — new suites for the assets naming and validation rules and the command's gates (refusal without designation, refusal without authorisation, target binding, branch creation, idempotent re-run, batch refusal, findings weaving); submit's 42 pass unchanged on the extracted gate. * fix(review): publish-assets round-1 self-review — six findings Round-1 review of this branch, walked with the angles the author-side pass does not cover: - submit.ts kept its parseReviewArgs import after the authorization extraction; vitest does not typecheck, `tsc --build` does, and CI's build leg failed on TS6133. (The whole first CI round's failures cascade from this one break.) - ensureBranch %2F-encoded the slashed ref path; GitHub's documented form is literal slashes and %2F routes inconsistently across endpoints — a 404 here reads as "branch missing" and turns every re-run into a 422 on the create. Ref paths are now literal (the branch name is built from a validated integer, so interpolation is safe); the contents `?ref=` query VALUE keeps its encoding, which is the correct position for it. - The authorization gate bound URL-shaped `--comment` arguments against the ASSETS repo, refusing legitimately authorised runs whenever the assets repo is a fork rather than the reviewed repo. The shared gate's repo binding is now optional — submit still always binds it; publish-assets binds the PR number (and host) alone, with a new optional --reviewed-repo to restore the stronger binding when the orchestrator knows the reviewed repo. - URLs were pinned to the last PUT response's commit.sha; on an identical-content update that field's shape is GitHub's to decide, not ours to assume. The head is now read from the branch ref after the uploads — one extra call for independence from the response shape. - putContent's catch-all retried EVERY failure through the exists path, answering a 401 with a confusing secondary error from the sha lookup; the retry now fires only on the 422/needs-sha shape and rethrows the rest. - --findings without --findings-out silently skipped the URL weaving; it now warns, and --findings-out implies --findings. New tests: literal-ref assertion, non-exists rethrow, URL-shaped authorisation without assets-repo binding, --reviewed-repo mismatch refusal. 45 files / 1399 assertions green; `tsc --build` clean. * fix(review): publish-assets round-2 — empty-findings no-op, reviewed-repo hint Round-2 findings on this branch: - A findings artifact carrying no assetFiles is the ORDINARY case for most reviews, but publish-assets answered it with exit 3 — a refusal an orchestrator calling the command unconditionally on every posting run would read as a failure to repair. It is now a no-op (exit 0, {published:false, count:0}); a bare --files with nothing named keeps the exit-3 refusal, because there the emptiness IS the caller error. - SKILL.md's example now names --reviewed-repo for URL-target reviews, so the stronger authorisation binding is used where the orchestrator knows the reviewed repo. 44 files / 1387 assertions green; tsc --build clean. * test(review): fix invalid two-argument expect in assets.test.ts Round-3 sweep: vitest's expect takes one argument — the message-style second argument was a lint error and a weak assertion both. The offending value now rides inside the asserted object, so a regression names which shape slipped through instead of reporting 'expected true'. * test(review): pin the findings schema's evidence-asset validation directly Round-4 sweep: assetFiles/assets were exercised only through publish-assets' weaving test — the schema's own rejection paths (non-array, empty-string entry, empty-array drop) had no direct case, so a validation regression would have surfaced as a confusing weaving failure two layers up. * fix(review): address all six findings from the automatic review (R1-1..R1-6) The /review pipeline's own round-1 findings on this PR, each confirmed and fixed: - R1-1 (the real catch): the host-binding check sat nested inside the `req.repo !== undefined` guard, so a caller omitting --reviewed-repo also silently skipped the HOST binding — contradicting the documented "binds the PR number (and host) alone". The host check now stands on its own; a new test pins an Enterprise-host mismatch refusal with the repo binding absent. - R1-2: --pr accepted whatever yargs `type:'number'` passed through (NaN, 0, 3.5), and --user-authorized bypasses the gate that would have re-parsed the target — `pr-assets/NaN-review` was reachable. A Gate-0 positive-integer check now refuses first, matching submit's sibling discipline. - R1-3: the suite drove the skillArgs seam without clearing QWEN_CODE_SESSION_ID, so running it inside an active Qwen Code session spuriously failed eight tests. beforeEach now saves/clears the variable and afterEach restores it. - R1-4: the 40MB aggregate cap was enforced inline and untested (a mutation deleting it stayed green). The per-file rules and the total cap now live in one pure ruling, validateAssetBatch, unit-tested with five 9MB sizes and no fixtures. - R1-5: the asset_files snake_case alias was the one untested member of an otherwise-tested alias family; pinned. - R1-6: the setGhHost wiring had no command-level assertion; a GHE test now pins both the call and the host-carrying manifest URLs. 44 files / 1397 assertions green; tsc --build and eslint clean. * fix(review): address all ten round-2 findings from the automatic review Round-2 of the /review pipeline on this PR: 2 Critical, 8 Suggestions, every one confirmed against the code and fixed. Criticals: - The round-2 test block added for the empty-findings no-op omitted the QWEN_CODE_SESSION_ID save/delete/restore its two sibling blocks perform, so the suite spuriously failed inside an active Qwen Code session — the exact dogfooding environment this repo reviews from. - The gh routing and the returned URLs read the host from two different sources: with --host absent, gh children inherit an operator-exported GH_HOST (routing at Enterprise) while rawAssetUrl defaulted to github.com — every returned URL a 404. One effectiveHost (flag, then GH_HOST env) now feeds both. Suggestions: - putContent's retry discriminator matched a bare `422` anywhere in err.message — which execFileSync fills with the full command line, including the PR-numbered remote path: evidence for PR #4220 would read a 401 as "already exists". Anchored to `HTTP 422`. - ensureBranch's bare catch read every ref-lookup failure (401, 403 rate-limit) as "branch missing"; only HTTP 404 takes the create path now, and an empty assets repo — whose default_branch resolves while its head ref 404s — is named as the condition it is, with the fix stated. - Validation refusals threw (yargs exit 1, stack trace, empty stdout) while every other gate in the command answers exit 3 + {"published": false}; unreadable files and batch refusals now speak the same refusal language. - The command's idempotent writes (content-hashed PUTs, a ref create whose duplicate is tolerated) now go through a new ghWithInputRetried — sharing gh()'s transient-error retry — and ghWithInput's no-retry docstring names the two-caller split instead of claiming a sole caller. - parseAssetsRepo admitted dot-segment repos (`owner/..`) its docstring claimed were path-safe; segments now exclude `.`/`..`, mirroring submit's isRepo. - stringArray accepted whitespace-only evidence paths; trim(), matching the sibling asString. - The GHE test asserted setGhHost was called but not WHEN; it now asserts the call precedes the first API invocation. 44 files / 1403 assertions green; tsc --build and eslint clean. * refactor(review): one refusal helper for every publish-assets gate Round-2 of this branch's fresh review: the refuse() helper existed below seven inline copies of the identical three-line refusal — the drift shape where one site eventually forgets the exit code. Hoisted to the top of runPublishAssets and used by every gate; message content unchanged where tests pin it. 26/26; tsc clean. * fix(review): address the round-3 review — bidirectional host binding and 14 more The automatic review's third round on this PR: 1 Critical + 14 Suggestions, each verified and addressed. The Critical (host binding, both halves): - The gate's `req.host &&` guard bound the host in one direction only — an Enterprise-URL authorisation admitted a host-less write routed at github.com (or wherever GH_HOST pointed). The gate now compares the authorised host against the write's EFFECTIVE host, defaulting an absent req.host to github.com: a host is a host, not an exemption. - Both callers fed the gate the flag rather than the route: publish-assets computed effectiveHost (--host ?? GH_HOST) AFTER the gate and bound args.host; submit bound args.host while its gh child inherited GH_HOST. publish-assets now resolves effectiveHost before Gate 2 and binds it; submit binds the same resolution. The rest: - pr-assets/<N>-review registered in the asset-branch cleanup workflow, per its own every-producer-must-be-added-here rule — a branch nothing deletes is permanent. - ghWithInputRetried had been inserted between ghWithInput and its JSDoc, leaving the does-NOT-retry comment attached to the function that DOES retry; each function now carries its own doc. - putContent's retry-path contents-GET is wrapped: when the 422 was not the sha-missing shape and the path does not exist, the GET's 404 no longer replaces the PUT error the user needs. - stringArray treats null as absent like every sibling parser, so an artifact rendering "no assets" as null canonicalizes instead of crashing. - Test isolation, all four describe blocks: GH_HOST save/delete/restore, setGhHostMock.mockReset (a sibling's persistent throwing implementation survives mockClear — the malformed-host test also switched to mockImplementationOnce), and full mock resets in the blocks that lacked them. - The two regression-pin tests the review measured vacuous now discriminate: each fails only the one call under test and asserts the pipeline stopped THERE (no contents PUT after a bad create; exactly one gh call after a 403 lookup). - New positive pins: a double-fired create ("Reference already exists") succeeds; the canonical report shape this command's own --findings-out writes round-trips; an Enterprise-URL authorisation refuses a host-less write while a github.com-URL one passes it. Not changed: the finding that reverting the Finding-interface hunk leaves tests green — the fields are type-level and their removal fails tsc --build (the CI leg that caught this branch's own TS6133); a runtime pin would duplicate what the round-trip tests already exercise. 47 files / 1495 assertions green; tsc --build 0 errors; actionlint clean on the cleanup workflow. * fix(review): address the round-4 review — empty-GH_HOST passthrough and four test pins Round 4 came back COMMENTED (down from CHANGES_REQUESTED), 5 Suggestions, 0 Criticals — all five confirmed and fixed: - An exported-but-empty GH_HOST ("" from an unset workflow var) survives `??`, being non-nullish: effectiveHost became "" and the gate compared the authorised host against "", refusing a legitimate github.com write. Both call sites now collapse an empty trim to undefined (`|| undefined`, parenthesized). - The gate's URL-shaped repo/host binding was exercised only via publish-assets' suite; submit.test.ts now pins both directions of the host binding and the repo binding at its own call site. - ghWithInputRetried had no retry-contract test; gh.test.ts adds the symmetric block to ghWithInput's does-NOT-retry pin (transient 500 retried once then succeeds; non-transient 401 single call). - The publish-assets mock aliased ghWithInput and ghWithInputRetried to one mock, hiding which variant a write used; they are two mocks now, and the happy path asserts the non-retrying variant is never touched. - The Prepared interface's dead `name` field is gone. 46 files / 1476 assertions green; tsc --build and eslint clean.	2026-08-02 15:49:32 +00:00
Shaojin Wen	09d818867e	fix(review): follow output language in Tip lines and saved reports (#8370) ... * fix(review): follow output language in Tip lines and saved reports The /review skill's critical rule 2 already states that terminal output follows the user's output language preference, but three areas lacked explicit guidance, causing the model to output them in English even with a Chinese output language configured: - Follow-up Tip lines (e.g. "Tip: type post comments to ...") were specified as English templates with no translation instruction - The Step 8 saved report file used English section headings and prose - Step 6 labels (e.g. "Balanced review (effort: medium)") had no translation note Add explicit output-language guidance at each point of use, with a Chinese example for Tip lines and Chinese section heading examples for the saved report. Command keywords (post comments, fix these issues) stay verbatim since they are trigger phrases the user types back. * fix(review): disambiguate findings-artifact language boundary and complete low-effort translation notes (#8370) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>	2026-08-02 11:48:58 +00:00
易良	eea0a2b3b2	fix(github-channel): recover interrupted inbound tasks (#8306) ... * fix(github-channel): recover interrupted inbound tasks * fix(github-channel): make inbound recovery bounded * test(github-channel): cover delivery-failure lifecycle and audit-hit recovery Add a direct test for the onTaskLifecycle failed/delivery -> reply_pending transition and a recovery test for the publication-audit match-and-remove path. Restore the blank line between the constructor and createInitialCursor. * fix(github-channel): preserve cancelled inbound tasks and fail closed on bookkeeping (#8306) * fix(github-channel): harden inbound task lifecycle against partial persistence failures (#8306) * fix(github-channel): close crash-window duplicates and make tests load-bearing (#8306) * test(github-channel): add recovery test for suppressed audit outcome (#8306) * fix(github-channel): persist errorCommentPosted after post, add review test coverage (#8306) --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-02 07:27:44 +00:00
Dragon	e967cc9037	docs: document compaction and image model selection (#8348) ... * docs: document auxiliary model selection * docs: align model option labels * docs: clarify image model HTTPS requirement	2026-08-02 06:44:54 +00:00