fix(mcp): guide unindexed symbol renames

[tiendungdev]

Summary

• distinguish invalid symbol IDs from existing ignored or otherwise unindexed files
• return a structured refusal with a guarded edit-file recovery path and no automatic text replacement
• preserve indexed semantic rename behavior and the existing not-found response for invalid targets

Verification

• go test -race ./internal/mcp -run "^TestRename(Symbol_(UnindexedRecovery|IgnoredRecovery|InvalidTargetStaysNotFound)|Facade_UnindexedRecovery)$" -count=1 -timeout=5m
• go test ./internal/mcp -run "^TestRename" -count=1 -timeout=5m
• go vet ./internal/mcp
• go build ./internal/mcp
• git diff --check

Closes #567

[zzet]

The mechanism here is right, and the tests prove the important part: no bytes are written on the refusal path. unindexedRenameRecovery fires only where GetSymbol already returned nil, gates on resolveFilePath so it inherits the repo-root confinement guard, and uses the established graphPathSpelling → GetFileSymbols idiom. edit / file does map to edit_file (internal/mcp/facade_registry.go:334), so the suggested fallback is actionable.

Verified locally: merges clean, and with main merged in, go build ./... and go test -race ./internal/mcp -run "^TestRename" both pass.

Three things to address before this goes in.

1. The refusal returns IsError=false, so "refused" reads as success downstream

respondJSONOrTOON produces a non-error CallToolResult, which leaks in two places:

• cmd/gortex/edit.go:366 passes a nil renderer to runEditTool, so the payload is just pretty-printed and the command exits 0. Previously gortex edit rename late.go::Late --to X exited 1 with symbol not found. That is a silent regression for anything scripting the CLI on exit status.
• classifyFacadeOutcome (internal/mcp/facade_tools.go:1541) short-circuits on !result.IsError and returns facadeOutcomeSuccess, so the refusal is booked as a successful rename in facade telemetry.

Either return this through NewStructuredErrorResult — which is what the error_code key exists for — or keep the success payload and make the CLI exit non-zero on status: "refused".

2. symbol_not_indexed is not registered in errors.go

internal/mcp/errors.go is documented as the canonical one-place-to-grep list: "The codes live here … Adding new codes is OK; renaming or removing an existing one is a wire-contract break." This code is a bare string literal in rename_apply.go and appears nowhere else in the tree. One line keeps the wire contract greppable, alongside the existing ErrCodeSymbolNotFound / ErrCodeFileNotIndexed it sits between:

// ErrCodeSymbolNotIndexed — the symbol's file exists and is in scope but
// carries no indexed symbols, so semantic rename cannot see it.
ErrCodeSymbolNotIndexed ErrorCode = "symbol_not_indexed"

3. expected_occurrences is demanded but never supplied

required_guards tells the caller to pass expected_occurrences, but the payload carries no count — so the agent has to open the file to compute it, which is exactly the read this tool exists to avoid.

indexIdentifier is already in this same file, a few lines below the new function. Counting whole-identifier occurrences of requested_symbol in the resolved file is a handful of lines and buys two things:

• a usable occurrences: N for the guard the response is asking for;
• a way to keep a zero-hit file returning plain symbol not found.

That second point closes the issue's first acceptance criterion — distinguish a nonexistent target from an existing but ignored/unindexed file — for unindexed files too. As written, late.go::Typo gets the same confident recovery payload as late.go::Late, and the agent only discovers the identifier isn't there when edit_file fails.

Smaller notes

• The rename_symbol tool description (internal/mcp/tools_coding.go:177) and docs/mcp.md:289 both still say "Returns status (applied / would_apply / no_edits)". refused is undocumented in both, and the tool description is the contract agents actually read.
• required_guards mixes real edit_file parameter names (expected_occurrences, base_sha) with prose instructions ("whole-identifier match", "explicit file target") in one flat list. An agent may reasonably try to pass all four as arguments — worth splitting the parameters from the guidance.
• An indexed-but-symbol-less file (a README.md, an empty .go) also trips the recovery and gets "Cross-file references are not proven because this file is outside the graph.", which isn't true for it. Harmless, but the warning overclaims.

Nothing here is a correctness bug and nothing writes to disk, so (1) is the only item with user-visible fallout; (2) and (3) are cheap enough to fold into the same commit.

[tiendungdev]

Addressed the review in 5d4bf1d.

• Unindexed rename recovery now returns a structured symbol_not_indexed error with IsError=true, so CLI relays exit non-zero and facade telemetry classifies it as a tool error.
• Registered ErrCodeSymbolNotIndexed in the canonical error catalog.
• Recovery is emitted only when the requested name has whole-identifier hits on disk; zero-hit unindexed targets remain plain symbol not found.
• The payload now includes occurrences and an actionable guarded edit/file fallback with match, replacement, replace_all, expected_occurrences, and base_sha. Parameter data and prose guidance are separate.
• Warning language no longer claims every symbol-less file is outside the graph.
• Updated the rename_symbol tool contract and docs/mcp.md.

Verification:

• go test -race ./internal/mcp -run ^TestRename -count=1 -timeout=10m
• go test ./cmd/gortex -run ^TestEditRename_ -count=1 -timeout=5m
• go vet ./internal/mcp
• go build ./...
• git diff --check

The full cmd/gortex suite was also attempted on Windows; unrelated pre-existing/platform failures remain in agent-render goldens, daemon temp-path/signal tests, and locked SQLite cleanup. The focused rename CLI tests pass.

[zzet]

The follow-up at 5d4bf1d correctly fixes the earlier review items: the refusal is now a structured tool error, the error code is registered, facade telemetry sees a failure, occurrence/base-SHA data is present, and the tool contract is documented. Two correctness issues remain before merge.

1. Blocking — the emitted safe_fallback cannot enforce the identifier semantics it advertises.

unindexedRenameRecovery computes a supposed whole-identifier count, then emits match: requestedSymbol, replace_all: true, and that count (internal/mcp/rename_apply.go:30-86). The generated edit.file request reaches handleEditFile, which counts raw byte-substring matches and executes strings.ReplaceAll (internal/mcp/tools_fileops.go:761-800). This is not an identifier-aware operation.

A concrete valid-language case is JavaScript: $ is legal inside an identifier, but the shared boundary predicate only treats letters, digits, and _ as identifier runes (internal/mcp/notes.go:844-846). For an ignored file containing only foo$bar, requesting file.js::foo can therefore report a foo occurrence and supply a guarded fallback whose raw replacement changes foo$bar to renamed$bar, even though foo is not the target symbol. Standalone hits in comments or strings can likewise be treated as proof that a symbol exists and then rewritten. This violates #567’s requirement to distinguish nonexistent targets safely.

Please use a genuinely language/identifier-aware edit path, or emit reviewed contextual edits that the existing exact-string operation can enforce. Add an end-to-end test that executes the returned fallback; inspecting its JSON shape does not validate its safety.

2. Correctness — qualified method/nested symbol IDs do not recover.

The helper searches the complete suffix after ::. For the normal ID file.go::Server.Run, it searches for literal Server.Run, which is absent from a declaration such as func (s *Server) Run(). A real ignored/unindexed method therefore falls back to generic symbol not found. If the composite text appears only as a selector elsewhere, recovery can instead target that selector while missing the declaration. Please cover receiver-qualified method IDs and prove that recovery is anchored to the requested declaration rather than arbitrary lexical text.

All 11 checks are green. I also verified the focused rename/CLI tests and a clean prospective merge onto current main; these are uncovered semantic failures, not build failures. Path confinement, the SHA drift guard, and no-write-on-refusal behavior otherwise look correct.

[tiendungdev]

Implemented the requested rename-recovery changes in 2b1d93c4:

• Removed the raw substring/replace_all fallback.
• Unindexed recovery now parses the caller-owned bytes with the configured language extractor and anchors exactly one declaration.
• The returned fallback is a guarded exact-line edit(operation:"file") request with expected_occurrences: 1 and base_sha, explicitly scoped to declaration_only.
• Qualified method IDs such as late.go::Server.Run now resolve to the method declaration.
• Added an end-to-end test that executes the returned fallback through the edit facade and proves the declaration changes while s.Run() remains untouched.
• Added a JS regression showing foo does not recover from foo$bar.

Verification:

docker run --rm -v <worktree>:/src -w /src golang:1.26 go test ./internal/mcp -run '^TestRenameSymbol_(RecoveryExecutesDeclarationOnly|SubIdentifierIsNotADeclaration|UnindexedRecovery|IgnoredRecovery|InvalidTargetStaysNotFound)$' -count=1 -v -timeout=10m
PASS
ok github.com/zzet/gortex/internal/mcp 1.720s

git diff --check also passes. The native Windows cold build still hits the existing tree-sitter-swift C compiler warning/error path, so the focused evidence above was run in Linux, matching CI more closely.

[tiendungdev]

CI follow-up pushed in 6f9a54a and ca604b5. The first commit removes countIdentifierOccurrences, which golangci-lint correctly reported as unused. The second fixes the shared double cache restore in govulncheck by using go.mod and disabling only the nested action cache. Focused unindexed-rename tests, go vet ./internal/mcp, actionlint v1.7.7, and git diff --check pass on Linux.

[zzet]

Dependencies were updated in #579, so rebase / merge master will solve the problem
p.s. No reason to skip the cache (it may impact the CI timing)

[tiendungdev]

Applied the maintainer direction in 774ac8d. Merged upstream/main including PR #579 without force-push, then restored the upstream govulncheck cache configuration. The net workflow and go.mod/go.sum now match upstream/main. Focused unindexed-rename tests, go vet ./internal/mcp, and git diff --check pass on Go 1.26.6 Linux.

[zzet]

Blocking correctness issue: the advertised safe fallback can rename the wrong identifier.

unindexedRenameRecovery derives declarationColumn from node.StartColumn, but Go method nodes do not set StartColumn, so it remains zero. indexIdentifier can therefore select an earlier identical identifier on the declaration line.

For example, renaming late.go::Run.Run to Execute for:

type Run struct{}
func (r *Run) Run() {}

emits a supposedly safe fallback that changes the receiver to *Execute while leaving the method named Run. The parse gate, base_sha, and occurrence guard all accept that wrong-target edit.

Please anchor the exact declaration-name span, or reparse the candidate and prove it creates the expected renamed declaration ID. Add an executed-fallback regression where the receiver type and method are both named Run.

[tiendungdev]

Addressed the blocking declaration-anchor issue in 9755d55.

The unindexed recovery path no longer trusts node.StartColumn. It now tries each whole-identifier occurrence on the declaration line, reparses each candidate, and emits a fallback only when exactly one candidate removes the original symbol ID and creates the expected renamed ID on the same declaration line.

The executed-fallback regression now uses a receiver type and method both named Run. It proves the fallback changes Run.Run to Run.Execute while preserving the receiver type and the r.Run() call site.

Verification:

• go test ./internal/mcp -run UnindexedRecovery|IgnoredRecovery|RecoveryExecutesDeclarationOnly|SubIdentifierIsNotADeclaration|InvalidTargetStaysNotFound -count=1 -v -timeout=10m
• go vet ./internal/mcp
• git diff --check

[zzet]

Two blocking safety findings remain at head 9755d55.

1. Unbounded recovery reparsing bypasses parser safeguards

verifiedDeclarationRename rebuilds and reparses the entire file once for every whole-identifier occurrence on the declaration line. An ignored one-line minified JavaScript file with many same-name tokens therefore drives O(file bytes × hits) CPU and allocation growth.

The called ExtractSource invokes the extractor directly, bypassing normal size, minified-file, admission, timeout, and crash-isolation controls. No request context is checked, so the work continues after the MCP deadline abandons the handler.

Please route validation through a context-aware bounded/crash-isolated extraction API, cap candidate/file/line work or avoid per-occurrence full reparses, and add a cancellation/stress regression using an ignored one-line file.

2. The generated fallback can replace an in-repository symlink

Recovery follows a symlink with os.Stat/os.ReadFile, but the fallback targets the original symlink path. edit.file then atomically writes over that path, replacing the symlink with a regular copied file while leaving the real target unchanged. The content-only base_sha guard cannot detect this object-type change.

Please refuse symlink leaves with Lstat, or return the canonical in-repository target with an object-identity guard. Add an executed-fallback regression that verifies both target content and symlink type.

The earlier Run.Run wrong-token issue is fixed, and current CI is green, but these two cases make the advertised safe_fallback unsafe to merge.

[tiendungdev]

Addressed both blocking safety findings in 5f537db.

• ExtractSource now requires the request context and routes caller-owned bytes through the indexer's normal minified-content, admission, timeout/panic recovery, and optional crash-isolation path. It also enforces a hard 1 MiB ad-hoc source cap (or the lower configured max_file_size).
• Declaration recovery now refuses lines above 16 KiB, refuses more than 8 whole-name candidates before candidate reparsing starts, and checks cancellation before extraction and before every bounded candidate.
• Recovery now uses Lstat and refuses symlink leaves, so it never emits an edit.file fallback for a symlink path.
• Added regressions for an ignored one-line minified file, a parsed one-line file that exceeds the candidate budget, a canceled recovery, and a symlink leaf with target/type preservation checks.

Verification:

• go test ./internal/mcp -run TestRenameSymbol -count=1 -timeout=180s
• go vet ./internal/mcp
• git diff --check

The three resource/cancellation regressions execute and pass locally. The symlink test is present but skips on this Windows host because creating a symlink requires a privilege the runner does not have; it will execute on Linux CI.

[zzet]

Approved for merge per maintainer decision. Current-main extraction API compatibility and repository-aware unindexed recovery will be addressed immediately in a follow-up PR.