1437 commits

Author	SHA1	Message	Date
callmeYe	4839935e55	feat: register toggle-only Qwen reasoning (#9574)	2026-08-20 11:31:35 +00:00
ytahdn	bf0dbb1ae1	feat(web-shell): support mid-turn file attachments (#9570) ... Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-20 10:58:12 +00:00
jinye	2e6151aa15	fix(serve): Harden standalone conversation primitives (#9512) ... * fix(cli): pass ConversationDirectoryIdentityError cause through native Error options * fix(cli): re-inspect raced standalone directories and report first creation as created * fix(core): measure JSONL head integrity against a line budget * fix(core): Preserve plain JSONL record budgets Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-20 08:09:44 +00:00
Shaojin Wen	a8a855914b	fix(review): run verifier probes in a private scratch worktree (#9207) (#9221) ... * fix(review): run verifier probes in a private scratch worktree (#9207) Step 4's verifier is the review's one writing agent: it writes a probe, runs it, applies the one-line fix its flip-check needs, and restores. All of that landed in the shared review worktree — the tree `working_dir` pins every other agent to — and the pipelined loop launches a round's verifiers alongside the NEXT round's reverse auditors, so those writes are live exactly while the auditors read. "Leave the tree as you found it", which the brief has always said and verifiers do obey, cannot close that window: the exposure is *during* the probe. Measured on a live run, a round-5 auditor read a probe's mutant plus a leftover probe test and came within a step of filing a Critical against code no commit contains; it recovered only by improvising `git show HEAD:`, a fallback no brief mentions. Three parts, because isolation alone is a guarantee one regression away from being false: - `qwen review scratch-tree` gives each verifier shard a throwaway sibling worktree at the commit under review, with the review worktree's node_modules linked in so a unit harness starts without an install. Every call hands back a pristine tree (a previous finding's mutant surviving into the next probe would be a wrong verdict carrying a deterministic source tag); the label is the shard's record key, because the shards of one round run concurrently; and a tree it cannot create makes the probe inconclusive rather than falling back to the shared worktree. This is the isolation the test-efficacy probe has had since #6832 and the A/B's base tree has, extended to the last step that writes. - Every code-reading brief — dimension agents, chunk agents, reverse auditors and the verifier itself — now carries the rule that auditor had to invent: the worktree is shared, code that is not in the diff and not in the commit is not a finding, and anything surprising is judged against `git show HEAD:`. - `agent-prompt` reads the tree once per call, and every wave of agents is built by it immediately before launch: residue is named in each brief it builds and warned about on stderr, so a contaminated tree is announced to the agents about to read it instead of being discovered as a phantom Critical. `exposeDependencies` moves to `lib/worktree.ts` beside the other disposable-tree machinery, and now farms each workspace member's own node_modules as well as the root's. Measured on this repo, a tree with 1560 root packages linked still could not resolve `@testing-library/react` for a UI probe, because npm could not hoist that copy out of `packages/cli` — which also silently cost the efficacy probe the same class of test. `cleanup` sweeps the scratch family by prefix; the label half is the shard's key and cannot be reconstructed by the sweeper. * fix(review): address the scratch-tree review — two criticals and the honesty gaps Both Criticals were real, and both had the same shape: a claim the code made that one machine class or one leftover state falsified. - The welded `scratch-tree` command interpolated `--worktree` unquoted, so on any checkout under a path with a space or an apostrophe every shard's isolation was silently unavailable and every probe fell back to a reading. Quoted with `shellQuotePath`, like every other path this file prints into a command. - The reuse gate checked only that the scratch path EXISTS. A bare directory there — the leftover of a crashed `worktree add`, or of a cleanup whose `rmSync` failed — has no `.git`, so git walked up and ran `checkout --force --detach` against the user's own checkout: their uncommitted work discarded, their HEAD detached onto the PR's commit, and `rev-parse HEAD` then returning the sha that made the reset report success. Gated on the tree being a registered worktree, with the regression pinned by a test that fails on the un-gated code. The rest are the same defect class at lower stakes — something stated more broadly than it holds: - The residue probe hand-parsed porcelain's rendered form, so a path with a space, a non-ASCII byte, or a literal ` -> ` came back as a name matching nothing on disk, `--untracked-files=normal` collapsed a whole probe directory to one unactionable `dir/` entry, the 1 MB `maxBuffer` default answered the dirtiest trees with "clean", and the cap truncated silently while both renderers presented the list as complete. Now `-z --untracked-files=all` with a 64 MB buffer, and a `{paths, total}` result both renderers disclose. - `git checkout -- <path>` restores from the INDEX, so the advised recovery left staged residue in the tree; it now says `git checkout HEAD --`. - `git show HEAD:<path>` cannot produce an untracked path — the prototypical residue — so the rule now says what that answer means instead of handing the reader a command that exits 128. - Agent 8's `--whole-diff` block reads the same shared worktree and got neither the rule nor the residue paths; it is built outside `buildLaunch`, which is exactly how it was missed. - The stderr warning claimed every agent had been told; the block is gated on `reviewsCode`, and Agent 7 — which builds and tests that tree — is not. - A label that flattens to no path-safe character fell back to a shared `agent` tree, `git clean -fd` left a nested repo standing while the report said the tree was pristine, `dependencies: null` said "no node_modules" about a farm that had failed to link, `{0,0}` read as "already in place" for a `node_modules` holding nothing linkable, and `--out` was validated after the tree and its farm already existed. - The docs stated the isolation unconditionally: local-diff and file-path reviews have no worktree and no scratch tree, and SKILL.md/DESIGN.md and the user page now say so. Tests moved with the code (`exposeDependencies` and `worktreeCreateFailureDetail` now live beside `lib/worktree.ts`), and the plumbing that was pinned at both ends but not in the middle — that a verify shard's recorded brief carries ITS record key as the scratch label — is pinned by a test that fails when the key is dropped. * fix(review): close the round-2 review — Windows guard, Agent 7's blind spot, unpinned wording The Critical is a test that would only fail where this PR's CI does not look: the new chmod-based case guards on `process.getuid`, which is undefined on Windows, and `chmodSync` on a directory there sets a read-only attribute that does not stop `git worktree add` from creating a subdirectory — so the merge_group-only Windows leg would go red for every PR carrying the file. Sixteen of the seventeen chmod-permission tests in this repo already skip win32; this one now does too. The rest are the same class as round 1 — a claim wider than the code: - **Agent 7 had no protection for residue that PREDATES the round.** The exclusion's justification ("its own commands are the writes it sees") is only true for residue a round creates. A tree that starts dirty reaches Agent 7's compile and test run, where a `[build]`/`[test]` finding is pre-confirmed and skips verification — a merge-blocking phantom Critical of exactly the class this machinery exists to prevent. The residue paragraph now goes to EVERY brief (with "a defect confined to these paths is not a finding"); only the reader rule stays scoped to the roles that review code. The stderr warning says to restore before launching the wave, not before the next round. - **Residue paths reached two sinks unflattened.** `inertPath` moved to `lib/paths.ts` and now covers the scratch-tree note and the orchestrator's stderr line as well as the briefs — the `-z` format this PR introduced is precisely what lets a control byte in a filename arrive intact. - **The recovery wording could not clear two shapes the probe reports.** A path staged as NEW and a rename destination are in the index but not in HEAD, so `git checkout HEAD --` cannot match either; both renderers now name `git rm --cached` for those and `rm -rf` for untracked residue (including the nested- repo directory entry `--untracked-files=all` still cannot expand). - **The "full set" command was the one this PR calls unusable.** Both notes now say `git status --porcelain --untracked-files=all`, since the default collapses the probe directory whose files the count came from. - **`alreadyPresent` believed an empty farm dir.** The dir a previous call creates when the source holds nothing linkable is gitignored, so the reset spares it, and the second call flipped "no harness will start here" into "already in place" with nothing changed. It now requires a non-empty farm. - Doc corrections: `resetScratchTree`'s header still said `clean -fd`, and the residue probe's comment claimed `--untracked-files=all` ends directory-shaped entries — it does not for a directory holding its own `.git`. The invalid- UTF-8 limit (`encoding: 'utf8'` maps a bad byte to U+FFFD, and no string form of that name resolves) is documented rather than papered over. Pinned, each verified by reverting the fix and watching the test go red: the apostrophe ESCAPE in the welded command (the fixture had no apostrophe, so a naive `'…'` wrap passed), the `git checkout HEAD --` wording in both places, the capped-note arithmetic, the farm-failure note, stdout-before-side-file ordering and the exit-1 arm it also covers, and `sharedTreeResidueTotal` on the creation-failure return. * fix(review): close the round-3 review — hooks, hidden mutants, planted farms, suppression after restore Four of the seven Criticals are the same discovery from four directions: a scratch tree is a LINKED worktree and a `clean`/`checkout` reset is not the guarantee it reads as. - **Hooks resolve to the user's repository.** `git worktree add` and `checkout --force` both fire `post-checkout` from the common dir — the user's own `.git/hooks` — so creating or resetting a scratch tree executed whatever that repository holds. Every git call this command makes now runs with `core.hooksPath` pointed at a path holding no hooks, and the report says plainly that hooks, config and refs are shared rather than isolated. - **skip-worktree hid a mutant through the reset.** `checkout --force` silently skips a file carrying the bit and `clean` never touches tracked files, so a probe that set it (directly or via `sparse-checkout`) left a mutant that survived with `git status` reading empty and the sha still matching. The reset now refuses when `ls-files -v` still shows a hidden entry, which routes the caller to discard-and-rebuild. - **The farm was certified by existence.** `clean -ffd` spares ignored paths to keep the dependency farm — and equally spares whatever a probe installed or planted there. `node_modules` is now the one ignored path a reuse does not inherit: it is cleared and re-linked, and `exposeDependencies` marks the farms it builds so a directory it did not build is never certified as one. - **A broken leftover could not be rebuilt over.** A `.git` gitfile whose admin entry survives makes `worktree remove` fail and the next `worktree add` refuse "missing but already registered"; `discardWorktree` now prunes. The fifth is about the instruction rather than the tree: the residue paragraph is baked into every brief at build time, so restoring the paths and then launching the already-built wave tells every agent to drop findings in a file that is by then exactly the PR's code. Both the stderr warning and SKILL.md now say to rebuild the wave after restoring. The remaining two are the three new real-git fixtures missing `isolateHostGitConfig()`, which a polluted host gitconfig (`commit.gpgsign` with no key, a `core.hooksPath` hook) turns into suites that fail for reasons the branch never touched. The suggestions, in one line each: a rename now reports BOTH of its names (the restore needs the one that is gone); a `git status` that dies is reported as UNMEASURED rather than clean, in both renderers; `inertPath` covers `\p{Cf}` and `\p{Zl}`/`\p{Zp}` — bidi overrides and zero-width characters passed through the sanitizer that exists to defuse hostile filenames; residue paths and tree paths are shell-quoted where the notes prescribe commands over them (`rm -rf my probe.ts` deleted `probe.ts`); the verifier brief no longer says "then move on" after applying a candidate fix, because one tree serves every finding in a shard; `scratchLabel` strips a leading dash, which yargs read as a flag; the member-farm loop guards per member rather than around the loop; the unguarded `mkdirSync`/`readdirSync` in the farm now count as failures instead of throwing out of a best-effort contract; cleanup discloses a `readdir` failure instead of reporting "nothing to clean", and sweeps a dangling symlink `releaseWorktree` cannot see; the briefs' restore recipe gained the staged-only branch the scratch-tree note already had; and both "pristine" claims now say that gitignored paths survive. Also corrected: "the one agent whose job requires writing" — Agent 7's efficacy probe writes too, and has had its own tree since #6832. New tests, each verified by reverting its fix: the hooks suppression, the skip-worktree refusal, the planted-farm replacement, the prune, staged-residue detection, the unmeasured state, `inertPath`'s character class, and the residue reaching every launch class rather than the one role the earlier test inspected. * fix(review): close the round-4 review — untrusted workspace paths, member farms, locked leftovers The sharpest two are about treating the reviewed PR's own manifest as data rather than as input, in code that DELETES: - `exposeDependencies` fed workspace dirs from the root manifest of the code under review straight into `join()` and then into the farm's opening `rmSync`. A PR setting `"workspaces": [".."]` — or a committed SYMLINK at a workspace path, which `readWorkspacePackages` follows deliberately because npm does — pointed that delete at a directory outside both trees; in this pipeline's layout, at the reviewer's own checkout. Every member is now resolved through `realpathSync` and required to be contained in the tree it belongs to, which closes the string and the symlink vector together, and a member that escapes is counted as failed rather than silently skipped. - The reuse path wiped only the ROOT farm, so `<tree>/packages/<member>/ node_modules` survived with its marker and was certified as-is — the same hole round 3 closed at the root, one level down, where Node resolves a member's imports FIRST. `exposeDependencies` now takes `rebuild`, and the reuse path distrusts every farm rather than the top one. Two more that would have wedged a review: - `git worktree prune` never drops a LOCKED admin entry, and probe code has a shell inside these trees: one `touch` in the admin dir the tree's own gitfile names, and every later `worktree add` for that PR fatals "missing but locked" — permanently, since cleanup prunes too. `discardWorktree` now unlocks and retries with the second `--force`. - `ls-files -v` and `clean` ran under `spawnSync`'s default 1 MiB buffer, which a large repo passes; Node kills the child, the reset reads that as failure, and every reuse rebuilds forever. And a Windows one of the same class as round 2's: the new fixture wrote a file named `a -> b.ts`, and `>` is reserved on NTFS — the merge-queue-only Windows leg would have failed at fixture setup. Split, with the arrow half skipped there (the shape it pins cannot exist on NTFS). The rest: the residue restore recipe was WRONG for a staged rename's original name (`git rm --cached` stages a deletion there; `git checkout HEAD --` is what clears it) and the test that certified it never staged anything — both fixed, and the test now builds all four real shapes; the reader rule was gated on `reviewsCode`, which left Agent 0 and the test matrix reading worktree source without it (the gate is now "every role that judges code", i.e. all but Agent 7); the stderr warning did not actually carry the rebuild-after-restore instruction SKILL.md attributes to it; `ScratchTreeReport` dropped the `unmeasured` state, so a failed `git status` read as clean to a script; the scoped-package branch linked non-directory entries the top-level branch skips; `farmDependencies`' catch and its note branch were unreachable once round 3 guarded the fs calls, so they are gone rather than pretending to be a net; cleanup's family-read failure now fails the run instead of letting it announce "Nothing to clean", and its dangling-symlink branch no longer swallows a throwing `rmSync`; the residue note says the names are flattened for display and where the exact bytes are. Disclosed rather than enforced, with the reason in the code: the farm's links are read-write and point into the shared worktree, so a probe that writes THROUGH one (an `npm rebuild`, a package that writes into its own directory) lands outside its tree where the residue check cannot see it. Copying the farm would cost the minutes it exists to save; the verifier's block now says to replace a link with a copy before modifying a dependency. New coverage: workspace escape and symlinked-member containment, the self-farm guard, rebuild-on-reuse at member level, assume-unchanged beside skip-worktree, the unmeasured state through its renderers, the dangling-symlink sweep, the family-read failure, and the cleanup mock gaps that made two of those branches unreachable by construction. * fix(review): close the round-5 review — scoped prune, real pristine, submodules, symlink gadgets The findings that hold without assuming an attacker already has a shell: - **The prune added in round 4 was repo-wide.** `git worktree prune` drops any admin entry whose directory is momentarily absent — another shard's `worktree add` mid-flight (this pipeline runs discards and adds concurrently against one common dir), or the user's own worktree on a volume that happens to be unmounted. It now removes the one entry whose `gitdir` file names this path, and nothing else. - **"Pristine" spared ignored paths, and a probe's state lives there.** Its own `node_modules` at any depth, a `.tsbuildinfo`, a `dist/` it built and then mutated — all survived a reset the report called pristine, and the farm-level wipe could not reach them (a member farm whose source has none was skipped before the target was touched; the root rebuild was skipped entirely when the review worktree had no `node_modules`). The reset is now `clean -ffdx` and the farm is re-linked, so pristine means pristine. - **An initialized submodule was untouched by all of it**: `checkout --force` without `--recurse-submodules` leaves its working tree, `clean` never touches a tracked gitlink, and `rev-parse HEAD` is the superproject's. A tree holding one is now rebuilt rather than reset — a fresh `worktree add` leaves submodules uninitialized, so the rebuild is both correct and cheap. - **test-efficacy reuses one probe tree across every suite run**, and the code running in it is the PR's own test code. Round 4 answered this with the positive control; the control only covers the green-forcing direction, so the farm is now re-linked before each run instead — about a second against a 540-second budget. The remaining Criticals all presuppose code execution that this pipeline grants before any of this code runs (Agent 7 installs and tests the PR; a verifier's probe is arbitrary code by design), so they do not change what an attacker can do — but three of them describe gadgets that cost nothing to remove, and they are removed: - `cleanup` no longer hands ANY symlink at a family path to `git worktree remove`, which would follow it and delete whichever registered worktree it points at. It unlinks the link and reports that. - `resetScratchTree` validates that the tree IS the tree — not a symlink, and `rev-parse --show-toplevel` resolving to itself — before running a reset that would otherwise land wherever the path resolves. - `farmNodeModules` lstats its SOURCE root, so a symlink at the review worktree's `node_modules` cannot redirect every probe tree's dependencies. Also: the `dependencies: null` contract now says what it means (no `node_modules` to link from; a link failure arrives counted, not as null), the `alreadyPresent` branch of the note is gone with the reuse path that could reach it, and the residue note carries the flattened-names disclosure its sibling renderer already had. New tests: the symlinked scratch path, ignored-state clearing, the submodule rebuild, and the unmeasured pass-through through the command's own report. * fix(review): close the round-6 review — committed farms, foreign repos, deinit'd submodules One finding crosses the line the round-5 reply drew, and it is the important one: `node_modules` is gitignored by convention, not by rule, so a pull request can force-add `node_modules/.qwen-review-farm` beside its own module stubs and `git worktree add` checks both out. The fresh-create path then found a marker and certified the PR's own modules as the farm this code built — reachable from PR CONTENT, with nothing executed. Two changes: the marker records the dependency root it was built from and is only believed when it names this one, and both scratch-tree paths (fresh and reuse) now rebuild rather than trust anything found at that path. The other three that hold on their own: - **The identity check added in round 5 was incomplete.** `rev-parse --show-toplevel` prints the directory the `.git` file sits in, whatever that file points at — so a gitfile naming another repository, or a whole repo planted at the predictable scratch path, passed it while every command below would have run against someone else's objects, refs, hooks and config. The tree must now share a common dir with the review worktree. - **`git submodule deinit` walked through the round-5 submodule gate**: the status line goes back to `-` while the submodule's gitdir — its hooks, its config, its objects — survives under the common dir and is resurrected by the next `update --init`. The gate now reads the commit's gitlinks instead of the submodule's state, so a repo with submodules rebuilds rather than resets. - **The round-5 anti-planting rebuild never reached the positive control or the hunk probes.** They defaulted `dependencyRoot` to the probe tree, where `exposeDependencies` returns before clearing anything — so the control, the run that decides whether ANY mutant verdict is trusted, resolved through whatever the baseline's test code had planted. Smaller, from the same review: a dangling symlink at the target skipped the rebuild wipe (`existsSync` follows links); a dangling top-level link in the source was dropped without counting; a stray file under a scope directory was linked as a package where the top-level branch skips one. Disclosed rather than fixed, with the count now in the report: the farm mirrors npm workspace SELF-links, so an import that goes through a package NAME resolves back into the review worktree's copy and a mutation made in the disposable tree is invisible to it. Re-pointing them would break resolution outright for any package whose entry point is a build artifact a fresh checkout does not have, so `DependencyFarm.selfLinked` carries the number instead of the layout carrying it silently. Not fixed, and this is the third round of stating it: the remaining findings need a prior arbitrary-code-execution foothold (a background process racing the containment check between resolve and use). The pipeline grants that capability several steps earlier and by other code — Agent 7 runs `npm ci` and the PR's test suite — so hardening this command changes which line appears in the trace, not what is possible. The place to change it is sandboxing the review's command execution, which is its own design. Tests: the locked-leftover recovery, the scoped registration drop (a sibling worktree whose directory is absent survives it), the foreign-repository gitfile, the deinit-proof submodule gate, and the scope-directory stray file. The foreign-repo fixture is a CLONE on purpose — with an unrelated repo the reset fails for the wrong reason and the test passes without the guard it pins. * fix(review): close the round-7 review — walk-up escapes, identity gates, farm wipe Five Criticals, each probe-verified on real git before the fix and mutation-checked after: - worktreeResidue verifies the path IS a worktree before measuring: with the .git file gone, git discovery walks up into the user's checkout and answers with the user's own dirty state — fail closed instead (R7-4). - runScratchTree applies the same --show-toplevel check to the trusted --worktree argument before resolving HEAD (R7-3). - The reuse identity gate rejects a tree whose gitdir equals the common dir: a .git symlinked or hand-edited to name it passed every prior check while checkout --force detached the user's main HEAD onto the PR sha and rewrote the main index (R7-2). - farmNodeModules detects the rebuild target with lstatSync: a dangling symlink a PR committed as node_modules read as absent, skipped the wipe, and died EEXIST on every rebuild (R7-1). - The residue probe's node_modules/dist exclusion is enforced by a pipeline-controlled excludes file rather than borrowed from the commit's .gitignore (R7-5). Plus the production-shaped UNMEASURED fixture (R7-19) and the three remaining stale contract texts on the reuse reset (R6-7). * fix(review): close the round-8 review — tripwire identity, GIT_DIR redirects, symlink writes Round-8 Criticals, each probe-confirmed and pinned by a test that goes red when the fix is reverted: - The residue tripwire certified a CLEAN status for a repository planted over the contamination (`rm .git && git init && commit`): no local check can tell a planted repo from the tree it replaced. It now fails closed unless `.git` is a gitfile, and reports unmeasured for what `git status` cannot see inside committed gitlinks (a non-empty submodule directory) instead of clean. - An inherited GIT_DIR (or GIT_WORK_TREE/GIT_INDEX_FILE/...) redirected every identity check at once — both sides of each comparison see the same override, so none could detect it. Every git call in the scratch-tree lifecycle and the tripwire now drops the redirect variables. - The scratch-tree reuse gate accepted a planted gitfile naming a SIBLING worktree's admin entry and reset the sibling: the gate now requires the admin entry's gitdir backpointer to resolve to this tree. - Probe writes followed PR-committed leaf symlinks (mode 120000) into the shared review worktree — the positive-control injection, every mutant and every hunk restore. Each write site refuses a symlinked target as inconclusive. - discardWorktree handed a symlink at the tree path to `git worktree remove --force`, which resolved it and force-deleted the registered victim it pointed at; a symlink is unlinked instead. Plus the record-key comments (chunk id separates --all-chunks shards) and the orphaned marksOurFarm doc block. * fix(review): close the round-9 review — gitlink residue, env redirects, probe escapes Round-9 Criticals, each probe-confirmed on real git before the fix and pinned by a test that goes red when the fix is reverted: - worktreeResidue's gitlink blind set was parsed from RENDERED `ls-files -s`: default core.quotepath quotes a non-ASCII gitlink name into a spelling that never resolves on disk, the entry dropped from `blind`, and a contaminated gitlink was certified CLEAN. It now reads `-z` (R9-1). - An exported GIT_DIR redirected every inheriting git call at once: test-efficacy's head-sha read, probe resets and revert checkout, and base-tree's add and reuse check. Every git spawn in both files now drops the redirect variables (R9-16). - cleanup's symlink guard covered only the scratch family; the three named family paths still reached releaseWorktree, whose existsSync follows a LIVE link and whose `worktree remove --force` resolves it — deleting the registered victim while reporting success, and silently skipping a dangling link. The guard moved into the shared report path all four families go through (R9-18). - exposeDependencies' rebuild wiped only the farm-owned node_modules; anything planted at another path — packages/node_modules resolves before the root farm in a packages/* workspace — survived between probe runs and decided their verdicts. Rebuild now wipes every node_modules the farm does not recreate, following no links (R9-37). - NO_HOOKS covered hooks only, not config-driven content FILTERS: a planted `filter.<name>.smudge|clean` plus one attributes line — both writable into the common dir — executed on every reset and rebuild checkout. runScratchTree detects repo-local filter config and refuses rather than run it (R9-38). - The harness never WRITES probe test files, so no write guard ever saw one committed as mode 120000 — vitest collected it through the link and scored mutants against code the tree never mutated. Probe files are checked against the committed index and dropped as not-run before any suite collects them (R9-40). - The probe write guards lstat-checked the LEAF only; lstat resolves intermediate components, so a relinked ancestor in a reused tree read ordinary and every later write followed it out of the tree. The guard walks every component (as safeRmWithin does for deletes), and each restore write re-validates immediately before writing — a mid-run relink stops the phase instead of writing through the link (R9-17). * fix(review): close the round-10 Criticals — env config injection, sweep scope, probe-set holds Taken back from the autofix loop, and scoped to Criticals from here: the findings that hold without assuming a foothold, plus the ones whose fix is a few lines and removes a step from a chain. Reachable from the environment or from PR content, with nothing executed: - **`sanitizedGitEnv` dropped only the discovery redirects.** `GIT_CONFIG_COUNT` with `GIT_CONFIG_KEY_<n>`/`GIT_CONFIG_VALUE_<n>` sets any config key for the run — `core.fsmonitor` and the `filter.*` pair are command execution — and `GIT_CONFIG_GLOBAL`/`GIT_CONFIG_SYSTEM`/`GIT_CONFIG_PARAMETERS` reach the same place by other routes. A gate on the front door with the window open. - **`lib/git.ts` never had the gate at all**: `gitOpts()` spread `process.env` into every shared helper, `releaseWorktree`'s `worktree remove --force` included — the commands that run against the user's own repository, so they need it more than the disposable-tree ones that got it first. - **The residue probe could be steered by the tree it measures**: `core.fsmonitor` runs a command on `status`, so the tripwire was an executor. Emptied for that invocation, the same discipline the checkouts' `core.hooksPath` already had. - **`probeTargetEscapes` treated a backslash as a separator on POSIX**, where it is an ordinary name character: `x\y.test.ts` split into phantom components, the first `lstat` died ENOENT, and the catch reported "no escape" for a leaf nobody looked at. It also never checked the tree ROOT, which defeats every per-component check at once. - **The symlink drop reassigned the probe list before the collocated-test hold ran**, so a mutant whose own test was dropped answered "there is no such test" and was scored `survived` on the strength of the other probes passing — the hold exists to refuse exactly that. - **The rebuild sweep matched `node_modules` case-sensitively** on the two filesystems this file already special-cases: `Node_Modules` resolves like the real one on APFS and NTFS and survived every rebuild. - **A tracked path in the residue list suppressed real findings.** The brief said "none of it is the PR's code, so a defect confined to those paths is not a finding" — false for a file the commit contains and this PR changes: only the uncommitted DIFFERENCE is foreign. It now separates the two, and a defect present in the committed version stays a finding. Cheap chain removal, though each needs a prior foothold: - The excludes file moved from a constant name in the shared temp dir to a `mkdtemp` directory written `wx` — git re-reads its CONTENTS on every later `status`, so a writable fixed path could blind the tripwire. - `discardWorktree` now clears the registration named by the TREE's own `.git` pointer, read before anything is removed, instead of scanning `<common>/worktrees/<id>/gitdir` files — which any same-user process can rewrite to aim this cleanup at a live sibling. The scan remains only as the fallback for a tree whose pointer is unreadable, which is the case the step exists for, and it now refuses entries whose tree still stands. - The scratch sweep refuses to run at all when its parent is not a directory: a symlinked `.qwen/tmp` redirects every delete below it, and no per-leaf check can see an ancestor. - `removeUnownedNodeModules` counts a wipe it could not perform, instead of letting a planted directory survive every rebuild undisclosed. Left as recorded decisions, not re-litigated: R4-5, R6-2 and the R6-5/R9-14/ R9-38/R9-40/R10-25/R10-27 class, which need probe code already running as the user — a capability Agent 7's install and test step grants several steps earlier. The remaining Suggestions stay on the deferred queue for a follow-up, per the triage gate's own call that seven rounds was the moment to land this. * fix(review): close the round-11 Criticals — the half-fixes from round 10 Five of the eight are residue from the previous round's own fixes, which is the honest reason to take them: - **The fsmonitor neutralization covered one spawn of two.** `git status` got `-c core.fsmonitor=`; the sibling `git ls-files -s -z` that builds the gitlink blind set did not, so the same planted command still fired during the measurement. - **The gitlink blind-set filter failed OPEN.** An absent directory is the clean shape `worktree add` leaves; an UNREADABLE one is a place neither `git status` nor this probe can see, and both answered "nothing to hide there". Only ENOENT is clean now. - **The wipe walk skipped symlinked directories entirely**, so a `node_modules` behind a PR-COMMITTED symlink directory survived every rebuild with nothing said. The walk still refuses to follow a link — the farm's own entries point out of the tree — but a link resolving back INSIDE the tree that hides dependencies is now counted as a failure the caller reports. - **The sweep's ancestor check stopped at its own argument.** `lstat` refuses to dereference only the last component, so a symlink one hop above (`.qwen` over `.qwen/tmp`) resolved silently and redirected the same deletes. Every ancestor up to the filesystem root is checked. - **Detection was followed by the damage it detected.** When the mutation phase aborts because the probe tree was relinked mid-run, the revert phase ran anyway — `git checkout base -- …` and `safeRmWithin` with a cwd resolving through the link into the shared review worktree. The phase now re-validates the tree first, and `safeRmWithin` lstats its own ROOT, which its docstring always promised and its loop never did (a root link resolves the whole prefix in the kernel, so every component below it looks ordinary). Left as recorded decisions: R11-1 (`refs/replace` in the shared common dir), R11-2's residual fallback path, and R11-3 (content filters during `status`) — each needs probe code already running as the user, the capability Agent 7's install and test step grants several steps earlier, and the scan fallback exists precisely for the corrupt-pointer recovery the earlier rounds pinned. * fix(review): close the round-12 Criticals — sanitizer gaps, sweep scope, false farm failures Nine of the twenty-four hold without a foothold, or are defects in the previous rounds' own fixes: - **`residue.unmeasured` rode raw into two sinks** while the paths beside it went through `inertPath` — and that string is built from `ls-files -z` gitlink names, so a PR that commits a gitlink with ESC bytes puts a control sequence on the orchestrator's terminal and in the verifier's note. - **The branch delete never got the env the check did.** `refExists` resolves the real repository through the sanitized helpers, while `git branch -D` in cleanup and fetch-pr still inherited `process.env`: with `GIT_DIR` exported the pipeline verified a branch in one repository and deleted it in another. - **The ancestor refusal guarded one sweep of three.** It announced that `.qwen/tmp` hangs off a symlink and the same function went on to delete the base-tree lock and every side file underneath it. The check moved to the top of `runCleanup` and now refuses the whole clean. - **A family symlink that would not unlink released the lease**, unlike the three sibling failures that hold it, leaving the next `fetch-pr` to pass a gate over an occupied path. - **The symlink disclosure added last round produced FALSE failures**: it never consulted the `owned` set, so a PR-committed `alias → .` counted a failure for the farm this very call had just re-linked, and it judged before the walk had wiped the target it was complaining about. It now runs after the walk and skips owned farms. - **Probe names were passed to `git ls-files` as raw pathspecs**, so a probe committed as `:(literal)x.test.ts` — a legal filename — was parsed as magic and its symlink never found. Every pathspec is `:(literal)`-prefixed now, and a refused pathspec drops every probe rather than answering "no symlinks". - **The residue oracle could be blinded by index bits.** `skip-worktree` and `assume-unchanged` make `status` answer clean for an edited tracked file — the hazard the scratch tree's reset already refuses to certify around, missing from the reader-side probe that tells auditors the tree is pristine. - The excludes file is created 0600 rather than 0644. Left on the recorded boundary: the filter-gate coverage findings (R12-6, R12-24), the hardlink and runtime-relink shapes (R12-31, R12-32, R12-72). Each needs the PR's own test code running as the user — the capability Agent 7's install and test step grants several steps earlier — and the last three are properties of executing untrusted tests at all, not of this command. * fix(review): close the round-13 Criticals — spelling, empty probe sets, raw error text Five hold without a foothold, and four of them are defects in the previous round's own fixes: - **`owned` mixes path spellings by construction** — the tree root as the caller spelled it, each member as `containedIn` resolved it — and last round's disclosure compared only one of them, so a farm reached through a link counted a failure for a directory that same call had just re-linked. Both spellings are asked now, through one helper the wipe and the disclosure share. - **A suite run with an EMPTY probe list runs vitest with no filter**, which collects whatever the repository holds and scores it as this probe's evidence. After the committed-symlink drop empties the set, the mutation and revert gates now skip rather than run. - **Git's own stderr rode raw into the verifier-facing note** through `Error.message`, as did the filter KEY NAMES in the refusal (a git subsection name carries any byte but newline and NUL). Same sink class as last round's `unmeasured`, flattened the same way. - **Every refusal that fires before the residue is measured answered with the empty list a measured-clean tree produces.** A consumer could not tell "the tree is clean" from "this call never looked"; the refusals now carry the unmeasured reason. Left on the recorded boundary, re-checked and unchanged: R12-6, R12-22, R12-24 (config planting), and the R13-9/R13-10/R13-13 family, which need probe code already running as the user. * fix(review): close the round-14 Criticals — farm containment, residue blinds, release guard Five hold without a foothold, and three of them are channels a pull request controls outright — no same-user foothold anywhere: - **Farm entries were mirrored link-or-not.** A committed symlink under `node_modules` (force-add defeats gitignore) pointing out of it became a write channel from the disposable tree to wherever it points — into the shared worktree's tracked files — recreated on every rebuild. Entries now resolve through realpath before linking; only a borrowed `node_modules` or an npm workspace self-link passes, everything else is counted and disclosed. (R10-18) - **The residue probe's untracked view came from `status` alone**, which honors ignore rules the contaminator controls: a committed whitelist-form `.gitignore` (`*` with `!`-negations) blinded it to contamination. The probe now merges `ls-files --others` without `--exclude-standard` and filters the pipeline's build artifacts in code. (R10-19) - **A gitlink named in bytes UTF-8 cannot decode dropped from the blind set**: the mangled spelling never resolves on disk, the readdir read absent, and a contaminated gitlink certified clean. Such names now fail closed into unmeasured. (R11-4, second entrance) - **The `owned` set the rebuild disclosure asks held one spelling** while the disclosure loop presented another — on a host whose tree path carries a symlinked ancestor (macOS's `/var` vs `/private/var`) every rebuild counted a phantom failure for the farm the same call had just re-linked. The set is normalized once, at build time. (R13-1 remainder) - **`releaseWorktree` followed a symlink standing at the path**: `existsSync` resolved it and `git worktree remove --force` deleted whichever registered worktree it named — and a dangling one wedged the next `worktree add` while invisible. The lstat-first guard cleanup's family sweep applies to every path now lives at this choke point, which is where fetch-pr's `cleanStale` releases. (R13-3) Pinned against real git and real filesystems; all seven new tests fail on the pre-round sources. * fix(review): skip the invalid-UTF-8 gitlink fixture off Linux APFS and NTFS both refuse a filename that is not valid UTF-8, so the fixture cannot be created there — `mkdir` fails ENOENT on macOS, and the round-14 test reddened for every maintainer on a Mac while the repo's `pull_request` CI, which runs the unit legs on Linux only, stayed green. The shape it pins cannot exist on those filesystems either, so the skip loses no coverage. Same class as the round-2 and round-6 guards (`chmod` semantics, NTFS reserved characters): a fixture that cannot exist on a platform is skipped there rather than left to fail in a leg nobody watches. * fix(review): close the round-15 Criticals — a tripwire that was firing on every healthy run The important one is a regression this PR introduced two rounds ago, and it is measurable rather than hypothetical. Round 14 answered "a `.gitignore` can hide a probe's leftovers from `status`" by merging the ignore-INDEPENDENT untracked listing into the residue set. Measured on a healthy review worktree of this repo, after the `npm ci` and build the pipeline itself runs there: `git status` reported NOTHING and the tripwire reported 3 957 paths — coverage HTML, `.tsbuildinfo`, husky's installed hooks. Every one of them reached every verifier and every reverse auditor as contamination, with `rm` and `git checkout` recipes attached, and real residue would have been three lines inside that. A tripwire that fires on every healthy run is not read. What separates the two sets is not the pattern, which is unbounded, but WHO WROTE THE RULE. An ignore file the commit under review carries is ordinary repo hygiene that every reader of that tree sees identically; a rule from anywhere else — `info/exclude`, a `.gitignore` written after the checkout — is the plant. `check-ignore -v` names the source file per path, so the reconciliation asks git that question instead of inferring it from the name. One exception, because it is the whitelist form's whole trick: a rule from the commit that hides EVERYTHING (`*` with `!`-negations) vouches for nothing and is not believed. Measured again through the real function afterwards: 3 957 → 0, with the two files that were genuinely modified at that moment still named. Also: - **The index-bit oracle failed OPEN.** Its guard asked for `status === 0`, and a spawn that dies leaves `status` null — so the one tree it could not read the skip-worktree/assume-unchanged bits of was certified clean, which is precisely the tree that can be carrying a mutant `status` cannot see. It now fails closed like its three siblings, and takes `-z` so `core.quotepath` cannot octal-expand a non-ASCII name past the buffer. - **The efficacy probe tree carried tracked writes between runs.** Re-linking `node_modules` before each run (round 5) covers half of what a run leaves behind; tracked files are the other half and the more direct one — a suite that rewrites a probe file after vitest has collected it stays green for its own run and hands every later run a file of its choosing, and the verdict that buys is `killed`: "a test catches this", asserted for statements no test covers. Every run now opens by putting the tree back to its commit, guarded by the same identity gate the residue probe uses so a directory that is not a checkout is skipped rather than having the enclosing repository checked out into it. Two fixtures had to change with it, and both were relying on the hole. The control-setup test made its point by having the fake runner DELETE the probe file; a deleted probe file now comes straight back, so it relinks it instead (the read-failure path itself is pinned directly in the unit suite). The wrong-line-mutant test wrote its mismatched content without committing it, which is not a state a probe tree can be in — a probe tree is a detached checkout — so it commits it. Each fix was verified by reverting it and watching the new test go red: the provenance filter, its trust direction, the catch-all exception, the `.husky/_` footprint entry, the fail-closed oracle, and the tracked-file restore. * chore: keep main's package-lock.json A local `npm install` during the merge dropped six `"peer": true` markers from the lock. Unrelated to this branch, so it goes back to main's copy. * fix(review): close the round-16 Criticals — catch-alls, the exec env family, untracked plants Two of these are holes in last round's own fix, which is the right way round for them to be found: - **`hidesEverything` recognised only the `*`/`**` spellings.** `?` matches any single character, so `?*` is `*` with extra steps — a committed `.gitignore` whitelist written that way vouched for everything it hid, with no execution anywhere: it is committed content. The check is now shape-based rather than a list of spellings: every segment wildcard-only, at least one `*`. Enumerating the spellings one at a time is how the hole got made. - **The probe tree's pre-run restore failed OPEN on an unresolvable `.git`.** The gate skips a directory with no `.git` because there is no commit to restore FROM — but `.git` is untracked, so nothing ever restores IT, and a guest that overwrites it once bought "proceed, nothing to put back" from every later phase. Present-but-unresolvable is now a failure, which is the same discipline round 15 applied to the residue probe's index-bit oracle. And three that stand on their own: - **`safeRmWithin` split the PR-controlled path on backslashes unconditionally.** On POSIX a backslash is an ordinary filename character, so ONE committed name — `\..\review-pr-42\package.json`, legal on POSIX — became three components, two of them `..`, which `join` normalises away silently. The revert phase's delete then reached the sibling shared review worktree. `probeTargetEscapes` already split platform-dependently with a comment naming this hazard: the write side had the fix, the delete side did not. Both now, and a `..` component is refused outright rather than resolved. - **`sanitizedGitEnv` closed redirection and config injection and left execution open.** `GIT_SSH_COMMAND` and `GIT_EXTERNAL_DIFF` are a command, `GIT_EXEC_PATH` moves git's own subcommand and remote-helper lookup, `GIT_TEMPLATE_DIR` plants hooks for the next `init`. Not a new judgement call: `config/shared-env-keys.ts` blocks exactly this family for session subprocesses, with the rationale written out there. The setter need not be an attacker — a reviewer's shell profile exporting `GIT_EXEC_PATH` silently changes which `git-remote-https` every fetch in this pipeline runs. - **Nothing removed UNTRACKED files between probe runs.** Round 15 restored the tracked half; a `vitest.config.ts` — untracked, because no zero-config project commits one — is what a suite reaches for to decide the next run's collection. `clean -fd` and not `-fdx`, so the borrowed farm and the ignored build output the probes need survive. Also, the live-symlink test in `git.integration.test.ts` compared git's CANONICAL worktree path against an un-canonicalized `mkdtemp` path. It passes on macOS by accident — `/private/var/…` contains `/var/…` as a substring, so `toContain` succeeds — and would not on a fixture reached through a symlinked ancestor. It now realpaths, so it passes on purpose. Each fix was verified by reverting it alone and watching its test go red. * fix(review): close the round-17 Criticals — provenance, absent .git, ignored plants, ancestor links Three are holes in the two previous rounds' own fixes: - **"Tracked" is not "unchanged".** The round-15 provenance test asked `ls-files` whether the ignore file's PATH is in the index, so a `.gitignore` the commit carries went on vouching for rules appended to it after the checkout. The status set the same function already computed answers this: a source that appears there has been edited away from the commit, and its rules are the writer's. - **An ABSENT `.git` was read as "nothing to restore".** Round 16 made the probe tree's restore fail closed on a `.git` it cannot resolve and left the cheaper state open — `.git` is an untracked pointer file inside the tree the PR's own suite runs in, so one `rm` bought "proceed" from every later phase. Running the restore anyway is not the alternative: with no `.git`, discovery walks UP and checks the enclosing repository out into the tree. Refusing is the only answer that is neither. - **The between-run sweep honored the ignore rules.** `clean -fd` skips what the commit's own `.gitignore` names, and those rules are the PR's to write, so a plant named to match one survived every restore. It is `-ffdx` now, with `-e node_modules`: the borrowed farm is the one ignored thing in that tree the probes cannot run without, and everything else ignored goes. The two restore spawns also empty `core.fsmonitor`, which both of them execute. And four that stand on their own: - **`releaseWorktree`'s symlink guard was leaf-only.** `lstatSync` dereferences every component except the last, so a link at `.qwen/tmp` left every path under it looking ordinary while `git worktree remove --force` landed in whatever checkout it named. `runCleanup` refuses its whole sweep for this; `cleanStale` releases with no guard of its own, so the refusal now lives at the choke point every caller inherits. - **`runCleanup`'s own ancestor guard ran before a network-bound audit** and nothing re-checked it afterwards, though the lease condition beside it gets exactly that re-check for exactly that window. - **The scratch tree's filter screen read the wrong tree's config.** It runs against the review worktree, while the checkout it authorises runs in the SCRATCH tree, whose own `config.worktree` is honored once `extensions.worktreeConfig` is on. The screen now reads every entry under the common dir's `worktrees/`. - **`runOneMutant` and `runControlMutant` had no pre-write escape re-check**, while `runOneHunkProbe` — in this same diff — carries one with a comment explaining the threat. `redirectedAncestor` is now one shared function rather than two, and its walk STOPS at the checkout instead of climbing to `/`: `/var` is a symlink on every macOS box, so the unbounded version refused every sweep there while reporting that it had found a redirect. Two fixtures became real checkouts, because a probe tree is one in production and a bare `mkdtemp` no longer reaches the behaviour they pin. Each fix with a behavioural test was verified by reverting it alone and watching that test go red. The pre-mutation re-checks are window-narrowing on a check-then-use race and are not pinned by one. * docs(review): drop the JSDoc left stranded when redirectedAncestor moved Round 17 moved `redirectedAncestor` into `lib/worktree.ts` and deleted the copy here, but not the block above it — which then sat on `scratchWorktreesOf`, describing a function this file no longer has and, worse, describing the OLD behaviour: "the walk stops at the filesystem root" is exactly what round 17 changed, because `/var` is a symlink on macOS and an unbounded walk refuses every sweep there. * fix(review): close the round-19 Criticals — PR-content channels, and the guards this PR wrote for itself Two are reachable from a PR's committed content with nothing executed, which is the line this branch draws everywhere else: - **A committed `vendor -> ../stash` was never counted.** The rebuild walk discloses a `node_modules` symlink resolving back INSIDE the disposable tree and silently skipped one resolving OUT of it — so the farm reported `{linked: n, failed: 0}` while a `node_modules` under the link's target survived every rebuild. Node realpaths the importing file, so imports under the link resolve in the stash and decide later verdicts. Outside state cannot be wiped from here; it is COUNTED, which is what the contract promises. - **`workspaces: ["."]` widened the self-link whitelist to the whole shared worktree.** npm accepts a root manifest declaring itself a workspace and creates the self-link itself, `containedIn(root, '.')` answers the root, and every `node_modules` link resolving anywhere inside the review worktree then passed the verdict and was mirrored into the disposable tree as a read-write channel back. A member that resolves to the dependency root is not a member. Four more are this PR's own guards not being applied to the code beside them: - **`resetScratchTree` lstat'd only the immediate parent**, on the stated premise that `.qwen/tmp` is the one component above the leaf anything here can replace. False one hop higher: a link at `.qwen` redirects the whole path, and then every check in that gate agrees with every other because they all resolve through it — toplevel self-equality, common-dir equality, gitdir ≠ commondir, and even the backpointer round-trip. It walks every ancestor now, bounded at the repository the common dir belongs to. - **`restoreProbeTreeTracked` had none of the gates its sibling reset has.** A rewritten gitfile pointing at another repository passed `--show-toplevel` self-equality and the "restore" wrote that repository's content in; a skip-worktree bit made `checkout --force` skip a tampered file silently while the function answered "as the commit left it". Both are checks `scratch-tree` documents for the identical reset. The backpointer and ancestor checks apply to the linked-worktree shape a probe tree actually has — a standalone checkout has no admin entry to round-trip and IS the repository root. - **The revert phase collected probes screened once, from the index, before the baseline.** Every run since executes the PR's own test code, which can replace a probe with a link; the mutation writers re-check immediately before they write and this collection had no equivalent, so a relinked probe was collected THROUGH the link and scored against code the revert never touched. - **`runCleanup`'s symlink branch never pruned.** It returns before `releaseWorktree`, which is where the pipeline's only other prune lives — so the family paths were unlinked and reported swept while their registrations stayed behind to wedge the next `worktree add`. And two cheap ones with no argument against them: `sanitizedGitEnv` deleted by exact case, which removes nothing on Windows where env lookup is case-insensitive (the `shared-env-keys.ts` list it is modelled on folds case for this reason); and it now sets `GIT_NO_REPLACE_OBJECTS=1`, because one `git replace <sha> <evil>` in the common dir makes every `checkout --detach <sha>` here materialise someone else's tree while `rev-parse <sha>` still answers the original. Each fix is pinned by a test that goes red when that fix alone is reverted. * fix(review): close the round-20 Criticals — an empty probe list, a dropped refusal, a symlinked root The first one is a bug this branch shipped yesterday, and the worst kind: it turns a screen into a wider run. - **The revert phase could call `runProbeSuite` with an EMPTY list.** Round 19 added a screen that drops probes relinked out of the tree after the baseline; the `probes.length > 0` gate the phase opens with was taken before that screen could empty the list, and `vitest run` with no file argument collects the WHOLE suite — so "every probe was tampered with" became "score everything", with the verdicts attributed to files the phase never selected. It now stops the phase and says which of the two happened, once rather than per file. - **`cleanStale` discarded `releaseWorktree`'s refusal.** The guard added in round 17 declines to release through an ancestor symlink and reports why; `fetch-pr` called it as a bare statement, so the sweep looked successful and the next `worktree add` wedged at a path nobody was told about. The result is read and the reason printed, like every other failure on that path. - **The probe tree's own root was never lstat'd.** The ancestor walk added in round 19 starts above the leaf, and every identity comparison realpaths both sides — so a probe tree that IS a symlink into the shared review worktree agrees with itself all the way down, and the restore's `checkout --force` and `clean -ffdx` would have run in the tree every other agent is reading. The first is pinned end-to-end by the fixture that already relinks its probes mid-run: before round 19 it scored that relinked probe `inert` — a fabricated verdict read through the link — and the assertion now names the phase-level refusal instead. Reverting the guard alone turns it red. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-20 07:50:44 +00:00
ChiGao	02d303f849	feat(serve): persist prompt terminal ledger for cold-load reconciliation (#9426) ... * feat(serve): persist prompt terminal ledger for cold-load reconciliation Turn terminal events (turn_complete / turn_error) were synthesized by the ACP bridge and published over SSE only, so a prompt that was in flight when the daemon died could never be resolved after a restart: the cold load replay emits transcript chunks and carries no terminal evidence, leaving promptId-keyed orchestrators stuck on "unknown". Each session now owns an append-only sidecar ledger next to its transcript. The bridge appends one in_flight record at prompt admission and one terminal record at the single publishPromptTerminal exit (covering the close/kill/channel-crash/daemon-shutdown flushes) through an injected synchronous sink. Ledger writes are best-effort and never block prompt execution or teardown, and records carry only ids, states, and timestamps — no prompt text, user content, or paths. On a cold session load the serve layer reconciles prompts left dangling by a dead daemon: it classifies the transcript tail with the existing turn-interruption detector and appends a completed (stop reason reconstructed_from_transcript) or interrupted (code daemon_lost) verdict, guarded by an attribution check so an unattributable tail stays unknown (fail-closed). The load response gains an optional promptTerminals field with the trailing 64 terminal records, omitted entirely when the ledger holds no terminal evidence, and archive/unarchive move the sidecar alongside the transcript so evidence survives storage lifecycle. Design: docs/design/2026-08-19-prompt-terminal-ledger-design.md * fix(serve): tighten ledger reconciliation fail-closed semantics and complete sidecar lifecycle Address review findings on the prompt terminal ledger: - reconcile: fail closed on multiple dangling prompts (no synthesized terminal for the newest either); attribute the oldest dangling prompt only when the attribution guard skips settled admissions (fixes the [A if, B if, B cancelled] misattribution veto), the transcript's last write postdates the admission (temporal evidence), and a clean verdict is upgraded to interrupted when the model tail holds any functionCall part, id or not (id-less tool-call guard covering the detectTurnInterruption wire-pairing blind spot) - lifecycle: removeSessionFiles deletes the ledger in both archive states; archive/unarchive move it through a single getPromptLedgerPathForState helper with merge semantics when the destination already exists (append-and-unlink instead of a permanent split); move warnings carry full source and destination paths in both directions - scans: DataProcessor.scanChatFiles and usageHistoryService.rebuildFromSessionJsonl exclude .ledger.jsonl sidecars (the ledger is not a transcript) - writer: appendPromptLedgerRecord seals a torn tail before appending so a torn fragment cannot fuse with (and destroy) the next record - tests: pin the new behavior across multi-dangling fail-closed, settled-then-queued attribution, valid interleave migration, temporal veto, id-less tool-call guard, sidecar lifecycle (move/merge/warn-only delete), torn-tail sealing, queued-admission flush on shutdown, active-prompt and resume load contracts, and ledger exclusion from insight scans - docs: sync the design doc's reconciliation algorithm, lifecycle, and fail-closed invariants * perf(serve): read only the ledger tail for load-response promptTerminals readRecentPromptTerminals ran on every POST /session/:id/load (including attached hot loads) and synchronously read and JSON-parsed the entire ledger — a multi-megabyte event-loop stall for long sessions on the per-request hot path. Add a tailBytes option to readPromptLedgerRecords that reads a trailing byte window (the first window line is always dropped: the window start can tear a line in half). The load path now reads a 256 KiB window, which holds hundreds of ~150-byte records against the 64-terminal response cap; sessions whose ledger outgrows the window return a best-effort trailing subset, which the response contract already allows. * fix(serve): close wrong-terminal attribution classes in cold-load reconciliation Strengthen the reconcile attribution evidence per review round 2: measure the temporal evidence on the same api-history projection the verdict uses, fail closed on a compression checkpoint written after the target's admission, and require the visible tail to postdate every other prompt's settled terminal (FIFO evidence). Also fix a TS18048 narrowing gap in the window test, make the seal test assert the raw file layout, and restructure the window test so the call-site tailBytes wiring is actually observable. * fix(serve): keep ChatRecord import inline so lint-staged cannot merge it into a type-only import * fix(serve): fail-closed reconciliation on millisecond clock equality and deadline-overlapped turns * fix(serve): TOCTOU fence before ledger append and documented residual attribution risk * test(serve): pin the ledger race fixture on the transcript timeline * feat(serve): bind cold-load evidence to the admission via a dispatch marker * test(core): pin the ledger sidecar exclusion in usage rebuild * fix(serve): create ledger sidecar owner-only and fence marker-era compression by position Round-7 review Criticals: - appendPromptLedgerRecord created the sidecar with umask-default permissions (0o644) while the adjacent transcript is owner-only; the ledger now follows the 0o600 convention at creation time. - Marker-bearing admissions fence post-admission compression by marker position instead of wall clock, so a backward clock step cannot hide a compression reset that voids the evidence chain. - Design doc: the residual-risk claim is corrected — the dispatch marker binds ordering, not ownership; the two ownership classes that survive it (recordless predecessor with continued writes, ledger-less cross-client writer) are documented, pending writer identity on transcript records (#9483). --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>	2026-08-20 07:27:49 +00:00
易良	48b30647d0	refactor: centralize cross-package contracts (#9497) ... * refactor: centralize cross-package contracts * fix(build): harden cross-package contract checks * docs(core): clarify sub-session prompt limit scope	2026-08-20 06:24:41 +00:00
ytahdn	a41d5ec058	feat(web-shell): unify file uploads and references (#9477) ... * feat(web-shell): unify file upload and reference flow * fix(web-shell): address attachment review feedback * fix(web-shell): address attachment review feedback * fix(web-shell): address attachment review feedback * fix(webui): restore optimistic text prompts --------- Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-20 06:18:34 +00:00
jinye	a659539bc7	feat(cli): Add standalone conversation isolation primitives (#9341) ... * docs: finalize standalone PR2 core design Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(cli): Add standalone conversation isolation primitives Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): block mixed-case standalone restore Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): fail closed on corrupt session metadata Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): repair PR2A CI doubles and identity replacement cases The ubuntu Test job failed on ten PR2A cases that pass on macOS: - Export SessionIdCaseConflictError from the worktree test's core mock and give its SessionService double findSessionIdIgnoringCase, since loadSession now resolves persisted spelling before reading metadata. - Add readCreationMetadataIfReadable to the Live task fake and mirror it onto the three server lifecycle spies so the fail-closed store interface sees the same fixture metadata as the legacy tolerant readCreationMetadata path it replaced. - Pin the original inode via rename in the two same-path replacement cases. ext4/overlayfs recycle a freed inode immediately, so rm+mkdir at the same path could satisfy the recorded device+inode identity on Linux runners and make a real replacement look valid. * fix(serve): key restore shared guard on persisted session id spelling The restore handlers resolved the persisted (possibly uppercase) spelling of a session id only inside the shared coordinator guard, while batch delete locks its exclusive guard on the raw caller ids. A restore of the normalized request id therefore raced a concurrent batch delete of the persisted-spelled id on case-sensitive volumes. Resolve the persisted spelling before acquiring the shared guard and key runSharedMany on the resolved id so both sides contend on the same key, in both the REST and ACP restore handlers. Regression tests assert the guard key at both transports. * fix(serve): unify persisted-session conflict contract across restore surfaces - SessionIdCaseConflictError now carries an optional candidateSessionId with a shape-aware message, so a same-spelling active+archived conflict names the persisted spelling instead of blaming the request-case id. - REST/ACP-HTTP conversion re-checks the candidate spelling before mapping to SessionConflictError; ACP child surfaces both shapes as INTERNAL_ERROR + errorKind 'session_conflict', and the reserved-source rejection carries errorKind 'reserved_session_source'. - Pin the previously ungated guards from review: lineage validity conjuncts and archived-state reads (session-source), persisted-spelling adoption in ACP load/resume, ensureStandaloneDirectory EEXIST raced re-inspection, and the trailing root revalidation inside inspectConversationDirectoryIdentity (fs-interception seam). * docs: sync standalone PR2 plan and design docs with R1 review - Plan doc: ship the readCreationMetadataIfReadable signature in the store snippet, record jsonl-utils/error-response/readCreationMetadata in the PR2A checklist with the second-core-file re-audit outcome, name the launcher as child-UUID generator in both sections, extend the PR2A vitest and prettier gates to every PR-touched file, and stop claiming junction/Windows coverage the matrix cannot run. - Design doc: reconcile the initial-prompt ordering clause with the strict create schema (prompt admitted by createWithInitialPrompt after the create transaction commits), replace the stale "before the UUID can be released" wording with the terminal-reservation model, and enumerate all seven deny categories in the acceptance matrix. * fix(cli): normalize session-map lookups in restore failure cleanup guards The sessions map is keyed by normalizeSessionIdForLookup-folded ids, but the three cleanupAfterRequestFailure guards read it with the raw config.getSessionId(). After a restore adopts a non-canonical persisted spelling (uppercase legacy transcript), those reads always miss and the guard would treat a still-stored session as unstored, shutting its config down in the double-cleanup-failure window. Normalize at the read sites; no behavior change for canonical or non-UUID ids. The adoption test now also pins that caller-case follow-up operations (cancel) still reach the adopted session through the normalized key. * fix(cli): snapshot standalone dir entries after final identity inspect prepareStandaloneDirectory read the child entries before the trailing identity re-inspection, so a same-uid entry appearing between the two steps would not flip the not_empty verdict. Read entries after the final inspect so the emptiness check runs on the freshest snapshot the identity machinery can guarantee. Addresses yiliang114's review on PR #9341. * fix(cli): lock restore guard on both request and persisted session id spellings Restore keyed its shared SessionArchiveCoordinator guard only on the resolved persisted spelling while batch delete/archive/unarchive lock on raw caller ids, so on a case-insensitive filesystem a delete carrying the request-case spelling never collided and could unlink the transcript mid-restore (R2-1). Lock both spellings on the REST and ACP-HTTP restore surfaces. Also from R2 review: - Pin the pre-guard/in-guard conflict-conversion stages in the both-states restore tests (R2-8: call-count + guard-not-entered assertions; mutation witness supplied by the reviewer). - Add the toRpcError SessionIdCaseConflictError producer case to dispatch-error.test.ts and list the suite in the PR2A verification block; extend the PR2B block with the collocated suites its checklist modifies (R2-5). - Correct the plan checklist label for the pre-existing shared jsonl-utils module from Create to Modify (R2-7). * fix(cli): canonicalize session-archive coordinator lock keys The two-spelling restore guard from the previous commit closed only the enumerated spellings: any third case variant of a caller id took an exclusive key that collided with no held guard, and on a case-insensitive filesystem could unlink the transcript mid-restore — including the common case where request and persisted spellings coincide and the lock set collapses to one key (R3 review, probe- verified on a case-insensitive mount). Canonicalize lock keys with normalizeSessionIdForLookup at the coordinator boundary (runSharedMany / runExclusiveMany / assertNotTransitioning) so every case variant of a session id contends on one key, and revert the restore-side spelling enumeration it makes redundant. Add a coordinator-level regression test for the case-fold collision, and fix a misleading comment above the in-guard call-count assertion (R3-1). * test(cli): pin workspace ordering/race propagation and align PR2 plan From the R4 review: - Pin that prepareStandaloneDirectory reads entries after the final identity re-inspection, via an interposed inspect that plants an entry mid-sequence (R4-3; mutant-verified to flip). - Pin that ensureStandaloneDirectory propagates a raced 'compromised' inspection verbatim instead of collapsing it to identity_changed (R4-7; mutant-verified to flip). - Plan: daemon bridge live-entry lookups (including getSessionEventEpoch) use the canonical ID — acp-bridge byId.get is exact-match with no id normalization — while the storage spelling is confined to SessionService filename/directory-hash/ACP-child storage operations (R4-1). - Plan: declare the session-archive coordinator lock-key canonicalization in the PR2A inventory and run session-archive.test.ts in the PR2A block (R4-2); add dispatch-error.test.ts to the PR2B block (R4-5). * fix(core): make case-insensitive resolver conflict decisions content-based The resolver threw SessionIdCaseConflictError on filename enumeration alone, before any content validation, and silently dropped a single candidate whose head recovered no records. Two probe-verified failure modes from the R5 review: - A present-but-unreadable case-variant transcript (torn/empty/ foreign-project head) resolved to undefined, so create admission admitted the canonical spelling and materialized a case-only twin; every later resolve then threw on the duplicate, permanently locking out the just-created session (R5-1). - A valid session with an unreadable same-spelling twin in the other state directory threw on enumeration while getSessionLocation cleanly reported one readable copy — listed as loadable, but every restore 409'd (R5-2). Conflict arms now consult getSessionLocation: exactly one readable spelling wins; conflict is thrown when two or more are genuinely readable (or a single candidate is conflicted across states); a candidate whose head fails validation still occupies the id when its file is on disk, while one that raced away mid-resolution resolves to undefined. Admission already maps the thrown conflict to persisted-true, so no admission change is needed. * fix(cli): make caller-supplied sessionId create admission case-aware The argv['sessionId'] branch (reached by raw stdio ACP session/new with a requestedSessionId, without any daemon reserveCreate) checked occupancy with exact-spelled sessionExistsInAnyState only, so a legacy mixed-case transcript did not block the create and the daemon persisted a case-only twin — which the resolver's conflict semantics then make permanently unrestorable on every surface (R5-2). Route the check through the case-insensitive resolver, treating its conflict throw as occupancy. Also pin that the ACP restore path hands the resolver-adopted storage spelling to assertSessionLoadable: archived uppercase transcript restored via the canonical lowercase id must surface errorKind 'session_archived' (R5-3; the request-spelling mutant skips the error on case-sensitive filesystems). * fix(cli): narrow reserved-source restore gate to internal runtimes Two items from the maintainer's round-2 live verification: - N1: the restore-side reserved-standalone-source gate fired for every runtime, so a transcript persisted on main with the client-supplied sourceType "standalone" became permanently unloadable while still listed. The create side already blocks new reserved-source transcripts, so any such file on an ordinary store predates the gate — keep it loadable there and hide only on the internal Conversations runtime (REST) / isolated ACP surface, where genuine standalone sessions will live. Generic-arm tests on both surfaces flipped to pin the compat restore; the internal-arm 404 pin is unchanged. - N2: the R5 occupancy throw reused the both-states message for a single unreadable transcript. SessionIdCaseConflictError gains a 'unreadable_transcript' reason with a truthful message, used by both occupancy arms; the case_conflict shape is unchanged. * test(cli): flip the remaining generic-surface reserved-source test The exact-spelling variant was missed in the N1 narrowing commit and failed CI on ubuntu (session/load + session/resume expected the old generic-surface hide). Flip it to pin the compat restore like its mixed-case sibling. * docs: sync PR2A plan with the shipped create-admission resolver consumer The round-6 triage deferred note flagged the plan as desynced: the R5-2 fix made loadCliConfig's caller-supplied sessionId branch a sixth findSessionIdIgnoringCase consumer. Declare it in the per-file checklist, count it in the consumer inventory, and add config.test.ts to the PR2A vitest block. * fix(core): narrow the session id case resolver's occupancy arms Three shapes were classified as permanent occupancy, regressing paths that loaded or created fine before the resolver replaced the exact-existence check: - Candidates were enumerated by case-insensitive filename match without the pattern gate that getSessionLocation applies, so an agent-suffixed id (which the CLI admits and writes under the raw session id) resolved to an unreadable-head conflict. Skip names the classifier would reject. - On a case-insensitive filesystem every spelling opens the same physical transcript, so a readable copy plus a torn case twin reported two readable candidates and raised a conflict for a session with one loadable copy. Collapse spellings that share a device/inode and resolve to the one whose own directory entry backs the file. - An unreadable head under the requested spelling itself is a case-only twin of nothing, yet it refused the id with no listing entry to delete or unarchive. Report it absent, matching getSessionLocation, so a first run that crashed before its first record can reuse its own 0-byte transcript. The twin-minting protection still applies when the persisted spelling differs from the requested one, and genuinely distinct readable spellings still conflict. Also pin the explicit-standalone child branch's sourceId guard and its documented lineage behaviour, which no case covered. * fix(cli): key the private conversation directory on the canonical id Restore derived the directory hash from the persisted spelling while the seven other materialize/discard call sites derived it from the lowercased live id, so restoring a legacy mixed-case transcript produced one directory and every later Live or task call produced a second, empty one — either orphaning what the first held or failing the call because the session sat outside its isolated directory. Rollback then inspected the other hash and leaked the first directory permanently. The directory belongs to the live entry, which the bridge registers under the canonical id alongside the lifecycle locks and in-flight maps, so both restore paths now derive it from that id too. This also keeps directories that pre-date the change reachable: restore used the lowercased request id before, so every one already on disk is canonical-keyed. Storage-facing operations — transcript filenames, metadata reads and the ACP child's own session storage — keep the authoritative spelling. The design doc and PR2 plan are corrected to scope the spelling rule accordingly. * test(cli): follow the canonical private-directory key in the Live restore case The internal-restore case pinned the directory hash to the persisted spelling, which the canonical-key change inverted. It now asserts the canonical id for the directory and bridge cwd, and keeps the original intent explicit by asserting that the creation-metadata read still uses the persisted spelling. * fix(cli): report proven parent lineage from the loadable-session reader The exported reader returned one verdict for two different situations: a child whose parent lineage it had verified, and an explicit standalone child whose parent it never read. PR2B is being built on that reader, so the ambiguity mattered even though no caller was affected yet. The verdict now carries the parent's own classification. An explicit standalone child whose parent is still readable must have a standalone top-level parent, which also rejects a grandchild or a lineage cycle because neither parent classifies as top-level. A parent that has been archived away or deleted keeps the child loadable — it is self-describing, and its own transcript is the evidence that a valid parent existed when it was created — but `parentSource` is then absent, so a caller that needs proven lineage rejects on that rather than guessing from `kind`. The compatibility adapter reads the new field instead of re-reading the store to re-derive the same classification, so its behaviour is unchanged while a duplicated location lookup and metadata read disappear from every legacy standalone child restore. Adapter output is identical for every input: explicit standalone was already filtered out before the parent check, so no current caller changes behaviour. * fix(core): stop the alias resolver from turning I/O and missing inodes into conflicts Two defects in the case-variant collapse added earlier in this branch. The stat guard was statically dead: `statSync` without `{ bigint: true }` always returns numbers, so the `typeof` test could never fire. The hazard it was meant to cover is a filesystem that exposes no inodes — FAT/exFAT and some SMB mounts report `ino === 0` for every file — where `dev:ino` collapses genuinely distinct transcripts onto one identity and a real two-transcript conflict silently resolves to one spelling. That is a fail-open on a correctness decision, so it now uses the existing `hasVerifiableInode()` helper, whose docblock describes exactly this case. The resolver also swallowed every `statSync` failure into `undefined`, which its caller reads as positive proof of a conflict. A transient EACCES or EMFILE therefore surfaced as `409 session_conflict` — a permanent-looking answer for a blip that succeeds on retry. Only ENOENT is now treated as meaningful: a transcript that raced away is no longer a competing spelling. Every other error propagates. * fix(core): let a crashed first run resume its transcript past a case twin The self-escape added for an unreadable transcript under the requested spelling only covered the single-candidate arm. Once any case twin was enumerated, resolution took the all-unreadable arm instead, where presence was computed over every candidate including the requested spelling's own file — so the documented crash recovery vanished the moment a stale twin existed, and neither file could be deleted because both classify as nonexistent. Reusing an id whose file is already on disk mints no case-only twin, so that arm now takes the same escape. A twin under a different spelling still occupies the id, because minting the requested spelling beside it is what would make both unrestorable. The disappearance test was vacuous: its candidate spelling equalled the request, so it returned at the self-escape and never reached the race loop it named — deleting that loop left it green. It now uses a differing spelling with `existsSync` false, and forcing the loop to throw unconditionally kills it. The all-unreadable rejection test likewise needed two spellings that are both distinct from the request to exercise twin-minting protection. * fix(cli): fail closed when a filesystem cannot prove directory identity The conversation-directory checks compared `dev`/`ino` directly, so on a filesystem that exposes no inodes — FAT/exFAT and some SMB mounts, where Node reports `ino === 0` for every entry — every directory compared equal. The root pin, the two anti-swap re-probes around `realpath`, and the expected identity check would all confirm a directory that had in fact been replaced, which is the swap those probes exist to catch. They now require a verifiable inode on both sides before treating a match as proof, reusing the `hasVerifiableInode()` helper already written for this in core and exporting it from the package surface. An unverifiable inode reads as a changed identity rather than as a match. The regression test pins a root whose inode is also 0, so a plain `===` comparison still matches and only the verifiability guard can fail it. * fix(cli): keep caller-supplied session-id admission fail-closed on I/O errors Swapping the existence check for the case-insensitive resolver narrowed the catch to `SessionIdCaseConflictError` and rethrew everything else, but the resolver deliberately propagates non-ENOENT `readdir` and transcript-read failures. An unreadable chats directory therefore killed startup with a raw EACCES or ENOTDIR instead of the guarded message, and bypassed the `throwOnSessionIdConflict` contract the ACP path depends on. The previous check answered "occupied" for any read failure. Restoring that keeps an unprovable id on the guarded path; distinguishing "cannot determine" from "occupied" would be a new response shape and is left alone here. * fix(cli): keep the directory identity module out of the core package barrel Importing `hasVerifiableInode()` from the core package barrel pulled core's whole module graph into the serve pre-listen bundle closure, so `check:serve-fast-path-bundle` reported glob, chokidar, fzf, @iarna/toml and the core shell tool runtime as statically reachable from `run-qwen-serve`. This module is deliberately dependency-free for that reason. The predicate is restated locally with a comment recording why it is not imported, since core has no subpath export for it. The barrel export added for that import is reverted so the package surface is unchanged. * fix: correct three defects introduced by the previous review round **The inode guard made a directory fail to equal itself.** `createConversationRootIdentity()` compares `before`/`after` of the same path, so requiring a verifiable inode threw `identity_changed` on the very first root establishment and, because the workspace clears its cached root on failure, Conversations never started on exFAT/FAT or an inode-less SMB mount. "Cannot prove unchanged" is not "changed": the root is now established with `inodeVerifiable: false` recorded, comparisons fall back to device, canonical path and stat shape, and the weaker guarantee is explicit on the identity for callers to surface. Where inodes exist they are still required to match. **The occupancy escape was placed to discard a real twin.** It returned early for the whole arm whenever the requested spelling was enumerated, so a present-but-unreadable twin stopped occupying the id — the case-only twin the surrounding comment exists to prevent. The escape belongs per candidate, not per arm: the requested spelling's own file never counts as occupancy, every other spelling still does. **The private directory was still a caller obligation.** The comment claimed the bridge registers live entries canonically and every materialize derives from that id, but the bridge echoes whatever the caller passed, and `LiveTaskService.ensureResident()` passes an id that originates in a tool argument. `ConversationWorkspace` now canonicalizes before hashing, so one session resolves to one directory by construction. Also unifies the resolver's two arms, which were the same algorithm written twice — that duplication is why the escape landed in only one copy. * fix(cli): Collapse case-variant session ids in batch lifecycle and CLI create Batch delete/archive/unarchive locked on canonical keys but still deduped raw spellings, so two case variants of one id deadlocked the batch. CLI --session-id now stores the lowercase spelling so new mixed-case transcripts stop accumulating. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: Cursor <cursoragent@cursor.com>	2026-08-20 06:15:52 +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
易良	b5577b7d11	fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel (#9202) ... * fix(sdk): route unrecognized diagnostics onto a bounded transcript sidechannel Normalizer-classified unrecognized_event / unrecognized_session_update debug events no longer enter transcript blocks[]: they are mirrored onto a capped unrecognizedDiagnostics sidechannel instead. This stops them from finalizing a streaming assistant/thought block (which dropped a following assistant.usage frame) and from consuming the maxBlocks budget (which let repeated noise evict real conversation content). malformed_payload diagnostics and client-dispatched debug events keep their existing block semantics. * fix(sdk): align browser bundle budget * fix(sdk): close the sidechannel review round (#8823) - export the sidechannel API through the daemon barrel (selectUnrecognizedDiagnostics, UNRECOGNIZED_DIAGNOSTICS_LIMIT, DAEMON_UI_UNRECOGNIZED_DIAGNOSTIC_REASONS + types) and pin the reachability in daemon-public-surface.test.ts - restore the MAX_TEXT_BLOCK_LENGTH cap on sidechannel text, mirroring truncateText exactly (suffix fits within the cap) - ship the unrecognized reason subset as a runtime const array and route by membership, so a new reason cannot fall through to appendStatusBlock - copy the correlation fields createBase stamps (promptId, sourceRecordIds, branchRecordId, originatorClientId) onto sidechannel entries; drop the dead source/data switches - un-fuse the budget-history comment chain in scripts/build.js - update docs/developers/daemon-ui for the split routing - tests: full entry shape, text cap, block-path debugReason counterpart, and a webui malformed_payload interleave sibling so the #7012 flush-before-guard keeps a discriminating stimulus * fix(sdk): address round-2 sidechannel review for #8823 - build.js: bump daemon browser bundle budget 191KB -> 192KB (195,591 bytes measured > 195,584 cap; build failed at head) - webui: narrow the observer-mode debug guard so unrecognized_* diagnostics reach the reducer sidechannel; only block-path debug events are dropped - webui: merge history-store unrecognizedDiagnostics in applyTranscriptHistory so paged-back sessions keep diagnostics - transcript: extract truncateTextAtLimit shared by the block and sidechannel truncation paths - transcript: reset unrecognizedDiagnostics on rewind alongside the sibling per-turn state resets - types: rename DaemonUnrecognizedDiagnostic.receivedAt to clientReceivedAt (matches the sibling block projection) - tests: reason-prefix conformance pin, rewind reset, narrowed guard, history pagination merge * fix(webui): avoid flushing sidechannel diagnostics * fix(sdk): preserve diagnostics across rewind * fix(webui): dedupe sidechannel history records * fix(webui): align the paging sidechannel test with the normalizer keys The paging test added in e6b40e5c failed deterministically (webui suite red, CI Test job red) for two reasons: 1. The fixtures stamped only _meta['qwen.session.recordId'], but the SDK normalizer's extractSourceRecordIds reads _meta.qwenTranscript.sourceRecordIds — no sidechannel entry ever carried sourceRecordIds, so the dedupe assertion could not pass and the new displayedRecordIds loop was never exercised by a passing test. Stamp BOTH keys, matching production replay frames (acp-bridge buildUpdateMeta) and the sibling dedupe test. 2. Cap arithmetic: LIMIT-1 live entries + 2 fresh history entries = LIMIT+1, so the newest-wins slice evicted record-old-1 which the test asserted present. Emit LIMIT-2 live events so the post-merge total lands exactly on the cap. Also correct the post-merge index assertions: history entries come first (old-1, old-2), then the deduped-once live overlap, then the first live mystery event. Suite 506/506, eslint + prettier clean. * fix(sdk): raise diagnostic sidechannel bundle budget * fix(sdk): raise the daemon browser bundle budget to 198KB and pin the diagnostics selector - The sidechannel routing + selector cost ~1037 B over the 197KB cap (bundle measured 201893 B), failing the browser-bundle size gate; bump MAX_DAEMON_BROWSER_BUNDLE_BYTES to 198 * 1024. - Fold the rebase-residue 190→191→192 KB ledger entries into the accurate 190→195→196→197→198 lineage so the next bump has one canonical history. - Add a behavioral pin for selectUnrecognizedDiagnostics: it must return the routed sidechannel itself (toBe), discriminating a `return []` or shallow-copy regression that the typeof-only surface test cannot see; flip-verified. * fix(sdk): reset the user pointer on sidechanneled diagnostics, share the routing predicate appendUnrecognizedDiagnostic left activeUserBlockId untouched while the replaced appendStatusBlock path reset it for every non-user block; a later mergeable user.text.delta with no promptId stamp (e.g. a peer client's $ <cmd> echo) then appended onto the earlier user block across the diagnostic, collapsing two user turns into one and skewing rewindTranscriptToUserTurn's kind==='user' turn indexing. Keep the reset (assistant/thought pointers stay untouched, the point of the sidechannel); witness test flip-verified red without the one-line reset. Also export isUnrecognizedDiagnosticReason from types.ts next to DAEMON_UI_UNRECOGNIZED_DIAGNOSTIC_REASONS and call it at all three routing-guard sites (reducer, provider flush condition, provider drop filter) so the #7012/#8823 guard pair classifies every debug event against one source instead of three hand-written copies. * fix(ci): prevent bite harness SIGPIPE --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-19 14:38:00 +00:00
Heyang Wang	5003ab3c7f	feat(web-shell): add transcript contract prevalidation (#9388) ... * test(web-shell): add transcript contract prevalidation Freeze reproducible evidence for current transcript paths before any VS Code or HTML export production migration. - Add versioned fixtures, closed export schema, and capability gates - Probe direct-daemon and ACP identity under partial history prepend - Preserve raw adapter semantics and full write_file Turn Output diffs - Document the two-MR architecture, security constraints, and blockers * fix(web-shell): harden transcript prevalidation gates Make the evidence-only contract suite enforce the review assumptions it documents while preserving the existing runtime transcript behavior. - Run the contract suite in the required no-AK integration job - Fail closed on ambiguous identity probes and deduplicate gate kinds - Enforce manifest, hash, export safety, and renderer version boundaries - Cover visible transcript text and stable Desktop packaging semantics - Record the complete PR comment evaluation and verification outcome * fix(web-shell): close transcript prevalidation gaps * fix(web-shell): remove brittle Desktop wiring probe Keep transcript contract prevalidation at the evidence level it can actually prove. The previous source-text assertion could both reject equivalent formatting and pass unreachable packaging code. - Remove the Desktop script parser and its false behavioral claim - Mark installed-artifact verification as deferred to Desktop smoke tests - Clarify MR1 matrix, CI wiring, and provenance evidence boundaries - Refresh the hash-locked capability matrix fixture Note: This does not change Web Shell or Desktop production behavior. --------- Co-authored-by: heyang.why <heyang.why@alibaba-inc.com>	2026-08-19 14:13:12 +00:00
ChiGao	d96f264de7	feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent (#9391) ... * feat(telemetry): link daemon HTTP request spans to inbound W3C traceparent The daemon HTTP surface records a request span per request, but every span starts a new trace: a caller forwarding the standard W3C traceparent header (OTel-instrumented clients, proxies, gateways) gets no linkage back to its own trace. Extract traceparent/tracestate from inbound request headers in the daemon telemetry middleware and parent the request span to that remote context. Extraction reuses the same path as the existing JSON-RPC _meta extraction (global propagator first, strict manual fallback so behavior is identical without a registered SDK) and fails closed: requests without a valid header keep the exact current span shape. * fix(telemetry): guard inbound traceparent sampling and align W3C fallback - Force TraceFlags.SAMPLED on inbound HTTP parents via the existing shouldForceSampled() matrix: an unsampled remote parent under the default parentbased_always_on sampler silently dropped the request span, the whole next() subtree, and the session-subprocess spans forwarded via _meta (review C1). - Replace the hand-rolled manual fallback parser with a direct W3CTraceContextPropagator instance so acceptance rules (future versions, tracestate, all-zero ids, version-00 extension field) match the registered path with or without an initialized SDK. - Gate middleware extraction behind isTelemetrySdkInitialized() to skip the hot-path parse when telemetry is off, and emit a debug daemon log when a present-but-invalid traceparent header is rejected. - Re-export DaemonRequestSpanOptions from the core barrel and add a type-level guard so the parentContext field cannot silently disappear (vitest alone cannot catch its removal). * chore(vscode): regenerate companion NOTICES.txt for @opentelemetry/core * fix(telemetry): lazy-load OTel core fallback propagator behind SDK init Address review feedback on the inbound traceparent linkage: - Keep @opentelemetry/core out of the static graph. The module-level W3CTraceContextPropagator in daemon-tracing.ts pulled the CJS barrel (bot-measured +65,046 bytes) into every closure loading that module, including telemetry-off deployments. daemon-tracing.ts now keeps only a holder + setter (setDaemonFallbackPropagator, typed against @opentelemetry/api — type imports stay free at runtime); the lazy sdk-impl.ts chunk, whose closure already contains @opentelemetry/core via sdk-node/resources, constructs and injects the W3C instance on the successful SDK assembly path. Until injection, extraction returns no parent context: the HTTP edge is already gated on isTelemetrySdkInitialized (nothing changes when telemetry is off), and the _meta edge's consumers (withDaemonSpan / withInteractionSpan) short-circuit on the same flag, so an unresolved pre-init parent never had an observable effect. - Add the mutation-verified fail-closed test for the header-extraction try/catch in daemonTelemetryMiddleware: a throwing extractor leaves the request settling normally (recordDaemonHttpRequest still fires once) with no parentContext on the span options. - Record the rejected traceparent value (truncated to 128 chars) as http.request.header.traceparent on the invalid-header breadcrumb — traceparent only carries trace-id/span-id/flags, so this is privacy-safe and makes broken cross-service joins diagnosable. Also document why the _meta extraction path deliberately skips shouldForceSampled (trusted in-process bridge vs external HTTP input). * feat(telemetry): carry inbound trace id into daemon access log with telemetry off Telemetry off (the default) left daemon logs without any trace id: with no request span, the log trace prefix never fires, so a caller forwarding W3C traceparent could not be joined to its daemon log lines. The middleware now parses the header with a plain regex (extractInboundTraceId — same shape/all-zero/ff rejections as the W3C propagator, no OTel machinery) and stores the trace id on the per-response telemetry context. The access log emits it as the camelCase traceId field of "request completed", keeping the log-based join alive with no telemetry config and no trace backend. With telemetry on nothing changes: the request span already carries the caller's trace id into the log prefix. * fix(telemetry): unify _meta/HTTP sampling and repair build export - Export extractInboundTraceId from the core barrel: the previous commit exported it from daemon-tracing.ts only, so downstream package builds failed with TS2305. - extractDaemonTraceContext now applies the same shouldForceSampled() matrix as the HTTP edge: the _meta path is also reachable from direct ACP clients (acpAgent newSession/loadSession/unstable_resumeSession and Session.prompt pass caller-controlled _meta), so an external sampled=0 parent no longer silences daemon spans there either. The in-process bridge is unaffected (its injected values are already SAMPLED). - The rejected-header breadcrumb now goes through sanitizeLogText so a crafted traceparent cannot forge log line structure with control characters. - Add the sdk-impl wiring test: after initializeTelemetry the injected W3C fallback propagator resolves inbound HTTP parents. * fix(telemetry): align log-path traceparent parsing and emit traceId in both modes - extractInboundTraceId now mirrors the vendored W3C propagator's acceptance exactly: single optional leading/trailing whitespace and trailing extension fields above version 00 (version 00 must stay four fields). Previously the strict four-field anchor made the two paths disagree on the same forward-compatible header, silently dropping the access-log traceId for exactly the callers the propagator path supports. - The camelCase traceId access-log field is now captured whenever a valid header parses, regardless of telemetry mode, so one saved log query / alert shape works for every deployment; with telemetry on the snake_case span prefix carries the same id redundantly. * fix(telemetry): move inbound trace id getter out of the middleware module 52d572c0f2 made the access log statically import the telemetry middleware module to read the captured inbound trace id. The access log sits inside the serve fast-path pre-listen closure (run-qwen-serve imports it directly), so the middleware's core-barrel import graph came along for the ride and check-serve-fast-path-bundle started failing: the 5.6MB core chunk (shell tool, glob, chokidar, @iarna/toml, fzf) became statically reachable from run-qwen-serve. Move the response-context symbol, its type, and the getDaemonTelemetryInboundTraceId getter into a new import-light telemetry-context.ts; the middleware imports the symbol from there and re-exports the getter, so the access log no longer links against the telemetry module at all. * fix(telemetry): capture inbound trace id pre-auth under a dedicated symbol * test(telemetry): pin the trace id seam through the context module getter --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com>	2026-08-19 12:21:49 +00:00
callmeYe	e4f5504e9f	feat(extensions): support authenticated HTTPS Git installs (#9458) ... * feat(extensions): support authenticated HTTPS Git installs * test(serve): update capability integration baseline	2026-08-19 09:06:54 +00:00
jinye	577f719130	fix(cli): surface daemon duplicate tool-call breaker as loop-detected stop (#9435) ... The ACP daemon Session was the only duplicate-provider-id circuit breaker path (PR #5657) that terminated silently: the turn ended as a normal end_turn with nothing in the transcript and no telemetry, so the session looked hung. Route the breaker through recordDaemonLoopDetected with LoopType.GLOBAL_TOOL_CALL_DUPLICATE — the same loop type the non-interactive CLI reports — so foreground turns fail with the visible LOOP_DETECTED turn error, the context message is preserved for the next turn, and the LoopDetectedEvent telemetry is emitted. The bespoke repeatedDuplicateProviderToolCall result flag and its dead consumer branch are removed in favor of the existing loopDetected plumbing.	2026-08-19 07:14:27 +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
jinye	83fc634f61	feat(serve): measure ACP child peak old-generation heap (#9380) ... * feat(serve): measure ACP child peak old-generation heap Co-authored-by: Cursor <cursoragent@cursor.com> * fix(cli): Omit unmeasured child heap reports and pin the GC observer path Self-review round 1 on the child heap measurement: - A child whose every getHeapSpaceStatistics() call throws (restricted container) reported a zeroed heap object with unclassifiedSpaceNames: [], which downstream reads as measured, needs-nothing, and coverage-complete. The probe now reports no heap at all until its first successful space read, matching what a child without the probe already sends. - The GC observer callback — the only writer of peakLiveSetBytes, majorGcCount, and majorGcMs — had no test delivering a gc entry, so a wrong detail.kind check or a callback that never runs stayed green. The observer is now injectable and a test pins the major/minor split. - The status/protocol/design text said the aggregate maximum "names the single worst-off child", but the aggregation takes Math.max per field independently. Wording now says each field is an independent maximum. --------- Co-authored-by: Cursor <cursoragent@cursor.com>	2026-08-19 05:00:07 +00:00
ytahdn	3192323c1d	perf(web-shell): keep streaming output responsive (#9405) ... * perf(web-shell): keep streaming output responsive * fix(web-shell): address streaming performance review --------- Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-19 03:54:00 +00:00
jinye	dd82ba404e	feat(serve): Add live-state session activity watermark (#9396) ... * docs(serve): Design live-state session activity timestamps Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(serve): Clarify live-state timestamp semantics Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(serve): Add live-state session activity watermark Advance a bridge-local per-session activity watermark once when a prompt that reached the running state publishes its formal terminal, project it as the existing optional BridgeSessionSummary.updatedAt, and expose it on the workspace live-state route. The advance is written before the terminal is published so a client that observes the terminal cannot read a stale value, and the extra millisecond keeps the watermark strictly increasing when several terminals share a wall-clock millisecond or the clock moves backward. A queued-only terminal, heartbeat, attach/detach, or streamed update never advances it, and turn activity does not change the session catalog version. Populating the already-typed summary field lets full workspace session lists merge live and persisted timestamps. Because the mtime and the running-turn watermark are different authorities and the recorder writes asynchronously, the merge picks the later valid timestamp instead of blindly preferring the live value, so a row cannot move backward when an async transcript write lands after the terminal. Extend the response schema documentation for the live-state route and GET /session/:id/status, and add the optional field on the TypeScript SDK DaemonSessionLiveState type so consumers can pre-flight the tag once and read the recency directly. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Apply the later-valid activity rule on Live Task read paths read_thread and wait_threads read the bridge summary directly, so once the summary began carrying the running-turn watermark their fallbacks stopped consulting the persisted transcript timestamp. Because the recorder writes asynchronously, one task could report a later recency from the thread list than from a thread read, and a wait cursor keyed on the live value alone stopped changing when only the transcript advanced, so a consumer waiting for that flush saw an unchanged cursor and exited early. Move the merge rule into a shared helper and apply it at all three read points, including the revision fallback used for a session with no attached client. Add the watermark cases the design doc enumerates but the previous commit did not ship: the deadline path publishes its terminal twice and must still advance exactly once, a corrected forward clock jump must never decrease the value, and a clock that advances between terminals must be reported instead of the logical tie-breaker. Cover the single-session status route's verbatim pass-through of the field, and cover the helper's both-invalid tail directly because no route can supply two invalid candidates. Correct two design-doc test-plan claims that did not match the code: the teardown paths advance a watermark no consumer can read, because the entry leaves live state in the same operation, and the duplicate deadline terminal comes from the raced rejection reaching the settle handler rather than from a late agent result. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Merge live state on every organized session page The organized view applied the live merge only on the first page, so page 1 sorted rows and encoded its cursor from merged activity keys while later pages keyed the same rows by persisted mtime alone. That was harmless while bridge summaries never carried an activity timestamp, because both keys were the mtime. Now that a settled turn advances a watermark that leads storage until the recorder flushes, a live row ordered onto page 1 by its watermark falls behind the page-1 cursor boundary on page 2 and is returned a second time, displacing a genuinely new row. Merge live state on every page so both pages key rows the same way; a live-only row still has no persisted key to page by and stays a first-page insertion. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(serve): Document activity-cursor duplication on live retirement The activity key merges the live watermark, which is in-memory only, so a session whose live entry retires mid-pagination falls back to its transcript mtime and can be admitted again by a cursor encoded from the higher watermark. Pre-change keys came from mtime alone and only advanced, so pages could skip a row but never repeat one. Name that mode in the design doc and warn SDK consumers of the activity-ordered cursors to key accumulated pages by sessionId. * fix(serve): Exclude emitted identities from activity-cursor re-admission An activity key merges the bridge's in-memory watermark, so it is not a stable property of a row: when a live entry retires mid-pagination the key regresses to the transcript mtime, and a live-only row that persists mid-pass re-enters the scan keyed by its first flush. Either way a row already emitted on an earlier page could pass the strictly-older cursor filter again and displace a genuinely new row, which was structurally impossible while activity keys came from mtime alone. The organized and metadata activity cursors now carry the identities already emitted at a live-derived key, and the after-cursor filter excludes them, so one pass returns a session at most once. The list prunes itself: an identity is dropped once its persisted floor alone can no longer pass the key filter or once the row leaves the filtered collection while not live. Past a 64-identity cap the highest floors are dropped first, degrading to the previous at-most- once duplicate instead of failing the pass. Cursors minted before the field existed stay valid, and the field is omitted when empty. * fix(serve): Close carried-identity drop paths in activity-cursor pagination The emitted-identity carry could still drop a carried session mid-pass and re-admit it later: an identity absent from a page's collection was discarded even though absence can be transient (pre-flush TTL cache, mid-pass group movement), organized re-entry was evaluated under the row's current pin state only, and the live-only cursor key could move backward when a wall-clock rollback landed the first watermark behind createdAt. Retain absent carried identities at a negative-infinity floor, test organized re-entry under both pin states, and floor the first watermark advance at the entry's createdAt. Extend the retire test to a three-page pass so carried-set propagation through an intermediate cursor is pinned, probe scan visibility in the mid-pass-flush tests, and cover the live-list-failure and unpin paths. Scope the at-most-once pagination wording in the design and protocol docs to what the carry actually guarantees. * docs(design): floor the first watermark advance at createdAt in the normative formula --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-19 03:31:44 +00:00
Shaojin Wen	846fc05461	feat(ci): post autofix failure-path handoff comments bilingually (#9386) ... * docs(autofix): design bilingual failure-path handoff comments Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(ci): post autofix failure-path handoff comments bilingually Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): pin bilingual handoff sanitization by content, escape withdraw excerpt Address review R1-1/R1-2/R1-3: - Escape + iconv the issue-lane withdraw failure.md excerpt, the one publish site without `<!--` escaping: a failure.md quoting an HTML comment whose closer sits past the 1500-byte cut opened an unterminated comment that swallowed the new 中文说明 block (R1-3). - Widen the escape-site count test to the multi-`-e` sed form (9 -> 12 sites) and pin the full zh sanitization pipeline per-site on both lanes; dropping the `<!--` expression from either zh site or a tag substitution from the withdraw site now fails (R1-1). - Pin EN/ZH correspondence for every non-empty assignment site of HEADLINE/CAUSE/LAST_FIX/GATE_CLAUSE/IDLE_CLAUSE/REMEDY: count-only pins let a swapped adjacent HEADLINE_ZH pair pass all tests (R1-2). All four mutation witnesses from the review now fail the suite (verified locally: probe each mutation, expect red, restore). * fix(ci): close the bilingual handoff review gaps (R2/R3) Workflow fixes: - Neutralize :: in the issue-lane run-log dump loop (agent-written files on step stdout parse as workflow commands; the PR-lane twin already did this) — R2-1. - Extend the wrapper-defense substitutions (<details, </details, <summary) to the three excerpt sites that only escaped <!-- and now sit above the new 中文说明 wrapper: API_ERROR_DETAIL (flows into HEADLINE_ZH inside the wrapper) — R2-3; the PR-lane DETAIL_FILE excerpt (address-summary/no-action files are mandated to END with their own <details> tail, so a cut-straddling tail leaves a live severed opener) — R2-4; the withdraw failure.md excerpt — R3-1. - The withdraw comment's 中文说明 block now renders unconditionally with a translated REASON (REASON_ZH per branch), mirroring the PR-lane headline floor: crash shapes where run-agent.mjs writes failure.md itself no longer degrade to zero Chinese — R3-2. Accepted and documented (design doc §5): fence-token severance across the byte cut — render-only, markers parse raw, and a balancing heuristic stays wrong when the cut lands mid-closer — R2-2. Test pins (each mutation-verified locally): branch-selected zh labels — R2-5; zh gate-note text + condition + position — R2-6; failure.zh.md membership in all four dump loops plus the issue-lane :: sed — R2-8; the ZH_DETAIL guard — R2-9; full-line rm -f pins on the three pre-agent cleanup sites — R2-10; the BODY append shape — R3-3; wrapper internal ordering — R3-4. Design doc §2 reconciled with §5 on the no-detail fallback sentence — R2-7. * fix(ci): close the R4 review gaps (case-insensitive tag defense, pin gaps) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-19 00:17:35 +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
ytahdn	0c35b304a3	fix(web-shell): use backend-authoritative queue state (#9407) ... Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-18 12:07:55 +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
易良	179c8f80fd	fix(memory): improve recall reliability and candidate coverage (#8716) ... * fix(memory): improve recall delivery and multilingual fallback * fix(memory): bound heuristic recall scoring * test(memory): pin initial recall budget with fake timers Rewrite the slow-recall test to assert with fake timers that the main request is still held 1 ms inside the 100 ms initial budget and proceeds without memory at expiry, so budget changes can no longer pass unnoticed. * test(memory): pin recall budget and scoring contracts Address review findings with mutation-verified pins: - settle-early: bounded wait ends when recall settles, not at full budget - Cron and ToolResult consume points stay zero-wait - post-wait replacement guard refuses stale handles - type boost flips the winner (tie-break no longer masks its removal) - hiragana-only coverage for the CJK tokenizer - design doc: RFC #7040 sets no numeric overhead target; fix attribution * fix(memory): preserve recall field weighting * fix(memory): recall relevant topics beyond scan cap (#8803) * fix(memory): bound recall candidates after full scan * test(memory): pin bounded selector inputs * fix(memory): preserve bounded recall candidates * fix(memory): preserve lexical recall candidates * fix(memory): prioritize lexical model candidates * fix(core): preserve UTF-16 manifest boundaries * fix(memory): address recall review feedback * test(memory): measure recall rollout gate against the pre-change scorer RFC #7040 gates the multilingual precision change on evidence that English Recall@5 and no-result precision do not regress. Add a labeled 45-case corpus and an evaluation harness that scores both the shipped deterministic selector and a frozen copy of the pre-change scorer over it, so the gate is reproducible rather than asserted. * fix(memory): deliver a deterministic fast recall result on the initial turn The initial-turn budget is 100 ms, but recall awaits the model selector, which is a network side query with a 30 s ceiling. The budget therefore expires on the common path and delivery falls through to the ToolResult point — which a tool-free turn never reaches, so the result is discarded as no_safe_delivery_point. That is the case memory matters most for. Publish the deterministic candidates that selectModelCandidateDocuments already computes, before blocking on the selector, and inject them when the budget expires. The refined result still lands at ToolResult, with documents the fast phase already delivered filtered out. phase telemetry now carries both stages: phase is the delivery stage, strategy is the selection method, and they are orthogonal. * docs(memory): record the fast-path decision and phase/strategy split * test(memory): report the mixed-language slice in the rollout gate * docs(memory): align recall docs on the deterministic fast path memory-system.md documented recall selection but never documented delivery, so the delivery telemetry from #7393 was undocumented and the fast path had no home in the canonical reference. Add a delivery section and the delivery event table, and correct two docs that still described the single-path behaviour. * fix(memory): use Array<T> for the fast-path test doc lists @typescript-eslint/array-type forbids T[] for non-simple types. * docs(memory): clarify recall delivery telemetry * fix(memory): report already-delivered recall count * docs(memory): align recall delivery claims * fix(memory): rank ties by recency and record fast-delivered discards Three review follow-ups on the recall reliability change. Tie-break: `selectRelevantAutoMemoryDocuments` broke score ties with `type.localeCompare`, which orders feedback < project < reference < user. That was tolerable while the result was five documents wide; the fast path takes only MAX_FAST_RECALL_DOCS = 2, so a tied user-typed document was dropped every time — the exact memory a tool-free turn exists to surface. Ties now fall to recency, then to input order, which keeps the project-before-user precedence the concatenation already establishes. Corpus: the case labeled `semantic-no-lexical` had no relevant documents, so it was a no-result case wearing the wrong label and nothing measured the cost of "no lexical match, no score". Relabel it and add three genuine answerable-but-lexically-disjoint cases. Both scorers return nothing for them, so the slice sits outside the quality floor and is asserted separately: the fast path closes the timing gap, not the matching gap. Tool-free delivery is 92.3%, not 100%, and the residual is that slice. Telemetry: a tool-free turn logs its terminal event from the discard path, which did not apply the fast-phase exclusion. A turn whose every selected document had already been fast-delivered was recorded as `no_safe_delivery_point`, inflating the "memory never reached the model" bucket with turns that got it. Apply the same rule the ToolResult consume point uses; a partial overlap still reports the cancellation reason. * docs(memory): state the candidate-cap trade and the per-turn document count Two review follow-ups, documentation only. No behaviour change. "Removes the 200-document cap" oversold the candidate change. What it does is swap a per-scope, query-blind recency truncation for a global, query-aware one, and the effect is not a uniform widening: at or under 200 documents nothing was excluded by count under either design, but the new 25,000-byte manifest budget is a ceiling the old path lacked; between 200 and 400 with neither scope over 200 the old path sent every document and the new one sends at most 200, so fewer reach the model; only a scope over 200 is the case the change is actually for. Record all three, plus the fact that the manifest budget packs rather than prefixes. MAX_RELEVANT_DOCS = 5 bounds one prompt, not one turn. A fast delivery of two plus a refined delivery of five disjoint documents puts seven in front of the model; dedupe removes repeats, not the sum. This follows from dropping combined fast/refined budget accounting, which was a deliberate choice, but the number was never written down next to the constant that reads like a hard cap. * fix(memory): end the initial recall wait on the fast result, widen tokenization The 100 ms initial budget was a fixed cost, and the evidence for it measured the wrong thing. Deterministic *scoring* is microseconds, but the fast result is only published once recall has enumerated, read, and parsed the memory tree — and this branch removed the 200-document cap for recall, so that scan grows with the tree. recall-scan-latency.test.ts adds that measurement against a real temporary tree: ~29 ms at 200 topics, ~70 ms at 500, ~130 ms at 1000. So for any tree small enough to scan in time — the ordinary case — the fast result was in hand tens of milliseconds before the budget expired, and the rest of the budget was spent waiting on a model selector this design already assumes will miss it. The wait now ends on whichever comes first: recall settling, the fast result being published, cancellation, or the ceiling. The preference order is unchanged, because the code after the wait still prefers a settled recall. Past roughly a thousand topics the scan alone exceeds the ceiling and the turn pays the full budget for nothing; that is recorded as a known limitation rather than fixed, since the fix is a persistent catalog. Tokenization kept only [a-z0-9]{3,} runs, so Cyrillic, Greek, Arabic, and accented Latin produced no tokens at all and the deterministic path was unconditionally silent for them. Keep whole runs of non-CJK letters, marks, and digits instead. CJK is excluded per character rather than by alternation order: \p{L} also matches Han, so a Latin-initial run would otherwise swallow the CJK after it and turn abc漢字 into one token. Scripts without word separators outside the CJK set still collapse to one run, which is recorded rather than claimed as segmentation. Two smaller follow-ups. The active-tool alias set is now derived once per recall instead of once per scanned document, which mattered little under the old 200-document cap and more without it. And the eval prints the Recall@5 a query-blind random scorer would score on this corpus (20%), with a test holding that floor at or below 25%, because a small corpus flatters every design and the headline was unreadable without it. * docs(memory): correct the initial-turn preference claim, pin it with a test Local end-to-end verification on #8716 found the claim added in 01ef7d7d — "the preference order is unchanged: whatever ends the wait, a settled recall is still delivered in preference to the fast result" — to be false in the case that matters. `onFastResult` is published before recall issues the selector request at all, so the recall promise cannot be settled when the wait ends on the fast result. Measured against a selector settling in 15 ms, comfortably inside the ceiling, the initial turn still delivers the deterministic pair and discards the model's picks. The behaviour is right and stays: a model side query does not return inside a 100 ms ceiling in production, so arbitrating would spend the rest of the budget on every turn to win a race that does not happen, and the selector's judgement still lands at ToolResult with the fast documents excluded. What was wrong was the description. State it directly instead — on the initial turn, once the deterministic scorer matches, the fast result wins regardless of selector latency — and pin it with a test that fails when the early exit is removed, so it reads as a decision rather than an accident. Two measurements corrected while here. The scan crossover is machine- dependent, not a fixed topic count: the same three sizes measure 9/21/46 ms on faster hardware against 29/70/130 ms on the machine the tables were written from, so the ceiling is not reached there at all. And MAX_MODEL_CANDIDATE_DOCS = 200 is rarely the binding constraint — MAX_MODEL_MANIFEST_BYTES is, at roughly 90-150 documents once absolute paths and timestamps are counted. Measured runs sent 94 and 96 manifest lines where the document cap would have allowed 200, which also explains why the recency reserve has to be interleaved rather than appended. --------- Co-authored-by: yiliang114 <jinjing.zzj@gmail.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-18 07:50:26 +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
ytahdn	721e435f7e	feat(web-shell): support upload directory and hard-disable drag-in when fileUploadEnabled=false (#9382) ... fileUploadEnabled={false} previously still admitted file drags onto the inline image/text lane, producing attachment tags with no upload. It now disables file drag-and-drop entirely — no drag highlight, no drop ingestion, no upload — while clipboard paste stays enabled. The composer core gates its drag/drop handlers behind a new fileDragEnabled option; ChatEditor cancels the drop outright. A new fileUploadDirectory prop (relative to the workspace root) sets the drag-upload target directory; the root remains the default. The daemon upload route now materializes a missing target directory (recursively, depth-capped) via a new WorkspaceFileSystem.mkdir, so a configured drop folder needs no manual setup. mkdir follows the existing write-path safeguards: trust gate, generation guard, path lock, audit, and symlink-swap re-checks per created component and its parent. Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-18 05:04:52 +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
ytahdn	789a6691ed	revert(web-shell): restore pre-#8098 composer animations at 50% opacity (#9349) ... * Revert "fix(web-shell): stabilize mobile composer after resume (#8263)" This reverts commit d213c8519a. # Conflicts: # packages/web-shell/client/e2e/web-shell.composer.mobile.spec.ts * Revert "feat(web-shell): enhance composer and empty-session animations (#8098)" This reverts commit e379ea4e53. * fix(web-shell): drop #8601 mobile-composer overrides superseded by the animation revert The #8098 animation revert removes the position: relative / z-index: 1 on .appChatEmpty .chatViewWrap, so #8601's position: static override (a fix for the containing-block shift that #8098 introduced) is a no-op and its comment describes a dot field that no longer exists. Sync the e2e layout assertions to the restored pre-#8098 values (z-index auto, static wrap at 761px) while keeping the footer-anchoring contract they verify. * style(web-shell): cap the dac composer glow at 50% opacity Design feedback: the restored composer glow reads too bright. Halve the opacity ceiling for the aura grid, the halo and the pulse glow while keeping the dac-aura-cols dance animation and the focus/pulse dynamics intact. * fix(web-shell): keep the custom-footer wrap positioned for the bottom panels Dropping the #8601 override removed the positioned containing block for the bottom panels (status panel) in the custom-footer mobile welcome variant: that variant renders the composer footer display: contents, so its wrap is the only positioned ancestor the absolutely-positioned bottom panels can anchor to. Restore position: relative on just that variant instead of the full-positioned wrap the deleted rule had excluded, keeping the plain variants static so the footer keeps anchoring to the chat pane. * fix(web-shell): address review feedback on the animation revert - Restore disabled?: boolean on the ChatEditor test render props: two surviving call sites still pass disabled: true, and dropping the member leaves object-literal excess-property violations (latent only while test files stay out of tsc). - Update the stale chatViewWithCustomFooter marker comment in App.tsx: the :not(...) exclusion it described was removed, and the class now completes the compound selector that keeps the custom-footer wrap positioned for the bottom panels. - Document the once-only glow effect invariant and the legacy data-dac-glow marker, both restored verbatim from the pre-#8098 implementation. --------- Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-18 03:22:29 +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
jinye	f16975ee56	feat(serve): Add workspace session live-state endpoint and catalog version (#9261) ... * docs(serve): Design workspace session live-state protocol Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(serve): Refine workspace session live-state design Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * feat(serve): Add workspace session live-state endpoint and catalog version Add GET /workspaces/:workspace/sessions/live-state: a memory-only volatile snapshot (clientCount, hasActivePrompt, waiting flags) plus an in-memory catalog version (generation+revision equality token), so clients stop polling the persisted catalog for volatile state. The bridge owns the clock: registration/removal marks flow through the emitSessionLifecycle choke point; rename, automatic title, worktree, and persisted branch commits mark at exact points; serve-layer REST/ACP mutations share an invalidate-then-mark helper with exact no-op semantics (deleted:false group deletes, removeSession:false cleanups, no-op renames). The route exposes a new version only after invalidating both persisted catalog scopes, enabling the client live-A -> full catalog -> live-B reconciliation handshake. Wire-additive: new unconditional capability workspace_session_live_state, TypeScript SDK types and DaemonClient/WorkspaceDaemonClient methods (native REST, no per-poll capability preflight), telemetry label, and protocol/capability/SDK docs. Required clock methods on AcpSessionBridge are a source-level contract change for external structural implementations; in-repo fakes updated. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(serve): Cover workspace_session_live_state in the serve integration baseline The capabilities envelope E2E asserts the exact advertised feature list; the new unconditional live-state capability must appear after the archived-export tag, matching registry declaration order. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * docs(serve): Close the handshake-vs-single-request argument gap Separate the two claims the earlier paragraph conflated: the catalog path cost is irrelevant to carrying a version (it runs anyway on a full reload), but stamp placement decides consistency. Stamp-after-scan can silently accept a bundle missing a mid-scan mutation; stamp-first is safe and self-heals within one poll cycle, which a client may legitimately choose. The A/B handshake buys provable consistency for one extra cheap live-state read; the server supports both and the Web Shell PR picks per product tolerance. * fix(serve): Mark catalog version on persisted session renames The metadata route's SessionNotFoundError fallback renamed persisted sessions without advancing the catalog revision, so version-watching clients kept the stale display name. Mark after a successful persisted rename (parity with the live path, which marks on an actual change). Also reconcile the design doc summary with its Implementation Boundaries (the implementation ships in this PR, not a follow-up) and spell out the child-recording persistence mechanism behind the auto-title catalog mark. * test(serve): Pin catalog-mark and live-state behaviors from the review round - Assert markSessionCatalogChanged in the scheduled-task rollback (including the no-op-removal negative case), the sub-session and Live coordinator rollback paths, and the never-live orphan deletion; previously each mark could regress with suites green. - Cover the live-state route's ?? false projection for both wait flags, and its first-exposure invalidation arm (revision unchanged, both organized scopes refilled). - Cover the side-task generation-closed rollback arm (kill, remove, catalog mark). - Compile the SDK live-state type fence via tsconfig.test-fence.json so shape assertions really pin the wire contract; the default tsconfig excludes test/. - Align the design doc's cache-consistency goal with the cache mechanics (waiters joined before an invalidation may resolve, but cannot install). --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-17 15:01:50 +00:00
jinye	2858538fb2	feat(core): Add privacy-safe tool-result boundary diagnostics (#9039) ... * feat(core): add privacy-safe tool result diagnostics Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9039) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-17 09:12:38 +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
ytahdn	c0791f0450	feat: support session media references end-to-end (#9310) ... * feat: support session media references end-to-end * fix(cli): catalog session media routes and update mid-turn replay meta test The three new media routes (POST/GET/DELETE /session/:id/media[/:mediaId]) were registered but missing from legacySessionTelemetryRoutes, tripping the route drift guard; add them as handler_resolved like their sibling routes. The mid-turn history-replay expectation now carries the replay meta this PR adds (source: mid_turn_message_injected, qwenDiscreteMessage: true). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): harden session media per review feedback Addresses the Critical review findings on the session-media PR: - Reject image/svg+xml uploads and serve stored media with Content-Disposition: attachment and X-Content-Type-Options: nosniff (same-origin XSS vector on the daemon/Web Shell origin). - Keep the retained-media TTL sweep running when the session reaper is disabled (sessionReapIntervalMs <= 0) on the default 60s cadence. - Record only the inline bytes the media references actually cover: the gate now counts image blocks only (references are image-only), and the strip keeps unrelated inline parts (e.g. @-mentioned files). - Show '[User message with attachments]' on TUI resume for image-only mid-turn messages recorded with an empty displayText. - Exempt mid-turn injected echoes from the Web Shell status-noise and plan-JSON filters. - Degrade refresh-rebuilt queue rows to summary-only when media hydration failed, so editing cannot silently discard attachments. - Retry cross-session media removal without the clientId when the daemon rejects the stale persisted id (invalid_client_id). - Register session_media in the integration capabilities baseline. Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): bound media content and fix resume/echo edge cases Addresses the remaining Critical review findings on the session-media PR: - Cap media content blocks at 256 on the mid-turn and prompt routes and resolve each distinct mediaId once per resolveContent call — an unbounded array of duplicate references amplified one small request into gigabytes of heap at dispatch. - Record a '[User message with attachments]' placeholder for inline-media-only mid-turn messages with no references, keeping '' only for the reference shape that replay projects. - Restore the same placeholder for image-only ordinary prompts on TUI resume instead of dropping the message from the restored history. - Treat a mid-turn injected echo as renderable when its items carry a non-empty text block, so the degraded-media echo (messages: [''] plus the placeholder text block) is not discarded as malformed. - Release session media in killSession's force-kill and closing-session fallback branches instead of degrading to the crash-path detach retention. - Remove the unreachable duplicate return in DaemonSessionClient.load(). Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): resolve round-4 session media findings Addresses the round-4 Critical review findings on the session-media PR: - Move the mid-turn media-reference validation below the idempotent retry-ack rings so a same-id retry whose media was already removed (delete racing an in-flight POST, or a refresh re-enqueueing from the snapshot) settles idempotently instead of failing with session_media_gone (410). - Keep image/* (unknown mime type) prompt images inline instead of uploading them: the media route matches concrete image types only, so the upload POST 400s and the whole submission hard-failed, regressing pre-upload behavior for untyped images. - Project the degraded-media drain echo's placeholder text block when the echo text is empty, so the Web Shell shows the unavailability notice instead of rendering an empty bubble. Each fix carries a regression test that fails on the pre-fix code. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): resolve round-5 session-media critical findings (#9127) - Stop deleting a session-media blob when one queued prompt / mid-turn message referencing it is removed: the store has no reference counting, so siblings, replay metadata, or other clients may still hold the same mediaId. Blobs now live until session close / TTL sweep or an explicit removeSessionMedia. - Reject duplicate mediaId occurrences in one message at assertReferences (covers the prompt and mid-turn admission paths); the serializer expands every reference at dispatch, so repeats amplified one upload into an unbounded payload even though only one read is needed. - Align the mid-turn display-text and reference-persistence gates: compute the same willPersistReferences condition before finalizing displayText, so a partially-referenced message records the attachments placeholder instead of an empty displayText with no references. - Keep the webui mid_turn_message_injected sidechannel alive for degraded image-only echoes whose items carry only the placeholder text block, mirroring the SDK normalizer's hasRenderableItemContent. * fix(daemon): resolve round-6 session-media critical findings (#9127) * fix(daemon): resolve round-7 session-media critical findings (#9127) * fix(daemon): resolve round-8 session-media critical findings (#9127) * fix(daemon): correct session media recovery and queue isolation * fix(daemon): remove media with deleted queue items * fix(daemon): bound repeated media in queue drains --------- Co-authored-by: 钉萁 <dingqi.jww@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-17 07:59:41 +00:00
ytahdn	addde7c1d0	feat(web-shell): decouple composer from catch-up and rebuild SSE on disconnected submit (#9323) ... Catch-up (SSE history replay) no longer disables the composer input or swaps its placeholder to "加载中": replaying history does not conflict with typing, and a stuck catch-up must never lock the input. A prompt submitted while the SSE stream is down is no longer blocked with a toast: admission rebuilds the stream immediately by aborting the reconnect backoff and resuming via Last-Event-ID, keeping the session handle intact. Drops the now-unreachable 'loading' placeholder state from the public WebShellComposerPlaceholderState type and syncs docs (README, design doc, provider and entry type comments). Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.com>	2026-08-17 06:39:23 +00:00
ytahdn	9a5b07c0b1	feat(web-shell): improve sidebar session management (#9311) ... * feat(web-shell): improve sidebar session management * feat(web-shell): keep sessions accessible when sidebar is collapsed * test(web-shell): cover hover session details * test(web-shell): align workspace sidebar coverage * fix(web-shell): keep collapsed session actions open * fix(web-shell): layer collapsed session menus * fix(web-shell): address sidebar session details review findings (#9122) - sdk: route workspace session metadata PATCH through direct REST - collapsed switcher: cancel stale hover-close timers on reopen, keep the surface open while the group picker or keyboard focus is inside it, and emit the missing close signal when a tracked menu unmounts open - suppress the session menu's close focus restore when it started a rename - migrate the primary workspace expansion preference across the provisional-to-resolved cwd key change - honor a persisted workspace collapse over stale one-shot auto-expansion - drop the inert archived-row tab stop - align the constrained smoke test with the single-line details title * fix(web-shell): resolve standing sidebar session details blockers (#9122) - collapsed switcher: resolve pointer targets through composedPath and make the close timer's focus guard shadow-DOM aware so hover-open containment works in shadowDom portal mode - reset search state when the sidebar collapses so the autofocused search input no longer mounts inside the hover popover and steals keyboard focus - rename: propagate the daemon-resolved displayName (clamped to 256) instead of the locally typed string and cap both rename inputs at 256 characters - keep the session list scrollable clear of the fixed footer so rows stay hoverable, and close the details popover before each constrained re-hover in the smoke test - projects section: write the expansion preference outside the state updater, never lock hideProjectHeader consumers behind a stored collapse, and reset the one-shot show-all per session source and primary workspace - stop a double-click inside a mounted rename input from restarting the rename and discarding the typed text * fix: address round-6 review findings in serve metadata and sidebar (#9122) - serve: reject empty/whitespace displayName on the workspace metadata route so archived sessions never persist an empty custom_title record - serve: advertise workspace_session_metadata in the integration capability baseline to match the registry and unit baselines - sidebar: end the session scroll port above the fixed footer so rows can never park under it and block hover (drops stale clearances) - sidebar: reset search state whenever the collapsed surface closes so a stale autofocused input cannot steal composer focus on hover-open - sidebar: keep keyboard-opened collapsed switcher in keyboard semantics; a pointer graze no longer suppresses focus restoration - sidebar: busy-guard the archived rename menu item, align the group-create icon with its siblings, and reset per-section show-all on session-source change to match the flat list Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix: address round-7 review findings in serve metadata and sidebar (#9122) * fix: address round-8 review findings in sidebar rename and switcher (#9122) * fix: address round-9 review findings in sidebar rename and menus (#9122) * fix: address round-10 review findings in sidebar actions and rename (#9122) * fix(web-shell): remove unsafe rename unmount cleanup * fix(web-shell): stabilize sidebar session mutations * fix(web-shell): polish sidebar session interactions * feat(web-shell): complete collapsed sidebar navigation --------- Co-authored-by: 钉萁 <dingqi.jww@alibaba-inc.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 06:06:56 +00:00
samuelhsin	5abd367f47	feat(daemon): attach skill-toggle mutation metadata to settings_changed (#9051) ... * feat(daemon): attach skill-toggle mutation metadata to settings_changed Hosts can apply Skill toggles incrementally without a full task reload or suppressing skills.* events. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(review): fit skill-toggle mutation metadata in the SDK bundle budget The new normalizer parser pushed the browser daemon bundle over the 186KB cap. Raise it to 187KB and pin the review gaps that were cheap to close. Co-authored-by: Cursor <cursoragent@cursor.com> * test(daemon): pin skill-toggle mutation event count and parser edges Co-authored-by: Cursor <cursoragent@cursor.com> * fix(sdk): raise daemon browser bundle budget for skill-toggle metadata The 190KB cap overflowed by 491 bytes after merging main, so the SDK build fails before tests run. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>	2026-08-17 05:01:41 +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	90c166585a	feat(release): user-facing bilingual digest for release notes (#9216) ... * feat(release): user-facing bilingual digest for release notes Stable release notes read as a type-bucketed PR list, which users find hard to scan. The finalize step now asks the model to group changes into user-facing themes with short intros, mirrors highlights and themes into a Chinese digest, attaches screenshots found in merged PR bodies (host allowlist, per-release cap), and collapses the full PR list into an appendix with normalized titles. Every model failure path keeps today's v1 output byte-for-byte, and CHANGELOG.md accepts the new v2 marker. * fix(release): tighten v2 digest fallbacks and changelog skeleton (#9216) Address review round 1 findings: - usedAi only counts themes that carry content, so a release whose digest has zero model text is no longer reported as AI-generated - hasChinese is derived from what the Chinese block actually renders, not raw model output, so zh-only-on-breaking releases no longer emit an empty or English-only section - a PR repeated inside one theme is deduped instead of discarding the whole themes digest with a misleading cross-theme error - fallback titles in the v2 digest are normalized like the appendix, killing the mixed-style look in the degradation case - normalizeAppendixTitle strips only the conventional types the changelog's formatEntry strips, keeping ci/test/security prefixes - the changelog unwraps the v2 appendix at the same sibling rank as v1's Complete Change List instead of nesting it under the previous section - drop a dead summaries max_tokens scaling term and a verbatim copy of renderChangeLine's attribution rendering * fix(release): close digest image breakout and tighten fallback signals (#9216) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(release): drop camo proxy and harden digest text validation (#9216) * fix(release): neutralize markdown breakouts in digest text and images (#9216) * fix(release): close classification, image-URL, and text-validation bypasses (#9216) --------- 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-17 00:12:04 +00:00
jinye	889f0d8bbd	feat(daemon): Isolate the Conversations runtime boundary (#9181) ... * feat(daemon): isolate the Conversations runtime boundary Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: fix CI failure on PR #9181 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: fix CI failure on PR #9181 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9181) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 17:32:45 +00:00
jinye	c48809341b	feat(external-context): Add provider extension profile (#9068) ... * feat(external-context): Add provider extension profile Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Harden provider extension profile Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Harden provider profile bounds Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): validate provider profile boundaries Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): harden provider extension example Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(external-context): Use forward proxy for HTTP providers Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 15:50:45 +00:00
jinye	62014c0188	fix(serve): Bound ACP HTTP pre-attach buffers by bytes (#9007) ... * fix(acp): Account for JSON string escaping in response budgets Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Bound ACP HTTP pre-attach buffers by bytes Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Preserve ACP pre-attach stream scope Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Expose ACP guard failures by workspace Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Preserve ambiguous WebSocket deliveries Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(serve): Report failed ACP response delivery Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9007) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): make websocket teardown logging safe Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(serve): align ACP fork fixtures after rebase Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 11:59:23 +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
jinye	43b0779bcf	fix(telemetry): Address main agent tracing edge cases (#9121) ... * codex: address PR review feedback (#9107) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * codex: address PR review feedback (#9121) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 00:13:09 +00:00
Shaojin Wen	e93da9e387	feat(autofix): escalate stopped takeover PRs and age out unanswered pauses (#8960) ... * feat(autofix): escalate stopped takeover PRs and age out unanswered pauses Takeover PRs that hit the round cap (or a circuit breaker) went silent: no label, no dashboard entry, no escalation — five PRs had been paused for days. The fleet shepherd only tracked bot-authored PRs, so the whole 35-PR human takeover pool was invisible. The autofix scan now applies an autofix/needs-human label whenever a PR reaches its cap (the write rides every cap detection, so already-paused PRs backfill on the regular scan rotation), and removes it wherever management resumes or a human releases the PR. The fleet shepherd enumerates the takeover pool onto its dashboard (state, stop reason, pause age, plus an awaiting-human section for released PRs) and gains a single bounded lever: a takeover whose pause went unanswered for AUTO_RELEASE_DAYS days gets its takeover label removed with a bilingual summary, keeping the needs-human label as the filterable TODO. Resume evidence newer than the pause notice — bot markers, trusted re-arm commands, fresh labeled events — vetoes the release; every read fails closed and a per-tick cap bounds blast radius. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): harden the takeover auto-release against review round 1 Addresses the PR review's two Criticals and eleven Suggestions: - Command-comment resume evidence now counts only while FRESH (2h grace) and UNSUPERSEDED by a refusal ack (fork-refused/base-refused/ skip-blocked) — an ignored command expires instead of vetoing the release forever, and no permission logic is mirrored from the route. - The release lever's population comes from the needs-human enumeration (needs-human ∩ takeover), never the display window; both enumerations cap at 100 with saturation warnings, and a failed enumeration degrades to an error row so the dashboard write (and its liveness watermark) always runs. - The auto-release summary posts before the label DELETE, dedup'd by its own marker — neither half can strand the other on a transient failure. - Awaiting-human rows use neutral wording (capped bot PRs land there too) and a shepherd-side heal clears stale needs-human labels left by manual UI releases on fork PRs (human unlabeled event, budgeted, skip-vetoed). - Fail-closed deferrals now still render a dashboard row (the row append moved outside the evaluation arms); tick summary and dashboard header report the same counters; days_since() replaces pasted epoch math. - Tests: command-evidence gate replays (fresh/refused/expired/acked), refusal-variant and command-string cross-file pins, DELETE-target and fallback-assignment pins, heal jq replays, unified-row-render pin. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 2 — cycle-scoped heal, retryable summary - The stale-label heal now only counts a human unlabel NEWER than the latest label-apply, so an unlabel from an earlier takeover cycle can no longer heal the current cycle's needs-human after an auto-release (R2-1). - The summary dedup marker is scoped to the current pause cycle (markers older than the latest cap notice are ignored), so a re-armed and re-capped PR still gets its second release summary (R2-4). - The two DELETE levers no longer redirect act()'s stdout, keeping the DRY-RUN preview and failure warning visible (R2-5). - AUTO_RELEASE_DAYS is base-10 normalized after the numeric guard, so a zero-padded repo variable can't silently kill the lever (R2-6). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 2 — cycle-scoped markers and mutation-tested pins - Heal is cycle-correlated: only a human unlabel NEWER than the latest label-apply counts (an earlier cycle's unlabel can't heal this cycle). - The release summary dedup marker is scoped to the current pause cycle, so a re-armed and re-capped PR still gets its second summary. - act() stdout is no longer redirected on the two DELETE levers (DRY-RUN preview and failure warning stay visible). - AUTO_RELEASE_DAYS is base-10 normalized so a zero-padded repo variable cannot silently kill the lever. - Doc/workflow-header text corrected to the implemented order (summary first, marker-dedup'd) and to the idle-backoff backfill timing. - Mutation-tested test pins for every gap the reviewer probed: days_since replay, NH_PREFIX interpolation + truth map, loop-1 deferral, full cross-file marker/refusal-set equality, label-constant cross-pin, EVENT_TS merge + promotion ordering, CLEANUPS increment, unclassified headline classification, filter byte-identity, exit-spelling ban, @uri encoding, sort/field-list attribution, paginate shapes, scope --arg bindings, LIVE_LABELS_JSON wiring, positional append pin, label-create idempotence + POST guard, and per-branch removal attribution in the toggle replay. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 3 — release-ack label gate and shared classifiers - R3-1 (Critical): the every-scan cap-branch label POST is now suppressed when a release ack (takeover-ack released) is newer than the last re-arm, so a released bot PR is not re-labeled each scan (which would fight every release-side removal and ping-pong with the shepherd cleanup). A re-arm advances the window past the release ack, re-enabling the label. - R3-7: the /retry re-arm's needs-human removal now honors autofix/skip, mirroring the takeover-command guard — a frozen PR keeps its only filterable escalation state. - R3-2 (Critical): the takeover-enum error row no longer claims 'no release evaluation ran' — the lever is fed by the needs-human enumeration. - R3-8: the conflict-dispatch lever refuses a paused (needs-human) PR instead of spending a dispatch slot the scan would refuse. - R1-10: extracted pending_checks()/failed_test_url() helpers so both dashboard loops share one CI-status classifier (the round-1 reply was wrong that the restructure removed this duplication — it did not). - Hardened the mutation-tested pins: exact terminal-headline count (5), full rearm DELETE line + single-API-write, AUTO_RELEASE_DAYS guard order, runRearm env/stub/assertion for the /retry DELETE + skip guard, and the scope-guard comparison operator. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 4 — label-lifecycle hardening - R4-C1: the conflict-dispatch lever reads needs-human from the LIVE label payload (after live_skip), not the tick-start snapshot, so a label applied after enumeration is still honored. - R4-C2: a re-armed PR that still carries needs-human (a resume-side removal failed) now gets a bounded, skip-vetoed cleanup retry instead of staying pinned in the paused population forever. - R4-C3: the per-tick release budget is consumed before the first external write — a DELETE outage can no longer mutate many PRs while RELEASES=0. - R4-C4: dashboard row routing follows post-action label state — a released PR moves to Awaiting human, a healed one drops off entirely. - R4-C5: the AUTO_RELEASE_DAYS guard also rejects over-long digit strings before any arithmetic (Bash-int overflow would wrap negative and pass -ge). - R4-32: takeover-command stop only removes needs-human when the takeover release actually landed (REMOVED_OK; 404 counts) — a failed release no longer strands the escalation label while latching RELEASE_ACKED. - R4-2: the /retry skip guard fails closed — an unreadable label state keeps the label (mirrors takeover-ack's exit-1 convention). - R4-3: the takeover-ack released arm and the stop branch both honor autofix/skip when removing needs-human. - R4-S1: producer headlines must be explicitly classified terminal or transient — an unclassified headline now fails the cross-file test. - Pins updated/added for every behavior above. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 4 — robust release detection and marker-true gates - R4-1/R4-5: release detection now uses the takeover unlabeled EVENT (recorded on every removal path, unlike the tolerated-lost ack comment), and the suppression only applies to human-authored PRs — a bot PR released from takeover returns to standard management and keeps the cap notice + escalation label. - R4-6: the conflict-dispatch lever requires marker truth (conflict_paused) — an armed PR with a stale needs-human label is dispatched normally. - R4-C1: the pause check reads needs-human from the live label payload. - R4-C2: re-armed PRs with a stale label get a bounded cleanup retry. - R4-C3: the release budget is consumed before the first external write. - R4-C4: dashboard rows route on post-action label state. - R4-C5: AUTO_RELEASE_DAYS rejects over-long digit strings before arithmetic. - R4-32/R4-2/R4-3: stop/ack/retry removal paths gate on REMOVED_OK and skip. - R4-9/R4-10/R4-13: membership check, STATE escaping, HM_OK-branched error row. - R4-14: command evidence requires a write/maintain/admin commenter. - R4-11/R4-15/R4-24: behavioral replays for the classifiers, the release jq, and the gate nesting. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): keep the label-DELETE idiom byte-identical across workflows R4-32's REMOVED_OK tracking reworked the takeover-command stop branch's 404-tolerance block, breaking the pr-self-report-label ↔ qwen-autofix contract test that pins the two workflows' label-DELETE idiom byte-identical. Keep the canonical idiom and derive REMOVED_OK from REMOVE_ERR's content afterward (empty = landed, 404 = already off, anything else = release did not land) — same behavior, contract intact. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(autofix): close review round 4b — mutation-tested harness hardening - R4-4: re-bound the conflict-lever regex spans and anchor on conflict_paused so the pin can't resolve live_skip against the sync lever's call site. - R4-16: runAck records gh calls and asserts per-branch needs-human DELETE counts (engaged/released=1; base-refused/skip=0). - R4-17: pin the first-pickup scan DELETE inside the engage-ack success branch. - R4-18/R4-19: ordering pins — takeover POST before needs-human DELETE (engage), marker comment before cleanup DELETE (/retry). - R4-20: deleteFail stub branch replays non-404 (warns, status 0) and 404 (silent) DELETE outcomes. - R4-21: identity-failure paths assert no DELETE ran. - R4-22: runRearm stub serves labels only when --json labels is requested. - R4-23: full api-write census pinned (exactly api user + one DELETE). - R4-25: skip fixture uses the production multi-label shape. - R4-28: loop-2 fetch pins include the jq -s 'add // []' merge program. - R4-29: cmdGate scenario where a refusal is OLDER than the fresh command. - R4-30: takeoverEnum asserts its own sort:updated-asc qualifier. - R4-31: multi-entry fixtures pin the max/last/length aggregation operators on CMD_TS, EVENT_TS, REASON, SUMMARY_POSTED, and the heal lever's LATEST_LABEL_TS/UNLABEL_ACTOR programs. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 5 — trust boundaries and evidence freshness - R4-5 residual: PR_META now fetches author so IS_BOT_AUTHOR actually resolves (the exemption was dead on arrival), with a behavioral replay. - R5-1: REMOVED_OK derives from the captured stream ('HTTP ' non-404 = not landed) instead of output emptiness — GitHub returns a body on success. - R5-2: /retry only drops needs-human when management actually resumes (takeover label present or bot-authored) — an auto-released human PR keeps its escalation label. - R5-3: conflict_paused requires a real cap notice AND a newer resume marker — label-present/notice-absent now fails closed toward paused. - R5-4: a failed permission read defers the release (PERM_READ_FAILED), never counts as no-permission — at both evaluation points. - R5-5: compute_resume_ts scans in-grace commands newest-first and permission-checks each (≤2 reads), so a stranger's echo can't shadow a maintainer's command. - R5-6: the release branch re-fetches evidence and recomputes resume state immediately before the first write. - R5-8: the heal re-checks the takeover label from the live payload before clearing needs-human. - R5-9: same-second ties resolve toward resume/release suppression in both files (RESUME>=TERM; RELEASE_ACKED >= window). - R5-10: the heal anchors to the current pause boundary (latest needs-human apply event); an absent anchor skips the cleanup, fail closed. - Tests: whole-function compute_resume_ts replay (permission/shadow/tie/ grace/refusal cases), heal anchor fixtures, toggle stub models the real DELETE body, runRearm orphan case. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 6 — contract-safe gates and subshell flag fix - B5: the bot-fleet enumeration failure now degrades to a loud error row and falls through (FLEET_OK gate) instead of exiting before the independently-fed takeover/needs-human processing and the dashboard's liveness-watermark write. - B12: the cap-branch LIVE_LABELS consent re-read fails closed on an unreadable gh pr view (a collapse to '' ignored a concurrently added skip for standard bot PRs). - R6-1/R6-19: the takeover-release landed flag is keyed on the DELETE exit status (LBL_DEL_FAILED set inside the pinned idiom's failure branch) — never on output text, which lies in both directions. The pr-self-report-label idiom evolves identically to keep the cross-workflow contract green (and its own 'removed' log line no longer lies either). - R5-4 residual: compute_resume_ts now returns via globals (RESUME_OUT/PERM_READ_FAILED) and both call sites invoke it directly — the previous subshell silently dropped PERM_READ_FAILED, leaving the fail-closed defer branches dead. - R6-3: command candidates are deduped by author before permission reads, so a stranger posting N commands can't burn the 2-read budget and shadow a maintainer's command. - R6-4: an unreadable release history is reported as such, not as 'released'. * fix(autofix): close review round 7 — lever starvation, re-arm anchoring, permission shadows - R5-7: the release lever gets its OWN enumeration of the paused population (takeover+needs-human, stale-first) instead of the long-lived needs-human display window — released-awaiting PRs aging back into that window could truncate exactly the fresh pauses that become release-eligible, starving the lever and making the zombie state permanent and self-feeding. - R6-3: the 2-read permission budget now sets PERM_READ_FAILED on exhaustion (it was failing open), and the candidate walk sorts newest-first per author (group_by+max_by+sort) instead of unique_by's alphabetical order, so two read-only strangers can't shadow a maintainer's newer command. - R7-1: the stale-label cleanup anchors on the current pause boundary (latest needs-human apply) and is marker-confirmed only — not keyed on TERM_TS, and never on command/label evidence — so a re-paused PR with a lost cycle-2 notice isn't read as re-armed on stale cycle-1 evidence. - R7-7: the /takeover stop success echo is gated on REMOVED_OK — a failed DELETE no longer logs 'removed'. - R7-2: TAKEOVER_COMMAND/RETRY_COMMAND mirrored into the shepherd env and passed via --arg, so the resume matcher can't drift from the route. - Tests: conflict_paused + re-arm guard behavioral replays, mirrored-command cross-file pin, engaged/released-with-skip ack matrix cells, LBL_DEL_FAILED branching, gnuDateShim hoisted to module scope, R4-24 nesting indices. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(autofix): complete the R4-16 ack-matrix DELETE-count coverage Add fork-refused and skip-blocked ack cases to the takeover-ack harness — management never resumed on either, so zero needs-human DELETEs, each asserted by total DELETE count (not just toContain). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 8 — reachable re-arm cleanup, release race guards, pin census * fix(autofix): close review round 9 — honest release-failed ack, cleanup attempt budgets, dashboard single-owner routing - /takeover stop whose label DELETE failed no longer posts a 'Takeover released' ack: a release-failed variant names the retry (R9-4), and the R7-7 echo pair gains symmetric log pins (R9-3) - stale-label cleanups count ATTEMPTS like the release budget, so a DELETE outage trips the cap instead of leaving it inert (R9-5) - dashboard renders each both-label PR exactly once: loop 1 defers by paused membership, loop 3 is the render of last resort (R9-1/R9-13) - a 404 from the collaborators-permission endpoint classifies the author read-only instead of renewably deferring the release (R9-10) - cap-branch release evidence reuses the per-iteration events fetch under a success flag (R9-18); release-clock comment corrected (R9-11) - harness gates end-anchor the --json field list (R9-14/R9-15); the escalation POST and the ack-body census gain count pins (R9-16); the date shim answers only the +%s shape it emulates (R9-9) * fix(autofix): close review round 10 Criticals — exact HTTP 404 release classification, isolated replay fixtures Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(autofix): close review round 11 Criticals — engaged stale-ack guard, exact HTTP 404 permission classification --------- 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-15 17:32:23 +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
Heyang Wang	9f8f65dde0	feat: support fork from any conversation (#8817) ... * feat(web-shell): branch from completed assistant responses Add durable response checkpoints so Web Shell sessions can branch from eligible completed Assistant turns without mutating the source history. - Record and validate checkpoints behind serialized topology fences - Preserve historical anchors through replay, daemon, SDK, and UI layers - Publish bounded forks with crash-safe ownership and referenced backups - Serialize prompt, rewind, branch, automatic turn, and close mutations - Cover stale anchors, replay pagination, cleanup, and pending UI states Note: Responses recorded before this change remain non-branchable. # Conflicts: # packages/acp-bridge/src/bridge.ts # packages/acp-bridge/src/bridgeTypes.ts # packages/cli/src/acp-integration/acpAgent.test.ts # packages/cli/src/acp-integration/acpAgent.ts # packages/cli/src/serve/routes/session.ts # packages/cli/src/serve/server.test.ts # packages/core/src/services/chatRecordingService.ts # packages/core/src/services/sessionService.test.ts # packages/core/src/services/sessionService.ts # packages/sdk-typescript/src/daemon/DaemonClient.ts # packages/web-shell/client/components/MessageItem.tsx # packages/web-shell/client/components/MessageList.tsx * fix(session): preserve historical branch checkpoints Keep Assistant-response branching intact across the daemon stack after rebases, including history serialization and persisted-session ownership. - Forward durable checkpoint IDs through Bridge, SDK, and UI layers - Serialize live history mutations and retain valid nested branch anchors - Preserve persisted branches during generation cleanup - Add cross-layer regression tests for replay and stale checkpoints * fix(web-shell): harden response session branching * chore: remove PR comment evaluation artifact Keep the PR review report as a local ignored backup instead of shipping it with the feature branch. - Remove the generated PR comment evaluation from tracked files - Preserve the report under the ignored analyze directory * fix(web-shell): guard historical branch mutations Historical branch requests could outlive the client timeout during an active turn, and interactive forks lacked the recorder's cross-process writer-lease barrier. - Hide Assistant Branch actions while a turn is active - Run interactive fork creation inside the recorder write barrier - Use the concrete checkpoint recorder contract in Session - Document committed-session ownership and implemented design status * perf(core): index historical branch points during transcript scan Build branch catalogs during the frozen index scan so the first history page no longer reopens and materializes the complete active chain. - Retain a compact projection for shared branch-point resolution - Correlate live branch anchors with the completed prompt and final reply - Complete recorder mocks required by the concrete Session contract - Update the reviewed design with performance and correlation invariants * fix(core): address review findings — dead code, boundary remap, promptId guard, stale toast (#8274) * fix(core): address review findings — dead code, boundary remap, promptId guard, stale toast (#8274) * fix(core): address review findings — archived GC, subtype registration, UUID validation, dead code (#8274) * test: strengthen branch-point and fork coverage from review (#8274) Add focused tests requested in PR review: - branch catalog resolves checkpoints that fall on a later page - accept a parallel tool batch closed within a single turn - exercise the linkSync->copyFileSync fork backup fallback success path - prove a remapped checkpoint stays usable via a nested fork - isolate each branch-point validation conjunct across bridge and SDK * fix: address round-4 review feedback for session branching (#8274) - Make the directory-fsync durability test platform-aware (skip on win32), since fsyncDirectoryBestEffort swallows the injected error on Windows and the rejection path is non-Windows by design. - Reject atRecordId on the side-task fork path instead of silently discarding it, so the API surface no longer implies acceptance. - Correct the design doc: name the real promptQueue FIFO (not the nonexistent historyMutationQueue) and describe filtered checkpoint boundaries as remapped to the nearest retained predecessor, not unconditionally null. - Add focused tests: branch-point assistantRecordUuid mismatch rejection, and insight-block branchRecordId anchoring (insight-only block must not anchor onto the previous reply). * fix: address round-5 review feedback for session branching (#8274) * fix: address round-6 review feedback for session branching (#8274) * fix: address round-7 review feedback for session branching (#8274) * fix(core): harden branch-point resolution against malformed transcript shapes (#8274) - Filter null/non-object part elements in the shared branch resolver so a transcript containing null parts no longer makes forkSession throw a TypeError for every checkpoint. - Tag tool calls carried in from the pre-boundary prefix so a dangling call left by a crashed turn no longer permanently disables checkpoint recording; only calls issued inside the turn must close. - Merge duplicate-uuid records first-wins for identity fields in the transcript reader, matching the byUuid index and fork aggregation, so the reader never advertises a branch marker the fork path must reject. * fix: address round-8 review feedback for session branching (#8274) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix: address round-9 review feedback for session branching (#8274) * test(core): pin branch GC isolation from throwing warning callbacks (#8274) * fix(acp-bridge): reject rewind at admission while a prompt is active (#8274) * fix(web-shell): harden session branch publication Preserve direct ACP prompt preemption while fencing branch and rewind history mutations at the Session boundary. Convert branch publication, backup staging, cleanup, and stale-claim GC to asynchronous filesystem APIs, and surface unsupported hard-link commits as typed ACP and HTTP errors. Expand regression coverage and update the reviewed design contract. * fix(web-shell): harden historical response branching Reject branches during prompt admission and keep dispatched mutations owned until their real outcome is known. - remove detached timeouts across ACP, SDK, and WebUI - bound branch cleanup and make title scans asynchronous - avoid full branch-point scans during transcript pagination - add regression coverage from Core through the real daemon and browser * fix: address round-11 review feedback for session branching (#8274) * fix: address round-12 review feedback for session branching (#8274) * refactor(branching): remove branch-specific overdesign Simplify historical session branching around the minimum persistence, recording, and navigation invariants required by the Web Shell flow. - Replace branch claims and garbage collection with staged publication - Validate completed turns incrementally instead of reloading transcripts - Separate persisted branch creation from live session restoration - Bound SDK waits and prevent late results from replacing navigation - Remove unused checkpoint prompt IDs while reading legacy records Note: A pre-commit crash may leave hidden staging or orphan backups. * chore(sdk): update browser bundle budget Account for the combined historical branching and transcript projection APIs after merging main while keeping the browser bundle size guard narrowly bounded. * fix(branching): address review lifecycle gaps Harden historical session branching against cancellation, observer, navigation, and shutdown races found during review. - Normalize cancellation keys and bound close-time mutation waits - Preserve anchors after observer completion and load persisted forks - Report success only when the guarded session switch starts - Cover recorder cursors, fork cleanup, admission, and rollback - Align daemon events and branch errors with runtime behavior * refactor(session): simplify branching safeguards Reduce the session branching surface after review while preserving the critical concurrency, durability, and ownership guarantees. - Remove the unused full-chain resolver and test production entry points - Copy backups from verified open handles instead of using hard links - Reuse the bounded title scan instead of maintaining an async mirror - Deduplicate UI branch requests and fail fast for busy automatic turns - Consolidate repeated mutation tests and retain critical race coverage - Document the retained invariants and rejected overdesign explicitly * test(branching): simplify regression coverage Reduce duplicated branching tests while retaining regression coverage for the safety, concurrency, and lifecycle fixes introduced by this feature. - Consolidate symmetric bridge and agent scenarios with table-driven cases - Remove repeated cross-layer assertions and brittle implementation spies - Drop redundant UI permutations and branch-only visual snapshots * fix(serve): handle branch busy admission * fix(sdk): preserve v1 branch session contract Keep existing latest-state branch callers source- and wire-compatible while retaining the persisted-only behavior for historical checkpoint branches. - Restore no-anchor branches before returning their live client identity - Add a separate typed result for persisted historical branch requests - Clean up restored attachments on stale navigation and disconnect races - Cover immediate continuation and historical persistence independently * fix(daemon): guard branching history mutations Prevent branch creation and automatic Goal turns from racing session teardown or interactive history mutations. - Reject branch admission while a conditional close is authorized - Serialize Goal continuations behind the history mutation gate - Limit branch checkpoints to interactive prompts - Add regressions for close and Goal scheduling races * fix(branching): preserve fork and checkpoint semantics Keep branch checkpoints and file-history snapshots correct across resumed, forked, and non-interactive session flows. - Track the restored active-chain base before the first appended turn - Preserve backup file modes during fork publication - Exclude authenticated channel prompts from checkpoint recording - Add regressions for all three review failures * fix(branching): harden branch and rewind behavior Handle the remaining branch and rewind review findings without widening the feature contract. - Ignore benign concurrent branch rejections in the Web Shell - Validate rewind prompt IDs before using string operations - Pin mutation ordering, cleanup, compaction, and checkpoint invariants - Align sourced-fork fixtures with the canonical side_task value --------- Co-authored-by: heyang.why <heyang.why@alibaba-inc.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: 易良 <1204183885@qq.com> Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-bot <qwen-code-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-15 10:01:49 +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