@qwen-code /triage

Thanks for the PR!

Template: content is all there under reorganized headings (Summary / Validation / Fixes #5453). Missing the formal "Risk & Scope" section and 中文说明 — not blocking for a focused security fix, but worth including next time.

On direction: this is a clear-cut security fix. The shell tool's directory parameter is a safety gate before auto/YOLO execution, and the naive startsWith prefix check lets sibling paths like /tmp/project-other slip through when the workspace is /tmp/project. Issue #5453 describes the exact scenario. Solid alignment with project security posture.

On approach: the fix is exactly right — WorkspaceContext.isPathWithinWorkspace() already exists and is used by grep, read-file, glob, monitor, ripGrep, and ls for the same purpose. It uses path.relative() under the hood, which correctly handles path boundaries (no false prefix matches, handles symlinks, handles .. traversal). Replacing the bespoke startsWith with the canonical method is the minimal correct fix. Scope is tight: 2 files, +25/-6, no drive-by changes. Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

The fix is clean and minimal. Two observations from reading the code:

Independent proposal: I would have done exactly the same thing — replace the bespoke startsWith with the existing WorkspaceContext.isPathWithinWorkspace() that's already used by grep, read-file, glob, monitor, ripGrep, and ls. It uses path.relative() + boundary checks under the hood, which is the correct approach.

Diff comparison: the PR's implementation matches my proposal. The production change is 4 lines deleted, 2 lines added — swaps the manual loop+startsWith for a single method call. The regression test properly mocks isPathWithinWorkspace returning false and asserts the error message + that the method was called with the sibling-prefix path. No correctness issues, no style violations, follows project conventions.

One note on the test: the mock's default isPathWithinWorkspace implementation (from createMockWorkspaceContext) also uses startsWith — same bug pattern. The test overrides it explicitly with mockReturnValue(false), which is fine. Worth knowing for future test authors.

Testing

Unit tests (240 tests, all pass)

$ npx vitest run src/tools/shell.test.ts
 ✓ src/tools/shell.test.ts (240 tests) 1478ms
 Test Files  1 passed (1)
      Tests  240 passed (240)

Note: the new regression test from this PR (should reject sibling-prefix) isn't in main yet (cross-repo fork), so it can't be run from main. The existing 240 tests all pass.

Real-scenario verification

Direct verification of isPathWithinRoot (the method the fix delegates to) against the exact scenario from issue #5453:

$ node tmp/triage-test-102240/fix-verify.mjs
Direct verification of isPathWithinRoot (the fix):
  sibling-prefix /tmp/test-workspace-other: PASS (rejected)
  child /tmp/test-workspace/subdir: PASS (allowed)
  workspace itself /tmp/test-workspace: PASS (allowed)
  parent /tmp: PASS (rejected)
  traversal /tmp/test-workspace/../other: PASS (rejected)
  unrelated /etc: PASS (rejected)

Before/after comparison using the same test script:

$ node tmp/triage-test-102240/test-fix.mjs
=== Shell Directory Validation: Before vs After Fix ===

Workspace: /tmp/test-workspace

Sibling prefix: /tmp/test-workspace-other
  Before (startsWith):  ALLOWED ✗ (bug!)
  After  (path.relative): REJECTED ✓

Child path: /tmp/test-workspace/subdir
  Before (startsWith):  ALLOWED ✓
  After  (path.relative): ALLOWED ✓

All cases behave correctly with the fix. The sibling-prefix bug is resolved, and legitimate child paths still work.

— Qwen Code · qwen3.7-max

This is a textbook minimal security fix. The PR does exactly what I would have done: swap the bespoke startsWith check for the canonical isPathWithinWorkspace() that six other tools already use. No new abstractions, no extra scope, no drive-by changes.

The before/after verification confirms the bug is real and the fix works — sibling-prefix paths get rejected, child paths and the workspace itself still work. All 240 existing unit tests pass.

The one thing I'd flag for the author's awareness (not a blocker): the mock helper createMockWorkspaceContext has the same startsWith pattern in its default isPathWithinWorkspace implementation. The test works around this by overriding with mockReturnValue(false), but future tests that rely on the mock's default behavior would inherit the same bug. That's a separate cleanup, not something this PR needs to take on.

Approving. ✅

— Qwen Code · qwen3.7-max

✅ Local verification — safe to merge

I verified this PR locally before merging. Tested PR head c88da853 on its base 5924ab3a, in an isolated git worktree with a clean npm ci (Node v22.22.2 / npm 10.9.7), with the suites driven under tmux.

Note: I deliberately used a fresh worktree of the PR head rather than my local branch — my working branch has diverged from main on shell.ts and workspaceContext.ts, which would have made the results unfaithful to what actually lands on main.

Results — all green

The fix is load-bearing (before / after)

Running the new regression test against the pre-fix shell.ts:

× ShellTool > build > should reject sibling-prefix directories outside the workspace
  AssertionError: expected [Function] to throw an error

That is exactly the bug: the old params.directory.startsWith(wsDir) accepts /tmp/project-other for workspace /tmp/project. With the fix in place, it correctly throws and the test passes. ✔️

One nuance worth flagging (non-blocking)

The committed regression test mocks the very gate it depends on:

• shell.test.ts:338 → vi.mocked(workspaceContext.isPathWithinWorkspace).mockReturnValue(false), and
• createMockWorkspaceContext's default isPathWithinWorkspace is itself the buggy startsWith.

So the committed test proves that shell.ts delegates to and respects isPathWithinWorkspace — but it does not exercise the real sibling-prefix rejection. There is also no non-mocked sibling-prefix test elsewhere (none in workspaceContext.test.ts).

To close that gap, I ran a throwaway end-to-end test using a real WorkspaceContext backed by real on-disk directories (…/project, …/project-other, …/project/sub) — 8/8 passed:

• real isPathWithinWorkspace → rejects the sibling, accepts the workspace dir and a real child (guarded by a sanity assertion that the workspace actually registered the dir, so the rejection cannot pass vacuously);
• real ShellTool.build() → throws for the sibling, succeeds for the workspace dir and child.

The real behavior is correct because isPathWithinRoot already uses path.relative + .. checks. Optional suggestion (low priority): add one non-mocked case — e.g. /tmp/project vs /tmp/project-other in workspaceContext.test.ts — to lock in the real boundary, since the shell-tool test cannot catch a regression in the gate itself.

Verdict

The change is correct, minimal, and well-scoped; the bug is real and the fix closes it. All checks pass. LGTM 👍

× ShellTool > build > should reject sibling-prefix directories outside the workspace
  AssertionError: expected [Function] to throw an error