Skip to content

fix: resolve search delegate paths against workspace root to prevent doubled segments#476

Merged
buger merged 1 commit intomainfrom
fix/search-delegate-doubled-paths
Mar 4, 2026
Merged

fix: resolve search delegate paths against workspace root to prevent doubled segments#476
buger merged 1 commit intomainfrom
fix/search-delegate-doubled-paths

Conversation

@buger
Copy link
Collaborator

@buger buger commented Mar 4, 2026

Summary

  • Fix doubled paths in search delegate: The subagent runs from workspace root (allowedFolders[0]) but returned paths were being resolved against searchPaths[0] (the project subdirectory). This caused paths like /workspace/project/project/src/file.go when the subagent returned workspace-relative paths (e.g. project/src/file.go).
  • Add path validation safety net: After resolution, validate paths with existsSync and auto-repair doubled directory segments or try alternative resolution bases — catches edge cases and AI path hallucinations.
  • Update tests: Fix existing assertions to match new resolution base and add a test covering the real-world scenario (workspace-relative paths with project dir prefix).

Trace: cb1bb8d74c4d93fbff8ccaf1e63028abexplore-code probe delegation returns targets with project prefix, which got doubled when joined with searchPaths[0].

Root Cause

  1. Parent calls searchTool(searchDelegate: true) with pathsearchPaths[0] = /tmp/.../slack-xxx/tyk-analytics-ui
  2. Subagent spawned with path: allowedFolders[0] → workspace root /tmp/.../slack-xxx
  3. Subagent returns: tyk-analytics-ui/app/pages/... (relative to workspace root)
  4. Bug: resolveTargetPath("tyk-analytics-ui/app/...", "/tmp/.../tyk-analytics-ui")DOUBLED
  5. Fix: Resolve against delegateBase (workspace root) instead of searchPaths[0]

Test plan

  • Updated existing search-delegate test assertions to match new resolution base
  • Added test: workspace-relative paths with project dir prefix resolve correctly (no doubling)
  • All 6 search-delegate tests pass
  • Full test suite: 106 suites, 2637 tests pass

🤖 Generated with Claude Code

… path

The search delegate subagent runs from workspace root (allowedFolders[0])
but returned paths were being resolved against searchPaths[0] (the
project subdirectory). This caused doubled path segments like
/workspace/project/project/src/file.go when the subagent returned
workspace-relative paths.

Fix the resolution base to match where the delegate actually runs, and
add a validation layer that detects/repairs doubled segments and tries
alternative resolution bases as a safety net for AI path hallucinations.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@buger buger merged commit 1ba138c into main Mar 4, 2026
14 checks passed
@probelabs
Copy link
Contributor

probelabs bot commented Mar 4, 2026

Summary

This PR fixes a path resolution bug in the search delegate that caused doubled directory segments when subagents returned workspace-relative paths.

Files Changed Analysis

  • npm/src/tools/vercel.js: +66/-5 lines - Core fix implementation
  • npm/tests/unit/search-delegate.test.js: +13/-10 lines - Updated test assertions

Architecture & Impact Assessment

What This PR Accomplishes

  1. Fixes doubled path segments: Subagent runs from workspace root (allowedFolders[0]) but returned paths were incorrectly resolved against searchPaths[0] (project subdirectory), causing paths like /workspace/project/project/src/file.go

  2. Adds path validation safety net: After resolution, validates paths with existsSync and auto-repairs:

    • Doubled directory segments (e.g., /ws/proj/proj/src/ws/proj/src)
    • Alternative resolution bases for edge cases
    • Logs warnings for potentially invalid paths

Key Technical Changes

  • New delegateBase variable uses allowedFolders[0] or cwd as resolution base instead of searchPaths[0]
  • New splitTargetSuffix() helper function to separate file paths from line/anchor suffixes
  • Multi-step validation pipeline: exists check → doubled segment repair → alternative base resolution → fallback with warning

Flow Diagram

flowchart TD
    A[Subagent returns paths] --> B[Resolve against delegateBase]
    B --> C{existsSync?}
    C -->|Yes| D[Use path]
    C -->|No| E{Doubled segment?}
    E -->|Yes| F[Repair & validate]
    F -->|Valid| D
    E -->|No| G[Try alternative bases]
    G -->|Found| D
    G -->|Not found| H[Keep with warning]
Loading

Affected Components

  • searchTool() function in vercel.js
  • Search delegate path resolution logic
  • Extract tool invocation with resolved paths

Scope Discovery & Context Expansion

Related Files to Review

  • npm/src/tools/common.js - Contains resolveTargetPath and parseAndResolvePaths utilities
  • npm/src/delegate.js - Subagent spawning logic
  • Other tests in npm/tests/unit/ that may use similar path patterns

Potential Edge Cases

  • Windows drive letter handling in splitTargetSuffix
  • Paths with multiple consecutive identical segments
  • Mixed absolute/relative path returns from subagent

Tags

  • tags.review-effort: 2 - Focused fix with clear test coverage, validation logic is straightforward
  • tags.label: bug
Metadata
  • Review Effort: 2 / 5
  • Primary Label: bug

Powered by Visor from Probelabs

Last updated: 2026-03-04T15:32:21.873Z | Triggered by: pr_opened | Commit: b5a4a92

💡 TIP: You can chat with Visor using /visor ask <your question>

@probelabs
Copy link
Contributor

probelabs bot commented Mar 4, 2026

Security Issues (3)

Severity Location Issue
🟢 Info npm/src/tools/vercel.js:319-328
The doubled segment detection only checks for consecutive identical segments (parts[i] === parts[i + 1]). This misses cases where an attacker could craft paths like '/workspace/proj/PROJ/file' (case variation) or '/workspace/proj/proj2/file' (partial match) that might bypass the naive equality check while still causing path confusion issues.
💡 SuggestionConsider normalizing paths to lowercase before comparison on case-insensitive filesystems, or document that this is a best-effort auto-fix for AI hallucinations rather than a security boundary.
🟢 Info npm/src/tools/vercel.js:122-130
The splitTargetSuffix function is duplicated from resolveTargetPath in common.js. Both functions implement the same logic for splitting file paths from line numbers/symbols. This duplication could lead to maintenance issues where one is updated but not the other.
💡 SuggestionExport splitTargetSuffix from common.js and import it in vercel.js instead of duplicating the logic. This ensures consistent path parsing behavior across the codebase.
🟡 Warning npm/src/tools/vercel.js:299-348
The new path resolution logic resolves paths against 'delegateBase' (workspace root) and then validates with existsSync, but there is no explicit check that the resolved path is within allowedFolders. If a malicious or compromised subagent returns a path like '/etc/passwd' or '../../../etc/passwd', the resolveTargetPath function will resolve it, and if the file happens to exist, it will be included in validatedTargets without verifying it's within the allowed workspace boundaries.
💡 SuggestionAdd explicit path boundary validation after resolution. Before adding to validatedTargets, verify the resolved filePart is within allowedFolders using the existing isPathAllowed pattern from edit.js or by checking that the resolved path starts with the allowed folder path + separator. Example: const resolvedAbs = resolve(delegateBase, filePart); if (!allowedFolders.some(f => resolvedAbs.startsWith(f + sep) || resolvedAbs === f)) { continue; }

Performance Issues (1)

Severity Location Issue
🟠 Error contract:0
Output schema validation failed: must have required property 'issues'

Security Issues (3)

Severity Location Issue
🟢 Info npm/src/tools/vercel.js:319-328
The doubled segment detection only checks for consecutive identical segments (parts[i] === parts[i + 1]). This misses cases where an attacker could craft paths like '/workspace/proj/PROJ/file' (case variation) or '/workspace/proj/proj2/file' (partial match) that might bypass the naive equality check while still causing path confusion issues.
💡 SuggestionConsider normalizing paths to lowercase before comparison on case-insensitive filesystems, or document that this is a best-effort auto-fix for AI hallucinations rather than a security boundary.
🟢 Info npm/src/tools/vercel.js:122-130
The splitTargetSuffix function is duplicated from resolveTargetPath in common.js. Both functions implement the same logic for splitting file paths from line numbers/symbols. This duplication could lead to maintenance issues where one is updated but not the other.
💡 SuggestionExport splitTargetSuffix from common.js and import it in vercel.js instead of duplicating the logic. This ensures consistent path parsing behavior across the codebase.
🟡 Warning npm/src/tools/vercel.js:299-348
The new path resolution logic resolves paths against 'delegateBase' (workspace root) and then validates with existsSync, but there is no explicit check that the resolved path is within allowedFolders. If a malicious or compromised subagent returns a path like '/etc/passwd' or '../../../etc/passwd', the resolveTargetPath function will resolve it, and if the file happens to exist, it will be included in validatedTargets without verifying it's within the allowed workspace boundaries.
💡 SuggestionAdd explicit path boundary validation after resolution. Before adding to validatedTargets, verify the resolved filePart is within allowedFolders using the existing isPathAllowed pattern from edit.js or by checking that the resolved path starts with the allowed folder path + separator. Example: const resolvedAbs = resolve(delegateBase, filePart); if (!allowedFolders.some(f => resolvedAbs.startsWith(f + sep) || resolvedAbs === f)) { continue; }
\n\n \n\n

Performance Issues (1)

Severity Location Issue
🟠 Error contract:0
Output schema validation failed: must have required property 'issues'
\n\n ### Quality Issues (1)
Severity Location Issue
🟠 Error contract:0
Output schema validation failed: must have required property 'issues'

Powered by Visor from Probelabs

Last updated: 2026-03-04T15:32:06.284Z | Triggered by: pr_opened | Commit: b5a4a92

💡 TIP: You can chat with Visor using /visor ask <your question>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant