Skip to content

fix(mcp): keep path anchoring when the server has no multiIndexer - #645

Merged
zzet merged 1 commit into
zzet:mainfrom
tiendungdev:fix/resolve-symbol-id-without-multiindexer
Aug 21, 2026
Merged

fix(mcp): keep path anchoring when the server has no multiIndexer#645
zzet merged 1 commit into
zzet:mainfrom
tiendungdev:fix/resolve-symbol-id-without-multiindexer

Conversation

@tiendungdev

Copy link
Copy Markdown
Contributor

Root cause

resolveSymbolID has three rungs: exact hit, cwd-prefix, then graphRelID. The multiIndexer == nil early return sits above the last two — but only the cwd rung actually needs the multi-repo index (it maps a directory to a repo prefix). graphRelID does not, and it was being skipped for free.

Probed on windows/amd64 with the move/inline fixture, which builds its Server through NewServer(...) with no MultiRepoOptions:

multiIndexer == nil:               true
GetNode("pkga/a.go::Foo"):         false
resolveSymbolID("pkga/a.go::Foo")  "pkga/a.go::Foo"   <- returned unchanged
graphRelID("pkga/a.go::Foo")       "pkga\a.go::Foo"
GetNode(graphRelID):               true                <- resolves
graphRelPath("pkga/a.go")          "pkga\a.go"         <- needs no multiIndexer

The machinery was already there and already deliberate — graphPathSpelling's doc-comment names this exact failure:

a forward-slash path — the spelling every agent writes — missed every node below the repo root on Windows and the read tools answered file_not_indexed for an indexed file

The rung was simply unreachable for a server built without MultiRepoOptions.

Change

Scope the guard to the cwd rung. Additive by construction: the graphRelID result stays gated on GetNode(rel) != nil, so it can only turn a miss into a hit — it can never redirect an id that already resolves, and rung 1 still short-circuits an exact match before anything else runs.

Effect

Whole internal/mcp package on windows/amd64, go1.26.6, -count=1:

failures
main (b8b13ca7) 40
with this change 27

Diffing the failing test names rather than the counts: exactly 13 flip, and the newly-broken set is empty.

FIXED:  TestMoveSymbol_{SamePackage_FunctionRelocated, TargetFileMissing_CreatesIt,
        DryRun_NoOnDiskChanges, CrossPackage_RewritesThirdPartyCaller,
        CrossPackage_CallerInTarget_BecomesBare, CrossPackage_CallerInSource_GainsImport}
        TestInlineSymbol_{TrivialAccessor, TwoCallsites_BothRewritten,
        ArgumentSubstitution, SideEffectArgRefuses, DeleteAfterFalse_KeepsCallee,
        RefusesDefer, RefusesMultipleReturns}
NEWLY BROKEN: (none)

Test

TestResolveSymbolID_WithoutMultiIndexer_StillAnchorsThePath uses an absolute-path id rather than the Windows separator. That exercises the same rung on every platform, so the linux/macos matrix protects this fix and no new windows CI step is needed.

Sabotage-verified: restoring the early return fails both the new unit test (on the anchoring assertion) and TestMoveSymbol_SamePackage_FunctionRelocated; reverting the sabotage returns both to green.

Scope of the user-visible impact — stated narrowly on purpose

The real daemon is not affected: internal/serverstack/shared_server.go:620 passes multiOpts.... The production call sites that build a Server without MultiRepoOptions are cmd/gortex/eval_recall.go:247 and cmd/gortex/eval_server.go:71. So on Windows this cost the eval surfaces their path anchoring, and it cost the test suite 13 tests. I would rather say that plainly than sell it as a broken daemon.

Verification

  • golangci-lint run ./internal/mcp/... reports 6 staticcheck SA5011 findings — pre-existing, byte-identical with this change stashed. Untouched here.
  • git diff --check clean.

Branched from main at b8b13ca7. Windows 11, go1.26.6.

Related, not included: the remaining 27 windows failures in this package split into symlink-privilege cases (needs SeCreateSymbolicLinkPrivilege, environment rather than code) and further POSIX-spelled assertions, e.g. TestPathToFileURI_Absolute expecting file:///work/main.go where Windows resolves file:///D:/work/main.go. Happy to take those separately if they are worth having.

resolveSymbolID returned early on a nil multiIndexer, before the
graphRelID rung. Only the cwd rung needs the multi-repo index — it maps a
directory to a repo prefix — so the early return also discarded the path
anchoring that works without one.

Measured on windows/amd64 with the move/inline fixture, which builds its
Server through NewServer(...) with no MultiRepoOptions:

    multiIndexer == nil:              true
    GetNode("pkga/a.go::Foo"):        false
    resolveSymbolID("pkga/a.go::Foo") "pkga/a.go::Foo"   <- unchanged
    graphRelID("pkga/a.go::Foo")      "pkga\a.go::Foo"
    GetNode(graphRelID):              true                <- resolves

graphRelPath/graphPathSpelling already reconcile the spelling, and their
doc-comment names this exact failure: a forward-slash path is "the
spelling every agent writes", and without the rewrite it "missed every
node below the repo root on Windows". The rung was simply unreachable
for a server built this way.

Scope the guard to the cwd rung. The change is additive: the graphRelID
result is still gated on GetNode(rel) != nil, so it can only turn a miss
into a hit, never redirect a resolvable id.

Whole package on windows before/after: 40 -> 27 failures. Diffing the
failing test names, exactly the 13 TestMoveSymbol_* / TestInlineSymbol_*
cases flip and nothing else changes state.

The regression test uses an absolute-path id rather than the Windows
separator, so it exercises the rung on every platform and the
linux/macos matrix protects the fix without a new windows CI step.
@zzet
zzet merged commit c982cf5 into zzet:main Aug 21, 2026
11 checks passed
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.

2 participants