From 0a9db3822109dc04fc09244b976039512d520d6f Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:40:10 +0800 Subject: [PATCH 01/38] =?UTF-8?q?feat(review):=20add=20`review=20run`=20?= =?UTF-8?q?=E2=80=94=20headless=20review=20with=20a=20machine-readable=20v?= =?UTF-8?q?erdict=20(#7983)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(review): add `review run` — headless review with a machine-readable verdict The review pipeline already runs non-interactively: `qwen --prompt "/review …"` expands the bundled skill, launches the dimension agents, and honors the approval mode. What that path lacks is a contract. The verdict lives in the model's prose and in files whose names the caller must simply know, the exit code says nothing about the outcome, and piped stdin silently defeats slash-command detection (the runner prepends piped input, so the leading `/` is no longer first). Anyone who wants "run a review, tell me what it decided" ends up scraping a terminal. `qwen review run [target]` is that contract and nothing more. It assembles the /review invocation from typed flags (--effort, --comment), re-enters this build's own CLI in a child process with stdin closed, streams the child's progress to stderr, and then reads the verdict from the artifact compose-review wrote — the same JSON the skill treats as the verdict authority — never from anything the model printed. stdout carries only the result (human lines, or the full JSON with --json). Exit codes make the outcome scriptable without parsing: 0 = the review completed (whatever it decided), 1 = it never reached a verdict (child failure, timeout, or no composed artifact — a clean child exit without one is a run that wandered off, not an approve), 3 = completed AND --fail-on request-changes AND the event is REQUEST_CHANGES, so a CI gate can tell "blocking verdict" from "the tool broke". Artifact discovery is scoped to this run (mtime cutoff with a small slack for coarse filesystem clocks): a stale composed JSON from an earlier review says whatever THAT review decided, which is exactly the wrong thing to republish. * fix(cli): harden review run against EPIPE, target injection, and drift (#7983) - Use writeStderrLineSafe in the timeout and spawn-error handlers and guard the progress stream, so an EPIPE on stderr can no longer skip the child kill, hang the promise, or orphan the review. - Reject a review target carrying whitespace or a leading dash before it is re-tokenized by the child CLI (e.g. `123 --comment` silently authorising posting). - Constrain --approval-mode to the same choices as the top-level CLI. - Capture the child's exit signal and surface it (OOM/SIGKILL vs spawn fail). - Sync the top-level `qwen --help` review description with the command. - Register `run` in the review.test.ts subcommand expectation and add tests for the timeout branch, the readComposed guard, and target rejection. * fix(cli): kill process group on review run timeout, harden edge cases (#7983) The CLI relaunches itself in a child process (for --max-old-space-size), so child.kill() only reached the relaunch wrapper — the real review was reparented to PID 1 and kept burning API calls. Spawn with detached:true and kill the process group (-pid) so the timeout actually terminates the review. Also: clamp negative --timeout-minutes to a 1-minute floor, distinguish a corrupt composed artifact from a missing one in human-readable output, and add test coverage for the default (non-JSON) output path. * fix(cli): use specific MockInstance type for process.kill spy (#7983) * fix(cli): capture review run verdict before cleanup, forward signals (#7983) * fix(cli): reject quoted review targets, pin signal forwarding (#7983) * fix(cli): keep captured review verdict when timeout fires after compose (#7983) --------- Co-authored-by: verify Co-authored-by: qwen-code-dev-bot Co-authored-by: qwen-code-bot --- docs/users/features/code-review.md | 20 + packages/cli/src/cli.ts | 2 +- packages/cli/src/commands/review.test.ts | 1 + packages/cli/src/commands/review.ts | 6 +- packages/cli/src/commands/review/run.test.ts | 616 +++++++++++++++++++ packages/cli/src/commands/review/run.ts | 495 +++++++++++++++ 6 files changed, 1137 insertions(+), 3 deletions(-) create mode 100644 packages/cli/src/commands/review/run.test.ts create mode 100644 packages/cli/src/commands/review/run.ts diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index fd69536aca..7a30a62b94 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -277,6 +277,26 @@ The deterministic halves of the pipeline — argument parsing (`qwen review pars Every run ends with one machine-readable line (`Review complete: — `), so scripts and CI wrappers can detect completion and outcome with a single `^Review complete: ` match. +## Headless runs (`qwen review run`) + +`/review` is interactive. When a script or CI job needs to run a review and act on its outcome, use the headless wrapper: + +```bash +qwen review run [target] [--json] [--fail-on request-changes] [--comment] [--quiet] +``` + +`target` is a PR number, a PR URL, or a file path; omit it to review the local working tree. The command runs this build's own CLI non-interactively (with stdin closed, so slash-command detection survives), streams the child's progress to **stderr**, and prints the verdict to **stdout** — or, with `--json`, the full result object. The verdict is read from the artifact `compose-review` writes (the same JSON the skill treats as the verdict authority), never parsed from the model's prose. + +The exit code is the contract a gate should read: + +| Exit | Meaning | +| ---- | ------------------------------------------------------------------------------------------------- | +| `0` | The review completed (whatever it decided) | +| `1` | It never reached a verdict — the child failed, timed out, or left no composed artifact | +| `3` | It completed with `REQUEST_CHANGES` **and** `--fail-on request-changes` was set (opt-in blocking) | + +`3` (not `2`) lets a gate distinguish "the review is blocking" from "the tool broke" — yargs already uses `1` for usage errors — without parsing any output. `--timeout-minutes` (default 120, floored at 1) terminates a hung review and exits `1`, and cancelling the command (Ctrl+C / SIGTERM) terminates the review's process group rather than orphaning it. + ## Cross-file Impact Analysis A dedicated cross-file tracer (Agent 1c) owns this walk end-to-end. When code changes modify exported functions, classes, or interfaces, it searches for all callers and checks compatibility: diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index b4d95c662f..41d07de0a3 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -25,7 +25,7 @@ export const TOP_LEVEL_COMMANDS = [ ['mcp', 'Manage MCP servers'], [ 'review ', - 'Internal helpers used by the /review skill (PR worktree setup, context fetch, rules loading, presubmit checks, cleanup)', + 'Run a review non-interactively (`run`), plus the internal helpers used by the /review skill (PR worktree setup, context fetch, rules loading, presubmit checks, cleanup)', ], [ 'serve', diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index f8150cc0ef..677dcad60d 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -39,6 +39,7 @@ describe('reviewCommand', () => { it('registers exactly the expected internal helper subcommands', () => { expect(registeredSubcommands()).toEqual([ + 'run', 'parse-args', 'fetch-pr', 'capture-local', diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 03f8091a9c..7f5f61c7d7 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -26,13 +26,15 @@ import { scriptLintCommand } from './review/script-lint.js'; import { submitCommand } from './review/submit.js'; import { testEfficacyCommand } from './review/test-efficacy.js'; import { cleanupCommand } from './review/cleanup.js'; +import { runCommand } from './review/run.js'; export const reviewCommand: CommandModule = { command: 'review', describe: - 'Internal helpers used by the /review skill (PR worktree setup, context fetch, rules loading, presubmit checks, cleanup)', + 'Run a review non-interactively (`run`), plus the internal helpers used by the /review skill (PR worktree setup, context fetch, rules loading, presubmit checks, cleanup)', builder: (yargs: Argv) => yargs + .command(runCommand) .command(parseArgsCommand) .command(fetchPrCommand) .command(captureLocalCommand) @@ -52,7 +54,7 @@ export const reviewCommand: CommandModule = { .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, compose-review, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/run.test.ts b/packages/cli/src/commands/review/run.test.ts new file mode 100644 index 0000000000..68e6593bde --- /dev/null +++ b/packages/cli/src/commands/review/run.test.ts @@ -0,0 +1,616 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `review run` is a contract around the headless review: build the right +// /review invocation, republish the verdict compose-review wrote (never the +// model's prose), and map outcomes onto exit codes a CI gate can trust. The +// child CLI itself is tested elsewhere; these tests pin the contract — prompt +// assembly, artifact discovery (this run's verdict, not a stale one), the +// completed/failed/blocking exit split, and the spawn wiring. + +import { + describe, + it, + expect, + beforeEach, + afterEach, + vi, + type MockInstance, +} from 'vitest'; +import { EventEmitter } from 'node:events'; +import { + mkdtempSync, + mkdirSync, + rmSync, + writeFileSync, + utimesSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const spawnMock = vi.hoisted(() => vi.fn()); +const execFileSyncMock = vi.hoisted(() => vi.fn()); +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + default: { ...actual, spawn: spawnMock, execFileSync: execFileSyncMock }, + spawn: spawnMock, + execFileSync: execFileSyncMock, + }; +}); + +const { + buildReviewPrompt, + newestArtifactSince, + exitCodeFor, + killProcessGroup, + runCommand, +} = await import('./run.js'); +const { REVIEW_TMP_DIR, REVIEWS_DIR } = await import('./lib/paths.js'); +// The real cleanup, not a mock: the regression test below must prove the parent +// captures the verdict before Step 9's actual sweep deletes it. +const { runCleanup } = await import('./cleanup.js'); + +describe('buildReviewPrompt', () => { + it('reviews the local tree when no target is given', () => { + expect(buildReviewPrompt({})).toBe('/review'); + }); + + it('threads target, effort, and --comment through verbatim', () => { + expect( + buildReviewPrompt({ target: '7724', effort: 'high', comment: true }), + ).toBe('/review 7724 --effort high --comment'); + }); + + it('omits what was not asked for', () => { + expect(buildReviewPrompt({ effort: 'medium' })).toBe( + '/review --effort medium', + ); + }); + + it('rejects a target that would re-tokenize into extra args', () => { + // `123 --comment` would split into a target plus a flag the child + // honours, silently authorising a post the run never asked for. + expect(() => buildReviewPrompt({ target: '123 --comment' })).toThrow( + /Invalid review target/, + ); + expect(() => buildReviewPrompt({ target: '--comment' })).toThrow( + /Invalid review target/, + ); + }); + + it('rejects a target carrying quote characters', () => { + // tokenizeArgs strips quotes, so `src/it's-a-file.ts` would re-tokenize + // to `src/its-a-file.ts` — silently re-targeting a file never named. + expect(() => buildReviewPrompt({ target: "src/it's-a-file.ts" })).toThrow( + /Invalid review target/, + ); + expect(() => buildReviewPrompt({ target: 'src/"quoted".ts' })).toThrow( + /Invalid review target/, + ); + }); +}); + +describe('newestArtifactSince', () => { + let dir: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'run-artifacts-')); + }); + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + function file(name: string, mtimeMs: number): string { + const path = join(dir, name); + writeFileSync(path, '{}', 'utf8'); + utimesSync(path, mtimeMs / 1000, mtimeMs / 1000); + return path; + } + + it('ignores artifacts older than the run', () => { + // A stale composed JSON is the LAST review's verdict — republishing it + // would report an outcome this run never produced. + const start = Date.now(); + file('qwen-review-local-composed.json', start - 60_000); + + expect( + newestArtifactSince(dir, /^qwen-review-.*composed\.json$/, start), + ).toBeNull(); + }); + + it('returns the newest matching artifact from this run', () => { + const start = Date.now() - 10_000; + file('qwen-review-local-composed.json', start + 1_000); + const newer = file('qwen-review-pr-9-composed.json', start + 5_000); + file('unrelated.json', start + 9_000); + + expect( + newestArtifactSince(dir, /^qwen-review-.*composed\.json$/, start), + ).toBe(newer); + }); + + it('returns null when the directory does not exist', () => { + expect( + newestArtifactSince(join(dir, 'absent'), /composed/, Date.now()), + ).toBeNull(); + }); +}); + +describe('exitCodeFor', () => { + it('splits completed / no-verdict / blocking into 0 / 1 / 3', () => { + expect(exitCodeFor(true, 'APPROVE', 'none')).toBe(0); + expect(exitCodeFor(true, 'REQUEST_CHANGES', 'none')).toBe(0); + expect(exitCodeFor(false, null, 'none')).toBe(1); + expect(exitCodeFor(true, 'REQUEST_CHANGES', 'request-changes')).toBe(3); + expect(exitCodeFor(true, 'COMMENT', 'request-changes')).toBe(0); + // An incomplete run is 1 even under --fail-on: "the tool broke" must never + // read as "the review blocked". + expect(exitCodeFor(false, 'REQUEST_CHANGES', 'request-changes')).toBe(1); + }); +}); + +describe('killProcessGroup', () => { + let processKill: MockInstance; + + beforeEach(() => { + processKill = vi.spyOn(process, 'kill').mockImplementation(() => true); + execFileSyncMock.mockReset(); + }); + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('kills the POSIX process group with a negative pid', () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux'); + killProcessGroup(12345, 'SIGTERM'); + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGTERM'); + expect(execFileSyncMock).not.toHaveBeenCalled(); + }); + + it('kills the process tree via taskkill on Windows', () => { + // A negative pid is not a process group on win32; the group kill must fall + // back to a tree kill or the timeout/cancel termination silently no-ops. + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); + killProcessGroup(12345, 'SIGTERM'); + expect(execFileSyncMock).toHaveBeenCalledWith( + 'taskkill', + ['/pid', '12345', '/T', '/F'], + { stdio: 'ignore' }, + ); + expect(processKill).not.toHaveBeenCalled(); + }); +}); + +describe('review run (handler)', () => { + let dir: string; + let cwd: string; + let outs: string[]; + let errs: string[]; + let exitCode: number | undefined; + let processKill: MockInstance; + + class FakeChild extends EventEmitter { + pid = 12345; + stdout = Object.assign(new EventEmitter(), { resume: () => {} }); + stderr = Object.assign(new EventEmitter(), { resume: () => {} }); + kill = vi.fn(); + } + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'run-handler-')); + cwd = process.cwd(); + process.chdir(dir); + outs = []; + errs = []; + exitCode = process.exitCode as number | undefined; + vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { + outs.push(String(chunk)); + return true; + }); + vi.spyOn(process.stderr, 'write').mockImplementation((chunk) => { + errs.push(String(chunk)); + return true; + }); + processKill = vi.spyOn(process, 'kill').mockImplementation(() => true); + spawnMock.mockReset(); + }); + + afterEach(() => { + process.exitCode = exitCode; + vi.restoreAllMocks(); + vi.useRealTimers(); + process.chdir(cwd); + rmSync(dir, { recursive: true, force: true }); + }); + + function runHandler(over: Record = {}): Promise { + return (runCommand.handler as (a: unknown) => Promise)({ + comment: false, + json: true, + 'fail-on': 'none', + 'timeout-minutes': 120, + 'approval-mode': 'yolo', + quiet: true, + ...over, + }); + } + + /** Child that "completes", writing (or not) a composed verdict first. */ + function armChild(exit: number, composed?: Record): void { + spawnMock.mockImplementation(() => { + const child = new FakeChild(); + setImmediate(() => { + if (composed) { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + mkdirSync(REVIEWS_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify(composed), + 'utf8', + ); + writeFileSync(join(REVIEWS_DIR, 'review.md'), '# report', 'utf8'); + } + child.emit('close', exit); + }); + return child; + }); + } + + it('republishes the composed verdict and exits 0', async () => { + // Non-default values for every republished field, so a dropped or + // hard-coded `composed?.X ?? default` mapping cannot pass. + armChild(0, { + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + baseEvent: 'REQUEST_CHANGES', + cappedBy: ['unreviewed-dimension'], + downgraded: true, + downgradedFrom: 'Request changes', + remediation: ['do x'], + }); + await runHandler(); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.event).toBe('COMMENT'); + expect(result.verdictLine).toBe('Verdict: Comment'); + expect(result.baseEvent).toBe('REQUEST_CHANGES'); + expect(result.cappedBy).toEqual(['unreviewed-dimension']); + expect(result.downgraded).toBe(true); + expect(result.downgradedFrom).toBe('Request changes'); + expect(result.remediation).toEqual(['do x']); + expect(result.reportPath).toContain('review.md'); + expect(process.exitCode).toBe(0); + }); + + it('exits 3 on a blocking verdict only when --fail-on asks for it', async () => { + armChild(0, { + event: 'REQUEST_CHANGES', + verdictLine: 'Verdict: Request changes', + }); + await runHandler({ 'fail-on': 'request-changes' }); + + expect(process.exitCode).toBe(3); + }); + + it('treats a clean child exit without a composed verdict as failure', async () => { + // The model can wander off and exit 0 without ever reaching Step 7. That is + // "no verdict", never "approve". + armChild(0); + await runHandler(); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(1); + }); + + it('captures the verdict before Step 9 cleanup sweeps it', async () => { + // The regression this command shipped with: the child runs the bundled + // skill through Step 9, whose `cleanup` deletes the composed verdict before + // the child exits. A parent that reads only after `close` sees nothing and + // reports a completed review as a failure. The capture poll must snapshot + // the verdict while the child still runs. + vi.useFakeTimers(); + let child!: FakeChild; + spawnMock.mockImplementation(() => { + // Step 6: compose-review writes the composed verdict. + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), + 'utf8', + ); + child = new FakeChild(); + return child; + }); + + const done = runHandler(); + // The capture poll snapshots the verdict while the child still runs... + await vi.advanceTimersByTimeAsync(1_000); + // ...then Step 9 runs the REAL cleanup, which sweeps the verdict... + runCleanup('local'); + outs.length = 0; // drop cleanup's "Removed temp file" stdout noise + // ...and only then does the child exit. + child.emit('close', 0); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.event).toBe('APPROVE'); + expect(result.composedPath).toContain('qwen-review-local-composed.json'); + expect(process.exitCode).toBe(0); + }); + + it('closes the child stdin so piped input cannot defeat slash detection', async () => { + armChild(0, { event: 'APPROVE', verdictLine: 'Verdict: Approve' }); + await runHandler(); + + const [, argvUsed, opts] = spawnMock.mock.calls[0] as [ + string, + string[], + { stdio: unknown[]; detached: boolean }, + ]; + expect(opts.stdio[0]).toBe('ignore'); + expect(opts.detached).toBe(true); + // --expose-gc must lead the argv: spawning argv[1] directly would drop the + // flag the memory-pressure monitor's critical tier needs (cli-entry.js + // passes it for exactly this relaunch path). + expect(argvUsed[0]).toBe('--expose-gc'); + expect(argvUsed).toContain('--prompt'); + expect(argvUsed).toContain('/review'); + }); + + it('passes the approval mode through to the child CLI', async () => { + armChild(0, { event: 'APPROVE', verdictLine: 'Verdict: Approve' }); + await runHandler({ 'approval-mode': 'default' }); + + const [, argvUsed] = spawnMock.mock.calls[0] as [string, string[]]; + const i = argvUsed.indexOf('--approval-mode'); + expect(i).toBeGreaterThan(-1); + expect(argvUsed[i + 1]).toBe('default'); + }); + + it('treats a composed verdict without a string event as no verdict', async () => { + // readComposed must refuse a file whose `event` is not a string, or a + // corrupt verdict would read as completed with event null and exit 0. + armChild(0, { event: 123, verdictLine: 'Verdict: Approve' }); + await runHandler(); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(1); + }); + + it('reports a launch failure when the child emits an error', async () => { + // A missing CLI binary or an OS that cannot fork emits `error`, not + // `close`; the handler must still settle and report "no verdict". + spawnMock.mockImplementation(() => { + const child = new FakeChild(); + setImmediate(() => child.emit('error', new Error('spawn ENOENT'))); + return child; + }); + await runHandler(); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.childExitCode).toBeNull(); + expect(process.exitCode).toBe(1); + expect(errs.join('')).toContain('failed to launch the CLI'); + }); + + it('streams child progress to stderr, never stdout, when not quiet', async () => { + // The contract: stdout carries only the result. If progress leaked to + // stdout it would interleave with the JSON a CI consumer parses. + spawnMock.mockImplementation(() => { + const child = new FakeChild(); + setImmediate(() => { + child.stdout.emit('data', Buffer.from('progress noise')); + child.emit('close', 0); + }); + return child; + }); + await runHandler({ quiet: false }); + + expect(errs.join('')).toContain('progress noise'); + expect(outs.join('')).not.toContain('progress noise'); + }); + + it('reports a timed-out run as incomplete and kills the process group', async () => { + vi.useFakeTimers(); + const child = new FakeChild(); + spawnMock.mockImplementation(() => child); + + const done = runHandler({ 'timeout-minutes': 1 }); + await vi.advanceTimersByTimeAsync(60_000); // fire the timeout + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGTERM'); + // A child that ignores SIGTERM is escalated to SIGKILL after 10 s. + await vi.advanceTimersByTimeAsync(10_000); + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGKILL'); + child.emit('close', null, 'SIGTERM'); // the kill takes effect + await done; + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.timedOut).toBe(true); + expect(result.childExitCode).toBeNull(); + expect(result.childSignal).toBe('SIGTERM'); + expect(process.exitCode).toBe(1); + }); + + it('keeps a captured verdict when the timeout fires after compose-review', async () => { + // The race the contract must survive: compose-review writes the verdict + // (Step 6) and the capture poll snapshots it, but --timeout-minutes fires + // before the child exits (Steps 7–9). The flag terminates a run "without a + // verdict", so a captured verdict still counts as completed — the kill must + // not flip exit 0 to 1 or suppress the verdict, and `timedOut` alone still + // records that the timer fired. + vi.useFakeTimers(); + let child!: FakeChild; + spawnMock.mockImplementation(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), + 'utf8', + ); + child = new FakeChild(); + return child; + }); + + const done = runHandler({ 'timeout-minutes': 1 }); + // The capture poll snapshots the verdict while the child still runs... + await vi.advanceTimersByTimeAsync(1_000); + // ...then the timeout fires and kills the group before the child exits. + await vi.advanceTimersByTimeAsync(60_000); + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGTERM'); + child.emit('close', null, 'SIGTERM'); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.timedOut).toBe(true); + expect(result.event).toBe('APPROVE'); + expect(process.exitCode).toBe(0); + }); + + it('forwards a parent signal to the child group and exits 128+signum', async () => { + // The detached child sits outside the foreground group a terminal's + // Ctrl+C signals, and a cancelled CI job sends the parent SIGTERM. + // Without forwarding, the parent dies and the review is reparented to + // PID 1, burning API calls for the full timeout. Pin the registration + // and the 128+signum mapping so a refactor cannot silently drop them. + vi.useFakeTimers(); + const child = new FakeChild(); + spawnMock.mockImplementation(() => child); + const exitSpy = vi + .spyOn(process, 'exit') + .mockImplementation(() => undefined as never); + const onSpy = vi.spyOn(process, 'on'); + + const done = runHandler({ 'timeout-minutes': 1 }); + + // All three signals must be registered. + const registered = onSpy.mock.calls + .map(([sig]) => sig) + .filter((sig) => ['SIGHUP', 'SIGINT', 'SIGTERM'].includes(sig as string)); + expect(registered).toEqual( + expect.arrayContaining(['SIGHUP', 'SIGINT', 'SIGTERM']), + ); + + const handler = onSpy.mock.calls.find( + ([sig]) => sig === 'SIGTERM', + )?.[1] as (signal: NodeJS.Signals) => void; + handler('SIGHUP'); + handler('SIGINT'); + handler('SIGTERM'); + + // The group is killed and each signal maps onto 128+signum. + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGTERM'); + expect(exitSpy).toHaveBeenNthCalledWith(1, 129); + expect(exitSpy).toHaveBeenNthCalledWith(2, 130); + expect(exitSpy).toHaveBeenNthCalledWith(3, 143); + + // The handler cleared the timeout timer: crossing it must not fire the + // timeout path (which would write its own stderr notice). + await vi.advanceTimersByTimeAsync(60_000); + expect(errs.join('')).not.toContain('timeout after'); + + child.emit('close', null, 'SIGTERM'); + await done; + }); + + it('prints the verdict line and report path in human-readable mode', async () => { + armChild(0, { event: 'APPROVE', verdictLine: 'Verdict: Approve' }); + await runHandler({ json: false }); + + const output = outs.join(''); + expect(output).toContain('Verdict: Approve'); + expect(output).toContain('Report: '); + expect(process.exitCode).toBe(0); + }); + + it('distinguishes a corrupt composed artifact from a missing one', async () => { + spawnMock.mockImplementation(() => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + '{truncated', + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }); + await runHandler({ json: false }); + + const output = outs.join(''); + expect(output).toContain('could not be parsed'); + expect(output).not.toContain('no composed verdict was produced'); + expect(process.exitCode).toBe(1); + }); + + it('preserves the exit code when writing the result to stdout throws', async () => { + // The pipe reader can go away (EPIPE) mid-write. The exit code is the + // contract a CI gate reads, so it must be set before — and survive — the + // write, not downgraded to yargs' generic exit 1 by the throw. + armChild(0, { + event: 'REQUEST_CHANGES', + verdictLine: 'Verdict: Request changes', + }); + vi.spyOn(process.stdout, 'write').mockImplementation(() => { + throw new Error('EPIPE'); + }); + + await runHandler({ 'fail-on': 'request-changes' }); + + expect(process.exitCode).toBe(3); + }); + + it('clamps a negative timeout to the 1-minute floor', async () => { + vi.useFakeTimers(); + const child = new FakeChild(); + spawnMock.mockImplementation(() => child); + + const done = runHandler({ 'timeout-minutes': -5 }); + // 59 s is under the 1-minute floor — must not fire. + await vi.advanceTimersByTimeAsync(59_000); + expect(processKill).not.toHaveBeenCalled(); + // Crossing the floor fires the timeout. + await vi.advanceTimersByTimeAsync(1_000); + child.emit('close', null, 'SIGTERM'); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.timedOut).toBe(true); + expect(process.exitCode).toBe(1); + }); + + it('floors a zero timeout to 1 minute rather than the 120-minute default', async () => { + // `|| 120` treats 0 as falsy and would silently substitute the default; + // an explicit 0 must still reach the Math.max(1, …) floor. + vi.useFakeTimers(); + const child = new FakeChild(); + spawnMock.mockImplementation(() => child); + + const done = runHandler({ 'timeout-minutes': 0 }); + await vi.advanceTimersByTimeAsync(59_000); + expect(processKill).not.toHaveBeenCalled(); + await vi.advanceTimersByTimeAsync(1_000); + expect(processKill).toHaveBeenCalledWith(-12345, 'SIGTERM'); + child.emit('close', null, 'SIGTERM'); + await done; + + const result = JSON.parse(outs.join('')); + expect(result.timedOut).toBe(true); + expect(process.exitCode).toBe(1); + }); +}); diff --git a/packages/cli/src/commands/review/run.ts b/packages/cli/src/commands/review/run.ts new file mode 100644 index 0000000000..7d54003535 --- /dev/null +++ b/packages/cli/src/commands/review/run.ts @@ -0,0 +1,495 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `qwen review run`: execute a full /review non-interactively and report the +// verdict in a machine-readable way. +// +// The review pipeline already runs headless — `qwen --prompt "/review …"` expands +// the bundled skill, launches the dimension agents, and honors the approval mode. +// What that path does NOT give a caller is a contract: the verdict lives in the +// model's prose and in files whose names the caller would have to know, the exit +// code says nothing about the review's outcome, and a piped stdin silently +// defeats slash-command detection (the runner prepends piped input, and +// `isSlashCommand` requires the FIRST character to be `/`). Every consumer that +// wants "run a review, tell me what it decided" has been re-deriving those facts +// by scraping a terminal. +// +// This command is that contract, and nothing more: it assembles the /review +// invocation, runs the CLI's own non-interactive path in a child process with +// stdin closed, and then reads the verdict from the artifact `compose-review` +// wrote — the same JSON the skill treats as the verdict authority — rather than +// from anything the model said. Progress streams to stderr; stdout carries only +// the result; the exit code distinguishes "review completed" from "review never +// reached a verdict" from "blocking verdict" (opt-in via --fail-on). + +import type { CommandModule } from 'yargs'; +import { spawn, execFileSync } from 'node:child_process'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join, resolve } from 'node:path'; +import { + writeStdoutLine, + writeStderrLineSafe, +} from '../../utils/stdioHelpers.js'; +import { REVIEW_TMP_DIR, REVIEWS_DIR } from './lib/paths.js'; +import { EFFORT_LEVELS } from './parse-args.js'; + +export interface RunReviewArgs { + target?: string; + effort?: string; + comment: boolean; + json: boolean; + failOn: 'none' | 'request-changes'; + timeoutMinutes: number; + approvalMode: string; + quiet: boolean; +} + +/** + * The composed-verdict fields this command republishes (see compose-review). + * `findings`, `model`, and `disclosures` (named by #7981) are deliberately + * absent: compose-review does not emit them as discrete fields, so there is + * nothing to republish until the composed artifact grows them. + */ +interface ComposedVerdict { + event?: string; + verdictLine?: string; + baseEvent?: string; + cappedBy?: string[]; + downgraded?: boolean; + downgradedFrom?: string | null; + remediation?: string[]; +} + +export interface RunReviewResult { + completed: boolean; + event: string | null; + verdictLine: string | null; + baseEvent: string | null; + cappedBy: string[]; + downgraded: boolean; + downgradedFrom: string | null; + remediation: string[]; + composedPath: string | null; + reportPath: string | null; + childExitCode: number | null; + childSignal: string | null; + timedOut: boolean; + durationMs: number; +} + +/** The composed verdict `compose-review` writes and Step 9 cleanup sweeps. */ +const COMPOSED_PATTERN = /^qwen-review-.*composed\.json$/; + +// How often to poll for the composed verdict while the child runs. The verdict +// sits on disk from Step 6 (compose-review) until Step 9 (cleanup) — a window +// spanning the model's between-step narration and the report write, i.e. +// seconds — so a quarter-second poll catches it with a wide margin. +const COMPOSED_POLL_MS = 250; + +// Conventional exit codes for a run cancelled by a signal (128 + signum). +const SIGNAL_EXIT_CODES: Record = { + SIGHUP: 129, + SIGINT: 130, + SIGTERM: 143, +}; +const PARENT_SIGNALS = Object.keys(SIGNAL_EXIT_CODES) as NodeJS.Signals[]; + +/** The /review invocation the child runs — built from flags, never hand-typed. */ +export function buildReviewPrompt(args: { + target?: string; + effort?: string; + comment?: boolean; +}): string { + const parts = ['/review']; + if (args.target) { + // The child re-tokenizes this string; a target carrying whitespace or a + // leading dash would split into extra tokens (`123 --comment` would + // silently authorise posting), and a quote is stripped by the tokenizer + // (`src/it's.ts` would re-target to `src/its.ts`) — refuse anything but a + // single clean token. + if ( + /\s/.test(args.target) || + args.target.startsWith('-') || + /['"]/.test(args.target) + ) { + throw new Error( + `Invalid review target ${JSON.stringify(args.target)}: expected a single PR number, PR URL, or file path`, + ); + } + parts.push(args.target); + } + if (args.effort) parts.push(`--effort ${args.effort}`); + if (args.comment) parts.push('--comment'); + return parts.join(' '); +} + +/** + * The newest file under `dir` matching `pattern` whose mtime is at or after + * `startMs`, or null. Pre-existing artifacts from earlier reviews in the same + * repo must not be mistaken for this run's verdict — a stale composed JSON says + * whatever the LAST review decided, which is exactly the wrong thing to + * republish — so anything older than the run is invisible here. + */ +export function newestArtifactSince( + dir: string, + pattern: RegExp, + startMs: number, +): string | null { + let best: { path: string; mtime: number } | null = null; + let names: string[]; + try { + names = readdirSync(dir); + } catch { + return null; // no directory — the review never got far enough to create it + } + for (const name of names) { + if (!pattern.test(name)) continue; + const path = join(dir, name); + let mtime: number; + try { + mtime = statSync(path).mtimeMs; + } catch { + continue; + } + if (mtime < startMs) continue; + if (!best || mtime > best.mtime) best = { path, mtime }; + } + return best ? best.path : null; +} + +/** + * Exit code contract: 0 = the review completed (whatever it decided); 1 = it + * never reached a verdict (child failed, timed out with no verdict captured, + * or left no composed artifact); 3 = it completed AND the caller asked + * --fail-on request-changes AND the event is REQUEST_CHANGES. 3, not 2 — yargs + * exits 1 on usage errors and some shells reserve 2, so a CI gate can tell + * "review is blocking" from "the tool broke" without parsing anything. + */ +export function exitCodeFor( + completed: boolean, + event: string | null, + failOn: 'none' | 'request-changes', +): number { + if (!completed) return 1; + if (failOn === 'request-changes' && event === 'REQUEST_CHANGES') return 3; + return 0; +} + +function readComposed(path: string): ComposedVerdict | null { + try { + const parsed = JSON.parse(readFileSync(path, 'utf8')) as ComposedVerdict; + // The one field everything downstream keys on. A file without it is not a + // composed verdict, whatever its name says. + return typeof parsed.event === 'string' ? parsed : null; + } catch { + return null; + } +} + +/** + * Terminate the child's process group — the detached relaunch wrapper AND the + * real review it spawned. On POSIX a negative pid names the group; on Windows + * there are no POSIX process groups and a negative pid is meaningless, so fall + * back to `taskkill /T`, which walks the tree the detached child spawned. Both + * are best-effort: killing a group that is already gone throws, and that is + * fine. + */ +export function killProcessGroup(pid: number, signal: NodeJS.Signals): void { + if (process.platform === 'win32') { + try { + execFileSync('taskkill', ['/pid', String(pid), '/T', '/F'], { + stdio: 'ignore', + }); + } catch { + // Already dead, or taskkill unavailable. + } + return; + } + try { + process.kill(-pid, signal); + } catch { + // Already dead. + } +} + +async function runReview(args: RunReviewArgs): Promise { + const startMs = Date.now(); + const prompt = buildReviewPrompt(args); + + // The verdict cutoff carries slack: a coarse filesystem clock can stamp a + // file a moment BEFORE the Date.now() captured at run start, and a review's + // own verdict must not be discarded over clock granularity. Artifacts from a + // previous review are minutes old, far outside any slack. + const cutoffMs = startMs - 2_000; + + // Re-enter THIS build's CLI, not whatever `qwen` PATH resolves to — the same + // version-skew rule the skill's own subprocesses follow via QWEN_CODE_CLI. + // process.argv[1] is the entry that is already running this command. + // --expose-gc comes first, exactly as the relaunch wrapper passes it + // (cli-entry.js): a full review is the longest, most memory-hungry session + // the CLI runs, and spawning argv[1] directly would silently drop the flag + // the memory-pressure monitor's critical tier needs to call global.gc(). + const child = spawn( + process.execPath, + [ + '--expose-gc', + process.argv[1], + '--prompt', + prompt, + '--approval-mode', + args.approvalMode, + ], + { + // stdin CLOSED, not inherited: piped input would be prepended to the + // prompt and the leading `/` would no longer be the first character — + // the slash command would reach the model as plain text. + stdio: ['ignore', 'pipe', 'pipe'], + // The CLI relaunches itself in a child (for --max-old-space-size), so + // the pid we spawn is a wrapper whose grandchild is the real review. + // A new process group lets the timeout kill reach both. + detached: true, + }, + ); + + if (!args.quiet) { + // Progress belongs on stderr; stdout is reserved for the result. A throw + // here (EPIPE once the pipe reader exits) would crash the parent and orphan + // the child review, so the write stays incidental. + const writeProgress = (chunk: Buffer): void => { + try { + process.stderr.write(chunk); + } catch { + // stderr is gone; the verdict, not the progress, is what matters. + } + }; + child.stdout?.on('data', writeProgress); + child.stderr?.on('data', writeProgress); + } else { + child.stdout?.resume(); + child.stderr?.resume(); + } + + // The composed verdict is transient: the child's Step 9 `cleanup` sweeps + // every `.qwen/tmp/qwen-review--*` file — including it — before the + // child exits. Reading it only AFTER `close` therefore sees nothing and + // reports a review that completed as one that failed. Snapshot it the moment + // compose-review writes it: the first verdict newer than the run start is + // this run's, and caching it in memory survives the sweep. + let capturedPath: string | null = null; + let capturedVerdict: ComposedVerdict | null = null; + const captureTimer = setInterval(() => { + if (capturedVerdict !== null) return; + const path = newestArtifactSince( + REVIEW_TMP_DIR, + COMPOSED_PATTERN, + cutoffMs, + ); + if (path === null) return; + // A half-written file fails to parse; the next tick retries it. + const verdict = readComposed(path); + if (verdict !== null) { + capturedPath = path; + capturedVerdict = verdict; + } + }, COMPOSED_POLL_MS); + + let timedOut = false; + const timeoutMs = args.timeoutMinutes * 60_000; + const timer = setTimeout(() => { + timedOut = true; + // Safe write: a throw on EPIPE would skip the kill below and leave the + // child review running on, burning compute and model API calls. + writeStderrLineSafe( + `review run: timeout after ${args.timeoutMinutes} minutes — terminating the review`, + ); + // Kill the process group, not just the wrapper: child.kill() would only + // reach the relaunch wrapper, leaving the real review reparented to PID 1 + // and still burning API calls. + const pid = child.pid; + if (pid !== undefined) { + killProcessGroup(pid, 'SIGTERM'); + setTimeout(() => killProcessGroup(pid, 'SIGKILL'), 10_000).unref(); + } + }, timeoutMs); + + // The child is detached (its own process group) so the timeout kill can reach + // the relaunch wrapper's grandchild — but that also puts it outside the + // foreground group a terminal's Ctrl+C signals, and a cancelled CI job sends + // the parent SIGTERM. Without forwarding, the parent dies and the review is + // reparented to PID 1, burning model API calls for up to the full timeout + // (and, with --comment, can still post after the job that spawned it is + // gone). Terminate the group on the way out, mirroring the timeout path. + const onParentSignal = (signal: NodeJS.Signals): void => { + clearTimeout(timer); + clearInterval(captureTimer); + const pid = child.pid; + if (pid !== undefined) { + // SIGTERM's default action terminates the node group; the parent exits + // immediately, so there is no later moment to escalate to SIGKILL. + killProcessGroup(pid, 'SIGTERM'); + } + process.exit(SIGNAL_EXIT_CODES[signal] ?? 1); + }; + for (const signal of PARENT_SIGNALS) process.on(signal, onParentSignal); + + const childOutcome = await new Promise<{ + code: number | null; + signal: string | null; + }>((resolvePromise) => { + child.on('close', (code, signal) => resolvePromise({ code, signal })); + child.on('error', (err) => { + writeStderrLineSafe( + `review run: failed to launch the CLI: ${err.message}`, + ); + resolvePromise({ code: null, signal: null }); + }); + }); + const childExitCode = childOutcome.code; + const childSignal = childOutcome.signal; + clearTimeout(timer); + clearInterval(captureTimer); + for (const signal of PARENT_SIGNALS) process.off(signal, onParentSignal); + + // The verdict is what compose-review wrote, not what the child printed. A + // clean child exit without a composed artifact means the run wandered off + // before Step 7 — that is "no verdict", not "approve". Prefer the verdict + // captured during the run (Step 9 cleanup has usually swept the file by now); + // fall back to a disk scan for a child that died before cleanup ran. + // Annotated, not inferred: capturedPath/capturedVerdict are mutated only + // inside the poll closure, so control-flow analysis would narrow them to + // their `null` initializer and reject the fallback reassignment below. + let composedPath: string | null = capturedPath; + let composed: ComposedVerdict | null = capturedVerdict; + if (composed === null) { + composedPath = newestArtifactSince( + REVIEW_TMP_DIR, + COMPOSED_PATTERN, + cutoffMs, + ); + composed = composedPath ? readComposed(composedPath) : null; + } + const reportPath = newestArtifactSince(REVIEWS_DIR, /\.md$/, cutoffMs); + + const completed = composed !== null; + const result: RunReviewResult = { + completed, + event: composed?.event ?? null, + verdictLine: composed?.verdictLine ?? null, + baseEvent: composed?.baseEvent ?? null, + cappedBy: composed?.cappedBy ?? [], + downgraded: composed?.downgraded ?? false, + downgradedFrom: composed?.downgradedFrom ?? null, + remediation: composed?.remediation ?? [], + composedPath: composedPath ? resolve(composedPath) : null, + reportPath: reportPath ? resolve(reportPath) : null, + childExitCode, + childSignal, + timedOut, + durationMs: Date.now() - startMs, + }; + + // Assign the exit code BEFORE writing the result: a stdout write can throw + // (EPIPE once the pipe reader exits), and the exit code — not the prose — is + // the contract a CI gate reads. A throw must not downgrade a blocking verdict + // (exit 3) to yargs' generic failure (exit 1). + process.exitCode = exitCodeFor(completed, result.event, args.failOn); + + try { + if (args.json) { + writeStdoutLine(JSON.stringify(result, null, 2)); + } else if (completed) { + writeStdoutLine(result.verdictLine ?? `Event: ${result.event}`); + if (result.reportPath) writeStdoutLine(`Report: ${result.reportPath}`); + } else { + const detail = + composedPath !== null + ? `a composed verdict was found at ${resolve(composedPath)} but could not be parsed` + : 'no composed verdict was produced'; + writeStdoutLine( + timedOut + ? 'Review did not complete: timed out.' + : `Review did not complete: ${detail}` + + `${childExitCode !== null ? ` (CLI exit ${childExitCode})` : ''}` + + `${childSignal !== null ? ` (killed by ${childSignal})` : ''}.`, + ); + } + } catch { + // stdout is gone; the exit code above is the contract, not this prose. + } +} + +export const runCommand: CommandModule = { + command: 'run [target]', + describe: + 'Run a full /review non-interactively and print the verdict (machine-readable with --json)', + builder: (yargs) => + yargs + .positional('target', { + type: 'string', + describe: + 'What to review: a PR number, a PR URL, or a file path; omit to review the local working tree', + }) + .option('effort', { + type: 'string', + choices: [...EFFORT_LEVELS], + describe: + 'The review effort. Defaults to the skill default for the target (high for a PR, medium locally).', + }) + .option('comment', { + type: 'boolean', + default: false, + describe: + 'Authorise posting the review to GitHub (PR targets only) — same meaning as `/review --comment`', + }) + .option('json', { + type: 'boolean', + default: false, + describe: 'Print the full result as JSON on stdout', + }) + .option('fail-on', { + type: 'string', + choices: ['none', 'request-changes'], + default: 'none', + describe: + 'Exit 3 when the review completes with this outcome — lets CI gate on the verdict without parsing output', + }) + .option('timeout-minutes', { + type: 'number', + default: 120, + describe: + 'Terminate the review after this long without a verdict (exit 1)', + }) + .option('approval-mode', { + type: 'string', + default: 'yolo', + choices: ['plan', 'default', 'auto-edit', 'auto', 'yolo'], + describe: + 'Approval mode for the child CLI. The default is yolo: headless runs cannot answer ' + + 'confirmation prompts, and anything still unapproved would be auto-denied mid-review.', + }) + .option('quiet', { + type: 'boolean', + default: false, + describe: 'Suppress the child CLI progress stream on stderr', + }), + handler: async (argv) => { + await runReview({ + target: argv['target'] as string | undefined, + effort: argv['effort'] as string | undefined, + comment: Boolean(argv['comment']), + json: Boolean(argv['json']), + failOn: (argv['fail-on'] as 'none' | 'request-changes') ?? 'none', + // `|| 120` would treat an explicit `--timeout-minutes 0` as falsy and + // silently substitute the default; decide default-vs-value by finiteness + // so 0 still reaches the 1-minute floor. + timeoutMinutes: Number.isFinite(Number(argv['timeout-minutes'])) + ? Math.max(1, Number(argv['timeout-minutes'])) + : 120, + approvalMode: String(argv['approval-mode'] ?? 'yolo'), + quiet: Boolean(argv['quiet']), + }); + }, +}; From 19761af0724941c03fb20b4368039b27e23dc444 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:40:18 +0800 Subject: [PATCH 02/38] fix(cli): stamp QWEN_CODE_CLI at the workspace entry and publish QWEN_CODE_MODEL (#7993) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): stamp QWEN_CODE_CLI at the workspace entry and publish the active model as QWEN_CODE_MODEL Skill subprocesses shell out through `"${QWEN_CODE_CLI:-qwen}"`. The npm entry (scripts/cli-entry.js) stamps QWEN_CODE_CLI, but the workspace entry (packages/cli/dist/index.js) never did — so a dev run, or any direct `node dist/index.js` launch, leaves the variable unset and every review subcommand the /review skill issues silently lands in whatever global `qwen` PATH resolves to. Measured on a live run: a freshly built CLI's review pipeline executed entirely on a global v0.21.0 — `script-lint` did not exist there, so the deterministic gate the skill expected was silently absent, and any behavioral fix to the review CLI is inert in such runs. Stamp the entry in runCliEntryPoint, first-writer-wins: an outer launcher (cli-entry.js, the desktop shim) has already stamped in-process and must keep winning; an empty value counts as unset, matching the consumer's `:-` semantics. The entry is derived as `../index.js` from the compiled dist/src/cli.js — the shebang-bearing bin — and skipped entirely for non-file schemes (vitest) and unbuilt layouts (tsx dev runs keep today's fallback). tsc emits dist/index.js as 0644 and the spawn-time filter blanks a non-execable entry, so the stamp grants 0o755 best-effort; a failed chmod degrades to today's `qwen` fallback. Separately, subprocesses had no authoritative way to learn the ACTIVE model: the /review skill's compose step wants a modelId, and the orchestrator resorts to reading settings files — wrong under QWEN_HOME isolation and after /model switches (measured: a report stamped with a model that never ran the review). Publish QWEN_CODE_MODEL exactly the way QWEN_CODE_SESSION_ID is published: the first Config claims the process-global slot, only the claiming instance republishes (on refreshAuth and every model-change notification), and getShellContextEnvVars passes it through, omitted when absent. The daemon limitation is the session ID's own — later sessions read the first session's model — and is documented at both the producer and the consumer. * fix(core): clarify QWEN_CODE_MODEL daemon comment and cover refreshAuth republish (#7993) * test(cli): cover stampCliEntryEnv wiring in runCliEntryPoint (#7993) * fix(core): publish QWEN_CODE_MODEL per session and preserve entry mode on stamp (#7993) Address review feedback: - Key QWEN_CODE_MODEL on the session (registerSessionModel/getSessionModel), mirroring the project dir, so daemon-mode subprocesses read their own session's active model instead of the first session's. The process-global slot remains as the single-session CLI fallback. This also neutralizes the order-dependent claim: a throwaway Config's registration is keyed under a session id no real spawn resolves. - stampCliEntryEnv now adds exec bits to the existing mode (mode | 0o111) rather than setting 0o755, so a private 0o600 checkout becomes execable without becoming world-readable. - Cross-reference scripts/dev.js and scripts/start.js in the stamp doc comment and note the bundled `node dist/cli.js` launch is intentionally not stamped. - Widen the AuthType test mock to include QWEN_OAUTH, pin the stamp-before-run ordering in the wiring test, and cover the per-session model lookup. * fix(cli): correct comment on Vite rewrite mechanism in protocol guard (#7993) * fix(core): re-key per-session model registry on startNewSession (#7993) startNewSession minted a new session id and re-stamped QWEN_CODE_SESSION_ID but left the per-session model registry keyed on the outgoing id. After /clear (or /reset, /new, /resume) a non-owner Config's subprocesses then resolved the model by the new id, missed, and fell back to another session's value. Unregister the old entry and republish under the new id. Also correct the stampCliEntryEnv comments: npm start / npm run dev route through scripts/start.js and scripts/dev.js, which stamp QWEN_CODE_CLI themselves, so the only uncovered launcher is a direct node dist/index.js. --------- Co-authored-by: verify Co-authored-by: qwen-code-ci-bot Co-authored-by: Qwen Code Bot --- packages/cli/src/cli.test.ts | 119 ++++++++++++ packages/cli/src/cli.ts | 88 ++++++++- .../src/config/config-session-env.test.ts | 172 ++++++++++++++---- packages/core/src/config/config.ts | 56 ++++++ packages/core/src/utils/sessionIdContext.ts | 33 ++++ .../core/src/utils/shellContextEnv.test.ts | 81 +++++++++ packages/core/src/utils/shellContextEnv.ts | 25 ++- 7 files changed, 539 insertions(+), 35 deletions(-) diff --git a/packages/cli/src/cli.test.ts b/packages/cli/src/cli.test.ts index a664d8fee5..dbd136921b 100644 --- a/packages/cli/src/cli.test.ts +++ b/packages/cli/src/cli.test.ts @@ -33,6 +33,7 @@ import { resolveBootstrapRoute, runCliEntry, runCliEntryPoint, + stampCliEntryEnv, } from './cli.js'; const mocks = vi.hoisted(() => ({ @@ -358,6 +359,110 @@ describe('runCliEntry', () => { }); }); +describe('stampCliEntryEnv', () => { + // Isolated because the CLI exports QWEN_CODE_CLI to every shell it spawns — + // a test run started from inside a qwen session inherits it. + let originalCli: string | undefined; + let tempDir: string; + + beforeEach(() => { + originalCli = process.env['QWEN_CODE_CLI']; + delete process.env['QWEN_CODE_CLI']; + tempDir = mkdtempSync(path.join(tmpdir(), 'qwen-entry-stamp-')); + }); + + afterEach(() => { + if (originalCli !== undefined) { + process.env['QWEN_CODE_CLI'] = originalCli; + } else { + delete process.env['QWEN_CODE_CLI']; + } + rmSync(tempDir, { recursive: true, force: true }); + }); + + it('stamps the built bin entry so skill shell-outs reach THIS build', () => { + // A direct workspace launch (`node dist/index.js`) never passes through + // scripts/cli-entry.js, so without this stamp every + // `"${QWEN_CODE_CLI:-qwen}"` resolved a global install off PATH. + const entry = path.join(tempDir, 'index.js'); + writeFileSync(entry, '#!/usr/bin/env node\nconsole.log("hi");\n'); + + stampCliEntryEnv(entry); + + expect(process.env['QWEN_CODE_CLI']).toBe(entry); + }); + + it("never overwrites an outer launcher's stamp", () => { + // cli-entry.js may have selected a standalone shim, and the desktop app + // stamps its vendored bundle — both know launch details this module + // cannot see, and both run before runCliEntryPoint in the same process. + const entry = path.join(tempDir, 'index.js'); + writeFileSync(entry, '#!/usr/bin/env node\n'); + process.env['QWEN_CODE_CLI'] = '/outer/launcher/qwen'; + + stampCliEntryEnv(entry); + + expect(process.env['QWEN_CODE_CLI']).toBe('/outer/launcher/qwen'); + }); + + it('treats an inherited empty string as unset', () => { + // A parent session's spawn filter writes '' for an entry its shell could + // not exec. That verdict is about the parent's entry — this build must + // still stamp its own. + const entry = path.join(tempDir, 'index.js'); + writeFileSync(entry, '#!/usr/bin/env node\n'); + process.env['QWEN_CODE_CLI'] = ''; + + stampCliEntryEnv(entry); + + expect(process.env['QWEN_CODE_CLI']).toBe(entry); + }); + + it('grants the execute bit tsc never emits, so the spawn filter passes the stamp', () => { + // tsc writes dist/index.js as 0644 and only npm's bin-link chmods it; the + // spawn-time filter in core blanks a shebang-bearing entry without X_OK, + // which would turn this stamp into a no-op on every plain-build checkout. + const entry = path.join(tempDir, 'index.js'); + writeFileSync(entry, '#!/usr/bin/env node\n', { mode: 0o644 }); + + stampCliEntryEnv(entry); + + expect(process.env['QWEN_CODE_CLI']).toBe(entry); + expect(statSync(entry).mode & 0o111).not.toBe(0); + }); + + it('leaves the slot unset when the derived entry does not exist', () => { + stampCliEntryEnv(path.join(tempDir, 'no', 'such', 'index.js')); + + expect(process.env['QWEN_CODE_CLI']).toBeUndefined(); + }); + + it('derives the bin entry one level up from the compiled module', () => { + // cli.ts emits to dist/src/cli.js and the shebang bin is dist/index.js — + // one level up, not two. Two lands on the unbuilt packages/cli/index.js, + // which fails the existence check and silently never stamps, and no other + // test can catch that: the derivation is only reachable under a built + // layout, where vitest never runs. + const source = readFileSync('src/cli.ts', 'utf8'); + expect(source).toContain("new URL('../index.js', import.meta.url)"); + expect( + new URL('../index.js', 'file:///repo/packages/cli/dist/src/cli.js') + .pathname, + ).toBe('/repo/packages/cli/dist/index.js'); + }); + + it('default derivation never throws and never stamps outside a built layout', () => { + // Under vitest Vite rewrites new URL(…, import.meta.url) to a non-file + // URL, and in dev runs the derived ../index.js is the unbuilt + // packages/cli/index.js. Both must keep the bare-`qwen` fallback — a + // failed derivation taking the CLI down would be worse than the version + // skew this stamp exists to fix. + stampCliEntryEnv(); + + expect(process.env['QWEN_CODE_CLI']).toBeUndefined(); + }); +}); + describe('bootstrap import boundaries', () => { it('keeps fast-path-only dependencies out of static imports', () => { const source = readFileSync('src/cli.ts', 'utf8'); @@ -1091,4 +1196,18 @@ describe('bootstrap error handling', () => { expect(output).toContain('Error handler failed:'); expect(output).toContain('handler failed'); }); + + it('wires stampCliEntryEnv into the entry point', () => { + const source = readFileSync('src/cli.ts', 'utf8'); + const entryPoint = source.slice( + source.indexOf('export async function runCliEntryPoint'), + ); + expect(entryPoint).toContain('stampCliEntryEnv()'); + // "First thing in runCliEntryPoint" is the property the doc relies on: the + // stamp must land before the CLI runs, not merely somewhere in the body — + // a stamp moved below `await run()` would still pass a contains() check. + expect(entryPoint.indexOf('stampCliEntryEnv()')).toBeLessThan( + entryPoint.indexOf('await run()'), + ); + }); }); diff --git a/packages/cli/src/cli.ts b/packages/cli/src/cli.ts index 41d07de0a3..cfda29414b 100644 --- a/packages/cli/src/cli.ts +++ b/packages/cli/src/cli.ts @@ -4,7 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { pathToFileURL } from 'node:url'; +import { + accessSync, + chmodSync, + constants, + existsSync, + statSync, +} from 'node:fs'; +import { fileURLToPath, pathToFileURL } from 'node:url'; import type { ArgumentsCamelCase, Argv, Options } from 'yargs'; import { normalizeServeFastPathArgv } from './serve/fast-path-argv.js'; import { initStartupProfiler } from './utils/startupProfiler.js'; @@ -449,10 +456,89 @@ function writeStderrLine(line: string): void { process.stderr.write(line.endsWith('\n') ? line : `${line}\n`); } +/** + * The entry a subprocess should call to reach THIS build, consumed by shell + * children as `"${QWEN_CODE_CLI:-qwen}"` (see getShellContextEnvVars in core). + * The npm bin wrapper (scripts/cli-entry.js) stamps installed launches, but a + * workspace launch — a direct `node dist/index.js` — never passes through + * it (the npm `start` and `dev` scripts stamp QWEN_CODE_CLI in their own + * launchers), so every skill shell-out resolved `qwen` off PATH: a different + * install, silently. + * + * Stamps the bin entry (dist/index.js), not this module: cli.ts compiles to + * dist/src/cli.js, which carries no shebang, and the spawn-time filter blanks + * an entry a shell cannot exec. Skipped when the derived path does not exist + * (dev runs execute .ts sources with no built entry; the bare-`qwen` fallback + * is the pre-existing behavior there) and when the module was not loaded from + * the filesystem at all — under test runners, Vite statically rewrites the + * new URL(…, import.meta.url) expression to a non-file URL, and the stamp + * must never take the CLI down. + * + * The execute bit is granted here when missing, best-effort: the stamped file + * must be shell-execable, but tsc emits dist/index.js as 0644 and only npm's + * bin-link ever chmods it — on a plain `npm run build` checkout the spawn + * filter would blank the stamp and the version skew this exists to fix would + * survive. A failed chmod keeps the old fallback: the filter writes '' and + * subprocesses run `qwen`. + * + * First writer wins, unlike the wrapper's unconditional assignment: an + * already-set value may come from an outer launcher in THIS process — + * cli-entry.js selecting a standalone shim, or the desktop app's vendored + * bundle — which knows launch details this module cannot see and must not be + * overwritten. The cost is that a value inherited from a PARENT qwen session + * also survives, since the two cases are indistinguishable here; the primary + * skew scenario — a workspace launch from a plain terminal — has the slot + * unset either way. Empty counts as unset: a parent session's spawn filter + * writes '' for an entry its shell could not exec, and that verdict is about + * the parent's entry, not this build's. + * + * scripts/dev.js and scripts/start.js assign QWEN_CODE_CLI unconditionally — + * the opposite policy on purpose, not an oversight: those files ARE the outer + * launcher (they spawn the CLI as a child and must re-point an inherited value + * at this build), whereas this module runs in-process AFTER an outer launcher + * may already have stamped, so it yields. The bundled `node dist/cli.js` launch + * (the desktop error message's instruction) is not stamped either — cli.js sits + * at the package root, so the derived ../index.js does not exist and the + * existence check skips it, consistent with this PR's workspace-entry scope. + */ +export function stampCliEntryEnv(entryPath?: string): void { + if (process.env['QWEN_CODE_CLI']) { + return; + } + let entry = entryPath; + if (entry === undefined) { + // dist/src/cli.js → dist/index.js. In dev (src/cli.ts) this lands on the + // unbuilt packages/cli/index.js and the existence check below skips it. + const entryUrl = new URL('../index.js', import.meta.url); + if (entryUrl.protocol !== 'file:') { + return; + } + entry = fileURLToPath(entryUrl); + } + if (existsSync(entry)) { + try { + accessSync(entry, constants.X_OK); + } catch { + try { + // Add exec bits to whatever mode the build/umask chose, rather than + // setting 0o755 — a deliberately-private 0o600 checkout becomes + // execable without also becoming world-readable. + chmodSync(entry, statSync(entry).mode | 0o111); + } catch { + // Not chmoddable (read-only checkout): the spawn filter blanks the + // stamp and subprocesses fall back to `qwen`, as before this stamp. + } + } + process.env['QWEN_CODE_CLI'] = entry; + } +} + export async function runCliEntryPoint( run: () => Promise = runCliEntry, handleError: (error: unknown) => Promise = handleCriticalError, ): Promise { + stampCliEntryEnv(); + process.on('uncaughtException', (error) => { if (isExpectedPtyRaceError(error)) { return; diff --git a/packages/core/src/config/config-session-env.test.ts b/packages/core/src/config/config-session-env.test.ts index 30ae571883..18c11d4fd4 100644 --- a/packages/core/src/config/config-session-env.test.ts +++ b/packages/core/src/config/config-session-env.test.ts @@ -7,14 +7,16 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; /** - * Tests for the module-level `sessionEnvClaimed` guard in Config. + * Tests for the module-level `sessionEnvClaimed` and `modelEnvClaimed` + * guards in Config. * - * The guard ensures that only the first Config instance in a process sets - * `process.env['QWEN_CODE_SESSION_ID']`, preventing throwaway instances - * (e.g. telemetry-only) from overwriting the real session's ID. + * The guards ensure that only the first Config instance in a process sets + * `process.env['QWEN_CODE_SESSION_ID']` / `process.env['QWEN_CODE_MODEL']`, + * preventing throwaway instances (e.g. telemetry-only) from overwriting the + * real session's values. * * We use `vi.isolateModules` to get a fresh module scope (resetting the - * module-level flag) for each test. + * module-level flags) for each test. */ // Shared mocks needed by Config constructor @@ -41,7 +43,7 @@ vi.mock('../core/contentGenerator.js', () => ({ }), createContentGeneratorConfig: vi.fn().mockReturnValue({}), createContentGenerator: vi.fn().mockReturnValue({}), - AuthType: { API_KEY: 'apiKey' }, + AuthType: { USE_GEMINI: 'gemini', QWEN_OAUTH: 'qwen-oauth' }, })); vi.mock('../core/baseLlmClient.js'); vi.mock('../core/toolHookTriggers.js', () => ({ @@ -92,6 +94,7 @@ vi.mock('../memory/const.js', () => ({ import * as fs from 'node:fs'; import type { Mock } from 'vitest'; import type { ConfigParameters } from './config.js'; +import type { ContentGeneratorConfig } from '../core/contentGenerator.js'; const baseParams: ConfigParameters = { cwd: '/tmp', @@ -110,36 +113,44 @@ const baseParams: ConfigParameters = { // The reset is load-bearing for what these tests check, so give them headroom. vi.setConfig({ testTimeout: 30_000, hookTimeout: 30_000 }); -describe('Config sessionEnvClaimed guard', () => { - let originalEnv: string | undefined; +let originalEnv: string | undefined; +let originalModelEnv: string | undefined; - beforeEach(() => { - originalEnv = process.env['QWEN_CODE_SESSION_ID']; +beforeEach(() => { + originalEnv = process.env['QWEN_CODE_SESSION_ID']; + delete process.env['QWEN_CODE_SESSION_ID']; + originalModelEnv = process.env['QWEN_CODE_MODEL']; + delete process.env['QWEN_CODE_MODEL']; + + (fs.existsSync as Mock).mockReturnValue(true); + (fs.readdirSync as Mock).mockReturnValue([]); + (fs.statSync as Mock).mockReturnValue({ + isDirectory: vi.fn().mockReturnValue(true), + }); + vi.mocked(fs.realpathSync).mockImplementation((p) => String(p)); + (fs.mkdirSync as Mock).mockImplementation(() => undefined); + (fs.writeFileSync as Mock).mockImplementation(() => undefined); + (fs.renameSync as Mock).mockImplementation(() => undefined); + (fs.copyFileSync as Mock).mockImplementation(() => undefined); + (fs.unlinkSync as Mock).mockImplementation(() => undefined); + (fs.readFileSync as Mock).mockImplementation(() => undefined); +}); + +afterEach(() => { + if (originalEnv !== undefined) { + process.env['QWEN_CODE_SESSION_ID'] = originalEnv; + } else { delete process.env['QWEN_CODE_SESSION_ID']; + } + if (originalModelEnv !== undefined) { + process.env['QWEN_CODE_MODEL'] = originalModelEnv; + } else { + delete process.env['QWEN_CODE_MODEL']; + } + vi.resetModules(); +}); - (fs.existsSync as Mock).mockReturnValue(true); - (fs.readdirSync as Mock).mockReturnValue([]); - (fs.statSync as Mock).mockReturnValue({ - isDirectory: vi.fn().mockReturnValue(true), - }); - vi.mocked(fs.realpathSync).mockImplementation((p) => String(p)); - (fs.mkdirSync as Mock).mockImplementation(() => undefined); - (fs.writeFileSync as Mock).mockImplementation(() => undefined); - (fs.renameSync as Mock).mockImplementation(() => undefined); - (fs.copyFileSync as Mock).mockImplementation(() => undefined); - (fs.unlinkSync as Mock).mockImplementation(() => undefined); - (fs.readFileSync as Mock).mockImplementation(() => undefined); - }); - - afterEach(() => { - if (originalEnv !== undefined) { - process.env['QWEN_CODE_SESSION_ID'] = originalEnv; - } else { - delete process.env['QWEN_CODE_SESSION_ID']; - } - vi.resetModules(); - }); - +describe('Config sessionEnvClaimed guard', () => { it('first Config sets process.env QWEN_CODE_SESSION_ID to its sessionId', async () => { const { Config } = await import('./config.js'); const config = new Config({ ...baseParams }); @@ -179,3 +190,98 @@ describe('Config sessionEnvClaimed guard', () => { expect(process.env['QWEN_CODE_SESSION_ID']).not.toBe(originalSessionId); }); }); + +describe('Config modelEnvClaimed guard', () => { + it('first Config publishes its model to QWEN_CODE_MODEL', async () => { + const { Config } = await import('./config.js'); + new Config({ ...baseParams }); + + expect(process.env['QWEN_CODE_MODEL']).toBe('test-model'); + }); + + it('a later Config does not overwrite the claimed slot', async () => { + const { Config } = await import('./config.js'); + new Config({ ...baseParams }); + + // Second Config (daemon side-session or telemetry-only throwaway) + new Config({ ...baseParams, model: 'other-model' }); + + expect(process.env['QWEN_CODE_MODEL']).toBe('test-model'); + }); + + it('only the claiming Config republishes on setModel', async () => { + const { Config } = await import('./config.js'); + const owner = new Config({ ...baseParams }); + const later = new Config({ ...baseParams, model: 'other-model' }); + + // Simulate /model on the live session + await owner.setModel('switched-model'); + expect(process.env['QWEN_CODE_MODEL']).toBe('switched-model'); + + // A non-owner's switch must not touch the process-global slot + await later.setModel('hijacked-model'); + expect(process.env['QWEN_CODE_MODEL']).toBe('switched-model'); + }); + + it('republishes on refreshAuth when the resolved model changes', async () => { + const { Config } = await import('./config.js'); + // Import from the same (mocked) module instance the cold config.js import + // above binds to, so the re-mock below is what refreshAuth actually calls. + const { resolveContentGeneratorConfigWithSources, AuthType } = await import( + '../core/contentGenerator.js' + ); + const config = new Config({ ...baseParams }); + expect(process.env['QWEN_CODE_MODEL']).toBe('test-model'); + + // Auth flows call refreshAuth directly — no model-change listener fires — + // and the resolved model can differ from the pre-auth one; the slot must + // follow it so subprocesses report the model that is actually active. + vi.mocked(resolveContentGeneratorConfigWithSources).mockReturnValue({ + config: { + model: 'auth-resolved-model', + apiKey: 'k', + } as ContentGeneratorConfig, + sources: {}, + }); + await config.refreshAuth(AuthType.USE_GEMINI); + + expect(process.env['QWEN_CODE_MODEL']).toBe('auth-resolved-model'); + }); + + it("registers each Config's model per session, so a daemon side-session reads its own", async () => { + const { Config } = await import('./config.js'); + // Import from the same cold module graph the Config above bound to, so this + // reads the registry the constructor actually wrote. + const { getSessionModel } = await import('../utils/sessionIdContext.js'); + const first = new Config({ ...baseParams }); + const later = new Config({ ...baseParams, model: 'other-model' }); + + // The process-global slot is first-writer-wins (covered above), but the + // per-session registry holds EACH session's model — this is what daemon + // mode reads at spawn time, so a later session is not stuck reporting the + // first session's model. + expect(getSessionModel(first.getSessionId())).toBe('test-model'); + expect(getSessionModel(later.getSessionId())).toBe('other-model'); + }); + + it('re-keys the per-session model registry on startNewSession', async () => { + const { Config } = await import('./config.js'); + const { getSessionModel } = await import('../utils/sessionIdContext.js'); + // Owner boots first and claims the process-global slot; the side-session + // is the non-owner Config whose subprocesses read the per-session registry. + new Config({ ...baseParams }); + const side = new Config({ ...baseParams, model: 'other-model' }); + const oldSessionId = side.getSessionId(); + expect(getSessionModel(oldSessionId)).toBe('other-model'); + + // /clear (and /reset, /new, /resume) flow through startNewSession, which + // mints a new session id. The registry entry must move with it — leaving + // it keyed on the old id would make the side-session's subprocesses miss + // and fall back to the owner's model. + const newSessionId = side.startNewSession(); + + expect(newSessionId).not.toBe(oldSessionId); + expect(getSessionModel(newSessionId)).toBe('other-model'); + expect(getSessionModel(oldSessionId)).toBeUndefined(); + }); +}); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 031859c02f..9057a5af91 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -190,7 +190,9 @@ import { DEFAULT_QWEN_CUSTOM_IGNORE_FILE_NAMES } from '../utils/qwenIgnoreParser import { DEFAULT_TOOL_RESULTS_TOTAL_CHARS_THRESHOLD } from './clearContextDefaults.js'; import { DEFAULT_QWEN_EMBEDDING_MODEL } from './models.js'; import { + registerSessionModel, registerSessionProjectDir, + unregisterSessionModel, unregisterSessionProjectDir, } from '../utils/sessionIdContext.js'; import { Storage } from './storage.js'; @@ -1574,6 +1576,7 @@ const EMPTY_DISABLED_SKILL_NAMES: ReadonlySet = Object.freeze( // processes to claim their own (they start with a fresh module scope). let sessionEnvClaimed = false; let projectDirEnvClaimed = false; +let modelEnvClaimed = false; function resolveSensitiveSpanAttributeMaxLength( value: number | undefined, @@ -2022,6 +2025,10 @@ export class Config { private messageBus?: MessageBus; private readonly memoryManager: MemoryManager; private readonly modelChangeListeners = new Set<(model: string) => void>(); + // True on the Config that claimed the process-global QWEN_CODE_MODEL slot + // (first in this process); gates the global write in publishModelEnv so no + // other instance updates it. Per-session publishing is not gated on it. + private readonly ownsModelEnvSlot: boolean = false; private readonly settingsWatcher?: { stopWatching(): void }; constructor(params: ConfigParameters) { @@ -2348,6 +2355,20 @@ export class Config { onModelChange: this.handleModelChange.bind(this), }); + // Publish the active model id for shell subprocesses. Every Config + // publishes its own session's model — publishModelEnv registers it per + // session, like the project dir above, so daemon-mode subprocesses read + // theirs, not the first session's. The process-global slot is claimed + // first-writer-wins, as QWEN_CODE_SESSION_ID is, so a throwaway Config + // never clobbers the live session's global value. Done here rather than + // alongside the session ID because the value comes from the ModelsConfig + // just constructed. + if (!modelEnvClaimed && process.env) { + modelEnvClaimed = true; + this.ownsModelEnvSlot = true; + } + this.publishModelEnv(); + if ( this.telemetrySettings.enabled && !this.telemetryInitializationDeferred @@ -3567,6 +3588,9 @@ export class Config { // Only assign to instance properties after successful initialization this.contentGeneratorConfig = newContentGeneratorConfig; this.contentGeneratorConfigSources = sources; + // Auth flows call refreshAuth directly — no model-change notification + // fires — and the resolved model can differ from the pre-auth one. + this.publishModelEnv(); // Re-apply the user's reasoning effort that the provider sync above wiped. if (priorReasoningEffort) { @@ -3672,6 +3696,13 @@ export class Config { if (process.env) { process.env['QWEN_CODE_SESSION_ID'] = this.sessionId; } + // Re-key the per-session model registry onto the new session id. Without + // this the entry stays keyed on the outgoing id, so after /clear (or + // /reset, /new, /resume) a non-owner Config's subprocesses resolve the + // model by the new id, miss, and fall back to another session's value. + // Drop the orphaned entry too — shutdown only unregisters the current id. + unregisterSessionModel(previousSessionId); + this.publishModelEnv(); this.sessionData = sessionData; this.pendingRecoveredAgentsNotice = null; this.getOwnActiveTodoReminders().clear(); @@ -3861,12 +3892,33 @@ export class Config { } private notifyModelChangeListeners(): void { + this.publishModelEnv(); const model = this.getModel(); for (const listener of this.modelChangeListeners) { listener(model); } } + // Keeps QWEN_CODE_MODEL on the model that is actually active. A subprocess + // has no other authoritative source: settings files miss /model switches and + // describe the wrong home under QWEN_HOME isolation. Published per session — + // every Config writes its OWN session's model, keyed on sessionId exactly + // like the project dir, so in daemon mode each session's subprocesses read + // theirs, not the first session's (a process-global slot alone would hold + // whichever session booted first). The process-global slot is the + // single-session CLI fallback, gated on the claiming instance so a throwaway + // Config never clobbers the live session's value there. + private publishModelEnv(): void { + if (!process.env) { + return; + } + const model = this.getModel(); + registerSessionModel(this.sessionId, model); + if (this.ownsModelEnvSlot) { + process.env['QWEN_CODE_MODEL'] = model; + } + } + /** * Returns the configured fast model selector when it resolves to an available * model. Bare selectors stay bare and authType-qualified selectors keep their @@ -4739,6 +4791,10 @@ export class Config { unregisterSessionProjectDir(this.sessionId); this.sessionProjectDirRegistered = false; } + // Drop this session's model registry entry. It is registered at + // construction (publishModelEnv), so it is released on every shutdown — + // same daemon-mode leak rationale as the project dir above. + unregisterSessionModel(this.sessionId); if (Object.hasOwn(this, 'goalRuntime')) { this.goalTurnHostUnbind?.(); diff --git a/packages/core/src/utils/sessionIdContext.ts b/packages/core/src/utils/sessionIdContext.ts index c86ef5af86..eafaa93b85 100644 --- a/packages/core/src/utils/sessionIdContext.ts +++ b/packages/core/src/utils/sessionIdContext.ts @@ -61,3 +61,36 @@ export function getSessionProjectDir(sessionId: string): string | undefined { export function unregisterSessionProjectDir(sessionId: string): void { projectDirBySession.delete(sessionId); } + +/** + * Each session's active model id, keyed by its session id. + * + * A subprocess that reports which model ran (the /review compose step) needs + * the model that is ACTIVE in this session, and settings files are not a + * substitute: they miss /model switches and, under QWEN_HOME isolation, + * describe a different home entirely. So the live model is passed down through + * the environment. + * + * Keyed on the session for the same reason the project dir is: a single + * process-global slot holds whichever session booted first, and in daemon mode + * every later session would then hand its subprocesses another session's model + * — a confidently-wrong id, worse than an absent one. + */ +const modelBySession = new Map(); + +export function registerSessionModel(sessionId: string, model: string): void { + if (sessionId && model) modelBySession.set(sessionId, model); +} + +export function getSessionModel(sessionId: string): string | undefined { + return modelBySession.get(sessionId); +} + +/** + * Drop a session's entry when it ends, for the same reason as + * {@link unregisterSessionProjectDir}: the map would otherwise grow one entry + * per session for the life of a daemon process. + */ +export function unregisterSessionModel(sessionId: string): void { + modelBySession.delete(sessionId); +} diff --git a/packages/core/src/utils/shellContextEnv.test.ts b/packages/core/src/utils/shellContextEnv.test.ts index 5da2eea3ed..c521023377 100644 --- a/packages/core/src/utils/shellContextEnv.test.ts +++ b/packages/core/src/utils/shellContextEnv.test.ts @@ -15,6 +15,8 @@ import { sessionIdContext, registerSessionProjectDir, unregisterSessionProjectDir, + registerSessionModel, + unregisterSessionModel, } from './sessionIdContext.js'; import { isShellTracePropagationEnabled, @@ -41,6 +43,9 @@ describe('getShellContextEnvVars', () => { // here also cleans up after the per-session tests below, which assign it and // used to leak the assignment into every later test in the file. let originalProjectDir: string | undefined; + // And QWEN_CODE_MODEL — Config claims it into process.env, so a test run + // started from inside a qwen session inherits it too. + let originalModel: string | undefined; beforeEach(() => { originalSessionId = process.env['QWEN_CODE_SESSION_ID']; @@ -49,6 +54,8 @@ describe('getShellContextEnvVars', () => { delete process.env['QWEN_CODE_CLI']; originalProjectDir = process.env['QWEN_CODE_PROJECT_DIR']; delete process.env['QWEN_CODE_PROJECT_DIR']; + originalModel = process.env['QWEN_CODE_MODEL']; + delete process.env['QWEN_CODE_MODEL']; }); afterEach(() => { @@ -67,6 +74,11 @@ describe('getShellContextEnvVars', () => { } else { delete process.env['QWEN_CODE_PROJECT_DIR']; } + if (originalModel !== undefined) { + process.env['QWEN_CODE_MODEL'] = originalModel; + } else { + delete process.env['QWEN_CODE_MODEL']; + } }); it('passes the running CLI down, so a subprocess does not resolve `qwen` off PATH', () => { @@ -306,6 +318,75 @@ describe('getShellContextEnvVars', () => { }); }); + describe('active model id (QWEN_CODE_MODEL)', () => { + it('passes the active model down from the Config-claimed slot', () => { + // A subprocess that must report which model ran (the /review compose + // step) has no other authoritative source — settings files miss /model + // switches and describe the wrong home under QWEN_HOME isolation. + process.env['QWEN_CODE_MODEL'] = 'qwen3-coder-plus'; + expect(getShellContextEnvVars()['QWEN_CODE_MODEL']).toBe( + 'qwen3-coder-plus', + ); + }); + + it('omits the key when no Config has claimed the slot', () => { + // Same rule as the session ID: nothing in process.env means the + // spawn-site spread has nothing stale to leak, so absence is correct. + expect('QWEN_CODE_MODEL' in getShellContextEnvVars()).toBe(false); + }); + + it('reflects a republished slot after a model switch', () => { + // publishModelEnv in config.ts rewrites the slot on set/switchModel and + // refreshAuth; spawn-time reads must see the CURRENT value, not one + // captured earlier. + process.env['QWEN_CODE_MODEL'] = 'model-before-switch'; + getShellContextEnvVars(); + process.env['QWEN_CODE_MODEL'] = 'model-after-switch'; + expect(getShellContextEnvVars()['QWEN_CODE_MODEL']).toBe( + 'model-after-switch', + ); + }); + }); + + describe('model is per-session, not per-process', () => { + it('hands each session its own active model', () => { + // One daemon process, two sessions, two /model selections. A single + // process-global slot holds whichever booted first — and every later + // session would then stamp a model that never ran the review, the exact + // bug this PR opens with, relocated to the consumer. + registerSessionModel('sess-A', 'model-A'); + registerSessionModel('sess-B', 'model-B'); + process.env['QWEN_CODE_MODEL'] = 'model-A'; // the first to boot + + const a = sessionIdContext.run('sess-A', () => getShellContextEnvVars()); + const b = sessionIdContext.run('sess-B', () => getShellContextEnvVars()); + + expect(a['QWEN_CODE_MODEL']).toBe('model-A'); + expect(b['QWEN_CODE_MODEL']).toBe('model-B'); // NOT A's + }); + + it('drops a session entry on unregister — no daemon leak', () => { + registerSessionModel('sess-X', 'model-X'); + expect( + sessionIdContext.run('sess-X', () => getShellContextEnvVars())[ + 'QWEN_CODE_MODEL' + ], + ).toBe('model-X'); + unregisterSessionModel('sess-X'); + delete process.env['QWEN_CODE_MODEL']; + expect( + sessionIdContext.run('sess-X', () => getShellContextEnvVars())[ + 'QWEN_CODE_MODEL' + ], + ).toBeUndefined(); + }); + + it('falls back to the global slot for the single-session CLI', () => { + process.env['QWEN_CODE_MODEL'] = 'model-only'; + expect(getShellContextEnvVars()['QWEN_CODE_MODEL']).toBe('model-only'); + }); + }); + it('sets empty string for agent/prompt to override inherited env', () => { // Simulates a nested qwen-code process where parent injected these const env = getShellContextEnvVars(); diff --git a/packages/core/src/utils/shellContextEnv.ts b/packages/core/src/utils/shellContextEnv.ts index f39e96e9e4..1d7dc09cad 100644 --- a/packages/core/src/utils/shellContextEnv.ts +++ b/packages/core/src/utils/shellContextEnv.ts @@ -26,7 +26,11 @@ import { accessSync, closeSync, constants, openSync, readSync } from 'node:fs'; import { getCurrentAgentId } from '../agents/runtime/agent-context.js'; import { promptIdContext } from './promptIdContext.js'; -import { sessionIdContext, getSessionProjectDir } from './sessionIdContext.js'; +import { + sessionIdContext, + getSessionProjectDir, + getSessionModel, +} from './sessionIdContext.js'; import { isShellTracePropagationEnabled, getTraceContext, @@ -127,6 +131,25 @@ export function getShellContextEnvVars(): Record { env['QWEN_CODE_CLI'] = isUnusableScriptEntry(cliEntry) ? '' : cliEntry; } + // The model id that is ACTIVE in this session, for subprocesses that report + // which model ran (the /review skill stamps its compose report with one). + // Settings files are not a substitute: they miss /model switches, and under + // QWEN_HOME isolation they describe a different home entirely. Config + // publishes it per session and republishes on every model change + // (publishModelEnv in config.ts). Keyed on this session exactly as the + // project dir above is — a process-global slot would hold whichever session + // booted first, and in daemon mode every later one would hand its + // subprocesses another session's model. Falls back to the global slot for the + // single-session CLI. Omitted (not blanked) when absent, for the session ID's + // reason — no value in this process means the spawn-site spread has nothing + // stale to leak. + const model = + (sessionId ? getSessionModel(sessionId) : undefined) ?? + process.env['QWEN_CODE_MODEL']; + if (model) { + env['QWEN_CODE_MODEL'] = model; + } + // For agent/prompt IDs: explicitly set empty string when no ALS context // exists, so that stale values inherited from a parent qwen-code process // (via process.env spread) are overwritten rather than leaked. From 0232e7381135e283b23ddc9f6b1693bf4720f4b3 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:40:26 +0800 Subject: [PATCH 03/38] feat(verify-pr): add seven techniques from maintainer verification rounds (#8010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(verify-pr): add four techniques from maintainer verification rounds Two hand-written maintainer rounds contained methods the skill could not have produced. Checked each against the current text before adding it; these four had no coverage at all. From #7914 (live daemon A/B on the artifact-recording change): - Run every control on BOTH arms. That round's sharpest finding came from a control whose only job was to validate the BASE probe — "the empty list is a real absence, so have the model call record_artifact and watch an entry appear". Run on head as well, it showed the curated title being silently discarded. The control was not hunting for a bug; running it symmetrically is what found one. - A new writer into a shared store is an ordering change. The PR added write_file as a second writer into the artifact list; the bug was not in the new writer but in the collision, where a pre-existing first-writer-wins merge began discarding record_artifact's curated title and description while still reporting success. Enumerate the other writers, exercise the collision in both orders, and check what the loser is told — and separate the pre-existing cause from the PR's contribution so the author is not blamed for the policy. From #7998 (ink cursor fix, real-terminal A/B): - When the oracle is an instrument, corroborate it with a mechanism that does not use that instrument. The hardware cursor row came from `tmux display-message -p '#{cursor_y}'`, then from a marker printed after the TUI exits — which lands wherever the cursor actually was. Two agreeing instruments turn a measurement into evidence; one tool's report about the system is not the system. - Re-run the generator on committed generated artifacts and diff. That round re-ran `npx patch-package ink` and found byte-different hunk headers, proving the .d.ts hunks were hand-written rather than regenerated as the description claimed. Also strengthens Not covered: proving a limitation is environmental requires an A/A control (boot base and head identically, show both fail the same way), because "seems environmental" and a real regression look identical in a report. Mutation-verified 4/4, each with landing proof. Two initially reported `landed: False` — the assertions match the whitespace-normalised text while the rules wrap across lines in the source, so the replace never fired and the green result proved nothing. Re-run against line-accurate anchors, both kill. 89/89 tests; prettier and eslint clean. * feat(verify-pr): teach the timing-race and scenario-arrival checks Third maintainer round mined for method (#7934 R4). The blocker it found had zero coverage in the skill — `timer`, `wall-clock`, `flake`, `retry`, `duration`, `deterministic` all returned 0, and the one `race` hit was a substring of "trace". - **Timing-triggered assertions have a threshold — measure it, do not sample it.** A new guard (`expect(false).toBe(true)` after an abort loop) turned a vacuous pass into a deterministic failure, because the case triggers its abort from `setTimeout(..., 1000)` while the query's duration is set by CLI startup rather than the server. Natural completion measured 730-2151 ms, so every box on the fast side of 1000 ms fails. The rule says to measure the operation's natural duration with the trigger disabled and compare it to the timer, because a green run only proves this box was slow enough. - **A speed-correlated failure is not flake, and a retry budget does not absorb it.** Random flake becomes a pass under `retry: 2`; this failed 5/5 runs on all three attempts. The two get opposite verdicts, so the kind has to be established before the verdict is written. Stated plainly in the skill: the verify job runs on a shared, loaded runner — the regime where such a test PASSES. Repetition cannot reproduce a fast-machine failure there; only computing the margin can. A rule that said "run it more times" would be useless in this lane. - **The failure one level before vacuity: the scenario never reached the code under test.** The vacuity check asks whether an assertion can fail; this asks whether the code ever ran. Four abort cases fired during CLI process startup, so the fake server saw zero requests and a suite named for mid-stream aborts never streamed — with every assertion green. Instrument the seam and assert the count is non-zero. Mutation-verified 5/5, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. Skill is 472 lines, up from 392. * feat(verify-pr): six more techniques, from three maintainer rounds Mined #7836 R2, #7885 and #7899 for method. Checked each candidate against the current text first; six had zero coverage, the rest were already there (harness teeth-checks, pin dereferencing, boundary probing, and the follow-up round's "re-measure, never diff"). The one that corrects the skill's own core method, from #7836: - **Before calling a survivor vacuous, escalate to a finer mutation.** A whole-file revert is blunt enough to remove the PRECONDITION a test depends on, so a good test goes green because its scenario no longer occurs — from the outside, identical to a test that asserts nothing. A `finally`-cleanup test survived reverting four production files and died to deleting one line. Coarse survived + fine killed ⇒ the test is fine and the mutation was wrong. A false "your test is vacuous" costs the author more than a missed survivor does. From #7836, the root cause shared by both of its blockers: - **When the same predicate is checked in two places, verify they see the same state.** A guard duplicated across a process boundary is two implementations of one question that diverge when their INPUTS differ. One settings key made a route ask sessionExistsInAnyState() with an unpinned runtime dir while the child asked with a pinned one, turning a clean 409 into a 500 plus a process.exit(1) that killed every session on the channel. Includes the temporal half: lazily-created backing files leave a window where a just-created entity is invisible to any on-disk existence check. - **Measure the blast radius on bystanders.** The caller's own error code understates a shared-state failure; the number that matters is an unrelated session going 200 -> 404 and a workspace list going 2 -> 0. From #7885, which took a performance claim apart: - **Isolate the slice the mechanism can actually affect.** A speedup claim is two claims: the mechanism works, and the thing it speeds up matters. `--ignore-scripts` isolated what an npm download cache can touch — 36s of a 226s install — so the ceiling was 20s and the real saving 15%, not the claimed 75%. Then check it against the whole job: 33s off 14m37s. - **A mechanism that persists something has a cost — price it.** 219 MB per lockfile hash into a pool at 9.98 GB of a 10 GB cap, with 39 distinct lockfile states in 30 days: at the cap every entry evicts by LRU, including entries other jobs need and its own. - **Test the scarier consequences and report which do NOT hold.** The write-path finding was real; code injection was disproved (npm integrity-checks a tampered cache and refetches) and privilege escalation was disproved (chown -R does not follow symlinks). A finding that names what it is not is harder to wave away. - **Verify third-party actions from their own manifest.** The PR said the cache dir was discarded after the job; `action.yml` declares `post: dist/save/index.js` with `post-if: success()`, which uploads it as root with credentials intact — the opposite of the claim, and the whole finding. From #7899: - **To exercise real production data safely, interpose a refusing proxy on the write path.** Wrap the client so every mutating call hard- fails, then run the shipped script verbatim: real counts, mechanical guarantee of no side effects. Mutation-verified 9/9, each with landing proof against line-accurate anchors. 89/89 tests; prettier and eslint clean. The skill is now 546 lines, up from 392 — the growth is deliberate and called out in the PR body's risk section. * feat(verify-pr): decomposed fixes, contextual limits, destination counts From #7862 R4. Three additions, and a deliberate stop. - **When one fix bundles two changes, build the intermediate variants.** An A/B against base proves the pair works and says nothing about what each half does. That round compiled a third build with only the ordering change reverted, and the three-row table showed the halves do different jobs: moving `initialized = true` after the fallible work converts a 2,999-and-climbing backlog flood into a fail-safe retry, while `reduce()` restores liveness. Either alone leaves a channel that floods or wedges — a conclusion the two-cell A/B cannot reach. - **A limit measured in isolation does not transfer to the real call site.** The same `Math.max` spread threw between 110k and 130k elements inside a deep async stack, well below a standalone micro-benchmark. Bisect thresholds through the real code path and quote the harness; a limit taken from documentation or a toy loop is a guess about the system under test. - **Count at the destination, not at the component boundary.** The mirror of the scenario-arrival rule added earlier: envelopes the adapter emitted and prompts that reached the agent are different numbers, and every gate lives between them. A count taken at the seam can be right while the feature is silently dropped downstream. Declined from the same report, to protect prompt budget rather than because they are wrong: siblings-as-convention-oracle (the lockfile version table across five channels), degenerate fixtures that cannot distinguish two sort keys, and naming the condition under which a cosmetic finding becomes real. Each is a good technique; none is worth another rule competing for attention with the ones already here. The skill is now 578 lines, up from 392 on main (+47%) across this branch. That growth is the main risk on this PR and further additions should wait until a live round shows the current set changes behaviour. Mutation-verified 3/3 with landing proof. One mutation initially SURVIVED — it deleted text sitting AFTER the asserted phrase, so the assertion still matched and the green proved nothing; re-run against the phrase itself, it kills. 89/89 tests; prettier and eslint clean. * test(scripts): drop stale technique count from verify test name (#8010) * fix(triage): correct verify-skill worked examples and verdict path (#8010) Address review feedback on the verification-techniques skill: - Make the npm-cache worked example's numbers close: separate the 20 s download-slice ceiling (36 s to 16 s) from the 15% end-to-end saving (226 s to 193 s) rather than conflating them. - Stop overstating the tarball experiment: one tarball was poisoned, and the 2262-entry integrity coverage is a separate static fact. - Give the speed-correlated-failure rule a contract-legal verdict path by encoding the margin as a scripted assertion, and mark the load/idle sweep as the local-mode variant. - Fix the one bullet that broke its 2-space list continuation. - Pin the new contract-encoding clause in the workflow test. --------- Co-authored-by: wenshao Co-authored-by: qwen-code-dev-bot --- .qwen/skills/verify-pr/SKILL.md | 198 ++++++++++++++++++++- scripts/tests/qwen-triage-workflow.test.js | 96 ++++++++++ 2 files changed, 293 insertions(+), 1 deletion(-) diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 7cdb03a962..502fe68e16 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -168,11 +168,105 @@ differs only by the change under test; the verdict is the pair of counts. change — and every residual delta gets accounted for ("the closure is 1.3 KB larger: that is the new guards themselves"). An unexplained residue is a finding, not noise. +- **Isolate the slice the mechanism can actually affect, then show what + fraction of the total it is.** A speedup claim is really two claims: the + mechanism works, and the thing it speeds up matters. Add an arm that + strips everything the mechanism cannot touch — measured example: an npm + download cache was claimed to cut `npm ci` by ~75%; running with + `--ignore-scripts` isolated pure download+extract at 36 s cold of a 226 s + install, and warming just that slice removed 20 s of it (36 s → 16 s) — + the cache's ceiling. End-to-end the install went 226 s → 193 s, a 15% + saving rather than the claimed 75%, the rest of the cost being the repo's + own `postinstall`/`tsc`/bundler work. Then check that saving against the + **whole job budget**: 33 s off a 14 m 37 s job is not the headline the + description claimed. A perf PR whose mechanism works but targets 15% of the + cost is a finding about the premise, not the code. +- **A mechanism that persists something has a cost, not only a benefit — + price it.** Caches, artifacts and generated entries consume a shared, + bounded resource. Measure what it adds (219 MB per lockfile hash), what + the pool holds (9.98 GB of a 10 GB cap), and the churn rate (39 distinct + lockfile states in 30 days) — because at the cap every new entry evicts + by LRU, including entries other jobs depend on, and possibly its own, + degrading the very hit rate the saving assumes. +- **Test the scarier consequences and report which ones do NOT hold.** Having + found a real problem, the temptation is to report the worst reading of it. + Bound it instead: in the cache case the write-path finding was real + (a post-step uploads the directory that untrusted code can write), but + code injection was **disproved** — tampering with a cached tarball made + npm reject it against the lockfile hash and refetch under the flag CI + uses, and all 2262 lockfile entries carry an `integrity` hash, so nothing + installs unhashed — and privilege escalation was **disproved** — + `chown -R` does not follow symlinks. What survived was content and quota + abuse. A finding that names what it is _not_ is far harder to wave away + than one that implies everything. - When the PR adds a defensive guard or shape check, its unit tests usually mock the reject path — so verify the **accept path against the real artifacts it will see in production** (the shipped chunks, the real module namespaces, the actual wire payloads). A guard that is too strict fails in production on a path no mocked test covers. +- **When one fix bundles two changes, build the intermediate variants.** An + A/B against base proves the pair works; it says nothing about what each + half does or whether both are needed. Compile a third build with one half + reverted and put all three in one table. Worked example, on a first-poll + drain fix that both replaced `Math.max(...spread)` with `reduce()` and + moved `initialized = true` after the fallible work: + + | build | RangeError | prompts dispatched | cursor saved | + | ----------------------------- | ---------- | ---------------------- | ------------ | + | base (`Math.max`, flag first) | yes | **2,999 and climbing** | none | + | flag moved only | yes | 0 | none | + | both (head) | no | 0 | saved | + + The ordering change is what converts a backlog flood into a fail-safe + retry; `reduce()` is what restores liveness. Either alone leaves a channel + that floods or wedges — a conclusion the two-cell A/B cannot reach. + +- **A limit measured in isolation does not transfer to the real call site.** + Argument-count caps, stack depth, buffer sizes and timeouts all move with + context: the same `Math.max` spread threw between 110k and 130k elements + inside a deep async stack, well below what a standalone micro-benchmark + suggests. Bisect the threshold **through the real code path**, and quote + the harness you bisected with — a limit quoted from documentation or from + a toy loop is a guess about the system under test. +- **When the same predicate is checked in two places, verify they see the + same state.** A guard duplicated across a process boundary — a route and + the child it spawns, a parent and a worker, a cache and its source — is + two implementations of one question, and they diverge whenever their + _inputs_ differ rather than their logic. Find the configuration that makes + them disagree and drive it: one measured case had the route ask + `sessionExistsInAnyState()` with an unpinned runtime dir while the child + asked it with a pinned one, so a single settings key flipped a clean 409 + into a 500 plus a `process.exit(1)` that killed every session on the + channel. Two related questions expose most of this class: does one side + observe state the other cannot, and **is the state observable yet at all** + — lazily-created backing files (`ensureConversationFile()` writes nothing + until the first prompt) leave a window in which a just-created entity is + invisible to any existence check that looks on disk. +- **Measure the blast radius on bystanders, not just on the caller.** When a + failure path can take down shared infrastructure, the interesting number + is what happened to everything else: an unrelated session going + `200 → 404`, a workspace list going `2 → 0`. Assert on a third party you + set up beforehand — the caller's own error code understates a shared-state + failure every time. +- **Run every control on BOTH arms, not just the arm that needs it.** A + control usually exists to validate the probe on one side — "the empty list + on base is a real absence, so let the model call the API explicitly and + watch an entry appear". Run that same step on head anyway. The single + highest-value finding of a real round came from exactly this: the + base-side positive control, executed identically on head, showed the + curated title being silently discarded. The control was not looking for a + bug; running it symmetrically is what found one. +- **A new writer into a shared store is an ordering change, not just an + addition.** When the PR makes some new path write into a store that + already has writers — an artifact list, a cache, a registry, a settings + merge — the bug is rarely in the new writer. It is in the _collision_: + the store's existing merge policy (first-writer-wins, last-writer-wins, + shallow merge) was chosen when only one writer existed, and the PR + changes who arrives first. Enumerate the other writers, exercise the + collision **in both orders**, and check what the loser is told — a silent + no-op that reports success is a finding even when the merge policy itself + is pre-existing and correct. Name the pre-existing cause and the PR's + contribution separately, so the author is not blamed for the policy. ### Vacuity check on new/changed tests @@ -196,6 +290,67 @@ If deleting the new guard leaves its own new test green, that test is pinned by something else (an earlier early-return, a different branch) and asserts nothing about the change. Name what actually pins it. +**And the failure one level earlier: the scenario never reached the code +under test.** A vacuity check asks whether the assertion can fail; this asks +whether the code ever ran. Instrument the seam and count — requests the fake +peer actually received, invocations of the function under test, frames +rendered — then assert that count is non-zero. Worked example: four abort +cases in an E2E suite fired their aborts during **CLI process startup**, so +`modelRequestsSeenByFakeServer` was `0` and `messages` empty; a suite named +for aborting mid-stream never streamed. Every assertion passed. Fixing the +race also restored the coverage the tests were named for +(`modelRequestsInFlightAtAbort=1`), which is the tell that the original +green meant nothing. + +The mirror of it: **count at the destination, not at the component +boundary.** What a component emits and what survives to the end of the +pipeline are different numbers, and the gates live in between — "envelopes +the adapter emitted" versus "prompts that actually reached the agent" differ +by every filter on the path. Assert the number a user would experience; a +count taken at the seam can be right while the feature is silently dropped +downstream. + +**Timing-triggered assertions have a threshold — measure it, do not sample +it.** When an assertion's outcome depends on a wall-clock timer racing an +operation whose duration you do not control (`setTimeout(() => abort(), 1000)` +against a query bounded by process startup, not by the server), the test +encodes a margin nobody has measured. Measure the operation's natural +duration directly — run the scenario with the trigger disabled — and compare +it to the timer. If the distribution crosses the threshold, the test fails on +every machine on the fast side of it. A green run proves only that _this_ box +was slow enough. + +This matters most because **a speed-correlated failure is not flake, and a +retry budget does not absorb it.** Ordinary flake is random, so `retry: 2` +converts it to a pass; a failure driven by machine speed is fully correlated +across attempts — measured on a real PR as 5/5 runs failing all three +attempts. Before writing off an intermittent failure as flake, establish +which kind it is: in local mode, repeat under load and idle, and report the +natural durations alongside the outcomes. The two get opposite verdicts — +flake is a note, a speed-correlated failure is blocking. Make that blocking +verdict expressible in the contract by encoding the margin as a scripted +assertion: measure the natural duration N times and assert it stays on the +side the test needs (here `min(duration) > timer`, because the test fails on +the fast side). A distribution that crosses the threshold then lands in +`fail`, and the existing rule (nonzero `fail` ⇒ not `merge-ready`) carries +the verdict without a special case. + +Note the CI verify job runs on a **shared, loaded** runner, which is the +regime where such a test passes. You cannot reproduce a fast-machine failure +here by repetition; you can only compute the margin and say what it implies. + +**Before calling a survivor vacuous, escalate to a finer mutation.** A +whole-file revert is a blunt instrument: it can remove the _precondition_ a +test depends on, so a perfectly good test goes green because its scenario no +longer occurs — indistinguishable, from the outside, from a test that asserts +nothing. Worked example: a `finally`-cleanup test survived reverting all four +production files, which read as vacuity; deleting the single line +(`inFlightSessionIds.delete(...)`) killed it cleanly. It was doing exactly the +job it was added for. Coarse mutation survived, fine mutation killed ⇒ the +test is fine and the mutation was wrong. Report the finer result, not the +coarse one — a false "your test is vacuous" costs the author more than a +missed survivor. + And do not generalize from one dead guard to its siblings. A clause that is unreachable in one call path may be the only thing protecting another — check each on its own evidence and report the contrast, so "this guard is @@ -235,6 +390,24 @@ inconclusive. - Assert **both sides of the wire** where a protocol is involved: what the peer actually received (method, path, headers, exact body, request count) and what the caller observed — plus that stderr stayed clean. +- **When the oracle is an instrument, corroborate it with a mechanism that + does not use that instrument.** A tool's _report_ about the system is not + the system: a cursor query, a profiler number, a coverage percentage can + each be wrong in ways your assertion cannot see. Find a second effect of + the same physical fact whose failure mode is independent. Worked example: + the hardware cursor row was read with + `tmux display-message -p '#{cursor_y}'`, then confirmed by letting the TUI + exit and printing a marker — anything printed after exit lands wherever the + cursor actually was, so the marker's row corroborates the query without + trusting it. Two agreeing instruments turn a measurement into evidence. +- **To exercise real production data safely, interpose a refusing proxy on + the write path.** Read-only claims about a live system are best tested + against that system, and the objection is always side effects. Remove it + mechanically: wrap the client so every mutating call hard-fails, then run + the shipped script verbatim. A workflow verified this way returned real + counts (1085 unminimized comments, `rateLimit.cost = 2`) with a guarantee + no write could occur — stronger evidence than a fixture and safer than a + careful hand. Say in the report which wrapper enforced it. - Every assertion is a scripted comparison that can fail. Keep harnesses as `.mjs` files inside the artifact dir so a maintainer can rerun them. @@ -278,6 +451,23 @@ since the merge-base, say so and re-measure there. whether it is a coverage gap or a real defect, and prove which independently rather than by reading the code. Confirm the unmutated control is green, or the kills mean nothing. +- **Third-party actions and dependencies**: verify what they do from **their + own manifest**, never from the PR's description of them. A change asserted + that a cache directory was "ephemeral, discarded after the job"; reading + `action.yml` showed `post: 'dist/save/index.js'` with `post-if: success()` + — a post-step uploads that directory as root with the Actions credentials + intact, which is the opposite of the claim and the whole finding. Also + confirm a pinned SHA dereferences to the tag the PR says it does. +- **Committed generated artifacts** (a `patch-package` patch, a lockfile, a + generated schema or `.d.ts`, a checked-in snapshot): the description + usually says it was regenerated with the tool. **Re-run the generator and + diff its output against what was committed.** A byte-difference proves the + file was hand-edited rather than generated, which is a maintenance hazard + even when the content is functionally identical and applies cleanly — the + next regeneration will produce a confusing diff. Worked example: re-running + `npx patch-package ink` produced hunk headers carrying the function-context + suffix that the committed `.d.ts` hunks lacked. Report it at the severity + it deserves (usually a nit), and say plainly that the content matched. - **Multi-commit PRs**: verify each commit's claim separately when the commits are reachable. In CI they usually are **not** — the checkout is depth 2, giving only the merge commit, the base tip (`HEAD^1`), and the PR @@ -369,7 +559,13 @@ central claim from being tested — say why. collapsed minimal suggested fix that preserves the original commit's intent. 6. **Not covered** — every claim, surface, or gate you skipped. A silent cap - reads as "covered everything"; never allow that. + reads as "covered everything"; never allow that. When something failed to + run rather than being skipped by choice, **prove it was environmental + before saying so**: boot the identical thing on base and on head and show + both fail the same way (an A/A control). "The dev harness renders blank — + base and head both blank, so this is my sandbox, not a regression" is a + claim a reader can check; "seems environmental" is not, and the two look + identical in a report. 7. **Methodology** — one paragraph: environment, how each harness drove the code, where the raw logs live. diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 9bc63659eb..2c80749678 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -2089,6 +2089,102 @@ describe('qwen-triage verify hardening round 2', () => { } }); + // Verification techniques lifted from maintainer-written verification rounds that + // the skill could not previously have produced. Each is pinned to the + // failure it exists for, because a rule stated without its failure reads + // as advice and gets skipped. + it('carries the maintainer-round verification techniques', () => { + const flat = verifySkill.replace(/\s+/g, ' '); + + // #7914 §4: the PR added write_file as a second writer into the shared + // artifact store, and the store's pre-existing first-writer-wins merge + // then silently discarded record_artifact's curated title. Nothing in + // the old skill pointed at collisions between writers. + expect(flat).toContain('new writer into a shared store'); + expect(flat).toContain('in both orders'); + expect(flat).toContain('what the loser is told'); + + // ...and that finding surfaced from a control that existed to validate + // the BASE probe, run identically on head. + expect(flat).toContain('Run every control on BOTH arms'); + + // #7998: the hardware cursor was read with a tmux query, then + // corroborated by a post-exit marker — a second effect of the same fact + // whose failure mode does not involve the query. + expect(flat).toContain('corroborate it with a mechanism that does not use'); + + // #7998 nit: re-running patch-package produced a byte-different file, + // proving the committed hunks were hand-written. + expect(flat).toContain('Re-run the generator and diff'); + + // #7998 "what I did not verify": a blank harness was proved + // environmental by booting base and head identically. + expect(flat).toContain('A/A control'); + + // #7934 R4 §1: a new guard turned a vacuous pass into a failure that is + // deterministic on a fast machine. Sampling cannot find it from a + // loaded CI box — only measuring the margin can, so the rule must say + // measure and must say why repetition is the wrong instrument here. + expect(flat).toContain('measure it, do not sample it'); + expect(flat).toContain('speed-correlated failure is not flake'); + expect(flat).toContain('You cannot reproduce a fast-machine failure'); + // The blocking verdict must be expressible in the contract, not just + // asserted in prose: encode the margin as a scripted assertion so a + // crossing distribution lands in `fail`. + expect(flat).toContain('encoding the margin as a scripted assertion'); + + // #7934 R4 §2: the abort cases fired during CLI startup, so the fake + // server saw zero requests — a suite named for mid-stream aborts never + // streamed, and every assertion passed. + expect(flat).toContain('the scenario never reached the code under test'); + expect(flat).toContain('assert that count is non-zero'); + + // #7836: both blockers had one root cause — a route and the child it + // spawns asked the same question against different state (pinned vs + // unpinned runtime dir), and a lazily-created backing file left a + // window where a just-created session was invisible to any on-disk + // existence check. + expect(flat).toContain('the same predicate is checked in two places'); + expect(flat).toContain('is the state observable yet at all'); + expect(flat).toContain('blast radius on bystanders'); + + // #7836: a whole-file revert removed a test's precondition, so a good + // test looked vacuous. A false "your test is vacuous" is worse than a + // missed survivor, so the rule must demand the finer mutation first. + expect(flat).toContain('escalate to a finer mutation'); + + // #7885: an npm cache claimed ~75% off npm ci; isolating the slice a + // download cache can touch (--ignore-scripts) showed 36s of 226s, so + // the real saving was 15% — and 33s off a 14m37s job at that. + expect(flat).toContain('Isolate the slice the mechanism can actually'); + expect(flat).toContain('has a cost, not only a benefit'); + // ...and the severity of the finding it did have was bounded by + // disproving the scarier readings. + expect(flat).toContain('report which ones do NOT hold'); + + // #7885: the PR said the cache dir was discarded after the job; the + // action's own manifest declares a post-step that uploads it as root. + expect(flat).toContain('their own manifest'); + + // #7899: the shipped script was run verbatim against the live repo with + // every mutating call hard-failing — real data, no possible side effect. + expect(flat).toContain('interpose a refusing proxy on the write path'); + + // #7862 R4: the fix bundled reduce() with an ordering change. A third + // build with only the ordering change showed each half does a different + // job — either alone leaves a channel that floods or wedges, which the + // two-cell A/B cannot reach. + expect(flat).toContain('build the intermediate variants'); + // ...and the same round bisected the RangeError threshold through the + // real async stack, where it fired far below a micro-benchmark's number. + expect(flat).toContain( + 'A limit measured in isolation does not transfer to the real call site', + ); + // ...counting emitted envelopes instead of delivered prompts would have + // hidden every gate on the path. + expect(flat).toContain('count at the destination, not at the component'); + }); + // PR #7836's report said "Verdict: merge-ready — the 7 failures are all // expected A/B base-cell failures proving the tests are load-bearing" // while assertions.json said fail:7 — so the publisher's trust rule From 36fe53d42132f34266bdc728c3d5f4dd3f95e864 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:40:34 +0800 Subject: [PATCH 04/38] feat(triage): raise the /verify agent budget from 25m to 120m (#8014) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(triage): raise the /verify agent budget from 25m to 120m Maintainer decision: give /verify enough time to do what a maintainer's local round does, and add machines if the pool gets tight. Three values encode this one budget and each has its own silent failure when edited apart, so all three move together: - agent kill 25m -> 120m (the graceful budget; ships a partial report on expiry) - watchdog secs 1500 -> 7200 (distinguishes that kill from an OOM; left low, a late 137 is mislabelled `timeout` and publishes "partial evidence" for a crash) - job timeout 60m -> 150m (guards infra hangs only; it must clear agent + install/build + overhead or it kills the container mid-run and the ship-what-ran path never executes) 150 rather than 130: measured install+build is ~6m (run 30284341325, npm ci 3m00 + build 2m40), budgeted at 15m for a cold cache, plus ~5m of tools/checkout/pin/upload/cleanup — worst case ~140m, leaving 10m of headroom. The skill's advertised budget moves too, and this is the part that actually changes behaviour: it read "≈ 20 minutes (hard 25-minute kill)", and an agent obeying that would have self-limited no matter what the workflow allowed. It now reads 110/120 and says what the extra time is FOR — bisecting a threshold through the real code path, compiling an intermediate build to separate the halves of a bundled fix, adjudicating mutation survivors, driving a real daemon end to end. It also says plainly that spending it on breadth is the one way to waste it; the rule that one proven load-bearing claim beats ten unverified observations does not relax because the clock did. Pinned by a new test that asserts the RELATIONSHIPS rather than the numbers: watchdog == agent budget in seconds, job >= agent + 20m, and the skill's advertised hard kill == the workflow's, with the soft budget strictly below it. Mutation-verified 4/4 — leaving the watchdog at 1500, the job at 60, the skill at 25m, or raising the soft budget above the hard kill each turn it red. Cost, stated so it is a decision and not a surprise: a verify run now holds one ECS slot for up to 2.5h instead of 1h. Concurrency is unchanged (per-PR group, cancel-in-progress false), so this reduces how many distinct PRs can verify at once, not how many runs a PR can queue. 90/90 tests; prettier, eslint and actionlint clean. * fix(ci): pin soft-budget lower bound in verify consistency test (#8014) * fix(ci): harden verify budget guard and clarify budget is a ceiling (#8014) * fix(ci): measure elapsed before artifact copy and tighten budget guard (#8014) * test(ci): pin elapsed-time chain in verify budget drift guard (#8014) * refactor(ci): derive verify watchdog threshold from the agent budget (#8014) The watchdog threshold that tells a graceful timeout (137 at the budget) from an OOM kill (137 before it) was a bare `7200` coupled to the `120m` agent timeout only by a comment. Define the budget once as AGENT_BUDGET_M and derive both the timeout (`${AGENT_BUDGET_M}m`) and the threshold (`$((AGENT_BUDGET_M * 60))`) from it, so editing one cannot silently desync the other. The consistency test now asserts that derivation rather than re-extracting two independent literals. Co-authored-by: qwen-code-ci-bot --------- Co-authored-by: wenshao Co-authored-by: qwen-code-dev-bot Co-authored-by: Qwen Code Autofix Co-authored-by: qwen-code-ci-bot --- .github/workflows/qwen-triage.yml | 36 +++++++++---- .qwen/skills/verify-pr/SKILL.md | 12 ++++- scripts/tests/qwen-triage-workflow.test.js | 62 ++++++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) diff --git a/.github/workflows/qwen-triage.yml b/.github/workflows/qwen-triage.yml index 8c6a677d9f..6fc7d257b6 100644 --- a/.github/workflows/qwen-triage.yml +++ b/.github/workflows/qwen-triage.yml @@ -1850,12 +1850,24 @@ jobs: format('{0}-verify-run-{1}', github.workflow, github.run_id) }} cancel-in-progress: false - # 60, not 45: the agent's own 25m timeout is the graceful budget (it - # ships a partial report); the job limit only guards infra hangs. At 45 - # a slow npm ci + build (15m+ on this monorepo) could let the JOB - # timeout kill the container mid-run, bypassing the agent's + # The agent's own 120m timeout is the GRACEFUL budget — it ships a + # partial report on expiry. This job limit only guards infra hangs, so it + # must stay comfortably ABOVE `agent budget + everything before it`, or + # the JOB timeout kills the container mid-run and bypasses the # ship-what-ran path entirely. - timeout-minutes: 60 + # + # agent budget 120m + # install + build ~6m measured (run 30284341325: npm ci + # 3m00 + build 2m40), budget 15m for a + # cold cache or a heavier dependency tree + # resolver/tools, checkout, + # pin, upload, cleanup ~5m + # ------------------------------------ + # worst case ~140m ⇒ 150 leaves 10m of headroom. + # + # Cost of the raise, stated so it is a decision and not a surprise: a + # verify run now occupies one ECS slot for up to 2.5h instead of 1h. + timeout-minutes: 150 runs-on: ['self-hosted', 'linux', 'x64', 'ecs-qwen'] # The job checks out and executes PR code. Run the steps in a container so # package scripts/builds cannot persist changes in the self-hosted runner's @@ -2962,12 +2974,15 @@ jobs: QWEN_ENV+=("OPENAI_MODEL=$OPENAI_MODEL") fi + # The agent's graceful kill budget in minutes. The watchdog + # threshold below derives from it, so the two cannot drift apart. + AGENT_BUDGET_M=120 # Elapsed time of the WATCHDOG CHILD only: $SECONDS includes proxy # and network setup, so an OOM kill late in the step could cross a # global threshold and be mislabeled a configured timeout. AGENT_START=$SECONDS set +e - timeout --kill-after=10s 25m runuser -u node -- env -i "${QWEN_ENV[@]}" "${QWEN_CMD[@]}" \ + timeout --kill-after=10s "${AGENT_BUDGET_M}m" runuser -u node -- env -i "${QWEN_ENV[@]}" "${QWEN_CMD[@]}" \ --prompt "/verify-pr ${PR_NUMBER} --repo ${REPOSITORY}" \ --output-format stream-json \ | tee "$RUNNER_TEMP/verify-results/output.jsonl" @@ -2983,6 +2998,7 @@ jobs: TEE_STATUS=${PIPE_STATUS[1]:-0} EXIT_CODE=$AGENT_STATUS set -e + AGENT_ELAPSED=$((SECONDS - AGENT_START)) # Collect the skill's artifacts (report.md, verdict.txt, # assertions.json, harness scripts, raw logs) into the upload dir. @@ -2994,10 +3010,12 @@ jobs: # 137 is ambiguous: the watchdog escalating past --kill-after looks # identical to an OOM kill. Use the elapsed budget to tell them - # apart instead of labelling every 137 a crash. - AGENT_ELAPSED=$((SECONDS - AGENT_START)) + # apart instead of labelling every 137 a crash. The threshold is + # derived from AGENT_BUDGET_M so it cannot drift from the budget: + # a 137 at or after the full budget is the watchdog, an earlier + # 137 is an OOM. WATCHDOG_FIRED=false - if [ "$EXIT_CODE" -eq 137 ] && [ "$AGENT_ELAPSED" -ge 1500 ]; then + if [ "$EXIT_CODE" -eq 137 ] && [ "$AGENT_ELAPSED" -ge $((AGENT_BUDGET_M * 60)) ]; then WATCHDOG_FIRED=true fi if [ "$EXIT_CODE" -eq 124 ] || [ "$WATCHDOG_FIRED" = true ]; then diff --git a/.qwen/skills/verify-pr/SKILL.md b/.qwen/skills/verify-pr/SKILL.md index 502fe68e16..19b0abc8cf 100644 --- a/.qwen/skills/verify-pr/SKILL.md +++ b/.qwen/skills/verify-pr/SKILL.md @@ -30,9 +30,19 @@ The workflow (`qwen-triage.yml` `verify` job) guarantees: - **You may execute PR code freely.** This job is the designated sandbox (container, no credentials) — the opposite of the `/triage` rules. Builds, node processes, loopback servers, and scratch `git worktree`s are all fine. -- **Time budget ≈ 20 minutes** of agent time (hard 25-minute kill; install +- **Time budget ≈ 110 minutes** of agent time (hard 120-minute kill; install and build happen before your clock starts and do not eat it). Pick scope first (below); when time runs out, ship the report with what ran. + This budget is large on purpose. It is enough to bisect a threshold + through the real code path, compile an intermediate build to separate the + halves of a bundled fix, run a mutation matrix and adjudicate its + survivors, or drive a real daemon end to end — the things a maintainer's + local round does and a 20-minute round had to skip. Spending it on more + breadth instead is the one way to waste it: the rule that one proven + load-bearing claim beats ten unverified observations does not relax + because the clock did. It is a ceiling, not a target: once the central + claim is proven and the report is written, ship. There is no credit for + using the clock. - If the directory holding `$QWEN_VERIFY_CONTEXT` contains `previous-report.md`, this is a **follow-up round**. The workflow snapshots the newest _substantive_ report — never a "running"/cancelled/infra diff --git a/scripts/tests/qwen-triage-workflow.test.js b/scripts/tests/qwen-triage-workflow.test.js index 2c80749678..1df2cca45c 100644 --- a/scripts/tests/qwen-triage-workflow.test.js +++ b/scripts/tests/qwen-triage-workflow.test.js @@ -2996,6 +2996,68 @@ describe('qwen-triage verify round-3 hardening', () => { expect(group).toContain("vars.MAINTAINER_ECS_RUNNER_DISABLED != 'true'"); }); + // One budget drives the agent's graceful kill and the watchdog threshold + // that distinguishes that kill from an OOM — both derive from a single + // AGENT_BUDGET_M, so they cannot drift apart. The job limit and the + // skill's advertised budget are still separate literals with their own + // quiet failures, so pin their relationship to the budget, not the number. + it('keeps the verify budget, watchdog and job limit consistent', () => { + const verifyJob = job('verify'); + const runStep = stepIn('verify', 'Run verification agent'); + + const agentMinutes = Number(runStep.match(/AGENT_BUDGET_M=(\d+)/)?.[1]); + const jobMinutes = Number( + verifyJob.match(/^ {4}timeout-minutes: (\d+)/m)?.[1], + ); + expect(agentMinutes).toBeGreaterThan(0); + expect(jobMinutes).toBeGreaterThan(0); + + // The graceful kill and the watchdog threshold must both derive from + // AGENT_BUDGET_M rather than carry their own literals — that derivation + // is what makes the coupling correct by construction. A hardcoded + // `120m` or `7200` would reintroduce the drift this test exists to + // catch: set the threshold below the budget and a late OOM (137) reads + // as `timeout`, publishing "partial evidence" for a crash; set it above + // and a real timeout reads as a crash. + expect(runStep).toMatch(/timeout --kill-after=10s "\$\{AGENT_BUDGET_M\}m"/); + expect(runStep).toMatch( + /"\$AGENT_ELAPSED" -ge \$\(\(AGENT_BUDGET_M \* 60\)\)/, + ); + + // That comparison is only meaningful if the elapsed-time chain exists: + // drop the baseline and AGENT_ELAPSED is total shell uptime, so a late + // OOM reads as `timeout`; drop the assignment and it is empty, so the + // watchdog never fires and a real timeout reads as a crash. Pin both + // lines so deleting either fails here instead of silently in CI. + expect(runStep).toMatch(/AGENT_START=\$SECONDS/); + expect(runStep).toMatch(/AGENT_ELAPSED=\$\(\(SECONDS - AGENT_START\)\)/); + + // A step-level `timeout-minutes` on the run step would cap the agent + // below the budget while every relationship above stays green — the + // job limit must be the only cap. + expect(runStep).not.toMatch(/^\s+timeout-minutes:/m); + + // The job limit only guards infra hangs, so it must clear the agent + // budget plus install/build plus fixed overhead — otherwise the job + // is killed mid-run and the agent never ships its partial report. + // 20 minutes is the documented allowance (≈15m install/build + ≈5m + // tools, checkout, pin, upload, cleanup). + expect(jobMinutes).toBeGreaterThanOrEqual(agentMinutes + 20); + + // And the skill has to advertise the same budget, or the agent + // self-limits to the old number and the raise does nothing. + const advertised = Number(verifySkill.match(/hard (\d+)-minute kill/)?.[1]); + expect(advertised).toBe(agentMinutes); + const soft = Number(verifySkill.match(/Time budget ≈ (\d+) minutes/)?.[1]); + expect(soft).toBeLessThan(agentMinutes); + expect(soft).toBeLessThanOrEqual(agentMinutes - 10); + // A proportional lower bound catches the most common drift — the hard + // kill is raised but the soft budget is forgotten, silently wasting CI + // time — without freezing the reserve at a fixed minute count for every + // future budget. + expect(soft).toBeGreaterThanOrEqual(Math.floor(agentMinutes * 0.8)); + }); + // Cleanups must never descend through a PR-writable parent, and an // outward-resolving hooks entry must be removed rather than reported. it('survives symlink escapes in the workspace cleanup', () => { From 25f514767755fab25a4e94539cb1ace3bb0ac4e6 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:40:55 +0800 Subject: [PATCH 05/38] feat(autofix): post a takeover milestone digest every tenth pushed round (#8046) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(autofix): post a takeover milestone digest every tenth pushed round The takeover round cap (100) bounds runaway but carries no signal about when a human should step in: #7469 ground to round 12 over seven days of takeover with fifteen pushes, and the only place that trajectory was visible was the Actions logs. Every 10th pushed round under takeover, the report step now posts a window-scoped census on the PR itself — pushed fixes, no-change reviews, timeouts, rejected attempts, base updates — plus the three options (keep going / split or reduce / release takeover). The digest is a separate comment with its own autofix-milestone marker and no autofix-eval marker, so every census (round, consecutive-failure, watermark) ignores it and the feedback filters keep it out of the agent's prompt. Posting is best-effort: a digest failure never fails a good push. The rejected-attempt count matches both the current and the reworded gate-rejection headline so the census cannot silently zero; base updates carry no win= field and are windowed by timestamp instead (the window key is the engage ack's created_at). * feat(autofix): milestone digest review follow-ups - Residual bucket: every outcome the four buckets missed (crash, model error, gate error, infra) now lands in 'other round(s)' — a window that burned 80% of its budget on crashes was rendering as four zeros, QUIETER than a healthy window, the inverse of the digest's premise. - Crossing trigger: fire on the first pushed round once 10+ rounds have accumulated since the last digest in this window, instead of an exact %10 hit — failure rounds advance the counter too, so push@9/crash@10/ push@11 skipped the digest forever on exactly the failure-heavy PRs it exists for. - The success log is chained to the post; a failed comment no longer logs 'posted' after its own warning. - WINDOW=none says 'since the PR opened (no counting window yet)' instead of claiming a current window while counting all time. - A census that parses zero window markers at round 10+ skips the digest instead of posting a fabricated all-zero one. - autofix-milestone joins BOT_COMMENT_FILTER (marker inventory). - The timeout needle anchors on the verbatim emitted headline. - The pin-only test became a behavioral replay: the digest block runs under bash with a stubbed gh over fixture ic.json histories — bucket counts, residual loudness, crossing suppression/resumption, old-window isolation, none-window phrasing, non-pushed/non-takeover gating, and the empty-census skip. The OUTCOME == fixed conjunct is pinned. * test(autofix): cross-pin digest census needles to headline emission sites (#8046) * test(autofix): widen rejected-headline cross-pin to match #8044 reword (#8046) * test(autofix): behaviorally cover the digest comment-failure branch (#8046) --------- Co-authored-by: verify Co-authored-by: qwen-code-dev-bot Co-authored-by: qwen-code-dev-bot --- .github/workflows/qwen-autofix.yml | 85 +++++- scripts/tests/qwen-autofix-workflow.test.js | 285 +++++++++++++++++++- 2 files changed, 366 insertions(+), 4 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index cc65d6dbb2..1f86721c8c 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -2281,7 +2281,7 @@ jobs: # bot's own eval markers, and known non-actionable bot comments # (triage stages, coverage reports, legacy suggestion summaries, # force-push reminders). - BOT_COMMENT_FILTER='") ] | .[] + | select(.[1] == $win) | (.[0] | tonumber) ] + | max // 0' "${WORKDIR}/ic.json" 2> /dev/null || echo 0)" + if [[ "$(( NEXT_ROUND - MS_LAST ))" -ge 10 ]]; then + WIN_HEADS="$(jq -r --arg ab "${AUTOFIX_BOT}" --arg win "${WINDOW:-none}" ' + [.[] | select((.user.login // "") == $ab) + | select((.body // "") | contains("")) + or ($win == "none" and (((.body // "") | contains("win=")) | not)))] + | sort_by(.created_at) | .[] + | (.body | gsub("\r"; "") | split("\n")[0])' "${WORKDIR}/ic.json" 2> /dev/null || true)" + if [[ -z "${WIN_HEADS}" ]]; then + # Reaching round 10+ with zero window markers means the + # parse failed (prior markers must exist to be here) — a + # fabricated all-zero census is worse than no digest. + echo "::warning::milestone census found no window markers on #${PR}; skipping the digest" + else + N_PUSHED="$(grep -c 'Addressed the latest review feedback' <<< "${WIN_HEADS}" || true)" + # This round's own marker was posted just above but ic.json + # predates it — count it in by hand. + N_PUSHED=$(( N_PUSHED + 1 )) + N_NOOP="$(grep -c 'no changes needed' <<< "${WIN_HEADS}" || true)" + # Needle matches the emitted headline verbatim — first + # lines can embed provider error text. + N_TIMEOUT="$(grep -c 'AutoFix ran out of time before finishing' <<< "${WIN_HEADS}" || true)" + # Both wordings of the gate-rejection handoff, past and + # present — the census must not silently zero when the + # headline is reworded. + N_REJECTED="$(grep -cE 'Could not (address the latest feedback|produce a passing fix)' <<< "${WIN_HEADS}" || true)" + # Every other outcome (crash, model error, gate error, + # infra) lands in a residual bucket: a window that burned + # 80% of its budget on crashes must be the LOUDEST line in + # the digest, not four zeros quieter than a healthy one. + N_TOTAL=$(( $(grep -c . <<< "${WIN_HEADS}" || true) + 1 )) + N_OTHER=$(( N_TOTAL - N_PUSHED - N_NOOP - N_TIMEOUT - N_REJECTED )) + (( N_OTHER < 0 )) && N_OTHER=0 + # Base updates carry their own marker with no win= field; + # their window is recovered by timestamp (the window key IS + # the engage ack's created_at — 'none' means count all, + # and the header says so). + N_BASE="$(jq -r --arg ab "${AUTOFIX_BOT}" --arg win "${WINDOW:-none}" ' + [.[] | select((.user.login // "") == $ab) + | select((.body // "") | contains("")) + | select($win == "none" or ((.created_at // "") > $win))] + | length' "${WORKDIR}/ic.json" 2> /dev/null || echo 0)" + WIN_DESC='in the current window' + WIN_DESC_ZH='当前窗口' + if [[ "${WINDOW:-none}" == 'none' ]]; then + WIN_DESC='since the PR opened (no counting window yet)' + WIN_DESC_ZH='自 PR 创建以来（尚无计数窗口）' + fi + if gh pr comment "${PR}" --repo "${REPO}" --body "$(printf '📊 Takeover milestone — round %s/%s, %s. Census: %s pushed fix(es), %s no-change review(s), %s timeout(s), %s rejected attempt(s), %s other round(s) (crash / model error / gate error / infra), %s base update(s).\n\nThis many rounds deserves a human look. Options: keep going (fine — nothing changes), split or reduce the PR if rounds keep accumulating, or release takeover (remove the `%s` label or comment `%s stop`). Management continues unchanged unless you act.\n\n
\n中文说明\n\n📊 接管里程碑 —— 第 %s/%s 轮（%s）。统计：推送修复 %s 次、审阅无需改动 %s 次、超时 %s 次、验证拒绝 %s 次、其他轮次（崩溃/模型错误/门错误/infra）%s 次、base 更新 %s 次。\n\n轮次到这个量值得人工看一眼。可选：继续（无需操作）；若轮次持续累积，考虑拆分或缩减 PR；或释放接管（移除 `%s` 标签或评论 `%s stop`）。不操作则托管照常继续。\n\n
\n\n' "${NEXT_ROUND}" "${MAX_ROUNDS}" "${WIN_DESC}" "${N_PUSHED}" "${N_NOOP}" "${N_TIMEOUT}" "${N_REJECTED}" "${N_OTHER}" "${N_BASE}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}" "${NEXT_ROUND}" "${MAX_ROUNDS}" "${WIN_DESC_ZH}" "${N_PUSHED}" "${N_NOOP}" "${N_TIMEOUT}" "${N_REJECTED}" "${N_OTHER}" "${N_BASE}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}" "${NEXT_ROUND}" "${WINDOW:-none}")"; then + echo "📊 milestone digest posted on #${PR} (round ${NEXT_ROUND})" + else + echo "::warning::milestone digest failed to post on PR #${PR}; the round report above already landed" + fi + fi + fi + fi + { ISSUE_REF="" [[ "${ISSUE}" != "${PR}" ]] && ISSUE_REF=" (issue #${ISSUE})" diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 094e4ebabd..85b01659e2 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -4609,6 +4609,285 @@ describe('qwen-autofix workflow', () => { expect(workflow).not.toContain('cat > "${proxy_script}"'); }); + it('posts a takeover milestone digest as rounds accumulate, with a residual bucket', () => { + // The takeover cap (100) bounds runaway but says nothing about when a + // human should step in: #7469 ground to round 12 over 7 days with the + // only budget signal buried in Actions logs. Once 10+ rounds accumulate + // since the last digest, a window-scoped census lands on the PR itself. + // Digest fires only on takeover PRs and only for PUSHED rounds… + expect(pushAndReportStep).toContain('"${OUTCOME}" == "fixed"'); + expect(pushAndReportStep).toContain( + '"${MAX_ROUNDS}" == "${TAKEOVER_MAX_ROUNDS}"', + ); + // …on a CROSSING trigger, not an equality test: failure rounds also + // advance the counter, so `push@9, crash@10, push@11` would skip an + // exact %10 forever — on exactly the failure-heavy PR the digest + // exists for. + expect(pushAndReportStep).toContain('"${NEXT_ROUND}" -ge 10'); + expect(pushAndReportStep).toContain('$(( NEXT_ROUND - MS_LAST ))'); + // Its own marker, NOT autofix-eval: every census (round, consec, + // watermark) selects on autofix-eval, so the digest must stay + // invisible to all of them; the feedback filters drop bot comments, + // so the agent never reads it as feedback either. + expect(pushAndReportStep).toContain('`, + }); + const baseC = (at) => ({ + user: { login: 'qwen-code-dev-bot' }, + created_at: at, + body: '🔀 Base updated: …\n', + }); + const msC = (round, win, at) => ({ + user: { login: 'qwen-code-dev-bot' }, + created_at: at, + body: `📊 …\n`, + }); + const runDigest = ( + comments, + { + nextRound = 10, + window = K, + outcome = 'fixed', + maxRounds = '100', + commentExit = 0, + } = {}, + ) => { + const dir = mkdtempSync(join(tmpdir(), 'milestone-')); + try { + writeFileSync(join(dir, 'ic.json'), JSON.stringify(comments)); + const bin = join(dir, 'bin'); + mkdirSync(bin); + const commentBody = + commentExit === 0 + ? `printf '%s' "$7" > ${JSON.stringify(join(dir, 'digest.md'))}; exit 0` + : `exit ${commentExit}`; + writeFileSync( + join(bin, 'gh'), + `#!/usr/bin/env bash\nif [[ "$1" == 'pr' && "$2" == 'comment' ]]; then ${commentBody}; fi\nexit 1\n`, + ); + chmodSync(join(bin, 'gh'), 0o755); + const log = execFileSync( + 'bash', + ['-c', `set -euo pipefail\n${digestBlock.replace(/\n {10}/g, '\n')}`], + { + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + WORKDIR: dir, + OUTCOME: outcome, + NEXT_ROUND: String(nextRound), + MAX_ROUNDS: maxRounds, + TAKEOVER_MAX_ROUNDS: '100', + WINDOW: window, + AUTOFIX_BOT: 'qwen-code-dev-bot', + REPO: 'o/r', + PR: '1', + TAKEOVER_LABEL: 'autofix/takeover', + TAKEOVER_COMMAND: '@qwen-code /takeover', + }, + encoding: 'utf8', + }, + ); + const digestPath = join(dir, 'digest.md'); + return { + log, + body: existsSync(digestPath) ? readFileSync(digestPath, 'utf8') : '', + }; + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + + // Mixed healthy history: counts land in the right buckets, an + // old-window push and a HUMAN quoting a marker verbatim are excluded, + // both rejection wordings count, base updates window by timestamp. + const mixed = runDigest([ + evalC(HEADS.push, K, '2026-07-02T00:00:00Z'), + evalC(HEADS.push, K, '2026-07-03T00:00:00Z'), + evalC(HEADS.push, K, '2026-07-04T00:00:00Z'), + evalC(HEADS.push, K, '2026-07-05T00:00:00Z'), + evalC(HEADS.push, K, '2026-07-06T00:00:00Z'), + evalC(HEADS.noop, K, '2026-07-07T00:00:00Z'), + evalC(HEADS.timeout, K, '2026-07-08T00:00:00Z'), + evalC(HEADS.rejectedOld, K, '2026-07-09T00:00:00Z'), + evalC(HEADS.rejectedNew, K, '2026-07-10T00:00:00Z'), + evalC(HEADS.push, '2026-05-01T00:00:00Z', '2026-06-01T00:00:00Z'), + evalC(HEADS.push, K, '2026-07-11T00:00:00Z', 'some-human'), + baseC('2026-07-12T00:00:00Z'), + baseC('2026-05-02T00:00:00Z'), + ]); + expect(mixed.body).toContain( + '6 pushed fix(es), 1 no-change review(s), 1 timeout(s), 2 rejected attempt(s), 0 other round(s)', + ); + expect(mixed.body).toContain('1 base update(s)'); + expect(mixed.body).toContain('round 10/100, in the current window'); + expect(mixed.body).toContain( + '', + ); + expect(mixed.log).toContain('milestone digest posted'); + + // Failure-heavy window: crashes and gate errors land in the residual + // bucket and make it the LOUDEST line, not four zeros quieter than a + // healthy window. + const grim = runDigest([ + evalC(HEADS.push, K, '2026-07-02T00:00:00Z'), + evalC(HEADS.crash, K, '2026-07-03T00:00:00Z'), + evalC(HEADS.crash, K, '2026-07-04T00:00:00Z'), + evalC(HEADS.crash, K, '2026-07-05T00:00:00Z'), + evalC(HEADS.crash, K, '2026-07-06T00:00:00Z'), + evalC(HEADS.gate, K, '2026-07-07T00:00:00Z'), + evalC(HEADS.gate, K, '2026-07-08T00:00:00Z'), + evalC(HEADS.gate, K, '2026-07-09T00:00:00Z'), + evalC(HEADS.gate, K, '2026-07-10T00:00:00Z'), + ]); + expect(grim.body).toContain( + '2 pushed fix(es), 0 no-change review(s), 0 timeout(s), 0 rejected attempt(s), 8 other round(s)', + ); + + // Crossing trigger: a digest at round 10 suppresses round 12 but not + // round 20; a failure at the exact multiple no longer loses the digest. + const suppressed = runDigest( + [ + evalC(HEADS.push, K, '2026-07-02T00:00:00Z'), + msC(10, K, '2026-07-03T00:00:00Z'), + ], + { nextRound: 12 }, + ); + expect(suppressed.body).toBe(''); + const dueAgain = runDigest( + [ + evalC(HEADS.push, K, '2026-07-02T00:00:00Z'), + msC(10, K, '2026-07-03T00:00:00Z'), + ], + { nextRound: 20 }, + ); + expect(dueAgain.body).toContain('round 20/100'); + // An old-window milestone marker does not suppress a fresh window. + const freshWindow = runDigest( + [ + evalC(HEADS.push, K, '2026-07-02T00:00:00Z'), + msC(11, '2026-05-01T00:00:00Z', '2026-07-03T00:00:00Z'), + ], + { nextRound: 11 }, + ); + expect(freshWindow.body).toContain('round 11/100'); + + // WINDOW=none says what it counts instead of claiming a window. + const noWindow = runDigest( + [evalC(HEADS.push, null, '2026-07-02T00:00:00Z')], + { window: 'none' }, + ); + expect(noWindow.body).toContain('since the PR opened'); + + // Non-pushing outcomes and non-takeover caps never digest. + expect( + runDigest([evalC(HEADS.push, K, '2026-07-02T00:00:00Z')], { + outcome: 'noop', + }).body, + ).toBe(''); + expect( + runDigest([evalC(HEADS.push, K, '2026-07-02T00:00:00Z')], { + maxRounds: '10', + }).body, + ).toBe(''); + // A census that parses zero window markers at round 10+ posts NOTHING + // rather than a fabricated all-zero digest. + const empty = runDigest([]); + expect(empty.body).toBe(''); + expect(empty.log).toContain('skipping the digest'); + + // Best-effort is behavioral, not just string-pinned: when `gh pr + // comment` fails, the block must still return normally (no throw under + // the step's `set -e` lineage) and only warn — a good push must never + // go red over a failed digest. + const commentFailed = runDigest( + [evalC(HEADS.push, K, '2026-07-02T00:00:00Z')], + { commentExit: 1 }, + ); + expect(commentFailed.body).toBe(''); + expect(commentFailed.log).toContain( + '::warning::milestone digest failed to post on PR #1', + ); + + // Best-effort stays pinned: the success log is chained to the post + // (no unconditional "posted" after a failed comment), the failure + // path only warns. + expect(pushAndReportStep).toMatch( + /then\n\s+echo "📊 milestone digest posted/, + ); + expect(pushAndReportStep).toContain('milestone digest failed to post'); + }); + it('pushes autofix branches without rewriting remote history', () => { expect(workflow).not.toMatch(/\bgit push\b[^\n]*--force(?:-with-lease)?/); // No bare -f / +refspec force forms either. (--no-verify is NOT a force @@ -6191,13 +6470,13 @@ describe('qwen-autofix workflow', () => { expect(workflow).toContain("RETRY_COMMAND: '@qwen-code /retry'"); expect(workflow).toContain(''); expect(workflow).toContain( - '")) + or ($key == "none" and (((.body // "") | contains("win=")) | not))) + ] | sort_by(.created_at) + | map((.body | gsub("\r"; "") | split("\n")[0])) + | (map(test("Addressed the latest review feedback|no changes needed")) | rindex(true) // -1) as $lastok + | [ .[($lastok + 1):][] | select(contains("AutoFix ran out of time before finishing")) ] | length' "${WORKDIR}/ic.json" 2> /dev/null || true)" + if [[ "${PRIOR_TIMEOUTS}" -ge 1 ]]; then + echo + echo '## Budget warning: previous round(s) ran out of time' + echo + echo "${PRIOR_TIMEOUTS} round(s) since the last successful round exhausted the agent time budget before finishing anything. Do NOT attempt everything at once this round:" + echo '- Address the smallest set of blocking (Critical) findings first, and commit as soon as that subset is done.' + echo '- Prefer minimal, focused diffs; decline refactors and nice-to-haves with a one-line reason rather than implementing them.' + echo '- If the remaining feedback cannot fit the budget, defer it the normal way: leave those findings out of resolved-comments.txt and record each deferral in comment-replies.json so every open thread gets its reason — never let a deferral live only in the summary.' + fi } > "${WORKDIR}/feedback.md" echo '--- feedback.md ---' cat "${WORKDIR}/feedback.md" @@ -3222,6 +3268,7 @@ jobs: "${WORKDIR}/failure.md" \ "${WORKDIR}/handoff.md" \ "${WORKDIR}/gate-output.log" \ + "${WORKDIR}/gate-rejection.md" \ "${WORKDIR}/agent-api-error" \ "${WORKDIR}/agent-api-error-kind" \ "${WORKDIR}/agent-timeout" @@ -3866,7 +3913,22 @@ jobs: MARK_TS='9999-12-31T23:59:59Z' HEADLINE="🤖 AutoFix updated a stale base — the fix did not pass verification, but this PR was behind \`${DEFAULT_BRANCH:-main}\`, so it merged current main in via update-branch and will retry on the next scan. A stale base (a dependency or symbol main already changed) can fail the build without being the fix's fault; if it still fails once current, it hands off to a human." else - HEADLINE="🤖 Could not address the latest feedback automatically (round ${MARK_ROUND}/${MAX_ROUNDS}). A human should take over this PR." + # Say what actually happens next. The old "A human should + # take over this PR" read as a full release, but the loop + # is NOT done with the PR: this feedback's watermark + # advances (no automatic retry of THIS item), while + # management continues for new feedback and base conflicts + # — #7929 posted the old wording and then kept pushing + # rounds, which read as a contradiction. + # Name the gate ONLY when it actually ran: this branch is + # reached for every outcome=failed verdict, but reject_fix + # is the sole writer of gate-rejection.md — the failure.md / + # dirty-tree / unchanged-branch / missing-summary paths made + # no gate decision, so a blanket clause would repeat the very + # wording-doesn't-match-behaviour bug this PR fixes. + GATE_CLAUSE='' + [[ -s "${WORKDIR}/gate-rejection.md" ]] && GATE_CLAUSE=' — the verification gate rejected the attempt' + HEADLINE="🤖 Could not produce a passing fix for this feedback (round ${MARK_ROUND}/${MAX_ROUNDS})${GATE_CLAUSE}. This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own." fi fi elif [[ "${PREPARE_OUTCOME}" != 'success' && "${PREPARE_OUTCOME}" != 'failure' ]]; then @@ -3967,6 +4029,36 @@ jobs: MARK_ROUND="${MAX_ROUNDS}" HEADLINE="🤖 AutoFix stopped after ${CONSEC_FAIL} consecutive rounds that failed to push anything (timeouts and/or gate rejections). Retrying at the same per-round budget is not converging — this usually means the PR is too large or conflicts with a fast-moving \`main\`. A human should rebase, split, or reduce it, then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." fi + # CUMULATIVE timeout breaker — the sibling of the consecutive + # one above, for the failure shape it cannot see: timeouts + # interleaved with pushed rounds. A push resets CONSEC_FAIL, + # but it does not make the next timeout cheaper — each burns a + # full agent budget with nothing to show (observed on #7929: + # three timeouts with successes in between; #7846 twice). The + # census reuses PRIOR_HEADS, so it is window-scoped exactly + # like the consecutive one and a re-arm clears it. Only + # overrides a would-be RETRY: a round already terminal keeps + # its own headline (the consecutive breaker included). + if [[ "${MARK_ROUND}" != "${MAX_ROUNDS}" ]]; then + # Needle matches the emitted headline verbatim — first lines + # can embed provider error text (API_ERROR_DETAIL puts up to + # 200 bytes of it on the same line), so a loose phrase could + # count a model error message as a timeout. + TIMEOUT_N="$(grep -c 'AutoFix ran out of time before finishing' <<< "${PRIOR_HEADS}" || true)" + if [[ -n "${AGENT_TIMEOUT:-}" ]]; then + TIMEOUT_N=$(( TIMEOUT_N + 1 )) + fi + if [[ "${TIMEOUT_N}" -ge "${TIMEOUT_WINDOW_CAP}" ]]; then + MARK_ROUND="${MAX_ROUNDS}" + # The headline states what the census MEASURED — the + # window's cumulative count — not "stopped after N + # timeouts": the round that trips this can itself have + # failed differently (a gate rejection landing on a window + # that already carries the cap — the exact rollout state + # of #7929/#7846). + HEADLINE="🤖 AutoFix stopped: this counting window now contains ${TIMEOUT_N} time-budget exhaustions (pushed rounds in between included; this round itself may have failed differently). That is ${TIMEOUT_N} full agent runs that pushed nothing. A human should split or reduce the PR (or raise the agent time budget), then comment \`${RETRY_COMMAND}\` to re-arm. Until then future scans will skip this PR." + fi + fi fi { echo "${HEADLINE}" diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 85b01659e2..080f4fd192 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -3205,6 +3205,162 @@ describe('qwen-autofix workflow', () => { expect(broken.body).toBe(''); }); + it('narrows the agent prompt after a timeout since the last successful round', () => { + // Re-running the identical address-everything prompt after a timeout + // walks straight into the same wall (#7929 burned three 50-minute + // timeouts that way, #7846 two). From the second attempt on, the + // feedback file ends with an explicit narrowing instruction: smallest + // blocking subset first, commit early, and defer through the SKILL + // contract (out of resolved-comments.txt, into comment-replies.json) + // — never only into the summary, which is exactly the cheap path a + // budget-pressured agent would otherwise take. + expect(prepareBranchAndFeedbackStep).toContain('PRIOR_TIMEOUTS='); + expect(prepareBranchAndFeedbackStep).toContain( + 'Budget warning: previous round(s) ran out of time', + ); + expect(prepareBranchAndFeedbackStep).toContain( + 'commit as soon as that subset is done', + ); + expect(prepareBranchAndFeedbackStep).toContain( + 'record each deferral in comment-replies.json', + ); + expect(prepareBranchAndFeedbackStep).toContain( + 'decline refactors and nice-to-haves with a one-line reason', + ); + // The trigger threshold itself is pinned — a `-ge 99` mutation would + // otherwise leave the feature inert with every string pin green. + expect(prepareBranchAndFeedbackStep).toContain( + 'if [[ "${PRIOR_TIMEOUTS}" -ge 1 ]]', + ); + + // Behavioral replay of the census (the string pins alone cannot catch + // a broken filter): extract the real jq and run it against fixture + // ic.json shapes. + const censusSrc = prepareBranchAndFeedbackStep.match( + /PRIOR_TIMEOUTS="\$\(jq -r[\s\S]*?ic\.json" 2> \/dev\/null \|\| true\)"/, + )?.[0]; + expect(censusSrc).toBeTruthy(); + const TIMEOUT_HEADLINE = + '🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 2/100) — it will retry on the next scan.'; + const PUSH_HEADLINE = + '🤖 Addressed the latest review feedback (round 2/100). What changed…'; + const NOOP_HEADLINE = + '🤖 Reviewed the latest feedback — no changes needed. Why, point by point:…'; + const mk = (headline, win, at, login = 'qwen-code-dev-bot') => ({ + user: { login }, + created_at: at, + body: `${headline}\n`, + }); + const runCensus = (comments, key) => { + const dir = mkdtempSync(join(tmpdir(), 'timeout-census-')); + try { + writeFileSync(join(dir, 'ic.json'), JSON.stringify(comments)); + return execFileSync( + 'bash', + [ + '-c', + [ + 'set -uo pipefail', + `WORKDIR='${dir}'`, + "AUTOFIX_BOT='qwen-code-dev-bot'", + `LIVE_REARM_KEY='${key}'`, + censusSrc, + 'printf %s "${PRIOR_TIMEOUTS}"', + ].join('\n'), + ], + { encoding: 'utf8' }, + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const K = '2026-07-29T03:00:00Z'; + // A push RESETS the narrowing (the breaker stays cumulative — this + // census feeds only the prompt): timeout, push, timeout → 1. + expect( + runCensus( + [ + mk(TIMEOUT_HEADLINE, K, '2026-07-29T04:00:00Z'), + mk(PUSH_HEADLINE, K, '2026-07-29T05:00:00Z'), + mk(TIMEOUT_HEADLINE, K, '2026-07-29T06:00:00Z'), + ], + K, + ), + ).toBe('1'); + // A no-op round RESETS the narrowing too: the reset alternation has a + // `no changes needed` branch the push case above never touches, so a + // no-op between two timeouts must also collapse the count to the single + // trailing timeout. + expect( + runCensus( + [ + mk(TIMEOUT_HEADLINE, K, '2026-07-29T04:00:00Z'), + mk(NOOP_HEADLINE, K, '2026-07-29T05:00:00Z'), + mk(TIMEOUT_HEADLINE, K, '2026-07-29T06:00:00Z'), + ], + K, + ), + ).toBe('1'); + // Two trailing timeouts count as two. + expect( + runCensus( + [ + mk(PUSH_HEADLINE, K, '2026-07-29T04:00:00Z'), + mk(TIMEOUT_HEADLINE, K, '2026-07-29T05:00:00Z'), + mk(TIMEOUT_HEADLINE, K, '2026-07-29T06:00:00Z'), + ], + K, + ), + ).toBe('2'); + // Legacy pre-takeover markers (no win= field) count under key 'none' — + // the common real case: a PR that timed out before any re-arm. + expect( + runCensus( + [ + mk(TIMEOUT_HEADLINE, null, '2026-07-29T04:00:00Z'), + mk(TIMEOUT_HEADLINE, null, '2026-07-29T05:00:00Z'), + ], + 'none', + ), + ).toBe('2'); + // Old-window timeouts do not leak into a fresh window (re-arm clears). + expect( + runCensus( + [ + mk(TIMEOUT_HEADLINE, 'old-window', '2026-07-29T04:00:00Z'), + mk(TIMEOUT_HEADLINE, 'old-window', '2026-07-29T05:00:00Z'), + ], + K, + ), + ).toBe('0'); + // Non-timeout rounds and a HUMAN quoting the timeout headline verbatim + // both count zero (author-filtered like every census). + expect(runCensus([mk(PUSH_HEADLINE, K, '2026-07-29T04:00:00Z')], K)).toBe( + '0', + ); + expect( + runCensus( + [mk(TIMEOUT_HEADLINE, K, '2026-07-29T04:00:00Z', 'some-human')], + K, + ), + ).toBe('0'); + + // The census needle matches the emitted headline VERBATIM — first + // lines can embed provider error text (API_ERROR_DETAIL), so a loose + // phrase could count a model error message as a timeout. + expect(prepareBranchAndFeedbackStep).toContain( + 'contains("AutoFix ran out of time before finishing")', + ); + expect(reviewAddressReportStep).toContain( + 'CAUSE="ran out of time before finishing (${AGENT_TIMEOUT})"', + ); + // Pin the template that JOINS them: a mutation to the prefix (e.g. + // adding a colon) breaks the census while both pins above stay green. + expect(reviewAddressReportStep).toContain( + 'HEADLINE="🤖 AutoFix ${CAUSE} (attempt', + ); + }); + it('switches to Critical-only feedback after five change rounds', () => { // ROUND counts change-producing rounds, so 4 still starts the fifth // suggestion-capable change while 5 starts the first Critical-only round. @@ -5240,7 +5396,7 @@ describe('qwen-autofix workflow', () => { repairDeterministicRejectionStep.match( /rm -f \\\n([\s\S]*?)\n {10}rm -rf "\$\{QWEN_HOME\}"/, )?.[1] ?? ''; - expect(repairCleanup).not.toContain('gate-rejection.md'); + expect(repairCleanup).toContain('gate-rejection.md'); expect(repairDeterministicRejectionStep).not.toContain( '"${WORKDIR}/resolved-comments.txt"', ); @@ -5426,7 +5582,16 @@ describe('qwen-autofix workflow', () => { 'for f in failure.md handoff.md address-summary.md no-action.md', ); expect(reviewAddressReportStep).toContain( - 'Could not address the latest feedback automatically', + 'Could not produce a passing fix for this feedback', + ); + // The handoff must not read as a full release: the loop stays engaged + // for NEW feedback (#7929 posted "a human should take over" and then + // kept pushing rounds — a contradiction to anyone reading the thread). + expect(reviewAddressReportStep).toContain( + 'the loop stays engaged and still picks up new feedback', + ); + expect(reviewAddressReportStep).toContain( + 'will not retry this item on its own', ); expect(reviewAddressReportStep).toContain('gh pr comment "${PR}"'); expect(reviewAddressReportStep).toContain( @@ -5693,13 +5858,18 @@ describe('qwen-autofix workflow', () => { expect(decision).toBeTruthy(); const SENTINEL = '9999-12-31T23:59:59Z'; const NEWEST = '2026-07-20T10:00:00Z'; - const run = (env) => { + const run = (env, { gateRejection = false } = {}) => { // The gate-rejection branch (OUTCOME=failed, no crash/timeout) now probes // whether the PR is behind main and, if so, updates the base — so stub gh: // commits/
→ a SHA, compare → CMP_STATUS_STUB (default 'ahead', i.e. // NOT behind, so the existing rejection cases still hand off), and // update-branch → UPDATE_OK_STUB (default success). const dir = mkdtempSync(join(tmpdir(), 'decision-')); + // reject_fix is the ONLY writer of gate-rejection.md, so its presence is + // the exact "the gate actually ran" discriminator the headline keys on. + if (gateRejection) { + writeFileSync(join(dir, 'gate-rejection.md'), '**build failed**'); + } const bin = join(dir, 'bin'); mkdirSync(bin); writeFileSync( @@ -5725,6 +5895,7 @@ describe('qwen-autofix workflow', () => { env: { ...process.env, PATH: `${bin}:${process.env.PATH}`, + WORKDIR: dir, REPO: 'o/r', PR: '1', REPORT_HEAD: 'prhead123', @@ -5747,9 +5918,24 @@ describe('qwen-autofix workflow', () => { // Declared rejection, PR up to date ('ahead') -> a genuine fix failure -> // advance the watermark and hand off to a human. - const rejected = run({ OUTCOME: 'failed' }); + // A genuine gate rejection: reject_fix wrote gate-rejection.md, so the + // headline names the gate. + const rejected = run({ OUTCOME: 'failed' }, { gateRejection: true }); expect(rejected.split('|')[0]).toBe(NEWEST); - expect(rejected).toContain('Could not address the latest feedback'); + expect(rejected).toContain('Could not produce a passing fix'); + expect(rejected).toContain('the verification gate rejected the attempt'); + expect(rejected).toContain('stays engaged'); + // outcome=failed WITHOUT a gate decision (failure.md abort, dirty tree, + // unchanged branch, or missing summary) must NOT claim the gate rejected: + // gate-rejection.md is written only by reject_fix, so its absence is the + // exact discriminator. A blanket clause would repeat the + // wording-doesn't-match-behaviour bug this PR fixes. + const failedNoGate = run({ OUTCOME: 'failed' }); + expect(failedNoGate.split('|')[0]).toBe(NEWEST); + expect(failedNoGate).toContain('Could not produce a passing fix'); + expect(failedNoGate).not.toContain( + 'the verification gate rejected the attempt', + ); // #7471: the gate rejected the fix, but the PR was BEHIND main — the build // failed on a stale base (a dependency main already removed), not the fix. @@ -5774,7 +5960,7 @@ describe('qwen-autofix workflow', () => { UPDATE_OK_STUB: '0', }); expect(staleConflict.split('|')[0]).toBe(NEWEST); - expect(staleConflict).toContain('Could not address the latest feedback'); + expect(staleConflict).toContain('Could not produce a passing fix'); // Gate crash (no verdict): keep the feedback live and retry. const crashed = run({ OUTCOME: '' }); @@ -5974,6 +6160,12 @@ describe('qwen-autofix workflow', () => { workflow.match(/TAKEOVER_MAX_ROUNDS: '(\d+)'/)?.[1], ); expect(cap).toBeLessThan(takeoverCap); + // Cumulative timeout sub-cap: same constraints as the consecutive one. + const timeoutCap = Number( + workflow.match(/TIMEOUT_WINDOW_CAP: '(\d+)'/)?.[1], + ); + expect(timeoutCap).toBeGreaterThan(0); + expect(timeoutCap).toBeLessThan(takeoverCap); const block = reviewAddressReportStep.match( /if \[\[ "\$\{MARK_ROUND\}" != "\$\{MAX_ROUNDS\}" \]\] && \[\[ "\$\{PREPARE_OUTCOME\}" == 'success' \|\| "\$\{PREPARE_OUTCOME\}" == 'failure' \]\] && \[\[ "\$\{STALE_BASE_RETRY:-false\}" != 'true' \]\] && \{ \[\[ -z "\$\{API_ERROR_DETAIL\}" \]\] \|\| \[\[ "\$\{API_ERROR_KIND\}" == 'auth' \]\]; \}; then\n {14}CONSEC_FAIL=1\n[\s\S]*?\n {14}fi\n {12}fi\n/, @@ -5982,7 +6174,7 @@ describe('qwen-autofix workflow', () => { const script = block.replace(/^ {12}/gm, ''); const FAIL = - '🤖 Could not address the latest feedback automatically (round 3/100).'; + '🤖 Could not produce a passing fix for this feedback (round 3/100) — the verification gate rejected the attempt.'; const FAIL_TIMEOUT = '🤖 AutoFix could not reach the model (attempt 2/3)'; const PUSH = '🤖 Addressed the latest review feedback (round 2/100).'; const NOOP = '🤖 Reviewed the latest feedback — no changes needed.'; @@ -6004,6 +6196,7 @@ describe('qwen-autofix workflow', () => { apiErrorKind = '', prepareOutcome = 'success', staleBaseRetry = false, + agentTimeout = '', } = {}, ) => { const dir = mkdtempSync(join(tmpdir(), 'consec-')); @@ -6032,7 +6225,7 @@ describe('qwen-autofix workflow', () => { 'bash', [ '-c', - `set -uo pipefail\nWORKDIR='${dir}'\nMARK_ROUND=${markRound}\nMAX_ROUNDS=100\nCONSECUTIVE_FAILURE_CAP=${cap}\nCONSEC_FAIL=0\nREPO=o/r\nPR=1\nAUTOFIX_BOT=qwen-code-dev-bot\nRETRY_COMMAND='@qwen-code /retry'\nAPI_ERROR_DETAIL='${apiErrorDetail}'\nAPI_ERROR_KIND='${apiErrorKind}'\nPREPARE_OUTCOME='${prepareOutcome}'\nSTALE_BASE_RETRY='${staleBaseRetry}'\n${window !== undefined ? `WINDOW='${window}'\n` : ''}HEADLINE=orig\n${script}\nprintf '%s|%s|%s' "$MARK_ROUND" "${'${CONSEC_FAIL}'}" "$HEADLINE"`, + `set -uo pipefail\nWORKDIR='${dir}'\nMARK_ROUND=${markRound}\nMAX_ROUNDS=100\nCONSECUTIVE_FAILURE_CAP=${cap}\nTIMEOUT_WINDOW_CAP=${timeoutCap}\nAGENT_TIMEOUT='${agentTimeout}'\nCONSEC_FAIL=0\nREPO=o/r\nPR=1\nAUTOFIX_BOT=qwen-code-dev-bot\nRETRY_COMMAND='@qwen-code /retry'\nAPI_ERROR_DETAIL='${apiErrorDetail}'\nAPI_ERROR_KIND='${apiErrorKind}'\nPREPARE_OUTCOME='${prepareOutcome}'\nSTALE_BASE_RETRY='${staleBaseRetry}'\n${window !== undefined ? `WINDOW='${window}'\n` : ''}HEADLINE=orig\n${script}\nprintf '%s|%s|%s' "$MARK_ROUND" "${'${CONSEC_FAIL}'}" "$HEADLINE"`, ], { env: { ...process.env, PATH: `${bin}:${process.env.PATH}` }, @@ -6142,6 +6335,81 @@ describe('qwen-autofix workflow', () => { terminal: true, headline: 'orig', }); + + // CUMULATIVE timeout breaker: pushes in between reset CONSEC_FAIL but + // must NOT reset this one — #7929's exact shape (timeout, push, timeout, + // push, timeout) never tripped the consecutive cap while burning a full + // agent budget each time. + const TIMEOUT_HEAD = + '🤖 AutoFix ran out of time before finishing (timeout (3000000ms)) (attempt 2/100) — it will retry on the next scan.'; + const interleaved = run([TIMEOUT_HEAD, PUSH, TIMEOUT_HEAD, PUSH], { + agentTimeout: 'timeout (3000000ms)', + }); + expect(interleaved.terminal).toBe(true); + expect(interleaved.headline).toContain('time-budget exhaustions'); + expect(interleaved.headline).toContain('/retry'); + // One short of the cap keeps retrying (current round not a timeout). + expect(run([TIMEOUT_HEAD, PUSH, TIMEOUT_HEAD])).toMatchObject({ + terminal: false, + }); + // Window-scoped like every other census: pre-re-arm timeouts don't count. + expect( + run( + [ + { headline: TIMEOUT_HEAD, win: 'old-window' }, + { headline: TIMEOUT_HEAD, win: 'old-window' }, + ], + { window: 'new-window', agentTimeout: 'timeout (3000000ms)' }, + ), + ).toMatchObject({ terminal: false }); + // A NON-timeout failure landing on an already-capped window still + // trips it — the #7929/#7846 rollout state the headline's + // parenthetical describes. A plausible "only count when this round + // timed out" cleanup (wrapping the block in an AGENT_TIMEOUT check) + // would silently delete this documented case. + expect( + run([TIMEOUT_HEAD, PUSH, TIMEOUT_HEAD, PUSH, TIMEOUT_HEAD]), + ).toMatchObject({ terminal: true }); + // Inherited from the outer guard, pinned so a refactor that hoists the + // timeout block out of it cannot mass-terminate every in-flight PR + // during a provider outage. + expect( + run(Array(5).fill(TIMEOUT_HEAD), { + apiErrorDetail: '429 rate limited', + apiErrorKind: 'transient', + }), + ).toMatchObject({ terminal: false }); + // The timeout breaker ALSO inherits the stale-base exemption from the + // outer guard: a stale-base retry on a window already carrying the + // timeout cap must not terminate (the base was just updated, the next + // round builds fresh). Pinned so the same hoist that would delete the + // API-error exemption above cannot silently delete this one either. + expect( + run(Array(5).fill(TIMEOUT_HEAD), { staleBaseRetry: true }), + ).toMatchObject({ terminal: false }); + // When BOTH breakers would fire, the consecutive one (evaluated first) + // keeps its headline — a terminal round is never overridden. + const bothCapped = run(Array(cap - 1).fill(TIMEOUT_HEAD), { + agentTimeout: 'timeout (3000000ms)', + }); + expect(bothCapped.terminal).toBe(true); + // 'consecutive' alone is satisfied by EITHER branch; assert the + // consecutive breaker's own phrase AND the absence of the timeout one + // — an `if true` mutation on the timeout guard flips the headline and + // must fail here. + expect(bothCapped.headline).toContain( + 'consecutive rounds that failed to push', + ); + expect(bothCapped.headline).not.toContain('time-budget exhaustions'); + // Pin the census greps to the actual emit line: the timeout CAUSE text + // and the breaker's grep needle must stay in lockstep, or the census + // silently counts zero. + expect(reviewAddressReportStep).toContain( + 'CAUSE="ran out of time before finishing (${AGENT_TIMEOUT})"', + ); + expect(reviewAddressReportStep).toContain( + 'TIMEOUT_N="$(grep -c \'AutoFix ran out of time before finishing\' <<< "${PRIOR_HEADS}" || true)"', + ); // The reset detector keys on literal substrings; pin them to the actual // "Push and report" emit lines so a reword breaks this test, not silently // the streak reset in production. From d5ac7a9642026ca6eb94042d3623f315e2967350 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Thu, 30 Jul 2026 10:41:08 +0800 Subject: [PATCH 07/38] fix(autofix): post the takeover engage ack from the command itself (#8043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(autofix): post the takeover engage ack from the command itself The engage ack rode a pull_request:labeled round-trip: takeover-command applies the label, the labeled event routes, and the takeover-ack job posts the confirmation. That event has now been observed to simply not fire twice in one day (#7999 — the author read the silence as failure and removed the label; #8002 — an engaged fork PR with no ack for hours), and fork label events can never ack at all since they carry no secrets: a fork /takeover stayed silent until the next scan picked the PR up (2h41m on #7993). takeover-command now posts the engage ack directly after applying the label — every admission gate has already passed at that point, so 'engaged' is truthful for in-repo and fork PRs alike; the fork variant adds the expectation that the first round comes from the next scheduled scan. The route side suppresses the label-path ack when the label sender is the bot (only the ack: the immediate scan still routes), and the review-scan's existing first-pickup ack dedups against the command's comment and heals it if the post failed. Two more silent paths become audible while here: a /takeover on a stacked (non-main-base) PR now refuses out loud instead of dropping with only a log line, and a /takeover stop on a non-main PR now proceeds to remove the label instead of leaving it stuck. * fix(autofix): ack command-driven releases directly and key the scan grace on the label actor Review follow-up: the engage-side fix left the release side on the fragile round-trip — a loud add next to a mute stop re-creates the exact 'did it work or did the event get lost?' ambiguity this PR set out to remove, now on release. And a fork or non-main release could never ack at all (fork unlabeled events carry no secrets; the route ignores non-main releases). - takeover-command now posts the release ack directly after removing the label, mirroring the engage side, with the same three variants as the ack job (plain release / bot-authored / bot-authored+skip) chosen from the same PR_INFO the gates used. The route suppresses the unlabeled-path ack when the label sender is the bot. - The scan's first-pickup grace is keyed on the label event's actor: a bot-applied label defers only 45s (the command's own write is seconds behind the label — fork or in-repo alike), so the immediate scan still heals a failed command ack instead of slipping to the next scheduled tick, and an ic.json snapshot taken between the label write and the command ack cannot double-post. A human in-repo label keeps the 3-minute ack-job grace; a human fork still posts right away. - The fork note now says 'usually within minutes', matching the */10 cron instead of contradicting it by 6x. - Tests kill the two surviving mutations from review: the EN/ZH fork-note swap (per-language-half assertions) and the warning fallback downgraded to '|| true' (both fallback strings pinned). * test(autofix): behaviorally pin the LAST_LABELED_BY jq extraction (#8043) * fix(autofix): correct stale comments about scan heal timing and command blast radius (#8043) * test(autofix): pin release-ack body identity across the command and ack jobs (#8043) --------- Co-authored-by: verify Co-authored-by: Qwen Code Autofix --- .github/workflows/qwen-autofix.yml | 123 ++++++++++-- scripts/tests/qwen-autofix-workflow.test.js | 206 ++++++++++++++++++-- 2 files changed, 301 insertions(+), 28 deletions(-) diff --git a/.github/workflows/qwen-autofix.yml b/.github/workflows/qwen-autofix.yml index bea3b0d40f..344bd4c46d 100644 --- a/.github/workflows/qwen-autofix.yml +++ b/.github/workflows/qwen-autofix.yml @@ -414,8 +414,8 @@ jobs: # applies TAKEOVER_LABEL, 'TAKEOVER_COMMAND stop' removes it — # nothing else. The label stays the single source of truth: # engagement and release happen ONLY via the label events - # below, so the command's whole blast radius is one label - # toggle. Exact match on the trimmed body (constants, never + # below; the command also posts acks directly in both + # directions (#7999, #8002). Exact match on the trimmed body (constants, never # user-input parsing); allowed senders: the PR author (who may # lack label access) or a write+ collaborator. This immediately # narrows a previously fully-closed surface reopened under @@ -528,7 +528,19 @@ jobs: else DO_REVIEW=true ROUTE_PR="$(sanitize_number "${PR_NUMBER_EVENT}")" - TAKEOVER_ACK='engaged' + if [[ "${SENDER_LOGIN}" == "${AUTOFIX_BOT}" ]]; then + # The bot only applies this label from takeover-command, + # which posts the engage ack ITSELF: the labeled event + # has been observed to simply not fire (#7999 — the + # author read the silence as failure and removed the + # label; #8002), so the user-visible ack must not + # depend on this round-trip. Suppress only the ack — + # the immediate scan is this event's real work and + # still routes. + echo "🧭 engage ack skipped: label applied by ${AUTOFIX_BOT} — the command path already acked" + else + TAKEOVER_ACK='engaged' + fi echo "🧭 ${TAKEOVER_LABEL} applied by ${SENDER_LOGIN} on PR #${PR_NUMBER_EVENT} → review phase (takeover)" fi elif [[ "${EVENT_ACTION}" == 'unlabeled' ]]; then @@ -543,6 +555,12 @@ jobs: # fail its identity check — a red run for a label that # never engaged anything. Log and stop. echo "🧭 takeover release ignored: PR is a fork (${PR_HEAD_REPO} != ${REPO})" + elif [[ "${SENDER_LOGIN}" == "${AUTOFIX_BOT}" ]]; then + # Mirror of the labeled-path suppression: the bot only + # removes this label from takeover-command, which posts + # the release ack itself — acking here too would + # double-post on every command-driven stop. + echo "🧭 release ack skipped: label removed by ${AUTOFIX_BOT} — the command path already acked" else TAKEOVER_ACK='released' echo "🧭 ${TAKEOVER_LABEL} removed from PR #${PR_NUMBER_EVENT} by ${SENDER_LOGIN} → released" @@ -1397,10 +1415,27 @@ jobs: # State/base can change while this job sits in its per-PR queue — # re-verify what the route checked so a stale command cannot label # a closed or non-main PR. - if [[ "$(jq -r '.state // ""' <<< "${PR_INFO}")" != "OPEN" || "$(jq -r '.baseRefName // ""' <<< "${PR_INFO}")" != "main" ]]; then - echo "🧭 takeover command dropped: PR #${PR} is no longer an open main-targeting PR" + if [[ "$(jq -r '.state // ""' <<< "${PR_INFO}")" != "OPEN" ]]; then + echo "🧭 takeover command dropped: PR #${PR} is no longer an open PR" exit 0 fi + CMD_BASE_REF="$(jq -r '.baseRefName // ""' <<< "${PR_INFO}")" + if [[ "${CMD_BASE_REF}" != "main" ]]; then + if [[ "${CMD}" == 'add' ]]; then + # Refuse OUT LOUD — the silent drop made a /takeover on a + # stacked PR indistinguishable from a lost event. Mirrors the + # label path's base-refused ack, except no label was applied + # here, so the ask is to re-run the command after retargeting + # (not "the label is left in place"). + gh pr comment "${PR}" --repo "${REPO}" --body "$(printf '🚫 Takeover not engaged: the loop only manages PRs that target `main`, and this one targets `%s`. A stacked PR moves whenever its base branch does, so "new feedback since the last round" and base-conflict resolution are not well defined until the base lands. Retarget this PR to `main` once the base PR merges and re-run `%s` — or take over the base PR instead.\n\n
\n中文说明\n\n🚫 未接管：循环只管理以 `main` 为 base 的 PR，而本 PR 的 base 是 `%s`。堆叠 PR 会随 base 分支移动，因此“自上一轮以来的新反馈”与 base 冲突处理都无法良定义。待 base 的 PR 合入后将本 PR 改为面向 `main` 并重新执行 `%s`；或改为接管 base 那个 PR。\n\n
\n\n' "${CMD_BASE_REF}" "${TAKEOVER_COMMAND}" "${CMD_BASE_REF}" "${TAKEOVER_COMMAND}")" + echo "🧭 takeover command refused: PR #${PR} targets '${CMD_BASE_REF}' not 'main'" + exit 0 + fi + # 'stop' proceeds: removing the label from a non-main PR is + # harmless and matches the latest intent — dropping it here left + # a manually-applied label stuck with no command able to remove + # it (the label path's release ack ignores non-main PRs too). + fi # Skip wins over takeover EVERYWHERE — including here: engaging or # re-arming a skip-labeled PR would post an 'engaged' window anchor # for management that the scans deliberately refuse to perform. @@ -1453,6 +1488,25 @@ jobs: else gh pr edit "${PR}" --repo "${REPO}" --add-label "${TAKEOVER_LABEL}" echo "🏷️ applied ${TAKEOVER_LABEL} to #${PR}" + # Ack HERE, not via the pull_request:labeled round-trip: that + # event has been observed to simply not fire (#7999 — the + # author read the silence as failure and removed the label; + # #8002 — no ack for hours), and fork label events could never + # ack at all (they carry no secrets). Every admission gate + # above has already passed, so 'engaged' is truthful for both + # in-repo and fork PRs. The route side suppresses the + # label-path ack when the label sender is the bot, and the + # scan's first-pickup ack dedups against this comment — and + # heals it on the next scan if this post fails, which is why + # a failure here only warns. + FORK_NOTE='' + FORK_NOTE_ZH='' + if [[ "$(jq -r 'if has("isCrossRepository") then .isCrossRepository else true end' <<< "${PR_INFO}")" != "false" ]]; then + FORK_NOTE=' This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes).' + FORK_NOTE_ZH='本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。' + fi + gh pr comment "${PR}" --repo "${REPO}" --body "$(printf '🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached.%s Remove the `%s` label (or comment `%s stop`) to release.\n\n
\n中文说明\n\n🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。%s移除 `%s` 标签（或评论 `%s stop`）即可释放。\n\n
\n\n' "${FORK_NOTE}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}" "${FORK_NOTE_ZH}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}")" \ + || echo "::warning::engage ack comment failed on #${PR}; the scan's first-pickup ack heals it" fi else if [[ "${HAS}" != 'true' ]]; then @@ -1460,13 +1514,38 @@ jobs: else gh pr edit "${PR}" --repo "${REPO}" --remove-label "${TAKEOVER_LABEL}" echo "🏷️ removed ${TAKEOVER_LABEL} from #${PR}" + # Release ack, direct from the command — the exact mirror of + # the engage side above, for the same reason: the unlabeled + # round-trip is the thing we no longer trust, fork unlabeled + # events can never ack (no secrets), and a non-main release + # never even reaches the ack job. A loud add next to a mute + # stop would re-create the "did it work or did the event get + # lost?" ambiguity on the release side. Variant selection + # mirrors the ack job verbatim (live author + skip label from + # the same PR_INFO the gates used); the route side suppresses + # the unlabeled-path ack when the label sender is the bot. + REL_AUTHOR="$(jq -r '.author.login // ""' <<< "${PR_INFO}")" + REL_HAS_SKIP="$(jq -r --arg t "${SKIP_LABEL}" '[.labels[].name] | index($t) != null' <<< "${PR_INFO}")" + if [[ "${REL_AUTHOR}" == "${AUTOFIX_BOT}" && "${REL_HAS_SKIP}" == "true" ]]; then + REL_BODY="$(printf '👋 Takeover mode ended. This bot-authored PR also carries `%s`, which opts it out of standard bot management entirely — nothing will engage it until that label is removed.\n\n
\n中文说明\n\n👋 接管模式结束。本 bot 创建的 PR 同时带有 `%s`，已完全退出常规 bot 管理 —— 移除该标签前不会有任何介入。\n\n
\n\n' "${SKIP_LABEL}" "${SKIP_LABEL}")" + elif [[ "${REL_AUTHOR}" == "${AUTOFIX_BOT}" ]]; then + REL_BODY="$(printf '👋 Takeover mode ended: the raised round cap no longer applies. This is a bot-authored PR, so STANDARD bot management continues under the strict cap (apply `%s` to opt it out entirely). Re-apply `%s` (or comment `%s`) for the raised cap again.\n\n
\n中文说明\n\n👋 接管模式结束：提升的轮次上限不再适用。这是 bot 创建的 PR，常规 bot 管理仍将继续（严格上限；如需完全退出请打 `%s`）。重新打上 `%s` 标签（或评论 `%s`）可恢复提升上限。\n\n
\n\n' "${SKIP_LABEL}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}" "${SKIP_LABEL}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}")" + else + REL_BODY="$(printf '👋 Takeover released: the autofix loop will no longer engage this PR (an in-flight round, if any, completes its bounded work). Re-apply `%s` (or comment `%s`) to re-engage.\n\n
\n中文说明\n\n👋 已释放：autofix 循环不再介入此 PR（在飞的一轮如有，将完成其有界工作）。重新打上 `%s` 标签（或评论 `%s`）即可再次接管。\n\n
\n\n' "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}" "${TAKEOVER_LABEL}" "${TAKEOVER_COMMAND}")" + fi + gh pr comment "${PR}" --repo "${REPO}" --body "${REL_BODY}" \ + || echo "::warning::release ack comment failed on #${PR}" fi fi # =========================================================================== # TAKEOVER ACK — visible confirmation when a maintainer engages or releases - # a PR via the takeover label. Label events are explicit user actions, so - # every toggle acks (no dedup wanted). In-repo PRs only reach this job. + # a PR via the takeover label. Manual label toggles are explicit user + # actions, so every one acks (no dedup wanted). Command-driven toggles are + # acked by takeover-command itself in BOTH directions — the label event has + # been observed to not fire at all (#7999, #8002), so those acks cannot + # depend on this round-trip — and the route suppresses this job for them + # (label sender is the bot). In-repo PRs only reach this job. # =========================================================================== # Re-arm a stranded PR without deleting anything. Recovery previously meant # `gh api -X DELETE` on the bot's own autofix-eval marker comment: raw API @@ -1994,13 +2073,29 @@ jobs: | .created_at] | sort | last // ""' "${WORKDIR}/pr-events.json")" if [[ -z "${LAST_ENGAGE_ACK_TS}" ]]; then NEED_ENGAGE_ACK='true' - # In-repo label events have a DEDICATED ack job — the scan is - # only its healer. Within a short grace after the label lands, - # defer: a concurrent ack job must not be double-posted (that - # shifts the window anchor). A failed ack job is healed by the - # next scan, which is past the grace. Forks have no ack job, - # so no grace applies there. - if [[ "$(jq -r '.isCrossRepository // false' <<< "${PR_META}")" != "true" ]] \ + # Grace windows keyed by WHO owns the missing ack, read from + # the label event's actor (pr-events.json is already here). + # A bot-applied label came from takeover-command, which posts + # the ack itself within seconds — fork or in-repo alike — so + # a SHORT grace covers the write's own latency and an + # ic.json snapshot taken between the label write and the ack + # landing; past it, the command's post failed and the next + # scheduled scan heals it (≤10 min), instead of waiting on + # a label event that may never arrive. A human-applied + # in-repo label is owned by the + # DEDICATED ack job, which needs job-spin-up time — the + # longer grace stands. A human-labeled fork has no other + # owner, so no grace: the scan posts right here. + LAST_LABELED_BY="$(jq -rs --arg lb "${TAKEOVER_LABEL}" ' + add | [.[] | select(.event == "labeled") + | select((.label.name // "") == $lb)] + | sort_by(.created_at) | last | .actor.login // ""' "${WORKDIR}/pr-events.json")" + if [[ "${LAST_LABELED_BY}" == "${AUTOFIX_BOT}" ]]; then + if [[ -n "${LAST_LABELED_TS}" && "${LAST_LABELED_TS}" > "$(date -u -d '45 seconds ago' +%Y-%m-%dT%H:%M:%SZ)" ]]; then + echo "🧭 engage ack deferred for #${PR}: command-applied label <45s ago — the command's own ack is in flight" + NEED_ENGAGE_ACK='false' + fi + elif [[ "$(jq -r '.isCrossRepository // false' <<< "${PR_META}")" != "true" ]] \ && [[ -n "${LAST_LABELED_TS}" && "${LAST_LABELED_TS}" > "$(date -u -d '3 minutes ago' +%Y-%m-%dT%H:%M:%SZ)" ]]; then echo "🧭 engage ack deferred for #${PR}: in-repo label applied <3m ago — the ack job owns it" NEED_ENGAGE_ACK='false' diff --git a/scripts/tests/qwen-autofix-workflow.test.js b/scripts/tests/qwen-autofix-workflow.test.js index 080f4fd192..9edc47bdc1 100644 --- a/scripts/tests/qwen-autofix-workflow.test.js +++ b/scripts/tests/qwen-autofix-workflow.test.js @@ -1960,13 +1960,18 @@ describe('qwen-autofix workflow', () => { // one lost Chinese section against a duplicate elsewhere): engage, // honest bot-PR release, skip-labeled bot-PR release, human-PR // release, re-arm, fork allow-edits refusal, two skip-blocked refusals, - // the non-main base refusal, the cap pause, and the scan-side - // first-pickup engage ack (fork label events carry no secrets, so the - // scan anchors the window itself). + // the label-path non-main base refusal, the command-path non-main base + // refusal, the cap pause, the command-path direct engage ack (the + // labeled event has been observed to not fire — #7999, #8002 — so the + // command acks itself), the command-path direct release acks (all three + // variants, mirroring the ack job — a loud add next to a mute stop + // would re-create the lost-event ambiguity on the release side), and + // the scan-side first-pickup engage ack (fork label events carry no + // secrets, so the scan anchors the window itself). const ackBodies = workflow.match( /printf '[^']*takeover-(?:ack|cap)[^']*'/g, ); - expect(ackBodies).toHaveLength(12); + expect(ackBodies).toHaveLength(17); for (const body of ackBodies) { expect(body).toContain('中文说明'); } @@ -1979,6 +1984,26 @@ describe('qwen-autofix workflow', () => { // continues; only takeover mode (the raised cap) ends. expect(workflow).toContain('Takeover mode ended'); expect(workflow).toContain('STANDARD bot management continues'); + // The three release acks are duplicated VERBATIM between the command + // path (REL_BODY) and the ack job (BODY) — indentation and the variable + // name are the only differences. A wording change must land in BOTH, or + // users see divergent release messages depending on whether they used + // `/takeover stop` or removed the label. No other test guards this + // cross-site identity: pin that each of the three variants appears + // exactly twice and that the two copies are byte-identical. + const releaseBodies = + workflow.match( + /printf '👋[^']*takeover-ack released[^']*'(?: "\$\{[A-Z_]+\}")+/g, + ) ?? []; + expect(releaseBodies).toHaveLength(6); + const releaseCounts = new Map(); + for (const body of releaseBodies) { + releaseCounts.set(body, (releaseCounts.get(body) ?? 0) + 1); + } + expect(releaseCounts.size).toBe(3); + for (const count of releaseCounts.values()) { + expect(count).toBe(2); + } // Commands are serialized per PR — an older /takeover can never land // after a newer /takeover stop read the unlabeled state. expect(workflow).toContain( @@ -2269,9 +2294,65 @@ describe('qwen-autofix workflow', () => { expect(reviewScanJob).toContain( 'DRY-RUN: would post engage ack on #${PR} (window key untouched)', ); - // In-repo first-pickup defers to the label event's DEDICATED ack job - // within a short grace, so a concurrent ack job is never double-posted. + // First-pickup grace is keyed by WHO owns the missing ack (the label + // event's actor): a human in-repo label defers to the DEDICATED ack + // job (3m of job spin-up), a bot-applied label defers only to the + // COMMAND's own in-flight write (45s — fork or in-repo alike); past + // the grace the next scheduled scan heals a failed command ack + // (≤10 min), and an ic.json snapshot taken between + // the label write and the command's ack cannot double-post. expect(reviewScanJob).toContain('engage ack deferred for #${PR}'); + // Behaviorally pin the LAST_LABELED_BY jq extraction — the load-bearing + // input to the actor-keyed grace — mirroring the labeledTs treatment + // above: regex-extract the program, exec it against a multi-event + // fixture, and assert the returned actor. A presence check alone + // survives any mutation to the jq body (.actor.login → .user.login, + // wrong source file, sort_by on the wrong field). + const labeledByProgram = reviewScanJob + .match( + /LAST_LABELED_BY="\$\(jq -rs --arg lb "\$\{TAKEOVER_LABEL\}" '([\s\S]*?)' "\$\{WORKDIR\}\/pr-events\.json"\)"/, + )?.[1] + ?.replace(/\n {18}/g, '\n'); + expect(labeledByProgram).toBeTruthy(); + const labeledBy = execFileSync( + 'jq', + ['-rs', '--arg', 'lb', 'autofix/takeover', labeledByProgram], + { + encoding: 'utf8', + input: + JSON.stringify([ + { + event: 'labeled', + label: { name: 'autofix/takeover' }, + actor: { login: 'wenshao' }, + created_at: '2026-07-02T00:00:00Z', + }, + { + event: 'labeled', + label: { name: 'other' }, + actor: { login: 'mallory' }, + created_at: '2026-07-09T00:00:00Z', + }, + ]) + + JSON.stringify([ + { + event: 'unlabeled', + label: { name: 'autofix/takeover' }, + actor: { login: 'qwen-code-dev-bot' }, + created_at: '2026-07-08T00:00:00Z', + }, + { + event: 'labeled', + label: { name: 'autofix/takeover' }, + actor: { login: 'qwen-code-dev-bot' }, + created_at: '2026-07-06T00:00:00Z', + }, + ]), + }, + ).trim(); + expect(labeledBy).toBe('qwen-code-dev-bot'); + expect(reviewScanJob).toContain('command-applied label <45s ago'); + expect(reviewScanJob).toContain(`date -u -d '45 seconds ago'`); // A fork fetch failure (force-push/rename race) discards gracefully // instead of a red run, and a fork moved since the scan is discarded at // the live re-check rather than fetched/pushed at the stale path. @@ -2322,8 +2403,12 @@ describe('qwen-autofix workflow', () => { 'cap notice skipped: consent changed since the snapshot', ); // The queued toggle re-verifies state and base, and author privilege is - // LIVE (triage+ today), never durable authorship alone. - expect(workflow).toContain('no longer an open main-targeting PR'); + // LIVE (triage+ today), never durable authorship alone. A closed PR + // drops silently; a non-main base refuses out loud (engage side only). + expect(workflow).toContain('no longer an open PR'); + expect(workflow).toContain( + `takeover command refused: PR #\${PR} targets '\${CMD_BASE_REF}' not 'main'`, + ); expect(routeStep).toContain('admin|maintain|write|triage)'); expect(reviewScanJob).toContain('"${ROUND}" -ge "${EFF_MAX_ROUNDS}"'); // The effective cap travels in the matrix target and SHADOWS the @@ -2422,13 +2507,14 @@ describe('qwen-autofix workflow', () => { authorPerm = 'write', state = 'OPEN', base = 'main', + author = 'fork-owner', }) => { const dir = mkdtempSync(join(tmpdir(), 'autofix-toggle-')); try { const prJson = JSON.stringify({ isCrossRepository: fork, maintainerCanModify: canModify, - author: { login: 'fork-owner' }, + author: { login: author }, state, baseRefName: base, labels: labels.map((name) => ({ name })), @@ -2440,7 +2526,7 @@ describe('qwen-autofix workflow', () => { `if [[ "$1" == "api" && "$2" == */collaborators/*/permission ]]; then printf '%s' '${authorPerm}';`, `elif [[ "$1" == "pr" && "$2" == "view" ]]; then printf '%s' '${prJson}';`, `elif [[ "$1" == "pr" && "$2" == "edit" ]]; then echo "EDIT $*" >> '${join(dir, 'writes.log')}';`, - `elif [[ "$1" == "pr" && "$2" == "comment" ]]; then echo "COMMENT $4" >> '${join(dir, 'writes.log')}'; cat > /dev/null <<< "$6";`, + `elif [[ "$1" == "pr" && "$2" == "comment" ]]; then echo "COMMENT $*" >> '${join(dir, 'writes.log')}';`, 'fi', ].join('\n'), ); @@ -2459,6 +2545,7 @@ describe('qwen-autofix workflow', () => { TAKEOVER_LABEL: 'autofix/takeover', SKIP_LABEL: 'autofix/skip', TAKEOVER_COMMAND: '@qwen-code /takeover', + AUTOFIX_BOT: 'qwen-code-dev-bot', GITHUB_TOKEN: 'x', }, encoding: 'utf8', @@ -2473,11 +2560,16 @@ describe('qwen-autofix workflow', () => { rmSync(dir, { recursive: true, force: true }); } }; - // add + absent → label applied, no ack from this job. + // add + absent → label applied AND the engage ack posted directly from + // this job: the labeled event has been observed to not fire at all + // (#7999, #8002), so the user-visible ack cannot depend on that + // round-trip. In-repo PRs get no fork note. const addAbsent = runToggle({ cmd: 'add' }); expect(addAbsent.writes).toContain('EDIT pr edit 7165'); expect(addAbsent.writes).toContain('--add-label'); - expect(addAbsent.writes).not.toContain('COMMENT'); + expect(addAbsent.writes).toContain(''); + expect(addAbsent.writes).not.toContain('next scheduled scan'); + expect(addAbsent.writes).not.toContain('定时扫描'); // add + present → re-arm ack, label untouched. const rearm = runToggle({ cmd: 'add', labels: ['autofix/takeover'] }); expect(rearm.writes).toContain('COMMENT'); @@ -2489,6 +2581,28 @@ describe('qwen-autofix workflow', () => { labels: ['autofix/takeover'], }); expect(removePresent.writes).toContain('--remove-label'); + // Release acks directly too — the exact mirror of the engage side: a + // loud add next to a mute stop re-creates the lost-event ambiguity on + // the release side (and fork/non-main releases have no other ack path + // at all). Human-authored PR → the plain released variant. + expect(removePresent.writes).toContain(''); + expect(removePresent.writes).toContain('Takeover released'); + // Variant selection mirrors the ack job: bot-authored → standard + // management continues; bot-authored + skip → fully opted out. + const botRelease = runToggle({ + cmd: 'remove', + labels: ['autofix/takeover'], + author: 'qwen-code-dev-bot', + }); + expect(botRelease.writes).toContain('STANDARD bot management continues'); + const botSkipRelease = runToggle({ + cmd: 'remove', + labels: ['autofix/takeover', 'autofix/skip'], + author: 'qwen-code-dev-bot', + }); + expect(botSkipRelease.writes).toContain( + 'opts it out of standard bot management entirely', + ); // remove + absent → explicit no-op, no writes at all. const removeAbsent = runToggle({ cmd: 'remove' }); expect(removeAbsent.writes.trim()).toBe(''); @@ -2504,7 +2618,21 @@ describe('qwen-autofix workflow', () => { expect(forkRefused.writes).not.toContain('EDIT'); const forkManaged = runToggle({ cmd: 'add', fork: true }); expect(forkManaged.writes).toContain('--add-label'); - expect(forkManaged.writes).not.toContain('COMMENT'); + // Fork label events carry no secrets, so no other job could ever ack a + // fork engage — the command's own ack is the ONLY one, and it sets the + // expectation that the first round comes from the next scheduled scan. + // Assert each language's note in ITS OWN half of the body: a mutation + // swapping the EN/ZH printf args ships the Chinese sentence inside the + // English paragraph (and vice versa) while a whole-body toContain + // still passes. + expect(forkManaged.writes).toContain(''); + const [forkEn, forkZh] = forkManaged.writes.split( + '中文说明', + ); + expect(forkEn).toContain('next scheduled scan (usually within minutes)'); + expect(forkEn).not.toContain('定时扫描'); + expect(forkZh).toContain('通常几分钟内'); + expect(forkZh).not.toContain('next scheduled scan'); // A below-write fork author would be a ghost engagement (label sticks, // nothing ever manages it) — the command refuses with the adoption ask. const forkGhost = runToggle({ cmd: 'add', fork: true, authorPerm: 'read' }); @@ -2520,6 +2648,40 @@ describe('qwen-autofix workflow', () => { labels: ['autofix/takeover'], }); expect(forkStop.writes).toContain('--remove-label'); + // Fork unlabeled events carry no secrets, so this is the ONLY possible + // release ack for a fork — it must post here. + expect(forkStop.writes).toContain(''); + // A /takeover on a stacked PR refuses OUT LOUD (the silent drop made it + // indistinguishable from a lost event) and never applies the label. + const stacked = runToggle({ cmd: 'add', base: 'feat/base-pr' }); + expect(stacked.writes).toContain(''); + expect(stacked.writes).toContain('`feat/base-pr`'); + expect(stacked.writes).not.toContain('EDIT'); + // …but a stop on a non-main PR PROCEEDS: removing a stuck label is + // harmless and matches the latest intent (previously dropped, leaving a + // manually-applied label with no command able to remove it). + const stackedStop = runToggle({ + cmd: 'remove', + base: 'feat/base-pr', + labels: ['autofix/takeover'], + }); + expect(stackedStop.writes).toContain('--remove-label'); + // A non-main release never reaches the ack job (route ignores it), so + // the command's own ack is the only voice here too. + expect(stackedStop.writes).toContain(''); + // A closed PR still drops silently for both directions. + const closed = runToggle({ cmd: 'add', state: 'CLOSED' }); + expect(closed.writes.trim()).toBe(''); + expect(closed.log).toContain('no longer an open PR'); + // Both ack posts keep their non-fatal fallback: under bash -e a failed + // gh pr comment would otherwise abort the step RED after the label was + // already toggled — a worse signal than the silence being fixed. A + // mutation to `|| true` must not survive either: the warning is what + // makes the failure diagnosable. + expect(workflow).toContain( + `engage ack comment failed on #\${PR}; the scan's first-pickup ack heals it`, + ); + expect(workflow).toContain('release ack comment failed on #${PR}'); }); it('behaviorally resets round counting at the latest takeover engage ack', () => { @@ -3058,6 +3220,7 @@ describe('qwen-autofix workflow', () => { action = 'labeled', headRepo = 'QwenLM/qwen-code', label = 'autofix/takeover', + sender = 'wenshao', }) => // The block also logs its reasoning; only the trailing summary is asserted. execFileSync( @@ -3085,7 +3248,8 @@ describe('qwen-autofix workflow', () => { PR_STATE: state, PR_BASE_REF: base, PR_NUMBER_EVENT: '7368', - SENDER_LOGIN: 'wenshao', + SENDER_LOGIN: sender, + AUTOFIX_BOT: 'qwen-code-dev-bot', }, encoding: 'utf8', }, @@ -3102,6 +3266,11 @@ describe('qwen-autofix workflow', () => { // Unchanged: a main-targeting in-repo PR still engages, and engagement // carries no base (the field exists only to name a refusal). expect(run({})).toBe('ack=engaged base= review=true'); + // A label applied BY THE BOT came from takeover-command, which posts the + // engage ack itself (the labeled event has been observed to not fire — + // #7999, #8002 — so the ack cannot depend on this round-trip). Only the + // ack is suppressed; the immediate scan still routes. + expect(run({ sender: 'qwen-code-dev-bot' })).toBe('ack= base= review=true'); // Still deliberately silent — these were never engaged and a comment on // them would be noise, not information: a closed PR, a fork (whose label // event carries no secrets to comment with), a non-takeover label, and @@ -3111,6 +3280,15 @@ describe('qwen-autofix workflow', () => { 'ack= base= review=false', ); expect(run({ label: 'kind/bug' })).toBe('ack= base= review=false'); + // A label REMOVED by the bot came from a /takeover stop — the command + // posts the release ack itself, mirroring the engage-side suppression. + expect(run({ action: 'unlabeled', sender: 'qwen-code-dev-bot' })).toBe( + 'ack= base= review=false', + ); + // …while a human removing the label still gets the ack-job release ack. + expect(run({ action: 'unlabeled' })).toBe( + 'ack=released base= review=false', + ); expect( run({ action: 'unlabeled', base: 'ci/autofix-gate-crash-retry' }), ).toBe('ack= base= review=false'); From 4558bfa725cac112970168b76b944ba3cc0a7435 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 30 Jul 2026 10:44:54 +0800 Subject: [PATCH 08/38] test(e2e): force delegation in flaky subagent case (#8070) (#8073) The main agent runs in yolo mode with read_file registered, so the model sometimes read the file directly instead of delegating, failing the foundTaskTool assertion. Make the prompt require delegation and forbid direct reads, and make the subagent imperative about calling read_file. Tool restriction (coreTools/excludeTools) is not viable: the subagent shares the session permissionManager, so it would lose read_file too. Co-authored-by: Qwen Code Autofix --- integration-tests/sdk-typescript/subagents.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/integration-tests/sdk-typescript/subagents.test.ts b/integration-tests/sdk-typescript/subagents.test.ts index 96aaef720f..6d78bbf388 100644 --- a/integration-tests/sdk-typescript/subagents.test.ts +++ b/integration-tests/sdk-typescript/subagents.test.ts @@ -250,7 +250,7 @@ describe('Subagents (E2E)', () => { name: 'file-reader', description: 'Reads a requested file and reports its exact contents.', systemPrompt: - 'Use read_file to read the requested file, then report its exact contents.', + 'Use the read_file tool to read the requested file, then report its exact contents. Never answer from memory.', level: 'session', tools: ['read_file'], }; @@ -258,6 +258,7 @@ describe('Subagents (E2E)', () => { const testFile = helper.getPath('test.txt'); const q = query({ prompt: + `Do not read the file yourself; you must delegate this task. ` + `Use the agent tool to ask the file-reader subagent to read ${testFile}. ` + `Return the file contents reported by the subagent.`, options: { From d328dd32999b514bd6ab8a43fc76479debdcc176 Mon Sep 17 00:00:00 2001 From: ytahdn <1294726970@qq.com> Date: Thu, 30 Jul 2026 10:52:35 +0800 Subject: [PATCH 09/38] fix(web-shell): stabilize enhanced table controls (#8041) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web-shell): stabilize enhanced table controls * fix(web-shell): stabilize table detail scrolling * fix(web-shell): preserve fixed table widths * fix(web-shell): address review feedback on enhanced table controls (#8041) * fix(web-shell): remove dead branch and cover mixed column sizing (#8041) * test(web-shell): cover frozen-shadow resize fallback and pin column-width divisor (#8041) * refactor(web-shell): remove cell widths ignored under fixed table layout (#8041) --------- Co-authored-by: 钉萁 Co-authored-by: Qwen Code Autofix Co-authored-by: qwen-code-ci-bot --- .../messages/EnhancedMarkdownTable.module.css | 63 +++- .../messages/EnhancedMarkdownTable.test.tsx | 329 +++++++++++++++--- .../messages/EnhancedMarkdownTable.tsx | 182 ++++++++-- 3 files changed, 495 insertions(+), 79 deletions(-) diff --git a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.module.css b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.module.css index cd31279a01..a21ac613b7 100644 --- a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.module.css +++ b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.module.css @@ -1,5 +1,6 @@ .tableShell { position: relative; + --toolbar-height: 38px; max-width: 100%; margin: 8px 0; overflow: visible; @@ -52,7 +53,7 @@ gap: 8px; align-items: center; box-sizing: border-box; - height: 38px; + height: var(--toolbar-height); padding: 4px 10px; border-bottom: 1px solid var(--border); border-radius: 14px 14px 0 0; @@ -128,6 +129,14 @@ font-size: 12px; } +.densityTrigger[data-state='open'] { + border-color: transparent; + box-shadow: none; + outline: none; + --tw-ring-color: transparent; + --tw-ring-shadow: 0 0 #0000; +} + .densityTrigger svg, .densityMenu svg { width: 14px; @@ -160,6 +169,7 @@ } .scroller { + position: relative; container: table-scroller / inline-size; max-width: 100%; max-height: min(75vh, 720px); @@ -168,23 +178,40 @@ outline: none; } -.scrollerWithDetail { - max-height: none; -} - .scroller:focus-visible { outline: 2px solid var(--agent-blue-500); outline-offset: -2px; } +.frozenColumnShadow { + position: absolute; + top: var(--toolbar-height); + bottom: 0; + z-index: 9; + width: 14px; + pointer-events: none; + transform: translateX(-1px); + background: linear-gradient(90deg, rgb(0 0 0 / 8%), rgb(0 0 0 / 0)); +} + .table { --action-column-width: 40px; - width: max-content; - min-width: 100%; + width: 100%; border-collapse: separate; border-spacing: 0; font-size: 13px; + table-layout: fixed; +} + +.actionColumn { + width: var(--action-column-width); + min-width: var(--action-column-width); + max-width: var(--action-column-width); +} + +.fillerColumn { + width: auto; } .headerCell, @@ -193,6 +220,20 @@ border-bottom: 1px solid var(--border); } +.fillerHeaderCell, +.fillerCell { + padding: 0; + border-bottom: 1px solid var(--border); + background: var(--card); +} + +.fillerHeaderCell { + position: sticky; + top: 0; + z-index: 3; + height: 40px; +} + .headerCell { position: sticky; top: 0; @@ -368,6 +409,10 @@ tr:hover .cell { background: var(--subtle-bg-strong); } +tr:hover .fillerCell { + background: var(--subtle-bg-strong); +} + .selectedCell, tr:hover .selectedCell { background: color-mix(in srgb, var(--agent-blue-500) 20%, transparent); @@ -410,9 +455,7 @@ tr:hover .selectedCell { .hasFrozenColumn .frozenHeaderCell, .hasFrozenColumn .frozenCell { - box-shadow: - 8px 0 14px -12px rgb(0 0 0 / 55%), - inset -1px 0 0 var(--border, rgb(255 255 255 / 18%)); + box-shadow: inset -1px 0 0 var(--border, rgb(255 255 255 / 18%)); } .stickyActionHeaderCell, diff --git a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.test.tsx b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.test.tsx index bab1bd84cd..a44397f03f 100644 --- a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.test.tsx +++ b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.test.tsx @@ -321,6 +321,17 @@ function dataCell( return cell!; } +function columnGroup( + container: HTMLElement, + visibleColumnIndex: number, +): HTMLColElement { + const col = [...container.querySelectorAll('col')].slice(1)[ + visibleColumnIndex + ]; + expect(col).toBeDefined(); + return col!; +} + function dragCells(from: Element, to: Element): void { act(() => { from.dispatchEvent( @@ -1213,10 +1224,7 @@ describe('EnhancedMarkdownTable', () => { window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '220px', - ); - expect(dataCell(container, 0, 0).style.width).toBe('220px'); + expect(columnGroup(container, 0).style.width).toBe('220px'); }); it('clamps resized columns to the minimum width', () => { @@ -1239,10 +1247,7 @@ describe('EnhancedMarkdownTable', () => { window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '80px', - ); - expect(dataCell(container, 0, 0).style.width).toBe('80px'); + expect(columnGroup(container, 0).style.width).toBe('80px'); }); it('clamps resized columns to the maximum width', () => { @@ -1265,10 +1270,7 @@ describe('EnhancedMarkdownTable', () => { window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '640px', - ); - expect(dataCell(container, 0, 0).style.width).toBe('640px'); + expect(columnGroup(container, 0).style.width).toBe('640px'); }); it('stops resizing a column when the window blurs', () => { @@ -1293,9 +1295,7 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '160px', - ); + expect(columnGroup(container, 0).style.width).toContain('160px'); }); it('flushes pending resize width when the window blurs', () => { @@ -1318,9 +1318,7 @@ describe('EnhancedMarkdownTable', () => { window.dispatchEvent(new Event('blur')); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '260px', - ); + expect(columnGroup(container, 0).style.width).toBe('260px'); }); it('stops resizing a column when page visibility changes', () => { @@ -1349,9 +1347,7 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '160px', - ); + expect(columnGroup(container, 0).style.width).toContain('160px'); }); it('keeps resizing when page visibility changes while visible', () => { @@ -1381,9 +1377,7 @@ describe('EnhancedMarkdownTable', () => { window.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '280px', - ); + expect(columnGroup(container, 0).style.width).toBe('280px'); }); it('resizes a column with keyboard arrows', () => { @@ -1399,9 +1393,7 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '176px', - ); + expect(columnGroup(container, 0).style.width).toBe('176px'); act(() => { resize.dispatchEvent( @@ -1412,9 +1404,92 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '160px', + expect(columnGroup(container, 0).style.width).toContain('160px'); + }); + + it('uses a filler column after every visible column is manually sized', () => { + const container = renderTable(); + const table = container.querySelector('table'); + expect(table).not.toBeNull(); + expect(container.querySelector('[class*="fillerColumn"]')).toBeNull(); + + for (const column of ['Team', 'Score']) { + act(() => { + button(container, `Resize ${column}`).dispatchEvent( + new KeyboardEvent('keydown', { + bubbles: true, + key: 'ArrowRight', + }), + ); + }); + } + + const columns = Array.from(table!.querySelectorAll('col')); + expect(columns).toHaveLength(4); + expect(columns[0]?.style.width).toBe('40px'); + expect(columns[1]?.style.width).toBe('176px'); + expect(columns[2]?.style.width).toBe('176px'); + expect(columns[3]?.className).toContain('fillerColumn'); + expect(table!.querySelector('thead th:last-child')?.className).toContain( + 'fillerHeaderCell', ); + expect(table!.querySelector('tbody td:last-child')?.className).toContain( + 'fillerCell', + ); + }); + + it('subtracts fixed widths from the flexible column calc()', () => { + const container = renderTable(); + + const initialColumns = Array.from(container.querySelectorAll('col')); + // Two flexible columns share the remaining width, so the divisor is 2 + // (jsdom serializes `/ 2` as `0.5 *`); a `/ 1` mutant would claim the + // full width and fail this assertion. + expect(initialColumns[1]?.style.width).toContain( + '0.5 * (100cqw - 40px - 0px)', + ); + expect(initialColumns[2]?.style.width).toContain( + '0.5 * (100cqw - 40px - 0px)', + ); + + act(() => { + button(container, 'Resize Team').dispatchEvent( + new KeyboardEvent('keydown', { + bubbles: true, + key: 'ArrowRight', + }), + ); + }); + + const columns = Array.from(container.querySelectorAll('col')); + expect(columns).toHaveLength(3); + expect(columns[1]?.style.width).toBe('176px'); + expect(columns[2]?.style.width).toContain('1 * (100cqw - 40px - 176px)'); + expect(container.querySelector('[class*="fillerColumn"]')).toBeNull(); + }); + + it('spans the detail row across the filler column', () => { + const container = renderTable(); + + for (const column of ['Team', 'Score']) { + act(() => { + button(container, `Resize ${column}`).dispatchEvent( + new KeyboardEvent('keydown', { + bubbles: true, + key: 'ArrowRight', + }), + ); + }); + } + expect(container.querySelector('[class*="fillerColumn"]')).not.toBeNull(); + + click(button(container, 'View details for row 1')); + + const detailCell = container.querySelector( + '[class*="detailCell"]', + ); + expect(detailCell).not.toBeNull(); + expect(detailCell!.colSpan).toBe(4); }); it('resizes compact auto columns from their rendered width with keyboard arrows', () => { @@ -1437,7 +1512,7 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(header?.style.width).toBe('108px'); + expect(columnGroup(container, 0).style.width).toBe('108px'); }); it('ignores keyboard resize arrows with modifiers', () => { @@ -1454,9 +1529,7 @@ describe('EnhancedMarkdownTable', () => { ); }); - expect(button(container, 'Sort by Team').closest('th')?.style.width).toBe( - '160px', - ); + expect(columnGroup(container, 0).style.width).toContain('160px'); }); it('reorders columns and quick copies in the visible order', () => { @@ -1587,6 +1660,145 @@ describe('EnhancedMarkdownTable', () => { ).not.toContain('frozenHeaderCell'); }); + it('positions the frozen shadow from the rendered column edge', () => { + const container = renderWideTable(); + const shell = container.querySelector('[class*="tableShell"]'); + const header = button(container, 'Sort by Team').closest('th'); + expect(shell).not.toBeNull(); + expect(header).not.toBeNull(); + Object.defineProperty(shell, 'clientLeft', { + configurable: true, + value: 1, + }); + Object.defineProperty(shell, 'getBoundingClientRect', { + configurable: true, + value: () => ({ left: 20 }) as DOMRect, + }); + Object.defineProperty(header, 'getBoundingClientRect', { + configurable: true, + value: () => ({ right: 301 }) as DOMRect, + }); + + freezeFirstColumn(container); + + expect( + container.querySelector('[class*="frozenColumnShadow"]') + ?.style.left, + ).toBe('280px'); + }); + + it('repositions the frozen shadow when the ResizeObserver fires', () => { + const callbacks: ResizeObserverCallback[] = []; + const OriginalResizeObserver = globalThis.ResizeObserver; + class CapturingResizeObserver { + constructor(private readonly callback: ResizeObserverCallback) { + callbacks.push(callback); + } + observe() {} + unobserve() {} + disconnect() {} + } + (globalThis as { ResizeObserver?: unknown }).ResizeObserver = + CapturingResizeObserver; + + try { + const container = renderWideTable(); + const shell = container.querySelector( + '[class*="tableShell"]', + ); + const header = button(container, 'Sort by Team').closest('th'); + expect(shell).not.toBeNull(); + expect(header).not.toBeNull(); + Object.defineProperty(shell, 'clientLeft', { + configurable: true, + value: 1, + }); + Object.defineProperty(shell, 'getBoundingClientRect', { + configurable: true, + value: () => ({ left: 20 }) as DOMRect, + }); + Object.defineProperty(header, 'getBoundingClientRect', { + configurable: true, + value: () => ({ right: 301 }) as DOMRect, + }); + + freezeFirstColumn(container); + + expect( + container.querySelector('[class*="frozenColumnShadow"]') + ?.style.left, + ).toBe('280px'); + + Object.defineProperty(header, 'getBoundingClientRect', { + configurable: true, + value: () => ({ right: 351 }) as DOMRect, + }); + act(() => { + for (const cb of callbacks) { + cb([], {} as ResizeObserver); + } + }); + + expect( + container.querySelector('[class*="frozenColumnShadow"]') + ?.style.left, + ).toBe('330px'); + } finally { + (globalThis as { ResizeObserver?: unknown }).ResizeObserver = + OriginalResizeObserver; + } + }); + + it('repositions the frozen shadow on window resize without ResizeObserver', () => { + const OriginalResizeObserver = globalThis.ResizeObserver; + // @ts-expect-error -- force the window-resize fallback branch + delete globalThis.ResizeObserver; + + try { + const container = renderWideTable(); + const shell = container.querySelector( + '[class*="tableShell"]', + ); + const header = button(container, 'Sort by Team').closest('th'); + expect(shell).not.toBeNull(); + expect(header).not.toBeNull(); + Object.defineProperty(shell, 'clientLeft', { + configurable: true, + value: 1, + }); + Object.defineProperty(shell, 'getBoundingClientRect', { + configurable: true, + value: () => ({ left: 20 }) as DOMRect, + }); + Object.defineProperty(header, 'getBoundingClientRect', { + configurable: true, + value: () => ({ right: 301 }) as DOMRect, + }); + + freezeFirstColumn(container); + + expect( + container.querySelector('[class*="frozenColumnShadow"]') + ?.style.left, + ).toBe('280px'); + + Object.defineProperty(header, 'getBoundingClientRect', { + configurable: true, + value: () => ({ right: 351 }) as DOMRect, + }); + act(() => { + window.dispatchEvent(new Event('resize')); + }); + + expect( + container.querySelector('[class*="frozenColumnShadow"]') + ?.style.left, + ).toBe('330px'); + } finally { + globalThis.ResizeObserver = OriginalResizeObserver; + } + }); + it('dismisses the first-column context menu without clearing the active column', () => { const container = renderWideTable(); @@ -1920,28 +2132,30 @@ describe('EnhancedMarkdownTable', () => { it('selects display density from the toolbar', () => { const container = renderTable(); const shell = container.querySelector('[class*="tableShell"]'); - const teamHeader = button(container, 'Sort by Team').closest('th'); + const columns = () => Array.from(container.querySelectorAll('col')); expect(shell?.className).toContain('densityStandard'); expect(button(container, 'Table density').textContent).toContain( 'Standard density', ); - expect(teamHeader?.style.width).toBe('160px'); + expect(columns()[0]?.style.width).toBe('40px'); + expect(columns()[1]?.style.width).toContain('160px'); selectValue(button(container, 'Table density'), 'compact'); expect(shell?.className).toContain('densityCompact'); expect(button(container, 'Table density').textContent).toContain( 'Compact density', ); - expect(teamHeader?.style.width).toBe('auto'); - expect(teamHeader?.style.minWidth).toBe(''); - expect(teamHeader?.style.maxWidth).toBe(''); + expect(columns()[0]?.style.width).toBe('40px'); + expect(columns()[1]?.style.width).toContain('72px'); + expect(columns()[1]?.style.width).not.toContain('160px'); selectValue(button(container, 'Table density'), 'comfortable'); expect(shell?.className).toContain('densityComfortable'); expect(button(container, 'Table density').textContent).toContain( 'Comfortable density', ); - expect(teamHeader?.style.width).toBe('160px'); + expect(columns()[0]?.style.width).toBe('40px'); + expect(columns()[1]?.style.width).toContain('160px'); }); it('renders compact row details with blank values and globally expandable long values', () => { @@ -1972,6 +2186,43 @@ describe('EnhancedMarkdownTable', () => { expect(textButton(container, 'Collapse text')).toBeDefined(); }); + it('keeps the opened row anchored when the table scroller expands', () => { + const container = renderTable(); + container.style.overflowY = 'auto'; + container.scrollTop = 40; + const detailsButton = button(container, 'View details for row 3'); + const row = detailsButton.closest('tr'); + expect(row).not.toBeNull(); + const rowRect = vi + .spyOn(row!, 'getBoundingClientRect') + .mockReturnValueOnce({ top: 120 } as DOMRect) + .mockReturnValueOnce({ top: 280 } as DOMRect); + + click(detailsButton); + + expect(rowRect).toHaveBeenCalledTimes(2); + expect(container.scrollTop).toBe(200); + expect(detailsButton.getAttribute('aria-expanded')).toBe('true'); + }); + + it('falls back to window.scrollBy when no scroll ancestor exists', () => { + const container = renderTable(); + const scrollBySpy = vi + .spyOn(window, 'scrollBy') + .mockImplementation(() => {}); + const detailsButton = button(container, 'View details for row 3'); + const row = detailsButton.closest('tr'); + expect(row).not.toBeNull(); + vi.spyOn(row!, 'getBoundingClientRect') + .mockReturnValueOnce({ top: 120 } as DOMRect) + .mockReturnValueOnce({ top: 280 } as DOMRect); + + click(detailsButton); + + expect(scrollBySpy).toHaveBeenCalledWith(0, 160); + expect(detailsButton.getAttribute('aria-expanded')).toBe('true'); + }); + it('shows statistics for a numeric selection', () => { const container = renderTable(); diff --git a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.tsx b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.tsx index 24caf6f2d2..1428ef3a68 100644 --- a/packages/web-shell/client/components/messages/EnhancedMarkdownTable.tsx +++ b/packages/web-shell/client/components/messages/EnhancedMarkdownTable.tsx @@ -5,6 +5,7 @@ import { useCallback, useEffect, useId, + useLayoutEffect, useMemo, useRef, useState, @@ -181,7 +182,10 @@ const NUMBER_FILTER_LABEL_KEYS: Record = { export const MAX_ENHANCED_TABLE_ROWS = 500; export const MAX_ENHANCED_TABLE_COLUMNS = 50; +// Must stay in sync with --action-column-width in EnhancedMarkdownTable.module.css +const ACTION_COLUMN_WIDTH = 40; const DEFAULT_COLUMN_WIDTH = 160; +const COMPACT_COLUMN_WIDTH = 72; const MIN_COLUMN_WIDTH = 80; const MAX_COLUMN_WIDTH = 640; const KEYBOARD_COLUMN_RESIZE_STEP = 16; @@ -189,13 +193,10 @@ const COLUMN_DRAG_MIME = 'application/x-qwen-web-shell-table-column'; const LONG_CELL_TEXT_LENGTH = 60; const LONG_CELL_LINE_COUNT = 3; const DENSITY_OPTIONS: TableDensity[] = ['standard', 'compact', 'comfortable']; -const DEFAULT_COLUMN_STYLE: CSSProperties = { - width: DEFAULT_COLUMN_WIDTH, - minWidth: DEFAULT_COLUMN_WIDTH, - maxWidth: DEFAULT_COLUMN_WIDTH, -}; -const COMPACT_AUTO_COLUMN_STYLE: CSSProperties = { - width: 'auto', +const ACTION_COLUMN_STYLE: CSSProperties = { + width: ACTION_COLUMN_WIDTH, + minWidth: ACTION_COLUMN_WIDTH, + maxWidth: ACTION_COLUMN_WIDTH, }; function clampColumnWidth(width: number): number { @@ -245,6 +246,24 @@ function densityClassName(density: TableDensity): string { } } +function findVerticalScrollContainer( + element: HTMLElement | null, +): HTMLElement | null { + let current = element; + while (current) { + const overflowY = window.getComputedStyle(current).overflowY; + if ( + overflowY === 'auto' || + overflowY === 'scroll' || + overflowY === 'overlay' + ) { + return current; + } + current = current.parentElement; + } + return null; +} + function getTextContent(node: ReactNode): string { if (node === null || node === undefined || typeof node === 'boolean') { return ''; @@ -1468,6 +1487,9 @@ export function EnhancedTable({ const [cellDialog, setCellDialog] = useState(null); const [longTextExpanded, setLongTextExpanded] = useState(false); const [density, setDensity] = useState('standard'); + const [frozenColumnShadowLeft, setFrozenColumnShadowLeft] = useState< + number | null + >(null); const [isDragging, setIsDragging] = useState(false); const [copiedVisible, setCopiedVisible] = useState(false); const [copiedSelection, setCopiedSelection] = useState(false); @@ -1488,6 +1510,12 @@ export function EnhancedTable({ const mountedRef = useRef(true); const shellRef = useRef(null); const containerRef = useRef(null); + const frozenHeaderCellRef = useRef(null); + const detailToggleAnchorRef = useRef<{ + element: HTMLElement; + scrollContainer: HTMLElement | null; + top: number; + } | null>(null); const columnContextMenuRef = useRef(null); const cellDialogRef = useRef(null); const cellDialogFocusReturnRef = useRef(null); @@ -1789,6 +1817,14 @@ export function EnhancedTable({ const frozenColumnIndex = freezeFirstColumn ? orderedVisibleColumnIndexes[0] : undefined; + const fixedVisibleColumnWidth = orderedVisibleColumnIndexes.reduce( + (total, index) => total + (columnWidths[index] ?? 0), + 0, + ); + const flexibleColumnCount = orderedVisibleColumnIndexes.filter( + (index) => columnWidths[index] === undefined, + ).length; + const hasFillerColumn = flexibleColumnCount === 0; const currentCellDialogCell = useMemo(() => { if (!cellDialog) return null; const row = visibleRows.find((item) => item.key === cellDialog.rowKey); @@ -1859,6 +1895,19 @@ export function EnhancedTable({ } }, [cellDialog, detailRowKey, visibleRows]); + useLayoutEffect(() => { + const anchor = detailToggleAnchorRef.current; + detailToggleAnchorRef.current = null; + if (!anchor?.element.isConnected) return; + const offset = anchor.element.getBoundingClientRect().top - anchor.top; + if (offset === 0) return; + if (anchor.scrollContainer) { + anchor.scrollContainer.scrollTop += offset; + } else { + window.scrollBy(0, offset); + } + }, [detailRowKey]); + useEffect(() => { if (!cellDialog) return; return registerInteractionBlocker(); @@ -2017,7 +2066,16 @@ export function EnhancedTable({ }); }; - const toggleRowDetail = (rowKey: string) => { + const toggleRowDetail = (rowKey: string, rowElement: HTMLElement | null) => { + if (detailRowKey !== rowKey && rowElement) { + detailToggleAnchorRef.current = { + element: rowElement, + scrollContainer: findVerticalScrollContainer( + shellRef.current?.parentElement ?? null, + ), + top: rowElement.getBoundingClientRect().top, + }; + } setSelection(null); setCellDialog(null); resetCopiedCellDialog(); @@ -2086,26 +2144,59 @@ export function EnhancedTable({ ); }; - const columnStyle = ( - columnIndex: number, - extra?: CSSProperties, - ): CSSProperties => { + const flexibleColumnWidth = ( + minWidth: number, + fixedWidth: number, + flexibleColumnCount: number, + ): string => + `max(${minWidth}px, calc((100cqw - ${ACTION_COLUMN_WIDTH}px - ${fixedWidth}px) / ${flexibleColumnCount}))`; + + const columnGroupStyle = (columnIndex: number): CSSProperties => { const width = columnWidths[columnIndex]; - if (width === undefined) { - const defaultStyle = - density === 'compact' - ? COMPACT_AUTO_COLUMN_STYLE - : DEFAULT_COLUMN_STYLE; - return extra ? { ...defaultStyle, ...extra } : defaultStyle; - } + if (width !== undefined) return { width }; + const minWidth = + density === 'compact' ? COMPACT_COLUMN_WIDTH : DEFAULT_COLUMN_WIDTH; return { - width, - minWidth: width, - maxWidth: width, - ...extra, + width: flexibleColumnWidth( + minWidth, + fixedVisibleColumnWidth, + flexibleColumnCount, + ), }; }; + useLayoutEffect(() => { + if (!freezeFirstColumn || frozenColumnIndex === undefined) { + setFrozenColumnShadowLeft(null); + return; + } + const shell = shellRef.current; + const frozenHeader = frozenHeaderCellRef.current; + if (!shell || !frozenHeader) return; + const updateShadowPosition = () => { + const shellRect = shell.getBoundingClientRect(); + const headerRect = frozenHeader.getBoundingClientRect(); + setFrozenColumnShadowLeft( + Math.max(0, headerRect.right - shellRect.left - shell.clientLeft), + ); + }; + updateShadowPosition(); + if (typeof ResizeObserver === 'undefined') { + window.addEventListener('resize', updateShadowPosition); + return () => window.removeEventListener('resize', updateShadowPosition); + } + const resizeObserver = new ResizeObserver(updateShadowPosition); + resizeObserver.observe(shell); + resizeObserver.observe(frozenHeader); + return () => resizeObserver.disconnect(); + }, [ + columnWidths, + density, + freezeFirstColumn, + frozenColumnIndex, + orderedVisibleColumnIndexes, + ]); + const startColumnResize = ( event: ReactMouseEvent, columnIndex: number, @@ -2608,13 +2699,21 @@ export function EnhancedTable({
+ + + {orderedVisibleColumnIndexes.map((columnIndex) => ( + + ))} + {hasFillerColumn && } + ); })} + {hasFillerColumn && ( + @@ -2767,7 +2870,12 @@ export function EnhancedTable({ {detailOpen && (
openColumnContextMenu(event, columnIndex) } - style={columnStyle(columnIndex, headerAlignStyle)} + style={headerAlignStyle} title={columnName} >
@@ -2750,6 +2850,9 @@ export function EnhancedTable({
@@ -2879,6 +2994,13 @@ export function EnhancedTable({
)} + {freezeFirstColumn && frozenColumnShadowLeft !== null && ( +