fix(nav): plain Go method names resolve everywhere; session_start per-agent LSP state and detached HEAD - #559
Merged
Merged
Conversation
… HEAD and server find_references and get_definition answered "No symbol named" for a plain Go method name, because gopls names methods "(*Recv).Method" and the shared resolver only matched that shape through a dotted query. read_symbol only found it through its tree-sitter fallback. The resolver now matches a plain name against a flat Go method's own name, so every tool that takes a symbol name agrees. A shared name still matches every symbol: the read-only tools list each, rename_symbol and move_symbol refuse, and topology_impact's cross-file callers report nothing rather than another receiver's callers. session_start keyed its Branch line and git-policy section on a branch name, so a detached HEAD, the usual review-worktree setup, lost both. They now show "detached at <short sha>" and the policy. The GOWORK=off, warm-up and diagnostics-mode lines described the connection's primary language server, so a subagent pinned to a worktree was not told its server runs with GOWORK=off and one on the main checkout was told it did. They now ask about the workspace session_start resolved for the caller. docs/configuration.md says a go.work edit reaches gopls only after plumb restart, and the paragraph moves to its own [lsp.<language>] subsection so it no longer reads as covering the [git] env composition rules. Fixes #546 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: still-spruce
…nt before its server starts Review of #559 found two regressions in the first cut. Matching a plain name against gopls's flat "(*S).Run" made a function literally named Run ambiguous with that method, so move_symbol (and rename_symbol) refused a move main performs, and suggested a name_path form the refusal never spelled out. The resolver now prefers a symbol carrying the name itself and falls back to receiver-stripped methods only when none does, so only equally good matches stay ambiguous. move_symbol's refusal now lists the Receiver/Method name_path for each match, which the topology index resolves. topology_impact picks the method its node names by line instead of giving up on a shared name such as Close. The per-agent session_start lines only worked once the worktree's server was running, but a per-agent re-pin starts none: a subagent's first session_start got no GOWORK line and "LSP is ready" for the connection's server. The packet now asks whether the caller's own server has started, says when it has not, and names the go.work it will start with GOWORK=off against, decided from disk the way the pool decides it. The test no longer pre-seeds the worktree's server; a seeded variant covers the running case. The docs now say an idle teardown also re-decides GOWORK, and describe rename_symbol's line/character fallback and move_symbol's name_paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Plumb-Session: bright-horse
…neric receivers
For `func (s *S[T]) Run`, move_symbol's refusal offered the name_path
`S[T]/Run`, and passing it moved the wrong method. The Go topology extractor
had no case for an instantiated generic type, so the receiver rendered as `_`
and the node's Qualified came out as `(*_).Run`. topologyNodeByPath then found
no parent matching `S[T]` and silently fell back to the first node named
`Run`, which was another type's method.
Three fixes, root cause first:
- topologyNodeByPath no longer falls back. A Parent/Name path whose parent is
not evidenced matches nothing; a plain name is unchanged. A parent is
evidenced by the node's Qualified (the Go extractor) or, for the extractors
that record a member under its bare name (Python, Java, Rust, Kotlin), by a
node of that name whose span encloses it, so the fallback still resolves
Class/method there, and "Other/run" no longer answers with Greeter's run. An
empty segment matches nothing. resolveSymbolOrFallback says the fallback
found nothing either when the language server failed to answer, since that
server's own error ("retry shortly") says nothing about the path.
- typeStr renders IndexExpr and IndexListExpr as the base type, so the method
is `(*S).Run`, and the hint offers `S/Run` with the type parameters stripped.
- The offered name_paths were honest only through the topology index: gopls
never nests a method, so findSymbolByPath("A/Run") missed and a server with
no index wired answered "not found". findSymbolByPath now resolves
Recv/Method against gopls's flat `(*Recv).Method` symbols, and moveNamePaths
offers a path only once findSymbolByPath is seen to return exactly that
match, so a symbol nested two deep gets the generic hint, not a path that
errors.
The CHANGELOG and docs/tools.md claimed equally good matches are never
resolved silently. The exact tier hides `(*T).Run` behind any symbol literally
named Run (a struct field, an interface method), so both now say so.
Also pins the GOWORK prediction session_start makes for a server that has not
started to what the pool then spawns, across every outcome of the decision.
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…actor The hand-built nodes proved the rule; the Python extractor proves it holds for a language whose nodes record a member under its bare name. Without containment the no-fallback change would refuse every Class/method path such a language resolves today, so the test guards the half of the change that could regress an unrelated language. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…-resolving it for the doc comment The round-2 parent rule accepted any enclosing node called by the last parent segment and checked no other segment. Outer/run answered with the run of a class nested in Outer, Wrong/Inner/run resolved although there is no Wrong, and a TypeScript B/Foo/run answered with A's. With a healthy language server it did real damage: replace_symbol_body with include_doc_comment resolved Outer/run through the server, then resolved the same path again through the tree-sitter index to find the doc comment. The two disagreed, the edit began at Inner's doc comment, and Inner's run was deleted. The rule also refused every Rust Type/method path, since an impl block is no node and the method's qualified name is bare. - topologyNodesByPath (was topologyNodeByPath) returns every match. P1/.../Pn/Name now needs Name's direct parent to be Pn, that node's direct parent Pn-1, and so on, as findSymbolRecursive does. A direct parent is the innermost enclosing named node or the node a containment edge ties the member to; a qualified name counts only when it spells out the whole chain. No match, or more than one, is refused, and the ambiguity refusal says which lines matched. - Rust resolves through the extractor's impl-to-type link, for inherent and trait impls, when the file declares the type. That needed the edges, so the store gains ExtractFileGraph; ExtractFile is unchanged. A package node's edge is no parent (Go's package, a file-scoped C# namespace), so p/Run stays refused. - docCommentStartPreferTopology and topologyDocCommentStart take the symbol the tool already resolved and find the node of that name whose span starts on its line (column, then kind, tell apart several on one line), else the line-scan runs. They never resolve the name_path again. - The Go receiver forms (*S, (*S), S[T]) are normalised in the topology tier as findFlatGoMethod does in the language-server tier, and stripTypeParams no longer cuts at a bracket that is never closed. - CHANGELOG and docs/tools.md stop claiming Rust was covered by containment and state exactly what is refused. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Plumb-Session: giant-bison
golimpio
previously approved these changes
Oct 1, 2026
golimpio
left a comment
golimpio
left a comment
Contributor
There was a problem hiding this comment.
Three review rounds. The topology-fallback blockers that kept recurring were resolved by cutting to strict semantics. A name_path must match its full direct-parent chain, any ambiguity is refused with the candidate lines, and the doc-comment path verifies the already-resolved symbol instead of re-resolving it. Rust resolves via impl edges. Every round-3 repro now refuses or resolves correctly, including the end-to-end data-loss case. Mutants are killed, and a 29-extractor survey resolves every member except three deliberately refused ambiguous pairs.
atlas-from-plumb
enabled auto-merge
October 1, 2026 10:05
golimpio
previously approved these changes
Oct 1, 2026
golimpio
left a comment
golimpio
left a comment
Contributor
There was a problem hiding this comment.
Re-approving after update-branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #546
What changed
(*Recv).Method, sosymbol_name: "WroteMtime"failed infind_referencesandget_definitionwhileread_symbolfound it via its tree-sitter fallback.resolveSymbolsByName(shared by every name-taking tool) now matches a plain name against a flat Go method's own name, as it already did for nested methods. A symbol literally namedRunbeats a method that matches only once its receiver is stripped, soRunstays the function beside(*S).Run(review B1:move_symbol/rename_symbolkeep working). Equally good matches ((*A).Run,(*B).Run) are never resolved silently: the read-only tools and hierarchies list each;rename_symbolrefuses with provenRecv.Methodcandidates (line/character fallback where no name proves out);move_symbolrefuses with theA/Run,B/Runname_paths, which the topology index resolves (tested).topology_impactpicks the centre among same-named symbols by its line span (review N1).gitHeadLabelreturns the branch, ordetached at <short sha>. The Branch line and the git-policy section in both the brief and full packets use it.Go LSP: … GOWORK=off, warm-up and diagnostics-mode accessors now take the workspacesession_startresolved for the caller. Ininternal/cli, a workspace other than the connection's root is answered by the server detection assigns it (routingProxy.workspaceTarget: the same keyroute()gives that workspace's files). The connection's own root keeps the primary path, so single-agent output is unchanged. A per-agent re-pin starts no server, so when the caller's server has not started the packet says so (not "LSP is ready") and names the go.work it will start with GOWORK=off against, decided from disk with the pool's own config/env (review B2/N3: readiness and diagnostics-mode lines are per agent too).daemon_infostill reports the connection's server.GOWORKparagraph moves to its own[lsp.<language>]subsection ("The Go language server andGOWORK"). It now says the decision is made once per server start (kept through a crash restart or a hibernation wake):plumb restartalways applies ago.workedit, and a primary server's idle teardown also does; an on-demand server for another root lives until the daemon stops. The[git] envtext now runs straight into the project/global composition paragraph.docs/tools.mdnotes the plain-name and ambiguity rules.Verification
Review round (B1, B2, N1, N2, N3, docs nuance): new tests red first; the per-agent cli test no longer pre-seeds the worktree's server (a seeded variant covers the running case). 12 overlay mutants and 2 more through plumb's
mutation_testwere all killed.Red first: every new test failed before its fix. The plain-method tests returned "No symbol named"; the detached-HEAD test had no Branch/Git lines; the per-agent cli test failed both ways against connection-level stubs.
TestSessionStart_LSPStateIsPerAgentchecks both directions. A subagent in a worktree on a main-checkout connection sees GOWORK=off and warming, and does not see the main server'sdiagnostics: pull. A subagent on the main checkout on a worktree connection sees neither line. The coordinator gets the reverse in each case.TestSessionStartLSPStateWiredPerAgentchecks thatregisterAllToolswires the per-workspace accessors.Mutation: 8 hand mutants via
go test -overlay, all killed by real test failures. These were the resolver branch, the cross-file!= 1guard, the detached label, the identity and warmingwsarguments,servesConnectionRoot,workspaceTargetand the diag-mode accessor. 2 of them were re-run through plumb'smutation_teston./internal/cli/: 2 killed, 0 survived.go test ./...(GOWORK=off, GOTMPDIR=.testcache, isolated XDG_DATA_HOME) passed before and after merging origin/main.golangci-lint run ./...reported 0 issues, andmake check-size check-brief check-changelog check-changelog-placementpassed. No tool description changed (tools/list budget untouched).🤖 Generated with Claude Code