8676 commits

This branch

This branch

All branches

Author	SHA1	Message	Date
俊良	dd6b7824dd	fix(cli): harden agent view lifecycle recovery	2026-08-21 14:50:03 +08:00
俊良	d0fa70a1a2	fix(cli): serialize stopped session healing	2026-08-21 11:06:23 +08:00
俊良	b0b103c57b	fix(cli): close agent view lifecycle races	2026-08-21 01:51:16 +08:00
俊良	9a7bad886e	fix(cli): make agent view lifecycle recoverable	2026-08-20 15:11:07 +08:00
yiliang114	6432b72fa9	fix(cli): close agent view lifecycle races	2026-08-19 23:49:34 +08:00
yiliang114	eccde8caed	Merge remote-tracking branch 'origin/agent/agent-view-pty-workers' into codex/pr-7801-closeout ... # Conflicts: # packages/cli/src/agent-view/pty-host-process.test.ts # packages/cli/src/agent-view/pty-host-process.ts	2026-08-19 23:31:38 +08:00
yiliang114	3c63475d29	fix(cli): wait for PTY host endpoint shutdown	2026-08-19 23:23:43 +08:00
俊良	70ead6d664	fix(cli): close agent view prompt recovery races	2026-08-19 18:00:31 +08:00
俊良	f8961af8e3	fix(cli): serialize agent view lifecycle transitions	2026-08-19 12:22:11 +08:00
俊良	6fc40ee248	fix(cli): harden agent view lifecycle races	2026-08-19 11:26:06 +08:00
俊良	ff20a1acdd	fix(cli): Confirm worker death before terminal verdicts and close roster fail-open paths ... Round-16 review fixes for the Agent View lifecycle: - Roster strict reader now rejects a declared-but-non-boolean pinned field; roster mutations read through the same fail-closed path so a corrupt-but-present roster.json can no longer be overwritten with an emptied roster (pins exist nowhere else). - The dead-worker event guards also drop events against 'hibernating' records, so a straggler event after the sweep's point of no return cannot flip the record back to alive and fail the hibernated mark. - respawn releases the predecessor's registry entry before launching, keeping the graceful-stop fallback timer from matching the retired host mid-launch. - Worker events normalize waitingFor case at ingest so the queued- prompt dequeue gates and presentation agree. - shouldAdvanceActivityTime stops advancing lastActivityAt while an input control suppresses the marker dequeue, keeping the stale-marker wall-clock evidence honest. - Hibernation sweep, stop fallback, remove, and shutdownAll confirm the actual exit (shutdown RPC with SIGKILL escalation, then host.exited) before writing terminal verdicts: a still-draining worker must not keep the session socket with no signalling path able to reach it.	2026-08-18 22:22:42 +08:00
俊良	878eedad25	Merge agent/agent-view-pty-workers into agent/agent-view-lifecycle ... Resolve pty-host-process.ts: keep the base's kill() settle-after-RPC refinement; for shutdown() keep the R15-5 semantics (no exit verdict — the shutdown RPC returns when the drain starts, which can be survived, so the remote exit poller reports the real outcome).	2026-08-18 15:30:30 +08:00
俊良	b14082ec61	fix(cli): Harden Agent View lifecycle verdicts, ghost cleanup, and resume argv	2026-08-18 15:02:49 +08:00
俊良	4c21f39ac0	fix(cli): close R14 respawn EADDRINUSE, dispatch pid timing and stale verdict races	2026-08-18 10:25:16 +08:00
俊良	adc3bbf419	fix(cli): close R13 verdict races, resume casing and stop/respawn lifecycle gaps	2026-08-17 17:38:11 +08:00
俊良	b78e37444a	fix(cli): close R12 stop-launch orphan, marker clobber and pid-signaling races ... - terminate orphan worker when dispatch/adopt stop errors bypass queueStop - drain queued stop control before hibernate; re-validate pin under roster lock - serialize worker file writes through the shared per-path mutation queue - make dequeue/state heals marker-guarded and skip terminal sessions - sanitize adopted session ids once; guard stale pid signaling	2026-08-17 10:47:10 +08:00
俊良	8572f36818	Merge remote-tracking branch 'origin/main' into codex/qwen-code-pr-7800	2026-08-17 10:28:35 +08: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	d5b26b411f	fix(review): close out the four leftover findings from the #9222 review (#9270) ... * fix(review): close out the four leftover findings from the #9222 review - findings refuses a --to-anchors that resolves to the same file as --input/--out/--outcomes/--test-delta: the resolver input would silently destroy its counterpart while stderr reports every write as successful, and Step 7 joins the pair by id. - the --to-anchors flag now has a wiring gate: every command-boundary test built argv by hand with the camelCase key, so a mismatch between the declared option name and the handler's read would stay green — the new test parses through the actual yargs builder and feeds the parsed object to the handler. - the final absence reason's rewrite gets a distinguishing assertion: the existing toContain check matches the pre-PR wording too, so a revert would leave the suite green while the reason stopped naming the fragment shape. - the shape-2 recovery map gains the substring tier's exact-multiplicity reason: the generic more-lines remedy cannot recover it because the containment tier accepts single-line snippets only; a longer same-line fragment or the line number does (witnessed by probe in the #9222 thread). * fix(review): compare file identity in the findings collision guard and pin its tests * fix(review): harden the findings collision guard to filesystem identity The guard compared path strings — realpathSync where a file existed, lexical resolve() otherwise — so it admitted exactly the collisions it exists to refuse: hard links are two names of one inode that no realpath resolves, a dangling --out symlink aliasing the not-yet-created --to-anchors target rode the "absent file" fallback, and a symlinked directory component aliased absent paths on both sides. Compare dev/ino where both sides exist, canonicalise the deepest existing ancestor where one does not, refuse a dangling sibling link outright, and extract the check into review/lib, replacing the two byte-identical private sameFile copies in repo-context and save-artifact. * fix(review): refuse nested --to-anchors collisions and pin the guard migrations	2026-08-17 00:32:29 +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	195128a17a	fix(daemon): Preserve sessions when active-work close is refused (#9134) ... * fix(daemon): preserve active-work sessions on close refusal Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): guard deferred spawn-owner kill against in-flight closes The reaper can hold a conditional-close probe on a tombstoned entry for up to ACTIVE_WORK_CLOSE_TIMEOUT_MS; a deferred kill fired in that window bounces off the close gate of the child and killSession escalates the error to a channel kill, taking every sibling session down with it. Re-add the activeWorkCloseInFlight exclusion to the branch: the probe resolves the entry one way or the other, and a refusal leaves the tombstone to complete on the next settle event. Also extract sessionCloseDrainBudgetMs so the child drain budget lives in one place, report the shared drain budget instead of the phase-2 residue in the close timeout message, and pin the guards with tests (in-flight probe, in-flight force close, no abort on refusal, phase-1 settle timeout, close-gate release on the timeout path). Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): key sessionClose drain budgets to the actual outer wait Round-2 review follow-ups: - notifyAgentSessionClose derived the child drain budget from the initTimeoutMs default even when the caller applied a shorter outer wait (opts.timeoutMs): a condemned-channel close with an operator --initialize-timeout-ms of 30s told the child to drain for 24s while the daemon stopped listening after 10s, and the unknown outcome escalates to a channel kill. The budget now keys off the wait actually applied. - branchSession's partial-restore cleanup was the last sessionClose sender without a drainTimeoutMs; it now uses the shared helper. - The deferred spawn-owner kill branch uses the canonical isClosingOrAuthorizingClose predicate instead of an inline copy. - The conditional close's history-mutation wait now receives the shared-budget residue instead of a fresh full budget, keeping the round trip under the daemon's outer wait (the body stays untimed, so the guarantee is approximate); its timeout message reports the shared budget. - Tests: pin the conditional-close success path (abort ordered after the first settle via invocationCallOrder) and deduplicate the close-gate tracking mock into trackCloseGateHeld. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(daemon): tolerate definitive close refusals in killSession Round-3 review follow-ups: - killSession escalated ANY sessionClose error to a channel kill, but a child holding its own close gate (changeSessionCwd, restore — child-side state the daemon cannot observe) answers with a definitive RequestError. On that answer the kill now resets entry.closing and returns false, leaving the deferred tombstone to complete on the next settle event, instead of SIGTERMing every sibling session on the channel. Pinned by a test that flips the child from definitive refusal to acceptance and asserts the kill completes without a channel kill. - The deferred spawn-owner kill logs one stderr line before firing; its success path was previously unattributable in daemon logs. - sessionCloseDrainBudgetMs gains a literal unit test pinning the ratio strictly under the outer wait and the >=1ms clamp, so the documented invariants cannot drift with the implementation. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 17:54:32 +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
易良	f9e6b67aa7	fix(ci): stop triaging the autofix bot's own deferred-finding tracking issues (#9264) (#9271) ... * fix(ci): stop triaging the autofix bot's own deferred-finding tracking issues (#9264) Every PR that defers findings for the first time opens a tracking issue upserted by the autofix bot, and the issues trigger (opened/edited/reopened) ran a full triage agent on that bookkeeping issue per deferral — the authorize gate exempts the issues path as read-only, so nothing stopped it. Condition the triage job's issues clause on the creator not being the autofix bot (the same vars.AUTOFIX_BOT_LOGIN || 'qwen-code-dev-bot' identity qwen-autofix.yml upserts under), and route bot-created issues runs to a per-run concurrency group: GitHub evaluates concurrency before the job if, so a run left in the shared per-number group would still cancel an in-progress triage of the same issue before its own skip is evaluated. Pins: the issues-clause guard, the group routing, and the cross-workflow identity sync, all on the parsed document. * test(ci): pin triage issue guard connectors * test(ci): harden triage bot guard pins --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-16 17:09:42 +00:00
qwen-code-dev-bot	f7f78fab4a	fix(ci): force-push release branch so retries replace failed attempts (#9076) (#9082) ... * fix(ci): force-push release branch so retries replace failed attempts (#9076) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): re-validate the release version before force-pushing (#9076) prepare's doesVersionExist check runs minutes to hours before publish pushes (validation jobs and the production-release approval gate sit in between), and --force removed the non-fast-forward rejection that used to serialize the push itself. Concurrent same-version runs could therefore diverge the npm artifacts, the git tag, and main. Serialize publish per release tag and re-validate the unshipped invariant — every published package, the tag, and the release — immediately before the push; pin all three invariants in the workflow tests. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(ci): unify the push-time release guard and key concurrency by dry-run (#9076) * fix(ci): fail closed on push-time release probes and test the CLI seam (#9076) * fix(ci): clarify push-time release refusals and keep benign ones out of autofix (#9076) The push-time guard refused retries after a partial npm publish without saying where the version had shipped or how to recover, and every refusal failed the publish job into notify_failure, filing a "Release Failed" issue and dispatching the autofix agent against releases that did not fail. - Scan all published packages in strict mode and name every shipped location in the refusal (npm packages, origin tag, GitHub release) with partial-publish recovery guidance; a decisive hit ends the check so a flaky later probe cannot mask the refusal with a probe error. - Give the guard distinct exit codes: 3 = already shipped (decisive, benign), 2 = probe or usage failure. Exit 1 is reserved for uncaught node errors so a crash can never masquerade as the benign marker. The push step marks exit-3 refusals via the version_refusal job output, and notify_failure skips its issue + autofix dispatch for exactly that failure while genuine failures still notify. - Cover runCli's default dispatch (prepare's path), the exit-code contract, and the process.exit wiring end to end. * fix(ci): fail closed when the release ref predates the push-time guard (#9076) * fix(ci): keep refusals decisive after shipped hits and skip the POSIX-only test on win32 (#9076) * fix(ci): write push-time guard error annotations to stdout (#9076) Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-16 16:33:14 +00:00
Shaojin Wen	f2a6e6c727	feat(review): validate and scope the incremental anchor inside fetch-pr (#9100) ... * feat(review): validate and scope the incremental anchor inside fetch-pr The incremental path was the one diff this skill still asked the orchestrator to hand-compute: Step 1 said 'git diff <sha>..HEAD inside the worktree', and the recovered-anchor bullet asked for hand-run cat-file / merge-base --is-ancestor checks — the exact shape the skill forbids everywhere else (the diff is a file the CLI writes, never a command a run can skip or get wrong). fetch-pr gains --since <sha>: a hex allowlist first (an anchor from a cache file or a posted marker is never handed to git flag-shaped), then cat-file/is-ancestor/rev-parse behind an injectable probe, ruling one of: effective (diff and chunk plan scoped to since..head), upToDate (anchor is the head, or the commits since it change no bytes — full-range plan kept, because the flows that continue past an up-to-date anchor run full), or refused with the reason (not-an-ancestor for a rebase/force-push, unknown-commit, capture-failed) and a full-range fallback. The decision is the report's incremental field; emptyDiff and collapsedFromUpstream are skipped on a delta scope — both compare the full merge-base range against GitHub's advertised full-PR stat, and a delta is always far smaller, so the collapse ratio would fire on every incremental review. SKILL.md Step 1's two incremental bullets now read the report instead of running git: the cache is read BEFORE fetch-pr and its lastCommitSha passed as --since; the marker-anchor path re-runs the same command with --since appended once the side file lands. * fix(review): keep the empty-delta capture out of emptyDiff, and scope Agent 7 to the delta Six findings from this PR's dogfood review round: - Critical: the empty-delta capture set diffPath, and the full-range fallback can fail or never run (no merge base) — the leaked path made isEmptyDiff read 'captured the full range, and it is empty' and recommend a live PR for closure on an infrastructure state. The fallback now resets diffPath/diffPathAbsolute first, and an upToDate ruling whose full-range capture did not survive is demoted to capture-failed rather than left promising a plan the report does not carry. Both pinned through the real handler. - Agent 7's test-efficacy probe welded --base mergeBaseSha into its brief regardless of scope; on a delta round it would spend the probe budget reversing already-reviewed hunks and report survivors outside the scoped diff. The report's incremental ruling now carries diffBase (the delta's left side, full sha) and the role-7 brief prefers it. - The side-file recovery gate lost the refused-cache-anchor fallback in the rewrite: a force-pushed PR with a newer round posted elsewhere never consulted the marker's still-valid anchor. Restored, with the already-refused sha excluded from the re-run. - The setup-batch ordering contract gains the new edge: a side-file fetch-pr --since re-run must precede repo-context, whose in-place enrichment the re-run's from-scratch report write would discard. - The pr-context ledger hint and the ledger.ts SHA_RE docstring still instructed the hand-run validation this PR deletes; both now point at the fetch-pr --since re-run. - The four handler branches are now integration-tested through the mock harness (mutation-verified: dropping the scopedDelta gates fails the valid-delta case), and the builder pins --since registration. * fix(review): clamp the anchor to the merge base, demote every planless ruling, and pin the seams Six findings from this PR's second dogfood review round: - The anchor was validated against the head but never the merge base: an anchor older than the base (a partial merge landing between rounds, or a tampered deep-history sha) scoped anchor..head — the PR plus a slice of base history, whose comments 422 the whole Create Review call. resolveIncrementalAnchor now takes the merge base and refuses such an anchor as behind-merge-base; null skips the clamp. - captureRange published diffPath before any caller accepted the capture — the round-1 Critical fix was a caller-site reset guarding producer state. The capture now returns text only and the two accepting sites publish; the reset disappears structurally. - A buildDiffPlan partition failure on a delta left the incremental ruling standing over a diff-less, zero-chunk plan. The catch demotes it (scopedDelta stays true so the full-range flags cannot fire over the delta text). - The demoted-upToDate shape conflated two failures under capture-failed and its stderr claimed 'reviewing the full diff' with no diff in existence. It gets its own reason (full-range-unavailable), the status line is emitted after planning and keys on the final ruling, and the SKILL bullet routes it to the diffPath:null degraded state. - The role-7 --base selection and the git-probe invocation shapes were unpinned (both mutation-measured green): agent-prompt tests now assert --base <diffBase> on a scoped round and the merge-base fallback on upToDate; the handler tests assert the exact cat-file / is-ancestor / rev-parse call shapes plus refused-anchor and behind-merge-base rounds end to end, and the partition-failure demotion through a steerable buildDiffPlan. * fix(review): one planless reason, a tree-based empty ruling, and a base the clamp can trust Six findings from this PR's third dogfood review round: - Critical: three shapes published capture-failed over a zero-chunk plan while the reason contract promised the full range was in hand (partition failure; delta throw with the fallback also throwing; delta throw with no merge base). Any refusal ending with no captured diff now reports full-range-unavailable — one reason names the degraded flow — and the stderr line names the underlying refusal. - Critical: the delta round suppressed emptyDiff, so a PR that collapsed to empty between rounds (head tree == merge-base tree while anchor..head stays non-empty) was never recommended for close-as-superseded and fanned agents over hunks GitHub's empty PR diff does not contain. A delta round now rules emptiness by TREE comparison — what the flag has always meant — under the same stale-base guard as the text path. - The behind-merge-base clamp ruled on a base the run had flagged unreliable; a stale base now refuses the anchor (base-untrusted), matching isEmptyDiff and isCollapsedFromUpstream, which both decline to rule in that state. - The anchor-shape predicate was a second, case-divergent copy of the ledger's SHA_RE; it is now imported, so a ledger-blessed anchor and a cache-supplied one cannot be judged by two predicates. - The demotion literal, written three times, is one demote() closure. - No test expanded an abbreviated anchor, so diffBase: resolved -> diffBase: since survived; the resolver now pins the expansion. * fix(review): check hunk containment, read the full range once, and normalize the anchor flag Sixteen findings from the fourth dogfood round (4 Critical, 12 Suggestion). The Criticals: - An in-range anchor can still produce delta hunks the PR's own diff does not contain: an 'undo per feedback' commit reverts previous-round lines back to base content, and a comment anchored there 422s the whole Create Review call. Ancestry cannot see this, so containment is now checked on the hunks (hunksContainedIn, new-side ranges per file) before a delta may be the round's scope. - The recovery flow appended a second --since to a command that already carried one; yargs folds a repeated flag into an array, which stringified to 'shaA,shaB' and failed the hex gate, refusing a valid anchor as unknown-commit with zero git probes. The CLI normalizes to the last value and the SKILL says REPLACE, not append. - An upToDate ruling whose full-range recapture succeeded but failed partitioning was never demoted (guard 1 had already run; the catch required scopedDelta; the post-plan guard required !effective). - base-untrusted refused a fully validated anchor even with no base resolved at all, contradicting the docstring's own 'a null sha skips the clamp' — the delta range needs no base. The structural change under them: the full range is READ ONCE up front whenever a base exists, and is the containment oracle, the fallback, and the quantity emptyDiff/collapsedFromUpstream are defined against (both had been judging a delta — the collapse ratio fired on every incremental round, and an emptied PR went unflagged). That also lets a delta the partitioner refuses retry the full range instead of ending the round diff-less, under a reason that names it (partition-failed). Tests: hunk containment, the emptied-PR round, base-untrusted wiring and its null-base carve-out, the partition retry and the both-fail case, upToDate-outranks-base-untrusted, and toEqual on the resolveCommit-null refusal. reportFor now reads the LAST report — a two-round test was asserting against the first. * fix(review): make the containment oracle a real diff parser, and pin what it rules Thirteen findings from the fifth dogfood round. All three Criticals were the same defect class in the hunk-containment parser this PR added: it recognized structure by prefix with no hunk-state tracking, so it could be spoofed, and it saw only sections that carry hunks. - An ADDED line shaped like a `+++ b/path` header (an embedded diff fixture is exactly that) was read as structure and re-attributed every later hunk — the oracle corrupted in both directions. Both sibling parsers in this codebase already guard it (countDiffChangedLines tracks inHunk; parseAddedLines documents the hazard); this one now consumes hunk bodies by their declared line counts and recognizes structure only outside them. - A whole-file deletion contributed no new-side range and passed vacuously, so a delta whose only content is a file the PR's own diff never mentions became the review's scope. - Sections with no hunks at all — mode-only, binary, pure renames — were invisible for the same reason. Sections are now keyed off the `diff --git` line (present in every shape, including all three above), the path check is separate from the range check, and a path this parser cannot name unambiguously fails closed to the full range. The rest: the dead tree-comparison seam in the test helper and the stale comment naming a gate that no longer exists are gone; SKILL.md's reason enumeration names every reason the CLI emits, and drops partition-failed from the infrastructure-retry list (it re-fails identically for the same sha and already carries a rescued plan). New pins: seven hunksContainedIn cases (both boundary directions, per-file isolation, the spoof, the deletion, hunk-less sections, unparseable paths), a repeated --since whose abbreviation is resolved end to end, the unknown-commit precedence when both probes fail, and the two base-selection conjuncts agent-prompt never pinned. * fix(review): fail closed when the containment oracle is lost, and pin what it parses Eight findings from the sixth dogfood round. The Critical: the containment guard failed OPEN when the full-range capture threw while a base WAS resolved — `fullText === null` short-circuited the conjunct and the delta published with the oracle never run. That is the infrastructure failure the guard exists to refuse, on exactly the large long-lived PR --since is for. It now demotes; the base-FREE shape stays deliberate (no PR diff to be contained in) and both are pinned. Also from the round: - Git C-style-quotes any non-ASCII path by default, which made the parser return null and refuse every --since round on a PR touching such a file — under a reason asserting DISPROVED containment. `core.quotePath=false` is now pinned beside the other capture knobs, and the ruling is tri-state: an oracle that could not rule reports `containment-unverified`, not the delta's guilt. - `upToDate` is no longer demoted when the full range is unavailable. It is a fact about the ANCHOR, proven by the delta capture (or by arithmetic for anchor-at-head), and the flow it serves — "No new changes since last review", stop — consumes no plan at all. On a PR whose base branch was deleted, every same-sha retry was fanning out a degraded full review instead of stopping. - Two fixtures declared old-side lines their bodies never emitted, so a second hunk was swallowed as body content: the `sec` generator now models a pure addition, and FULL_DIFF's header matches its body. Both were latent (single-hunk uses only) and both are now covered by a multi-hunk case. - New pins: the no-newline marker mid-body, and the disk payload of `publish()` in both the scoped-delta and the partition-rescue tests — a write unpaired from the text the report describes is the same mismatch class as the diffPath leak this PR shipped and fixed. - The comment claiming the full-range flags are "skipped on a delta" described the retired design; they read fullText on every round. * fix(review): keep upToDate through a partition failure, and stop renaming deterministic refusals Fourteen findings from the seventh dogfood round. The Critical: the partition-failure demotion keyed on `effective`, which upToDate rounds also carry, so `demote()` rebuilt the decision without `upToDate` and the planless stamp renamed it — publishing "the anchor is invalid" for an anchor that IS the head, and losing the stop branch on a PR with zero new commits. Its stated rationale (Agent 7's welded --base reading diffBase) cannot even apply there: an upToDate ruling never carries one. The other contract fix: the planless stamp fired on any refusal ending with no plan, renaming two captured-but-degraded shapes into the class SKILL retries. It now fires only when nothing was captured at all (`fullText === null`); a partition failure keeps `partition-failed`, which SKILL excludes from the same-sha retry precisely because the same bytes re-fail identically. Also from the round: - The round-6 comment fix landed on the sibling clause; the one above isCollapsedFromUpstream still claimed the retired "skipped on a delta". - pr-context commanded the --since re-run unconditionally, contradicting Step 1's admissibility gate; it now defers to it. - An anchor resolving to the merge base re-captured the identical range; it reuses the one already read. - containmentRuling was computed twice on the refusal path. - The comment claiming a raw-bytes write described the pre-restructure code; the capture decodes to UTF-8 text now. New pins for the round-6 fixes that had none: the oracle-LOST arm (base resolved, its capture threw — the delta must not become the scope), the tri-state reason on an unnameable path, the quotePath shape from the oracle side, the disk payload on the containment-refusal path, `diffPathAbsolute` in both directions, the no-newline marker landing mid-body with counts still owed, and the pr-context routing sentence. * perf(review): parse each diff once per containment ruling The round-7 hoist removed the duplicate containmentRuling call but left the duplicate parse inside it: the null check parsed both texts, then hunksContainedIn parsed them again. Both now share sectionsContained over already-parsed maps, so a ruling is two parses instead of four — on the multi-megabyte diffs --since exists for, that is the difference worth having. hunksContainedIn keeps its string signature; it is the form the tests read. * fix(review): let a refusal reason name only its cause, and unquote paths in the oracle Fourteen suggestions from the eighth round — no Criticals this time, and three of them were previous rounds' fixes that had not actually landed. The contract change, flagged twice now: the planless stamp renamed refusals into a label that meant both "why" and "no plan", so a deterministic partition failure entered the retryable class and a validity refusal invited re-running an anchor already ruled invalid. The stamp is gone. A reason names its CAUSE; whether a plan exists is `diffPath`, which the report already carries. SKILL.md's reason bullet and same-sha retry list follow, and `full-range-unavailable` is retired. The oracle now unquotes C-style paths itself (`unquoteCStylePath`, the helper the chunk parser already uses). Real git quotes a path holding a quote, a backslash or a control character even under `core.quotePath=false`, so trusting the capture's config was never enough — and the pin is now asserted where it is declared, since deleting it fails nothing else. Round 6's no-newline fixture is finally the shape it was meant to be: my round-7 edit over-escaped and silently did not apply, so the marker still arrived after both counts were spent and the in-hunk branch it was written for never ran. Also: an empty-string `diffBase` no longer silently drops Agent 7's whole probe block, and SKILL's model-differs branch carries the same `diffPath: null` caveat its siblings do. New pins: the `!upToDate` exemption in the partition catch, the anchor-at-head handler path, the merge-base dedupe shortcut, a plan-derived field on the delta happy path, a multi-hunk OUTER range, the base-untrusted/clamp order, the resolved-sha comparison for an abbreviated head, the negative case for the routing tail, and the pinned-config contract itself. * refactor(review): build the containment oracle on the shared diff parser Seven suggestions from the ninth round, again with no Criticals. The class-level one is the structural change: three consecutive rounds each found a fresh shape-tolerance defect in the hand-rolled unified-diff grammar this PR added — count-less headers, trailing function context, quoted rename headers, deletion junctions, hunk-less sections. The oracle now reads its sections and hunks out of `parseDiff`, the parser the chunk planner already trusts on these exact captures: it unquotes paths, tracks hunk bodies, and knows the binary and rename shapes. A ruling is set arithmetic over its output, and the private grammar is gone. Paths that used to fail closed (a space, a quote, a mixed quoted/unquoted rename) now rule normally; the remaining "could not rule" state is a payload with no sections in it at all. Also from the round: - A probe ERROR was being reported as a verdict about the anchor: `gitOpt` collapses every non-zero exit to null, so a 128 or a timeout kill read as a definitive "not an ancestor" — a reason the recovery flow treats as deterministic, so the anchor was never retried and the round paid a full review for a transient fault. A new `gitProbe` keeps the exit status, exit 1 stays the definitive no, and anything else refuses as infrastructure. - A value-less `--since` (yargs parses a bare flag to the empty string) reported `unknown-commit`, asserting this history never held a sha nobody supplied. It is now ignored with a line saying so. New pins: the arm order that keeps `upToDate` through a lost oracle, the `effective` clause that stops a refused anchor being relabelled `partition-failed`, `diffPathAbsolute` nulling in the partition catch, and the empty-string conjunct in Agent 7's base selection. * fix(review): drop the containment slack, unpeel the existence probe, survive a bad flag and a bad write Four findings from the tenth round, three of them Critical and all verified against real git. - The oracle's `e + 1` end-slack, documented for deletion junctions, was applied to EVERY inner hunk, so a delta hunk ending one line past the covering PR-diff hunk published as the review scope — a line GitHub does not display, where an anchored comment 422s the whole review. The measured trigger is ordinary: a revert adjacent to a kept change. The slack is gone; shared deletions are covered at equality, since both captures share the head tree. - `cat-file -e <sha>^{commit}` answers 128, not 1, for a well-formed but unknown sha, so the definitive-absent branch was dead code: every unknown anchor was reported as a transient failure the recovery flow retries forever, and `unknown-commit` was unreachable. The peel is gone; "is it a commit" stays covered by `resolveCommit`, whose null maps to `unknown-commit`. - yargs turns `--no-since` into boolean `false` even for a string option, which reached the hex test and then crashed on `since.slice(…)` after the worktree existed and before any report was written. A non-string anchor now falls through to the no-anchor path, like the documented empty-string sibling. - The extracted `publish()` left the diff-file write outside the capture's try/catch, so an ENOSPC on a constrained runner killed the command where it used to degrade to a diff-less report with disclosed coverage. It returns false instead, and both call sites treat a failed write as the capture failure it is — including the partition rescue, where it must not be swallowed as a tiling failure. Each is pinned, and the boundary test now asserts what its comment always claimed. The suite's writeFileSync mock is reset per test for the same reason its siblings are. * fix(review): resolve commit-ness before ancestry, and give every probe the same three-way split Twenty findings across rounds 10 and 11 — eight of round 10's were missed on my side, because `reviewThreads(first: 100)` pages and this PR now has more than 100 threads. The Critical and the shape of the rest: - Three deterministic anchor classes were still landing in the retryable `capture-failed`: an existing NON-commit object (a blob sha passes `cat-file -e`, and asking `merge-base --is-ancestor` about it is an error, not a "no"), an absent ABBREVIATED sha (git answers 128, not 1), and the 41-64-hex band a SHA-256 marker occupies against SHA-1 history. Commit-ness now resolves BEFORE ancestry, so the whole class becomes what it is — an anchor this history holds no commit for — and `cat-file`'s 128 is read as absence rather than as the surface failing. - `resolveCommit` was the last probe folding every git failure into a verdict about the anchor; it now splits three ways like its siblings. - An uppercase-but-valid sha was refused before any probe ran, under a reason asserting the history never held it, and echoed back cased so a recovery flow re-submitted it forever. Anchors normalise at entry. - The rescue path could announce "the round is a full review" and stamp `partition-failed` when the rescue tiled but its write failed; that is a capture fault and now says so. - The planless stderr line blamed the capture on `partition-failed` rounds, where both captures provably succeeded. Docs corrected where they contradicted the code: the `commitExists` signature (no `^{commit}` peel), the unconditional "a fetchFailed base REFUSES the anchor" (only when a base resolved), the timeout's exit status (a signal kill leaves `status: null`, not a high number), and the "stays a valid patch" claim (the file is read, never applied). New pins: the deletion junction in both directions, every one of the ten pinned diff flags, an ancestry-only probe error, quote-bearing paths keying apart, a multi-section delta, `emptyDiff` under ENOSPC, the null-base clamp asserted on the CALL, and `gitProbe`'s exit-status extraction against real git. * test(review): observe the core.quotePath pin in the real-git capture parseDiff unquotes C-quoted paths defensively, so every path assertion in the integration suite reported sub中文.ts whether or not the capture pinned core.quotePath=false. Deleting the pin from PINNED_DIFF_CONFIG left all eight tests green while consumers that read the raw capture rather than the parse tree would have started seeing the octal-escaped shape. Assert both shapes against the raw text on either side of the pin, and set core.quotePath=true in the hostile config rather than resting the control on a git default that is free to change. * fix(review): rule deletions by content, and pin the seams round 12 measured The containment oracle compared NEW-side line ranges only, and a deleted line occupies none of them: what survives a deletion hunk on the new side is its context, which the covering hunk contains for free. So a delta that removed lines the PR itself introduced after the merge base — the "undo per feedback" round — was ruled contained, and the review scope became a diff whose content GitHub displays on neither side, where one anchored comment 422s the whole Create Review call. Reproduced against real git: delta `@@ -6,9 +6,6 @@` inside full `@@ -2,14 +2,14 @@`, ruled ok. The two sides are comparable in different ways. Both captures end at the same head tree, so new-side line numbers name the same lines and compare as numbers; their old sides are the anchor and the merge base, so deletions compare only by content. `-X` in the delta means X stood at the anchor and is gone at head, so if X also stood at the base the PR must delete it too — and its absence from the full capture is exactly the refusal. Also from this round: name the planless cause instead of inferring it from the refusal reason (a refused anchor whose full range then failed to tile reported "no diff could be captured" moments after the capture succeeded); drop `hunksContainedIn`, an exported wrapper with no production callers that collapsed the two-fact ruling the refusal enum pays to keep; and correct the `core.quotePath` rationale, which asserted a counterfactual this PR's own parser refutes. The rest is coverage for seams the round measured as unpinned — each added test verified to kill the mutant that motivated it, and no fixture relaxed to accommodate the new rule: - probe exits split three ways (128 twice, a signalled `status: null`); the shared shim can only produce 0 and 1 - commit-ness settled before ancestry, with ancestry given an error channel - `not-an-ancestor` ruled ahead of `base-untrusted` - the SHA-256 ceiling, from both the accepting and refusing sides - the containment start boundary at one line, and each oracle input null independently - the rescue's write-failure and no-base branches, both previously unreachable - Agent 7's `--base` welded through the real brief over the real report - real-git `--is-ancestor` yes, no, and error * fix(review): close four fail-open paths in the oracle, and pin round 13's seams The containment oracle accepted an "undo per feedback" delta on four distinct paths, each reproduced before it was fixed: - Both captures reach the oracle already decoded as UTF-8, and that decode is lossy. Two filenames differing only in an invalid byte share one U+FFFD map key, so one file's hunks were judged against the other's ranges; two byte-distinct deleted lines matched 1:1. Neither is detectable after the decode, so the oracle now declines to rule rather than ruling on text it knows is not the text git produced. - Deletions compared by set membership, so a single `-X` in the PR's diff cleared any number of `-X` lines in the delta. They are counted now: a round deleting two identical lines where the PR deletes one is refused. - A delta section with nothing comparable — a mode change, a pure rename — carries no range and no deletion, so both containment loops iterated zero times and it passed vacuously. That is right only when the covering section is equally contentless; against one with hunks it is refused. Agent 7's `--base` was interpolated unquoted into a fenced bash block behind a `typeof === 'string'` check, which `abc123; touch /tmp/pwned` satisfies. It takes SHA_RE now — the same predicate the anchor itself must pass — and falls back to the merge base rather than throwing, because unlike the sibling `host` guard a correct fallback exists here. Two documentation defects the round found. The retry taxonomy forbade retrying `partition-failed` because the partitioner re-fails on identical bytes — true of the partitioner, false of a PLANLESS round, where the rescue had no base and the re-run's base fetch is the component that changes. And three fixture comments described `parseDiff` as closing hunks by their declared counts; it closes them structurally at the next header, and the mismatch was teaching future fixtures the wrong mechanics. The rest is coverage for seams the round measured as unpinned, each added test verified to kill the mutant that motivated it: - gitProbe's `status: null` half against real git (PATH sabotage), and its `out` half, whose trim `resolveCommit` depends on - the timeout-kill arm of rev-parse and merge-base, not only cat-file - an uppercase anchor through to acceptance, not only to refusal - `collapsedFromUpstream` as a full-range fact on a delta-scoped round - the deletion rule's per-file keying, the empty-plan invariant of a failed rescue write, and the narration that distinguishes it from a tiling failure - parseDiff's hunk-closing and no-newline-marker mechanics - revert-guards for the SKILL.md sections and the pr-context tail this PR ships, including the admissibility condition on the re-run instruction * fix(review): match deletions inside the enclosing hunk, and widen the pins The deletion budget was keyed per FILE, so a `-X` the PR's diff displays in one hunk cleared a `-X` the delta performs thirty lines away in another — a line displayed nowhere near where the delta deletes it, which is the same 422 the whole check exists to prevent. Locality was available all along: both captures end at the same head tree, which is the fact the range check already rests on. The parsed shape is now per hunk rather than flattened per file, and each delta hunk's deletions are drawn only from the outer hunks that enclose it. The empty-enclosure case subsumes the previous hunk-less guard, so a delta that has hunks against a covering section that has none is refused by the same arithmetic. Agent 7's `--base` now shape-checks both sources. Only the anchor was checked last round, while `mergeBaseSha` reaches the identical unquoted interpolation on every non-incremental round — the common case — and the plan is parsed with no field validation on that path. The test fixture's `mergeBaseSha` was `abc123`, six hex characters, below git's own abbreviation floor: a value `merge-base` cannot produce, so it is a full sha now. The retry taxonomy's planless exception was too broad. A planless `partition-failed` splits on `mergeBaseSha`: null means the rescue never ran for want of a base, so a re-run's base fetch is what changes; non-null means both ranges were in hand and both refused to tile, which a re-run reproduces exactly. The bullet names the field. Coverage for the seams this round measured, each verified to kill the mutant that motivated it: - a deletion ending the hunk body with no trailing context, pinning the body scan's trailing bound as well as its leading one - one-sided lossy captures, which are what separate `containment-unverified` from the reason that asserts a proven violation - short deleted lines, where stripping two marker characters instead of one transforms both captures identically and collapses `-a` onto `-` - the provenance rule's discriminating condition and its fallback half - the `cat-file` / `merge-base --is-ancestor` prohibitions, named in a test's comment but asserted nowhere - the ledger tail's antecedent and its validates-and-scopes clause Also drops a dead ternary whose arms were byte-identical, in the helper that builds the U+FFFD collision fixtures. * fix(review): tie deletions to their junction, and stop scoping without a base Three more ways an unchecked delta could publish as the review scope, each reproduced before it was fixed. Content alone never said WHERE. A `-dup` the PR displays near the top of a file cleared a `-dup` the delta performs thirty lines down, at a junction the PR's diff never touches — a single inner hunk against a single outer hunk, budget spent exactly once, so no amount of counting closed it. Deletions are now keyed by `content@junction`, the new-side cursor position where the line stood. Junctions are comparable for the same reason ranges are: both captures end at the same head tree. That is also what makes the file-wide budget safe to hold across delta hunks, where rebuilding it per hunk had let one displayed deletion clear one in each. The base-free arm published the delta with neither the clamp nor the oracle run — deliberately, on the reasoning that with no merge base there is no PR diff to be contained in. The capture reasoning was right and the scope reasoning was not: no diff to check against is the absence of proof, not proof, and it was the one arm shipping a scope nothing had checked. It degrades to `containment-unverified` now, which costs no review that existed — a base-free round has no full range either, and already tells agents to run their own diff. The retry exception added last round keyed on a null `mergeBaseSha`, but that null has two causes: a base fetch that failed, which a re-run repeats, and a merge-base that found no common ancestor on unrelated history, which a re-run reproduces exactly. It now reads `baseFetchFailed` as well. The junction change required the fixtures to model real captures, which the round independently asked for: `secDeleting` now places its deletions at a stated junction, and three fixtures that declared hunk counts their bodies could not produce are corrected. The DELTA/FULL pair gave one head commit two different trees; it is one history now. Coverage for the seams the round measured, each verified against its mutant: - the cache-path opening of the incremental bullet, the only instruction that makes `--since` fire at all, and the retry classification the recovery loop acts on - the body scan's leading bound, against the shape that injects a spurious deletion from a section header - the over-supplied deletion direction, where an equality rewrite refuses a valid round under a reason that asserts a proven violation - the no-newline marker's effect on junctions - the ledger tail's command-naming and scoping clauses * test(review): pin that an at-head anchor runs no delta capture The handler test claimed to pin the `resolved === fetchedSha` arm "not just the empty delta", but its fixture made the two indistinguishable: with the arm removed the anchor resolves to the head, the handler captures `f00df00d..f00df00d`, the mock answers empty, and the empty-delta arm sets the identical `upToDate` — report and written diff both byte-identical. Assert the capture ranges instead. Deciding at-head before any capture runs is what removes the redundant `git diff`, and that is now what the test measures: under the mutant the ranges come back as the merge-base range plus `f00df00df00d..f00df00df00d`.	2026-08-16 16:10:15 +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
Shaojin Wen	2af4d3919a	test(review): realpath the skill-parity fixture root (#9269) ... bundleStalenessNotices resolves the entry with realpathSync before deriving the tree, and the mock's fault injection matches by exact path. On a platform where tmpdir() is a symlink (macOS: /var -> /private/var) the unresolved fixture lived in a different namespace from every path the walk produces, so the read-fault stage never fired and its notice silently dropped out of the six-line pin — the suite failed with 'expected length 6 but got 5' at the base commit on such machines. Building the fixture under the realpath'd root puts every derived path in the same canonical namespace as the walk.	2026-08-16 15:46:43 +00:00
易良	8a34d3eee1	test(web-shell): pin silent failure of background artifact refreshes (#7427) (#9227) ... * test(web-shell): pin silent failure of background artifact refreshes (#7427) The toast-spam behavior reported in #7427 no longer exists on main — loadArtifacts carries no notice dispatch and the hook swallows background-refresh failures, keeping the last-good artifacts. What was missing is a regression pin: add one that fails a background refresh and asserts last-good artifacts survive, loading clears, no error surfaces, and the next refresh recovers. Mutation-verified: clearing artifacts in the catch turns it red. * test(web-shell): cover artifact refresh triggers * test(web-shell): tighten artifact refresh assertions * test(web-shell): settle artifact-refresh mocks via deferred awaits (#7427) The two regression tests added here flushed their mocked refreshes with a single microtask, so under full-suite parallel load the hook's refresh continuation intermittently missed the React commit and the last-good assertions saw artifacts === [] — a signature indistinguishable from a real #7427 regression. Model every mocked load as a deferred and resolve/reject + await it inside act, the shape the file's pre-existing tests already use (review round, 7 fragile flush sites). Also fold in two review pins while the tests are being rewritten: - R2-2: the superseded-failure test now rejects the stale load while the superseding load is still in flight and asserts loading stays true — the requestId guard in the finally cleanup becomes load-bearing (mutation-verified: dropping the guard fails the test). - add a waiting -> idle settling-trigger test so the prompt guard cannot be specialized to 'streaming' ('waiting' is a real prompt status). * test(web-shell): pin the version bookkeeping and owner-guard halves (#7427) - Non-monotonic artifactsVersion sequence (1->2->1 from a non-zero start) with exact loadArtifacts call counts: killing the previous-value bookkeeping silently skips the refresh that returns to a previously-seen version (stale artifacts panel, suite green) — measured mutant. - Owner-flip supersede variant: the provider flips isCurrent() the instant the session switches, before a re-render; an in-flight load resolving in that window passes the requestId half and only the !owner.isCurrent() half of the guard stops it from painting the previous session's artifacts — deleting that half left all prior tests green (measured). * test(web-shell): pin superseded artifact successes --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-16 15:29:58 +00:00
易良	6970c76114	test(review): close confirmed pin gaps from #9194 (batch 1) (#9225) ... * test(review): close confirmed pin gaps from #9194 (batch 1) Pin the auth<gh ordering half (issue-context/comment-body/fetch-diff), the issue-context write-target path, the full 'NOT in the closing set' extras header wording, and the Number.isInteger guard halves for fractional pr_number/id/--pr (mutation-verified). Audited the rest of the checklist: whitespace-only --host rejection, fetch-diff non-integer case, comment-body write path, and pr-context host-routing/omitted-host pins are already covered on main. Remaining #9194 items deferred to follow-up batches. * test(review): pin meta auth ordering * test(review): complete command pin gaps * test(review): pin omitted host resets --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com>	2026-08-16 15:29:44 +00:00
Shaojin Wen	13472eb721	fix(review): keep the presubmit overlap list out of the canonical findings artifact (#9268) ... Step 7 told the orchestrator to write the flat {path, line, id?} overlap list for presubmit --new-findings to the very file that holds the canonical findings artifact, replacing it with a flat shadow before Step 8 archives it. The list is presubmit input, not the artifact — give it its own file (qwen-review-{target}-new-findings.json), still covered by the Step 9 cleanup glob.	2026-08-16 15:09:22 +00:00
Shaojin Wen	c031cc6279	fix(ci): self-heal failed checkouts on the reused review runners (#9220) ... * fix(ci): self-heal failed checkouts on the reused review runners A checkout failure on the self-hosted review pool was terminal: either a transient network drop mid-fetch (curl 92 / early EOF), or a corrupt persisted workspace whose refs claim objects missing from its object store, after which every fetch dies in negotiation with 'remote did not send all necessary objects'. ecs-qwen-runner-64c-23 stayed in that state for two days (2026-08-13..15), failing seven review jobs on the same missing SHAs. Make the first checkout continue-on-error; on failure wipe the whole workspace (not just .git) and retry the identical checkout once. The workspace is disposable — later steps reinstall deps and tools. * fix(ci): heal with the pool wipe idiom and pin the checkout guardrails (#9220) * fix(ci): pin the heal chain's sudo leg, path guard, and survivor signal Addresses the 16:40 review round on the checkout self-heal: - The wipe-failure test leaned on the real sudo, so it covered a different branch per lane; replace it with a PATH-stubbed sudo that forces both legs to fail hermetically, and pin the survivors left in place plus their oncall-visible warning. - The '|| sudo -n find' escalation leg survived deletion mutants: add a stub-sudo test proving the leg actually runs when user-mode find fails (leg-deletion and '||'->'&&' mutants both verified red). - Reuse the triage idiom's suspicious-path guard before wiping. - Count post-wipe survivors and warn with the count — triage exits 1 here, but the heal chain must stay alive for the retry. - Disclose in the step comment that the sudo leg only helps pool members with passwordless sudo. * fix(ci): close the heal guard's trailing-slash hole and name wipe survivors (#9220) * fix(ci): canonicalize the heal guard's path match and allowlist the runner workspace (#9220) * fix(ci): strip the heal allowlist root's trailing slashes and pin the guard layers (#9220) * fix(ci): canonicalize the heal lock fixture and pin the WS strip loop (#9220) * fix(ci): keep the checkout-heal suite green on a BSD userland Addresses the 09:38 review round: three of the new tests assume the wipe script's `realpath -m` canonicalization actually ran, and `-m` is a GNU coreutils extension — Darwin ships FreeBSD's `realpath [-q]`, exits 1 on it, and the script's `|| printf` fallback silently keeps the raw path. The production script is unaffected (the review pool is Linux-only), but this suite is excluded on win32 alone, so it also runs on the macOS lane, where the assertions are red for a defect that cannot exist there. - Probe the host for `realpath -m` and skip the canonicalization test when it is absent, rather than skipping on `platform === 'darwin'`: the probe keeps the coverage on a Mac with coreutils on PATH and still skips on any other non-GNU userland. Mutation-checked on a GNU host — deleting the canonicalization line still turns the test red. - Spell both halves of the allowlist comparison the same way in the lock fixture: it resolved its workspace with realpathSync while the runner-workspace root stayed raw, so on a symlinked tmpdir (macOS /var -> /private/var) the two sat on opposite sides of the link, the guard refused, and the wipe helper threw before any assertion ran. Canonicalizing the root keeps both tests running everywhere instead of leaning on the GNU-only flag to reconcile them. - Record in the step comment that `-m` is GNU-only and that off-GNU the guard degrades to the strip loop and the allowlist. Verified by simulating a BSD userland (a PATH-fronted realpath that rejects -m): the suite goes from 1 failed / 12 passed to 12 passed on the CI lane's environment, and from 3 failed / 10 passed to 12 passed with a symlinked TMPDIR, matching the two failure shapes reported. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-16 15:06:54 +00:00
Shaojin Wen	df99f05e51	feat(review): run-session ledger and cross-session agent evidence (#9091) ... * feat(review): run-session ledger and cross-session agent evidence Groundwork for resuming an interrupted review run. fetch-pr now records its session id in a run ledger beside the prompt records and stamps the plan with a sha256 of the captured diff bytes; a new readRunTranscripts reads the harness transcripts of every session the ledger names (the current session's contract is unchanged), so coverage, retirement, the layer-audit gate and the cost ledger can credit an earlier attempt's certified work. Coverage counts such agents as recoveredAgents and discloses the continuity; the cost ledger folds the earlier sessions' main loop and agents into the run's totals and reports priorSessions. A run that never resumes sees no behavior change: with no ledger entries every reader reduces to its previous single-session read. Fabricated ledger entries grant nothing — they only name directories under the harness's own subagents tree, and credit still requires the existing content-shaped pairing (verbatim-delivered prompt, opened brief, diff reads). * fix(review): address review feedback on the session ledger Ledger.main drops its stale | null (computeLedger throws before folding when the current chat holds no above-floor record, so the renderLedger guard was dead code); recordResume/recordRestart gain dedup guards so a caller-side retry cannot double-count toward the resume cap or fake a second restart; fetch-pr's ledger append is now wired-tested (called with the plan path, after the plan write); and the layer-audit gate's real reader gets its own prior-session tests, including the discriminating partial-walk shape that separates invisible from credited. * fix(review): keep the run epoch stable and the continuity note non-capping Two Criticals from the automatic review. repo-context's enrichment rewrite advanced the plan's mtime — the run epoch every fence keys on — which orphaned the session-ledger entry fetch-pr had appended an orchestrator turn earlier: on a resume the prior attempt's transcripts were invisible and the feature silently re-ran everything in the primary medium/high flow. The rewrite now restores the plan's mtime (enrichment is not a re-capture), pinned by a test that backdates the plan and asserts the mtime survives the command. The continuity disclosure also moved off the capping disclose() channel: compose-review folds every disclosure into the unreviewed-dimension cap and the "Not reviewed:" rendering, so any resumed run that recovered work was permanently downgraded to COMMENT and its reused work called not-reviewed — unrepairably, since the prior records never leave the ledger. Coverage now only counts recoveredAgents, and compose-review renders its own continuity block beside the other disclosed-but-not-capping notes (deferred lint, test-plan rulings), on every verdict including Approve. A new compose test pins the clean resumed run at APPROVE with the note and without the partial-review opener. * fix(review): contain, bound and supersede the prior-session reads Round-2 blockers on the ledger PR. The currentDirOptional escape is now narrow on both axes — only ENOENT (the error's cause travels with it) and only when the run ledger actually names prior sessions — and it is passed at every reader that runs before the resumed session launches anything: coverageFromTranscripts, verificationGaps, the retirement scheduler and the layer-audit gate, which without it failed OPEN on the layers a prior attempt never walked. Prior-session directories now come from one guarded accessor that skips a symlinked subagents/<id>, so a planted link cannot feed foreign transcripts to the transcript union or to the cost ledger, which reads file content with no certification step. The ledger also clamps each prior chat to the moment the next attempt began (an interrupted CLI session that kept serving unrelated turns was billed as review cost), discloses an unreadable prior agent dir instead of silently flooring it, and sums each session's own span for wall time rather than the envelope across the dead gap. Coverage's Uncoverable declaration takes the supersession guard its sibling flags already had: a stale declaration deleted live coverage post-loop and order-independently, so no relaunch could ever clear the cap; a record that declared a chunk unreachable is also no longer counted as recovered. The ledger read path deduplicates and applies the session-id charset gate to the resume marker too. * docs(review): say what the recoveredAgents bar actually is It is a strict subset of the live credit bars, not "the same bar": no drift rescues, because the count reports reuse and caps nothing, so it should under-claim rather than vouch for a delivery the pairing could not fully confirm. * fix(review): bill a resumed run once, clamp its agents, and say what the pairing proves Five blockers from the round-3 review of the ledger PR. The cost ledger pushed TWO overlapping spans for a prior session — its chat window and the agent window nested inside it — so a resumed run's wall time double-counted the nested minutes; it now folds one span per session from the union. Prior-session agent transcripts were read with only the floor, so unrelated subagents launched in the interrupted CLI session after the review died billed to the review; readAgentDir now takes the same ceiling the chat already had. The two new chmod-000 fault tests get the repo's platform/root guard: on Windows chmod only toggles the read-only attribute and readdir still succeeds, which would have turned the test_windows merge-queue job red on this PR's own tests. And the module header's claim that the content-shaped pairing is something "fabrication cannot satisfy" was refuted by probe: an actor who can write into the harness tree can read a recorded prompt back and plant a matching transcript. The prose now states the guarantee that holds — it defeats an orchestrator fabricating its REPORT of the run, not an actor with write access to the tree, and that bar is the same one the current session's records have always cleared. Also from the same round: the ledger read paths refuse anything that is not a regular file (a planted FIFO blocked readFileSync forever while the write side was already noFollow-hardened), the epoch fence gained its upper half (a future-dated entry outlived every later rewrite), endsAtMs is derived in time order rather than file order so the cost clamp cannot invert, transcriptDirsForRun is gone (no production caller), and the tests gain the missing verdict assertion, the two zero-launch cases that actually exercise currentDirOptional at the retirement and layer-audit call sites, a findIndex-miss-proof ordering check, and a compose fixture that is no longer re-contaminated by base()'s eagerly evaluated default. * fix(review): refuse mid-flight credit, key the fence exactly, fold the ids A batch of findings that had gone untriaged (their threads were opened under the author account, so a reviewer-side filter had hidden them). A prior attempt's agent that died mid-flight — verbatim prompt, a logged diff read, no return — passed every coverage guard and marked its chunk covered, so the resumed run skipped the relaunch and the chunk's findings never existed anywhere. A prior-session record with an empty final text now credits nothing, in the coverage walk and in the recovered count alike. The current session keeps today's semantics: an empty return there is an agent still running, which the idle checks own. Session ids become PATH segments, so `s1` and `S1` are one directory on APFS and Windows: a case-variant ledger entry passed the string exclusion and re-read the current session as a prior one — every record twice, `recoveredAgents` minted on a run that never resumed, and the current chat folded into the prior totals. The exclusion and the dedup are now case-insensitive. The fresh-run fence was inexact by its own slack: a previous run that appended within it survived this run's plan write. Each entry now records the plan mtime it saw, and a reader keeps only entries that saw THIS plan — exact by construction, with the window as the fallback for entries written before the field existed. Also: the run-epoch fence had a verbatim private copy in three modules and now lives once in prompt-record.ts, and the prior-directory read no longer re-implements the files-to-records pipeline — one `recordsIn`, so a future record-level filter cannot apply to live evidence while bypassing recovered evidence. * fix(review): gate prior evidence on an authorized resume, and bound what it reads The round's central finding: the ledger is an address book, not permission. Any session pointing at an old plan could union the ledgered attempts' transcripts and inherit their coverage — after head drift, stale evidence could certify code nobody reviewed. Reading prior evidence now requires this session's own entry in the resume marker, which `fetch-pr --resume` writes only after every probe passed. One gate, in the accessor every reader goes through. With it, several narrower holes close too. A transcript is checked against the session that owns its directory, so a record COPIED into another attempt's directory cannot earn that attempt's credit. Each prior attempt's evidence is bounded above by the moment the next attempt began, so a session that kept running after the resume took over is no longer credited to the review. The exact fresh-run fence loses its legacy fallback — the field ships in the same change as the ledger, so there are no older files to be lenient toward, and the fallback was the only way a previous run's entry could survive a plan rewrite. And both ledger files are bounded in bytes and entries before they are parsed: a planted huge or duplicated ledger could otherwise stall every command that touches bookkeeping, or spend the resume cap. The fixtures now build their state with the real writers instead of hand-written JSON, which is why several of them changed shape: a hand-written ledger has no plan stamp, and a re-homed transcript that keeps its old session id is exactly the misplaced shape production now refuses. * fix(review): drop unfinished prior records at the source, and not count superseded ones Two halves of the same principle. A prior attempt's agent that never returned is excluded from the record set EVERY gate reads, not just from the coverage walk — the same record would otherwise still satisfy roster matching and the Step 4/5 delivery floor, and a gate reading a different evidence set than its siblings is how a review certifies work nobody did. And `recoveredAgents` no longer counts a prior record whose obligation a current relaunch already satisfied: the count is what the continuity note reports, so claiming recovery for work this run re-did would misdescribe it. The compose fixture that hid this is fixed too — `base()`'s literal evaluates its `coveredPlan()` default even when the caller overrides `planPath`, which re-created the current-session record the test had just re-homed. * fix(review): bill each attempt from its own start, and require delivery for a receipt The cost floor was the plan's mtime for every stream, so a review that began inside an existing CLI session billed that session's earlier, unrelated turns — and on a resumed run each prior attempt was floored the same way. Each stream is now floored at the moment its session became a review attempt, from its own ledger entry, and a stream that exists but cannot be read is counted and disclosed rather than silently skipped: a lower total must not be presented as a complete one. The layer-audit gate now requires a receipt-contributing auditor to have been launched with the CLI's own recorded prompt and to have opened the brief it points at. Territory alone let a compliant sibling satisfy the floor while a hand-written auditor supplied the claims. repo-context skips its enrichment write entirely when the serialized plan is unchanged — the common resumed case, where the commit-then-restore window simply cannot open — and when it does write, a failure to restore the plan's timestamp is reported instead of silently advancing the run epoch out from under this run's own evidence. Tests: verificationGaps is pinned directly on the zero-launch continuation (evidence only in a prior session, no current transcript dir), the resumed-run ledger line is asserted in both numbers and by its absence, and the compose auditor fixture now delivers the recorded prompt it always claimed to. * fix(review): close the certification, epoch and supersession gaps the audit found Five behavioural defects, each mutation-proven and each now pinned by a probe that reddens when the fix is reverted. The Step 4/5 gate read run-scoped evidence WITHOUT the dead-record filter its coverage sibling applies. Delivery is checked as recorded prompt plus opened brief and never consults the return, so an interrupted attempt's verifier that opened its brief and died satisfied the floor — a verification nobody performed, certified. The filter now lives in one function both gates call: a gate reading a different evidence set than its siblings is how a review certifies work nobody did, and that is exactly what the split allowed. The plan-epoch restore passed `Date` objects to `utimesSync`. A `Date` carries whole milliseconds; APFS and ext4 keep nanoseconds. The restored mtime was therefore CLOSE to the original and not equal to it, the exact `planMtimeMs === planMtime` fence read it as a different plan, and every session entry was dropped — the resume ledger silently emptied on the filesystems this actually runs on. Restored from float seconds now, and the verification compares with a millisecond of tolerance instead of exact float equality, which was firing on every content-changing enrichment (28 leaked WARNINGs in the suite, and not one test noticed). The recovery count's supersession check iterated ALL records, so two prior records for one obligation — a whiff-relaunch inside the interrupted attempt — superseded each other and both vanished from the count while coverage still credited their chunk. Narrowed to current-session records: the count answers "what did this run reuse", so only a relaunch HERE supersedes. Two test-only defects in the same pass. The check-coverage test titled as accepting prior-session Step 4/5 evidence contained none: with no such records both steps fail as not-built and merge into one gap whose subject is the combined `'verification and reverse audit'`, which equals neither name the assertions excluded — it passed on a review where nothing was verified. It now builds real Step 4/5 fixtures and asserts no gaps at all. The current-session billing floor had no discriminating test (reverting it kept all 56 green), and the APPROVE-path continuity separator had none either (dropping the clause glued the note onto the verdict with a single space, all 230 still green). Also: three JSDoc blocks that mid-file insertions had stranded on the wrong symbols, and a paragraph promising a window fallback for entries without `planMtimeMs` that the code stopped honouring when the fallback was removed — two contradictory contracts in one file, with the doc's version being the one that loses a resume its evidence. * fix(review): compare the plan-mtime fence within a millisecond, not exactly Linux CI caught what macOS hid. Restoring the plan's mtime through `utimesSync` costs a unit in the last place on ext4 — 1786717283911.999 goes back as 1786717283911.998 — because `mtimeMs` is a double over a nanosecond clock and `utimesSync` takes seconds as a double. Restoring from float seconds rather than a `Date` narrowed the drift from whole milliseconds to a fraction of a microsecond, but the ledger compared EXACTLY, so the run's own plan still read as a different one and every session entry was still dropped. The resume ledger emptied itself on the filesystem the CI runs on. The fence now allows a millisecond. That is orders of magnitude above the representation noise and orders of magnitude below the thing it must still separate: a fresh capture of the same PR rewrites the plan seconds or minutes later, never inside the same millisecond. The two epoch tests compared exactly for the same reason and are now on the same tolerance — and the sub-millisecond one no longer infers the property from a timestamp at all: it writes the ledger entry `fetch-pr` would have written, runs the enrichment, and asserts the entry is still visible to the continuation. That is the consequence the timestamp was standing in for, and it cannot pass on a filesystem whose round trip loses the fraction. * feat(review): expose the ledger's session count without the evidence gate The resume cap reads two counters so that deleting one cannot reset it. The second counter never worked: it came from `priorSessionIds`, which is gated on the calling session already appearing in the resume marker, and that entry is written only after a ruling passes — so at ruling time the ledger term was structurally zero, and deleting `resume.json` reset the cap the ledger was supposed to backstop. The gate protects EVIDENCE — it stops a session that was never granted a resume from reading another attempt's transcripts. A count is not evidence: it says how many times this review has been picked up and nothing about what any attempt did. So the count gets its own ungated accessor, running through the same `readSessions` fences as everything else, and the cap can be wired to it. * fix(review): verify against the CURRENT findings digest, and close the owed pins R1-5, deferred two rounds ago and now closed: `verificationGaps` took the best delivery across every `verify--<digest>` key ever recorded, so a verifier that succeeded against an EARLIER findings list satisfied the floor for a list it never opened. The behaviour predates the resume work, but widening the record set to prior sessions is what made it reachable in practice, so it lands here rather than as the follow-up it was parked as. The keys are narrowed to the newest digest before ranking, dated by the digest's own findings file — shard keys of one digest land within a moment of each other, a previous list's are a round older — and keys with no findings file stay in, because they cannot be dated and also cannot reach `ok`, so they only ever make the verdict stricter. Two invariants acknowledged in earlier threads now have their probes. The empty-current-chat refusal deliberately precedes the prior-session fold — prior events must not vouch for a broken current recorder — and every refusal test predated the ledger, so the faithful mutation (prior sessions excuse the emptiness) shipped green; it now reddens a probe with a healthy prior chat and an empty current one. The rendered "totals include N earlier session(s)" line is asserted on the rendered text, where it can be deleted or crash at print time with every return-value test still green. And the fixture drift named in review is repaired at both call sites: the `runLedger` helper with a dead first parameter, and the layer-audit fixture hardcoding the derived `plan-prompts` name instead of asking `promptRecordDir`. * fix(review): close the round-4 blockers a paginated sweep surfaced Nine Criticals, missed for two rounds because the triage sweep read the first GraphQL page of review threads and this PR crossed one hundred — the finding list below is what a paginated read surfaced. A RETURN is now a fact, not an inference from non-empty text. `finalText` keeps the last non-empty assistant message, which includes progress narrated between tool calls — an agent that said "reading the diff now…" and died carried plausible text that certified coverage and the Step 4/5 floor. `parseTranscript` marks text with tool traffic after it as progress, and every certification consumer (liveRecords, certifies, all four supersession predicates) requires the record to have returned. That last group also closes the probe-proven fail-open where an unreturned verbatim relaunch suppressed an honest `Uncoverable:` declaration and earned the chunk off the told-range presumption, let two honest declarations annihilate into `missingChunks`, and silenced a prior attempt's `Budget gap:` disclosure as a "genuine repair". The layer-audit gate no longer treats NAMING the brief as reading it — a grep whose args contain the path cleared `delivered()` while the auditor never opened its instructions — and its record read now takes the run-epoch fence `readRecordedPrompts` documents as mandatory for history readers: without it, a dead attempt's records beside the stable plan path let a hand-launched stale prompt corroborate a run whose builder never emitted an auditor, a fail-open on the gate's own withhold-only invariant. Retirement receipts classify only from auditors that READ the cumulative findings list their prompt points at: the comparison against known findings is the audit's method, and two skipping receipts retired a chunk on a comparison nobody made. Transcript ownership is now checked for the current session, not only prior ones — a foreign-stamped file planted under the current directory was trusted as current evidence, `since` blind to it (a copy gets a fresh mtime) and the prompt pairing deterministic. And the session-directory lookup applies the harness's own filename sanitizer: the harness writes `subagents/<sanitized>` while the ledger's charset admits dots, so a dotted id read a path that does not exist and every reader silently saw nothing. `appendRunSession` refuses to write an entry when the plan cannot be stat'ed — `readSessions` hard-requires the field, so the entry was a guaranteed-dead write that silently lost the id on the next append's rewrite. `repo-context` captures the plan's identity before the providers run and refuses to write if it moved — a concurrent capture otherwise had this run's stale contents restored under the other run's epoch, whose ledger then passed an exact fence against a plan it never described. Every fix is pinned, and each pin was mutation-verified against the exact regression the finding names. * fix(review): validate ledger entries before the cap consumes them R5-1: all three bounded reads sliced the untrusted array BEFORE validating it, so sixty-four malformed entries at the front consumed the whole cap and hid every real entry behind them — `sessionEntryCount` read zero and the resume cap reset, which is precisely the attack the count exists to survive; the marker's resumes and restarts had the same shape, resetting the once-per-review restart bound the same way. Filter first, cap the survivors. The validation cost the original order was avoiding is bounded by MAX_LEDGER_BYTES — the file cannot hold enough entries for the cheap field checks to matter — while the cap's actual job, bounding the per-entry directory reads consumers pay, is done by capping what is RETURNED, and that stands either way. Pinned from both sides: seventy junk entries ahead of one valid entry count as one, and seventy valid entries still cap at sixty-four. * fix(review): work through the round-3-to-6 suggestion backlog The behavioural fixes. Session identity now folds on the PATH the id becomes — sanitized and lowercased — everywhere at once: dedup, the current-session exclusion, the marker's dedup, and both write-side duplicate checks. Folding on the raw id left every alias the filesystem or the harness sanitizer collapses (case variants, trailing dots, sanitized '.') open as a second session wearing the first one's evidence. Read-time dedup keeps the EARLIEST duplicate rather than the first in file order, which handed an out-of-order hand-written duplicate the session's identity and erased the window between the real start and itself from billing. The resume marker takes the same exact plan fence as the session ledger — the window alone is inexact by its own slack, and a previous run's resumes surviving a rewrite arrive with the cap already spent — and restarts dedupe for the same reason resumes always did. Marker writes stamp the plan mtime and refuse when the plan cannot be stat'ed, like the ledger's own dead-write refusal. Appends refuse to rewrite over a ledger that EXISTS as a regular file but could not be read: rewriting from the empty fallback on a transient fault erased every previously recorded entry, and the guard is keyed on the file's type so the pinned self-healing over planted symlinks stands. The layer-audit gate's record fence is the STRICT plan mtime, not the slacked epoch — record mtimes and the plan's come off the same clock, and the slack would re-admit a dead attempt's records written in the two seconds before a re-capture. The epoch JSDoc now says which artifacts key on which fence instead of claiming one definition covers all. The cost refusal message names the boundary that actually filtered (this attempt's start, on a resumed run). repo-context's three stale comments now match the shipped tolerance fence, and the run-ledger module header claims containment only for the certifying readers — cost is accounting, and the honest claim stops there. Twenty-odd guarantees that could be deleted with the suite green are now pinned, each mutation-verified: the byte and entry caps, the per-session authorization property, currentSessionEntry at all, the charset gate at read WITH a valid fence, both write-side guards observed through the raw file, the marker's schemaVersion/case-dedup/noFollow/swallow properties, the symmetric plan fence, the earliest-duplicate rule, the prior-side ownership and window clamps, the byte-vs-string diff hash (an invalid-UTF-8 buffer, which no string fixture can express), the whole-diff recovery branch (exact count), the key-shaped recovery count, the Step 4/5 refusal by name, the `contributed > 0` guard, two-prior-session folding, the handoff boundary operators, the gate's territory clause and identity filter on fixtures that pass every OTHER clause, and repo-context's write-skip through the real serializer. * fix(review): close round 7 on the ledger — cap order, prior prefix, digest keys Three probe-proven blockers, all in code earlier rounds added. The bounded read capped in FILE order before sorting and deduplicating, so 64 valid hand-written duplicates at the front evicted every genuine entry — and the next append rewrote the file from the filtered survivors, laundering the plant permanently; with 64 distinct entries the just-appended current entry (always file-last) was the one dropped, nulling the cost floor. The pipeline is now sort → dedup → cap, each step defeating the payload the next one cannot. `priorSessionEntries` classified every non-current entry as prior, so a twice-resumed run read as the MIDDLE attempt received its own successor as a "prior session" with an unbounded window — its later unrelated activity folded into this attempt's bill, its records entered the evidence pool with no ceiling. Prior now means the PREFIX strictly before this session's own entry, and the last prior's window closes at this session's start. `currentDigestKeys` kept undatable verify keys on the premise that they cannot reach ok — false for the write-failure fallback, whose inlined list leaves no findings file and no pointer, making the findings-read floor vacuously true. A stale pointerless verifier could vouch for a newer dated list no verifier opened. Undatable keys are now dropped once any dated key exists; with no dated key at all they are the only evidence and stay. `sessionEntryCount` gains an exclude-current option for the cap's ledger term (consumed by the stack's fetch-pr): counting the session's own entry in either term refuses a same-session retry of the last permitted resume, whose fresh fall-through then destroys the very state being resumed. * fix(review): work through the round-7 suggestions on the ledger PR Code: the recovery count's uncoverable veto is chunk-scoped like the walk's (a recovered whole-diff auditor legitimately QUOTES the declarations it audited); the layer gate delivers only against reverse-audit records (a concatenated launch verbatim-matched a sibling role's record and satisfied the brief bar with the sibling's brief); retirement extracts the findings pointer ONCE from the raw prompt (the re-extraction from trim-normalized lines defeated the anchors that reject indented quotations) and the orphaned memo-contract JSDoc is back on findingsListFor; the endsAtMs doc and the diffSha256 comment now say what the code does at this commit. Fixtures: the flake family is closed — the three suites' prior-session helpers backdate their files ten seconds, so a CI stall between the ledger stamp and the write can no longer fence the fixtures out through the until clamp. And a dozen probes that could not discriminate the guard they name now can: the over-budget ledger carries a valid entry, the symlinked target carries a fenced entry, the marker's charset gates are pinned on both sides with valid fences, the alias fold's entries sit inside the window, the marker's plan fence survives a 3s nudge instead of dying on the epoch window, resumeAuthorized folds case, the stale gate record sits INSIDE the would-be slack, the identity anchor refuses a substring mention, a delivered auditor with zero diff reads stays refused, the digest narrowing accepts the compliant current digest, per-record granularity is pinned with a mixed prior session, the prior-side floors and the per-session ceiling have events inside their discriminating windows, the plan-swap abort fires on the restored-mtime rename shape, and the epoch test asserts the rewrite's content. * fix(review): round-9 blockers — single-read ledger writers, lifecycle-aware returns, floor narrowing run-ledger: the clobber guards decided from a SECOND read, so a transient fault clearing between the two reads let an append rewrite the whole ledger from the empty fallback; all three writers now classify the occupant once and decide everything from that one read, with the plan stat shared between the fence and the new entry. Plant shapes that can never be legitimate state — a directory (EISDIR on every rename, swallowed forever), an oversize regular file (frozen as "state to preserve" for the life of the plan) — are healed by the writers instead of freezing recording. The entry cap keeps the NEWEST end, so a backdated flood of distinct ids truncates itself rather than evicting the genuine entries and laundering the eviction on the next append. transcripts: `returned` now consults the harness's own lifecycle record — the `agent-<id>.meta.json` sidecar — so an agent killed after a text flush (which ends identically to a completed one in transcript content) no longer certifies; thought parts are excluded from the return text, so a thinking-mode agent killed between ROUND_TEXT and its tool calls does not hand its internal reasoning downstream as a verdict; and a transcript whose records carry two different session stamps is rejected whole — the grafted-tail shape that defeated the first-stamp ownership check. coverage: `currentDigestKeys` dates a findings-file-less key by its always-present prompt record instead of dropping it, closing the mirror case where the CURRENT digest's inlined-fallback keys were dropped and a previous round's verifier vouched for a list nobody opened; the same narrowing now applies to the Step 5 reverse-audit floor; and the Uncoverable supersession guard excludes records that themselves declare the same chunk, so two honest declarers no longer annihilate each other into a permanently unreviewable `missingChunks` loop. layer-audit-gate / retirement: both readers now require `returned` before consuming receipts — a died-mid-flight auditor's narration can carry receipt forms — and the gate's `delivered()` refuses a transcript that verbatim-matches more than one reverse-audit record (the concatenated-launch shape whose union territory corroborated layers no walk touched). Retirement's findings-read floor moves INTO the dry branch of `classifyReturn`, so a filed yield from an auditor that skipped the list read keeps its chunk hot instead of vanishing before classification. repo-context: the enriched plan is committed by a temp file that is stamped with the anchor's times BEFORE the rename and identity-checked against the anchor immediately before it — there is no longer an instant where the plan path carries an advanced epoch (the kill-window that permanently orphaned same-run evidence), and a concurrent capture landing during the read+compare+write window is refused instead of silently overwritten. Every fix carries a probe pinned by mutation: reverting each guard in isolation reddens at least one new test. * test(review): give the inode probe an imposter that cannot recycle the inode ext4 reuses a just-freed inode number immediately, so the delete-then-create fixture came back as the same (ino, mtime) identity — indistinguishable by design, and red only on Linux. The imposter is now created while the original exists and renamed over it, so its inode is distinct on every filesystem.	2026-08-16 13:18:25 +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	b744248656	fix(ci): keep a fallback comment when the PR review runner dies (#9255) ... * fix(ci): keep a fallback comment when the PR review runner dies A review job that dies abnormally never reaches its in-job fallback comment step: the runner worker crash in FinalizeJob on the PR #8894 run (EACCES creating under the runner home directory) left the PR with no review and no explanation. - Probe write access to $HOME, $RUNNER_TEMP and the runner root at job start, repair single-directory ownership with the existing sudo pattern, and fail fast with a clear message when repair is impossible instead of burning the review budget to die at finalize. - Add a fallback-comment job on an ephemeral hosted runner that posts the retry guidance whenever review-pr fails. It derives the PR number from the event payload (dead job outputs do not survive a crash) and dedupes on a qwen-review-fallback comment marker plus this run's URL, so the in-job step, the ack comment, and re-runs never double-post. * fix(ci): harden the PR review fallback comment (#9255) Review-round fixes for the fallback-comment defenses: - Probe the actual runner root (three levels above the workspace, not two) and the _diag subdirectory FinalizeJob writes in; a writable parent does not prove an existing subdirectory writable. - Open the fallback gate on authorize/review-config failures too — the incident's trigger can kill those earlier self-hosted jobs first, and a failed dependency marks review-pr 'skipped', which the old gate never matched. Guarded against resolve dispatch runs, which skip review-pr by design. - Author-scope the dedup lookup (resolved dynamically like upsert-bot-comment.sh) so a planted marker cannot suppress the fallback, and fail closed with bounded retry when the lookup or the state check fails instead of fail-open toward duplicates or a green job that never posted. - Skip the stale fallback when the PR head moved, but only on pull_request_target events where the run head is comparable — comment/review runs report main's tip, and posting wins over silence when the comparison is unavailable. - Define the marker once in a workflow-level env and pin all of the above in the workflow test suite, executing the fallback step's real bash against a stubbed gh. * fix(ci): close the fallback-comment gate gaps from round-2 review (#9255) - Exclude comment-driven /resolve runs from the fallback gate: authorize runs on `@qwen-code /resolve` issue comments where github.event.inputs is empty, so the dispatch-only exclusion never fired there and a failed resolve run was misdiagnosed as a dead review recommending the wrong command. - Enumerate precheck-pr and delay-automatic-review failures in the gate: either failure marks review-pr 'skipped' (a transient API 5xx in delay's re-check step, or the fork-PR chain root dying before it posts anything), which the old gate never matched — silence, against its own "a skipped review is as unexplained as a dead one" norm. Both are 'skipped' where they do not apply, so the gate stays closed there. - Anchor the cross-job dedup on the run URL's closing paren: run ids grow digits over time, so the unanchored substring let a later run's fallback comment (id 123450) suppress an earlier run's (id 12345) re-run comment; every marker body renders the URL as [workflow logs](...runs/<id>), so the id is always followed by ')'. - Pin the previously surviving mutants in the workflow suite: the _diag probe guard polarity, the workflow_dispatch disjunction, the ephemeral-hosted-runner placement, and the fallback body's marker-link shape the anchor relies on; add executed coverage for each head-lookup partial failure and for a distinct run's fallback not suppressing this run's comment. * fix(ci): close round-3 review gaps in the fallback-comment defenses (#9255) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com>	2026-08-16 11:36:37 +00:00
Shaojin Wen	a9bff6c9b8	feat(autofix): defer verified out-of-footprint findings to a surviving follow-up queue (#9189) ... * feat(autofix): route verified out-of-footprint findings to a surviving follow-up queue Anti-drift closure for the review loop: a finding that is REAL but whose fix lies outside the PR's footprint previously had only wrong outcomes — implement it (scope drift), decline it (the finding is lost when the PR merges and nobody re-reads its threads), or push it onto a maintainer. - SKILL gains the fourth disposition, Defer to follow-up: verified + out-of-footprint → record {id, path, reason} in deferred-findings.json, reply on the thread that it is deferred, leave the thread open. Decline stays for what is not worth doing anywhere; defer is for what is worth doing elsewhere. - The report step upserts these into one per-PR 'Deferred review findings' issue (marker-keyed, append-only by rc id, agent text token-neutralized and length-capped), for BOTH pushed and no-op outcomes. Best-effort: an upsert failure never fails a round. Deliberately no ready-for-agent label — feeding the bot's own deferrals back into its issue queue is a human authorization. - deferred-findings.json rides the artifact dump and the repair cleanup; the neutralization ledger grows to ten sites. - Tests: shape validation (non-empty array of numeric-id items), line building (dedupe by rc id against the existing issue body, newline flattening, truncation), and wiring pins for both call sites. * fix(autofix): rebuild the deferred-findings upsert per review round 1 - Extracted to a trusted staged script callable from ALL outcome paths — the failure/handoff path persists verified findings too (a failed round's commit dying says nothing about the findings' validity). - Append-only durability: the tracking issue's body is written once; every later round POSTS a comment — no read-modify-write can race a maintainer's edits, and a failed body/comments read SKIPS the round (never mistaken for empty history). Success is logged only when the write call succeeded; failures say NOT persisted. - Structured lookup: jq filtering over the real bodies (no line-joined awk under pipefail), pull requests excluded, lookup failure skips rather than creating duplicates. - Dedupe is line-anchored ('- rc:<id> ' at line start, body+comments corpus) with intra-batch unique_by; ids the round resolved in code are excluded (a finding cannot be implemented and outstanding at once). - Shape gate covers path (string when present); path bytes are charset-sanitized so a crafted path cannot forge queue bullets. - Publication-trust posture recorded: the deferred lines are the same agent-authored trust class as every other published output — marker neutralization, mention-free sanitized charset, length caps, and a 20-item batch cap bound the surface. - Tests: the real script runs against a recording gh stub — create, append+dedupe (body and comments), PR-carrying-marker exclusion, anchored dedupe vs free-text mentions, read-fail skip, resolved-exclusion, shape-gate loudness, write-fail honesty, and forged-path sanitization; the neutralization ledger returns to nine workflow sites with the script-side tenth pinned in place. * fix(autofix): harden deferred-findings upsert per review round 2 - Pass the known-id corpus to jq via --rawfile: a large corpus in one --arg argv element hits Linux MAX_ARG_STRLEN and the swallowed exec failure would silently drop the round's deferrals. - Digest-gate the staged upsert script: record upsert_sha256 at stage time (expression context) and verify before each of the three invocations; RUNNER_TEMP is agent-writable in between. A mismatch skips persistence, never the round. - Add the gh hygiene preamble (GH_HOST pin, GH_TOKEN unset, fresh GH_CONFIG_DIR) to the review-address failure/handoff report step — the one PAT-bearing gh step that lacked it. - Query the tracking-issue lookup with state=all so a maintainer-closed issue is appended to instead of forking a duplicate. - Enforce integer positive finding ids in the shape gate (a float id's dot is a regex wildcard in the anchored dedupe and never index()-matches resolved ids). - Clip the 20-item batch loudly and qualify success messages with kept/total counts instead of claiming full persistence. - Tests: digest + hygiene wiring pins; stub knobs for list/comments fetch failures; append-write failure, multiline-reason flattening, bad-id, loud-cap, and state=all cases. * fix(autofix): close bash/transport channel gaps and failure-path gate holes (review round 3) - Sweep BASH_ENV/ENV and imported BASH_FUNC_*%% functions plus proxy (HTTPS_PROXY/HTTP_PROXY/ALL_PROXY + lowercase) and SSL_CERT_FILE/DIR at all four gh-hygiene sites: both families are GITHUB_ENV-plantable and bypass the TRUSTED_PATH pin (child-bash startup) or reroute/ decrypt PAT-bearing HTTPS. Also de-shadow gate-critical names and hash -r, since a planted BASH_ENV runs before the step body. - Pin PATH to the staged trusted value (guarded for pre-stage crashes) and drop the loader trio in the failure/handoff report step — its digest gate previously ran under ambient PATH/LD_PRELOAD. - Failure-path upsert: skip with a plain notice when stage never ran (empty digest is not a tamper alarm), and verify the PAT's bot identity before writing (POST_HANDOFF's check is skipped on the fixed/noop-outcome path); correct the guard comment that claimed parity with the handoff guards. - Fold the twice-pasted digest-gate + upsert block in 'Push and report' into a step-local run_deferred_upsert(), matching the resolve_and_reply_threads convention. - Dedupe corpus reads bot-authored comments only, so a third party commenting on the public tracking issue cannot suppress a finding. - Tests: hygiene sweep pins ordered before the first gh call; placement assertions (function defined once, called after both resolve arms; failure invocation inside the DRY_RUN/STALE/token guard slice); digest/identity/notice pins; behavioral cases for foreign-author suppression, intra-batch duplicate ids, markerless-issue create path, creator/marker anchors, and marker neutralization through the append path. * fix(autofix): isolate deferred-upsert in a clean env -i child, drop the unsound in-shell sweep (review round 4) Round 4 (R4-1, five Criticals of one class) showed the in-shell BASH_FUNC/proxy denylist sweep the prior round added is unsound: it bootstraps trust from the very shell namespace it sanitizes, and a planted BASH_FUNC_env%%/unset%%/command%%, an expand_aliases alias, a readonly -f shadow or a DEBUG trap each defeat it — ending in the staged upsert script executing with CI_DEV_BOT_PAT in env. - Replace it with sound isolation: both upsert sites (run_deferred_upsert in 'Push and report', and the failure/handoff path) run the digest gate, the PAT identity check and the staged script in a fresh '/usr/bin/env -i … bash --norc -c' child. /usr/bin/env is invoked by absolute path — bash never does function/alias lookup on a slash-bearing word, so a planted BASH_FUNC_env%% cannot intercept it — and env -i drops every BASH_FUNC_*, BASH_ENV, SHELLOPTS, alias and trap before any gated work. GH_CONFIG_DIR is minted inside the clean child (its mktemp cannot be shadowed there), closing the mktemp-shadow hole in the failure step's preamble. - Remove the sweep and the in-step gh/PATH preamble the prior round added to all four PAT gh steps; the three pre-existing steps revert to their prior posture. Hardening the pre-existing PAT gh calls (handoff/report comment, push, publish) against BASH_FUNC/transport plants is noted as separate, out of this feature's scope. - Script: accept a contract-valid empty array as a clean no-op instead of a false 'malformed' alarm; 'set +C' so a planted read-only SHELLOPTS=noclobber cannot silently empty the dedupe corpus (belt to the env -i child that already drops SHELLOPTS). - Tests: replace the sweep pins with clean-child pins (absolute-path env -i at both sites, GH_CONFIG_DIR/PATH inside the child, failure launch inside the guard slice); add no-findings exit-0, empty-array no-op and set +C cases. Behavioral probe: a fully tainted parent (BASH_FUNC/alias/BASH_ENV plants) cannot reach into the env -i child. * fix(autofix): strip LD_* before the env -i upsert child; tighten id gate, mktemp/cap guards (review round 5) - R5-1 (Critical): LD_PRELOAD/LD_AUDIT/LD_LIBRARY_PATH is the one channel env -i cannot block — ld.so maps a planted library into /usr/bin/env itself at execve, before -i wipes anything. Neutralize it with a command-prefix assignment (LD_PRELOAD= LD_AUDIT= LD_LIBRARY_PATH=) before /usr/bin/env at both upsert launch sites: a pure shell parameter assignment no BASH_FUNC can shadow, applied to env's own environment. Probe: a parent LD_PRELOAD=/evil.so no longer reaches the env binary or the child. - R5-3: shape gate rejects integer-valued floats jq renders in scientific notation past 2^53 (1e21 -> "1E+21") — the '+' is a regex-active byte in the anchored dedupe. Add a <2^53 bound and a tostring plain-digits belt. - R5-4: guard mktemp failure (a known /tmp-exhaustion CI state) so it warns and skips instead of a silent exit 0 that violates the header contract. - R5-2: the cap warning no longer promises 're-defer in a later round' (impossible — the eval-watermark filters evaluated feedback out permanently); it names the dropped bullets for a maintainer. - Tests: LD prefix, GH_HOST-in-child and gate/exec ordering pins; tie the two near-verbatim clean-child bodies together (R5-6); source pins for both --paginate sites (R5-5); if-!-echo gate-condition pin (R5-9); failure step added to the sweep-removal regression loop (R5-11); behavioral cases for sci-notation id, mktemp failure, prefix-colliding dedupe boundary (R5-10) and the reworded cap. * fix(autofix): defuse mentions in deferred bullets, verify child liveness (review round 6) - R6-9 (Critical): the reason is agent-influenced prose published under the bot identity, so a raw @ fired real mentions from the tracking issue. Defuse before rendering: @ gets a trailing ZWSP, and the entity spellings GitHub decodes BEFORE its mention filter (&#64; &#x40; &#0064; &commat;) get their & escaped. Both measured inert against the real renderer; \@ and leaving & alone are not. Paths were already charset-reduced. Byte-exact probe: 2/2 @ defused, all four entity spellings escaped, no raw spelling survives. - R6-8: LD_* cannot be enumerated — LD_TRACE_LOADED_OBJECTS is presence-tested, so even the empty prefix assignment leaves trace mode on and /usr/bin/env prints its libs and exits 0 without ever running the child (probed). Verify the RESULT instead: the child prints a liveness sentinel first and its absence is reported. The inspection uses bash builtins only — an external grep would itself print-and-exit-0 under trace mode, neutering the check (measured: the first grep-based attempt failed exactly this way). - R6-4: guard the child's own GH_CONFIG_DIR mktemp; an empty value falls back to the shared ~/.config/gh. - R6-6: a line-builder (jq/sed) failure warns instead of exiting silently as 'nothing new' — the last path skipping the header contract. - R6-3: write-failure warnings say the findings are LOST (watermark- gated, never retried) and name the bullets, matching the round-5 cap wording fix. - Tests: allow-list entry pins (R6-7), sentinel/builtin-inspection and child-mktemp pins, identity-check ordering (R6-5), plus behavioral cases for mention defusing, reason-type and id-0 gate clauses (R6-2) and line-builder failure. * fix(autofix): defer findings from all three feedback sources; carry them across repair (review round 7) - R7-1 (Critical): only inline comments carried an id in feedback.md, so a verified out-of-footprint finding raised in a review body or an issue-level PR comment could not be deferred and was lost at merge. Feedback now renders [rv:<id>] and [ic:<id>] alongside the existing [rc:<id>], the record takes an optional "source", and bullets anchor under a per-source prefix so id spaces cannot collide. The resolved-id exclusion stays inline-only (that is what resolved-comments.txt holds). SKILL documents all three sources and that only inline findings have a thread to reply on. - R7-2 (Critical): every abort path said "skipping ... this round", implying a retry that cannot happen (the eval watermark filters this round's feedback out of every later round and the next reset wipes the file). All six now report the findings as LOST and dump the raw deferrals for manual recovery, with :: neutralized — the dump is agent-influenced and a raw :: at line start is a workflow command. - R7-3 (Critical): 'Repair deterministic rejection' deleted deferred-findings.json before any upsert site ran, so run 1's deferrals died in a repaired round. It now carries them into a sidecar the upsert unions in (merging if an earlier repair left one), and the sidecar rides the artifact dump. A/B probe: base arm loses run 1's deferral, fix arm persists both. - R7-4: a present-but-non-string path (false) passed the gate because // treats false as absent; the gate now tests .path|type directly. - R7-5: LD_PROFILE/LD_PROFILE_OUTPUT/LD_DEBUG/LD_DEBUG_OUTPUT are non-blocking loader file-write channels the liveness sentinel cannot catch, so they join the command-prefix neutralization (probed inert when empty). - Tests: per-source anchoring and dedupe, unknown-source rejection, path:false, carry-only and carry-merge cases, LOST dump with :: neutralization, plus wiring pins for the carry, the feedback ids and the extended LD prefix. * perf(autofix): bound the deferred-issue lookup and name gh failure causes Clears the two backlog items the review has re-raised every round since round 2 (R2-6, R2-10); both live in the file this PR adds. - R2-6: the tracking-issue lookup ran a full --paginate over every issue the bot has ever opened, on every round that defers anything, keeping only the first match. It now walks newest-first pages and stops at the first marker match: one request in the common case, a short page ends the scan (corpus exhausted -> create), and a 10-page cap bounds the worst case. Reaching the cap without a match SKIPS rather than opening a second tracking issue for the same PR. - R2-10: every gh call discarded stderr, so a rate limit, an expired PAT, a transport error and a 404 rendered identically in the feature's only signal. All five calls now capture stderr to one sink and the warnings name the cause, :: neutralized like every other echoed API/agent content. Measured on the shipped script with a recording gh stub: first-page hit 1 request, empty corpus 1 request + create, page-2 hit 2 requests, cap 10 requests + skip, and the 403/401 bodies reaching the warning text. * test(autofix): close the pin gaps the round-8 mutation sweep found Round 8 raised 15 findings, none Critical: two behavioural, the rest test pins the reviewer proved vacuous by mutation. - R8-3: the carry-merge failure branch discarded THIS run's deferrals with no raw dump — the one loss path in the feature without recovery output. It now prints the set (:: neutralized) before deleting it. - R8-1/2/5/6/7/8/9/10/11/12/13/14/15: pins that survived their own mutations. Notably: the negative sweep pins now assert the PROPERTY (no BASH_FUNC / unset -f / hash -r in non-comment lines) instead of round 3's exact spelling; allow-list entries must sit inside the env -i argument list, not merely somewhere in the step; the identity check is pinned whole so a fail-OPEN mutation cannot pass; the repair cleanup's deletion pin is spelling-independent and allows exactly the one delete that follows a merge; the staging pins are scoped to the stage step with cp ordered before the digest record; and the script's <!-- escape site gets the count+canonical treatment its workflow siblings already had. Each new pin was mutation-verified: 8/8 injected regressions turn the suite red (differently-spelled sweep, relocated allow-list entry, deleted GH_CONFIG_DIR export, fail-open identity check, re-added cleanup deletion, deleted re-print loops, ascending lookup order, no-op sed spelling). * fix(autofix): close the upsert TOCTOU and the per-id deferral collapse (review round 9) - R9-1 (Critical): the digest gate was check-then-use — sha256sum read the staged path and bash re-opened it, two opens of a path this PR itself calls agent-writable. The child now reads the script ONCE and runs those exact bytes (bash -c "$UPSERT_SRC"), so the bytes hashed are the bytes executed. A/B against an inotify-driven same-user rename(2) watcher: old shape 20/20 payload executions with the gate never firing, new shape 0/20 (legit 20/20, control 20/20). - R9-2 (Critical): unique_by([source, id]) collapsed DISTINCT findings sharing one review-body or issue-comment id — the two sources this PR adds — and reported success while losing them. Dedupe identity and the corpus check are now per rendered line for those sources (inline comments keep id identity and the cross-round anchor). Probe: two findings under one review id now both persist ("3 of 3 new"), byte-identical records still collapse, inline behaviour unchanged. - R8-4 (re-raised): fixed structurally instead of by the suggested prefix entry, which is a no-op — LD_SHOW_AUXV is presence-tested, so an empty assignment still dumps 22 auxv lines (measured; env -u does not help either). Loader side channels write to the LAUNCH process's stdout, so that stdout is discarded and the child logs to a private file; path and read-back are fork-free ($$ expansion, $(<file)) so a polluted parent cannot leak noise into the value. Measured: with LD_SHOW_AUXV planted the log holds 0 auxv lines and the child still runs; with LD_TRACE planted the sentinel is absent and the warning fires. - R9-3/4/5: the manual-recovery dumps now say when they truncate, and name the full byte count. - R9-11: the artifact dump neutralizes :: in the agent-written files it prints, like every other echo of them. - Tests: pins for the single-read exec, the private log, the fork-free parent handling, the identity check's ENFORCEMENT (R9-9), an allow-list that must hold ONLY the sanctioned entries (R9-10), the sentinel comparison inside the re-print loop (R9-13), and R9-2's multi-finding cases. Also fixes an argList slice that anchored on a comment mention and silently widened to the whole step. * fix(autofix): keep a poisoned carry from sinking the round; clear the pin backlog Clears the eleven items carried from round 9. - R9-18: a carried sidecar that PARSES but fails the shape gate used to abort this round's valid deferrals too — asymmetric with the unparseable-carry branch, which persists this round only. The gate is now a function applied to the merged set, with a retry on this round's own file; the carry is dumped and named LOST. Measured on the real script: valid own + gate-invalid carry -> own persisted, carry dumped; valid own + unparseable carry -> own persisted; invalid own -> loud total abort, nothing written. - R9-20 / R9-7: the union argument order IS the freshness guarantee (jq unique_by keeps first-of-group in original order), pinned at both sites; measured: a duplicate id keeps this round's text, not the carried one. - R9-3/4/5 follow-up: the three truncation dumps became one dump_file helper instead of a fourth copy. - Pins: --paginate anchored to the comments call (R9-8), the stale two-sites comment corrected (R9-6), the no-in-shell-sweep property widened past function-unset spellings to alias/trap/proxy forms (R9-12), the repair cleanup's deletion pin extended to rm -rf and to any second multi-line list (R9-16), runUpsert's spawnSync bounded like its sibling harness (R9-17), the explicit review_comment spelling covered (R9-19), and the 20-item cap's survivor set pinned from a MEASURED run whose sort order and input order disagree (R9-14) — the four records written first are the ones dropped. Mutation-verified 6/6: relocating --paginate, an alias-form sweep, an rm -rf deletion, either union order swapped, and dropping the poisoned-carry fallback all turn the suite red. * test(autofix): widen the denylist-sweep guards past their word-boundary hole `\btrap -\b` cannot match `trap - ERR EXIT`: the boundary sits between `-` and a space, both non-word characters. The same hole was in two sibling guards — `\bunset -f\b` misses `unset -fv name`, and `\bhash -r\b` misses any suffixed spelling. Drop the trailing boundary on all three. Mutation-verified: injecting `trap - ERR EXIT INT TERM`, `trap -- EXIT` or `unset -fv sha256sum` into a PAT-bearing step now turns the suite red; each passed before. * fix(autofix): two -e-fatal paths, an escape-order dedupe hole, and a rewording duplicate (review round 10) Six Criticals; four were defects this PR introduced. - R10-19 + R10-22 (Critical): the PAT steps run under 'bash -eo pipefail' (defaults.run.shell: bash). Measured: 'rm -f' on a planted DIRECTORY at the predictable log path exits 1 and kills the step, ': >' onto one likewise, and $(<missing) is fatal in a way NEITHER '|| true' NOR 'if !' rescues. Creation is now 'rm -rf' + '(set -C; : >)' with a warn-and-skip, and the read-back tests -f/-r first while staying fork-free. Probed all four planted shapes (fresh/file/symlink/dir): the step survives each. - R10-5 (Critical): the <!-- neutralization ran in a sed AFTER the jq corpus comparison, so an rv/ic line carrying <!-- compared its RAW rendering against the ESCAPED stored form — never matched, republished every round. Escaping moved inside jq, before the compare. - R10-17 (Critical): rv/ic identity was the exact rendered line, so any reworded re-emission (routine: the repair flow re-runs the agent) published a permanent duplicate. Identity is now a normalized digest — case-folded, punctuation-collapsed, trimmed, capped — which absorbs phrasing churn while keeping distinct findings apart. The tension with R9-2 is real and resolved deliberately toward a visible duplicate over a silent loss. - R10-1 (Critical): the sweep tripwire is a spelling denylist over an unbounded space. Reframed as what it is — a drift alarm, not the boundary (the boundary is the env -i child, pinned separately) — and aimed at the ENUMERATION PRIMITIVES a sweep needs (compgen -e, declare -x, env pipes, export -n) instead of more name vocabulary. - R10-6 (Critical): the jq stub hardcoded /usr/bin/jq, which does not exist on macOS; it now resolves through the original PATH. - R10-13: the child's log comes from agent-writable RUNNER_TEMP, so :: is neutralized on re-emission via parameter expansion (no fork). - R9-5 leftover: the third truncation dump now names the clipped size. Mutation-verified 5/5 and behaviour-probed 5/5 (escape-before-compare, reworded re-emission, distinct sibling, R9-2 no-regression, planted-path shapes). * fix(autofix): make the deferral identity lossless; keep the unmerged set on disk (review round 11) Two Criticals, both defects this PR introduced, plus the eleven items carried from round 10. - Identity key (Critical): the normalized key stripped every non-[a-z0-9] byte and capped at 160 chars, so CJK siblings collapsed to one key (this repo is bilingual) and a long path pushed the reason out of the identity entirely — silent loss, the exact outcome the feature exists to prevent and the opposite of what its own comment claimed. The key now normalizes case and PUNCTUATION only, keeping every letter of every script and no cap, at both the build and corpus sites. Rewording tolerance is unchanged. Probed: 2 CJK siblings -> 2 of 2; 2 siblings on a 200-char path -> 2 of 2; reworded duplicate -> 1 of 1. - Repair merge failure (Critical): the branch deleted deferred-findings.json before 'Show run artifacts' and the artifact upload ran, so its own pointer at the artifact dump was false past the 4000-byte clip. It now renames the set to deferred-findings.unmerged.json (kept in WORKDIR, added to the dump list) on the failure path and deletes only on the merge-success path. Probed: 6245 bytes preserved where the dump clipped at 4000. - R10-12: neither side is known to be the corrupt one (jq -s fails if either input is unparseable), so both merge-failure warnings say that instead of blaming this round. - R10-18: a second identity anchor — the derived title — so an edited body that loses the marker no longer orphans the issue into a duplicate; the marker still wins when both are present, and a same-titled PR is still never adopted. - R10-3: the carry branch is unreachable in today's topology (WORKDIR is wiped at run start, one repair step); kept as defensive with that stated. - R10-10: the builtins-only discipline is scoped to the child-output INSPECTION, which is what it always meant. - Pins: delimited-token allow-list incl. UPSERT_LOG's value (R10-7), every respelling of executing the staged path (R10-8), the tripwire extended to the issue-autofix failure steps (R10-11), launch-line anchoring in both steps (R10-15), flagless rm counted (R10-16), and behavioural cases for the 200/500 caps (R10-4), CJK and long-path siblings, and the title anchor. Mutation-verified 7/7. * refactor(autofix): remove the agent-writable paths the upsert depended on (review round 12) Rounds 9-12 each closed one hole in a design that read the staged script from an agent-writable path and buffered the child's output through another. Round 12 found four more of the same class (TOCTOU on the log reopen, a plantable FIFO and an unbounded read on each path). Rather than patch a fifth time, remove both paths. - The script travels as CONTENT: the stage step captures it from the trusted checkout into a step output (random heredoc delimiter), and the clean child runs `bash -c "$UPSERT_SRC"`. With no agent-writable copy there is nothing to verify — the digest gate, its check-then-use window, the staged cp and the FIFO/huge-file read all disappear. - The child's messages travel on fd 3, which the parent captures, while fd 1/2 are discarded. Every loader side channel writes there, so the noise still cannot reach the parsed output — and there is no log file to plant, race, bound, or clean up. Probed with LD_SHOW_AUXV and LD_TRACE planted: clean output, sentinel behaviour unchanged. - RC-1: resolved-comments.txt went to jq as one argv element, the exact MAX_ARG_STRLEN failure the neighbouring comment describes and that `known` already avoided. Both corpora use --rawfile now. Measured on a 348 KB corpus: the old form dies with "Argument list too long", the new one publishes normally. Net -113 lines, and the pins follow: no-path invariants replace the digest/log battery. Mutation-verified 6/6. * fix(autofix): reject multi-document deferral files; survive a base without the script (review round 13) - Multi-document JSON (Critical): `jq -e` without -s evaluates each document in turn and its exit status reflects only the LAST, so `[valid]\n[]` exited 0 silently (findings lost, no warning) and `[bad-id]\n[valid]` passed the shape gate outright. A single_doc gate now runs first, with the asymmetry the earlier rounds settled on: a bad OWN file is a total abort, a bad CARRY costs only the carry. - R13-7: the stage step reads the script from the TRUSTED BASE, where it does not exist until this PR merges — under -e that killed every pre-merge pull_request-triggered round (true of the old cp too, so this has been latent since the script was added). It now tolerates the absence and lets the consumers' own empty-content guard skip the round. - R13-2: the carry union requires both inputs to BE arrays; `add` on two non-arrays yields whatever they add to. - RA1R4-B: the resolved-corpus test is -f/-r, so a directory or FIFO at that path is treated as unusable rather than present. - R13-1: nine rationale comments still described the staged copy and the digest gate that round 12 removed. Probed: both multi-document shapes are rejected loudly with zero writes; a bad carry still leaves this round publishing 1 of 1. * docs(autofix): retire the last stale rationale comments (R13-1) Six of the nine locations were in the test file: comments still describing the digest gate, the staged copy and the read-once invariant that round 12 removed. Same honesty issue as their three workflow siblings. * fix(autofix): strip BSD wc padding; let wrapper warnings stay annotations (review round 14) - BSD wc (Critical): `wc -l` pads its count with leading spaces on macOS, and TOTAL_NEW is interpolated into the cap warning and the success line — the sibling `wc -c` already stripped it, this one did not. Tested with a padding `wc` stub, since GNU wc never pads and the regression is invisible on Linux CI otherwise. - The re-emit loop demoted the feature's own failure signal: every `::` became `;;`, including the wrapper's trusted messages. Wrapper-authored lines now carry a marker and are emitted VERBATIM (so they render as annotations again); the script's output, which interpolates agent content, stays neutralized. - \b on `declare -x`/`export -n` had the same word-boundary hole already fixed for `trap -`/`unset -f`. - Pins: the heredoc CLOSING delimiter, the merge-failure quarantine rename, and an allow-list comparison that is a sorted multiset rather than a Set — a symmetric same-name addition is exactly what that check exists to catch and a Set hid it. Mutation-verified 5/5. * fix(autofix): un-truncate the sibling identity; align the carry precedence (review round 15) Zero Criticals this round; five behavioural findings among the pins. - The rv/ic intra-batch identity was derived from the RENDERED line, i.e. after the 500-char reason cap, so two siblings differing only past the cap collided and one vanished silently — the same silent-loss class as the CJK and long-path entrances. It now comes from the uncapped path+reason; the corpus check still compares rendered forms (that is all the issue stores), so cross-round the cap can cost a duplicate, never a loss. - The repair carry union put the OLDER set first, inverting the newer-wins precedence the script documents for its own union. - The resolved-id parser dropped any line with stray surrounding whitespace, so a padded `rc:<id>` no longer suppressed its finding. - The clean child's catch-all warning lacked the trusted marker added last round, so the feature's most common failure message was still demoted out of annotation form. - The truncation notice pointed at the artifact dump even when the dumped file is a merge temp outside WORKDIR, which is never uploaded. Pins: the stage step's `id: 'stage'` (the link whose break empties every UPSERT_SRC), the empty-content skip branch, the capture's `|| true`, and the trusted marker on every warning inside the child. Mutation-verified 6/6.	2026-08-16 11:28:05 +00:00
Shaojin Wen	80e825c54c	fix(review): fix silent reverse-audit retirement failures and keep non-converged evidence (#9213) ... * fix(review): name reverse-audit certification failures, keep non-converged evidence (#9206) A round-5 reverse audit (PR #9118, 12 chunks) never retired a chunk: four territories returned substantive dry receipts in BOTH rounds 1 and 2, yet rounds 3-5 rebuilt all 12 auditors, no retirement note appeared, and nothing anywhere said which certification condition refused the receipts. Step 9 cleanup then deleted the prompt-record directory of the non-converged run, so the failure could never be diagnosed. Two root causes, reproduced end to end (issue-9206-repro.test.ts): - The dry-receipt separator class admitted dashes and colons only, but honest receipts arrive separated by sentence punctuation too — "No new issues were found. Re-walked …" and "未发现新问题，重新走查了…" both reproduce the never-retire loop byte for byte. The clause after the separator is the part that proves the walk; the separator only has to show it exists. Widen the class to period, comma and semicolon in either width. The substance floor, the tool-call bars and the territory bar are unchanged — the bare stock sentence still reads unknown, and a yield still outranks any receipt. - Every certification refusal landed in the same silent unknown, and the schedule's catch swallowed every exception without a word. classifyReturn now reports the first bar that fell; the schedule carries one diagnostic line per twice-audited chunk that neither retired nor yielded, and the builder prints them on stderr (stdout stays the deliverable). The catch names itself: a round whose transcripts cannot be read audits every chunk AND says so. And the evidence half: cleanup swept the record directory of a non-converged run unconditionally — the round-cap marker that proves the non-convergence sat inside the very directory deleted. A record directory holding a same-run stop marker is now kept, with a note naming it; a converged run clears its marker, so its history sweeps as before. * fix(review): refuse hedged receipts, keep cross-run audit evidence (#9213) The widened receipt separator admitted clauses that contradict the no-issues phrase ("…found, but I could not open the files") and let a quoted phrase open a clause out of its own negation, retiring chunks on their auditor's admission that nothing was checked. Judge polarity on every separator path and admit sentence-punctuation separators only when the phrase leads the return. Cleanup's retention keyed on the run-epoch-fenced marker reader, so a previous run's preserved evidence was swept by the next run — and clearBudgetStop unlinked any marker, including a previous run's that retention had just kept. Retention now reads the marker unfenced and also keeps record directories carrying files older than the plan's own capture (a killed run leaves no marker); convergence clears only its own run's marker. The per-chunk build path now prints its chunk's certification-failure diagnostics like the round builder does. * fix(review): diagnose every chunk build, invert the polarity burden (#9213) The per-chunk gate stamped the round on its first chunk build and gated the certification diagnostics behind that stamp, so chunks 2..N of a one-auditor-at-a-time round re-audited in the exact silence the note exists to end. Print the chunk's diagnostic on every build of the round; only the convergence and budget rulings stay gated on admission. The polarity guard enumerated contrast words over unbounded prose, and prose has no last hedge — though/Yet/unfortunately/只是 all retired chunks on clauses admitting nothing was checked, while a hedge in the phrase's filler never reached the clause-only test at all. Invert the burden: a clause carrying a negation or incapacity marker contradicts the phrase whatever its length, and a contrast word without one contradicts nothing — an innocuous "but I re-verified" retires again, and the guard's remaining gaps land on the audit side. The anchored receipt matcher now leads (the unanchored one truncated clauses at nested phrase occurrences), the judged text is re-trimmed after the budget-gap strip, and cleanup keeps a record directory whose plan a previous cleanup already swept and skips unstatable entries one by one instead of letting one veto the previous-run evidence. * fix(review): line-scope the polarity guard, case-blind parrot, safe notes (#9213) The polarity guard read only the match's own prefix and the clause, so a hedge BEFORE the phrase on the same line was invisible and retired the chunk (`I could not check everything, but no new issues — re-walked…`), and its marker list missed the incapacity/omission families the leak probes executed — unable/failed/skipped/unchecked/skimmed, zh bare-不 and 跳过. Scope the guard to the LINE the receipt matched on — the match plus any same-line text before the phrase, quoted spans exempted — and name the executed marker families. Prose on any other line neither certifies nor contradicts: scanning the whole return would refuse an honest receipt beside an innocent "not re-reporting it" paragraph. The substance floor now measures the phrase-STRIPPED clause (greedy with the phrase's filler), so an echoed phrase cannot lend the floor its length, and the comment's direction claim is corrected — a marker the list misses inside the line still fails toward RETIREMENT, stated rather than papered over. The parrot refusal compared case-sensitively, but the example clause starts lowercase only because it continues the model receipt mid-sentence; the widened sentence-punctuation separators let a parroting auditor open it as a NEW sentence and capitalize it. Compare in any casing — no honest clause contains the model clause verbatim in any casing. The three new informational stderr writes — both schedule catch NOTEs and the uncertified-chunks note — used the throwing writeStderrLine on the CONTINUING build path. A headless retry with stderr redirected or closed (the very #9206 shape) made the write throw out of a catch with no outer guard, abandoning the round that must audit every chunk. Use the Safe writer, matching writeFindingsFile. * fix(review): close the polarity guard's unbounded class with one receipt form (#9213) The polarity guard enumerated hedges over unbounded auditor prose, and every hedge the list missed failed toward RETIREMENT — the direction the module header declares impossible. Round 5 executes the structural fix the round-4 Critical named: one machine-parseable receipt form, and everything else read as unknown. The form: the phrase LEADS the return (anchor; the lead CONTRAST list is deleted), the receipt line stands ALONE (Budget gap: and Layer walked: lines stripped with audit-layers' own matcher; any other prose before or after reads unknown, named for the side it fell at), and the clause NAMES THE WALK (a verb from the form's vocabulary, or a named object — misses fail toward audit, the opposite direction of a marker miss). Within the form the marker test is repaired, not extended: the strip uses the phrase core without the greedy tail that swallowed its own listed markers, and the quoted-span exemption that blanked a self-admission is deleted. Zero words added to the vocabulary. Also closes R4-1: the clause capture ran to the END of the return, so prose after the receipt line contradicted the phrase while identical prose before it passed. The capture stops at the line; both sides now read unknown, symmetrically named. The reverse-audit brief mandates the form so compliant dry returns stay one line and keep retiring. * fix(review): line-bind the dry-receipt matcher and cut fused layer labels (#9213) --------- Co-authored-by: qwen-code-ci-bot <qwen-code-ci-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-16 11:16:23 +00:00
Shaojin Wen	c7d496c558	fix(review): normalize last-gate inputs and anchor mid-line fragments (#9222) ... * fix(review): normalize last-gate inputs and anchor mid-line fragments The /review pipeline's final gates rejected the shapes its own earlier stages produce (#9209), failing hours-long runs at the finish line: - findings normalizes the bracketed Source: tags the finding format mandates ([probe] -> probe) instead of hard-erroring - findings --to-anchors writes the Step 7 resolver input straight from the canonical artifact (one {id, path, anchor, line?} per anchored location, aggregates expanded to <id>-N), replacing the hand-written projection from locations[]; expanded-id collisions are refused at projection time instead of failing the whole resolve batch - compose-review/submit accept the array form of suggestionsDiscarded, counted by length - the anchor resolver gains a substring-of-hunk-line tier so a verbatim fragment of a KB-long Markdown line resolves to the containing line (>=12 chars, single line only, ambiguity refused or claim-decided); SKILL.md documents the tier and the finder briefs gain the off-diff file anchor strategy * fix(review): refuse expanded-id collisions with every finding id Review of the projection and substring tier found the guards narrower than their promises: - the expanded-id collision guard only compared minted request ids, so a minted <id>-N equal to another finding's own id passed silently whenever that finding emitted no bare-id request (aggregate, low-confidence, anchorless, or Nice-to-have); seen is now seeded with every finding's id, self-match exempted - the projection runs before the findings artifact is written, so a refused rerun leaves the previous consistent findings/anchors pair on disk instead of v2 findings beside v1 anchors - anchorless locations of postable findings are named on stderr — nothing downstream cross-checks the artifact against the resolver input - a below-floor fragment that IS contained in a hunk line reports a dedicated "too short — quote a longer stretch" reason instead of the false generic absence reason; boundary probes pin the 12-char floor - the substring fallback retries the marker-stripped reading for true mid-line containment, forgiving a copied + marker without letting it double as an indentation guess - SKILL.md's display rule attributes the aggregate expansion to Step 6's findings --to-anchors, not Step 7 * fix(review): keep anchor refusals honest and write anchors before findings * fix(review): state only what the anchor resolver actually guarantees * fix(review): refuse the marker retry beside a live equal line Review round 4 found the substring tier's marker retry could still misplace an anchor, and the count contract's comments misdescribed the prose: - the retry dropped a line equal to the fragment modulo whitespace as a lineGuess, then resolved the fragment to an unrelated containment line at matchCount:1, ambiguous:false — a confidently posted misplacement where the old contract was a loud unmatched; the indentation refusal now outranks any remaining containment candidate while such a line exists, and a regression test pins it (fails pre-fix); the copied-+ marker case with no equal line stays green - SKILL.md scopes the unmatched disposition to standalone findings, adds the aggregate partial-resolution case (discarded and counted once per finding, not per entry), and names both claim-immune multiplicity reasons — the loose tier's "indentation was normalised" alongside the substring tier's "whitespace is normalised" - the three suggestionsDiscarded comments now say the Step 7 prose prescribes a count and the list form older runs wrote is tolerated, matching the shipped count-first prose * fix(review): split the aggregate all-unmatched disposition by severity --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-16 10:47:37 +00:00
易良	3186d4ea67	fix(review): exempt carried-id re-posts from the presubmit overlap drop (#9212) ... * fix(review): exempt carried-id re-posts from the presubmit overlap drop The presubmit overlap gate was purely location-based, so a Step 6 ledger re-post — which lands on the original thread's line by construction — collided with the very comment it re-posts and was dropped, losing the carried id from the round's ledger marker. A same-line finding with a DISTINCT claim was also dropped invisibly. presubmit now accepts an optional finding id in --new-findings, additionally buckets id-matched overlap comments as repost (with matchedIds), and the skill's drop rule exempts findings whose id matches and names the overlapping comment (id + excerpt) on every drop. * fix(review): gate the carried-id exemption on authorship, harden id handling Cross-account ledger-id collision: ledger ids are scoped per account (two reviewers of the same PR each have their own R2-1), but the repost branch never consulted the comment's author — a different account's comment carrying a colliding id at the same line bucketed `repost` and exempted the finding, posting the duplicate the overlap gate exists to prevent. classifyExistingComments now takes the current user and runs the id match only on the reviewing account's own comments; location-based overlap stays cross-account. Regression test covers the different-account collision. Also: - the R<round>-<n> grammar now lives once in lib/ledger.ts (LEDGER_ID_TOKEN); presubmit's extractor and compose-review's prefix parser both compose from it, so the two ends cannot drift. - parseFindingsFile rejects present-but-misshapen ids (r3-2 / trailing space / empty) with the whole file — an accepted typo'd id can never match the extractor and would silently disable the exemption; three new table rows pin it. - the --new-findings yargs describe documents the id field. - extraction is pinned to the FULL raw body by a test whose carried id sits past the 80-char summary slice (the summary.body mutation no longer ships green). - the duplicated presubmitWithComments test helper is hoisted to the shared describe scope. - SKILL.md documents the known limitation that id-less originals (bodies without an id token) cannot be matched as re-post targets. * fix(review): treat id:null as a missing id; align schema directives Round-2 review of this change found three follow-ups: - parseFindingsFile rejected the WHOLE findings file on "id": null, but JSON has no undefined — a producer emitting the key uniformly uses null for id-less findings. One null entry flipped findingsFileInvalid, silently disabling overlap dedup and capping the verdict: disproportionate blast radius for a missing optional field. null now parses as "no id" (misshapen strings still reject); a table row pins it. - The schema's `overlap` line still said "BLOCK on submit if non-empty" directly above the repost exemption, contradicting it; rewritten to name the exemption. The byBucket line now notes that repost entries are a subset of overlap and counted in both. - DESIGN.md still said existingComments has 4 buckets. * fix(review): exempt unambiguous id-less first-round originals from the overlap drop (#9208) buildLedger assigns first-round ledger ids positionally without writing them into the posted comment bodies, so a first-round original carries no id token. The carried-id re-post exemption keyed on the body token then failed to match and the re-post was dropped again — the exact regression the gate exists to prevent, preserved for the id-less-original class. Exempt the re-post when the target is unambiguous: the own-account comment is TRULY id-less (no carried id at all, so it cannot belong to a different finding), exactly one carried finding anchors at the location, and exactly one own-account comment sits there. A comment carrying some OTHER id is a different finding thread and keeps the strict match; ambiguous cases (several id-less comments or several carried ids at one line) keep the strict body match too, staying dropped and visible in the drop log. The per-account gate and the cross-account/80-char cases landed in the prior commit stay as-is. Tests: replace the now-obsolete drop pin with an unambiguous-exemption case and an ambiguous-strict case (69 passed). * fix(review): anchor carried-id readback, harden id-less fallback Round of fixes for the carried-id re-post exemption (#9212 review): - extractCarriedIds now mirrors compose-review's prefix readback (severity marker stripped, claim-line prefix only) instead of a \\b-bounded whole-body scan; cross-references and hyphen-run tokens no longer match as carried ids - the id-less fallback requires NO id-shaped token anywhere in the body (ANY_CARRIED_ID, unbounded on purpose: _ is a word character), so a lone comment merely referencing an id cannot ride the fallback - own-account ambiguity count includes replied-to originals - findings-file contract: ids belong on carried-forward findings only; fresh findings of the round omit id - CommentSummary exposes user so authorship-refused exemptions are self-explanatory in the report - SKILL.md Known-limitation narrowed to the ambiguous residue; body-Critical placeholder entries carry no id; sibling wordings aligned - test pins: prefix anchoring, mid-body/underscore/hyphen-run tokens, multi-finding and empty-login fallback gates, prior-repost target, replied-to count, anchored id-shape table rows * fix(review): share carried-id readback, strip markers via constants (#9212) * test(review): kill the five surviving presubmut mutants (#9212) Round-4 test-efficacy probes left five mutants alive at head; each now has a decisive fixture: - repost gate guard: unknown login + author-less comment carrying the id must not match (forcing currentUserLogin !== '' true now fails) - count-loop commit filter: a stale-SHA own comment at the location must not inflate the id-less-fallback ambiguity count - count-loop login filter: another Qwen account's comment at the location must not inflate the count either - case-insensitive authorship: a case-variant own login rides the id-less fallback, which passes through BOTH toLowerCase sites - full-body extraction: padding after the marker pushes the id-led claim line past the 80-char summary slice; extraction must read the full body All five mutants re-probed and killed; 86/86 green; eslint clean. * fix(review): share the carried claim-line strip across readback ends (#9212) The id half of the carried-id readback has been shared since the LEDGER_ID_READBACK hoist, but the marker strip feeding it was still hand-duplicated across presubmit's extractor and buildLedger's titleOf chain, and the copies diverged on unmarked bodies (presubmit read them back, buildLedger skipped them). Hoist the strip as carriedClaimLine next to severityOf: marker choice, slice, colon/whitespace strip, first line, one statement for both ends. The read sides now agree that an unmarked body carries no claim line, the strict direction: submit refuses to post unmarked findings, so an unmarked body is never a re-post. Pinning fixtures: - LEDGER_ID_READBACK table test covering every tolerated terminator plus the hyphen-run and ^-anchor rejections (dropping '.' from the class survived both consuming suites; now it is the sole failure) - colon-right-after-marker shape on the presubmit side, previously pinned only on the compose side - unknown-login count guard: an author-less comment must not ride the degenerate '' === '' comparison into the id-less fallback - unmarked body leading with an id-shaped token stays a plain overlap (fails on the pre-round tree, pinning the unified decision) * docs(review): close the repost-bucket doc gaps + annotate the equivalent guard mutant (#9212) - presubmit: annotate the unknown-login count pre-pass guard as a deliberate short-circuit — its count is consumed only inside the repost gate, which itself requires a known login, so the guard-forcing mutant is provably equivalent (R6-7 round-8 agreement: keep as defense in depth, retire the surviving-mutant metric for it) - DESIGN.md: the convergence paragraph still asserted the Overlap check blocks the exact scenario now exempted as repost; spell out the #9208 exception - SKILL.md: the AMBIGUOUS-only residue enumeration omitted the third conjunct — an id-less original whose body mentions ANY ledger-id token stays excluded from the fallback even at an unambiguous location --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-16 08:18:46 +00:00
Shaojin Wen	a6c2e678c3	fix(serve): redact skill bodies from the Web Shell event surface (#9235) ... * fix(serve): redact skill bodies from the Web Shell event surface (#9234) available_commands_update snapshots embed every installed skill's full SKILL.md body for ACP clients (e.g. desktop), but no SSE/REST consumer reads them — with many skills installed each snapshot weighed ~640 KB that every browser tab parsed and discarded, pushing the renderer into sustained memory pressure until the tab crashed. Strip _meta.availableSkillDetails at the SDK/browser egress points (SSE frames and the session-load replay arrays); the /acp surface keeps delivering the full snapshot. * fix(serve): redact skill bodies from branch/side-task responses too (#9234) Review follow-up: POST /session/:id/branch and POST /session/:id/side-task serialize the same replay snapshot shape as load/resume but were missed by the redaction. Wrap both 201 responses the same way, harden the load test so a dropped liveJournal redaction can no longer pass silently, and pin the /acp surface's verbatim retention of availableSkillDetails with a mirror test. * fix(serve): harden skill-detail redaction per review (flat frames, envelope pins) - R2-1: recognize the persisted-transcript flat frame shape (data.sessionUpdate) in addition to the eventBus-wrapped shape, with a regression test using a flat frame in compactedReplay. - R3-1: apply the replay-array redaction unconditionally in the branch route instead of re-deriving the bridge's variant discrimination. - R3-2: pin the full frame envelope and response sessionId in the redaction tests so envelope-level regressions cannot ship green. - R3-3: add a colocated skill-details-redaction.test.ts unit suite. * fix(serve): close remaining skill-body egresses per review (#9234) - Redact the virtual-subagent load replay (same BridgeEvent[] shape as the load response). - Redact flat persisted-transcript frames on both transcript routes, where the flat shape is actually produced. - Add regression tests for both paths and the Apache-2.0 header. * fix(serve): tolerate transcript pages without events in redaction (#9234) Guard the transcript-route redaction with `?? []` so a page payload that omits `events` (as some test fakes produce) no longer throws a 500.	2026-08-16 07:49:49 +00:00
qqqys	cd54a50d25	refactor(goal): type the limit that stopped a Goal (#9165) ... * refactor(goal): type the limit that stopped a Goal `reduceGoalResume` decided whether a `usage_limited` Goal may resume by comparing `lastReason` against two exact sentence constants. That made a display string load-bearing: any future path that wants to raise a limit and resume — a budget the user increases, an evidence catalog the runtime salvages — would have to reproduce the prose byte for byte or fail silently against a guard that compares English. `GoalRecord` now carries `limitKind`, written beside `lastReason` wherever the runtime stops a Goal at one of the enumerated bounds, and the resume guard reads that. `lastReason` keeps its current text and stays the human-readable half. Goals persisted before this field existed still refuse to resume through the sentinel comparison behind the new check, so recovery from an older transcript is unchanged. No behavior change: the same Goals resume and the same Goals refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(core): cover Goal limit classification * refactor(goal): carry limit kinds structurally * fix(core): reject inconsistent Goal limit snapshots --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-16 07:42:31 +00:00
qqqys	d74f282dc0	fix(goal): summarise the last Goal when a turn holds no permit (#9164) ... * fix(goal): summarise the last Goal when a turn holds no permit A Goal that reaches a terminal status stops issuing turn permits, so every later `get_goal` answered a bare `{ "active": false }`. The run's own turn count, elapsed time and stop reason became unreadable at exactly the moment someone needed them — a session that ended in `usage_limited` could not report how many turns it had completed or why it stopped, and the answer had to be reconstructed from the assistant's own per-turn narration. The runtime already holds that record and reading it needs no permit, so `get_goal` now reports `lastGoal` alongside `active: false`: goalId, revision, status, turnCount, activeTimeMs and lastReason. Scalars only — the objective and the evidence checkpoint stay behind the permit, and a session with no Goal or with unreachable Goal persistence still answers the bare `{ "active": false }`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(core): narrow last Goal summary * fix(core): preserve unpermitted Goal summaries --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: qqqys <266654365+qqqys@users.noreply.github.com>	2026-08-16 07:41:23 +00:00
Shaojin Wen	337da2143c	fix(ci): stop dropping agent settings in resolve and follow-up workflows (#9252) ... * fix(ci): stop dropping agent settings in resolve and follow-up workflows Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(ci): pin remaining agent-settings guard gaps from review Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.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-16 03:27:00 +00:00
Shaojin Wen	2610f6ed6d	feat(review): apply the huge round reduction only when the run has a clock (#9203) ... * feat(review): let an operator lower the reverse-audit round cap The round cap is chosen from the diff topology, which is right for the cost question it answers but leaves no way for someone paying for reviews to say "spend less on this loop". `review.reverseAuditRounds` is that knob: a standing operator policy that lowers whichever tier applies. It can only lower, and the asymmetry is the point rather than caution about configuration. A single operator-chosen count is exactly what tiering removed — a round is one agent on a small diff and about ninety minutes on a huge one, so one number is wrong for at least one topology, and most wrong for the one whose cap exists to stop six-hour reviews that post nothing. Lowering carries no matching hazard: it can only end the loop sooner. The floor stays at the huge tier's three, for the reason the plan reader already refuses one and two — a cap below that pre-empts the two-consecutive-dry rule and buys a capped verdict rather than a cheaper review. The two things an operator means by "let it run longer" both have direct expressions that a round count only approximates: a ceiling longer than the huge tier assumes is a deadline, which the admission gate already prices a round against, and "keep going while it is still finding real defects" is a property of the findings rather than of a number chosen before the review starts. It is a setting, not a flag, and it resolves in the capture command rather than at the gate — so it lands in the plan and every reader sees one number without learning a setting was involved. That satisfies the module's standing rule that a budget the caller passes is a budget the caller can inflate, rather than making an exception to it. The reader needs no new code: a lowered value is inside the tier's band, which the existing clamp already honours. Operator scopes only, like the other review policy settings — a repository must not choose how deeply the pipeline verifies it, which the new tests pin by putting the setting in a workspace file and asserting it does nothing. Two things fall out of adding a fourth caller, both fixed here: - The plan builder now takes the ceiling as a REQUIRED parameter. Three capture commands build a plan, an optional one is a parameter a call site can quietly omit, and a setting that applies to two of the three review entry points is worse than one that applies to none. Passing undefined is how a caller says "no ceiling", visibly. - Reading the settings can throw: loading raises a fatal error when any settings file fails to parse, and this is now read while the diff is being captured — the review's first step. A stray comma in a file none of these settings had to come from would have ended the whole review. It degrades to the defaults and discloses instead, and every default is the conservative side: attribution on, no auto-posting, no effort or round-cap override. * chore(review): regenerate the settings JSON Schema for the new review setting The schema file is generated from the settings schema and checked in; adding a setting without regenerating it fails the CI check that keeps the two in step. No behaviour change — the file is derived output. * fix(review): stop the settings degrade from being killed by its own announcement The try/catch added here exists so a corrupt or unreadable settings file cannot end a review: loading throws a fatal error when any settings file fails to parse, and this is read while the diff is being captured, the review's first step. It then announced the degrade through the *throwing* stderr helper, so the announcement could end the review the degrade was written to save. Both halves are reachable together, and the second is ordinary rather than exotic: `process.stderr.write` throws on EPIPE or a closed fd — whenever the reader goes away (`qwen … | head`) or a daemon redirects its stderr — which is why the safe sibling exists and says so in its own docstring. With a broken settings file and no reader, the throw propagates out of the catch and all three capture commands crash before writing a plan. Switched to the safe writer, with the reason recorded at the call site. The test that covers it needed the mock repaired first, because the mock was hiding the bug: it mapped both writers to one non-throwing spy, which makes the throwing and safe helpers interchangeable and mocks away the entire distinction the degrade depends on. The safe one is now a spy that swallows what the underlying write throws, matching the real contract — so a test can make the write fail and see which helper the code chose. Reverting to the throwing helper turns the new test red; before this, it turned nothing red. * fix(review): correct two rationales that contradict the code they sit beside Both are claims about mechanism, both false, and both contradicted by documentation already in the same file — which is what makes them worth fixing past the round where only correctness fixes land: in this codebase the comment is the design record, and a wrong one outlives the round that shipped it. **Why a cap of one or two is refused** was stated in four places as "it forces a non-converged stop where two-consecutive-dry would have converged on its own". That is false for two: the convergence check runs before the cap gate, so an all-dry loop reaches CONVERGED under any cap of two or more — which the huge-tier constant's own docstring, two functions away, already said. The reasons are real but different for each value. One refuses the convergence pair's second member, so the loop can never produce the two dry audits convergence is defined by. Two lets an all-dry loop converge but leaves no round for a loop that reports anything, so the first finding makes the stop non-converged. Both end in a capped verdict rather than a cheaper review; only the mechanism was wrong. **When loading settings throws** was described as "any settings file fails to read or parse", with a stray comma as the example. Malformed JSON is the one case that does not throw: it is copied aside and recovered, under a comment that says "Never crash due to a corrupted settings file". The throw comes from a file that cannot be read, which is enough on its own to justify the degrade — this is read while the plan is being captured — so the correction narrows the claim without weakening the reason for the guard. * feat(review): apply the huge round reduction only when the run has a clock Three is the one tier lower than the topology beneath it, and read as a statement about auditing it is backwards. A huge diff has more defects and more territory than a chunked one, converges later, and on recall deserves more rounds rather than fewer — the standing counterexample is a 5,801-line PR that took eight review rounds and was still surfacing Criticals in code present since its first commit. It was never a statement about auditing. It is a statement about a wall: a reverse-audit round on a 4,000-line PR is about ninety minutes, five of them are 450, and a six-hour CI ceiling does not hold that plus the fan-out and the tail. The survey behind it measured absent reviews rather than slow ones — twenty-six timed-out jobs in one window, about 122 hours of compute, nothing posted. Three rounds reported beat five rounds lost. That argument is sound exactly where the wall is. A local run exports no review deadline, nothing kills it at six hours, and the reduction there trades recall away to fit a ceiling that does not exist — on the tier where recall matters most, and by a number calibrated against somebody else's CI. So the reduction now applies only when the run has a deadline at all. With a clock a huge diff caps at three, as before; without one it is simply a large chunked diff and caps at five. The clock is read where the settings ceiling is read — in the capture command, passed into the budget, recorded in the plan — so the two facts the cap depends on arrive the same way and the budget module keeps its property of touching neither the environment nor the settings. Both now travel as one context object rather than a growing parameter list, still required at the plan builder so a capture command cannot silently omit them. The admission gate asks the same question through the same parse the deadline gates already use, so "has a deadline" and "a deadline will be enforced" cannot come apart. All four capture/gate clock combinations are safe and covered: a plan captured without a clock records five and is honoured at five; read later under a clock its band closes to three and it is cut to three, which is the conservative direction when a wall turns out to exist after all. Two things this does not pretend to fix, both recorded in the design note. The deadline gate falls back to a flat thirty-minute round estimate until it has measured one, so on exactly the runs that time out it under-prices the first two rounds threefold and cannot refuse them — that, not the round count, is why a static reduction was needed on top of a working gate, and a size-aware first estimate is what would retire the reduction entirely. And chunk retirement can only begin at round three, so under a three-round cap only one round can ever shrink: the arithmetic that justifies the cap is an arithmetic the cap guarantees stays true. * fix(review): correct the claims this stack got wrong, and cover its untested seams Round-4 review of the three stacked changes, all of it comment-and-coverage rather than behaviour. Grouped by what was actually wrong. **A doc comment detached from what it documents.** The context interface was inserted between `reviewBudget`'s doc block and the function, so the whole block — including the input-laundering contract that opens it — attached to the interface and the function was left undocumented. The interface moves up beside its sibling with its own doc; the function keeps its contract. Its new paragraph also called both context fields environment values, when only one is — the other is resolved from settings. **Four test comments that argued for their assertions with false reasons.** The assertions were right and the rationales were not, which in a codebase whose comments are its design documentation is the more durable error. The integer guard is tested before the floor, not after, so the fractional value cited says nothing about what the floor would have caught alone. Not every value in the coercible-garbage list becomes zero — a negative stays negative and two strings become real counts, one of them large enough to land on the huge tier rather than the fallback. A single global bound of ten honours only two of the three clamp assertions, not all three; the third is an edge case, not a discriminator. And one comment justified its cases by naming a writer that does not exist on that branch. **A guarantee that is not guaranteed.** The settings fallback was documented as always degrading toward more work; that holds for three of the four fields and not for effort, where losing an operator's `high` returns the built-in rule and a local review drops to medium — less work, not more. Named as the exception it is, which is also why the fallback discloses on stderr. **Five prose sites that never learned the cap is conditional.** The step's stop rule, the clock module's own header, the large-diff cost model, the setting's schema description and the user-facing feature doc all still enumerated the cap unconditionally, contradicting the change one file away. The schema description additionally advised setting a deadline "to let a productive loop run longer", which is backwards: on a huge diff a deadline lowers the cap from five to three. **Two prose sites that overstated scope.** The setting is honoured from three operator scopes, not user only, and a value below the floor is ignored rather than clamped to it; and it cuts the cap for high-effort reviews only, since medium skips the reverse audit and low runs none. A cross-reference named a heading that does not exist. **The two seams nothing covered.** The environment boundary — whether this run has a deadline — was never exercised with the variable actually set: the budget tests pass the flag as a literal and the gate tests delete it. The operator ceiling's write path was likewise untested end to end; every builder call in the unit tests passes it as absent. Both now run through the real capture handler against a real environment and a mocked settings source, and both were mutation-checked: inverting the environment predicate and ignoring the ceiling each turn the new tests red. * fix(review): finish propagating the clock, and close the seams round 5 found Round-5 review of the stack. One real defect, one unsafe writer, and the rest is narration that stopped short plus coverage that could not see the change it was meant to cover. **A mock that leaked into every test after it.** The handler test added last round set an operator ceiling on a module-level settings mock and never restored it, so every later test in that file — including the whole trailing block — ran the real handler with an undeclared ceiling of twenty in play. Inert today because nothing downstream asserts on it, which is exactly how it would have survived to matter. Reset in the file's `beforeEach`. **The degrade announced itself with a writer that can destroy the degrade.** The settings fallback wrote its NOTE through the throwing stderr helper, whose safe sibling exists for precisely this case and says so in its own docstring: the write is incidental to the work in hand, and failing it would take down the fallback the guard exists to provide. Switched, with the reason recorded. **Five narration sites still stated the huge reduction unconditionally** — the field doc, the cap reader's own bullet, the enforcement-site comment above the round gate, the skill's version-skew paragraph, and the design note's "what an operator means by let it run longer". The last one was the most wrong: it named a deadline as the way to say "my ceiling is larger than six hours", but the check is for a deadline's *presence*, so any deadline however generous reads as three, and the cap is evaluated before the deadline arithmetic — setting one lowers the cap rather than raising it. Saying that would need the tier to read the deadline's size, which is now written down as the missing capability rather than implied to exist. **The setting was documented everywhere except the reference table.** The hand-maintained settings reference lists the review section exhaustively and did not list this one; the generator only writes the JSON schema, so it does not self-heal. Its schema description also never mentioned that only whole numbers are honoured, which matters because JSON Schema has no integer type here — a fraction validates in an editor and is discarded at runtime. **Two coverage gaps where the change was invisible to mutation.** Every gate test used the unsized fixture, whose tier is the fallback whatever the clock says, or forced a cap by storing one — so hardcoding the clock argument at all four call sites left the suite green. A sized huge plan is the only shape where the flag decides anything, and it now drives the gate on both sides. And two of the three capture commands had no budget assertion at all, so dropping either context field from their call sites compiled clean; the local one now asserts both. Both new tests were mutation-checked against exactly those edits.	2026-08-16 03:26:59 +00:00
Shaojin Wen	f9bc8cb250	fix(autofix): re-anchor growth divergence on measurement time and external head moves (#9192) ... * fix(autofix): re-anchor growth divergence on measurement time and external head moves Tightens the growth-divergence comparability window (PR #9104 follow-up, tracked as #9114): - measured_at (R2-6): the growth-now marker now carries the prepare-time measurement instant, and the divergence read filters on it instead of the comment's created_at. The report posts the marker only after the agent's ~120-minute run, so a round in flight when a concurrent base update landed would otherwise pass a created_at filter while carrying sums measured against the old base. - external head move (R2-8, subsumes R6-3): prior sums are measured against origin/main, so any commit an external actor (author push) or a stale-base merge added since the bot last evaluated the branch inflates this round's sum relative to them. BASE_UPD_AT only tracks the bot's own update-branch merge; the new GROWTH_NOW_CUTOFF also re-anchors (drops all prior sums) whenever the checked-out head is not the bot's last judged head (LIVE_RED_HEAD), covering author pushes and base updates alike. The reader now dedups/orders per run by measured= (a re-run's fresh measurement wins). Contract tests cover the measured-based cutoff, the external-head-move re-anchor (both branches), and the writer→reader round-trip with the new field. 172/172. R6-6 (markers don't store the effective budget, so a mid-window budget raise counts old rounds against the new regime — fail-safe, one round early) stays tracked in #9114. * fix(autofix): drop the head-move re-anchor, keep the measurement-time filter Review found the head-move half of this change broken in three ways (all probe-verified), so it is withdrawn and returned to #9114 rather than patched under review: - R1-1 (regression): `autofix-redcheck` records the head the agent was GIVEN, frozen before its push — so after any pushing round the next round's head differs and the cutoff was set to now, dropping every prior sum. In the push regime OVER_ROUNDS_PRIOR could never reach the threshold and the #9104 handoff would never fire at all. - R1-2: the cut was stateless — the round after a correct re-anchor fell back to an empty cutoff and re-admitted every pre-move sum. - R1-3: with no redcheck marker (a crash round) the `-n` guard skipped re-anchoring across a genuine external move. A correct version needs both a bot-authored-move test and a PERSISTED cut; that is its own change. What remains is the measurement-time filter (R2-6), which stands on its own: the marker carries the prepare-time instant and the divergence read filters/orders on it instead of the comment's post-agent created_at. Also from this review: - R1-4: `measured=` is OPTIONAL in the scan, falling back to the comment's created_at, so deploying does not blank an in-flight window's census. - R1-9: the per-run collapse now runs BEFORE the over/window/cutoff filters — a re-run whose fresh attempt came back under budget was still represented by its stale over=true attempt. - R1-7: comments corrected — run= is the DEDUP identity, measured= the ORDER key (four sites). - R1-8: recorded as a known residual next to the sibling growth-base reader, which still filters on created_at; tracked in #9114. - R1-5/R1-6: fixtures decouple created_at from measured=, cover a legacy marker (with and without the cutoff), and pin the measured_at source line in prepare. * fix(autofix): keep the failure-path growth marker scannable when prepare never ran * fix(autofix): prefer explicit measured= over created_at fallback in the per-run growth collapse * test(autofix): pin the explicit-measured preference in the per-run collapse cbb7186fb6 made the collapse prefer a marker with an explicit measured= over one falling back to created_at, but nothing pinned it: dropping `.explicit` from the max_by key shipped green. The failure path posts an inert over=false marker with no measured= when prepare never ran, and its fallback timestamp is ~2h later than the prepare-time measured= of the same run's real attempt — so without the preference the collapse keeps the inert marker and the over filter then drops the run entirely, losing the count (#9192 R3-1). Mutation-checked: reverting to max_by(.measured) turns this fixture from 'false 1' to 'false 0'. * fix(autofix): gate the measured= stamp on a real measurement (#9192) An unmeasured re-run attempt still emitted measured_at, so its report posted an explicit over=false marker that the per-run collapse preferred over the same run's real over=true measurement — erasing the count the explicit-measured preference was added to protect. Emit the stamp only when NET_MEASURED=true and let the push/no-op writers omit measured= like the failure path already does. Also pin the strict > cutoff boundary with an equality fixture (the >= mutant shipped green), drop the per-run-collapse fixture that duplicated the strictly-stronger one from cbb7186fb6, and document the three self-limiting deploy-transition residuals: the legacy-straddle collapse, the two-clock PREV_SUM skew, and the fetch->stamp base-update window. --------- Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>	2026-08-16 03:26:58 +00:00
Shaojin Wen	6f3b583701	fix(review): give duplicate-dropped Suggestions their own compose state and body sentence (#9215) ... * fix(review): give duplicate-dropped Suggestions their own compose state and body sentence A review whose confirmed Suggestions were dropped because the PR already carried them (prior round or concurrent reviewer) had nowhere to record the drop except suggestionsDiscarded — whose body sentence asserts an anchor failure. On #9204 resolve-anchors returned three exact-added matches, the drop reason was duplication, and the posted body claimed the findings "could not be anchored to a changed line" — a public claim the run's own artifacts contradict. Add a suggestionsDroppedAsDuplicates state field: entries name the finding and where it already lives, render as their own body paragraph (bilingual, comment refs linkified), and count toward S exactly like anchor-failure discards so an all-duplicate run never reads as zero-finding. The skill's Step 7 state list now routes duplicate drops to it instead of the count. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(review): hoist plan PR-identity read; pin duplicate-clause zh count and empty-entry filter * fix(review): duplicate-drop account renders on every event; one stripped-list helper * fix(review): cap-free duplicate-drop fixtures, honest seam pin, body-rule carve-out * test(review): pin the full duplicate list and the compose-review --input seam * fix(review): bound the duplicate-drop account like the deferred channel * fix(review): drop truncated comment refs, collapse CRs, bound cannot-tell (#9215) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>	2026-08-16 02:33:30 +00:00
Shaojin Wen	b29549ade8	feat(review): scale the reverse-audit round cap to the diff topology (#9183) ... * feat(review): scale the reverse-audit round cap to the diff topology The reverse-audit loop's round cap was one number for every topology, but what it bounds is a round — and a round costs one auditor on a small diff, one auditor per non-retired chunk on a chunked one, and about ninety minutes on a huge one. Two orders of magnitude across the topologies a single cap had to serve, so it was necessarily wrong at one end: too loose to bound the huge case, which is why a reduced cap had to be carved out of it, and tight enough on a small diff to stop loops that were still confirming Criticals — for a saving of about five calls out of a seventeen-to-twenty-three-call review. The asymmetry this design is built on says that is the wrong trade: a missed issue costs another review iteration, and per-run cost is the cheaper side. The cap now comes from the plan's topology tier — ten on a small diff, five on a chunked one, three when huge. The huge tier is checked first and wins, since it is a finishability ruling; a small diff can never reach it anyway, because the effective-line measure is bounded by the topology gate itself. Two supporting moves. The topology gate moves into the budget module, so the roster and the round cap read one predicate instead of two copies of the same pair of numbers. And the cap reader now takes the whole plan rather than its budget alone, so a stored value is clamped into that plan's own tier: a plan written before tiering gets the rounds its topology earns instead of another topology's number, and a hand-edited plan cannot buy a tier it did not qualify for. A plan carrying no usable size reads as the chunked tier — what every plan got before this change — because an unsized plan could be the large one. Nothing else about the loop changes. It still ends on two consecutive dry rounds, a cap stop is still a non-converged stop that writes its marker and caps the verdict, and in a time-budgeted run the deadline gate remains the operative bound. * fix(review): size the round tier from real numbers, not coercible garbage Round-2 review of the tiering change found the tier's usability check does the opposite of what its own comment promised. It coerced first and asked `Number.isFinite` after, and `Number()` turns `null`, `''`, `false` and `[]` into a finite zero — so a plan whose sizes are unknowable was read as a zero-line diff and handed the small tier's ten rounds, the most expensive cap, while the sibling case of an entirely absent field correctly fell back to five. That shape is not hypothetical: JSON serialization writes a not-a-number line count as null, so the corrupted plan the fallback exists for arrives looking exactly like an empty diff. A numeric string coerced too, which would have let a hand-edited huge plan reach the small tier through the very clamp added to stop it. The write path had the same hole from the other side: it sized the tier from counts already laundered to zero, recording ten rounds into a plan whose size never arrived, where the flat cap recorded five. Usability is now judged before coercion — a real, finite, non-negative number — and the write path hands the tier its raw input so a garbled count stays garbled instead of arriving as a legitimate zero. Zero itself still reads as what it is: an empty diff is a small diff, not an unsized one. Also from that review, all confirmed against the code: - The cap reader's comment claimed two things the code never did. It does not migrate a legacy small plan to ten — a pre-tiering CLI wrote five, five is inside a small plan's band, and a value the plan states is honoured; only an absent or out-of-band one reaches the tier. And it does not always err toward more auditing — a field-less huge plan now reads three where the flat fallback read five, deliberately less, because that tier is a finishability ruling. Both claims are replaced by what happens, and both are now pinned by tests rather than asserted in prose. - Two code comments still narrated the old two-value cap; the same sweep that updated every other prose site had missed them. - The garbled-value test never reached the integer guard: the lower bound rejects a fractional value below it first, so deleting the guard left the suite green and a fractional cap inside the band would have been honoured. - The design note's fork-subagent section had its call count updated but every figure derived from the old count left behind, so the section contradicted itself; the derived token figures are re-derived from the new range. - The effective-line measure was written twice in one module, which is two size measures that can drift apart inside one budget object. - The plan interface the cap reads through declared none of the three fields it now reads, so a rename on the writing side would compile clean and collapse every cap to the fallback tier in silence. * test(review): pin the round cap's band against a constant-bound mutation Every existing case for the stored-value clamp sits on a boundary — at the floor, at the tier, or outside it — and a boundary-only suite cannot tell a tier-relative bound from a constant one. Mutating the upper comparison from the plan's tier to the flat large value passed all fifty-seven tests, which means a later simplification back to one global bound would ship green while changing behaviour on exactly the plans the clamp exists for. The interior is where the two bounds differ, and it is reachable without a hand edit: the operator round ceiling writes such a value into the plan. Three assertions cover it from both sides — a small plan storing seven keeps seven rather than collapsing to its tier, and a huge plan storing four is still cut to three rather than being handed a round past its finishability tier. * fix(review): close three consistency gaps found in round 3 All three are about this change disagreeing with itself rather than about behaviour a user would see today. The tier function derived the same two line counts twice: once for the huge-diff gate, from validated locals, and again for the topology gate, by re-reading the raw object. They agree today, and they agreed right up until the last round found a defect of exactly that shape — one derivation laundering garbage the other rejected. The topology gate now takes the validated pair. The skill's version-skew paragraph still carried the slogan the code comment retracted a commit ago: that a plan with no budget field errs toward more coverage, never less. It does for the four fields it lists, and it does not for the round cap — a field-less huge plan reads three where the flat fallback read five. The paragraph now says so instead of asserting the opposite of the rationale two files away. Two of the four call sites that read the cap from the plan had no test at the new ten-round tier: the per-chunk build gate and the retirement note. Both are reachable on a small plan, which can carry chunks — the chunk budget is 400 lines while the small-diff gate admits 3200. The retirement-note case is the sharper of the two, and it is the same scenario as its cap-five sibling with the opposite outcome: a round-five retirement schedules a cold check for round six, which the small tier allows, so the note must promise that check rather than close the certificate.	2026-08-16 02:29:38 +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
易良	5677823abb	fix(review): note when --all-chunks fans out a plan whose numbers say 3A (#9249) ... * fix(review): note when --all-chunks fans out a plan whose numbers say 3A runAllChunks gates on requireAuditableChunks alone and never consults the topology gate, so --all-chunks can fan out one auditor per chunk on a plan whose own srcDiffLines/diffLines say Step 3A — one whole-diff auditor per round, which is what the reverse-audit round cap is priced for (#9242). Add a purely diagnostic stderr note after requireAuditableChunks (no exit-code change, no refusal — legitimate repair paths exist), and record the decision in SKILL.md next to the topology gate and the Step 5 builder paragraph so the orchestrator explains a deliberate fan-out instead of letting the mismatch ride silently. * fix(review): keep the 3A topology note below the gates and on both fan-out paths Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: yiliang114 <yiliang114@users.noreply.github.com> Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-15 23:27:18 +00:00
Shaojin Wen	3119d53e4d	fix(sdk): raise daemon browser bundle budget to 191KB (#9238) ... Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-15 18:30:09 +00:00
qwen-code-dev-bot	60a7bda120	fix(integration-tests): ack daemon tool-guard handshake in the mock ACP child (#9159) (#9161) ... * fix(integration-tests): ack daemon tool-guard handshake in the mock ACP child (#9159) The cross-worktree Git guard made the daemon's built-in tool guard unconditional, so the bridge now refuses any ACP child that does not acknowledge the required guard handshake during initialize. The mock ACP child used by the live-journal recovery E2E tests never acked, failing every session it served with "ACP child did not acknowledge the required external tool guard" on all E2E platforms and sandbox legs. Mirror the production child contract in the mock: consume the private guard marker and return the ready acknowledgment in the initialize response when the daemon requires the guard. * fix(integration-tests): ack session close ext method in the mock ACP child (#9159) * fix(integration-tests): keep mock ACP child lightweight and gate its close ack (#9159) * fix(integration-tests): narrow close-gate comment to what it catches (#9159) * fix(integration-tests): name the post-merge E2E workflow in the close-gate comment (#9159) --------- Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>	2026-08-15 18:28:01 +00:00