669 commits

Author	SHA1	Message	Date
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
callmeYe	daa7d61990	feat(daemon): add batch extension activation APIs (#8788) ... * feat(daemon): add batch extension activation APIs * fix(daemon): focus extension batch API on V2 * fix(sdk): export extension batch types * fix(core): reject empty extension batches * test(extensions): strengthen batch regression fences * feat(extensions): allow batch activation declarations * fix(extensions): preserve legacy activation declarations * fix(extensions): reject ambiguous legacy identities * fix(extensions): preserve batch activation lifecycle * feat(extensions): key batch activation by name * fix(extensions): harden batch activation lifecycle * fix(extensions): preserve declared activation lifecycle * fix(extensions): reconcile renamed and legacy policies * fix(extensions): preserve renamed artifact lifecycle * fix(extensions): validate persisted artifact paths * fix(extensions): preserve re-keyed artifact directory	2026-08-19 06:42:48 +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
BaboBen	9a64c0a963	feat(serve): add pollable daemon turn status (#9080) ... * feat(serve): add pollable turn-status endpoints for daemon sessions Add GET /session/:id/turns/current and GET /session/:id/turns/:promptId so external callers can poll a turn's lifecycle state (queued / running / completed / cancelled / error) and result instead of holding the SSE stream for the whole turn lifetime. - Live state comes from the bridge's pending prompt queue; settled outcomes from persisted turn_result transcript records, so results survive daemon restarts and the daemon keeps no per-turn memory - Each prompt captures its own recording and settles exactly that one, so overlapping turns (DAEMON-003 deadline overlap) can never misattribute one turn's outcome to another promptId - Enforces the same client authorization as POST /session/:id/prompt Refs #8680 * test(serve): update telemetry route count * fix(serve): prefer settled turn outcome over deadline error overlay When the prompt-deadline path latches an error terminal in the overlay and the child later settles and persists a non-error turn_result for the same promptId, the poll surface previously kept the overlay error while enriching it with the successful resultText, and flipped to completed only after overlay eviction or restart. Merge via mergeTerminalWithPersisted at the two enrich call sites so the persisted outcome supersedes a bridge-synthesized error terminal once it exists; the exactly-once turn_error event publication and FIFO release are unchanged. The different-promptId endedAt tie-break is intentionally untouched. * fix(serve): pin turn-start session identity for turn-result settle R4-1: settle resolved the ChatRecordingService at settle time, so a startNewSession rotation mid-turn could land the turn_result record in the new session's transcript while the poll surface kept reading the old one. Capture the recorder at turn start and settle on that instance; pin the outgoing service's session identity at rotation so the late append keeps the pre-rotation sessionId. R4-2: reject empty error.message/error.code in turn_result payloads, mirroring the existing empty-promptId rejection. Adds rotation/pin regression tests plus the round-4 test suggestions (extractor fallback, startedAt, cancel/error race matrix, resultCode defaulting, removed-prompt projections). * fix(serve): enforce the turn_result bounded contract on the write path R3-3: cap promptId, stopReason, and originatorClientId at 256 chars in isTurnResultRecordPayload, closing the unbounded echo of corrupted-transcript values through GET /session/:id/turns/:promptId; recordTurnResult now validates payloads against the same contract before appending, so type-correct but invalid shapes (error state without error, error on non-error states) can no longer produce records invisible to the restart scan. Also lands the four round-5 test assertions: merged-payload error-leak pin, multi-model-call settle count, successor attribution in the superseded-throws test, and the early session-mismatch guard pin. * fix(serve): address round-6 review findings on daemon turn status - Session: settle a successor-aborted turn as cancelled only when the thrown error is the abort itself; genuine failures after a NEW_PROMPT abort surface as error, matching the send-loop contract - bridge: serve repeat polls of a settled promptId from the enriched overlay instead of re-scanning the child transcript, and give the turn-status read the transcript timeout instead of the 10s init default - bridge: forward the channel display text unchanged; Session treats an empty display text as absent for the turn record ([image] fallback) - Session: cap streamed-response accumulation for turns without a channel delivery at the turn-result bound - docs: document the bounded non-monotonicity of poll terminals * fix(serve): guard turn-status reads against rewind races and keep the trusted prompt projection A successful rewind that completes while a getSessionTurnStatus child transcript scan is in flight could let the pre-rewind record be cached into the freshly cleared overlay and served forever. Track a per-session rewind generation captured before the scan and discard the scanned outcome when it moved. enrichTerminalTurnStatus and the deadline-supersede merge returned the child-recorded promptText ahead of the bridge's trusted display projection, leaking hidden channel context on the poll surface. Make promptText/promptTextTruncated backfill-only and keep the terminal's projection in the supersede path. Make the pinning test adversarial and correct a false comment about the child's ''-as-absent fallback. --------- 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: qqqys <qys177@gmail.com>	2026-08-18 06:35:17 +00:00
易良	99a3a08e17	feat(daemon): make serve new-file mode configurable (QWEN_SERVE_NEW_FILE_MODE) (#9364) ... * feat(daemon): make serve new-file mode configurable (QWEN_SERVE_NEW_FILE_MODE) qwen serve's atomic text writers created every NEW file at 0600 unconditionally, ignoring the daemon process umask with no way to opt out (issue reporter runs the daemon under a systemd UMask=0002 drop-in and every agent-created file diverged from the group-readable repo convention). Add a NewFileModePolicy ('owner' = 0600 default, 'system' = standard 0o666 & ~umask) on createWorkspaceFileSystemFactory, threaded through writeTextAtomic / writeTextOverwrite / edit / editAtomic and the same-host external tool-write route. resolveBridgeFsFactory reads QWEN_SERVE_NEW_FILE_MODE ('owner' | '0600' | 'system', case-insensitive); unrecognized values warn on stderr and keep the fail-closed 0600 default. Existing-file mode preservation is unchanged, binary uploads stay 0600, and the default behavior is bit-for-bit unchanged. Closes #9250 * fix(serve): register the new-file-mode env access and harden the knob's docs and wiring - process-env guard: register the whole-object process.env access in fs-factory.ts (the parseNewFileModePolicy default parameter) so the serve process.env guard suite passes — the PR-caused CI failure. - resolveNewFileModeBits: read the umask lazily only when the 'system' policy consumes it; the default 'owner' path no longer issues two umask(2) syscalls per write. - docs: state that the literal `0600` is an alias for `owner` (no other octal modes) in both tables; correct the binary-upload route to POST /file/upload; replace the phantom per-write mode-override clause with the factual statement that agents cannot pass one. - test: pin the resolveBridgeFsFactory env seam — with newFileMode uninjected, the policy must come from process.env.QWEN_SERVE_NEW_FILE_MODE (regression-mutates to a hard-coded default are now caught). * fix(serve): keep QWEN_SERVE_NEW_FILE_MODE out of project .env files R2-1: the daemon boot path loads the primary workspace .env into process.env before any fs factory is built, and the new-file-mode key was not in PROJECT_ENV_HARDCODED_EXCLUSIONS — a project-controlled file could flip the documented fail-closed 0600 posture to umask-derived modes daemon-wide with no warning (system is a valid value), widening the visibility of agent-created files on a multi-user host. Register it as a process-scoped operator knob like the other daemon posture keys, with a security test pinning the exclusion. * test(serve): pin the fail-closed 0600 default through the resolveBridgeFsFactory seam The env-wiring test only covered the 'system' half of the seam; the unset-env default (owner -> 0600) had no coverage through the same production path — a regression making the unset default resolve to 'system' would flip every agent-created new file to umask-derived modes with no test failing (mutant verified surviving all 13 prior tests; this mirror test fails it with 0o664 vs 0o600). --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-18 03:42:35 +00:00
易良	04043e555d	feat: consolidate Local Control into one daemon-owned implementation (#9106) ... * feat(cli): add daemon-owned Local Control service Local Control is implemented twice today — once in the CLI, once as an 830-line Rust TCP proxy in the Tauri shell — with two divergent security models. This adds the daemon-side service both can collapse onto. The Rust proxy exists only because `qwen serve` fixes its bind address at startup and cannot add a listener later; everything it does (Host/Origin rewriting, CRLF rejection, connection caps) is compensation for that one fact. `LocalControlService` attaches a second `http.Server` over the same Express app at runtime, so there is no hop to rewrite. Phases 1-3 of docs/plans/2026-08-13-local-control-consolidation.md: - Listener identity tagged on the `http.Server`, resolved per request, so credentials scope to the listener a request arrived on. - `CredentialStore` replaces `bearerAuth`'s single pre-hashed token. The runtime token is rejected on the LAN listener and the pairing token on loopback — the invariant the Rust proxy enforced by rejecting requests carrying the runtime token. Fixes the CLI path handing the LAN the full-strength daemon token with no revocation short of restart. - `hostAllowlist` now gates the LAN listener against its advertised authority. Previously it opted out entirely off loopback, leaving the CLI path with no DNS-rebinding defense. - `MutableOriginAllowlist` lets the LAN origin be added and removed at runtime; the middleware is still installed once. Empty-allowlist behavior is identical to the `denyBrowserOriginCors` wall it replaces. - ACP WS upgrade tracks a set of servers instead of one, and scopes the subprotocol credential the same way as the REST gate. - LAN selection advertises private/link-local IPv4 only, and surfaces ambiguity to the caller instead of failing (Rust) or emitting a QR per interface (CLI). Refs #9075 * feat(cli): wire Local Control into the daemon boot sequence Constructs the service in `createServeApp`, where the credential store and the CORS allowlist it mutates already live, and publishes it on `app.locals` alongside `acpHandle` — the channel `runQwenServe` already uses to reach into the built app for lifecycle work. - `bearerAuth` now takes the listener-scoped `CredentialStore`, and the ACP WS mount takes the same one so the `qwen-bearer.*` subprotocol cannot sidestep the scoping the REST gate enforces. - The CORS middleware is installed unconditionally over a `MutableOriginAllowlist`. With no `--allow-origin` this returns the same 403 envelope as the `denyBrowserOriginCors` wall it replaces, so the default posture is unchanged. - Daemon teardown disables Local Control before disposing the ACP handle, since detaching the LAN listener's upgrade registration goes through it. Token revocation and origin removal are synchronous, so they complete even though the enclosing dispose scope cannot await the socket close. - The LAN listener honors `--tls-cert` / `--tls-key`, reading them at enable time so a renewed certificate is picked up. Serving plaintext off a daemon deliberately put behind TLS would downgrade the more exposed of the two surfaces; `status.encrypted` reports which it is. Refs #9075 * feat(cli): repoint --local-control at the daemon service The flag stops being a second implementation and becomes a caller. Previously `--local-control` commandeered the daemon: it bound to `0.0.0.0`, generated a token that WAS the daemon token, and rewrote the origin allowlist — which is why it conflicted with `--token`, `--hostname`, `--allow-origin`, and an ephemeral port. The daemon now owns a separate LAN listener with a separate revocable credential, so none of those are in tension. A daemon can serve authenticated loopback and run a Local Control session at the same time, and `--no-web` is the only remaining conflict. - `localControlUrls` is deleted. Its "every non-internal IPv4" policy is the bug the service's private/link-local selection replaces; it would put a VPN or public address in a QR code. - Ambiguous multi-network hosts get `--local-control-address <ip>` instead of a QR per interface. - Sleep inhibition moves into the service, so it is held while the LAN listener is up and released when it goes down rather than for the lifetime of the process. - The pairing line now reports actual sleep-inhibition and encryption state instead of asserting the common case. - `RunHandle.getLocalControl()` reaches the service; a getter because the runtime app is mounted after the listener is up. Refs #9075 * fix(cli): harden daemon-owned Local Control * fix(cli): flush Local Control disable response * feat(desktop): move Local Control into Settings * fix(local-control): close listener lifecycle gaps * fix(cli): resolve local control review comments * fix(local-control): align route lifecycle * fix(serve): close local control review gaps * fix(serve): close Local Control QR and bridge-filter review blockers QR rendering in the Local Control routes is now best-effort: an over-capacity pairing URL (the target deep-link is caller-influenced) no longer turns enable/status into a 500 while the LAN listener stays live, which wedged the Web Shell card with no disable path. The interface denylist also stops rejecting physical LAN bridges (br0, Windows "Network Bridge") and only filters the virtual bridge shapes (Docker br-<hex>, macOS bridge<N>), matching the deleted Rust filter's per-platform behavior. Adds regression tests for both. * fix(serve): close round-5 Local Control review findings - Card: reconcile the selected LAN address on every status update, so a stale selection cannot survive a network change when only one candidate remains (the selector is hidden in that case and gave no affordance). - Interface filter: fold the hex run into the Docker bridge token (br-[0-9a-f]+) so bridge IDs starting with a letter stop escaping the shared boundary check. - LAN listener: drop the whole-request timeout budget; Node never resets it on body chunks, so it 408'd phones trickling large uploads through the shared Express app. Header and keep-alive timeouts stay. - Copy: Ctrl+C ends the whole daemon, not just Local Control (design doc, terminal banner, --local-control description). - Accessibility: aria-live on the card, role=alert on its error line. - Tests: QR happy path, listen-error handler cleanup, strict error-handler count after enable, letter-starting Docker bridge. * fix(web-shell): preserve local control base paths * fix(local-control): close round-7 review findings - card: keep the 409 candidate list on the error path — requestLocalControl attaches the parsed payload to the thrown error and toggle reconciles status/selection from it, so a stale address after a DHCP change recovers without a page remount (R7-3) - lan-interfaces: match `vpn` as a substring and add a `wintun` token, closing the OpenVPN Wintun escape (boundary semantics let `vpn` sit inside "openvpn" unmatched) plus mid-word names like vpnkit; regression test covers the adapter family (R7-1 demonstrated entrance; structural per-platform classification stays a follow-up) - drop the orphaned strictPort ServeOptions field, the EADDRINUSE-bump condition reading it, and its test — no production entry point sets it anymore (R7-5) - docs: refresh 12-auth-security.md / 02-serve-runtime.md for the new middleware topology — unconditional allowOriginCors over the mutable allowlist on the runtime app, deny wall only in the bootstrap app, listener-scoped pairing credential on the LAN listener, and the new mutation-gate row (R7-2) - remove the dead selectLanAddress barrel re-export * fix(local-control): enforce the loopback-bind precondition on runtime enable The LAN listener binds the primary listener's port on the selected LAN address, so a wildcard or LAN primary bind already owns it — the `--local-control` CLI flag refuses that configuration at boot, but the runtime enable route (driven by the Web Shell Settings card) skipped the check and surfaced a 500 `listen EADDRINUSE` with no remediation, silently unusable for the whole class of non-loopback deployments. Return 409 `local_control_non_loopback_bind` with the actionable restart hint instead; loopback binds (127.0.0.1/localhost/::1) stay enabled via the shared `isLoopbackBind` helper. * fix(local-control): close round-8 demonstrated escapes + doc/test gaps R7-1 (demonstrated false negatives): Docker veth peer IDs are veth<hex> and may be letter-led (vethd4a1b2c), which the bare token's digit boundary let escape — the token takes the same shape as br-<hex>. Corporate SSL-VPN adapters (Cisco AnyConnect, GlobalProtect, Pulse Secure, FortiClient, Cloudflare WARP) carry no `vpn` substring, so their vendor names are listed explicitly; a sole-candidate VPN address is no longer silently auto-advertised in the QR. Regression tests cover all six shapes. The vEthernet-external false positive and the class fix (structural classification instead of name matching) remain under #9158. Also: the settings card's status-fetch effect clears a stale error on re-run and ignores superseded responses; the detach test now connects a primary-listener client and asserts it survives detachServer (the per-server filter previously survived a mutation probe); the flags table gains the --local-control-address row; the design doc states that --allow-origin origins stay admitted alongside the LAN origin; the three Host-gate doc surfaces note that the LAN listener always enforces its advertised-authority Host check. * fix(local-control): bound slow-body slots + close round-7 adapter escapes (#9106) - service: replace requestTimeout=0 with a bounded 30-minute whole-request budget; an unlimited budget let an unauthenticated LAN client trickle bodies and hold every pre-auth connection slot open indefinitely (headersTimeout covers only headers, keepAliveTimeout only idle sockets) - lan-interfaces: add interim vendor tokens for post-rename SSL-VPN successors (ivanti, cisco secure, citrix, sonicwall); Ivanti Connect Secure (Pulse Secure renamed) escaped the enumerated list and was auto-advertised in the QR. Class fix stays tracked in #9158 - auth: document the MutationGateOptions caveat that on a no-token daemon the Local Control pairing credential admits loopback callers to the strict surface (round-7 design decision still open) * fix(local-control): stop serving the pairing secret to unauthenticated callers (#9106) Probe-verified hole: on a tokenless daemon any local process could POST /workspace/local-control/enable (or GET the unguarded status route) and read status.url — the pairing token in the fragment — then present it on the LAN listener, where the strictDenier passthrough admitted it to the whole strict mutation surface (file writes, memory CRUD, git push/pull, extension/MCP control) without the operator ever scanning anything. Close the acquisition step: - GET status / POST enable / POST disable now return url + qrText only to requests bearerAuth actually authenticated (requestWasAuthenticated); unauthenticated callers get the status with the secret stripped and urlRedacted: true while active - on an unauthenticated enable the pairing URL is printed to the daemon's own terminal instead — the one channel a local attacker cannot read over HTTP - web-shell Settings card renders a terminal hint when urlRedacted (en/zh) - MutationGateOptions caveat rewritten to the resolved state Authenticated callers (daemon token) are unchanged. Route tests: redaction for unauthenticated GET/enable, full payload for authenticated callers, terminal print on enable; suites 42/42, eslint/prettier clean, Codex security review CLEAN. * fix(local-control): close round-9/10/11 review findings (#9106) - write the pairing URL with writeStdoutLineSafe so a dead/full stdout cannot wedge enable into a false 500 - reject an empty --local-control-address instead of silently dropping it - pin --token/--allow-origin composition through to runQwenServe - drop stale serve.ts file:line references in credentials/lan-interfaces - correct the CORS caveat and the design doc's flag/origin claims * test(serve): drop stale strict-port assertion * fix(local-control): close round-12 review findings (#9106) - give the composition test a full enable payload and a handle.close so the detached handler cannot leak a real process.exit(1) - wrap the QR dynamic import + setErrorLevel in withUiData's fault isolation so a broken qrcode-terminal degrades to the raw URL instead of 500ing every status/enable - fix the ZH urlRedacted copy (it prints a URL, not a QR) - finish the denyBrowserOriginCors -> allowOriginCors doc sweep in 01-architecture.md and 18-error-taxonomy.md --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-17 16:44:48 +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	162213e9da	feat(review): adopt a round-aware convergence posture for posted findings (#9118) ... * feat(review): adopt a round-aware convergence posture for posted findings Re-reviews of the same PR regenerate non-Critical feedback at zero marginal cost after every push: the review files findings on code the previous round just added, the next push implements them, and the diff widens — which allocates more agents, which file more findings. Measured from the outside: one managed PR reached +13k lines across 8 review rounds with its per-round Critical count flat and was closed unmerged; on two others 78-86% of the growth was test lines. Every existing brake lives in the calling workflow and covers only bot-managed PRs; a human contributor answering a push-triggered review round by round rides the same loop with no brake at all. Convergence therefore has to be a property of /review itself, not of any orchestration around it. Teach the reviewer to raise its POSTING bar as rounds accumulate — finding, verification, the terminal report and the findings artifact are untouched: - The convergence posture (Step 6): with no flag, Suggestions post through round 5; from round 6 only Criticals post, and non-Critical findings are recorded, not requested — one line each into the new `deferredSuggestions` compose state, which compose-review renders as a disclosed, non-capping list on every event (nothing is silently dropped, and a deferral never withholds the incremental anchor). A Critical is never deferred, at any round under any floor. An APPROVE over a non-empty deferral list opens "No blocking issues" instead of "No issues found", and the round number in the clause comes from the same side-file read the ledger marker stamps, so the two cannot disagree. - The code-age rule (rounds 2-5): a NEW non-Critical finding anchored on code unchanged since the previous round's reviewed head defers the same way — that code was read last round and not flagged, so filing a nit on it now is re-derivation churn. The age reference is the previous review's own commit_id, which pr-context now recovers into the prev-ledger side file: unlike the ledger sha (a range certification, withheld on fail-closed rounds on purpose) it exists on every posted round, so the fail-closed full-range re-review — the common case in a bot loop — can still apply the rule. Validation and the newly-reachable exception mirror the anchor check; an absent or invalid reference skips the rule, never the review. - `--severity-floor <critical|suggestion>` and `review.severityFloor` make the posture an explicit knob in both directions: `critical` applies it from round 1, `suggestion` turns it off, and `auto` — the default — is the round-adaptive rule. Grammar, deferred warnings, non-PR gating and operator-scope resolution all mirror `--effort`/`--comment`. * chore(review): regenerate settings.schema.json for review.severityFloor * fix(review): close the round-1 review findings on the convergence posture Five verified findings from the PR's own reviews, each with its mechanism: - The verdict surface now carries the deferrals. A deferrals-only APPROVE set lowSignal, so verdictLine printed "none of the N review agents reported a finding" while the same body listed the findings two paragraphs down — on exactly the posture's canonical end state. lowSignal is skipped when deferrals exist, ComposeReviewResult gains deferredCount, and the verdict line names the deferrals instead. - Deferred findings count toward the verifier-delivery floor. They publish in the body as the deferral list, and an unverified claim does not become publishable by being deferred — a deferrals-only run owes a verifier exactly as a posting run does. - The deferrable set is narrowed to high-confidence Suggestions that would otherwise post. Low-confidence and Nice-to-have findings stay terminal-only: routing them through the deferral list would publish what the review contract keeps off the PR. - The code-age rule is hardened on both operands and both premises: the documented command quotes the PR-controlled path and passes --literal-pathspecs (an unquoted name executes shell; a glob name ages the finding against a sibling file's hunks), and age suppression is skipped for findings in scope the previous round disclosed as not reviewed — "the previous round saw this code" is false there. - One side-file read per compose: the deferral clause and the ledger marker take the same prevRound value, so a mid-compose update can no longer publish two different round numbers in one review. The deferral list also caps each entry at 240 chars (twenty 4,000-char entries would push the body past GitHub's 65,536 rejection line), and the disclosed 20×240 cap is now stated in the prose the list's survival promise lives in. Smaller closures from the same rounds: two invalid flag values are two typos, not a target and a tiebreak (neither becomes a file target); the --severity-floor warning arms invalid-eq and kept-as-target gain the tests whose absence a mutation probe demonstrated; the commitId fixture gains a newer marker-less review so a latest-review-wins mutant fails; the severityFloor setting joins the settings-dialog membership assertion and the configured-floor wiring tests; and the /review argument-hint now advertises the flag. * fix(review): carry deferredCount through the persisted verdict The save-artifact validator constructs the persisted verdict from the composed JSON and gained the field with absent-means-zero semantics: a composed file written by a build predating the posture must not fail a save over a count that only affects display, while a present value of the wrong shape is refused like every other field. * test(review): pin deferredCount passthrough and the pre-posture default in the saved artifact * fix(review): close the round-2 findings — deterministic deferrals and the truthful verdict line - The verifier-delivery floor now excludes deterministic [build]/[test]/ [probe] deferrals by their source tag, the same exclusion body Criticals get: a pre-confirmed finding never produces a verifier delivery, so counting it demanded a delivery that cannot exist — the cap never lifted, the anchor was withheld every round, and the posture's own enforcement regenerated the full-range re-review loop it exists to end. The deferral entry format now carries the source tag (SKILL prose + a pin), and the determinism regex is one shared constant for both scans. - verdictLine's "(listed in the body)" turns cap-aware past the 20-line render cap, so a verdict counting 21 no longer certifies a body listing 20; the line caps are module-scoped for the two readers. - The mutation-demonstrated assertion gaps are closed: deferredCount is asserted on the REQUEST_CHANGES and COMMENT return sites, and save-artifact's refuse arm has its wrong-shape cases ('two', -1, 1.5). * fix(review): close the round-3 findings — source-position tags, a Critical tripwire, and honest age-reference lifecycle - Deterministic classification of a deferral reads the SOURCE position only (tag immediately after the entry's first em-dash): a whole-entry scan classified a finding deterministic off a title that mentioned [test] and skipped the verifier floor for an unverified claim (probe-confirmed). - A deferral carrying a Critical marker is refused outright: the channel is model-written free text that casts no vote on C, and a Critical routed there composed an APPROVE over a blocker in a probe. Marker forms only, so prose like "critical-path" passes; artifact-level reconciliation stays a structural follow-up. - The prev-ledger side file now carries the winning review's own id, and a run that recovers no ledger strips a stale file's commitId/reviewId while keeping the round counter: an age reference the PR's current reviews no longer vouch for can wrongly defer a finding on code changed-and-reverted since the true previous round — snapshot diffs are not monotonic over intervals, which refutes the earlier keep-it-it's-conservative ruling. The write/strip logic is an extracted, filesystem-tested helper, closing the serialization blind spot two rounds of review asked about. - Prose repairs: the code-age rule is auto-only (an explicit suggestion floor turns it off, as the resolve-floor paragraph already promised); the fallback names the commitId, not the ledger sha; the not-reviewed check binds to the review the side file's reviewId names; and the context-unavailable state skips the age rule (the side file may be a previous run's). save-artifact's null-deferredCount reads as zero — the same absence semantics compose-review's toCount gives the field's siblings — now stated and tested rather than accidental. * fix(review): close the round-4 findings — licence the deferral channel, harden the side file, age aggregates per location Code: - The deferral channel gains its licence check: the resolved severityFloor rides the compose state, and a non-empty deferral under an explicit suggestion floor (posture off) or on round 1 of auto (no posture, no age reference) is refused — the one channel that removes findings from posting now gets the same deterministic treatment as the counts. Rounds 2-5 under auto stay licensed: the code-age rule defers there, which the reviewer's proposed round<6 condition would have wrongly outlawed. - Ledger recovery skips PENDING drafts (the API serves the caller's own unsubmitted reviews; a crashed run's draft is not a previous round), and the side file's lifecycle is three-way honest: recovered → written whole (atomically, temp+rename — a mid-write failure must never leave a truncated file that parses as no round); reviews read but no ledger for this account → file REMOVED (another account's round counter must not stamp this account's first review round N+1); recovery threw → round counter kept, age-sensitive commitId/reviewId stripped. - Disposal rule v3: an invalid flag value survives by what it could BE — a PR-shaped token survives unless a typed target exists (an unrelated typo must not change WHICH codebase is reviewed), a file-shaped token only as the sole kept token. - The per-entry cap backs off a split surrogate pair (zh titles ride the deferral list untranslated; unit 240 on a high surrogate shipped U+FFFD). Prose (SKILL): - Pattern aggregates age per location: ANY changed location posts the aggregate; deferral only when every location is unchanged and covered. - The reviewId body is consulted only after fetching a truncated tail (the 8,000-char render cap can hide the very "Not reviewed" disclosure the age rule depends on); an unreadable body skips the rule. - context-unavailable resolves the auto floor as round 1: no posture, full posting, said in the terminal — a posting bar in doubt fails open. - Deferred findings number under D<round>-<n>, never consuming an R id the ledger's buildLedger would reassign to a posted sibling. - The Step 8 record sentence now states exactly what survives where, and the state contract documents severityFloor. Tests pin all of it, including the mutation-shown gaps: the marker/clause round agreement, the exactly-20 verdict-line boundary, the submit-seam deferral passthrough, and sha survival through the side-file rewrites. Structural remainders are filed instead of grown: #9176 (typed deferral channel derived from the findings artifact), #9177 (whole-body byte budget). * fix(review): close the round-5 findings — carry the floor unresolved, cap unlicensed deferrals, guard the counter The round-5 reviews caught a shipped design contradiction: the SKILL told Step 6 to carry the RESOLVED floor into the compose state, so a legal rounds-2-5 age-rule deferral arrived as the string 'suggestion' — which the licence check reads as the operator's explicit posture-off override — and the compose threw, losing the entire round, Criticals included. Two fixes, one per side of the contract: - The state carries the verdict's floor UNRESOLVED: auto stays the literal 'auto', and the module licenses it by the round it derives itself. A SKILL pin makes the sentence load-bearing, and an end-to-end test pins the legal round-3 shape (side file at round 2, auto floor, deferral → clean APPROVE naming round 3). - Unlicensed deferrals CAP instead of throwing: prevRound is a best-effort side-file read whose every failure mode returns 0, so a missing file at a true round 6 must degrade to a disclosed, capped, anchor-withheld verdict — never to no verdict at all. The findings render under a warning clause; the Critical-marker tripwire stays a refusal (a Critical rendered as "recorded, not requested" would be the worse outcome). Adjacent round-5 closures: the tripwire is separator-agnostic (an ASCII hyphen where the format prescribes an em dash was the cheapest real miss); side-file removal is gated on a POSITIVELY read non-empty reviews list (ghApiAll flattens error envelopes to [], and an empty list must not delete a live round counter and its anchor); the atomic temp name is per-process so concurrent same-PR fetches cannot rename each other's bytes, with debris unlinked on a failed rename; two PR-shaped invalid flag values are refused as ambiguous instead of first-wins; the apostrophe escaping clause and the unresolved-carry sentence gain SKILL pins; and review.severityFloor joins the user-facing settings reference. Declined, recorded on the PR: requiring per-file coverage evidence for the age rule — the anchor chain already encodes it (an incremental round's anchor certifies the preceding full coverage; broken links are fail-closed rounds whose disclosures the rule consults). * fix(review): close the round-6 Criticals — relocate stray Criticals, license by evidence at hand, one separator grammar Round 6 tripled the finding count and aimed almost entirely at rounds 4-5's hardening code — the expansion signal this PR's own posture exists to answer — so this round lands only the confirmed Criticals and defers the Suggestion tail on the record: - A Critical-marked deferral is RELOCATED into the body Criticals instead of thrown: it counts toward C, the event blocks, and the round posts — the same doctrine the round-5 fix applied to the licence check, closing the last channel where one bad entry could cost a composed round (with real drafted Criticals attached). A lookbehind spares hyphenated compounds — the SKILL's own "non-Critical findings" phrasing was a realistic false positive that would have blocked over a nit. - The two deferral regexes share one separator grammar: the deterministic classifier now accepts hyphen/en/em like the tripwire, so two spellings of one [test] finding no longer produce opposite verdicts (the unmatched form demanded a verifier that cannot exist — the self-inflicted cap). - The licence closes its evidence gaps: an ABSENT severityFloor beside a non-empty deferral list is unlicensed (the field ships with the channel; omission must not silently re-license what an explicit suggestion floor forbade), auto in the context-unavailable state is unlicensed (the round is unknowable), and the unlicensed cap now joins the certification ladder so "Reviewed — no blockers." cannot open a body whose own warning says findings may be under-posted. - Deferred entries render Markdown-neutralized (mdField, the budget-gap rule — model text reaching a public body), carry a truncation ellipsis when the per-entry cap cut them, and --severity-floor accepts the documented `auto` spelling while quoted-empty flag values are consumed as missing instead of becoming empty file targets. - Prose: the age rule's deferrable set names the same high-confidence otherwise-postable Suggestions as the floor paragraph (never low-confidence/NTH); a reviewId body absent because it matched the canonical LGTM filter is disclosure-free by definition, not unreadable; and the posture round is the side file's — the cache scopes the diff but never decides the posture. The Suggestion tail and the two structural families are recorded, not grown: #9176 gains the age-evidence and id-timing bullets; #9177 already carries the whole-body budget. Declines stand where argued (R2-2/R2-15 anchor-chain; sole-file-shaped promotion keeps the forgot-the-level use). * fix(review): close the round-7 findings — kebab paths, the equals-form ambiguity hole, and the relocated blocker's ledger seat Round 7 caught a regression the round-6 fix itself introduced, plus one genuine hole in the round-5/6 ambiguity guard: - The deterministic classifier anchors on the first WHITESPACE-FLANKED separator after the leading file:line token. The round-6 negated-class walk stopped at the first hyphen INSIDE a kebab-case path — this repo's enforced .ts convention — so the common spelling of a [test] deferral was misclassified non-deterministic and demanded a verifier that cannot exist: the permanent self-cap loop, on exactly the entries the exclusion exists for. Kebab, [build] and [probe] cases pin all three tags on both separators. - The PR-target ambiguity pool counts BOTH flag spellings: an equals-form invalid value never enters the disposal set, so which of two PR numbers got reviewed depended on which syntax was typed. All four spelling combinations now land on the same loud refusal, pinned. - A relocated Critical rides the machine ledger: the split moved into a shared helper that the body composer and the marker builder both call, so a mis-routed blocker keeps its id continuity ("the findings always ride" includes the mis-routed ones). - The age rule's two diff-output doubt states fail open like every other arm: a non-matching pathspec (prove it with tree-relative cat-file before reading the diff's silence) is about the path, not the code, and a zero-hunk non-empty diff — a PR-controlled .gitattributes binary mark — is a file-level change; both post. Commands pinned to the worktree root. - The mutation-shown pin and test gaps are closed: explicit auto override, quoted-empty consumption on both flags, the invalid-configured-floor handler seam, the SKILL's validation commands, round-source and context-unavailable clauses, and the aggregate age clause. The four re-asserted body Criticals remain tracked (#9176/#9177 and the two serialization-ordering threads) — their standing disposition is the round-6 batch record, and their final resolution is the maintainer's merge decision. * fix(review): close the round-8 Criticals — one entry grammar, one rescue rule, deletion only on proven absence Round 8 landed three genuine Criticals, all on rounds 6-7's hardening code, plus a fourth from a local lane sharing the first's mechanism: - The deterministic classifier's entry grammar tolerates the shapes the SKILL itself prescribes: the aggregate `(+N locations)` suffix between the anchor and the separator, a leading space (entries are trimmed before the scan; the filter trimmed only for emptiness), and an en dash. Every one classified a pre-confirmed [test] deferral non-deterministic and demanded a verifier that cannot exist — the permanent self-cap on the posture's own stop signal, probe-demonstrated three ways. - A relocated Critical is classified by that same position-anchored rule, not the whole-entry tag scan the model's own body Criticals get: a title-borne [test] in a relocated unverified claim exempted it from the floor and posted it as a blocking Request changes with no verifier. The split helper reports the deterministic count and the body composer keeps the two provenances apart. - The equals-form PR-shaped invalid value joins the disposal pool exactly as the spaced form does — the round-7 fix wired it into refusal only, so `--severity-floor=6711` reviewed the local tree while `--severity-floor 6711` rescued PR 6711. Every spelling converges; the ambiguity pool counts distinct values, so two spellings of one PR are one candidate. - Side-file deletion requires PROVEN absence — a walked review list with no submitted review by this account — never "recovery returned null": an own review whose marker fails to parse (edited or damaged bot body, marker- less follow-up) is a persistent state, and deleting there stamped the next round "round 1" mid-PR and reset the posture clock. Recovery is three-valued; the middle state strips conservatively. Adjacent closures: an unrecognised severityFloor (model-transcribed drift like "Critical" or "auto ") is the unknown state — unlicensed with a list, inert without one — instead of a refusal that lost zero-deferral rounds over a field that changed no output; the interface doc says UNRESOLVED where it said RESOLVED (the round-5 regression's own wording); the deferrable-set description at four sites names the real set (otherwise-postable high-confidence Suggestions; low-confidence and NTH stay terminal-only) and the rounds-2-5 age deferrals; and the mutation- shown gaps are pinned — pure deferrals stay out of the ledger, a relocation-only run incurs no licence cap, an invalid configured floor is silent on non-PR targets, and a 64-hex commit id survives recovery. * feat(review): type the deferral channel — carry the fields, stop re-parsing prose Every genuine Critical in review rounds 5-8 lived in one place: the deferral channel was free text re-parsed for provenance it did not carry. A separator regex classified deterministic source, a marker regex caught mis-routed Criticals, and each round's probe found the spelling the last fix excluded — kebab paths, the SKILL's own aggregate suffix, an en dash, a title-borne [test], (Critical), a fullwidth colon. A whole-module self-audit (with reproduced witnesses) found eight more shapes and named the class: the fourth, fifth and sixth rounds of the same enumeration trap this repo's own review doctrine (#9095) says to close structurally. So the entry is typed. `deferredSuggestions` is an array of `{file, line?, source, severity, title, locations?}` copied from the findings artifact the model already wrote in Step 6: deterministic derives from `source` (build/test/probe), relocation from `severity === Critical` (counts toward C, blocks, rides the ledger, classified by its FIELD — a title mentioning [test] no longer exempts an unverified claim), a `Nice to have` or malformed or free-text entry is refused at the boundary like a NaN count (the channel that un-posts findings is not guessed at), and the human line `file:line — [source] title (+N locations)` is RENDERED by compose-review — nothing downstream parses it back. Both regexes and the split helper's string grammar are gone; the SKILL contract, the state bullet, the submit seam test and the whole deferral describe block are rewritten for the typed shape (no test probes a spelling any more). The self-audit's side-file findings land in the same commit: the recovered write never lowers the round (a stale walked list — a concurrent lane, or a paginated fetch that came back short — overwrote round 7 with round 2 and dropped the anchor sha; compare on round, reviewId as tiebreak), and login comparison is case-insensitive per GitHub (a case mismatch read "own review exists" as proven absence and deleted the counter). Both pinned. This pulls the structural half of #9176 into the PR; the issue keeps its remaining bullets (structured `deferred` artifact marker, age-evidence arm, R/D-id timing). * fix(review): bound the relocation exit and dedupe rescued targets by identity Round-9 findings on the pre-typed head, the halves that survive typing: - The relocation exit applies the same per-entry bound as the deferred exit — newline collapse, the 240-char cap without splitting a surrogate pair, the ellipsis on a trim, and Markdown neutralization — through one shared helper. Relocated titles were spliced into the body verbatim, an unbounded feed the deferred exit's cap was added precisely to prevent. - Rescued PR-shaped flag values dedupe by RESOLVED TARGET (number, plus host/owner/repo for a URL), not by raw string: a bare number and a same-number URL are one PR, and a raw-token Set read them as two and silently fell back to the local tree. Of several spellings of one rescued PR exactly one becomes the target; the restatements no longer surface as "Ignoring extra argument(s)" on the invocation the dedupe blesses. - The persist test's debris check asserts on the directory listing rather than a temp name no code path writes (the temp is per-process). The round's other two findings — the whole-entry Critical tripwire and its title-borne false positives — no longer have an input form: the channel is typed (6118109118) and severity is a field.	2026-08-15 17:09:43 +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
Shaojin Wen	4257916e7e	feat(daemon): guard cross-worktree Git mutations (#8687) ... * feat(daemon): guard cross-worktree Git mutations Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): keep Git guard off serve fast path Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): close daemon Git guard parser bypasses Rebuild the daemon-side Git relocation guard parser so the runtime-verified bypasses from review are closed: comment/glob tokens, backslash continuations, shell wrapper and path-qualified invocations, cwd-shifting builtins, env-var relocations, gitfile/symlink/worktree-admin indirection, -C vs relative git-dir ordering, --output and textconv-capable read-only subcommands, command-valued -c config, and dynamic expansion forms all fail closed for mutations outside the session working directory. Command splitting, canonicalization, and containment now reuse the core helpers. Key the child-side v1 restrictions (/fork, agent-backed workspace memory) and per-call daemon round trips on a real external provider being attached instead of on guard plumbing presence: under the built-in guard alone, hidden-agent tool calls traverse the same daemon-side policy, so those features stay available and non-shell tools resolve locally. Denial reasons are length-clamped and control-character-stripped so they always satisfy the guard result validation. * fix(serve): keep daemon Git guard out of serve fast-path closure Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): close re-reviewed daemon Git guard bypasses and subagent regression Address the re-review at b1b7606: keep the outermost entry cwd as the containment basis inside shell wrappers, fail closed on unrecognized programs that still reference a relocated Git command, skip leading shell keywords, deny undecidable and fused `-c` payloads, inspect command-executing `-c` config before the read-only allowance, drop `grep`/`status` from the relocated read-only set, validate the model-supplied `directory` against the effective working directory, and stop modelling `--exec-path`/`--list-cmds` as value-taking. Context-less shell paths (subagents, cron turns, background notifications, resumed background agents) previously failed closed under the now-unconditional managed guard: fall back to the scheduler-owned session id and validate those requests by session ownership, while external-provider consultation still requires a prompt binding. Move the guard's canonicalization off the daemon event loop with a promise-based realpathNearestExisting, drop the unread workspaceCwd request field, and restore the top-level guard import in run-qwen-serve. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): restore lazy daemon Git guard import Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): close daemon Git guard shell front-end bypasses All six forms below were reproduced against the real guard and confirmed to really escape the boundary with a real shell and git 2.47.3 (the outside worktree was reset, or the textconv marker file was created outside). - `cat-file --textconv`/`--filters` run programs configured by the *target* repository, so the relocated read-only allowance no longer applies when a `--textconv`, `--filters`, or `--output` flag is present, wherever it appears in the invocation. - `export GIT_WORK_TREE=<outside> && git reset --hard` hid the relocation in its own segment. `export`/`declare`/`typeset`/`readonly` operands (and plain assignments after `set -a`) are now recorded as exported relocations that apply to every later command in the chain, including wrapper payloads and substitution bodies. - `builtin cd <outside>` masked cwd tracking; `builtin` now takes the same prefix-skipping path as `command`. - `cd -P <outside>` consumed the flag as the directory operand, so containment was evaluated against `<cwd>/-P` — inside the boundary whenever such a directory exists. `cd`/`pushd` option words are skipped when locating the operand. - `cd <outside> && nice git reset --hard` passed because an unrecognized program word was only checked for relocation markers. It is now also denied when the tracked working directory is unknown or already outside the boundary, while the same command inside the boundary stays allowed. - `echo $(git -C <outside> reset --hard)` was folded into an opaque token. `$(…)` and backtick bodies are extracted from the raw segment and analysed as nested commands; `$((…))` is stepped over as arithmetic and an unterminated substitution is denied. The guard also covers the `monitor` tool, which spawns its `command` through the same shell with the same `directory` argument and was previously short-circuited to allow by the child before the daemon ever saw it. The shell-executing tool set is shared through acp-bridge so the child and the daemon policy cannot drift apart. * fix(serve): restore the shell-wrapper analysis return shape The unrecognized-shell-wrapper branch of `analyzeRun` still returned the pre-`assignmentsOnly` `other` shape, so `tsc --build` failed on the union. Every CI job that installs dependencies runs that build, so all three went red on it. * fix(serve): close the remaining daemon Git guard front-end gaps Round-4 review findings, each reproduced against the real guard before the fix and pinned by a regression test with an in-boundary control. - An unrecognized program word only failed closed on Git-flag markers, so a `cd` inside its quoted payload slipped through (`su -c 'cd <outside> && git reset --hard'`, the same through `xargs … sh -c`). `cd`/`pushd` now count as relocation markers in that scan. - The Git word was matched case-sensitively while program classification lowercases, so `nice GIT reset --hard` escaped on a case-insensitive filesystem. - A program word the daemon cannot read at all (`cd <outside> && $CMD git reset --hard`, also behind `command`) skipped the containment check that unrecognized program words get. - `export GIT_DIR` with no `=` exported an earlier shell-local assignment invisibly; shell-local GIT_* assignments are now tracked and promoted on a name-only export. `+=` appends and `set -o $OPT` are recorded as unresolved. - `eval` propagated only its cwd, so `eval 'export GIT_WORK_TREE=<outside>'` lost the export it performed in the current shell. - `--shallow-file` and `--attr-source` were not modelled as value-taking, so their value was read as the subcommand — which ends option parsing and hid every relocation after it. - The command-executing config set missed `core.sshCommand`, `diff.<driver>.textconv`/`command`, `merge.<driver>.driver`, `sequence.editor`, `gpg.program`, `pager.*`, `core.askPass` and `uploadpack.packObjectsHook`, and matched case-sensitively although Git config keys are not. - `env -S` accepted a dynamic payload as literal text and ignored the fused `env -S'cmd'` form. Docs: the Non-goals list claimed `CoreToolScheduler` was untouched although this PR adds the scheduler-owned `sessionId` to the guard context; the protocol capability row and the user guide still described the guard as `run_shell_command`-only and mis-stated the denial-message prefixes; and the relocation-never-revoked over-approximation (`unset GIT_DIR` does not clear a recorded relocation) is now stated in Limitations. * fix(serve): resolve the repository Git discovers, not just the directory Two more escapes from the round-4 review, both reproduced with a real shell and git 2.47.3 before the fix. - A `.git` gitfile inside the boundary redirects Git to an outside repository: `git -C <inside>/decoy commit` moved the outside repo's HEAD while the directory itself passed containment. A `cwd` target now resolves the first `.git` between it and the boundary through the same `resolveGitDirRepository` path `--git-dir` targets use, which keeps a linked-worktree session working because its own gitfile resolves back to that worktree's checkout — pinned by a test that runs a session whose `.git` points at an outside admin directory. - `cd -P <symlink>/..` lands the shell in the parent of the symlink's real target, which a lexical resolve places back inside the boundary. A `-P` cd whose target contains `..` now drops the tracked directory. The default logical form is unchanged and still allowed, because bash resolves it against the logical path and really does stay inside. * fix(serve): match chdir, brace-expansion and here-string semantics Three more escapes from the round-4 review, each reproduced against the real guard and then confirmed with a real shell and git 2.47.3. - `git -C <symlink>/.. reset --hard` escaped: git's `-C` reaches the kernel as a chdir, which resolves each component's symlinks, while the guard collapsed `..` lexically and landed back inside the boundary. `-C`, `env -C`, `sudo -D` and `cd -P` now resolve physically, component by component; bash's default `cd` stays lexical because that is what the shell itself does. - `git {-C,<outside>} reset --hard` escaped: brace expansion happens after this parse, so the tokens git receives were never the tokens the guard saw. A brace-expansion token now marks the invocation unresolved. - `sh <<< 'git -C <outside> reset --hard'` escaped: the tokenizer dropped redirect operands, and a here-string carries its whole payload in the command line. Redirect operands stay in the run, so the here-string is scanned like any other token; ordinary `>`/`2>` targets are inert text and a regression test keeps them allowed. Checked and not reproduced, so left alone: `describe --dirty` did not rewrite the target index, `GIT_OBJECT_DIRECTORY=<outside>` did not write objects there, and `bash -o allexport -c '…'` cannot export into the parent shell because the payload runs in a subprocess. * fix(serve): treat relocated git describe as a target-repo write Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(serve): state what git describe actually rewrites Keeping `describe` out of the relocated read-only set is right, but the reason as written does not match git 2.47.3. Measured against a real repository with a stale stat cache (mtime-only touch), `describe --dirty`, `--broken` and `--always --dirty` rewrite the target repository's `.git/index`, while a plain `describe`, `--tags` and `--always` leave it untouched. The subcommand still belongs outside the set — the flag is one token away from any describe a model writes — so only the comments and the design doc change. * test(serve): cover the provider-attached marker with a real handshake The only assertion on the provider marker was the negative case, so a break in the attached path — the marker comparison or the conditional child-env spread — would have gone unnoticed. This drives a loopback provider through the real `/v1/handshake` and asserts the child env carries the attached marker alongside the plumbing one. Verified load-bearing: forcing the marker to `undefined` fails it. * fix(serve): close the round-3 shell and repository-discovery gaps Every payload below was reproduced against the real guard first; the two that turn on git's own behaviour were measured with git 2.47.3. Repository discovery - `ls-files` executes the target repository's `core.fsmonitor` — the exact property that removed `status` — so it leaves the relocated read-only set. Measured: `git -C <outside> ls-files` runs the hook; `rev-parse` and `cat-file` remain side-effect free. - The discovery check was tied to a *cwd* relocation, so a `--work-tree`-only or bare relocation skipped it. Git discovers its repository from the cwd whenever no `--git-dir` names one, so the check now runs on that basis, and an unrecognized program (`cd sub && nice git branch`) gets it too. A linked worktree whose own gitfile points at an outside admin directory stays allowed — pinned from both sides. Shell front-end - `eval > /dev/null '…'` swallowed the redirection into its payload and lost the command. Redirect operands (and an `N>` descriptor prefix) are now flagged: still scanned for markers, never joined into argv. - `cd` glued to a control operator (`true;cd <outside>`) was not a marker. - Letters after `c` in a short bundle are more flags, not a fused payload: `bash -cx 'cd <outside> && …'` and `sh -co ignoreeof '…'` took their real payload from a later argv entry the guard never read. - `( … )` is now scoped like a subshell: `(cd <outside>); git commit` is allowed again, while `(cd <outside> && git reset --hard)` still denies. - `sudo -R <rootfs>`/`--chroot=` and a `PATH=`/`GIT_EXEC_PATH=` assignment make every path the daemon resolves meaningless, so they fail closed. - `env --unset=NAME`, `-uNAME` and `--split-string=` in their attached forms no longer read as unrecognized options, which was denying decidable commands. - Values assigned earlier in the same command are substituted before the dynamic-program check (`X=git; Y='-C <outside> …'; $X $Y`), `eval` carries its shell locals back out, and `export $NAME` fails closed. - A command that relinks a path (`ln`, `mv`) invalidates containment proved afterwards: `ln -s <outside> bait && git -C bait reset --hard` is checked while `bait` is still the original directory. - `resolvePhysicalPath` treated `\` as a separator on POSIX, where it is an ordinary filename character. - `gpg.<format>.program` and `core.hooksPath` join the command-executing config keys. * fix(serve): scope the relink invalidation to path-resolving Git runs The rule I added a commit ago denied every Git run that followed an `ln` or `mv` in the same command, which takes out `mv old new && git add -A` — as ordinary as it gets. The invalidation only makes sense for an invocation that resolves a path, so it now requires a relocation target or a shifted cwd: `ln -s <outside> bait && git -C bait reset --hard` still denies, while staging renamed files does not. * fix(serve): rebuild the relink defense and close the round-4 gaps Reproduced against the real guard first; the two environment claims were measured with git 2.47.3. Fixing my own two previous commits - The relink invalidation was both too wide and too narrow. It now records which paths a run may have re-pointed instead of setting one flag: a relinked `.git` invalidates repository discovery for every later command (`ln -s <outside>/.git .git && git status` mutated the outside repo), while a relinked directory only affects a run that resolves that very path, so `mv old new && git add -A` stays allowed. The scan no longer keys on `run[0]`, which `env ln …`, `X=1 ln …` and `nice ln …` walked straight past, and `cp -s` joins `ln`/`mv`. - Flagging redirect operands (the here-string fix) left `consumeShellWrapper` reading one as the `-c` payload: `sh -c > /dev/null 'git -C <outside> …'` dropped the real payload from analysis. - Leaving a subshell rolled back only the tracked cwd, so exports and shell locals made inside `( … )` kept denying later commands. - Shell-local reconstruction was last-assignment-wins, losing `X+=` appends. Repository discovery - A non-relocated Git command never reached discovery, so a planted `.git` gitfile at the session root redirected plain `git commit` outside. Discovery now runs for those too; a session bound below its repository is unaffected because the walk stops at the boundary, and a linked worktree still resolves back to its own checkout — both pinned. Environment and parsing - `GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, `GIT_CONFIG*` and `SHELLOPTS` mark the invocation unresolved. Measured: `GIT_OBJECT_DIRECTORY=<outside>/.git/objects git add` writes the blob there, and `GIT_CONFIG_GLOBAL=<outside>/cfg` makes git read that config. - `$'…'` is ANSI-C quoting where a backslash escapes, so `$'a\'b'` no longer leaves the substitution scanner a quote out of phase — which had hidden a following `$(git -C <outside> …)` from every analysis pass. Over-denial - A program with its own `-C` (`grep -C 5 git`, `tar -C dir`) no longer reads as a Git relocation. * fix(serve): carry relink and shell state across nested scopes Round-5 findings, all reproduced against the real guard. Four of the five are in the machinery I added over the last two commits. - Relink state was local to one `evaluateCommandWithCwd` call, so a symlink created inside `sh -c '…'`, `eval '…'` or a `$(…)` body was invisible to the parent, and a relink made in the parent was invisible to a nested Git run. It is now shared by reference in both directions. - Nothing consulted it for an unrecognized or dynamic program word, so `… && nice git add -A` after a relinked `.git` was allowed, and `X=ln; $X -s <outside>/.git .git` recorded nothing at all. A dynamic program word may itself be `ln`, so its operands are recorded too. - `<(…)` opens a paren that shell-quote reports without one, while its `)` still arrives — so `(cd <outside>; <(true); git reset --hard)` popped the subshell early and lost the `cd`. My round-4 triage called this one "not reproduced" because the probe used the top-level shape, which survives on the `Math.max(0, …)` clamp; the nested shape does not. - `eval` ran with an empty shell-local map, so `GIT_DIR=<outside>/meta; eval 'export GIT_DIR'` promoted invisibly. Locals now flow into `eval` and into substitution subshells (by copy, since their own assignments die with them); a `sh -c` subprocess still gets none. - Any unreadable word in a shell wrapper's argv can be the `-c` that carries the command, so `bash $A "$P"` is undecidable rather than absent. * fix(daemon): evaluate the guard against the directory the tool runs in A sub-agent pinned to a worktree — `working_dir`, or `isolation`, which sets `ov.targetDir` on the child Config — executes at `config.getTargetDir()` while still reporting the parent session id. The guard context carried only that session id, so the daemon evaluated every such call against the parent session's `effectiveCwd`: a plain `git commit` from an isolated sub-agent was judged in-boundary and allowed while running somewhere else entirely, and a relative `-C` resolved against a directory the command would never be in. The invocation now carries the directory it will run in, from `config.getTargetDir()` through the child guard to the daemon. It is explicitly untrusted, so the daemon accepts it only where it can verify it from state it owns: inside the session's effective working directory, or inside the worktree tree that session owns — worktrees live under `GitWorktreeService.getWorktreesDir(<session id>)`, and the session id is already validated by `ownsSession`. Anywhere else the scope cannot be established and the call fails closed. When an owned worktree is accepted it becomes the boundary, so an isolated sub-agent is contained to its own worktree instead of to its parent's checkout — reaching back into the parent is now denied, which is the escape this was reported for. * fix(daemon): reach the real guard path and close the round-6 escapes The headline finding is that the previous round's fix never reached the code it was written for. `Session.runTool` — the path daemon ACP sessions actually execute tools through — built the guard context without `sessionId` and without `cwd`, so both the session fallback this PR added and the execution directory added last round were unreachable there. Both are now supplied, exactly as `CoreToolScheduler` does. Escapes, each reproduced against the real guard first: - `GIT_SSH_COMMAND`, `GIT_EDITOR`, `GIT_SEQUENCE_EDITOR`, `GIT_ASKPASS`, `GIT_PAGER`, `GIT_EXTERNAL_DIFF`, `GIT_SSH` are programs git executes, and `GIT_CONFIG_PARAMETERS`/`GIT_CONFIG_COUNT`/`GIT_CONFIG_KEY_<n>` are its environment config channel — none were modelled. - `diff.external`, `core.gitProxy`, `interactive.diffFilter`, `credential.<url>.helper`, `remote.<name>.uploadpack`/`receivepack`/`proxy`, `tar.<format>.command`, `browser.<tool>.cmd`, `web.browser`, `help.browser`, `gc.recentObjectsHook` and `ssh.variant` join the command-executing config keys. - An unrecognized wrapper laundered that config: `nice git -c alias.pwn='!…'` never reached the git analysis. It is checked there too now. - `find <outside> -execdir git reset --hard` relocates through the program's own flag, leaving no marker. - An archive decides where it writes, so `tar`/`unzip`/`cpio`/`rsync` make their extraction directory suspect rather than their operands. - `alias g='git reset --hard'; cd <outside>; g` and the function-definition form both defer a body to wherever the bare word is later used. Over-denials, all introduced by earlier rounds of this PR: - `SHELLOPTS=errexit git status` (SHELLOPTS is bash's own options state — the rationale I gave for listing it was simply wrong, and it never reproduced as an escape), `env --ignore-environment`/`--null`/`--debug`, `curl -C - …`, `env -iS 'cmd'`, `d=<inside>; cd $d; git status`, and `set +a` turning allexport back off. Also: the sub-agent worktree test created and recursively deleted a directory under the user's global Qwen dir, keyed on a session id a real session could own. It now uses a process-unique id and cleans up in `finally`. * fix(daemon): contain a sub-agent to an in-project agent worktree `AgentTool` with `isolation: 'worktree'` provisions under `<projectRoot>/.qwen/worktrees/`, which is inside the session — so the acceptance rule's "inside the effective working directory" branch left the boundary alone and one sub-agent could still reach into a sibling's worktree, the very thing this PR is named for. A reported directory that is a checkout root in its own right now becomes the boundary wherever it lives, not only under the session-owned worktree tree. An ordinary subdirectory resolves to the session's own repository and changes nothing, which is what keeps `cd packages/cli && git commit` working. Also drops the unread `entryCwd` parameter from `evaluateUnrecognizedRun`, whose signature implied containment behaviour that function never had, and covers the child-side `invocationCwd` forwarding with a direct test — it is the only link between a pinned sub-agent's real execution directory and the daemon's check. * test(acp): assert the session identity and cwd the guard now receives Adding `sessionId`/`cwd` to the guard context in `Session.runTool` changed the shape these two assertions pin, and I ran the guard, acpAgent and serve suites but not this one — CI caught what I should have. * fix(daemon): record ordinary targets a dynamic relinker re-points The dynamic-program branch resolved the operands of an unreadable program word but only used them to raise the `.git` flag, so an ordinary target was never added to the relink set: `X=ln; $X -s <outside> src && git -C src reset --hard` validated `src` against what it pointed at before the same command replaced it. Ordinary operands are now recorded alongside the `.git` case. Verified load-bearing: dropping the new line fails the added regression test and nothing else. * fix(daemon): close the round-7 escapes, verified with the reported payloads The reviewers were right that my previous round's "denies as written" replies were built on non-equivalent counter-probes: a `.git` inside the path, or a literal `-C`, tripped an unrelated marker rule while the reported mechanism went untouched. Re-run with each payload verbatim — against a path with no Git word in it — five of them were allowed. All are fixed, and the new tests use those exact payloads. - `$'…'` is ANSI-C quoting only OUTSIDE double quotes, so the substitution in `echo "$'$(GIT_DIR=<outside>/.git git reset --hard HEAD~1)'"` is live. Both scanners now gate the skip on `!single && !double`. - The export attribute sticks to the name: after `export GIT_DIR`, a LATER assignment to it reaches the git subprocess. Name-only exports of a relocation key are recorded so those assignments count as exported. - Both sides of a pipe run in subshells, so a pipe-side `cd` must not move the shell. Segments that are pipeline components restore the directory they started with; the top-level separators are read with the same quoting rules `splitCommands` uses, and any disagreement falls back to treating every segment of a piped command as a component. - A bare digit before a *spaced* redirect is a real argv word, not a file descriptor, and nothing in the token stream distinguishes it — so it is marked ambiguous and a payload built from it fails closed instead of silently dropping the word. - `-o`/`-O` before `c` in a short bundle does not cancel the `c`: bash still executes it, taking the command from a later argv entry. The bundle is now parsed for how many entries the value flags consume on either side. - Rebuilding a command line out of separate argv words re-quotes anything that would otherwise split, so a path with a space stays one word and a `-C` value cannot shrink. `eval` keeps the verbatim join it needs, since it re-parses its argument as shell text. * fix(daemon): repair the round-7 patch and bound what this guard promises Two halves. First, the round-7 patch introduced seven defects of its own, six of them reproduced here before fixing: - the fd digit of `2>…` was eligible as a `-c` payload, because `nextArgvIndex` skipped only redirect-flagged tokens; - `o`/`O` letters after `c` in a bundle were counted by presence rather than per letter, shifting the extracted payload left; - `env --split-string=` still rebuilt its payload with the verbatim join while both sibling branches had moved to the re-quoting one; - the separator scan recorded no lone `&` and mistook `>|` for a pipe, and its disagreement fallback scoped nothing instead of everything; - the export-attribute set neither crossed `eval` nor rolled back with a subshell; - deferred alias and function bodies were keyed on `run[0]` rather than on the program word, so any prefix hid them. Second, and more important than any single rule: the docs now bound what this control claims. It is reliable against Git relocation written in the literal forms the design doc lists — the mis-targeted command it exists for — and best-effort, not a boundary, against shell text written to defeat it. Seven rounds of adversarial review support that framing rather than contradict it: each round closed the reported bypasses and the next found more, several inside the rules the previous round added. The gap is structural — the guard reads command text before a shell interprets it — so the honest fix is to move the decision off the text, deciding where a command may write when it runs rather than predicting it beforehand. That is a separate change with its own design, and this one should not grow into it by accretion. Saying so plainly is itself a safety property: an operator who believes the daemon cannot reach a sibling worktree would grant it more trust than the mechanism earns. * fix(daemon): close the round-9 critical forms an agent may actually emit Scoped to the Critical findings that reproduced against the real guard with the reviewer's payloads verbatim (Git-word-free path). Common shell forms, not adversarial exotica; the parser-edge tail stays under the bounded promise this PR now documents. - `&>`/`&>>` is a redirect operator, no longer read as a background `&`. - `function NAME { … }` (the keyword form, `()` optional) is recognised as a definition. - `git -c include.path=`/`includeIf.<cond>.path=` pull in a config file the guard cannot read; it can carry a `core.worktree` redirect or executable config, so it is treated as dangerous config and fails closed. - `imap.tunnel`, `instaweb.httpd` join the command-executing config keys, and `GIT_DIFFTOOL_EXTCMD` the executed-env keys. - `GIT_DIR=… set -a` persists (a prefix assignment on the special builtin `set`) and exports; that leading assignment is now carried, not dropped. - alias/function recognition starts at the program word — past a leading redirect (`2>/dev/null alias …`), keyword (`if …; then alias …`) or assignment — and records every pair of a multi-alias statement. - a heredoc body is stdin data, not commands: it is stripped before command splitting so a body `cd` cannot launder the tracked directory. - a function body that `splitCommands` cuts across segments is now captured whole and replayed, so a `-C <outside>` inside it is seen, not just the name. Two round-9 Criticals are deliberately not "fixed" here: `cd <outside> & git …` runs git in the parent shell at the in-boundary cwd, so allowing it is correct; and an archive that plants a `.git` for a later path-less discovery is a TOCTOU (the unpack happens after the decision), left to the same limitation as the symlink race rather than denying every `tar && git commit`. * fix(daemon): replay an alias with the args its invocation appends `alias gg='git'; gg -C <outside> reset --hard` ran `git -C <outside> reset --hard`, but the guard replayed only the recorded body (`git`) and dropped the appended argv, so the relocation was invisible and the command was allowed. An alias now replays as `body + trailing args`, so the invocation's own `-C <outside>` is seen. A function is unchanged: its args arrive through `$@` inside the body, which the recorded body already carries. Verified with in-boundary controls (`alias gg='git'; gg status`, `alias gg='git commit'; gg -m x`) staying allowed. * fix(daemon): carry a function/alias body's cwd and exports to the caller A shell function and an alias both run in the current shell, so a `cd` or an export inside the recorded body survives the call. The replay discarded `nested.cwdAfter` and the exported state, so `f() { cd <outside>; }; f; git reset --hard` kept the old in-boundary tracked cwd and the path-free git mutation was judged inside while the real shell had moved outside. The nested cwd, exports and shell-locals now propagate back, exactly as an `eval` payload already does. Distinct from the earlier case where git appeared in the body itself. Verified: `f() { cd nested; }; f; git status` and `f() { echo hi; }; f; git commit` stay allowed. * fix(daemon): inherit the caller's allexport into a same-shell body A body run in the current shell — `eval`, an alias, or a function — inherits the enclosing `set -a`, so a plain `GIT_WORK_TREE=<outside>` assignment there is exported to the following git. The nested evaluation initialized `allExport` to false instead of the caller's value, so with allexport on the assignment was treated as shell-local, no relocation was recorded, and the path-free mutation was allowed. `allExport` now flows into the same-shell scopes (and back out). An unexported assignment stays shell-local and is still ignored. * fix(daemon): complete the same-shell state model for bodies and substitutions Three related gaps, all in the shell-state sharing this PR has been building: - A command substitution inherits the enclosing `set -a` but did not carry it in, so `set -a; echo $(GIT_WORK_TREE=<outside>; git reset --hard)` was allowed. The substitution scope now inherits allexport (by copy — its own changes still die with the subshell). - A same-shell body could turn allexport on but not off: the merge-back only handled the truthy result, so `set -a; f() { set +a; }; f; GIT_WORK_TREE=<outside>; git status` denied even though bash leaves the later assignment unexported. Both the function and eval merges now propagate the boolean in both directions. - Recorded function/alias definitions were local to each evaluator, so a body could not see a function the caller had already defined: `inner() { cd <outside>; }; outer() { inner; }; outer; git reset --hard` ran `inner` as an opaque command and lost the cwd. The definition tables are now shared by reference with same-shell bodies (`eval`, function/alias replay) and copied for substitution subshells. * fix(daemon): resolve shadowing and exported functions; isolate pipe subshells Four related function-model findings, all reproduced first: - A recorded function shadows the git program or a builtin, and bash resolves it before either — `git() { cd <outside>; command git status; }; git` and `cd() { command cd <outside>; }; cd nested; git reset --hard` were allowed because `analyzeRun` classified `git`/`cd` before the body lookup. Recorded bodies are now resolved before program/builtin dispatch, via a shared `invokeDefinedBody`; `command`/`builtin` name a different program word and bypass it as bash does. - A function/alias redefinition in a pipeline component runs in a subshell and must not persist, but sharing `definedBodies` (previous commit) let it leak: `f() { cd <outside>; }; f() { :; } | cat; f` was modelled as a no-op. Pipe and background components no longer record a definition into the parent, and their cwd/allexport are already rolled back. - `export -f f` makes a function visible inside a `bash -c` subprocess, unlike an ordinary function. Those names are tracked and the subprocess payload is seeded with only the exported subset; an unexported function stays invisible to `bash -c`. * fix(daemon): close the interlocking gaps in my function-model work Four gaps in the recorded-body machinery the last commits built, all reproduced first: - `invokeDefinedBody` did not carry `exportedFunctions` into the replayed body, so a `export -f`'d function invoked from another function's body was invisible to its `bash -c`. - A prefix assignment on the invocation (`GIT_WORK_TREE=<outside> gg`) was dropped, because the defined-body gate skips `analyzeRun`; the run's leading assignments are now applied to the body as ambient relocations. - The pipe-component rollback restored cwd/allexport/definitions but leaked the subshell's exports, export attributes and shell-locals into the parent; all of them now roll back. * fix(daemon): deny relocations disguised by a redirection Two reachable escapes with ordinary (non-adversarial) commands: - `cd <outside> >&2; git reset --hard` — a stderr redirect on the `cd`, whose `&` was read as a background separator so the tracked cwd was rewound while the real shell had moved outside. `>&`/`<&` file-descriptor redirects are no longer treated as backgrounding. - `git 2>/dev/null -C <outside> reset --hard` — the redirect operand among the git args ended `readGitInvocation`'s option parsing before the `-C`, so the relocation was invisible. It now skips redirect/fd-flagged tokens. Ordinary trailing redirects (`git status 2>/dev/null`) stay allowed. * fix(daemon): deny relocation hidden by a leading redirect or a background & Two more reachable escapes with ordinary commands, triaged out of the R8 batch (the rest of which is Windows paths, docs wording, test coverage or adversarial parser edges under the documented best-effort promise): - `2>/dev/null gg` where `gg` is a recorded alias/function ran the body in bash, but `readProgramWord` returned the fd token instead of the program word, so the invocation was not resolved. It now skips redirect/fd operands. - `true & cd <outside>; git reset --hard` — only the segment a `&` follows is backgrounded (a subshell); the segment after it runs in the foreground, so its `cd` persists. The pipe-component test now treats a segment as a subshell only when it precedes `&`, while both sides of a `|` still are. * fix(daemon): don't let a harmless or removed shadow mask a relocation Two escapes where the guard replayed a recorded body while the real interpreter ran a relocating external git, both reproduced first: - `export -f` functions were seeded into every subprocess shell, but only bash imports them. `git() { :; }; export -f git; dash -c "git -C <outside> reset --hard"` was allowed because the guard replayed the harmless `:` for dash, while real dash resolves the external git and relocates. Exported functions are now seeded only for a bash child. - `definedBodies`/`gitShapedNames`/`exportedFunctions` only ever gained entries, so a removed shadow still replayed. `unset -f`/`unalias` now drop the function/alias (and `-a` clears all), and `export -n -f` clears the export attribute — `git() { :; }; unset -f git; git -C <outside> reset --hard` and the `unalias git` form now deny, while a live compatible shadow (bash-imported function, an alias still in effect) stays modelled. * fix(daemon): drop exported functions when env clears the child environment Two follow-ups to the per-interpreter shadow modelling: - The `unalias`/`unset -f` removal branch compared `removalProgram` against `'unalias'` after `isFunctions` had already narrowed it to `'unset'`, which `tsc --build` rejects as a no-overlap comparison (TS2367). `isFunctions` already covers every `unalias` case, so drop the redundant term. - `env -i` / `-` / `--ignore-environment` start the child from an empty environment, so a bash `-c` payload no longer inherits the parent's `export -f` functions. The env wrapper now records that the environment was cleared and the bash payload stops importing exported functions when it was, so `git() { :; }; export -f git; env -i bash -c "git -C <outside> reset --hard"` denies while `env -i bash -c "... rev-parse HEAD"` and an un-cleared `env FOO=bar bash -c` stay allowed. Regressions added. * test(daemon): pin the sh-wrapper fail-closed contract and document it `sh` is bash on macOS and dash elsewhere, so its `export -f` import behaviour cannot be decided from the basename. The guard already treats `sh` as non-importing — it never replays an exported shadow for `sh -c`, because doing so on a dash-backed `sh` would recreate the relocation escape. Pin that fail-closed contract with a regression (`export -f git; sh -c "git -C <outside> reset --hard"` denies) so a future change that widens the bash gate to include `sh` breaks a test, and record the deliberate over-denial in the design doc's non-goals. * fix(daemon): model shell-definition removal the way the real shell does The removal-builtin handling added earlier was too broad and dropped live relocating shadows, and the exported-function set was shared into subprocess scopes by reference. Each escape below was reproduced against the guard first. - `unset` has no `-a` option and `unalias -a` clears only aliases, yet both were treated as "clear every definition", so `pwn(){ git -C <outside> reset --hard; }; unset -a; pwn` (and the `unalias -a` form) wiped the function and ran it unrecognized. Removal is now kind-aware: `unalias` touches only aliases, `unset -f`/bare `unset` only functions. - A function shadowing `unset`/`unalias`/`export` runs instead of the builtin, so the removal never happens; the branch now fires only when the name is not itself a recorded shadow, and otherwise falls through to replay the shadow. - The bash `-c` subprocess and command-substitution scopes received the parent's `exportedFunctions` set by reference (or, for `$( )`, not at all), so a child `unset -f` retracted the parent's export and a substitution saw none. Both now take a copy. Adds regressions for each and keeps the existing shadow/removal cases green. * fix(daemon): bare unset keeps the function and env -u strips exported functions Two more escapes doudouOUC reproduced in the removal model, both verified against the guard first. - A bare `unset NAME` unsets a same-name variable first and removes the function only when none exists. This evaluator tracks no ordinary variables, so it cannot tell the two apart; treating every bare `unset NAME` as a function removal dropped a live relocating shadow (`pwn(){ git -C <outside> …; }; pwn=1; unset pwn; pwn`). Only `unset -f` now removes a function; a bare `unset` leaves it, the safe over-deny choice. - A bash `export -f foo` travels as a `BASH_FUNC_foo%%` environment entry, so `env -u BASH_FUNC_foo%%` (and the `--unset=` / attached forms) strips it before `bash -c` and the child runs the real program. The env wrapper now records unset keys in PrefixState and the payload seeding drops functions whose `BASH_FUNC_*` entry was removed, so a stripped harmless `git` shadow no longer masks the real relocation. Adds regressions for both; keeps `unset -f`, unrelated `env -u`, and live shadows behaving as before. * fix(daemon): fail closed when a removal builtin could retract a tracked shadow Modelling exactly which definition an `unset`/`unalias`/`export -n` removes is general shell semantics this guard does not attempt: a bare `unset NAME` drops a same-name variable before the function, `enable -n unset` turns the builtin into a no-op, a `command`/`builtin` prefix or a `( … )` subshell changes what runs, and fused flag clusters (`-nf`) hide the mode. Every attempt to model these precisely left a live relocating shadow reachable through a form it did not cover. Collapse the whole removal path to one rule: when a removal references a name tracked as a shadow (a defined body, a git-shaped name, or an exported function) — or clears all while any shadow exists — fail closed. This denies the previously-allowed `git(){ :; }; unset git; git -C <outside> …`, `export -nf`, `command unset -f`, `enable -n unset; unset -f`, and `( unset -f git ); git` forms, while a removal of an untracked name and every live-shadow replay behave exactly as before. Removes the earlier kind-aware bookkeeping the same escapes kept slipping through. * fix(daemon): skip leading redirections before the removal-builtin prefix scan The `command`/`builtin` strip in the shadow-removal guard started at raw token zero, but bash strips redirections from argv. A leading `2>/dev/null` before `command unset -f <tracked-function>` left the scan looking at the redirect operand, so `command` was never consumed, `readProgramWord` returned `command` rather than `unset`, the removal went unrecorded, and the stale harmless function masked the later external Git relocation. Skip redirect/fd operands before and between the `command`/`builtin` prefixes, the same normalization `readProgramWord` applies. Adds the leading-redirection variant to the command-prefix regression. * fix(daemon): replay a shadowed removal builtin and drop unset variables Two more escapes doudouOUC reproduced in the fail-closed removal rule. - The rule's early `continue` fired even when `unset`/`unalias`/`export` was itself a recorded function and the operands named only untracked state, so a shadowing `unset(){ git -C <outside> …; }; unset other` was classified as a harmless builtin removal and never reached the shadow dispatch that replays the relocating body. The branch now runs only when the program is not a shadowed function (a `command`/`builtin` prefix still forces the builtin). - The removal never dropped tracked variables, so `A=nested; unset A; cd $A` kept expanding the stale in-bounds value while bash's `unset A` leaves `$A` empty and `cd $A` lands at $HOME. `unset NAME`/`unset -v NAME` now deletes the shell-local, turning the later `$A` into an unresolved reference the cd fails closed on. `unset -f` is functions-only and leaves variables intact. Adds regressions for both. * fix(daemon): honor shadowed command/builtin prefixes and PATH-based relocation Two escapes surfaced by the round-11 review, both reproduced against the guard. - The shadow-removal prefix scan trusted a literal `command`/`builtin` word to force the real builtin, but bash resolves a function of that name first. A `command(){ git -C <outside> …; }; command unset other` therefore early-continued as a harmless builtin removal and never replayed the relocating body. The prefix loop now stops when the prefix word is itself a recorded shadow, leaving it for the normal shadow dispatch. - The unrecognized-program marker scans covered GIT_DIR/GIT_WORK_TREE-family assignments but not GIT_PROGRAM_ENV_KEYS (`PATH`/`GIT_EXEC_PATH`), which decide which git binary runs. The direct `PATH=/evil git …` was denied while `find … -exec sh -c 'PATH=/evil git …'` slipped through. Both marker scans now include those keys, and they remain gated on a co-present git word so an ordinary `PATH=… make` is unaffected. Adds regressions for the shadowed prefixes and the wrapped PATH/GIT_EXEC_PATH forms. * fix(daemon): catch delimiter-glued relocations and redirect-decoy prefix drops Two escapes surfaced by the round-12 review, both reproduced against the guard. - The env-assignment arm of both text marker patterns required `(^|\s)` before the key, while the sibling `cd`/`pushd` arm already allowed `;&|(){}` boundaries. A relocation glued to a delimiter inside a quoted wrapper payload (`su -c 'true;GIT_DIR=<outside> git reset --hard'`) therefore evaded the unrecognized-program backstop. Both arms now share the same boundary class. - `invokeDefinedBody` located the invoked name with a raw `findIndex` that also matched redirect operands, so a decoy `> g` whose target equals the function name truncated the prefix-assignment scan to empty and dropped the call's `GIT_DIR=` relocation. The lookup now skips redirect/fd operands like `readProgramWord` does. Adds regressions for the delimiter-glued and redirect-decoy forms. * fix(daemon): deny trailer/man/sendemail command-executing config keys The dangerous-config model already denies `git -c <key>=<command>` for the command-executing config families, but omitted three documented ones: `trailer.<token>.command`, `man.<tool>.cmd`, and `sendemail.(sendmailcmd|tocmd|cccmd)`. `git -c trailer.sign.command='…' interpret-trailers` (and the man/sendemail forms) ran the configured shell command while the guard allowed it. Adds the three patterns and regressions. --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-14 09:55:59 +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
Shaojin Wen	407cf0a7f8	feat(serve): adaptively grow live-journal caps before truncating mid-turn replay (#8905) ... * feat(serve): adaptively grow live-journal caps before truncating mid-turn replay A single turn fanning out many concurrent subagents (e.g. a /review run) can emit hundreds of thousands of source events, far past the per-session live-journal baseline caps (10 000 entries / 8 MiB), so a mid-turn (re)load silently shows a truncated replay until the turn finishes. Before evicting, the engine now asks a growth advisor: caps double (entries scaled proportionally) while the growth granted across the bridge's live sessions fits in a pool derived from the daemon memory budget (5%, clamped to [32, 1024] MB), never past a per-session hard cap of 256 MiB. Growth is on demand, throttled after a refusal, and accounted statelessly from the current caps of all live sessions, so granted headroom dies with its session. An operator-pinned --max-journal-events/--max-journal-bytes disables growth; without a pool the fixed-cap eviction behavior is unchanged. * fix(serve): address adaptive live-journal growth review feedback (#8905) * fix(serve): account in-flight restores in the journal growth pool (#8905) Concurrent restores hold their buses in pendingRestoreEvents rather than byId, so each advisor ask only saw its own caps and concurrent restores could each draw a full doubling from the same pool. Sum the current caps of every in-flight restore bus into allSessionLimitBytes. Also skip the growth ask when the breaching append is a turn boundary — compactCurrentTurn discards the journal immediately afterwards, so the grant would be charged to the pool while buying zero eviction. Pin the previously untested contracts with tests: restore-window accounting, concurrent-restore accounting, headroom release on session close, the hard-cap clamp term, partial-grant eviction, requester discrimination in the policy fixtures, the maxEvents safe-integer conjunct, and the dynamic-workspace bridge pool wiring. Fix the docs: add the missing journal-flag rows to the daemon configuration and operations pages, and correct the effective-budget definition. * test(serve): request 'response' replay in the transport-failure test (#8905) The merge of main pulled in #8933, which gates historyPageSize on historyReplay === 'response'. The 'transport failure marks the channel dying before process exit' test (from #8947) passes historyPageSize with the default stream replay, so the paged transcript fetch it waits on is never issued and the test times out — a cross-PR interaction between two main commits, failing deterministically on main. Pin the response replay mode the paged fetch requires. * fix(serve): share one daemon-wide journal growth pool (#8905) Address the automated review of adaptive live-journal growth: - The growth pool is now one daemon-wide aggregate shared by every workspace bridge instead of a full pool per bridge, and growth is disabled when the budget is insufficient or leaves no headroom after the root reserve. - Grants that cannot retain any additional journal entries (an oversized event survives as the sole entry either way) are refused so the pool is never charged for growth that preserves no replay. - The refusal throttle defaults to a monotonic clock and treats a backward clock jump as an elapsed window. - The proportional event hard cap is clamped to MAX_SAFE_INTEGER so a valid-but-extreme baseline cannot poison every grant. - /daemon/status reports the growth semantics: limits.memory.journalGrowth (pool size, hard cap, baselines), per-session effective caps in full diagnostics, and enforced:false scoped to the child-heap model. - Validation-boundary tests for the growth-pool normalizer and doc fixes (positive safe integer types; growth toward double, limited by pool headroom). * fix(serve): align growth-pool docs and harden growth tests (#8905) * fix(serve): account growth per session baseline and walk intermediate grants (#8905) * fix(serve): harden growth-pool tests and derive help figures from constants (#8905) * fix(serve): reject valueless journal cap flags and harden growth tests (#8905) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.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-13 11:35: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
qqqys	de48637aa0	refactor(serve): default project memory to workspace scope (#8856) ... * refactor(serve): default project memory to workspace scope * fix(serve): preserve launch env access guard * fix(serve): harden project memory scope resolution * fix(serve): harden project memory scope diagnostics * fix(serve): keep memory scope operator-owned --------- Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-12 02:38:09 +00:00
Dragon	ac78acd3c5	fix(core): resolve Qwen 3.8 reasoning budget conflicts (#8525) ... * fix(core): resolve Qwen 3.8 reasoning budget conflicts * fix(core): cover unconfigured Qwen 3.8 conflicts * chore: preserve latest main formatting * fix(core): harden DashScope thinking precedence * fix: honor DashScope thinking knob precedence * test(core): assert same-layer thinking knob drop warning for request pairs (#8525) * fix: align effort override reporting with wire resolution * fix: resolve thinking knob review findings * fix: sort Python SDK test imports * fix(core): ignore null thinking knobs * fix(core): register enable_thinking true in thinking knob selection (#8525) selectFromLayer only registered enable_thinking === false, so a higher-priority enable_thinking: true was invisible to cross-layer resolution: a lower-priority samplingParams disable won selection and rewrote the shipping tier to reasoning_effort 'none', inverting the documented extra_body > samplingParams precedence. Register the on-switch as the weakest knob in its own layer (an off-switch rewrites the tier, an on-switch never does) and make the drop branch value-aware: true keeps the shipping tier and drops only the redundant knobs, false keeps the canonical 'none' disable. getReasoningEffortOverride no longer reports an on-switch as shadowing the tier (the wire drops the switch and ships the tier), except for a request-level effort override that still shadows from under it. Also corrects the dropConflictingThinkingKnobs contract comment (only effort tiers ship alone; the 'none' disable and a winning budget keep a co-present enable_thinking) and the model-providers.md precedence callout, which overstated samplingParams precedence for older qwen hybrids where the reasoning-derived enable_thinking: true overrides it. * fix(core): preserve budget beneath thinking on-switch * fix(core): canonicalize disabled thinking knobs * fix: resolve round-6 thinking knob review findings (#8525) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): read the response body with a reader, not for-await (#8525) Port of #8764 (10621b3a93) to this branch. Async-iterating a ReadableStream needs [Symbol.asyncIterator] on the TYPE, and whether it is there depends on which lib set the program resolves — @types/node's stream has it, the DOM lib's needs lib.dom.asynciterable. This branch predates #8693, whose tsconfig "types" guard keeps @types/jsdom's lib.dom out of the package program; the autofix verification build resolves node_modules from the trusted base (which has @types/jsdom), so the guardless branch fails the build with TS2504 on the `for await`. The reader loop types identically in every lib set, so the build no longer depends on that resolution. Behavior is unchanged and pinned by the regression tests ported from the same commit: multi-chunk assembly, the exact MAX_RESPONSE_BYTES boundary, oversize rejection with stream cancellation, deferred-cancel sequencing, mid-stream read failure, and invalid-UTF-8 rejection. The Mem0-related changes that share main's http-client.ts (#8507) are intentionally not ported. * fix(sdk-python): expose effort status reason from CLI (#8525) The CLI emits a human-readable reason on effort_status and the TypeScript SDK surfaces it, but the Python EffortStatus TypedDict and _parse_effort_status dropped it, leaving Python callers to reconstruct the reason from override. Add reason as an optional field and pass it through, mirroring the TypeScript parser. * test(core): add direct unit tests for selectDashScopeThinkingKnob (#8525) 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-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-12 01:49:01 +00:00
callmeYe	a64d1291d2	feat(extensions): support Agent Plugins v1 (#8834) ... * feat(extensions): support Agent Plugins v1 * fix(extensions): address Agent Plugins review blockers * fix(extensions): address second review blockers	2026-08-11 19:45:11 +00:00
Nothing Chan	e6a3272271	feat(cli): expose reasoning effort through ACP (#8526) ... * feat(cli): expose reasoning effort to ACP clients * fix(cli): address ACP reasoning effort review * fix(cli): address ACP reasoning effort review round 2 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): migrate /effort dialog to applyReasoningEffort helper Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp): harden set_config_option routing and rejection messages (#8526) --------- 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-Coder <qwen-coder@alibabacloud.com>	2026-08-11 10:32:33 +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
jinye	60c18256b6	feat(cli): clean up OpenAI logs in non-interactive sessions (#8893) ... * feat(cli): clean up OpenAI logs in non-interactive sessions Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#8893) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#8893) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-11 07:09:28 +00:00
顾盼	e20601d6c4	fix(cli): switch completion tabs with bare arrows (#8576)	2026-08-11 02:29:11 +00:00
Shaojin Wen	95e17691a9	chore(serve): remove the /demo debug page (#8805) ... * chore(serve): remove the /demo debug page The daemon has shipped a real browser UI for a while: `resolveWebShellDir()` finds the bundled Web Shell assets and `mountWebShellAssets()` serves them at `/`, so `qwen serve` already opens onto a full client. `/demo` stayed behind as a 663-line inline-HTML console covering the same ground with none of the reach — nobody drives the daemon through it, and `npm run dev:daemon` starts the Web Shell dev server rather than the demo page. Keeping it around costs more than the dead code. It is the only file in the tree that pairs an event log with daemon HTTP, so work that starts as a Web Shell observation lands there instead: #8762 was found while running `/review` through the Web Shell and was fixed entirely inside the demo page's rendering, with "no Web Shell changes" in its own risk note. Deleting the page removes that decoy. Nothing is lost for protocol-level debugging: `GET /session/:id/events` streams the same raw frames the Events tab printed. `/health` shared `routes/health-demo.ts` with the demo handler, so the module is now `routes/health.ts` / `createHealthRoutes()` and drops its `getPort` dependency. The rate-limit exemption, the boot breadcrumb, and the daemon docs lose their `/demo` arms; the loopback self-origin shim regression test already asserted through `/health` and only needed its title corrected. * test(serve): pin the removed /demo contract and the pre-auth surface Review follow-up. Three of the removal hunks shipped ungated, and two doc sentences the removal rewrote were describing the pre-auth surface wrong — both before and after the edit. Deleting the `/demo` route took its assertions with it, so nothing failed if the handler came back: the Web Shell suite only exercised a generic deep link, and the rate-limit exemption could be widened again with the suite still green. `/demo` is now pinned as what it became — an ordinary unknown path: a non-navigation request 404s, a browser navigation is answered by the SPA fallback like any other deep link, and once a token is configured (with or without `--require-auth`) that navigation is refused with 401, because the fallback sits behind the bearer. The rate-limit test pins that `/health` is the only exempt GET, so re-adding a second pre-auth page to the predicate fails instead of silently escaping the limiter. Each new assertion was checked by reverting the hunk it guards and confirming it goes red. The `--allow-origin '*'` warning and both `--allow-origin` doc paragraphs enumerated `/health` as the residual tokenless surface and said nothing about the Web Shell static assets, which are mounted before the bearer in every launch mode and stay reachable even under `--require-auth` — the enumeration also claimed `/health` stays pre-auth on non-loopback binds, where it is registered behind the bearer and 401s. A probe across all three launch modes established the actual matrix; the warning and the docs now match it and name `--no-web` as the way to remove the residual browser surface. The warning text is asserted by a test for the first time. * fix(serve): correct Web Shell doc claims and re-pin the pre-auth CORS wall Review follow-up. The removal rewrote the daemon docs around the Web Shell, and three of the rewritten claims did not match what the runtime actually does: §1 never said how the bearer reaches the browser (with auth on, the plain URL loads a shell whose every API call 401s), §8 called the shell writable on any bind (on a non-loopback bind without `--allow-origin` its POSTs hit the CORS wall and 403), and §8 served `/session/:id` without the document-navigation qualifier its own code enforces. The §9 call-chain diagram also still listed the deleted `/demo` route, the developer flag references had no `--web`/`--no-web` row despite the new guidance pointing at the flag, and both design docs listed the JSON body parser ahead of post-auth `/health` while `createServeApp()` registers them the other way round. The deleted `/demo` CORS test was also the only assertion that a pre-auth page sits behind the Origin wall — every surviving Origin test targets an API path. Re-pin it for the shell root so a mount-order regression fails instead of exposing the pre-auth HTML surface cross-origin. * fix(serve): finish demo rename sweep and scope pre-auth shell claims to loopback 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-Coder <qwen-coder@alibabacloud.com>	2026-08-10 13:31:15 +00:00
jinye	a292c89a3b	feat(cli): add background cleanup for OpenAI API logs (#8862) ... * feat(cli): add background cleanup for OpenAI API logs With model.enableOpenAILogging on, every API call appends a full request/response JSON under logs/openai with no rotation — heavy usage accumulates hundreds of thousands of files (tens of GB) within months. Register a third cleaner in the existing background housekeeping pipeline that sweeps openai-*.json files older than the new model.openAILogRetentionDays setting (default 7 days). The filename-embedded UTC date is used as a fast path to avoid one stat() per file; the boundary day and unparseable names fall back to mtime. Throttling is keyed on the resolved log dir, so both the default per-CWD layout and a shared custom openAILoggingDir are swept at most once a day. The sweep runs regardless of whether logging is currently enabled, so residue from earlier debugging sessions still gets cleaned. Scope note: housekeeping only starts for interactive sessions, so headless (-p) / SDK processes are not covered yet. * codex: address PR review feedback (#8862) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-10 12:20:33 +00:00
jinye	fa8cae5418	fix(serve): Allow approved external built-in text writes (#8852) ... * fix(serve): allow approved external built-in text writes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): keep write provenance off startup bundle Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-10 12:19:58 +00:00
Shaojin Wen	7c89665be1	fix(cli): extend the #8663 loader denylist and harden its scrub lifecycle (#8763) ... * fix(cli): extend the #8663 loader denylist and harden its scrub lifecycle Follow-up to #8663. Its inherited-env denylist closed the NODE_OPTIONS/ NODE_PATH class but left sibling code-execution and TLS-trust-anchor vars that reach the same #8653 cross-workspace outcome — an untrusted workspace `.env` is frozen into daemonRuntimeBaseEnv and distributed to every workspace's session subprocesses. Denylist additions, split by the PR's own tiering: - Scrubbed loader tier (INHERITED_LOADER_ENV_KEYS — scrubbed from the inherited launch env and rejected from every `.env`/settings.env scope), for pure-injection vars with no legitimate operator-shell use: OPENSSL_CONF (startup dlopen of an attacker OpenSSL engine), NODE_REPL_EXTERNAL_MODULE, npm_config_node_gyp, npm_config_init_module. - Reject-from-project-`.env` tier (PROJECT_ENV_HARDCODED_EXCLUSIONS — rejected from project files, preserved from the shell / home `.env`), for vars with a legitimate operator-shell use whose only exposed vector is an untrusted project file: * TLS trust anchors SSL_CERT_FILE, SSL_CERT_DIR, CURL_CA_BUNDLE, REQUESTS_CA_BUNDLE, GIT_SSL_CAINFO (siblings of NODE_EXTRA_CA_CERTS; an attacker CA MITMs a session's git/npm/pip/curl traffic). * git command-execution family GIT_SSH_COMMAND, GIT_EXTERNAL_DIFF, GIT_CONFIG_GLOBAL/SYSTEM/COUNT and the numbered GIT_CONFIG_KEY_<n>/ GIT_CONFIG_VALUE_<n> pairs (matched by prefix). core/utils/git-branches.ts already scrubs these from the repo's own git invocations. * node-gyp interpreter selection NODE_GYP_FORCE_PYTHON, npm_config_python, PYTHON (run as the build Python during native-addon installs). Concurrency: the daemon's process.env scrub/restore and the loader-key rejection reporter were process-global with no guard for concurrent embedded daemons in one process (a documented supported config). The first daemon's close() restored loader vars into the shared env, re-poisoning a still-live sibling's sessions, and dropped its reporter. The scrub is now reference counted (acquireInheritedLoaderEnvScrub — snapshot on first acquire, restore only on last release) and the reporter is cleared only when still active. Test hardening from the same review: pin the daemon-worker scrub breadcrumb (not just key removal); pin the fast-path settings.env case-folded hardcoded-exclusion gate; drain the module-global fast-path stash so the accumulate assertion is order-independent. Docs updated for the new keys. * fix(cli): keep the loader-scrub process.env access in the serve guard surface The refcounted acquireInheritedLoaderEnvScrub read/wrote process.env from config/shared-env-keys.ts, which the serve process.env guard does not scan — moving the access out of run-qwen-serve.ts dropped its allowlisted count and failed process-env-guard.test.ts. Pass the env into the coordinator instead so run-qwen-serve.ts still owns the process.env reference (matching the existing scrub helpers), and update the allowlist to the new count. * fix(cli): block GIT_SSH and GIT_CONFIG_PARAMETERS in the project-env denylist Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): extend the project-env denylist across git exec, TLS, and rc-file tiers Close the round-2 review findings: block the remaining git command-execution siblings (GIT_EXEC_PATH, GIT_TEMPLATE_DIR, GIT_ASKPASS, GIT_PROXY_COMMAND, GIT_EDITOR), the npm/pip TLS trust knobs (npm_config_cafile, npm_config_ca, npm_config_strict_ssl, PIP_CERT, GIT_SSL_CAPATH), and the curl/wget rc-file redirects (CURL_HOME, WGETRC) from project .env files. Freeze the numbered GIT_CONFIG_KEY_/VALUE_ pairs on reload together with GIT_CONFIG_COUNT, and sync the qwen-serve.md loader-key enumeration with settings.md. * fix(cli): harden the project-env denylist and nested scrub snapshot (#8763) * fix(cli): merge the loader-env scrub snapshot into one pass (#8763) acquireInheritedLoaderEnvScrub iterated process.env twice (a snapshot pass, then the scrub); record the originals inside the scrub's single pass instead. Drop the acquire-time snapshot clear, which the release-time clear made unreachable defense, and add tests that kill the previously surviving mutants on the release-time clear, the test-only reset, and the undefined-value guard. * fix(cli): block the round-4 exec-redirect env keys from project files (#8763) --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-10 11:27:30 +00:00
Shaojin Wen	77bd04bd61	fix(acp-bridge): bound live journal replay chunks (#8801) ... * fix(acp-bridge): bound live journal replay chunks Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): isolate shell retention sidecars Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(integration): cover aggregated live journal replay Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(core): isolate registry sidecars Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp-bridge): keep unmodeled chunk keys out of live journal merges The merged live-journal entry is rebuilt by spread-merging the first and last source events, which was only safe because producers happen to emit exactly {sessionUpdate, content, _meta?} on mergeable chunks. Gate the merge on that key set so unmodeled data/update fields keep entries discrete instead of leaking into the aggregate. Also clarify the live-journal truncation marker: its retained/truncated counts describe source events, while the limits count replay entries. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp-bridge): align replay boundaries for discrete and meta-shaped chunks Turn compaction folded discrete thought chunks (and non-todo-stop-guard discrete messages) into one text slot with the last chunk's meta, while the live journal keeps every discrete chunk separate — resyncing from compactedReplay mis-attributed text across background tasks. Guard both chunk paths with the same hasDiscreteMessageMeta predicate the live journal already uses. Also align the merge gate with the shapes the shared meta builder emits: tolerate update-level timestamp/ serverTimestamp and qwenTranscript.planToolCallId, and treat an empty-string parentToolCallId as top-level the way the extractor does. Document that byte-cap truncation drops whole entries, so the retained tail can be much smaller than the cap, and tighten the integration assertion that became vacuous once entries merge source chunks. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp-bridge): merge subagent chunks in live journal replay SubAgentTracker stamps every streamed subagent fragment with { parentToolCallId, subagentType }, but the live-journal merge gate only modeled parentToolCallId, so subagent chunks stayed discrete and a high-fragment subagent stream could still trip history_truncated. Model subagentType as a carried label (like the completed-turn path, which merges by parentToolCallId alone) and cover the producer wire shape in the merge tests. * fix(acp-bridge): preserve TextContent metadata in live journal replay (#8801) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>	2026-08-10 09:52:17 +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
jinye	0a3d7bb5c1	feat(acp): Protect against repeated tool execution failures (#8469) ... * feat(acp): protect repeated tool execution failures Add a conservative prompt-local guard for repeated typed ACP tool execution failures, with shadow/warn/enforce rollout modes, privacy-safe telemetry, and coverage for the final execution outcome contract. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp): harden repeated tool failure guard rollout Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp): address repeated failure guard review Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(acp): improve repeated failure guard recall Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(acp): clarify review and rollout gates --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-09 15:47:38 +00:00
jinye	a810f7e16c	fix(serve): Make session restore timeouts safe and observable (#8691) ... * fix(serve): make session restore timeouts safe Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): restore missing core mock exports in the ACP worktree suite The restore-tracing change added `extractDaemonTraceContext` and `withDaemonSpan` to `acpAgent.ts`, but `acpAgent.worktree.test.ts` replaces `@qwen-code/qwen-code-core` with a full mock factory that never listed them. `loadSession` then failed on an undefined export, taking all three cases down and producing teardown rejections from the half-built agent. The sibling suite was updated; this one was missed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): bound and disambiguate the abandoned restore lifecycle Four follow-ups from review of the restore timeout work. A startup budget may now raise the restore budget but never lower it. Taking an explicitly configured `initializeTimeoutMs` as the restore fallback meant a deployment that tightened its child-initialize check still inherited a sub-default restore deadline — exactly the failure this change exists to remove. An explicit `sessionRestoreTimeoutMs` still wins outright, including below the default, for deployments that want restore to fail fast. Validation now names the field actually at fault. A restore fenced behind a timed-out predecessor is no longer reported as an ordinary in-flight restore. It carries `reason: awaiting_abandoned_cleanup` and a retry hint of one restore budget (capped at 120s) instead of the ordinary 5 seconds, because the fence cannot clear until the non-cancellable ACP request settles and a 5-second cadence just spins the caller against a 409 it cannot resolve. Whether a channel is condemned is now derived rather than sticky. A timeout recorded `emptyReapPending` permanently, so any channel that had ever seen one was guaranteed to be reaped once its remaining work drained, forcing a cold respawn even when the late restore had landed and closed cleanly. The reap condition is now computed from an outstanding `unsettledAbandonedRestores` set, quarantine, or an ordinary pending empty reap; real settlement clears the entry and hands the channel back to the configured idle policy. Abandonment no longer retains ownership without bound. One further restore budget after the deadline, a still-unsettled restore marks the channel `restoreSettlementOverdue`: existing sessions and workspace control keep working, but fresh session work is refused so the channel can drain, since closing the transport is the only lever that releases a permanently hung request. Releasing capacity while hidden work runs would allow unbounded oversubscription, and force-killing a channel with live siblings would reintroduce the failure this work removes, so neither is done. Fresh-admission blocking is now scanned across alive channels rather than tracked in a single reference, so a second condemned channel cannot silently displace the first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): keep the abandoned restore lifecycle off ids it no longer owns Two correctness gaps in the abandoned-restore machinery introduced by this PR, both reported by automated review and both confirmed by mutation testing (each new test fails when its fix is reverted). A caller-supplied `sessionId` is used verbatim by the agent, but `spawnOrAttach` never consulted `inFlightRestores`. A fresh spawn could therefore take an id that a restore still owns, in either lifecycle phase. The consequences were silent: `abandonedRestoreIds` suppresses session updates, guardrail events, and child notifications, so the new session would have registered successfully and then emitted nothing; and a late `settleAbandonedRestore` would have closed and tombstoned it out from under its owner. Such a spawn is now rejected with the same `RestoreInProgressError` and reason the restore path uses, so the caller gets the correct retry hint for whichever phase is holding the id. The cleanup path is guarded independently, because the request-level check only covers the id the caller asked for and a session registers under the id the child returns. An abandoned restore never reaches `createSessionEntry` — the deadline rejects before registration — so any live entry under that id belongs to someone else. Cleanup now detects that and returns without closing or tombstoning, releasing its own bookkeeping instead. The notification fence has no TTL and was only cleared by `markRestoreInFlight`, which covers a subsequent restore and nothing else. `createSessionEntry` now clears it for every registration route, so a legitimate owner of the id is never handed a session that silently drops everything the child sends it. Also tightens two tests that could not observe the values they pin. The SDK default restore timeout admitted any value in (30s, 70s]; it is now split at the exact boundary, so collapsing the default onto the 60s server budget — which would make the client abort race the daemon's own deadline and cost the caller its structured 504 — fails. And the advertised-budget propagation from capabilities through to the SDK call had no live-path assertion; dropping the capabilities argument at the real call site left every existing test green. The `as never` casts are replaced with typed `DaemonCapabilities` values so a field rename fails typecheck. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): let a condemned channel drain without its wedged child Merging main's active-work close protocol (#8588) into this PR's abandoned restore bound produced a deadlock that neither side has on its own, and the conflict resolution was committed without running tests. `maybeCloseIdleSession` now routes through `confirmChildUnheld`, which asks the child whether it still holds work before closing a session nobody is attached to. That is right in general and wrong for a channel this PR has already condemned. `restoreSettlementOverdue` and quarantine exist precisely because the child stopped being answerable, and their whole premise is that visible work drains so the channel can be reaped — closing the transport is the only thing that can release a restore we cannot cancel. Making that drain depend on a round trip to the wedged child inverts it: a child stuck in a non-cancellable restore is exactly the one that cannot reply inside `ACTIVE_WORK_CLOSE_TIMEOUT_MS`, so the sessions never close, the channel never drains, the reap never fires, and the bound never takes effect. A channel condemned by the restore lifecycle now skips the round trip and proceeds to local teardown. Nothing is attached to the session by then — `maybeCloseIdleSession` gates on that — and the sibling-safety invariant is untouched: this closes sessions whose clients have already left, it does not force-kill a channel that still has live ones. The regression test drives an overdue channel whose child never answers the close-if-unheld probe and asserts the detach still reaps it. Reverting the guard reproduces the deadlock as a test timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(serve): pin the restore-timeout contract the review found unasserted Automated review identified eleven places where the restore-timeout work's behavior was correct but unpinned — each with a mutation that ships green. Every fix below was verified the same way: apply the mutation, watch the new assertion fail, revert, watch it pass. The timeout path's telemetry had no coverage at all, which is the sharpest gap given that observability is what this work exists to deliver. A shared recorder now asserts the public timeout result and its kill_empty-vs- fence_shared signal, the late arrival, and the cleanup outcome for both the closed and quarantined cases. The deadline timer's cancellation on a successful restore was likewise unpinned: deleting both `clearTimeout` calls kept the whole suite green, while in production the stale timer fires one budget after a successful restore and abandons a live session — fencing its frames, closing its event bus, and emitting a spurious timeout. A success-path test now advances past the deadline and asserts no second public result. Three more bridge assertions proved less than they claimed: the concurrent- restore case never checked that the abandoned restore settles, the workspace-control case never checked that the deferred reap eventually fires, and the resolver never pinned the accepting side of the MAX boundary (a `>` to `>=` mutation rejects the largest legal delay at boot). The workspace-control case also needed a positive channel idle budget, since with the default zero the idle-timer kill substitutes for the reap junction under test; its assertions are rewritten around the derived reap semantics rather than the sticky flag they predate. Outside the bridge: the scheduled-task timeout wiring had no test, so deleting the arguments silently fell back to the helpers' own defaults; the cold restore path never asserted that `live_restore_ms` is absent; the SDK's per-request validation and its over-ceiling clamp were untested; the WebUI watchdog test jumped straight to its own value, staying green for any watchdog at or below it, including the 30s attach value that would recreate the original symptom in the browser; and the two new known error types were unexercised, so dropping either would relabel every restore-timeout and quarantine error as unknown. Two review items are deliberately not taken here and are recorded in the design doc's non-goals instead: transcript materialization is still not separately attributable from `config_setup`, which needs instrumentation inside the core session loader that P1/P2 restructures anyway, and sibling event-loop latency during a large restore remains unmeasured. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): bound the condemned-channel close and complete the fence contract Second automated review round, on the code the first round produced. One Critical and twelve suggestions; all verified by mutation before and after. **The Critical is a regression I introduced.** Letting a condemned channel skip the bounded hold probe routed it into `closeSessionImpl`, whose agent close is unbounded when it throws on failure — so the fix traded a bounded wait on a wedged child for an unbounded one. A settlement-overdue channel with an unresponsive child would hang `detachClient` forever, strand the session in `closing`, never drain, never reap, and 503 every new session until restart: strictly worse than before. `CloseSessionOpts` now carries an `agentCloseTimeoutMs` that the condemned path sets, so a hang lands in the existing unknown-outcome recovery, which kills the channel — the teardown the drain was waiting for. The earlier test missed this because its fake child still answered the plain close; it now answers nothing at all, and asserts the detach itself returns. **The fence was invisible on the transports clients actually use.** `toRpcError` had no `RestoreInProgressError` case, so over acp-http and acp-ws — which SDK negotiation prefers over REST — the fence degraded to an opaque internal 500 with no code, reason, or hint, and the backoff contract this work documents was impossible to honor. **Two retry hints still advertised five seconds for states that outlive a budget.** The restore 504 creates the fence, and quarantine lasts until the channel drains; a fresh-id caller never reaches the 409 that carries the real hint, so its header was the only signal it got. Both now derive from the budget through one shared clamp helper, which also replaces the formula that was inlined in the bridge and gives the documented 5-120s bounds a test. **A spawn collision reported an operation the caller never issued**, naming the restore owner's action as both the active and the requested one and telling the caller to retry an endpoint it never called. The rest: five places still described the initialize-timeout fallback as a plain chain rather than raise-only, contradicting sibling docs shipped in this same PR; the design doc omitted the retry-hint clamp; the protocol reference omitted the new spawn emission site; the error taxonomy omitted `restore_settlement_overdue`, which matters because its audience is monitoring. Test-only gaps: the dynamic 409 had no HTTP-layer coverage, the 120-second cap was unpinned, and the SDK's precedence of an explicit global timeout over the advertised budget was pinned only branch-by-branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(serve): preserve restore session ownership handoff Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-09 15:47:09 +00:00
易良	bf84caf173	feat: add Local Control pairing to CLI and Desktop (#8727) ... * feat(cli): add Local Control pairing * fix(cli): address Local Control review feedback * fix(cli): allow Local Control loopback origin * feat(desktop): add Local Control pairing * fix(local-control): bound unauthenticated connections * test(desktop): allow Windows proxy cleanup * test(desktop): avoid socket cleanup timing * fix(desktop): surface Local Control status * fix(desktop): simplify Local Control window * fix(desktop): harden Local Control pairing * fix(desktop): bind Mac wake lock to app	2026-08-09 08:31:54 +00:00
callmeYe	39377fcff3	feat(daemon): add batch skill toggle API (#8664) ... * feat(daemon): add batch skill toggle API * test(serve): update capability integration baseline * fix(daemon): apply skill batches atomically * test(daemon): pin Skill batch toggle contracts and fix docs examples * test(daemon): pin Skill batch toggle mutants flagged in review * test(daemon): cover Skill batch toggle edge cases * docs(daemon): clarify Skill batch toggle contract notes from review Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(daemon): pin Skill batch toggle cap semantics and SDK surface shape * test(daemon): pin Skill batch toggle mutants flagged in round-5 review --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-08 23:22:04 +00:00
callmeYe	33b124321c	feat(core): support Qoder plugin extensions (#8661) ... * feat(core): support Qoder plugin extensions * fix(core): address Qoder extension review feedback * fix(core): handle annotated tags and unsafe parse errors * fix(core): harden Qoder conversion edge cases * fix(core): sanitize Qoder conversion inputs * fix(core): address Qoder extension round-3 review feedback Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(core): honor explicit marketplace selection over Qoder manifest * fix(core): preserve nested plugin update provenance --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-08 22:35:54 +00:00
Shaojin Wen	bb8f2c0129	fix(cli): scrub inherited loader env vars from daemon session subprocesses (#8663) ... * fix(cli): scrub inherited loader env vars from daemon session subprocesses Daemon-mode sessions bound to one workspace inherited loader-affecting env vars (NODE_OPTIONS with dev-harness --import hooks, NODE_PATH, preload-class vars) from whatever shell launched the daemon, so subprocesses in another workspace resolved modules through the launching checkout's tree (fixes #8653). Scrub the loader subset of RELOAD_EXCLUDED_KEYS from process.env at the two process boundaries that host sessions: the daemon after freezing its boot env (the frozen copy keeps loader vars so dev-mode ACP children can still boot), and the ACP child after the relaunch/sandbox handoff (the respawned child re-scrubs itself). Fixes #8653 * fix(cli): reject loader env keys in initial .env load and log scrubs A trusted workspace's .env could re-populate the loader-key slots that scrubInheritedLoaderEnv() emptied in the daemon process, because canApplyParsedEnvKey applied RELOAD_EXCLUDED_KEYS only on reloads. Reject the loader subset on every .env application path so one workspace's loader hook cannot reach other workspaces' session subprocesses through the shared daemon env. Also make the scrub return the removed keys and emit a stderr breadcrumb naming them at both boundaries, so a session subprocess missing an inherited var can be traced back to the scrub. * fix(cli): reject loader env keys in serve fast path before env freeze * fix(cli): deny npm_config_node_options and report rejected loader keys Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): match loader env keys case-insensitively and report settings.env rejections * fix(cli): canonicalize loader env key spellings and scrub channel daemon workers npm maps non-leading underscores in npm_config_* keys onto hyphens, so npm_config_node-options injected NODE_OPTIONS exactly like npm_config_node_options while slipping past every loader gate and scrub. Canonicalize case and underscore/hyphen spelling on both sides of the loader-key membership test, covering .env loads, settings.env application, the serve fast path, and the inherited scrubs. Channel daemon workers are spawned with the daemon's pre-scrub base env but are not ACP children, so they never ran the self-scrub; mirror the ACP-child scrub at the worker entry so nothing a worker spawns inherits loader vars into another workspace. Scope the settings.env rejection warning per workspace so a multi-workspace daemon reports every workspace's rejection instead of deduping them all under one label, revert the unread loadServeFastPathEnvironment return value to void, and pin the buildRuntimeEnvironment settings.env gate and the consume-once stash reset with discriminating tests. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): address review findings for loader env denylist - report rejected loader keys through the daemon log after boot (per-workspace .env loads were silent once boot stderr was gone) - accumulate serve fast-path rejected keys across loads instead of overwriting, dedupe, and use normalized env file paths for rejection sources - restore scrubbed inherited loader env vars on embedded runQwenServe close() - add regression tests for ENV scope coverage, reporter dedupe, fast-path accumulation, and post-boot daemon-log diagnostics - scope docs: top-level env rejection does not apply to mcpServers[].env / hooks[].env; document serve loader-scrub behavior * fix(tests): add writeStderrLineSafe to stdioHelpers mocks and sync env guard allowlist * fix(cli): scrub loader vars from the daemon base env and retighten the denylist The frozen daemonRuntimeBaseEnv was captured before the launch-env scrub, so daemon-spawned session processes still booted under the inherited loader — the child-side post-boot scrub ran after Node had already consumed NODE_OPTIONS. The base env is now scrubbed before the freeze (except under the DEV=true harness, whose .ts entries need the tsx loader), and close() restores the host's launch env from a pristine snapshot. Denylist scope now follows the injection-vs-search-path split: adds the npm config-file redirect keys, ZDOTDIR, and a BASH_FUNC_* prefix rule; moves ENV/LD_LIBRARY_PATH/DYLD_LIBRARY_PATH back to their reload-only tier (mainstream toolchain compatibility); blocks QWEN_CLI_ENTRY and NODE_EXTRA_CA_CERTS from project .env files. The ACP-child scrub is gated on the daemon stamp (QWEN_CODE_SERVE) so direct editor ACP integrations keep the user's exported environment, and the daemon's per-workspace .env rejections are now reported from buildRuntimeEnvironment. * fix(cli): block DEV spoofing, case-insensitive env exclusions, serve boot env restore (#8663) Address round-6 review: DEV joins the hardcoded project-env exclusions so a workspace file cannot disable the daemon's loader-env scrub; the hardcoded tier is enforced case-insensitively (Windows env lookup is case-insensitive) via isHardcodedProjectEnvExclusion at every application gate; runQwenServe's catch restores the scrubbed launch env and detaches the rejection reporter when startup fails after the scrub. Tests gain the matching regressions, home-env hermeticity, source-scoped warning filters, and tmpdir cleanup; the unreachable reload delete-pass loader guard and its vacuous test are removed. * fix(cli): match the reload-excluded env tier case-insensitively too Round-6 follow-up: R6-3 named RELOAD_EXCLUDED_KEYS.has() among the gates a case variant slips, but the hardcoded-tier fix left the reload-only keys (QWEN_SERVER_TOKEN, PATH, HOME, TMPDIR, …) on exact-case matching. On Windows a lowercase twin names the same OS variable, so a mid-session settings.env/.env edit could still rotate the daemon token or move PATH through a case respelling. Fold the reload tier the same way and pin it with a reload-behavior regression test. Also note DEV in the settings.md exclusion docs. * test(cli): redirect HOME in environment.test.ts for full home-env hermeticity The source-scoped warning filters fixed the warning-count assertions, but the process.env assertions (e.g. 'never applies entrypoint or trust-anchor keys') still read state a real home .env can pollute: home scope deliberately bypasses the hardcoded exclusions, so a dev machine with QWEN_CLI_ENTRY in ~/.env applies it and fails the test while CI stays green. Redirect HOME/USERPROFILE to an empty temp dir in beforeEach — verified by running the suite with HOME pointed at a poisoned home. --------- 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-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-08 16:11:39 +00:00
C0d3N1nja97342	fd76d4ddde	fix(cli): let ESC cancel ongoing work before popping queued messages (#8353) ... * fix(cli): let ESC cancel ongoing work before popping queued messages When the agent is actively responding (streamingState === Responding), InputPrompt's ESC handler consumed the key before AppContainer's global cancel-work handler could fire. Users had to press ESC 3 times (pop queue, clear input, cancel work) to stop the agent. Skip the pop-queue-into-input and double-ESC-clear logic when the agent is responding, returning false so the key propagates to the global handler which cancels the ongoing request. The up-arrow key still pops queued messages into the input at any time. Fixes #8201 * fix: narrow ESC fall-through to empty buffer + add regression tests Address wenshao's review on #8353: - Gate the return false on buffer.text === '' to prevent BaseTextInput's default ESC from silently wiping typed input without double-press confirmation - Add resetEscapeState() before return false to clear any pending escPressCount/escape-prompt timer - Add two regression tests with streamingState: StreamingState.Responding: 1. queue non-empty + ESC -> popAllQueuedMessages NOT called 2. buffer has text + single ESC -> buffer NOT cleared * fix: correct ESC comment to reflect KeypressContext broadcast model Address bot suggestion: the comment claimed returning true 'consumed the key before the global cancel-work handler could fire', but KeypressContext broadcasts to all handlers regardless of return value. The real mechanism is that popQueueIntoInput() fills the shared buffer, steering AppContainer's handler into its 'input has content -> double-press to clear' branch instead of the cancel-work branch. * fix: correct comment to accurately describe return false -> BaseTextInput fall-through Bot suggestion: the comment said returning false 'avoids' BaseTextInput's wipe, but return false actually *enables* it (BaseTextInput only short-circuits on truthy returns). The buffer is safe because the buffer.text === '' gate makes the wipe a no-op, not because return false prevents it. Reworded to make this explicit and warn against relaxing the gate. * test(ui): add positive AppContainer ESC cancel regression test The PR's Responding guards were pinned only by InputPrompt-side tests (queue not popped; non-empty buffer preserved). Add the positive case the review asked for: while Responding with an empty buffer and queued follow-ups, a single Esc reaches the global handler's cancel-work branch (cancelOngoingRequest called once) and the queue is not consumed. #8201 * test(ui): clarify ESC cancel test scope vs end-to-end drain The positive ESC cancel test asserts popAllMessages is not called, but the comment framed it as 'must not consume the queue' end-to-end. In production that exact cancel path DOES drain the queue back into the buffer via the cancel handler (cancelOngoingRequest -> onCancelSubmit -> popAllMessages), under the 'never silently drop queued work' invariant. The assertion only holds because cancelOngoingRequest is replaced by a spy here, severing that hop. Reword the comment to describe the real contract: the global keypress handler itself doesn't pop the queue (InputPrompt owns that and skips it while Responding; #8201), while the end-to-end drain is a separate hop severed by the spy. Addresses the review finding on AppContainer.test.tsx:2405. * test(ui): pin ESC return-false branch and dedupe getGlobalKeypress Address two review suggestions on the ESC cancel tests: - The `return false` branch in InputPrompt.tsx (Responding + empty buffer + empty queue -> defer to AppContainer's cancel-work branch) had no test coverage: reverting it left all 334 tests green. Add an InputPrompt test that pins it (no queue pop, no buffer mutation). - The AppContainer cancel test inlined a byte-identical copy of the getGlobalKeypress() helper that already existed ~2600 lines down in the Ctrl+O describe block. Hoist the helper to the outer describe so both blocks share one definition of the fragile toString() discovery idiom. * test(ui): escape raw ESC byte in cancel test fixture Per review (R4-1): the sequence literal embedded a raw 0x1B control byte that renders as an empty string in diffs and truncates grep output, so the fixture was unreadable. Use the escaped form matching the sibling vim-INSERT fixture on the line above. * test(ui): assert buffer stays empty on ESC cancel + fix test comment Per review (R5-1/R5-2): the flagship #8201 test asserted only the mechanism (popAllQueuedMessages not called), not the effect (buffer stays empty so AppContainer takes its cancel branch). Add the buffer assertion. Also correct the return-false test comment: deleting that branch leaves the test green (KeypressContext.broadcast ignores return values, BaseTextInput's clear is a no-op on empty buffer), so the test pins the no-side-effect contract, not the branch itself. * test(ui): cover queue+text ESC and double-ESC-clear while responding Per review (R6-1/R6-2): the pop-skip guard was pinned only for the empty-buffer case, and the double-press clear contract had no test under Responding. Add: - non-empty queue AND typed text: ESC does not pop the queue and preserves the buffer (pins the guard regardless of buffer content). - double-ESC while Responding: first ESC preserves typed text, second clears it (pins the double-press contract this diff preserves). * test(ui): dedupe escKey fixture and tighten double-ESC timing Per review (R7-1/R7-2): the double-ESC test spaced presses with the default 150ms wait (~30% of the 500ms window); use 50ms to match the sibling double-ESC test. Hoist the escKey fixture to the Cancel Handler describe scope so both tests share one definition (matching the getGlobalKeypress hoist this PR already did). * test(ui): add missing removeGoalTurns to cancel-handler queue mock Per review: the cancel-handler test's useMessageQueue mock omitted removeGoalTurns, a required member that every other queue-mock override in this file includes. The real cancel handler calls removeGoalTurns() before popAllMessages(); the test passed only because cancelOngoingRequest was a spy severing that hop. * test(ui): reuse getGlobalKeypress in vim-INSERT cancel test Per review (R9-1): the vim-INSERT test still inlined a handler-discovery loop matching on 'handleExit', duplicating the hoisted getGlobalKeypress helper (matching TOGGLE_THINKING_EXPANDED). Both tokens occur in the same handleGlobalKeypress closure, so the two idioms can only drift. Reuse the shared helper. * test+docs(ui): escape ESC byte in shared fixture and document Responding Esc Per review (R10-1/R10-2): the shared escKey fixture embedded a raw 0x1B control byte (invisible in diffs, truncates grep). Use the escaped form. Also update keyboard-shortcuts.md: Esc now cancels the ongoing request while the agent is responding instead of moving queued messages back into the input. * docs(ui): correct ESC/Up-Arrow queue-pop description to match code Per review (R11-1): the previous wording said queue pop happens only when idle, but Up Arrow pops in any state (no streamingState guard) and Esc pops whenever not actively responding (including WaitingForConfirmation). Reword to match the code, and note that the responding-cancel only fires when the input is empty. * refactor(ui): drop dead Responding ESC guard per maintainer review wenshao's mutation test showed guard #2 (Responding + empty buffer -> return false) is dead code: with it gone, control falls to the escPressCount===0 branch which returns true on an empty buffer, and KeypressContext.broadcast ignores handler return values anyway - AppContainer's own cancel branch acts on the empty buffer either way. Remove it, fold the subscription-ordering invariants it relied on into guard #1's comment, and document that only Responding is gated (do not broaden to !== Idle or ESC becomes a no-op during a tool confirmation). Also tighten the docs wording: cancelled queued messages are moved back into the input, not preserved. #8201 * docs(ui): correct four review comments in ESC cancel path Round-13 review (no blockers) flagged comment inaccuracies that could misdirect future debugging: - R13-1: the invariant comment pointed at an integration test that does not exist - note the harnesses mock each other's side instead. - R13-2: the regression-test comment still said the branch returns false and that AppContainer acts on return values; it returns true and broadcast ignores return values. - R13-4: the docs row claimed Up Arrow/Esc pop in any state, but during WaitingForConfirmation Composer unmounts InputPrompt (isInputActive admits only Idle/Responding), so neither key pops. - R13-5: the guard comment warned against broadening to !== Idle as if WFC ran this branch; it never does because the component is unmounted. No behavior change. #8201 * docs(ui): correct subscription-order and double-ESC-cancel comments Round-14 review: the invariant comment overstated subscription order as load-bearing - the Responding pop guard skips the pop in either order, and InputPrompt re-subscribes after AppContainer on any remount (e.g. a tool-confirmation round trip), so only the buffer.text-liveness invariant matters. And the double-ESC clear comment now notes it composes with AppContainer's cancel on the same keypress in the initial order but lands on the next press after a remount. No behavior change. #8201 --------- Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-08 05:14:42 +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
Nothing Chan	c2026882b7	fix(acp): emit context usage updates (#8513) (#8528) ... Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-08 01:09:51 +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
Shaojin Wen	b40719a8bb	fix(cli): Run ACP agent fan-outs concurrently and past the tool-call cap (#8631) ... * fix(cli): Run ACP agent fan-outs concurrently and past the tool-call cap The daemon's ACP session executed tool batches differently from the core scheduler in two ways that broke long agent fan-outs such as /review: runBounded — the runner for concurrent batches — forced the first three calls of any batch larger than the invalid-params threshold to run one at a time, then clamped the rest to concurrency 3, although agent calls are concurrency-safe and core's runConcurrently runs them at QWEN_CODE_MAX_TOOL_CONCURRENCY (default 10). A /review fan-out of 10-15 agents therefore ran almost serially. Agent-only batches now skip the serial prefix and the clamp: an invalid agent call fails in build() before any side effect, so the concurrent loop's near-threshold check still catches invalid-params loops just as fast. The per-turn tool-call cap halted unconditionally at model.maxToolCallsPerTurn (default 100) while core's LoopDetectionService treats the default as adaptive — past the soft cap a productive turn (diverse calls, no repetition) continues until a stuck-repetition signal or the hard backstop (soft cap x 10). A /review orchestrator needs well over 100 calls, so every high-effort review under qwen serve died mid-review at call 101. The daemon now mirrors core's checkTurnToolCallCap semantics, reusing the same thresholds. Measured on two high-effort /review runs (PRs #8522 and #8529): the baseline died at call 101 after ~8.3h each; after this fix both reviews run to completion in 4.4h / 5.3h, first fan-out wave 85m -> 33m, reverse-audit rounds 63-86m -> 25-35m. * fix(cli): regenerate settings schema after maxToolCallsPerTurn doc update (#8631) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): Gate the daemon repeat halt on skipLoopDetection like core * fix(cli): Address ACP fan-out review: keep wide-batch results, shared cap predicate (#8631) - runBounded no longer aborts in-flight calls when loop detection fires in the capped race branch: wide batches keep in-flight results and only skip the unstarted tail, matching narrow-batch behaviour (nothing executed is discarded either way). - Extract shouldHaltOnTurnToolCallCap from core's checkTurnToolCallCap and call it from the daemon guard so the two runtimes share one halt predicate and cannot drift. - Hoist canonicalToolName into tools/tool-names.ts beside ToolNamesMigration; scheduler, loop detection, plan redaction and memory refresh now share the single alias resolver. - Correct the wrong-direction cap wording (the daemon undershoots an explicit cap / hard backstop — the batch check runs before execution; the adaptive soft cap is exceeded by design up to the backstop) in the daemon comment, settingsSchema.ts (schema regenerated) and settings.md, and scope the always-on-guard sentence to core-client sessions. - Tests: adaptive hard backstop, wide-batch loop tail skip, wide-batch keep-results, provider-duplicate counter exclusion, `task`-alias fan-out, getToolCallRepeatKey alias/key-order coverage; raise the fan-out concurrency deadline off the 2s wall clock. * fix(cli): Address review: complete loop-guard docs, pin test envs, drop dead export (#8631) * fix(cli): Address review: correct parity comment, pin halt semantics, test cross-response repeats (#8631) --------- 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>	2026-08-07 16:32:44 +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