fix: normalize empty working_dir to unset when isolation:worktree is set on AgentTool

[mvanhorn]

What this PR does

Normalizes an empty working_dir to unset when isolation: "worktree" is requested on the agent tool, so a model that emits working_dir: "" can still start a worktree-isolated sub-agent.

AgentTool.validateToolParams (packages/core/src/tools/agent/agent.ts) now treats an empty or whitespace-only working_dir as absent when isolation === 'worktree', then defers to the existing isolation path. Because BaseDeclarativeTool.build() (packages/core/src/tools/tools.ts) validates and constructs the invocation from the same params object, unsetting working_dir there is enough; createInvocation already branches on working_dir truthiness.

Closes #7316

Why it's needed

Some OpenAI-compatible models emit working_dir: "" even when asked to omit it. The current validation rejects any set working_dir that is empty before it checks isolation, so the call fails with Parameter "working_dir" must be a non-empty string when set. and those models can never use worktree isolation. The repo's own triage bot confirmed this root cause on the issue and pointed at the exact function.

Every other combination keeps its current behavior: an empty working_dir with no isolation is still rejected, and a non-empty working_dir alongside run_in_background/fork is still rejected.

Reviewer Test Plan

Covered by new unit tests in agent.test.ts plus the manual steps below; the package's targeted unit tests pass locally.

How to verify

1. Invoke the agent tool with isolation: "worktree" and working_dir: "" (or a whitespace-only string).
2. Before this change: the call fails with Parameter "working_dir" must be a non-empty string when set.
3. After this change: the empty working_dir is dropped and the sub-agent runs in an isolated worktree.
4. Confirm the guardrails still hold: working_dir: "" with no isolation is still rejected.

Added packages/core/src/tools/agent/agent.test.ts cases covering the empty-working_dir + isolation: worktree acceptance and the no-isolation rejection.

Evidence (Before & After)

• Before: validateToolParams returns the must be a non-empty string when set error, so the worktree path is never reached.
• After: validateToolParams returns no error for the empty-working_dir + worktree case and the invocation proceeds with working_dir unset.

Tested on

OS	Status
🍏 macOS	✅
🪟 Windows
🐧 Linux

Environment (optional)

Ran the package's targeted unit tests for the changed files locally.

Risk & Scope

• Main risk or tradeoff: none expected; the normalization is gated on isolation === 'worktree' plus an empty/whitespace working_dir.
• Not validated / out of scope: no change to non-worktree isolation modes or to the run_in_background/fork validation.
• Breaking changes / migration notes: none; previously-rejected valid calls now succeed and every previously-rejected invalid call is still rejected.

Linked Issues

Closes #7316

AI was used for assistance.

[qwen-code-ci-bot]

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

[qwen-code-ci-bot]

[Suggestion] validateToolParams mutates its input (params.working_dir = undefined), and the getToolDescription path in agent-core.ts passes the live args object through build(args) → validateToolParams. This means TOOL_CALL event emissions and pre-tool hooks silently see working_dir: undefined instead of the model-supplied "".

No functional breakage — the normalization is semantically correct — but event logs will differ from model output, which could confuse debugging (e.g. an investigator comparing model output to event logs would see an unexplained disappearance of the working_dir field).

Consider moving the normalization into createInvocation (and adjusting the validation block below to skip the empty-string check when isolation === 'worktree'), so validateToolParams stays a pure validator. Alternatively, structuredClone in getToolDescription would match the scheduler's hygiene.

— qwen3.7-max via Qwen Code /review

[wenshao]

Review — normalize empty working_dir under isolation: "worktree"

Overview

Small, well-scoped fix. When isolation: "worktree" is requested and a model emits an empty/whitespace working_dir, validateToolParams now unsets it (params.working_dir = undefined) instead of rejecting the call, then defers to the existing worktree path. Every other combination keeps its current behavior. Closes #7316.

Correctness — looks right, and the mutation is load-bearing

• I traced why unsetting (rather than merely accepting the empty string) is required. The invocation reads working_dir with strict !== undefined checks — agent.ts:2041, :2392, :2477 — not truthiness. If "" survived into the invocation, "" !== undefined is true, so the runtime would treat it as a caller-owned worktree pin and fail downstream (e.g. resolveWorkingDir(""), or the background-incompatibility guard). Setting it to undefined is what makes the empty case behave as "unset". Good call.
• The block is correctly gated: isolation === 'worktree' and string and trim().length === 0. Non-string values still fall through to the existing "must be a non-empty string" rejection, and an empty working_dir with no isolation is still rejected. Guardrails intact.
• Ordering is fine: the isolation block runs first, so empty working_dir + worktree still requires an explicit, non-fork subagent_type.
• Interaction with createInvocation is consistent — it branches on params.working_dir truthiness and drops isolation only when a real dir is present. After normalization working_dir is undefined (falsy), so isolation: "worktree" is preserved and the isolated-worktree path runs. Correct.

Conventions

• Mutating params inside validation matches the repo's own pattern — read-file.ts:629 sets params.pages = … : undefined, and glob/edit/ls/grep normalize paths in validateToolParamValues the same way. So this isn't a new smell.

Tests

• Coverage is solid: accepts empty and whitespace-only working_dir under worktree, and the createInvocation test locks in the key regression (working_dir ends up undefined, isolation stays worktree) while mirroring the real build() order (validate → createInvocation).
• Nice-to-have (non-blocking): a case for empty working_dir + isolation: worktree + run_in_background: true — it should now be accepted, since the working_dir+background guard is skipped once the field is unset; worth pinning that explicitly. Optionally one asserting empty working_dir + worktree still needs a subagent_type.

Minor

• PR body says it "Added … the no-isolation rejection" test. That guardrail is actually already covered by the pre-existing rejects an empty working_dir / rejects a whitespace-only working_dir tests — no new test adds it. Cosmetic.

Verdict

LGTM. Minimal, correctly gated, and tested; no security or performance concerns (validation-only, single branch). The two extra test cases above would round it out but aren't required.

---

_{🤖 Reviewed with Claude Code · Opus 4.8}

[doudouOUC]

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

[mvanhorn]

Thanks for this one as well, @wenshao - empty working_dir normalizing to unset makes worktree isolation behave predictably.