From 8f3f1730cfb6580262f92e709257521d9cb6fe62 Mon Sep 17 00:00:00 2001 From: qqqys Date: Wed, 12 Aug 2026 11:28:58 +0800 Subject: [PATCH 01/12] feat(core): let a workflow agent pin a directory and outlive the default bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps that together keep workflow subagents to short, in-place work. **`agent({workingDir})`.** A script had no way to run an agent inside a directory. `isolation: 'worktree'` is not a substitute: it CREATES a worktree from the current tree and refuses to run when the parent tree is dirty — the opposite of pinning an agent to a directory whose uncommitted state is the point (a review worktree, a checkout a previous step provisioned). `workingDir` is the same contract `AgentTool` already exposes as `working_dir`: an existing, caller-owned worktree that the harness neither creates nor removes. Two details are easy to get wrong and both are covered: - The fast path hands `config` to `AgentHeadless` untouched and cannot honour a rebind, so `workingDir` forces the override path. Left on the fast path it would be dropped in silence and the agent would run in the parent tree — the failure the option exists to prevent. - `canonicalizeAgentOpts` now projects `workingDir`. The same prompt run against two worktrees is two different questions; without the projection a resume that changed only the directory would replay the previous tree's answers as this one's. The validation moves to `agents/worktree-pin.ts`, shared with `AgentTool` rather than duplicated: the path comes from a model either way, and pinning replaces the child's `WorkspaceContext` wholesale, so it must resolve inside the repository and be a registered linked worktree. The caller passes the parameter name so errors say `workingDir` to a script and `working_dir` to a tool call. **Tunable per-subagent bounds.** `max_turns: 50` / `max_time_minutes: 10` were hard-pinned at both dispatch sites with no override, while the three other workflow bounds all have one. A build-and-test agent, or an analysis of a 2 000-line file, exceeds them routinely — and under the GOAL-terminal contract being cut off surfaces as a `null` element, an agent that silently went missing rather than one that visibly failed. Both are now env-tunable and clamped, and the doc comment states how `stallMs`, `max_time_minutes` and the run wall clock differ, since raising one without the others just moves which limit kills the run. **Headless regression test.** A foreground `Workflow` call must complete with no interactive session and no completion channel: `qwen --prompt` has no TUI, no approval bridge and a closed stdin, so anything reaching for interactivity inside the tool or runner would hang on a prompt nobody can answer. The background half was already refused explicitly; this pins the foreground half. Part of #8769. --- .../agents/runtime/workflow-journal.test.ts | 16 ++ .../src/agents/runtime/workflow-journal.ts | 21 ++- .../runtime/workflow-orchestrator.test.ts | 156 ++++++++++++++++++ .../agents/runtime/workflow-orchestrator.ts | 146 +++++++++++++--- .../agents/runtime/workflow-sandbox.test.ts | 44 +++++ .../src/agents/runtime/workflow-sandbox.ts | 41 ++++- packages/core/src/agents/worktree-pin.test.ts | 118 +++++++++++++ packages/core/src/agents/worktree-pin.ts | 142 ++++++++++++++++ packages/core/src/tools/agent/agent.ts | 116 +------------ .../core/src/tools/workflow/workflow.test.ts | 26 +++ packages/core/src/tools/workflow/workflow.ts | 9 +- 11 files changed, 693 insertions(+), 142 deletions(-) create mode 100644 packages/core/src/agents/worktree-pin.test.ts create mode 100644 packages/core/src/agents/worktree-pin.ts diff --git a/packages/core/src/agents/runtime/workflow-journal.test.ts b/packages/core/src/agents/runtime/workflow-journal.test.ts index e38d1d1c3ed..d4991bc8619 100644 --- a/packages/core/src/agents/runtime/workflow-journal.test.ts +++ b/packages/core/src/agents/runtime/workflow-journal.test.ts @@ -30,6 +30,22 @@ describe('canonicalizeAgentOpts', () => { expect(c).toBe(JSON.stringify({ agentType: 'a1', model: 'm1' })); }); + // The same prompt run against two worktrees is two different questions. + // Projecting workingDir away would let a resume that changed only the + // directory replay the previous tree's answers as if they were this one's. + it('keeps workingDir, so a resume cannot hit across directories', () => { + const a = deriveAgentKey('', 'review it', { + workingDir: '.qwen/tmp/review-pr-1', + }); + const b = deriveAgentKey('', 'review it', { + workingDir: '.qwen/tmp/review-pr-2', + }); + expect(canonicalizeAgentOpts({ workingDir: 'wt' })).toBe( + JSON.stringify({ workingDir: 'wt' }), + ); + expect(a).not.toBe(b); + }); + it('sorts object keys deeply so reordered schemas hash the same', () => { const a = canonicalizeAgentOpts({ schema: { type: 'object', properties: { b: 1, a: 2 } }, diff --git a/packages/core/src/agents/runtime/workflow-journal.ts b/packages/core/src/agents/runtime/workflow-journal.ts index 15a250e4c2c..5b1b3fe73fa 100644 --- a/packages/core/src/agents/runtime/workflow-journal.ts +++ b/packages/core/src/agents/runtime/workflow-journal.ts @@ -66,14 +66,25 @@ export interface JournalReplay { /** * Project the dispatch-affecting opts into a stable canonical string. Only - * `schema` / `model` / `isolation` / `agentType` change what the dispatch - * does; `label` / `phase` / `stallMs` are cosmetic or operational and must - * NOT bust the cache. Object keys are sorted recursively so a re-serialized - * schema with reordered keys hashes the same. + * `schema` / `model` / `isolation` / `agentType` / `workingDir` change what + * the dispatch does; `label` / `phase` / `stallMs` are cosmetic or + * operational and must NOT bust the cache. Object keys are sorted recursively + * so a re-serialized schema with reordered keys hashes the same. + * + * `workingDir` is dispatch-affecting for the same reason it exists: the same + * prompt run against two different worktrees is two different questions. Were + * it projected away, a resume that changed only the directory would replay + * the previous tree's answers as if they were this one's. */ export function canonicalizeAgentOpts(opts: WorkflowAgentOpts): string { const projected: Record = {}; - for (const k of ['schema', 'model', 'isolation', 'agentType'] as const) { + for (const k of [ + 'schema', + 'model', + 'isolation', + 'agentType', + 'workingDir', + ] as const) { const v = opts[k]; if (v === undefined || typeof v === 'function') continue; projected[k] = v; diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 238b747d3fd..9ea258ffc2d 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -15,6 +15,12 @@ import { DEFAULT_MAX_AGENTS_PER_RUN, resolveMaxAgentsPerRun, resolveConcurrencyLimit, + resolveSubagentMaxTurns, + resolveSubagentMaxTimeMinutes, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING, + HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING, } from './workflow-orchestrator.js'; import type { Config } from '../../config/config.js'; import { AgentEventType, type AgentEventEmitter } from './agent-events.js'; @@ -124,6 +130,36 @@ vi.mock('../../services/gitWorktreeService.js', async (importOriginal) => { }; }); +// `workingDir` resolution (shared with AgentTool's `working_dir`) is unit +// tested in `agents/worktree-pin.test.ts` against a mocked GitWorktreeService. +// Here it is a seam: these tests assert what the ORCHESTRATOR does with the +// resolver's verdict — routes off the fast path, rebinds the runtime context, +// surfaces the refusal — not whether git considers a directory registered. +const pinStub = vi.hoisted(() => ({ + resolve: { + value: undefined as ((workingDir: string) => Promise) | undefined, + }, + seenLabels: [] as string[], +})); + +vi.mock('../worktree-pin.js', () => ({ + resolveExternalWorktreeDir: async ( + _config: unknown, + workingDir: string, + label = 'working_dir', + ) => { + pinStub.seenLabels.push(label); + return ( + (await pinStub.resolve.value?.(workingDir)) ?? { + path: `/fake/repo/${workingDir}`, + branch: 'pr-7', + slug: workingDir, + repoRoot: '/fake/repo', + } + ); + }, +})); + vi.mock('./agent-headless.js', () => ({ AgentHeadless: { create: async ( @@ -1828,6 +1864,51 @@ describe('WorkflowOrchestrator P2 — parallel() / pipeline() / caps', () => { ).toBe(9999); }); + // A build-and-test agent, or an analysis of a 2 000-line file, exceeds + // 50 turns / 10 minutes routinely — and under GOAL-terminal semantics + // being cut off shows up as a `null` element, i.e. an agent that + // silently went missing. Both bounds are operator-tunable, on the same + // contract as the agent cap. + it('per-subagent bounds default, honor an override, and clamp', () => { + expect(resolveSubagentMaxTurns({})).toBe( + DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, + ); + expect(resolveSubagentMaxTimeMinutes({})).toBe( + DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + ); + expect( + resolveSubagentMaxTurns({ QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS: '120' }), + ).toBe(120); + expect( + resolveSubagentMaxTimeMinutes({ + QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES: '45', + }), + ).toBe(45); + expect( + resolveSubagentMaxTurns({ + QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS: '999999', + }), + ).toBe(HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING); + expect( + resolveSubagentMaxTimeMinutes({ + QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES: '999999', + }), + ).toBe(HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING); + }); + + it('per-subagent bounds reject non-decimal-integer overrides', () => { + for (const raw of ['0', 'abc', '2.5', '0x10', '1e3']) { + expect( + resolveSubagentMaxTurns({ QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS: raw }), + ).toBe(DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS); + expect( + resolveSubagentMaxTimeMinutes({ + QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES: raw, + }), + ).toBe(DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES); + } + }); + it('resolveConcurrencyLimit honors a valid override and clamps the cpu default to [1,16]', () => { expect( resolveConcurrencyLimit({ QWEN_CODE_MAX_WORKFLOW_CONCURRENCY: '4' }), @@ -1895,6 +1976,8 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( const { GitWorktreeService } = await import( '../../services/gitWorktreeService.js' ); + pinStub.resolve.value = undefined; + pinStub.seenLabels.length = 0; worktreeStubs.instances.length = 0; vi.mocked(GitWorktreeService).mockImplementation(() => { const stub = worktreeStubs.makeStub(); @@ -1906,6 +1989,8 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( type StubSubagentCall = { config: { name?: string; model?: string; disallowedTools?: string[] }; runtimeContextSame: boolean; + /** What the subagent's Config answers for "where am I?". */ + runtimeTargetDir?: string; options?: { runConfigOverrides?: unknown }; eventEmitterAttached: boolean; executeAgentId?: string | null; @@ -1977,6 +2062,7 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( const call: StubSubagentCall = { config: subagentConfig, runtimeContextSame: runtimeContext === cfg, + runtimeTargetDir: runtimeContext.getTargetDir(), options: { runConfigOverrides: options?.runConfigOverrides }, eventEmitterAttached: options?.eventEmitter !== undefined, }; @@ -2680,6 +2766,76 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( expect(helper.disposed).toBeGreaterThanOrEqual(1); }); + // ─── workingDir: pin to a caller-owned worktree ───────────────── + + // The fast path hands `config` to AgentHeadless untouched and has no way + // to rebind a directory, so a `workingDir` that reached it would be + // silently dropped and the agent would run in the parent working tree — + // the exact failure the option exists to prevent. + it('workingDir forces the override path even with no agentType/model/schema', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'pinned', terminateMode: 'GOAL' }), + }); + created.length = 0; + + await expect( + createProductionDispatch(helper.config)('hi', { + workingDir: '.qwen/tmp/review-pr-7', + }), + ).resolves.toBe('pinned'); + + // Went through SubagentManager, not the fast path's AgentHeadless.create. + expect(helper.calls).toHaveLength(1); + expect(created).toHaveLength(0); + }); + + it('workingDir rebinds the subagent runtime context to the pinned directory', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'pinned', terminateMode: 'GOAL' }), + }); + await createProductionDispatch(helper.config)('hi', { + workingDir: '.qwen/tmp/review-pr-7', + }); + + // The subagent's Config answers with the pinned directory, not the + // parent's '/fake/repo' — that rebind is what makes its file, shell and + // search tools resolve inside the worktree. + expect(helper.calls[0]!.runtimeTargetDir).toBe( + '/fake/repo/.qwen/tmp/review-pr-7', + ); + expect(helper.calls[0]!.runtimeContextSame).toBe(false); + }); + + // A refused pin must abort the dispatch, not fall through to an agent + // running in the parent tree — the failure mode the pin exists to prevent. + it('workingDir surfaces the resolver refusal and dispatches nothing', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + pinStub.resolve.value = async () => ({ + error: 'workingDir "x" is not a registered linked worktree.', + }); + + await expect( + createProductionDispatch(helper.config)('hi', { + workingDir: 'not-a-worktree', + }), + ).rejects.toThrow( + /agent\(\{workingDir: "not-a-worktree"\}\).*not a registered linked worktree/, + ); + expect(helper.calls).toHaveLength(0); + }); + + // The resolver names the offending parameter in its errors; a workflow + // script never wrote `working_dir`, so it must be told the opt's own name. + it('names the workflow opt, not the tool parameter, when resolving', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'ok', terminateMode: 'GOAL' }), + }); + await createProductionDispatch(helper.config)('hi', { workingDir: 'wt' }); + expect(pinStub.seenLabels).toEqual(['workingDir']); + }); + // ─── isolation:'worktree' provision error branches ────────────── // R2 self-review (test-1, [critical]): each provisionWorkflowWorktree // error branch had no unit test. Coverage came only from the real- diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index 9cac0ae9f2c..5abe1da07d6 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -44,6 +44,7 @@ import { FileDiscoveryService } from '../../services/fileDiscoveryService.js'; import { WorkspaceContext } from '../../utils/workspaceContext.js'; import { SyntheticOutputTool } from '../../tools/syntheticOutput.js'; import { rebuildToolRegistryOnOverride } from '../../tools/agent/agent.js'; +import { resolveExternalWorktreeDir } from '../worktree-pin.js'; import { toModelVisibleSubagentResult } from '../subagent-result.js'; import { SUBAGENT_PLAN_LIFECYCLE_TOOLS } from './subagent-plan-tool-policy.js'; import { runWithAgentContext } from './agent-context.js'; @@ -148,9 +149,90 @@ export function resolveConcurrencyLimit( * Bound the resource ceiling for workflow subagents so a single `agent()` * call cannot loop the model indefinitely. Values mirror conservative * upstream defaults; P5 will refine via `budget` once it exists. + * + * These are floors on *safety*, not statements about how long real work + * takes. A long-running agent — a build-and-test step, an analysis of a + * 2 000-line file, anything that pages through large reads — exceeds 50 + * turns or 10 minutes routinely, and under the GOAL-terminal contract + * hitting either shows up as a `null` element in `parallel()`: an agent that + * silently went missing rather than one that visibly failed. So both are + * operator-tunable via env, on the same pattern as the three other workflow + * bounds (`QWEN_CODE_MAX_WORKFLOW_AGENTS`, `QWEN_CODE_WORKFLOW_STALL_SECONDS`, + * `QWEN_CODE_MAX_WORKFLOW_SECONDS`), each clamped to a hard ceiling. + * + * Three time bounds act on a dispatch and they are NOT redundant: + * - `stallMs` (60s default) — no *progress* for this long ⇒ abort + retry. + * Held while a tool is in flight, so a slow tool is not a stall. + * - `max_time_minutes` (this) — total wall time for ONE attempt, stalled or + * not. Bounds the case the watchdog cannot see (a model that keeps + * emitting progress forever). + * - `QWEN_CODE_MAX_WORKFLOW_SECONDS` (30min default) — the whole run, + * every dispatch together. Raising the per-agent bound without raising + * this one just moves which limit kills the run. */ -const WORKFLOW_SUBAGENT_MAX_TURNS = 50; -const WORKFLOW_SUBAGENT_MAX_TIME_MINUTES = 10; +export const DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS = 50; +export const DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES = 10; +export const WORKFLOW_SUBAGENT_MAX_TURNS_ENV = + 'QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'; +export const WORKFLOW_SUBAGENT_MAX_MINUTES_ENV = + 'QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'; +/** 10× the defaults — generous for a legitimate long agent, still bounded. */ +export const HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING = 500; +export const HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING = 100; + +/** + * Resolve one env-tunable per-subagent bound. Same contract as + * {@link resolveMaxAgentsPerRun}: a non-integer / <1 override is rejected + * with a debug warning and the default is used; an override above the hard + * ceiling is clamped. + */ +function resolveSubagentBound( + envName: string, + defaultValue: number, + ceiling: number, + env: Record, +): number { + const raw = env[envName]; + if (raw === undefined || raw.trim() === '') return defaultValue; + const parsed = parsePositiveIntegerEnv(raw, 0); + if (parsed < 1) { + debugLogger.warn( + `Invalid ${envName}=${JSON.stringify(raw)}, using default (${defaultValue})`, + ); + return defaultValue; + } + if (parsed > ceiling) { + debugLogger.warn( + `${envName}=${parsed} exceeds hard ceiling (${ceiling}); clamping.`, + ); + return ceiling; + } + return parsed; +} + +/** Per-attempt turn ceiling for a workflow subagent. */ +export function resolveSubagentMaxTurns( + env: Record = process.env, +): number { + return resolveSubagentBound( + WORKFLOW_SUBAGENT_MAX_TURNS_ENV, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, + HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING, + env, + ); +} + +/** Per-attempt wall-clock ceiling, in minutes, for a workflow subagent. */ +export function resolveSubagentMaxTimeMinutes( + env: Record = process.env, +): number { + return resolveSubagentBound( + WORKFLOW_SUBAGENT_MAX_MINUTES_ENV, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING, + env, + ); +} /** * disallowedTools mirror the upstream `Tg8` workflow-subagent config. These @@ -433,11 +515,16 @@ async function runSingleDispatch( const workflowAgentId = `workflow-agent-${randomBytes(8).toString('hex')}`; debugLogger.debug(`[workflow] Dispatch ${workflowAgentId}`); + // The fast path hands `config` to AgentHeadless untouched, so it has no + // way to honour a directory rebind — `workingDir` MUST route through the + // override path or it would be silently dropped and the agent would run in + // the parent working tree. if ( opts.agentType === undefined && opts.model === undefined && opts.isolation === undefined && - opts.schema === undefined + opts.schema === undefined && + opts.workingDir === undefined ) { const subagent = await AgentHeadless.create( opts.label ?? 'workflow-agent', @@ -451,8 +538,8 @@ async function runSingleDispatch( // and the loop guards never tripped — combined with the cancellation // bug below, workflows were effectively unkillable. { - max_turns: WORKFLOW_SUBAGENT_MAX_TURNS, - max_time_minutes: WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + max_turns: resolveSubagentMaxTurns(), + max_time_minutes: resolveSubagentMaxTimeMinutes(), }, // T11 (PR #4732 R1): disallow SendMessage / ExitPlanMode to align with // upstream Tg8 — closes the back-channel that would let a subagent @@ -695,10 +782,27 @@ async function runOverridePath( let effectiveContext: Config = config; if (opts.isolation === 'worktree') { worktreeIsolation = await provisionWorkflowWorktree(config); - effectiveContext = createWorktreeConfigOverride( + effectiveContext = createDirScopedConfigOverride( config, worktreeIsolation.path, ); + } else if (typeof opts.workingDir === 'string' && opts.workingDir) { + // Caller-owned worktree: same rebind, no provisioning and no cleanup. + // Validated by AgentTool's own `working_dir` resolver so a script-supplied + // path cannot move the subagent's workspace boundary somewhere the + // equivalent `agent` tool call would have refused — the directory must be + // a registered linked worktree living inside this repository. + const resolved = await resolveExternalWorktreeDir( + config, + opts.workingDir, + 'workingDir', + ); + if ('error' in resolved) { + throw new Error( + `agent({workingDir: ${JSON.stringify(opts.workingDir)}}): ${resolved.error}`, + ); + } + effectiveContext = createDirScopedConfigOverride(config, resolved.path); } // R3 review (wenshao T2/T5 [M1]): named parent-abort listener so the @@ -772,8 +876,8 @@ async function runOverridePath( // own runConfig / maxTurns — these are workflow-level safety bounds, // not subagent-level preferences. P5 will refine via budget. runConfigOverrides: { - max_turns: WORKFLOW_SUBAGENT_MAX_TURNS, - max_time_minutes: WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + max_turns: resolveSubagentMaxTurns(), + max_time_minutes: resolveSubagentMaxTimeMinutes(), }, eventEmitter, }, @@ -1062,20 +1166,22 @@ async function provisionWorkflowWorktree( } /** - * Build a Config wrapper that rebinds every "where am I?" surface to the - * isolated worktree path. `Object.create(base)` keeps prototype lookups - * walking back to the parent for everything else (model config, session - * id, MCP servers), while the own-property overrides shadow the cwd- - * adjacent fields so the subagent's tools (Edit / Write / Read / Glob / - * Grep / Ls / Shell) anchor inside the worktree. + * Build a Config wrapper that rebinds every "where am I?" surface to a + * directory. `Object.create(base)` keeps prototype lookups walking back to + * the parent for everything else (model config, session id, MCP servers), + * while the own-property overrides shadow the cwd-adjacent fields so the + * subagent's tools (Edit / Write / Read / Glob / Grep / Ls / Shell) anchor + * inside it. * - * Mirrors the inline rebind block at agent.ts:2008-2024. Sets BOTH the - * field shape (e.g. `targetDir`) AND the method shape (`getTargetDir`) - * because JS does not promote a getter assignment to a field shadow — - * call sites that read `this.targetDir` directly inside Config methods - * would otherwise still resolve through the prototype to the parent. + * Shared by both directory-scoped dispatch modes — `isolation: 'worktree'`, + * which provisions the directory, and `workingDir`, which is handed one the + * caller already owns. Mirrors the inline rebind block at agent.ts:2008-2024. + * Sets BOTH the field shape (e.g. `targetDir`) AND the method shape + * (`getTargetDir`) because JS does not promote a getter assignment to a field + * shadow — call sites that read `this.targetDir` directly inside Config + * methods would otherwise still resolve through the prototype to the parent. */ -function createWorktreeConfigOverride(base: Config, wtPath: string): Config { +function createDirScopedConfigOverride(base: Config, wtPath: string): Config { // eslint-disable-next-line @typescript-eslint/no-explicit-any const ov: any = Object.create(base); ov.targetDir = wtPath; diff --git a/packages/core/src/agents/runtime/workflow-sandbox.test.ts b/packages/core/src/agents/runtime/workflow-sandbox.test.ts index 3c97dd17c5d..d2bdb73356e 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.test.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.test.ts @@ -1366,6 +1366,50 @@ describe('createWorkflowSandbox security', () => { ); }); + it('agent({workingDir}) is passed through to dispatch', async () => { + const seen: Array<{ prompt: string; opts: unknown }> = []; + const sandbox = createWorkflowSandbox({ + args: undefined, + dispatch: async (prompt, opts) => { + seen.push({ prompt, opts }); + return 'done'; + }, + }); + const result = await sandbox.run( + `return await agent("x", { workingDir: ".qwen/tmp/review-pr-7" });`, + ); + expect(result).toBe('done'); + expect((seen[0].opts as { workingDir?: unknown }).workingDir).toBe( + '.qwen/tmp/review-pr-7', + ); + }); + + it('agent({workingDir}) rejects a non-string', async () => { + const sandbox = createWorkflowSandbox({ + args: undefined, + dispatch: async () => 'ignored', + }); + await expect( + sandbox.run(`return agent("x", { workingDir: 7 });`), + ).rejects.toThrow(/workingDir.*non-empty string/); + }); + + // The two options make opposite claims about who owns the directory's + // lifetime, so the contradiction is named rather than resolved by + // precedence — a script that got a silent winner would believe it was + // isolated when it was pinned, or vice versa. + it('agent({workingDir, isolation}) rejects the combination', async () => { + const sandbox = createWorkflowSandbox({ + args: undefined, + dispatch: async () => 'ignored', + }); + await expect( + sandbox.run( + `return agent("x", { workingDir: "wt", isolation: "worktree" });`, + ), + ).rejects.toThrow(/incompatible options/); + }); + it('agent({isolation:"remote"}) is passed through to dispatch in P3', async () => { const seen: Array<{ prompt: string; opts: unknown }> = []; const sandbox = createWorkflowSandbox({ diff --git a/packages/core/src/agents/runtime/workflow-sandbox.ts b/packages/core/src/agents/runtime/workflow-sandbox.ts index 1be6c9d3eca..31207bc3e7a 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.ts @@ -394,6 +394,23 @@ export interface WorkflowAgentOpts { model?: string; isolation?: 'worktree' | 'remote'; agentType?: string; + /** + * Pin this agent to an EXISTING, caller-owned git worktree of the current + * repository — the same contract as `AgentTool`'s `working_dir`, and + * validated by the same check. The runtime neither creates nor removes the + * directory; it only rebinds the subagent Config's cwd surfaces, so the + * agent's file / shell / search tools resolve inside it. + * + * `isolation: 'worktree'` is not a substitute and the two are mutually + * exclusive: isolation CREATES a worktree from the current tree and refuses + * to run when the parent tree is dirty — which is the opposite of pinning an + * agent to a directory whose uncommitted state is the whole point (a review + * worktree, a scratch checkout a prior step provisioned). + * + * It is a cwd pin, not a filesystem sandbox: an explicit absolute path can + * still reach outside it. + */ + workingDir?: string; /** * P-stall: per-call stall-watchdog timeout in milliseconds. The dispatch * is aborted + retried (up to 3 attempts) after this many ms of no @@ -846,7 +863,7 @@ export function createWorkflowSandbox(opts: SandboxOptions): WorkflowSandbox { // FIX-Round1-T13: throw on any opts key not in the allowlist — catches // typos like { scema: ... } that previously slipped through the // [key:string]: unknown index signature. - const KNOWN_AGENT_OPTS = ['label', 'phase', 'schema', 'model', 'isolation', 'agentType', 'stallMs']; + const KNOWN_AGENT_OPTS = ['label', 'phase', 'schema', 'model', 'isolation', 'agentType', 'stallMs', 'workingDir']; globalThis.agent = vmAsync(function (prompt, agentOpts) { agentOpts = agentOpts || {}; const keys = Object.keys(agentOpts); @@ -876,6 +893,28 @@ export function createWorkflowSandbox(opts: SandboxOptions): WorkflowSandbox { "Known modes are: 'worktree', 'remote'." ); } + // NOTE: this init script is a host-side template literal — no + // backticks anywhere below, in code or comments. + // + // workingDir pins the agent to a worktree the CALLER already owns; + // isolation creates and reaps one. Asking for both is a contradiction + // about who owns the directory's lifetime, so name it here rather than + // silently letting one win. + if (agentOpts.workingDir !== undefined) { + if (typeof agentOpts.workingDir !== 'string' || agentOpts.workingDir.length === 0) { + throw new Error( + "agent({workingDir}): must be a non-empty string naming an existing " + + "git worktree of this repository." + ); + } + if (agentOpts.isolation !== undefined) { + throw new Error( + "agent({workingDir, isolation}): incompatible options. workingDir " + + "pins the agent to a worktree you already own; isolation creates " + + "a fresh one and removes it afterwards. Pass one." + ); + } + } if (typeof agentOpts.phase === 'string' && agentOpts.phase.length > 0) { if (__b.lastPhase() !== agentOpts.phase) { __b.pushPhase(agentOpts.phase); diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts new file mode 100644 index 00000000000..06519642bb9 --- /dev/null +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -0,0 +1,118 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const stubs = vi.hoisted(() => ({ + make: () => ({ + checkGitAvailable: vi.fn(async () => ({ + available: true, + error: undefined as string | undefined, + })), + isGitRepository: vi.fn(async () => true), + getRepoTopLevel: vi.fn(async () => '/repo'), + isRegisteredLinkedWorktree: vi.fn(async () => true), + getRegisteredWorktreeBranch: vi.fn(async () => ({ branch: 'pr-7' })), + }), + current: null as ReturnType<() => Record> | null, +})); + +vi.mock('../services/gitWorktreeService.js', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + GitWorktreeService: vi + .fn() + .mockImplementation(() => stubs.current as unknown), + }; +}); + +import { resolveExternalWorktreeDir } from './worktree-pin.js'; +import type { Config } from '../config/config.js'; + +const config = { getTargetDir: () => '/repo' } as unknown as Config; + +describe('resolveExternalWorktreeDir', () => { + let svc: ReturnType; + + beforeEach(() => { + svc = stubs.make(); + stubs.current = svc as unknown as Record; + }); + + it('resolves a registered worktree inside the repository', async () => { + const result = await resolveExternalWorktreeDir( + config, + '.qwen/tmp/review-pr-7', + ); + expect(result).toEqual({ + path: '/repo/.qwen/tmp/review-pr-7', + branch: 'pr-7', + slug: 'review-pr-7', + repoRoot: '/repo', + }); + }); + + // Pinning replaces the child's WorkspaceContext wholesale, so a path that + // escapes the repository would silently move the boundary of every file, + // shell and search tool the agent has. + it('refuses a path outside the repository', async () => { + const result = await resolveExternalWorktreeDir(config, '/elsewhere/tree'); + expect(result).toEqual({ + error: expect.stringContaining('resolves outside this repository'), + }); + }); + + // The authoritative gate: an unregistered directory is not isolation, it is + // a directory that happens to exist. + it('refuses a directory git does not know as a linked worktree', async () => { + svc.isRegisteredLinkedWorktree.mockResolvedValue(false); + const result = await resolveExternalWorktreeDir(config, 'plain-subdir'); + expect(result).toEqual({ + error: expect.stringContaining('is not a registered linked worktree'), + }); + }); + + it('names the missing git tooling rather than the directory', async () => { + svc.checkGitAvailable.mockResolvedValue({ + available: false, + error: 'git not found on PATH', + }); + const result = await resolveExternalWorktreeDir(config, 'wt'); + expect(result).toEqual({ error: expect.stringContaining('git not found') }); + }); + + // Without this preflight a non-repo parent produced the confusing "not a + // registered worktree" message instead of naming the real cause. + it('names a non-repository parent rather than the registration check', async () => { + svc.isGitRepository.mockResolvedValue(false); + const result = await resolveExternalWorktreeDir(config, 'wt'); + expect(result).toEqual({ + error: expect.stringContaining('/repo is not a git repository'), + }); + }); + + // A detached-HEAD worktree is legitimate and has no branch; the branch is a + // label, never a gate. + it('accepts a worktree with no branch label', async () => { + svc.getRegisteredWorktreeBranch.mockResolvedValue( + null as unknown as { branch: string }, + ); + const result = await resolveExternalWorktreeDir(config, 'wt'); + expect(result).toMatchObject({ path: '/repo/wt', branch: '' }); + }); + + // The same resolver serves AgentTool's `working_dir` and a workflow's + // `workingDir`; the caller says which name the reader will recognise. + it('names the caller parameter in its errors', async () => { + svc.isRegisteredLinkedWorktree.mockResolvedValue(false); + const asTool = await resolveExternalWorktreeDir(config, 'wt'); + const asOpt = await resolveExternalWorktreeDir(config, 'wt', 'workingDir'); + expect((asTool as { error: string }).error).toMatch(/^working_dir "/); + expect((asOpt as { error: string }).error).toMatch(/^workingDir "/); + }); +}); diff --git a/packages/core/src/agents/worktree-pin.ts b/packages/core/src/agents/worktree-pin.ts new file mode 100644 index 00000000000..e66932e3f0d --- /dev/null +++ b/packages/core/src/agents/worktree-pin.ts @@ -0,0 +1,142 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * @fileoverview Pinning an agent to a caller-owned git worktree. + * + * Two launch surfaces ask for the same thing — `AgentTool`'s `working_dir` + * parameter and a workflow script's `agent({workingDir})` — and both mean: + * run this agent inside a worktree that ALREADY exists and whose lifetime + * someone else owns. Neither creates the directory and neither removes it; + * they only rebind the child Config's cwd surfaces to it. + * + * The validation lives here, once, because it is the load-bearing half. The + * path comes from a model — a tool call's argument, or a line in a workflow + * script — and pinning replaces the child's `WorkspaceContext` wholesale, so + * an unvalidated path silently moves the boundary of every file, shell and + * search tool the agent has. + */ + +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import type { Config } from '../config/config.js'; +import { GitWorktreeService } from '../services/gitWorktreeService.js'; + +/** A validated pin target. `branch` / `slug` are labels, never gates. */ +export interface ResolvedWorktreePin { + path: string; + branch: string; + slug: string; + repoRoot: string; +} + +/** + * Resolve and validate a caller-owned worktree path (e.g. the PR-review + * worktree `/review`'s `fetch-pr` provisions). + * + * Two checks stop a bad path from aiming the agent somewhere it should not be: + * + * - It must resolve INSIDE the repository (canonical comparison), because + * pinning rebinds the child's `WorkspaceContext` wholesale. + * - It must be a REGISTERED linked worktree of this repository, enforced by + * `isRegisteredLinkedWorktree`: git's own registry entry for the path must + * point back at it, and it must not be the primary working tree. That + * rejects arbitrary directories, sibling `git init`s, plain sub-directories + * (including a stale registry record whose directory was recreated), other + * repositories' worktrees, and a directory carrying a copied `.git` file. + * + * `getRegisteredWorktreeBranch` is consulted only for a best-effort branch + * label; it is deliberately NOT a gate, since it returns null for a legitimate + * detached-HEAD worktree. + * + * @param label how to name the offending parameter in error text — `working_dir` + * for the tool, `workingDir` for the workflow opt. + * @returns the resolved absolute path + labels, or `{ error }` with a + * user-facing reason. + */ +export async function resolveExternalWorktreeDir( + config: Config, + workingDir: string, + label = 'working_dir', +): Promise { + const parentCwd = config.getTargetDir(); + const resolvedPath = path.resolve(parentCwd, workingDir); + + const probe = new GitWorktreeService(parentCwd); + const gitCheck = await probe.checkGitAvailable(); + if (!gitCheck.available) { + return { + error: `Cannot use ${label}: ${gitCheck.error ?? 'git is not available'}.`, + }; + } + // Mirror the isolation:'worktree' preflight. Without it, a non-repo parent + // dir yields the confusing "not a registered git worktree" error below + // (getRepoTopLevel() → null, validation then fails) instead of naming the + // real cause. + if (!(await probe.isGitRepository())) { + return { + error: `Cannot use ${label}: ${parentCwd} is not a git repository.`, + }; + } + // Anchor at the repo top-level so the common-dir comparison inside + // getRegisteredWorktreeBranch is against the repository, not a monorepo + // subdirectory the parent happened to launch from. + const repoRoot = (await probe.getRepoTopLevel()) ?? parentCwd; + const wtService = + repoRoot === parentCwd ? probe : new GitWorktreeService(repoRoot); + + // Containment. A registered worktree may live anywhere on disk, but pinning + // rebinds the child's WorkspaceContext wholesale, so a model-supplied path + // must not silently move the file tools' boundary outside the repository. + // (`isolation: 'worktree'` has this property implicitly — it always + // provisions under `/.qwen/worktrees/`.) Compare canonical + // paths so a symlink cannot straddle the boundary. + const realRepoRoot = await fs.realpath(repoRoot).catch(() => repoRoot); + const realResolved = await fs + .realpath(resolvedPath) + .catch(() => resolvedPath); + const relToRepo = path.relative(realRepoRoot, realResolved); + if (relToRepo.startsWith('..') || path.isAbsolute(relToRepo)) { + return { + error: + `${label} "${resolvedPath}" resolves outside this repository ` + + `(${realRepoRoot}). Pass a worktree that lives inside the repository.`, + }; + } + + // The single authoritative gate: the path must be a REGISTERED linked + // worktree of this repository — git's own registry entry for it points back + // at exactly this path, and it is not the primary working tree. That one + // check rejects the main tree, a plain sub-directory (including a stale + // registry record whose directory was recreated), a worktree belonging to + // another repo, and a hand-crafted directory carrying a copied `.git` file. + if (!(await wtService.isRegisteredLinkedWorktree(resolvedPath))) { + // Fails closed (returns false) on a git error too, so the cause is either + // "not a registered linked worktree" (main tree / unregistered) or "its + // git metadata could not be read" — name both rather than assert one. + return { + error: + `${label} "${resolvedPath}" is not a registered linked worktree of ` + + `this repository (it is the main working tree, is absent from \`git ` + + `worktree list\`, or its git metadata could not be read) — pinning an ` + + `agent there would not isolate it. Pass a worktree created via ` + + `\`git worktree add\`.`, + }; + } + // Best-effort branch label only — never a gate. A detached-HEAD worktree + // (`git worktree add --detach`, or a checkout of a bare commit) is a + // legitimate configuration with no branch, and `getRegisteredWorktreeBranch` + // returns null for it. `branch` is unused for caller-owned worktrees anyway + // (cleanup short-circuits on `externallyManaged`); it is carried only for + // parity with the isolation path. + const info = await wtService.getRegisteredWorktreeBranch(resolvedPath); + return { + path: resolvedPath, + branch: info?.branch ?? '', + slug: path.basename(resolvedPath), + repoRoot, + }; +} diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 95470a27948..c96f4afa0cd 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -5,8 +5,6 @@ */ import { randomUUID } from 'node:crypto'; -import * as path from 'node:path'; -import * as fs from 'node:fs/promises'; import { BaseDeclarativeTool, BaseToolInvocation, Kind } from '../tools.js'; import { ToolNames, ToolDisplayNames } from '../tool-names.js'; import { @@ -66,6 +64,7 @@ import { GitWorktreeService, writeWorktreeSessionMarker, } from '../../services/gitWorktreeService.js'; +import { resolveExternalWorktreeDir } from '../../agents/worktree-pin.js'; import { FileDiscoveryService } from '../../services/fileDiscoveryService.js'; import { WorkspaceContext } from '../../utils/workspaceContext.js'; import { getStartupContextLength } from '../../utils/environmentContext.js'; @@ -281,119 +280,6 @@ function getForkProfileModeError(config: Config): string | undefined { return undefined; } -/** - * Resolves and validates an `AgentParams.working_dir`: an EXISTING, - * caller-owned git worktree that a sub-agent should be pinned to (e.g. the - * PR-review worktree `/review`'s `fetch-pr` provisions). Unlike - * `isolation:'worktree'`, the harness neither creates nor tears down this - * directory — it only rebinds the child Config's cwd surfaces to it. - * - * Two checks stop a bad path from aiming the sub-agent somewhere it should not - * be: - * - * - It must resolve INSIDE the repository (canonical comparison), because - * pinning rebinds the child's `WorkspaceContext` wholesale. - * - It must be a REGISTERED linked worktree of this repository, enforced by - * `isRegisteredLinkedWorktree`: git's own registry entry for the path must - * point back at it, and it must not be the primary working tree. That - * rejects arbitrary directories, sibling `git init`s, plain sub-directories - * (including a stale registry record whose directory was recreated), - * other repositories' worktrees, and a directory carrying a copied `.git` - * file. - * - * `getRegisteredWorktreeBranch` is consulted only for a best-effort branch - * label; it is deliberately NOT a gate, since it returns null for a legitimate - * detached-HEAD worktree. - * - * @returns the resolved absolute path + branch, or `{ error }` with a - * user-facing reason. - */ -async function resolveExternalWorktreeDir( - config: Config, - workingDir: string, -): Promise< - | { path: string; branch: string; slug: string; repoRoot: string } - | { error: string } -> { - const parentCwd = config.getTargetDir(); - const resolvedPath = path.resolve(parentCwd, workingDir); - - const probe = new GitWorktreeService(parentCwd); - const gitCheck = await probe.checkGitAvailable(); - if (!gitCheck.available) { - return { - error: `Cannot use working_dir: ${gitCheck.error ?? 'git is not available'}.`, - }; - } - // Mirror the isolation:'worktree' preflight. Without it, a non-repo parent - // dir yields the confusing "not a registered git worktree" error below - // (getRepoTopLevel() → null, validation then fails) instead of naming the - // real cause. - if (!(await probe.isGitRepository())) { - return { - error: `Cannot use working_dir: ${parentCwd} is not a git repository.`, - }; - } - // Anchor at the repo top-level so the common-dir comparison inside - // getRegisteredWorktreeBranch is against the repository, not a monorepo - // subdirectory the parent happened to launch from. - const repoRoot = (await probe.getRepoTopLevel()) ?? parentCwd; - const wtService = - repoRoot === parentCwd ? probe : new GitWorktreeService(repoRoot); - - // Containment. A registered worktree may live anywhere on disk, but pinning - // rebinds the child's WorkspaceContext wholesale, so a model-supplied path - // must not silently move the file tools' boundary outside the repository. - // (`isolation: 'worktree'` has this property implicitly — it always - // provisions under `/.qwen/worktrees/`.) Compare canonical - // paths so a symlink cannot straddle the boundary. - const realRepoRoot = await fs.realpath(repoRoot).catch(() => repoRoot); - const realResolved = await fs - .realpath(resolvedPath) - .catch(() => resolvedPath); - const relToRepo = path.relative(realRepoRoot, realResolved); - if (relToRepo.startsWith('..') || path.isAbsolute(relToRepo)) { - return { - error: - `working_dir "${resolvedPath}" resolves outside this repository ` + - `(${realRepoRoot}). Pass a worktree that lives inside the repository.`, - }; - } - - // The single authoritative gate: the path must be a REGISTERED linked - // worktree of this repository — git's own registry entry for it points back - // at exactly this path, and it is not the primary working tree. That one - // check rejects the main tree, a plain sub-directory (including a stale - // registry record whose directory was recreated), a worktree belonging to - // another repo, and a hand-crafted directory carrying a copied `.git` file. - if (!(await wtService.isRegisteredLinkedWorktree(resolvedPath))) { - // Fails closed (returns false) on a git error too, so the cause is either - // "not a registered linked worktree" (main tree / unregistered) or "its - // git metadata could not be read" — name both rather than assert one. - return { - error: - `working_dir "${resolvedPath}" is not a registered linked worktree of ` + - `this repository (it is the main working tree, is absent from \`git ` + - `worktree list\`, or its git metadata could not be read) — pinning a ` + - `sub-agent there would not isolate it. Pass a worktree created via ` + - `\`git worktree add\`.`, - }; - } - // Best-effort branch label only — never a gate. A detached-HEAD worktree - // (`git worktree add --detach`, or a checkout of a bare commit) is a - // legitimate configuration with no branch, and `getRegisteredWorktreeBranch` - // returns null for it. `branch` is unused for caller-owned worktrees anyway - // (cleanup short-circuits on `externallyManaged`); it is carried only for - // parity with the isolation path. - const info = await wtService.getRegisteredWorktreeBranch(resolvedPath); - return { - path: resolvedPath, - branch: info?.branch ?? '', - slug: path.basename(resolvedPath), - repoRoot, - }; -} - const TEAM_AGENT_NAME_PROPERTY = { type: 'string', description: diff --git a/packages/core/src/tools/workflow/workflow.test.ts b/packages/core/src/tools/workflow/workflow.test.ts index b208e10b88a..58f8ff6c1ba 100644 --- a/packages/core/src/tools/workflow/workflow.test.ts +++ b/packages/core/src/tools/workflow/workflow.test.ts @@ -258,6 +258,32 @@ describe('WorkflowTool', () => { await expect(run(false)).resolves.toEqual(await run(undefined)); }); + // A headless run (`qwen --prompt`, CI, a cron job) has no TUI, no approval + // bridge, and a closed stdin. `getDefaultPermission()` is 'ask', which the + // scheduler resolves against the run's approval mode — but nothing INSIDE + // the tool or the runner may reach for interactivity, or the foreground + // call would hang forever on a prompt no one can answer. This is the + // regression test for that contract; the background half is already + // refused explicitly (see the interactive-TUI guard above). + it('foreground execute() completes with no interactive session or completion channel', async () => { + const registry = new WorkflowRunRegistry(); + const config = { + isInteractive: () => false, + getWorkflowRunRegistry: () => registry, + getSkipWorkflowUsageWarning: () => true, + } as unknown as Config; + expect(registry.hasCompletionCallback()).toBe(false); + + const result = await new WorkflowTool(config, { + dispatch: async (prompt) => `answered:${prompt}`, + }) + .build({ script: `return await agent('what is it');` }) + .execute(new AbortController().signal); + + expect(result.error).toBeUndefined(); + expect(JSON.stringify(result.llmContent)).toContain('answered:what is it'); + }); + it('execute() loads a saved-workflow scriptPath and records its provenance', async () => { const projectDir = await fs.mkdtemp(path.join(os.tmpdir(), 'wf-tool-')); try { diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index 9463dd5b590..fb0bd2013d1 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -79,7 +79,7 @@ const WORKFLOW_PARAM_SCHEMA = { 'JavaScript source of the workflow. Wrapped as an async IIFE. ' + 'May call the injected globals `phase(title)`, `log(msg)`, ' + '`agent(prompt, opts?)`, and read `args`. ' + - 'agent() opts: `{ label?, phase?, schema?, model?, agentType?, isolation? }`. ' + + 'agent() opts: `{ label?, phase?, schema?, model?, agentType?, isolation?, workingDir?, stallMs? }`. ' + '`schema` (JSON Schema object): the subagent must deliver its result ' + 'by calling `structured_output` with arguments matching the schema; ' + 'agent() resolves to the validated object. Two failed attempts produce ' + @@ -101,6 +101,13 @@ const WORKFLOW_PARAM_SCHEMA = { 'in this build" (parity with upstream). isolation=worktree refuses to ' + 'run when the parent working tree has uncommitted changes (the subagent ' + 'would see a stale HEAD). ' + + '`workingDir` (string): pin the subagent to an EXISTING git worktree of ' + + 'this repository that the caller owns — nothing is created and nothing ' + + 'is removed. Use it when the directory the agent must work in already ' + + 'exists and its uncommitted state is the point (a review worktree, a ' + + 'checkout a previous step provisioned) — exactly the case isolation ' + + 'cannot serve. Mutually exclusive with `isolation`. The path must live ' + + 'inside the repository and appear in `git worktree list`. ' + 'Workflow subagents always have SendMessage / Monitor / EnterPlanMode / ExitPlanMode ' + 'in their disallowed-tool floor regardless of agentType. ' + 'Concurrency: `parallel([() => agent(...), ...])` runs thunks ' + From af057d6f8b974f61a58aaada65e94d319402166e Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 12 Aug 2026 14:15:20 +0800 Subject: [PATCH 02/12] fix(workflows): harden agent worktree pinning --- .../src/agents/runtime/workflow-journal.ts | 6 +-- .../runtime/workflow-orchestrator.test.ts | 44 +++++++++++++++++++ .../agents/runtime/workflow-orchestrator.ts | 14 ++++-- .../agents/runtime/workflow-sandbox.test.ts | 5 ++- packages/core/src/agents/worktree-pin.test.ts | 5 +++ packages/core/src/agents/worktree-pin.ts | 6 ++- 6 files changed, 72 insertions(+), 8 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-journal.ts b/packages/core/src/agents/runtime/workflow-journal.ts index 5b1b3fe73fa..714edfb258a 100644 --- a/packages/core/src/agents/runtime/workflow-journal.ts +++ b/packages/core/src/agents/runtime/workflow-journal.ts @@ -21,9 +21,9 @@ * key, and so on — so the cache naturally invalidates from the edit point. * * The `canonicalOpts` projection keeps only the dispatch-affecting opts - * (`schema`, `model`, `isolation`, `agentType`) with object keys sorted, so - * cosmetic opt differences (a re-ordered schema, a `label` change) don't - * bust the cache. + * (`schema`, `model`, `isolation`, `agentType`, `workingDir`) with object keys + * sorted, so cosmetic opt differences (a re-ordered schema, a `label` change) + * don't bust the cache. * * Determinism requirement: workflow scripts are deterministic (`Date.now` * / `Math.random` throw in the sandbox), so the sequence of `agent()` diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 3996f9b04e8..19df63a2a2a 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -2585,6 +2585,7 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( runtimeContextSame: boolean; /** What the subagent's Config answers for "where am I?". */ runtimeTargetDir?: string; + runtimeIgnoreFiles?: string; options?: { runConfigOverrides?: unknown }; eventEmitterAttached: boolean; executeAgentId?: string | null; @@ -2640,6 +2641,11 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( // these methods. Provide deterministic returns so the tests can // drive GitWorktreeService stubs without re-deriving cwd. getTargetDir: () => '/fake/repo', + getFileFilteringOptions: () => ({ + respectGitIgnore: true, + respectQwenIgnore: true, + customIgnoreFiles: ['.cursorignore'], + }), getSessionId: () => 'sess_fake_test_id', getWorktreeSymlinkDirectories: () => [], getSubagentManager: () => ({ @@ -2657,6 +2663,9 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( config: subagentConfig, runtimeContextSame: runtimeContext === cfg, runtimeTargetDir: runtimeContext.getTargetDir(), + runtimeIgnoreFiles: runtimeContext + .getFileService?.() + .getQwenIgnoreFileNamesDisplay(), options: { runConfigOverrides: options?.runConfigOverrides }, eventEmitterAttached: options?.eventEmitter !== undefined, }; @@ -3398,6 +3407,18 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( '/fake/repo/.qwen/tmp/review-pr-7', ); expect(helper.calls[0]!.runtimeContextSame).toBe(false); + expect(helper.calls[0]!.runtimeIgnoreFiles).toContain('.cursorignore'); + }); + + it('workingDir rejects invalid values before dispatch', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + + await expect( + createProductionDispatch(helper.config)('hi', { workingDir: '' }), + ).rejects.toThrow(/workingDir.*non-empty string/); + expect(helper.calls).toHaveLength(0); }); // A refused pin must abort the dispatch, not fall through to an agent @@ -3420,6 +3441,29 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( expect(helper.calls).toHaveLength(0); }); + it('workingDir scrubs control characters from resolver errors', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + pinStub.resolve.value = async () => ({ + error: 'refused\r\nforged\u0000line', + }); + + let error: unknown; + try { + await createProductionDispatch(helper.config)('hi', { + workingDir: 'not-a-worktree', + }); + } catch (caught) { + error = caught; + } + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain('\r'); + expect((error as Error).message).not.toContain('\n'); + expect((error as Error).message).not.toContain('\u0000'); + expect(helper.calls).toHaveLength(0); + }); + // The resolver names the offending parameter in its errors; a workflow // script never wrote `working_dir`, so it must be told the opt's own name. it('names the workflow opt, not the tool parameter, when resolving', async () => { diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index 36bdfc823a2..ff766f1621f 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -792,7 +792,12 @@ async function runOverridePath( config, worktreeIsolation.path, ); - } else if (typeof opts.workingDir === 'string' && opts.workingDir) { + } else if (opts.workingDir !== undefined) { + if (typeof opts.workingDir !== 'string' || !opts.workingDir) { + throw new Error( + 'agent({workingDir}): must be a non-empty string naming an existing git worktree of this repository.', + ); + } // Caller-owned worktree: same rebind, no provisioning and no cleanup. // Validated by AgentTool's own `working_dir` resolver so a script-supplied // path cannot move the subagent's workspace boundary somewhere the @@ -805,7 +810,7 @@ async function runOverridePath( ); if ('error' in resolved) { throw new Error( - `agent({workingDir: ${JSON.stringify(opts.workingDir)}}): ${resolved.error}`, + `agent({workingDir: ${JSON.stringify(opts.workingDir)}}): ${sanitizeForErrorMessage(resolved.error)}`, ); } effectiveContext = createDirScopedConfigOverride(config, resolved.path); @@ -1196,7 +1201,10 @@ function createDirScopedConfigOverride(base: Config, wtPath: string): Config { ov.getCwd = () => wtPath; ov.getWorkingDir = () => wtPath; ov.getProjectRoot = () => wtPath; - const wtFileService = new FileDiscoveryService(wtPath); + const wtFileService = new FileDiscoveryService( + wtPath, + base.getFileFilteringOptions().customIgnoreFiles, + ); ov.fileDiscoveryService = wtFileService; ov.getFileService = () => wtFileService; const wtWorkspace = new WorkspaceContext(wtPath); diff --git a/packages/core/src/agents/runtime/workflow-sandbox.test.ts b/packages/core/src/agents/runtime/workflow-sandbox.test.ts index 45725a7d148..5eeff4825e7 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.test.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.test.ts @@ -1670,7 +1670,7 @@ describe('createWorkflowSandbox security', () => { ); }); - it('agent({workingDir}) rejects a non-string', async () => { + it('agent({workingDir}) rejects invalid values', async () => { const sandbox = createWorkflowSandbox({ args: undefined, dispatch: async () => 'ignored', @@ -1678,6 +1678,9 @@ describe('createWorkflowSandbox security', () => { await expect( sandbox.run(`return agent("x", { workingDir: 7 });`), ).rejects.toThrow(/workingDir.*non-empty string/); + await expect( + sandbox.run(`return agent("x", { workingDir: "" });`), + ).rejects.toThrow(/workingDir.*non-empty string/); }); // The two options make opposite claims about who owns the directory's diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts index 06519642bb9..81697c35850 100644 --- a/packages/core/src/agents/worktree-pin.test.ts +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -57,6 +57,11 @@ describe('resolveExternalWorktreeDir', () => { }); }); + it('accepts an in-repository worktree whose name starts with two dots', async () => { + const result = await resolveExternalWorktreeDir(config, '..hidden-wt'); + expect(result).toMatchObject({ path: '/repo/..hidden-wt' }); + }); + // Pinning replaces the child's WorkspaceContext wholesale, so a path that // escapes the repository would silently move the boundary of every file, // shell and search tool the agent has. diff --git a/packages/core/src/agents/worktree-pin.ts b/packages/core/src/agents/worktree-pin.ts index e66932e3f0d..9e93afde380 100644 --- a/packages/core/src/agents/worktree-pin.ts +++ b/packages/core/src/agents/worktree-pin.ts @@ -99,7 +99,11 @@ export async function resolveExternalWorktreeDir( .realpath(resolvedPath) .catch(() => resolvedPath); const relToRepo = path.relative(realRepoRoot, realResolved); - if (relToRepo.startsWith('..') || path.isAbsolute(relToRepo)) { + if ( + relToRepo === '..' || + relToRepo.startsWith(`..${path.sep}`) || + path.isAbsolute(relToRepo) + ) { return { error: `${label} "${resolvedPath}" resolves outside this repository ` + From b78fc69e1fe1a39f13614d8f80ec958dbcaee909 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 09:51:41 +0000 Subject: [PATCH 03/12] fix(workflows): anchor worktree-pin containment at the main working tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the remaining review findings on the agent workingDir pin. Containment anchored at `--show-toplevel`, which from inside a linked worktree answers with the worktree's own root — spuriously refusing registered sibling worktrees, the documented review-pipeline setup. Resolve the repository's main working tree via the first entry of `git worktree list --porcelain` (new GitWorktreeService helper) and anchor there, keeping the toplevel answer as fallback. Add dispatch-site wiring tests for the env-tunable subagent bounds at both the fast and the override path: with a clean env the DEFAULT_* constants and the resolvers are indistinguishable, so a revert mutation at either call site kept every existing test green. Cover the fs.realpath half of the containment guard with a real symlink fixture (plain-string stubs made both realpath calls reject, so the canonical-path logic never executed), and the sibling-anchor fix with a unit case. Correct the bounds doc comment: only QWEN_CODE_MAX_WORKFLOW_AGENTS and the subagent bounds clamp to a ceiling; the stall and wall-clock env overrides apply valid values verbatim. --- .../runtime/workflow-orchestrator.test.ts | 57 +++++++++++++++++++ .../agents/runtime/workflow-orchestrator.ts | 7 ++- packages/core/src/agents/worktree-pin.test.ts | 47 +++++++++++++++ packages/core/src/agents/worktree-pin.ts | 16 ++++-- .../core/src/services/gitWorktreeService.ts | 29 ++++++++++ 5 files changed, 149 insertions(+), 7 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 19df63a2a2a..c282da0efa4 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -1917,6 +1917,32 @@ describe('createProductionDispatch', () => { }); }); + // Resolver-level tests cannot catch a revert at the dispatch site: in a + // clean env the DEFAULT_* constants and the resolvers produce identical + // numbers, so the wiring test above stays green either way. Stub the env + // so the dispatched runConfig itself proves the resolver is wired in. + it('fast-path dispatch honors the env-tunable subagent bounds', async () => { + const prevTurns = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + const prevMinutes = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'] = '120'; + process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'] = '45'; + try { + const dispatch = createProductionDispatch(fakeConfig()); + await dispatch('hello', { label: 'h1' }); + expect(created[0]!.runConfig).toEqual({ + max_turns: 120, + max_time_minutes: 45, + }); + } finally { + if (prevTurns === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'] = prevTurns; + if (prevMinutes === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'] = prevMinutes; + } + }); + // T11: disallow SendMessage plus tools that break workflow return/cleanup // contracts. it('disallows workflow-only floor tools for workflow subagents', async () => { @@ -2822,6 +2848,37 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( expect(calls[0].config.model).toBe('qwen3-max'); }); + // Same wiring guard as the fast-path sibling above, for the override + // dispatch site: with a clean env the resolvers and the DEFAULT_* + // constants are indistinguishable, so only a stubbed env proves the + // runConfigOverrides handed to createAgentHeadless come from the + // resolvers. + it('override-path dispatch honors the env-tunable subagent bounds', async () => { + const prevTurns = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + const prevMinutes = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'] = '120'; + process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'] = '45'; + try { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'ok', terminateMode: 'GOAL' }), + }); + await createProductionDispatch(helper.config)('hi', { + model: 'qwen3-max', + }); + expect(helper.calls[0]!.options?.runConfigOverrides).toEqual({ + max_turns: 120, + max_time_minutes: 45, + }); + } finally { + if (prevTurns === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'] = prevTurns; + if (prevMinutes === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'] = prevMinutes; + } + }); + it("isolation:'remote' throws upstream-aligned 'not available' error", async () => { const { config } = fakeConfigWithMgr({ onCreate: async () => ({ finalText: '', terminateMode: 'GOAL' }), diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index ff766f1621f..3adaea62691 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -160,9 +160,10 @@ export function resolveConcurrencyLimit( * turns or 10 minutes routinely, and under the GOAL-terminal contract * hitting either shows up as a `null` element in `parallel()`: an agent that * silently went missing rather than one that visibly failed. So both are - * operator-tunable via env, on the same pattern as the three other workflow - * bounds (`QWEN_CODE_MAX_WORKFLOW_AGENTS`, `QWEN_CODE_WORKFLOW_STALL_SECONDS`, - * `QWEN_CODE_MAX_WORKFLOW_SECONDS`), each clamped to a hard ceiling. + * operator-tunable via env, on the same env-override pattern as the other + * workflow bounds; like `QWEN_CODE_MAX_WORKFLOW_AGENTS` (and unlike + * `QWEN_CODE_WORKFLOW_STALL_SECONDS` / `QWEN_CODE_MAX_WORKFLOW_SECONDS`, + * which apply valid overrides verbatim), clamped to a hard ceiling. * * Three time bounds act on a dispatch and they are NOT redundant: * - `stallMs` (60s default) — no *progress* for this long ⇒ abort + retry. diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts index 81697c35850..ebe5bb72b74 100644 --- a/packages/core/src/agents/worktree-pin.test.ts +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -5,6 +5,9 @@ */ import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; const stubs = vi.hoisted(() => ({ make: () => ({ @@ -14,6 +17,7 @@ const stubs = vi.hoisted(() => ({ })), isGitRepository: vi.fn(async () => true), getRepoTopLevel: vi.fn(async () => '/repo'), + getMainWorktreePath: vi.fn(async () => '/repo'), isRegisteredLinkedWorktree: vi.fn(async () => true), getRegisteredWorktreeBranch: vi.fn(async () => ({ branch: 'pr-7' })), }), @@ -62,6 +66,49 @@ describe('resolveExternalWorktreeDir', () => { expect(result).toMatchObject({ path: '/repo/..hidden-wt' }); }); + // From inside a linked worktree `--show-toplevel` answers with the + // worktree's own root. Containment must still anchor at the main working + // tree, or a registered sibling worktree — the documented review-pipeline + // setup — is spuriously refused. + it('accepts a sibling worktree when the parent runs inside a linked worktree', async () => { + svc.getRepoTopLevel.mockResolvedValue('/repo/.qwen/tmp/review-pr-1'); + const insideWorktree = { + getTargetDir: () => '/repo/.qwen/tmp/review-pr-1', + } as unknown as Config; + const result = await resolveExternalWorktreeDir( + insideWorktree, + '../review-pr-1-base', + ); + expect(result).toMatchObject({ + path: '/repo/.qwen/tmp/review-pr-1-base', + }); + }); + + // The containment comparison canonicalises both sides, so a symlink cannot + // straddle the repository boundary. Real temp dirs (not stubs) force the + // fs.realpath calls to actually run — with plain-string stubs both reject + // and the .catch() fallbacks degrade the check to string comparison. + it('refuses an in-repo symlink that canonicalizes outside the repository', async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'wt-pin-')); + try { + const repoDir = path.join(root, 'repo'); + const outsideTarget = path.join(root, 'outside', 'wt'); + await fs.mkdir(repoDir, { recursive: true }); + await fs.mkdir(outsideTarget, { recursive: true }); + await fs.symlink(outsideTarget, path.join(repoDir, 'link-wt')); + svc.getMainWorktreePath.mockResolvedValue(repoDir); + const localConfig = { + getTargetDir: () => repoDir, + } as unknown as Config; + const result = await resolveExternalWorktreeDir(localConfig, 'link-wt'); + expect(result).toEqual({ + error: expect.stringContaining('resolves outside this repository'), + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + // Pinning replaces the child's WorkspaceContext wholesale, so a path that // escapes the repository would silently move the boundary of every file, // shell and search tool the agent has. diff --git a/packages/core/src/agents/worktree-pin.ts b/packages/core/src/agents/worktree-pin.ts index 9e93afde380..aad29a04a71 100644 --- a/packages/core/src/agents/worktree-pin.ts +++ b/packages/core/src/agents/worktree-pin.ts @@ -81,10 +81,18 @@ export async function resolveExternalWorktreeDir( error: `Cannot use ${label}: ${parentCwd} is not a git repository.`, }; } - // Anchor at the repo top-level so the common-dir comparison inside - // getRegisteredWorktreeBranch is against the repository, not a monorepo - // subdirectory the parent happened to launch from. - const repoRoot = (await probe.getRepoTopLevel()) ?? parentCwd; + // Anchor at the repository's MAIN working tree. From inside a linked + // worktree (the normal state for /review-style pipelines) + // `--show-toplevel` answers with the worktree's own root, which would + // spuriously refuse registered sibling worktrees and mislabel the + // repository in the refusal below. The toplevel answer is the fallback + // when the worktree list cannot be read; either anchor also keeps the + // common-dir comparison inside getRegisteredWorktreeBranch against the + // repository, not a monorepo subdirectory the parent launched from. + const repoRoot = + (await probe.getMainWorktreePath()) ?? + (await probe.getRepoTopLevel()) ?? + parentCwd; const wtService = repoRoot === parentCwd ? probe : new GitWorktreeService(repoRoot); diff --git a/packages/core/src/services/gitWorktreeService.ts b/packages/core/src/services/gitWorktreeService.ts index dc8c17b0a4c..468057ac8c5 100644 --- a/packages/core/src/services/gitWorktreeService.ts +++ b/packages/core/src/services/gitWorktreeService.ts @@ -315,6 +315,35 @@ export class GitWorktreeService { } } + /** + * Returns the repository's primary working tree path, or `null` when it + * cannot be determined. Unlike `getRepoTopLevel()` — which answers + * "which worktree is this cwd in" and so names a linked worktree's OWN + * root when run inside one — this always resolves the MAIN tree: + * `git worktree list --porcelain` lists the primary working tree first + * regardless of where in the repository it runs. Callers anchoring a + * check at the repository itself (not the current worktree) use this. + * A main-tree path containing a newline truncates to the first line; + * consumers fail closed on the bad anchor and the authoritative + * registration checks never consult this value. + */ + async getMainWorktreePath(): Promise { + try { + const out = await ( + await this.getGit() + ).raw(['worktree', 'list', '--porcelain']); + const firstLine = out.split('\n', 1)[0]?.trim() ?? ''; + if (!firstLine.startsWith('worktree ')) return null; + const mainPath = firstLine.slice('worktree '.length).trim(); + return mainPath.length > 0 ? mainPath : null; + } catch (error) { + debugLogger.warn( + `getMainWorktreePath failed at ${this.sourceRepoPath}: ${error}`, + ); + return null; + } + } + /** * Checks if the source path is a git repository. */ From b989dc3da4212dd71cc017200dad5d83f7a85676 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 13:03:45 +0000 Subject: [PATCH 04/12] fix(workflows): refuse truncated main-tree anchors in worktree pinning (#8972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A main working tree whose path contains a newline splits the porcelain first entry of `git worktree list`, and the truncated prefix could resolve inside a different repository — re-anchoring the pin's containment and registration checks against that repo's worktree registry. Detect the malformed first record (a path remainder where a record attribute belongs) and fall back to `--show-toplevel`, whose single-value answer keeps interior newlines intact. Also pins down round-2 review findings: direct unit and real-git coverage for `getMainWorktreePath()` (whose semantics were only exercised through a stub), the symmetric journal-key HIT direction for `workingDir` resumes, and the model-facing `workingDir` eligibility description (the main checkout is not a valid pin target even though it appears in `git worktree list`). --- .../agents/runtime/workflow-journal.test.ts | 12 ++++ .../gitWorktreeService.linked.integ.test.ts | 72 +++++++++++++++++++ .../src/services/gitWorktreeService.test.ts | 70 ++++++++++++++++++ .../core/src/services/gitWorktreeService.ts | 39 ++++++++-- packages/core/src/tools/workflow/workflow.ts | 3 +- 5 files changed, 191 insertions(+), 5 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-journal.test.ts b/packages/core/src/agents/runtime/workflow-journal.test.ts index d4991bc8619..75b746be168 100644 --- a/packages/core/src/agents/runtime/workflow-journal.test.ts +++ b/packages/core/src/agents/runtime/workflow-journal.test.ts @@ -44,6 +44,18 @@ describe('canonicalizeAgentOpts', () => { JSON.stringify({ workingDir: 'wt' }), ); expect(a).not.toBe(b); + // Symmetric HIT direction: the same workingDir must derive the SAME key, + // or resumeFromRunId silently misses the journal and re-spends every + // dispatch of a workingDir-using workflow. + expect( + deriveAgentKey('', 'review it', { + workingDir: '.qwen/tmp/review-pr-1', + }), + ).toBe( + deriveAgentKey('', 'review it', { + workingDir: '.qwen/tmp/review-pr-1', + }), + ); }); it('sorts object keys deeply so reordered schemas hash the same', () => { diff --git a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts index 6f886d51885..2a66af17ad7 100644 --- a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts +++ b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts @@ -229,3 +229,75 @@ describe('GitWorktreeService.isRegisteredLinkedWorktree() (real git)', () => { }, ); }); + +describe('GitWorktreeService.getMainWorktreePath() (real git)', () => { + vi.setConfig({ testTimeout: 30000, hookTimeout: 30000 }); + + const tmpDirs: string[] = []; + + afterEach(() => { + for (const dir of tmpDirs.splice(0)) { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + function commitInitial(tree: string): void { + execFileSync('git', ['config', 'user.email', 't@e.com'], { cwd: tree }); + execFileSync('git', ['config', 'user.name', 't'], { cwd: tree }); + execFileSync('git', ['config', 'commit.gpgsign', 'false'], { cwd: tree }); + fs.writeFileSync(path.join(tree, 'README.md'), 'hi\n'); + execFileSync('git', ['add', '.'], { cwd: tree }); + execFileSync('git', ['commit', '-q', '-m', 'init', '--no-verify'], { + cwd: tree, + }); + } + + function initRepo(prefix: string): string { + const repo = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), prefix)), + ); + tmpDirs.push(repo); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: repo }); + commitInitial(repo); + return repo; + } + + it('answers the main tree even when called from inside a linked worktree', async () => { + // The anchor this PR re-anchored on: `--show-toplevel` from a linked + // worktree names the worktree's OWN root, which spuriously refused + // sibling pins. The porcelain listing names the main tree regardless of + // the calling worktree. + const repo = initRepo('qwen-mainpath-wt-'); + const wt = path.join(repo, '.qwen', 'tmp', 'review-pr-1'); + fs.mkdirSync(path.dirname(wt), { recursive: true }); + execFileSync('git', ['worktree', 'add', '-b', 'review-pr-1', wt, 'HEAD'], { + cwd: repo, + }); + + const fromWorktree = new GitWorktreeService(wt); + expect(await fromWorktree.getMainWorktreePath()).toBe(repo); + expect(await fromWorktree.getRepoTopLevel()).toBe(wt); + const fromMain = new GitWorktreeService(repo); + expect(await fromMain.getMainWorktreePath()).toBe(repo); + }); + + // A newline inside the main-tree path splits the porcelain first entry; + // the truncated prefix can fall inside a DIFFERENT repository, against + // whose worktree registry the pin gate would then validate. The parse must + // refuse the truncated anchor so callers fall back to `--show-toplevel` — + // a single value, so interior newlines survive. Newline path components + // are not representable on Win32. + it.skipIf(process.platform === 'win32')( + 'refuses the truncated anchor when the main-tree path contains a newline', + async () => { + const outer = initRepo('qwen-mainpath-outer-'); + const nlRepo = path.join(outer, 'sub', '\nR1'); + fs.mkdirSync(path.dirname(nlRepo), { recursive: true }); + execFileSync('git', ['clone', '-q', outer, nlRepo], { cwd: outer }); + + const svc = new GitWorktreeService(nlRepo); + expect(await svc.getMainWorktreePath()).toBeNull(); + expect(await svc.getRepoTopLevel()).toBe(nlRepo); + }, + ); +}); diff --git a/packages/core/src/services/gitWorktreeService.test.ts b/packages/core/src/services/gitWorktreeService.test.ts index ef46e06c82a..ed34731f14e 100644 --- a/packages/core/src/services/gitWorktreeService.test.ts +++ b/packages/core/src/services/gitWorktreeService.test.ts @@ -607,4 +607,74 @@ describe('GitWorktreeService', () => { ).toBeNull(); }); }); + + describe('getMainWorktreePath', () => { + it('parses the first porcelain entry as the main worktree path', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /repo\n' + + 'HEAD abc123\n' + + 'branch refs/heads/main\n' + + '\n' + + 'worktree /repo/.qwen/worktrees/wt\n' + + 'HEAD abc123\n' + + 'branch refs/heads/wt\n', + ); + const service = new GitWorktreeService('/repo/.qwen/worktrees/wt'); + + await expect(service.getMainWorktreePath()).resolves.toBe('/repo'); + expect(hoistedMockRaw).toHaveBeenCalledWith([ + 'worktree', + 'list', + '--porcelain', + ]); + }); + + it('accepts a bare-repository first entry', async () => { + hoistedMockRaw.mockResolvedValueOnce('worktree /srv/repo.git\nbare\n'); + const service = new GitWorktreeService('/srv/repo.git'); + + await expect(service.getMainWorktreePath()).resolves.toBe( + '/srv/repo.git', + ); + }); + + it('returns null when the first line is not a worktree entry', async () => { + hoistedMockRaw.mockResolvedValueOnce('HEAD abc123\n'); + const service = new GitWorktreeService('/repo'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + + it('returns null when the porcelain output is empty', async () => { + hoistedMockRaw.mockResolvedValueOnce(''); + const service = new GitWorktreeService('/repo'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + + it('returns null when git fails', async () => { + hoistedMockRaw.mockRejectedValueOnce(new Error('git unavailable')); + const service = new GitWorktreeService('/repo'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + + // A main-tree path containing a newline splits the first porcelain entry + // across lines; the truncated prefix can resolve inside a DIFFERENT + // repository and aim the containment gate at that repo's worktree + // registry. The path remainder lands where a record attribute belongs — + // it is not one, so the anchor is refused and callers fall back to + // `--show-toplevel`, which keeps interior newlines intact. + it('returns null when the main-tree path contains a newline', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /outer/sub/\n' + + 'R1\n' + + 'HEAD abc123\n' + + 'branch refs/heads/main\n', + ); + const service = new GitWorktreeService('/outer/sub/\nR1'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + }); }); diff --git a/packages/core/src/services/gitWorktreeService.ts b/packages/core/src/services/gitWorktreeService.ts index 468057ac8c5..6301b85d8d2 100644 --- a/packages/core/src/services/gitWorktreeService.ts +++ b/packages/core/src/services/gitWorktreeService.ts @@ -158,6 +158,24 @@ export function generateAgentWorktreeSlug(): string { return `${AGENT_WORKTREE_PREFIX}-${hex}`; } +/** + * Attribute lines a `git worktree list --porcelain` record can carry after + * its `worktree ` line. Anything else on those lines means the path + * above contained a newline and the entry was truncated. + */ +function isWorktreeListPorcelainAttribute(line: string): boolean { + return ( + line === 'bare' || + line === 'detached' || + line === 'locked' || + line === 'prunable' || + line.startsWith('HEAD ') || + line.startsWith('branch ') || + line.startsWith('locked ') || + line.startsWith('prunable ') + ); +} + export interface WorktreeInfo { /** Unique identifier for this worktree */ id: string; @@ -323,17 +341,30 @@ export class GitWorktreeService { * `git worktree list --porcelain` lists the primary working tree first * regardless of where in the repository it runs. Callers anchoring a * check at the repository itself (not the current worktree) use this. - * A main-tree path containing a newline truncates to the first line; - * consumers fail closed on the bad anchor and the authoritative - * registration checks never consult this value. + * + * The porcelain format is newline-delimited, so a main-tree path that + * itself contains a newline splits across lines and the first entry + * truncates. The remainder then appears where a record attribute belongs; + * it is not one, which is how the truncation is detected — this method + * returns `null` and callers fall back to `getRepoTopLevel()`, whose + * single-value `--show-toplevel` answer keeps interior newlines intact. + * (`--porcelain -z` would be immune but needs Git >= 2.36.) A truncated + * anchor is not merely wrong: it can resolve inside a DIFFERENT repository + * whose worktree registry the gate below would then consult. */ async getMainWorktreePath(): Promise { try { const out = await ( await this.getGit() ).raw(['worktree', 'list', '--porcelain']); - const firstLine = out.split('\n', 1)[0]?.trim() ?? ''; + const lines = out.split('\n'); + const firstLine = lines[0]?.trim() ?? ''; if (!firstLine.startsWith('worktree ')) return null; + for (const line of lines.slice(1)) { + const attr = line.trim(); + if (attr === '') break; // blank line ends the first record + if (!isWorktreeListPorcelainAttribute(attr)) return null; + } const mainPath = firstLine.slice('worktree '.length).trim(); return mainPath.length > 0 ? mainPath : null; } catch (error) { diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index d6439c4d6bc..39260566653 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -112,7 +112,8 @@ const WORKFLOW_PARAM_SCHEMA = { 'exists and its uncommitted state is the point (a review worktree, a ' + 'checkout a previous step provisioned) — exactly the case isolation ' + 'cannot serve. Mutually exclusive with `isolation`. The path must live ' + - 'inside the repository and appear in `git worktree list`. ' + + 'inside the repository and be a linked worktree registered via ' + + '`git worktree add` — the main checkout is not eligible. ' + 'Workflow subagents always have SendMessage / Monitor / EnterPlanMode / ExitPlanMode ' + 'in their disallowed-tool floor regardless of agentType. ' + 'Concurrency: `parallel([() => agent(...), ...])` runs thunks ' + From 1ef877794b394d4fc3845f905810c0e10bb4dec1 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 17:10:29 +0000 Subject: [PATCH 05/12] fix(workflows): harden worktree pinning per round-3 review (#8972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Round-trip-validate the porcelain main-tree anchor (git-common-dir must agree) so attribute-shaped or trailing-newline truncations cannot aim the pin gate at a different repository's worktree registry - Preserve legitimate path whitespace when parsing the anchor and the --show-toplevel fallback (terminator-only strip, untrimmed raw output) - Thread one canonical realpath through both pin gates and the rebind so a re-pointed symlink cannot land the child where neither gate looked - Canonicalise both containment sides or neither, so an absent target reaches the registration gate's accurate message instead of a manufactured outside-the-repository refusal - Name the degraded anchor in the containment refusal when the main working tree could not be determined - Throw on agent({workingDir, isolation}) at the orchestrator entrance (revived plain object — not evadable by the sandbox getter trick) - Trim-based blank check for workingDir at both workflow entrances - Document stallMs in the workflow schema and extend the tool-level capability enumeration to workingDir and stallMs --- .../runtime/workflow-orchestrator.test.ts | 32 ++++++++ .../agents/runtime/workflow-orchestrator.ts | 17 +++- .../agents/runtime/workflow-sandbox.test.ts | 6 ++ .../src/agents/runtime/workflow-sandbox.ts | 2 +- packages/core/src/agents/worktree-pin.test.ts | 81 +++++++++++++++++++ packages/core/src/agents/worktree-pin.ts | 56 +++++++++---- .../gitWorktreeService.linked.integ.test.ts | 36 +++++++++ .../src/services/gitWorktreeService.test.ts | 61 ++++++++++++++ .../core/src/services/gitWorktreeService.ts | 61 +++++++++++--- packages/core/src/tools/workflow/workflow.ts | 13 ++- 10 files changed, 335 insertions(+), 30 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index c282da0efa4..0ba5ad3b0a0 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -3478,6 +3478,38 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( expect(helper.calls).toHaveLength(0); }); + it('workingDir rejects whitespace-only values at the entrance', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + + await expect( + createProductionDispatch(helper.config)('hi', { workingDir: ' ' }), + ).rejects.toThrow(/workingDir.*non-empty string/); + expect(helper.calls).toHaveLength(0); + }); + + // The sandbox gate names this contradiction too, but it reads the raw opts + // BEFORE the JSON revival — an enumerable getter can withhold `isolation` + // during validation and surface it at stringify time. The orchestrator sees + // the revived plain object, so its refusal is the one that cannot be + // evaded; without it, isolation would silently win and workingDir would be + // dropped — the opposite of AgentTool's documented working_dir-wins rule. + it('workingDir + isolation throws instead of letting one silently win', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + + await expect( + createProductionDispatch(helper.config)('hi', { + workingDir: '.qwen/tmp/review-pr-7', + isolation: 'worktree', + }), + ).rejects.toThrow(/incompatible options/); + expect(helper.calls).toHaveLength(0); + expect(worktreeStubs.instances).toHaveLength(0); + }); + // A refused pin must abort the dispatch, not fall through to an agent // running in the parent tree — the failure mode the pin exists to prevent. it('workingDir surfaces the resolver refusal and dispatches nothing', async () => { diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index 3adaea62691..b09d01af566 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -787,6 +787,18 @@ async function runOverridePath( // resolve cwd-related getters via the prototype chain. let worktreeIsolation: WorkflowWorktreeIsolation | null = null; let effectiveContext: Config = config; + // Same contradiction the sandbox gate names, re-checked here: the sandbox + // gate reads the raw opts BEFORE the JSON revival, so an enumerable getter + // can withhold `isolation` during validation and surface it at stringify + // time. The host sees the revived plain object, so this check is the one + // that cannot be evaded. + if (opts.isolation !== undefined && opts.workingDir !== undefined) { + throw new Error( + 'agent({workingDir, isolation}): incompatible options. workingDir ' + + 'pins the agent to a worktree you already own; isolation creates ' + + 'a fresh one and removes it afterwards. Pass one.', + ); + } if (opts.isolation === 'worktree') { worktreeIsolation = await provisionWorkflowWorktree(config); effectiveContext = createDirScopedConfigOverride( @@ -794,7 +806,10 @@ async function runOverridePath( worktreeIsolation.path, ); } else if (opts.workingDir !== undefined) { - if (typeof opts.workingDir !== 'string' || !opts.workingDir) { + if ( + typeof opts.workingDir !== 'string' || + opts.workingDir.trim().length === 0 + ) { throw new Error( 'agent({workingDir}): must be a non-empty string naming an existing git worktree of this repository.', ); diff --git a/packages/core/src/agents/runtime/workflow-sandbox.test.ts b/packages/core/src/agents/runtime/workflow-sandbox.test.ts index 5eeff4825e7..8e77125b6f9 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.test.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.test.ts @@ -1681,6 +1681,12 @@ describe('createWorkflowSandbox security', () => { await expect( sandbox.run(`return agent("x", { workingDir: "" });`), ).rejects.toThrow(/workingDir.*non-empty string/); + // Whitespace-only used to clear both entrance gates and was refused only + // deep in the registration gate with a message blaming the directory + // instead of the argument. + await expect( + sandbox.run(`return agent("x", { workingDir: " " });`), + ).rejects.toThrow(/workingDir.*non-empty string/); }); // The two options make opposite claims about who owns the directory's diff --git a/packages/core/src/agents/runtime/workflow-sandbox.ts b/packages/core/src/agents/runtime/workflow-sandbox.ts index 4bf11fa68ce..c4e664ccc59 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.ts @@ -1403,7 +1403,7 @@ export function createWorkflowSandbox(opts: SandboxOptions): WorkflowSandbox { // about who owns the directory's lifetime, so name it here rather than // silently letting one win. if (agentOpts.workingDir !== undefined) { - if (typeof agentOpts.workingDir !== 'string' || agentOpts.workingDir.length === 0) { + if (typeof agentOpts.workingDir !== 'string' || agentOpts.workingDir.trim().length === 0) { throw new Error( "agent({workingDir}): must be a non-empty string naming an existing " + "git worktree of this repository." diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts index ebe5bb72b74..92199913d9e 100644 --- a/packages/core/src/agents/worktree-pin.test.ts +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -167,4 +167,85 @@ describe('resolveExternalWorktreeDir', () => { expect((asTool as { error: string }).error).toMatch(/^working_dir "/); expect((asOpt as { error: string }).error).toMatch(/^workingDir "/); }); + + // Canonicalise both sides or NEITHER: under a symlinked repo ancestor, an + // ABSENT pin target used to compare the canonical root against the + // verbatim spelling — manufacturing "resolves outside this repository + // (/canonical/root)" and hiding the real cause. The verbatim comparison + // lets the path reach the registration gate, which names the absence. + it('reports an absent target via the registration gate under a symlinked root', async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'wt-pin-')); + try { + const realRoot = path.join(root, 'real'); + const repoDir = path.join(realRoot, 'repo'); + await fs.mkdir(repoDir, { recursive: true }); + const linkRoot = path.join(root, 'link'); + await fs.symlink(realRoot, linkRoot); + svc.getMainWorktreePath.mockResolvedValue(path.join(linkRoot, 'repo')); + svc.isRegisteredLinkedWorktree.mockResolvedValue(false); + const localConfig = { + getTargetDir: () => path.join(linkRoot, 'repo'), + } as unknown as Config; + + const result = await resolveExternalWorktreeDir(localConfig, 'wt-x'); + expect(result).toEqual({ + error: expect.stringContaining('is not a registered linked worktree'), + }); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); + + // When the main-tree anchor is unavailable (worktree list unreadable or + // malformed), containment degrades to the CURRENT worktree's root, which + // from inside a linked worktree over-refuses legitimate sibling pins. The + // refusal must say so instead of asserting a containment problem the + // caller then retries against. + it('names the degraded anchor when the main worktree is undeterminable', async () => { + svc.getMainWorktreePath.mockResolvedValue(null); + svc.getRepoTopLevel.mockResolvedValue('/repo/.qwen/tmp/review-pr-1'); + const insideWorktree = { + getTargetDir: () => '/repo/.qwen/tmp/review-pr-1', + } as unknown as Config; + + const result = await resolveExternalWorktreeDir( + insideWorktree, + '../review-pr-2', + ); + expect(result).toEqual({ + error: expect.stringContaining( + 'main working tree could not be determined', + ), + }); + }); + + // Resolve once and thread the single resolution: the gates must see — and + // the result must bind — the exact directory object that was validated, + // not a lexical spelling a re-pointed symlink could swap out afterwards. + it('threads the canonical resolution through the gates and the result', async () => { + const root = await fs.mkdtemp(path.join(os.tmpdir(), 'wt-pin-')); + try { + const repoDir = path.join(root, 'repo'); + const wtIn = path.join(repoDir, 'wt-in'); + await fs.mkdir(wtIn, { recursive: true }); + await fs.symlink(wtIn, path.join(repoDir, 'link')); + svc.getMainWorktreePath.mockResolvedValue(repoDir); + const localConfig = { + getTargetDir: () => repoDir, + } as unknown as Config; + + const result = await resolveExternalWorktreeDir(localConfig, 'link'); + const canonical = await fs.realpath(wtIn); + expect(result).toEqual({ + path: canonical, + branch: 'pr-7', + slug: path.basename(canonical), + repoRoot: repoDir, + }); + expect(svc.isRegisteredLinkedWorktree).toHaveBeenCalledWith(canonical); + expect(svc.getRegisteredWorktreeBranch).toHaveBeenCalledWith(canonical); + } finally { + await fs.rm(root, { recursive: true, force: true }); + } + }); }); diff --git a/packages/core/src/agents/worktree-pin.ts b/packages/core/src/agents/worktree-pin.ts index aad29a04a71..18d3950d1b3 100644 --- a/packages/core/src/agents/worktree-pin.ts +++ b/packages/core/src/agents/worktree-pin.ts @@ -52,6 +52,12 @@ export interface ResolvedWorktreePin { * label; it is deliberately NOT a gate, since it returns null for a legitimate * detached-HEAD worktree. * + * The pin path is resolved ONCE and that single resolution is threaded + * through both gates and returned as `path`: re-resolving inside the gates — + * or returning the lexical spelling for the child to bind while the gates saw + * the canonical one — lets a symlink re-pointed after validation land the + * child somewhere neither gate checked. + * * @param label how to name the offending parameter in error text — `working_dir` * for the tool, `workingDir` for the workflow opt. * @returns the resolved absolute path + labels, or `{ error }` with a @@ -89,10 +95,8 @@ export async function resolveExternalWorktreeDir( // when the worktree list cannot be read; either anchor also keeps the // common-dir comparison inside getRegisteredWorktreeBranch against the // repository, not a monorepo subdirectory the parent launched from. - const repoRoot = - (await probe.getMainWorktreePath()) ?? - (await probe.getRepoTopLevel()) ?? - parentCwd; + const mainTreePath = await probe.getMainWorktreePath(); + const repoRoot = mainTreePath ?? (await probe.getRepoTopLevel()) ?? parentCwd; const wtService = repoRoot === parentCwd ? probe : new GitWorktreeService(repoRoot); @@ -101,21 +105,41 @@ export async function resolveExternalWorktreeDir( // must not silently move the file tools' boundary outside the repository. // (`isolation: 'worktree'` has this property implicitly — it always // provisions under `/.qwen/worktrees/`.) Compare canonical - // paths so a symlink cannot straddle the boundary. - const realRepoRoot = await fs.realpath(repoRoot).catch(() => repoRoot); - const realResolved = await fs - .realpath(resolvedPath) - .catch(() => resolvedPath); - const relToRepo = path.relative(realRepoRoot, realResolved); + // paths so a symlink cannot straddle the boundary — but canonicalise BOTH + // sides or NEITHER: when only one realpath succeeds (typically the pin + // target is absent), comparing a canonical root against the verbatim + // spelling manufactures a refusal that names a root the caller never typed + // and hides the real cause. The verbatim comparison lets the path reach the + // registration gate, whose message names the absence accurately. + const realRepoRoot = await fs.realpath(repoRoot).catch(() => null); + const realResolved = await fs.realpath(resolvedPath).catch(() => null); + const canonical = realRepoRoot !== null && realResolved !== null; + const containRoot = canonical ? realRepoRoot : repoRoot; + const relToRepo = path.relative( + containRoot, + canonical ? realResolved : resolvedPath, + ); if ( relToRepo === '..' || relToRepo.startsWith(`..${path.sep}`) || path.isAbsolute(relToRepo) ) { + // When the main-tree anchor was unavailable the containment check ran + // against the CURRENT worktree's root, which from inside a linked + // worktree over-refuses legitimate sibling pins and hands the caller + // guidance that cannot be satisfied — say so instead of asserting a + // containment problem that does not exist. + const degradedAnchor = + mainTreePath === null + ? ` The repository's main working tree could not be determined ` + + `(\`git worktree list\` unreadable or its path malformed), so ` + + `containment was checked against the current worktree root.` + : ''; return { error: `${label} "${resolvedPath}" resolves outside this repository ` + - `(${realRepoRoot}). Pass a worktree that lives inside the repository.`, + `(${containRoot}).${degradedAnchor} Pass a worktree that lives ` + + `inside the repository.`, }; } @@ -125,7 +149,9 @@ export async function resolveExternalWorktreeDir( // check rejects the main tree, a plain sub-directory (including a stale // registry record whose directory was recreated), a worktree belonging to // another repo, and a hand-crafted directory carrying a copied `.git` file. - if (!(await wtService.isRegisteredLinkedWorktree(resolvedPath))) { + // Thread the single resolution (see the doc block). + const pinnedPath = realResolved ?? resolvedPath; + if (!(await wtService.isRegisteredLinkedWorktree(pinnedPath))) { // Fails closed (returns false) on a git error too, so the cause is either // "not a registered linked worktree" (main tree / unregistered) or "its // git metadata could not be read" — name both rather than assert one. @@ -144,11 +170,11 @@ export async function resolveExternalWorktreeDir( // returns null for it. `branch` is unused for caller-owned worktrees anyway // (cleanup short-circuits on `externallyManaged`); it is carried only for // parity with the isolation path. - const info = await wtService.getRegisteredWorktreeBranch(resolvedPath); + const info = await wtService.getRegisteredWorktreeBranch(pinnedPath); return { - path: resolvedPath, + path: pinnedPath, branch: info?.branch ?? '', - slug: path.basename(resolvedPath), + slug: path.basename(pinnedPath), repoRoot, }; } diff --git a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts index 2a66af17ad7..566fde45240 100644 --- a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts +++ b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts @@ -300,4 +300,40 @@ describe('GitWorktreeService.getMainWorktreePath() (real git)', () => { expect(await svc.getRepoTopLevel()).toBe(nlRepo); }, ); + + // The parse check catches a remainder that is NOT attribute-shaped, but a + // remainder that itself is a record attribute (`detached`) — or a path + // ending right at a newline — parses cleanly. The anchor is only trusted + // after the round-trip: `rev-parse --git-common-dir` run at the truncated + // prefix must agree with this repository's common dir. Here the prefix + // falls inside the enclosing repository (first arm) or nowhere at all + // (second arm), so both anchors are refused and the `--show-toplevel` + // fallback keeps the path intact. + it.skipIf(process.platform === 'win32')( + 'refuses a truncated anchor whose remainder is attribute-shaped', + async () => { + const outer = initRepo('qwen-mainpath-attr-'); + const nlRepo = path.join(outer, 'sub', '\ndetached'); + fs.mkdirSync(path.dirname(nlRepo), { recursive: true }); + execFileSync('git', ['clone', '-q', outer, nlRepo], { cwd: outer }); + + const svc = new GitWorktreeService(nlRepo); + expect(await svc.getMainWorktreePath()).toBeNull(); + expect(await svc.getRepoTopLevel()).toBe(nlRepo); + }, + ); + + it.skipIf(process.platform === 'win32')( + 'refuses a truncated anchor when the main-tree path ends with a newline', + async () => { + const outer = initRepo('qwen-mainpath-trailnl-'); + const nlRepo = path.join(outer, 'sub', 'tree\n'); + fs.mkdirSync(path.dirname(nlRepo), { recursive: true }); + execFileSync('git', ['clone', '-q', outer, nlRepo], { cwd: outer }); + + const svc = new GitWorktreeService(nlRepo); + expect(await svc.getMainWorktreePath()).toBeNull(); + expect(await svc.getRepoTopLevel()).toBe(nlRepo); + }, + ); }); diff --git a/packages/core/src/services/gitWorktreeService.test.ts b/packages/core/src/services/gitWorktreeService.test.ts index ed34731f14e..2ee289107b9 100644 --- a/packages/core/src/services/gitWorktreeService.test.ts +++ b/packages/core/src/services/gitWorktreeService.test.ts @@ -619,6 +619,11 @@ describe('GitWorktreeService', () => { 'HEAD abc123\n' + 'branch refs/heads/wt\n', ); + // Round-trip validation: `--git-common-dir` answers absolute from a + // linked worktree and relative from the main tree; both resolve to the + // same common dir. + hoistedMockRaw.mockResolvedValueOnce('/repo/.git'); + hoistedMockRaw.mockResolvedValueOnce('.git'); const service = new GitWorktreeService('/repo/.qwen/worktrees/wt'); await expect(service.getMainWorktreePath()).resolves.toBe('/repo'); @@ -631,6 +636,8 @@ describe('GitWorktreeService', () => { it('accepts a bare-repository first entry', async () => { hoistedMockRaw.mockResolvedValueOnce('worktree /srv/repo.git\nbare\n'); + hoistedMockRaw.mockResolvedValueOnce('.'); + hoistedMockRaw.mockResolvedValueOnce('.'); const service = new GitWorktreeService('/srv/repo.git'); await expect(service.getMainWorktreePath()).resolves.toBe( @@ -676,5 +683,59 @@ describe('GitWorktreeService', () => { await expect(service.getMainWorktreePath()).resolves.toBeNull(); }); + + // The attribute-shape backstop: a remainder that is itself a record + // attribute (`detached`, `HEAD …`, …) — or a path ending right at a + // newline — parses cleanly, so the parse check alone cannot catch the + // truncation. The round-trip refuses the anchor: probed at the truncated + // prefix, `--git-common-dir` resolves against a DIFFERENT repository. + it('returns null when the truncated prefix belongs to another repository', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /outer/sub/\n' + + 'detached\n' + + 'HEAD abc123\n' + + 'branch refs/heads/main\n', + ); + hoistedMockRaw.mockResolvedValueOnce('.git'); + // git -C /outer/sub walks up into the enclosing repository. + hoistedMockRaw.mockResolvedValueOnce('/outer/.git'); + const service = new GitWorktreeService('/outer/sub/\ndetached'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + + it('returns null when the anchor probe fails', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /gone/repo\n' + 'HEAD abc123\n' + 'branch refs/heads/main\n', + ); + hoistedMockRaw.mockResolvedValueOnce('.git'); + hoistedMockRaw.mockRejectedValueOnce(new Error('not a git repository')); + const service = new GitWorktreeService('/gone/repo'); + + await expect(service.getMainWorktreePath()).resolves.toBeNull(); + }); + + // git preserves a path's leading/trailing whitespace verbatim in the + // porcelain output; the parse must not mutate the anchor (a trim would + // aim every subsequent gate at a different directory). + it('preserves whitespace in the main worktree path', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /srv/proj \n' + 'HEAD abc123\n' + 'branch refs/heads/main\n', + ); + hoistedMockRaw.mockResolvedValueOnce('.git'); + hoistedMockRaw.mockResolvedValueOnce('.git'); + const service = new GitWorktreeService('/srv/proj '); + + await expect(service.getMainWorktreePath()).resolves.toBe('/srv/proj '); + }); + }); + + describe('getRepoTopLevel', () => { + it('preserves whitespace in the repository top-level path', async () => { + hoistedMockRaw.mockResolvedValueOnce('/srv/proj \n'); + const service = new GitWorktreeService('/srv/proj '); + + await expect(service.getRepoTopLevel()).resolves.toBe('/srv/proj '); + }); }); }); diff --git a/packages/core/src/services/gitWorktreeService.ts b/packages/core/src/services/gitWorktreeService.ts index 6301b85d8d2..b68743a8d50 100644 --- a/packages/core/src/services/gitWorktreeService.ts +++ b/packages/core/src/services/gitWorktreeService.ts @@ -318,8 +318,13 @@ export class GitWorktreeService { */ async getRepoTopLevel(): Promise { try { - const out = await (await this.getGit()).revparse(['--show-toplevel']); - const top = out.trim(); + // `raw` rather than `revparse`: the latter trims the output, silently + // mutating a path that legitimately carries leading/trailing whitespace + // into a different anchor. Strip only the line terminator. + const out = await ( + await this.getGit() + ).raw(['rev-parse', '--show-toplevel']); + const top = out.replace(/\r?\n$/, ''); return top.length > 0 ? top : null; } catch (error) { // Caller falls back to its cwd via `?? cwd`. Log so a corrupt @@ -344,13 +349,18 @@ export class GitWorktreeService { * * The porcelain format is newline-delimited, so a main-tree path that * itself contains a newline splits across lines and the first entry - * truncates. The remainder then appears where a record attribute belongs; - * it is not one, which is how the truncation is detected — this method - * returns `null` and callers fall back to `getRepoTopLevel()`, whose - * single-value `--show-toplevel` answer keeps interior newlines intact. - * (`--porcelain -z` would be immune but needs Git >= 2.36.) A truncated - * anchor is not merely wrong: it can resolve inside a DIFFERENT repository - * whose worktree registry the gate below would then consult. + * truncates. The remainder then appears where a record attribute belongs + * — it is not one, which detects most truncations. Not all: a remainder + * that is itself attribute-shaped (`detached`, `HEAD …`, …) — or a path + * ending right at a newline, whose blank remainder ends the record — still + * parses. The parsed anchor is therefore round-trip-validated below: + * `rev-parse --git-common-dir` run AT the anchor must agree with this + * repository's own common dir, otherwise this method returns `null` and + * callers fall back to `getRepoTopLevel()`, whose single-value + * `--show-toplevel` answer keeps interior newlines intact. (`--porcelain + * -z` would be immune but needs Git >= 2.36.) A truncated anchor is not + * merely wrong: it can resolve inside a DIFFERENT repository whose + * worktree registry the caller's gate would then consult. */ async getMainWorktreePath(): Promise { try { @@ -358,15 +368,42 @@ export class GitWorktreeService { await this.getGit() ).raw(['worktree', 'list', '--porcelain']); const lines = out.split('\n'); - const firstLine = lines[0]?.trim() ?? ''; + // Strip only the line terminator (a CRLF carriage return included): + // git preserves a path's leading/trailing whitespace verbatim, and a + // trim would silently mutate it into a different, wrong anchor. + const firstLine = (lines[0] ?? '').replace(/\r$/, ''); if (!firstLine.startsWith('worktree ')) return null; for (const line of lines.slice(1)) { const attr = line.trim(); if (attr === '') break; // blank line ends the first record if (!isWorktreeListPorcelainAttribute(attr)) return null; } - const mainPath = firstLine.slice('worktree '.length).trim(); - return mainPath.length > 0 ? mainPath : null; + const mainPath = firstLine.slice('worktree '.length); + if (mainPath.length === 0) return null; + // Round-trip validation (see the doc block): `--git-common-dir` answers + // `.git` (relative) from a main tree and an absolute path from a linked + // worktree, so resolve both sides. realpath both so a symlinked + // ancestor on either side (macOS /tmp -> /private/tmp) cannot + // manufacture a mismatch. A probe failure at the anchor fails closed. + const realpathOr = async (p: string): Promise => { + try { + return await fs.realpath(p); + } catch { + return p; + } + }; + const { simpleGit } = await loadSimpleGit(); + const [ourRaw, anchorRaw] = await Promise.all([ + (await this.getGit()).raw(['rev-parse', '--git-common-dir']), + simpleGit(mainPath).raw(['rev-parse', '--git-common-dir']), + ]); + const ourCommonDir = await realpathOr( + path.resolve(this.sourceRepoPath, ourRaw.trim()), + ); + const anchorCommonDir = await realpathOr( + path.resolve(mainPath, anchorRaw.trim()), + ); + return ourCommonDir === anchorCommonDir ? mainPath : null; } catch (error) { debugLogger.warn( `getMainWorktreePath failed at ${this.sourceRepoPath}: ${error}`, diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index 39260566653..4b9ac5495c0 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -31,6 +31,10 @@ import { MAX_WORKFLOW_AGENTS_ENV, MAX_WORKFLOW_CONCURRENCY_ENV, } from '../../agents/runtime/workflow-orchestrator.js'; +import { + DEFAULT_STALL_MS, + MAX_STALL_ATTEMPTS, +} from '../../agents/runtime/workflow-stall.js'; import { MAX_TOKENS_PER_WORKFLOW_ENV } from '../../agents/runtime/workflow-budget.js'; import { WorkflowRunner, @@ -114,6 +118,13 @@ const WORKFLOW_PARAM_SCHEMA = { 'cannot serve. Mutually exclusive with `isolation`. The path must live ' + 'inside the repository and be a linked worktree registered via ' + '`git worktree add` — the main checkout is not eligible. ' + + '`stallMs` (number, ms): a no-progress watchdog, not a wall-clock cap. ' + + 'The dispatch is aborted and retried (up to ' + + `${MAX_STALL_ATTEMPTS} attempts total) after this many milliseconds ` + + 'with no observable subagent progress; the timer is suspended while ' + + 'a tool is in flight, so a legitimately slow tool is not a stall. ' + + `Default ${DEFAULT_STALL_MS}; \`0\` disables the watchdog. Wall time ` + + 'per attempt is bounded separately. ' + 'Workflow subagents always have SendMessage / Monitor / EnterPlanMode / ExitPlanMode ' + 'in their disallowed-tool floor regardless of agentType. ' + 'Concurrency: `parallel([() => agent(...), ...])` runs thunks ' + @@ -549,7 +560,7 @@ Reach for one to be comprehensive (decompose the work and cover every part in pa **Runtime** — see the \`script\` parameter for the detailed authoring contract. -\`phase(title)\`, \`log(msg)\`, \`agent(prompt, opts?)\`, \`parallel(thunks)\`, \`pipeline(items, ...stages)\`, \`workflow(nameOrRef, args?)\`, plus the \`args\` and \`budget\` globals. \`workflow()\` runs a saved workflow inline under this run's caps and nests one level only — a workflow reached through \`workflow()\` cannot call \`workflow()\` itself, and doing so throws. Saved workflows are \`.js\` files under \`/.qwen/workflows\` (project scope, also surfaced as \`/\` slash commands) or \`~/.qwen/workflows\` (user scope, lower precedence when both define the same name); \`workflow('')\` resolves against those two directories, while \`scriptPath\` takes an absolute path to a script anywhere. Default \`max(1, min(16, cpus-2))\` agents in flight per run (\`${MAX_WORKFLOW_CONCURRENCY_ENV}\`), up to ${DEFAULT_MAX_AGENTS_PER_RUN} agents total (\`${MAX_WORKFLOW_AGENTS_ENV}\`), under a 30-minute wall-clock cap per run (\`QWEN_CODE_MAX_WORKFLOW_SECONDS\`) — a fan-out near the agent cap will not fit inside the default cap. A per-run output-token cap may also be in effect: read \`budget.total\` (\`null\` = uncapped) before committing to a large fan-out, because once the cap is reached every further \`agent()\` call is refused — a bare sequential \`await agent()\` sees the rejection, while inside \`parallel()\`/\`pipeline()\` the refused slot becomes \`null\` and the script keeps running on partial results. Per-call \`agent({ schema, agentType, model, isolation: 'worktree' })\` covers structured-output contracts, declarative-agent selection, model override, and git-worktree-isolated subagents. \`resumeFromRunId\` resumes a prior run — agent() calls whose rolling prefix-hash matches the journal are served from cache for the longest unchanged prefix. Runs appear in the background-tasks view and the \`/workflows\` dialog (live phase tree, token usage, cooperative pause/resume, cancel); \`run_in_background: true\` returns a run handle immediately in the interactive TUI and delivers completion through the conversation. Scripts run in a node:vm sandbox with no filesystem or shell access — all I/O happens through the spawned agents. +\`phase(title)\`, \`log(msg)\`, \`agent(prompt, opts?)\`, \`parallel(thunks)\`, \`pipeline(items, ...stages)\`, \`workflow(nameOrRef, args?)\`, plus the \`args\` and \`budget\` globals. \`workflow()\` runs a saved workflow inline under this run's caps and nests one level only — a workflow reached through \`workflow()\` cannot call \`workflow()\` itself, and doing so throws. Saved workflows are \`.js\` files under \`/.qwen/workflows\` (project scope, also surfaced as \`/\` slash commands) or \`~/.qwen/workflows\` (user scope, lower precedence when both define the same name); \`workflow('')\` resolves against those two directories, while \`scriptPath\` takes an absolute path to a script anywhere. Default \`max(1, min(16, cpus-2))\` agents in flight per run (\`${MAX_WORKFLOW_CONCURRENCY_ENV}\`), up to ${DEFAULT_MAX_AGENTS_PER_RUN} agents total (\`${MAX_WORKFLOW_AGENTS_ENV}\`), under a 30-minute wall-clock cap per run (\`QWEN_CODE_MAX_WORKFLOW_SECONDS\`) — a fan-out near the agent cap will not fit inside the default cap. A per-run output-token cap may also be in effect: read \`budget.total\` (\`null\` = uncapped) before committing to a large fan-out, because once the cap is reached every further \`agent()\` call is refused — a bare sequential \`await agent()\` sees the rejection, while inside \`parallel()\`/\`pipeline()\` the refused slot becomes \`null\` and the script keeps running on partial results. Per-call \`agent({ schema, agentType, model, isolation: 'worktree', workingDir, stallMs })\` covers structured-output contracts, declarative-agent selection, model override, git-worktree-isolated subagents, pinning an agent to a caller-owned worktree, and the no-progress stall watchdog (\`stallMs: 0\` disables it). \`resumeFromRunId\` resumes a prior run — agent() calls whose rolling prefix-hash matches the journal are served from cache for the longest unchanged prefix. Runs appear in the background-tasks view and the \`/workflows\` dialog (live phase tree, token usage, cooperative pause/resume, cancel); \`run_in_background: true\` returns a run handle immediately in the interactive TUI and delivers completion through the conversation. Scripts run in a node:vm sandbox with no filesystem or shell access — all I/O happens through the spawned agents. **Scout first, then orchestrate** From 6fd1953fe5a8a504a9731879bf8b1290a7421b19 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 17:26:22 +0000 Subject: [PATCH 06/12] fix(workflows): type worktree-pin test stub as nullable per service (#8972) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-3 degraded-anchor test passes null to the getMainWorktreePath mock, but the vi.hoisted stub inferred Promise from its default implementation while GitWorktreeService.getMainWorktreePath() returns Promise — tsc --build failed on the clean rebuild of packages/core. Annotate the stub with the real service signature. --- packages/core/src/agents/worktree-pin.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts index 92199913d9e..ba55fecee69 100644 --- a/packages/core/src/agents/worktree-pin.test.ts +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -17,7 +17,7 @@ const stubs = vi.hoisted(() => ({ })), isGitRepository: vi.fn(async () => true), getRepoTopLevel: vi.fn(async () => '/repo'), - getMainWorktreePath: vi.fn(async () => '/repo'), + getMainWorktreePath: vi.fn(async (): Promise => '/repo'), isRegisteredLinkedWorktree: vi.fn(async () => true), getRegisteredWorktreeBranch: vi.fn(async () => ({ branch: 'pr-7' })), }), From cdcb10ac4386249a7770c3e4fae481b8cfe26be9 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Wed, 12 Aug 2026 22:55:34 +0000 Subject: [PATCH 07/12] fix(workflows): harden stallMs gate and test portability per round-4 review (#8972) - Reject non-numeric agent({stallMs}) loudly in the sandbox gate instead of silently dropping it to the default 60s watchdog, which contradicted the advertised "0 disables the watchdog". - Compare worktree paths via path.resolve/path.normalize in the new tests so the windows-latest unit lane stops failing on separator differences. - Correct the model-facing workingDir/stallMs descriptions to match the registry-only gate and the first-progress-event arming semantics. - Restore the Agent tool's historical "a sub-agent" refusal wording. --- .../agents/runtime/workflow-sandbox.test.ts | 35 +++++++++++++++++++ .../src/agents/runtime/workflow-sandbox.ts | 6 ++++ packages/core/src/agents/worktree-pin.test.ts | 11 +++--- packages/core/src/agents/worktree-pin.ts | 4 +-- .../gitWorktreeService.linked.integ.test.ts | 12 +++++-- packages/core/src/tools/workflow/workflow.ts | 14 +++++--- 6 files changed, 68 insertions(+), 14 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-sandbox.test.ts b/packages/core/src/agents/runtime/workflow-sandbox.test.ts index 8e77125b6f9..d6850b270ed 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.test.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.test.ts @@ -1705,6 +1705,41 @@ describe('createWorkflowSandbox security', () => { ).rejects.toThrow(/incompatible options/); }); + // The schema advertises "0 disables the watchdog", but a non-number used + // to be silently dropped downstream where the DEFAULT watchdog applied — + // the dispatch the author meant to leave unwatched got aborted + retried. + it('agent({stallMs}) rejects non-numeric values', async () => { + const sandbox = createWorkflowSandbox({ + args: undefined, + dispatch: async () => 'ignored', + }); + await expect( + sandbox.run(`return agent("x", { stallMs: "0" });`), + ).rejects.toThrow(/stallMs.*finite number/); + await expect( + sandbox.run(`return agent("x", { stallMs: NaN });`), + ).rejects.toThrow(/stallMs.*finite number/); + await expect( + sandbox.run(`return agent("x", { stallMs: true });`), + ).rejects.toThrow(/stallMs.*finite number/); + }); + + it('agent({stallMs: 0}) passes through and disables the watchdog', async () => { + const seen: Array<{ prompt: string; opts: unknown }> = []; + const sandbox = createWorkflowSandbox({ + args: undefined, + dispatch: async (prompt, opts) => { + seen.push({ prompt, opts }); + return 'done'; + }, + }); + const result = await sandbox.run( + `return await agent("x", { stallMs: 0 });`, + ); + expect(result).toBe('done'); + expect((seen[0].opts as { stallMs?: unknown }).stallMs).toBe(0); + }); + it('agent({isolation:"remote"}) is passed through to dispatch in P3', async () => { const seen: Array<{ prompt: string; opts: unknown }> = []; const sandbox = createWorkflowSandbox({ diff --git a/packages/core/src/agents/runtime/workflow-sandbox.ts b/packages/core/src/agents/runtime/workflow-sandbox.ts index c4e664ccc59..30ed487f66d 100644 --- a/packages/core/src/agents/runtime/workflow-sandbox.ts +++ b/packages/core/src/agents/runtime/workflow-sandbox.ts @@ -1398,6 +1398,12 @@ export function createWorkflowSandbox(opts: SandboxOptions): WorkflowSandbox { // NOTE: this init script is a host-side template literal — no // backticks anywhere below, in code or comments. // + // A non-number stallMs is silently dropped downstream and the + // default watchdog applies, contradicting "0 disables the watchdog" + // — refuse it loudly like the other option gates. + if (agentOpts.stallMs !== undefined && (typeof agentOpts.stallMs !== 'number' || !Number.isFinite(agentOpts.stallMs))) { + throw new Error("agent({stallMs}): must be a finite number of milliseconds (0 disables the watchdog)."); + } // workingDir pins the agent to a worktree the CALLER already owns; // isolation creates and reaps one. Asking for both is a contradiction // about who owns the directory's lifetime, so name it here rather than diff --git a/packages/core/src/agents/worktree-pin.test.ts b/packages/core/src/agents/worktree-pin.test.ts index ccb388736fc..ec36dc6f0e3 100644 --- a/packages/core/src/agents/worktree-pin.test.ts +++ b/packages/core/src/agents/worktree-pin.test.ts @@ -54,7 +54,7 @@ describe('resolveExternalWorktreeDir', () => { '.qwen/tmp/review-pr-7', ); expect(result).toEqual({ - path: '/repo/.qwen/tmp/review-pr-7', + path: path.resolve('/repo', '.qwen/tmp/review-pr-7'), branch: 'pr-7', slug: 'review-pr-7', repoRoot: '/repo', @@ -75,7 +75,7 @@ describe('resolveExternalWorktreeDir', () => { '../review-pr-1-base', ); expect(result).toMatchObject({ - path: '/repo/.qwen/tmp/review-pr-1-base', + path: path.resolve('/repo/.qwen/tmp/review-pr-1', '../review-pr-1-base'), }); }); @@ -85,7 +85,7 @@ describe('resolveExternalWorktreeDir', () => { it('accepts a registered worktree outside the repository directory', async () => { const result = await resolveExternalWorktreeDir(config, '/elsewhere/wt'); expect(result).toEqual({ - path: '/elsewhere/wt', + path: path.resolve('/elsewhere/wt'), branch: 'pr-7', slug: 'wt', repoRoot: '/repo', @@ -128,7 +128,10 @@ describe('resolveExternalWorktreeDir', () => { null as unknown as { branch: string }, ); const result = await resolveExternalWorktreeDir(config, 'wt'); - expect(result).toMatchObject({ path: '/repo/wt', branch: '' }); + expect(result).toMatchObject({ + path: path.resolve('/repo', 'wt'), + branch: '', + }); }); // The same resolver serves AgentTool's `working_dir` and a workflow's diff --git a/packages/core/src/agents/worktree-pin.ts b/packages/core/src/agents/worktree-pin.ts index 9a69141277d..691d0fc4909 100644 --- a/packages/core/src/agents/worktree-pin.ts +++ b/packages/core/src/agents/worktree-pin.ts @@ -117,8 +117,8 @@ export async function resolveExternalWorktreeDir( error: `${label} "${resolvedPath}" is not a registered linked worktree of ` + `this repository (it is the main working tree, is absent from \`git ` + - `worktree list\`, or its git metadata could not be read) — pinning an ` + - `agent there would not isolate it. Pass a worktree created via ` + + `worktree list\`, or its git metadata could not be read) — pinning a ` + + `sub-agent there would not isolate it. Pass a worktree created via ` + `\`git worktree add\`.`, }; } diff --git a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts index 566fde45240..c7fcbd7abbf 100644 --- a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts +++ b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts @@ -275,10 +275,16 @@ describe('GitWorktreeService.getMainWorktreePath() (real git)', () => { }); const fromWorktree = new GitWorktreeService(wt); - expect(await fromWorktree.getMainWorktreePath()).toBe(repo); - expect(await fromWorktree.getRepoTopLevel()).toBe(wt); + // Git emits forward slashes on Windows while `repo`/`wt` come from + // Node's fs APIs (backslashes); compare normalized forms so this asserts + // the resolved path, not the platform's separator style. + const mainFromWorktree = (await fromWorktree.getMainWorktreePath()) ?? ''; + expect(path.normalize(mainFromWorktree)).toBe(path.normalize(repo)); + const topFromWorktree = (await fromWorktree.getRepoTopLevel()) ?? ''; + expect(path.normalize(topFromWorktree)).toBe(path.normalize(wt)); const fromMain = new GitWorktreeService(repo); - expect(await fromMain.getMainWorktreePath()).toBe(repo); + const mainFromMain = (await fromMain.getMainWorktreePath()) ?? ''; + expect(path.normalize(mainFromMain)).toBe(path.normalize(repo)); }); // A newline inside the main-tree path splits the porcelain first entry; diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index 4b9ac5495c0..24045351c46 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -115,14 +115,18 @@ const WORKFLOW_PARAM_SCHEMA = { 'is removed. Use it when the directory the agent must work in already ' + 'exists and its uncommitted state is the point (a review worktree, a ' + 'checkout a previous step provisioned) — exactly the case isolation ' + - 'cannot serve. Mutually exclusive with `isolation`. The path must live ' + - 'inside the repository and be a linked worktree registered via ' + - '`git worktree add` — the main checkout is not eligible. ' + + 'cannot serve. Mutually exclusive with `isolation`. The path must ' + + 'be a linked worktree of this repository registered via ' + + '`git worktree add` (it may live anywhere on disk) — the main ' + + 'checkout is not eligible. ' + '`stallMs` (number, ms): a no-progress watchdog, not a wall-clock cap. ' + 'The dispatch is aborted and retried (up to ' + `${MAX_STALL_ATTEMPTS} attempts total) after this many milliseconds ` + - 'with no observable subagent progress; the timer is suspended while ' + - 'a tool is in flight, so a legitimately slow tool is not a stall. ' + + 'with no observable subagent progress once progress has begun ' + + '(a dispatch that produces no first response is bounded by the ' + + 'subagent time cap, not this watchdog); the timer is suspended ' + + 'while a tool is in flight, so a legitimately slow tool is not ' + + 'a stall. ' + `Default ${DEFAULT_STALL_MS}; \`0\` disables the watchdog. Wall time ` + 'per attempt is bounded separately. ' + 'Workflow subagents always have SendMessage / Monitor / EnterPlanMode / ExitPlanMode ' + From 5158c81523dbf71338384d60210277c09ace0887 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Thu, 13 Aug 2026 00:33:19 +0000 Subject: [PATCH 08/12] test(webui): deflake same-session refresh transcript assertion (#8972) The failing CI annotation named keeps-the-attachment-live (load variant): the live agent_message_chunk was asserted after a single-macrotask flush, but under runner contention the batched setTimeout(0) dispatch can land one tick after that window, so the chunk reads as missing even though it is delivered. Replace the fixed-depth read with a bound-wait (vi.waitFor) for the exact same blocks, preserving the assertion. Verified with 10 repeated full-file runs under load (all failing before the fix, all passing after). --- .../daemon/session/DaemonSessionProvider.test.tsx | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx b/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx index 6abe27c44d3..aeb960b293b 100644 --- a/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx +++ b/packages/webui/src/daemon/session/DaemonSessionProvider.test.tsx @@ -14600,9 +14600,15 @@ describe('DaemonSessionProvider', () => { await flushPromises(); await flushTranscriptDispatch(); }); - expect( - blocks.map((block) => ('text' in block ? block.text : undefined)), - ).toEqual(['A transcript', ' still live']); + // The chunk's path is generator wake -> consumer loop -> the batched + // setTimeout(0) dispatch. Under CI contention that flush can land one + // tick after the single-macrotask window above, so bound-wait for the + // same exact blocks instead of reading them at a fixed flush depth. + await vi.waitFor(() => { + expect( + blocks.map((block) => ('text' in block ? block.text : undefined)), + ).toEqual(['A transcript', ' still live']); + }); await act(async () => { target.reject(new Error(`${mode} refresh failed`)); From f11b7eba29f19641350e83529a1ba5a741a94e6f Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 14 Aug 2026 07:19:21 +0000 Subject: [PATCH 09/12] docs: note the workflow workingDir opt is stricter than working_dir (#8972) Co-authored-by: Qwen-Coder --- docs/users/features/sub-agents.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/users/features/sub-agents.md b/docs/users/features/sub-agents.md index b9f077837ba..8ebbd63bfbc 100644 --- a/docs/users/features/sub-agents.md +++ b/docs/users/features/sub-agents.md @@ -135,7 +135,7 @@ Use continuation for related follow-up work. Launch a new agent when the task is For a named regular subagent, `working_dir` pins the agent to an existing git worktree of the current repository. Relative paths resolve from the current directory, and the worktree must already be registered with git as a linked worktree of this repository. -A `working_dir` launch runs in the foreground because Qwen Code does not own that worktree's lifecycle. It cannot be combined with `subagent_type: "fork"` or background execution. If both `working_dir` and `isolation: "worktree"` are supplied, Qwen Code reuses the caller-owned worktree instead of creating another one. +A `working_dir` launch runs in the foreground because Qwen Code does not own that worktree's lifecycle. It cannot be combined with `subagent_type: "fork"` or background execution. If both `working_dir` and `isolation: "worktree"` are supplied, Qwen Code reuses the caller-owned worktree instead of creating another one. Workflow scripts are deliberately stricter: a workflow `agent()` call that receives both `workingDir` and `isolation` is rejected rather than run with `isolation` ignored. ## Getting Started From eaa6d6b4ccad246fc38412704b1bcab77a53d966 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Fri, 14 Aug 2026 10:27:48 +0000 Subject: [PATCH 10/12] fix(core): sanitize workingDir echo in workflow refusal errors (#8972) The workingDir refusal throw interpolated the model-authored path through JSON.stringify only, which escapes C0 but leaves DEL and the C1 range (incl. NEL U+0085) raw in the error message. Route the echo through sanitizeForErrorMessage like the sibling agentType path, and add a regression test that fails without the fix. Also address R7 review suggestions: name the QWEN_CODE_WORKFLOW_STALL_SECONDS override in the stallMs description to match every other env knob there, anchor the new workingDir/stallMs prose in the description regression test, and make the bounded runConfig test hermetic against the two new env knobs. Co-authored-by: Qwen-Coder --- .../runtime/workflow-orchestrator.test.ts | 55 ++++++++++++++++--- .../agents/runtime/workflow-orchestrator.ts | 5 +- .../core/src/tools/workflow/workflow.test.ts | 6 ++ packages/core/src/tools/workflow/workflow.ts | 3 +- 4 files changed, 60 insertions(+), 9 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index 0ba5ad3b0a0..df925e831bf 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -1907,14 +1907,29 @@ describe('createProductionDispatch', () => { }); // T11 (PR #4732 R1): subagents must be bounded so a single agent() call - // cannot loop the model indefinitely. + // cannot loop the model indefinitely. The dispatch site resolves both + // bounds from process.env, so delete the two knobs for this test — an + // operator shell exporting them must not flip this hermetic assertion. it('passes bounded runConfig (max_turns + max_time_minutes)', async () => { - const dispatch = createProductionDispatch(fakeConfig()); - await dispatch('hello', { label: 'h1' }); - expect(created[0]!.runConfig).toEqual({ - max_turns: 50, - max_time_minutes: 10, - }); + const prevTurns = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + const prevMinutes = process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + try { + const dispatch = createProductionDispatch(fakeConfig()); + await dispatch('hello', { label: 'h1' }); + expect(created[0]!.runConfig).toEqual({ + max_turns: DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, + max_time_minutes: DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + }); + } finally { + if (prevTurns === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS'] = prevTurns; + if (prevMinutes === undefined) + delete process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES']; + else process.env['QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES'] = prevMinutes; + } }); // Resolver-level tests cannot catch a revert at the dispatch site: in a @@ -3553,6 +3568,32 @@ describe('WorkflowOrchestrator P3 — agentType / model / isolation / schema', ( expect(helper.calls).toHaveLength(0); }); + // Same threat on the other interpolated half: the refusal echoes the + // model-authored `workingDir` itself, and `JSON.stringify` escapes only + // C0 (U+0000-U+001F), so DEL and the C1 range (incl. NEL U+0085) reach + // the message raw unless the echo is sanitized too. + it('workingDir refusal scrubs control characters from the echoed workingDir', async () => { + const helper = fakeConfigWithMgr({ + onCreate: async () => ({ finalText: 'unused', terminateMode: 'GOAL' }), + }); + pinStub.resolve.value = async () => ({ + error: 'not a registered linked worktree.', + }); + + let error: unknown; + try { + await createProductionDispatch(helper.config)('hi', { + workingDir: 'x\u0085forged\u007fline', + }); + } catch (caught) { + error = caught; + } + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).not.toContain('\u0085'); + expect((error as Error).message).not.toContain('\u007f'); + expect(helper.calls).toHaveLength(0); + }); + // The resolver names the offending parameter in its errors; a workflow // script never wrote `working_dir`, so it must be told the opt's own name. it('names the workflow opt, not the tool parameter, when resolving', async () => { diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.ts b/packages/core/src/agents/runtime/workflow-orchestrator.ts index e526b82b958..1bea657343d 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.ts @@ -825,8 +825,11 @@ async function runOverridePath( 'workingDir', ); if ('error' in resolved) { + // JSON.stringify escapes only C0 — sanitize the echo too so DEL / C1 + // (incl. NEL) in the model-authored path cannot fragment the message, + // the same threat the agentType SECURITY note above names. throw new Error( - `agent({workingDir: ${JSON.stringify(opts.workingDir)}}): ${sanitizeForErrorMessage(resolved.error)}`, + `agent({workingDir: ${sanitizeForErrorMessage(JSON.stringify(opts.workingDir))}}): ${sanitizeForErrorMessage(resolved.error)}`, ); } effectiveContext = createDirScopedConfigOverride(config, resolved.path); diff --git a/packages/core/src/tools/workflow/workflow.test.ts b/packages/core/src/tools/workflow/workflow.test.ts index a783d1ae041..7e50173c7ab 100644 --- a/packages/core/src/tools/workflow/workflow.test.ts +++ b/packages/core/src/tools/workflow/workflow.test.ts @@ -83,6 +83,12 @@ describe('WorkflowTool', () => { ]) { expect(description).toContain(anchor); } + // The per-call options this half of the description advertises are + // anchored too — a refactor that drops their sentences leaves the + // capabilities undiscoverable from the tool surface and no other test + // would notice. + expect(description).toContain('workingDir'); + expect(description).toMatch(/no-progress stall watchdog/); // One anchor per policy section — dropping any whole section has to // turn this test red, which is the regression it exists to catch. expect(description).toMatch(/Parallelism on its own is not a reason/); diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index 24045351c46..dc498ca923f 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -34,6 +34,7 @@ import { import { DEFAULT_STALL_MS, MAX_STALL_ATTEMPTS, + MAX_WORKFLOW_STALL_MS_ENV, } from '../../agents/runtime/workflow-stall.js'; import { MAX_TOKENS_PER_WORKFLOW_ENV } from '../../agents/runtime/workflow-budget.js'; import { @@ -127,7 +128,7 @@ const WORKFLOW_PARAM_SCHEMA = { 'subagent time cap, not this watchdog); the timer is suspended ' + 'while a tool is in flight, so a legitimately slow tool is not ' + 'a stall. ' + - `Default ${DEFAULT_STALL_MS}; \`0\` disables the watchdog. Wall time ` + + `Default ${DEFAULT_STALL_MS} (override via \`${MAX_WORKFLOW_STALL_MS_ENV}\`, whole seconds); \`0\` disables the watchdog. Wall time ` + 'per attempt is bounded separately. ' + 'Workflow subagents always have SendMessage / Monitor / EnterPlanMode / ExitPlanMode ' + 'in their disallowed-tool floor regardless of agentType. ' + From 49bfc9f500f6e77bb830fef456b318cb7fae913a Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sun, 16 Aug 2026 03:57:51 +0000 Subject: [PATCH 11/12] fix(core): name the subagent turn/time env caps in the workflow tool description (#8972) --- packages/core/src/tools/workflow/workflow.test.ts | 6 +++++- packages/core/src/tools/workflow/workflow.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/workflow/workflow.test.ts b/packages/core/src/tools/workflow/workflow.test.ts index 7e50173c7ab..1257af61742 100644 --- a/packages/core/src/tools/workflow/workflow.test.ts +++ b/packages/core/src/tools/workflow/workflow.test.ts @@ -17,6 +17,8 @@ import { DEFAULT_MAX_AGENTS_PER_RUN, MAX_WORKFLOW_AGENTS_ENV, MAX_WORKFLOW_CONCURRENCY_ENV, + WORKFLOW_SUBAGENT_MAX_MINUTES_ENV, + WORKFLOW_SUBAGENT_MAX_TURNS_ENV, } from '../../agents/runtime/workflow-orchestrator.js'; import { Storage } from '../../config/storage.js'; @@ -65,7 +67,7 @@ describe('WorkflowTool', () => { // would notice — so anchor the load-bearing claims. it('description carries both the runtime facts and the orchestration policy', () => { const { description } = new WorkflowTool(fakeConfig()); - // Every env knob the description names is anchored. The two that the + // Every env knob the description names is anchored. The four that the // orchestrator exports are anchored *through the exported constant*, so // a rename on the runtime side fails here too — a hardcoded literal // would only have caught a description-side typo, and the model would @@ -76,6 +78,8 @@ describe('WorkflowTool', () => { 'min(16, cpus-2)', MAX_WORKFLOW_AGENTS_ENV, MAX_WORKFLOW_CONCURRENCY_ENV, + WORKFLOW_SUBAGENT_MAX_TURNS_ENV, + WORKFLOW_SUBAGENT_MAX_MINUTES_ENV, 'QWEN_CODE_MAX_WORKFLOW_SECONDS', 'resumeFromRunId', '/workflows', diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index dc498ca923f..cd70cbd3c9b 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -28,8 +28,12 @@ import type { Config } from '../../config/config.js'; import type { WorkflowAgentDispatch } from '../../agents/runtime/workflow-orchestrator.js'; import { DEFAULT_MAX_AGENTS_PER_RUN, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, MAX_WORKFLOW_AGENTS_ENV, MAX_WORKFLOW_CONCURRENCY_ENV, + WORKFLOW_SUBAGENT_MAX_MINUTES_ENV, + WORKFLOW_SUBAGENT_MAX_TURNS_ENV, } from '../../agents/runtime/workflow-orchestrator.js'; import { DEFAULT_STALL_MS, @@ -565,7 +569,7 @@ Reach for one to be comprehensive (decompose the work and cover every part in pa **Runtime** — see the \`script\` parameter for the detailed authoring contract. -\`phase(title)\`, \`log(msg)\`, \`agent(prompt, opts?)\`, \`parallel(thunks)\`, \`pipeline(items, ...stages)\`, \`workflow(nameOrRef, args?)\`, plus the \`args\` and \`budget\` globals. \`workflow()\` runs a saved workflow inline under this run's caps and nests one level only — a workflow reached through \`workflow()\` cannot call \`workflow()\` itself, and doing so throws. Saved workflows are \`.js\` files under \`/.qwen/workflows\` (project scope, also surfaced as \`/\` slash commands) or \`~/.qwen/workflows\` (user scope, lower precedence when both define the same name); \`workflow('')\` resolves against those two directories, while \`scriptPath\` takes an absolute path to a script anywhere. Default \`max(1, min(16, cpus-2))\` agents in flight per run (\`${MAX_WORKFLOW_CONCURRENCY_ENV}\`), up to ${DEFAULT_MAX_AGENTS_PER_RUN} agents total (\`${MAX_WORKFLOW_AGENTS_ENV}\`), under a 30-minute wall-clock cap per run (\`QWEN_CODE_MAX_WORKFLOW_SECONDS\`) — a fan-out near the agent cap will not fit inside the default cap. A per-run output-token cap may also be in effect: read \`budget.total\` (\`null\` = uncapped) before committing to a large fan-out, because once the cap is reached every further \`agent()\` call is refused — a bare sequential \`await agent()\` sees the rejection, while inside \`parallel()\`/\`pipeline()\` the refused slot becomes \`null\` and the script keeps running on partial results. Per-call \`agent({ schema, agentType, model, isolation: 'worktree', workingDir, stallMs })\` covers structured-output contracts, declarative-agent selection, model override, git-worktree-isolated subagents, pinning an agent to a caller-owned worktree, and the no-progress stall watchdog (\`stallMs: 0\` disables it). \`resumeFromRunId\` resumes a prior run — agent() calls whose rolling prefix-hash matches the journal are served from cache for the longest unchanged prefix. Runs appear in the background-tasks view and the \`/workflows\` dialog (live phase tree, token usage, cooperative pause/resume, cancel); \`run_in_background: true\` returns a run handle immediately in the interactive TUI and delivers completion through the conversation. Scripts run in a node:vm sandbox with no filesystem or shell access — all I/O happens through the spawned agents. +\`phase(title)\`, \`log(msg)\`, \`agent(prompt, opts?)\`, \`parallel(thunks)\`, \`pipeline(items, ...stages)\`, \`workflow(nameOrRef, args?)\`, plus the \`args\` and \`budget\` globals. \`workflow()\` runs a saved workflow inline under this run's caps and nests one level only — a workflow reached through \`workflow()\` cannot call \`workflow()\` itself, and doing so throws. Saved workflows are \`.js\` files under \`/.qwen/workflows\` (project scope, also surfaced as \`/\` slash commands) or \`~/.qwen/workflows\` (user scope, lower precedence when both define the same name); \`workflow('')\` resolves against those two directories, while \`scriptPath\` takes an absolute path to a script anywhere. Default \`max(1, min(16, cpus-2))\` agents in flight per run (\`${MAX_WORKFLOW_CONCURRENCY_ENV}\`), up to ${DEFAULT_MAX_AGENTS_PER_RUN} agents total (\`${MAX_WORKFLOW_AGENTS_ENV}\`), under a 30-minute wall-clock cap per run (\`QWEN_CODE_MAX_WORKFLOW_SECONDS\`) — a fan-out near the agent cap will not fit inside the default cap. Each subagent attempt is separately capped at ${DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS} turns (\`${WORKFLOW_SUBAGENT_MAX_TURNS_ENV}\`) and ${DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES} minutes (\`${WORKFLOW_SUBAGENT_MAX_MINUTES_ENV}\`) — an attempt that hits either becomes \`null\` in \`parallel()\`/\`pipeline()\`, indistinguishable from a missing agent, so raise them for legitimately long work. A per-run output-token cap may also be in effect: read \`budget.total\` (\`null\` = uncapped) before committing to a large fan-out, because once the cap is reached every further \`agent()\` call is refused — a bare sequential \`await agent()\` sees the rejection, while inside \`parallel()\`/\`pipeline()\` the refused slot becomes \`null\` and the script keeps running on partial results. Per-call \`agent({ schema, agentType, model, isolation: 'worktree', workingDir, stallMs })\` covers structured-output contracts, declarative-agent selection, model override, git-worktree-isolated subagents, pinning an agent to a caller-owned worktree, and the no-progress stall watchdog (\`stallMs: 0\` disables it). \`resumeFromRunId\` resumes a prior run — agent() calls whose rolling prefix-hash matches the journal are served from cache for the longest unchanged prefix. Runs appear in the background-tasks view and the \`/workflows\` dialog (live phase tree, token usage, cooperative pause/resume, cancel); \`run_in_background: true\` returns a run handle immediately in the interactive TUI and delivers completion through the conversation. Scripts run in a node:vm sandbox with no filesystem or shell access — all I/O happens through the spawned agents. **Scout first, then orchestrate** From 0e365af85c0cf633fde2e90925baabe7bcc052cd Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Sun, 16 Aug 2026 06:45:34 +0000 Subject: [PATCH 12/12] fix(core): preserve trailing CR in git worktree path answers (#8972) --- .../runtime/workflow-orchestrator.test.ts | 14 +++--- .../gitWorktreeService.linked.integ.test.ts | 44 +++++++++++++++++++ .../src/services/gitWorktreeService.test.ts | 22 ++++++++++ .../core/src/services/gitWorktreeService.ts | 13 +++--- packages/core/src/tools/workflow/workflow.ts | 16 ++++--- 5 files changed, 93 insertions(+), 16 deletions(-) diff --git a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts index df925e831bf..829d0b0bea0 100644 --- a/packages/core/src/agents/runtime/workflow-orchestrator.test.ts +++ b/packages/core/src/agents/runtime/workflow-orchestrator.test.ts @@ -19,8 +19,6 @@ import { resolveSubagentMaxTimeMinutes, DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, - HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING, - HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING, } from './workflow-orchestrator.js'; import type { Config } from '../../config/config.js'; import { AgentEventType, type AgentEventEmitter } from './agent-events.js'; @@ -1918,9 +1916,11 @@ describe('createProductionDispatch', () => { try { const dispatch = createProductionDispatch(fakeConfig()); await dispatch('hello', { label: 'h1' }); + // Literal anchors, not the DEFAULT_* constants: an edit of the + // constant must fail this assertion, not move it along. expect(created[0]!.runConfig).toEqual({ - max_turns: DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS, - max_time_minutes: DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES, + max_turns: 50, + max_time_minutes: 10, }); } finally { if (prevTurns === undefined) @@ -2519,16 +2519,18 @@ describe('WorkflowOrchestrator P2 — parallel() / pipeline() / caps', () => { QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES: '45', }), ).toBe(45); + // Literal anchors for the hard ceilings — comparing against the + // HARD_* constants would assert them against themselves. expect( resolveSubagentMaxTurns({ QWEN_CODE_WORKFLOW_AGENT_MAX_TURNS: '999999', }), - ).toBe(HARD_WORKFLOW_SUBAGENT_MAX_TURNS_CEILING); + ).toBe(500); expect( resolveSubagentMaxTimeMinutes({ QWEN_CODE_WORKFLOW_AGENT_MAX_MINUTES: '999999', }), - ).toBe(HARD_WORKFLOW_SUBAGENT_MAX_MINUTES_CEILING); + ).toBe(100); }); it('per-subagent bounds reject non-decimal-integer overrides', () => { diff --git a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts index c7fcbd7abbf..c99f5424a54 100644 --- a/packages/core/src/services/gitWorktreeService.linked.integ.test.ts +++ b/packages/core/src/services/gitWorktreeService.linked.integ.test.ts @@ -342,4 +342,48 @@ describe('GitWorktreeService.getMainWorktreePath() (real git)', () => { expect(await svc.getRepoTopLevel()).toBe(nlRepo); }, ); + + // git's command stdout is LF-terminated on all platforms, so a trailing CR + // in the `--show-toplevel` / porcelain answer is part of the directory + // name, not a line terminator. Stripping it mutates the anchor into the + // CR-less sibling path — and when another repository lives there, the pin + // gate consults THAT repository's worktree registry and accepts its + // worktree. (A trailing CR is not representable on Win32.) + it.skipIf(process.platform === 'win32')( + 'preserves a trailing CR in the repository directory name', + async () => { + const base = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), 'qwen-mainpath-cr-')), + ); + tmpDirs.push(base); + const crRepo = path.join(base, 'repo\r'); + fs.mkdirSync(crRepo); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: crRepo }); + commitInitial(crRepo); + const sibling = path.join(base, 'repo'); + fs.mkdirSync(sibling); + execFileSync('git', ['init', '-q', '-b', 'main'], { cwd: sibling }); + commitInitial(sibling); + const foreignWt = path.join(sibling, 'wt'); + execFileSync('git', ['worktree', 'add', '-b', 'fbranch', foreignWt], { + cwd: sibling, + }); + const ownWt = path.join(crRepo, 'wt'); + execFileSync('git', ['worktree', 'add', '-b', 'sbranch', ownWt], { + cwd: crRepo, + }); + + const svc = new GitWorktreeService(crRepo); + const main = await svc.getMainWorktreePath(); + const top = await svc.getRepoTopLevel(); + expect(main).toBe(crRepo); + expect(top).toBe(crRepo); + // worktree-pin.ts anchors the registry gate at `main ?? top ?? cwd`: + // a mutated anchor would read the sibling's registry and accept its + // worktree as a pin target of THIS repository. + const gate = new GitWorktreeService(main ?? top ?? crRepo); + expect(await gate.isRegisteredLinkedWorktree(foreignWt)).toBe(false); + expect(await gate.isRegisteredLinkedWorktree(ownWt)).toBe(true); + }, + ); }); diff --git a/packages/core/src/services/gitWorktreeService.test.ts b/packages/core/src/services/gitWorktreeService.test.ts index 2ee289107b9..a9dfed1e641 100644 --- a/packages/core/src/services/gitWorktreeService.test.ts +++ b/packages/core/src/services/gitWorktreeService.test.ts @@ -728,6 +728,19 @@ describe('GitWorktreeService', () => { await expect(service.getMainWorktreePath()).resolves.toBe('/srv/proj '); }); + + // git's stdout is LF-terminated on all platforms, so a trailing CR in + // the porcelain answer is part of the directory name, not a terminator. + it('preserves a trailing CR in the main worktree path', async () => { + hoistedMockRaw.mockResolvedValueOnce( + 'worktree /srv/proj\r\n' + 'HEAD abc123\n' + 'branch refs/heads/main\n', + ); + hoistedMockRaw.mockResolvedValueOnce('.git'); + hoistedMockRaw.mockResolvedValueOnce('.git'); + const service = new GitWorktreeService('/srv/proj\r'); + + await expect(service.getMainWorktreePath()).resolves.toBe('/srv/proj\r'); + }); }); describe('getRepoTopLevel', () => { @@ -737,5 +750,14 @@ describe('GitWorktreeService', () => { await expect(service.getRepoTopLevel()).resolves.toBe('/srv/proj '); }); + + // git's stdout is LF-terminated on all platforms, so a trailing CR in + // the answer is part of the directory name, not a line terminator. + it('preserves a trailing CR in the repository top-level path', async () => { + hoistedMockRaw.mockResolvedValueOnce('/srv/proj\r\n'); + const service = new GitWorktreeService('/srv/proj\r'); + + await expect(service.getRepoTopLevel()).resolves.toBe('/srv/proj\r'); + }); }); }); diff --git a/packages/core/src/services/gitWorktreeService.ts b/packages/core/src/services/gitWorktreeService.ts index b68743a8d50..b58a9ebaebb 100644 --- a/packages/core/src/services/gitWorktreeService.ts +++ b/packages/core/src/services/gitWorktreeService.ts @@ -320,11 +320,13 @@ export class GitWorktreeService { try { // `raw` rather than `revparse`: the latter trims the output, silently // mutating a path that legitimately carries leading/trailing whitespace - // into a different anchor. Strip only the line terminator. + // into a different anchor. Strip only the LF terminator — git's stdout + // is LF-terminated on every platform, so a trailing CR is path data, + // not part of the terminator. const out = await ( await this.getGit() ).raw(['rev-parse', '--show-toplevel']); - const top = out.replace(/\r?\n$/, ''); + const top = out.replace(/\n$/, ''); return top.length > 0 ? top : null; } catch (error) { // Caller falls back to its cwd via `?? cwd`. Log so a corrupt @@ -368,10 +370,11 @@ export class GitWorktreeService { await this.getGit() ).raw(['worktree', 'list', '--porcelain']); const lines = out.split('\n'); - // Strip only the line terminator (a CRLF carriage return included): // git preserves a path's leading/trailing whitespace verbatim, and a - // trim would silently mutate it into a different, wrong anchor. - const firstLine = (lines[0] ?? '').replace(/\r$/, ''); + // trim would silently mutate it into a different, wrong anchor. A + // trailing CR is path data, not a terminator: git's stdout is + // LF-terminated on every platform. + const firstLine = lines[0] ?? ''; if (!firstLine.startsWith('worktree ')) return null; for (const line of lines.slice(1)) { const attr = line.trim(); diff --git a/packages/core/src/tools/workflow/workflow.ts b/packages/core/src/tools/workflow/workflow.ts index cd70cbd3c9b..63b58f54dbe 100644 --- a/packages/core/src/tools/workflow/workflow.ts +++ b/packages/core/src/tools/workflow/workflow.ts @@ -550,11 +550,17 @@ function safeStringifyDisplayPayload(payload: unknown): string { * shape — everything through one `parallel()` barrier, first answer taken at * face value. The prose below is therefore load-bearing, not documentation. * `script`'s own description carries the exact authoring contract (error - * strings, serialization rules). The agent cap and the two env knobs are - * interpolated from the orchestrator's exported constants - * (`DEFAULT_MAX_AGENTS_PER_RUN`, `MAX_WORKFLOW_AGENTS_ENV`, - * `MAX_WORKFLOW_CONCURRENCY_ENV`) in both halves, so raising a cap moves - * every model-visible copy at once — there is no prose to hand-sync. + * strings, serialization rules). Every cap and env knob is interpolated + * from exported constants, so raising a cap moves every model-visible copy + * at once — there is no prose to hand-sync. In both halves: + * `DEFAULT_MAX_AGENTS_PER_RUN`, `MAX_WORKFLOW_AGENTS_ENV`, + * `MAX_WORKFLOW_CONCURRENCY_ENV` (orchestrator exports). Runtime half only: + * the four subagent-bound constants `DEFAULT_WORKFLOW_SUBAGENT_MAX_TURNS`, + * `WORKFLOW_SUBAGENT_MAX_TURNS_ENV`, + * `DEFAULT_WORKFLOW_SUBAGENT_MAX_TIME_MINUTES`, + * `WORKFLOW_SUBAGENT_MAX_MINUTES_ENV` (orchestrator exports). Script half + * only: `DEFAULT_STALL_MS`, `MAX_STALL_ATTEMPTS`, + * `MAX_WORKFLOW_STALL_MS_ENV` (workflow-stall exports). * The wall-clock cap is the one exception: `DEFAULT_MAX_WALL_CLOCK_MS` is * private to `workflow-sandbox.ts`, so "30-minute" is still a literal here * and has to be edited alongside it. The output-token budget and the