Scope the analyze health and clones kinds to the caller's workspace - #395
Merged
Conversation
handleAuditHealth discarded both its context and its request, and ComputeAuditReport walked graph.AllNodes(), so the report always graded every symbol in the index. Reached as `analyze kind=health`, that made a workspace-bound session report on sibling workspaces, and an explicit repo/project/scope selector was accepted and silently ignored — the selector is lowered to a plain `repo` argument the handler never read. The kind is one of the analyze operations the facade routes to a legacy tool rather than the analyze dispatcher, so it never reached the dispatcher's resolveScope. Resolve the same uniform narrowing here and source the graded symbols through scopedNodesByKinds, which applies the session-workspace ceiling and the resolved allow-set. Fan-in / fan-out are still counted over the whole graph, so a cross-repo caller keeps contributing to a symbol's score; only which symbols get graded is scoped. That matches the health_score analyzer. ComputeAuditReport now takes the node set to grade instead of reading the whole store, and the response carries scope_applied like every other analyze result. `gortex audit --path` forwards the path as the selector, so the flag narrows the badge instead of only picking the daemon; a single-repo daemon is unaffected because unprefixed nodes are admitted rather than narrowed away.
find_clones matched its `repo` argument as a bare RepoPrefix equality and applied no workspace ceiling at all. Reached as `analyze kind=clones`, an unnarrowed call from a workspace-bound session returned clone clusters from sibling workspaces, and the path form of the selector — the form the facade lowers a filesystem target into — matched no prefix and silently narrowed to nothing. Resolve the uniform repo/project/workspace/scope narrowing the way the analyze dispatcher does and gate members on analyzeNodeVisible, which enforces both the workspace ceiling and the resolved allow-set. This also gives the tool the tracked-name-or-path selector handling and the out-of-workspace error that the rest of the analyze surface has.
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
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 #349.
What the report got right
Both reproductions are real, and the root cause is the one the reporter isolated:
analyze kind=healthaccepts atarget.reposelector and silently ignores it, whilekind=cloneshonours it.One correction to the framing. The title says "MCP analyze ignores workspace scoping", but that is not true of
analyzegenerally — it is true of exactly the two kinds tested. Kinds dispatched by the analyze handler (health_score,dead_code,hotspots, …) source their rows through the scoped-node accessors and are already workspace-clamped, with a cross-workspace-leak sweep over the whole scope-aware kind set guarding it.healthandclonesare different: they are not analyze kinds at all.Root cause
The
analyzefacade resolveskindto an operation, and the legacy-migration table maps a handful of operation names to a different legacy tool:That table is consulted before the fall-through that synthesises a spec for a real analyze kind, so
healthandclonesshadow the dispatcher and are forwarded straight toaudit_health/find_clones. Neither ever reaches the dispatcher'sresolveScope, so neither gets the workspace clamp or the repo allow-set.From there the two diverge, which is exactly why the reporter saw the selector work for one and not the other. The facade does lower
target.repoto a plainrepoargument for both:find_clonesreadsrepoand filters on it — hence repro 3 — but as a bareRepoPrefixequality, with no workspace ceiling at all. So an unnarrowed call from a bound session returns sibling workspaces' clusters, and the path form of the selector matches no prefix and narrows to nothing.handleAuditHealthwas declared(_ context.Context, _ mcp.CallToolRequest)— it discarded the entire request — andComputeAuditReportwalkedgraph.AllNodes(). It could not see the selector or the session, which is why both selector forms returned the identical global report.The invalid-key rejection the reporter noticed is the facade's own selector validation, which runs before the handler; that is why the key was policed but its value was dropped.
The fix
Two commits, one per handler.
audit_healthresolves the uniformrepo/project/workspace/scopenarrowing the way the dispatcher does and grades symbols sourced fromscopedNodesByKinds, which applies both the workspace ceiling and the resolved allow-set.ComputeAuditReportnow takes the node set to grade rather than reading the whole store, the selectors are declared on the tool instead of being tolerated undocumented, and the response carriesscope_applied. Fan-in/fan-out are still counted over the whole graph, so a cross-repo caller keeps contributing to a symbol's score — only which symbols get graded is scoped, matchinghealth_score.find_clonesresolves the same narrowing and gates members onanalyzeNodeVisible, replacing the bare prefix equality. It gains the tracked-name-or-path selector handling and the out-of-workspace error the rest of the analyze surface has.One CLI change rides along:
gortex audit --pathsent{}, so the flag only picked the daemon connection and the badge was graded against every tracked repo — its own help text promises "audit a different tracked tree". It now forwards the path as the selector.On single-repo daemons
Worth calling out since this backs the README badge. A single-repo daemon mints nodes with an empty
RepoPrefixand has no multi-indexer to resolve a path against, so the newrepoargument matches no registry prefix.QueryOptions.ScopeAllowsdeliberately admits unprefixed nodes rather than letting a repo narrow match vacuously, so the report is unchanged there. Pinned by a test.Tests
Seven new tests across two files, each verified to fail against the pre-fix handlers:
scope_appliedmust disclose itgo test -race ./...passes: 11643 tests across 179 packages.golangci-lintclean.Follow-up, deliberately not in scope here
healthandclonesare the two aliases with concrete reproductions, and they are fixed. The same shadowing applies to other entries in that table, and a few of those handlers are also globally unscoped —run_inspections,get_communities,get_processes,get_recent_changes, withaudit_agent_configroot-pinned by design. Each needs its own scoping decision (community detection runs one partition over the whole index and already has a dedicated clamp helper;agent_configis arguably correct as-is), so folding them in would mean several unrelated design calls in one change. Worth a follow-up issue.