feat(mcp): say whether ownership was ever mined instead of answering zero

[tiendungdev]

First slice of #511, and the one I picked deliberately: it is the smallest surface that needs every part of the contract, so it fixes the vocabulary before the pattern is applied anywhere else.

The zero being fixed

analyze kind=ownership answers no owners matched for two states that a caller must act on differently:

• nobody in scope carries an owner — an answer;
• git blame was never mined — the absence of one.

They render identically. The second one is the sentence an agent turns into "this code is unowned".

The contract

contract_tier_unbuilt already solves this for the contract tier, and its three properties are why it is safe to act on. All three are kept:

1. the zero says it is not absence evidence;
2. it names which repositories are in that state;
3. it names the recovery and rules out the one that looks obvious.

Property 3 carries the most weight here. Indexing never reads git blame, so reindex_repository — and a full untrack / track, the heavier thing a caller escalates to — leave the answer exactly as empty as they found it. The recovery string says so by name.

Five states, from the scan the answer already runs

state	meaning
never_built	no symbol in scope carries a stamp
partial	some repositories stamped, others not
built (filters)	no symbol was in scope at all — the zero is about kind / path_prefix
built (threshold)	owners were found and min_symbols removed every one
built	the data is there and nothing matched

The built half is the valuable one, exactly as the issue argues: it lets a caller say "actually nothing" with the tool's own confidence instead of hedging every zero forever. The threshold case is a third silent zero this handler already had, rendered like the other two.

Two choices worth arguing

partial and never_built are attached even when the answer has rows. A repository in scope with no blame stamps makes the answer an undercount either way, and a non-empty undercount is the more dangerous shape — an empty answer at least looks suspicious, while rows look like the answer. This is the same argument the route-inventory case in #511 makes: 130 of 153 rows is the reading that gets acted on. Only a built state is dropped when rows exist, since rows are their own proof that blame was mined.

The classification is not built on graph.EnrichmentState, even though that marker exists and would have been the obvious reuse. It is written only by the semantic providers, and recordEnrichMarker refuses to record on a dirty tree or without a sha — so a repo enriched under either condition would be reported as never enriched. Deriving the state from the same scan the answer came from is stronger evidence and, more importantly, cannot contradict the number it accompanies. The tallies ride along in the existing loop; no extra graph pass.

Tests

Five end-to-end cases through handleAnalyze, a table over the classifier, and a compact-encoding test — that path is prose only, so a caller there sees the line or nothing.

Thirteen mutants, each failing its test and passing restored: never_built and partial each collapsed into built · an empty scope and a threshold-emptied answer each blamed on enrichment · the caveat naming stamped repositories too · the note dropping its refusal of the absence reading · the recovery no longer ruling out reindex · a built state offering a recovery · the candidate and stamped tallies each not counted · the caveat dropped whenever rows exist · a complete answer annotated · the payload never carrying it.

One test failure during development is worth reporting because it changed the test rather than the code: the multi-repo case asserted repos == ["repo-b"] and got ["(default) repo-b"] — the test server's own fixture nodes are unstamped too, so naming them is correct. The assertion now pins the property that matters (the unstamped repo is named, the stamped one is not) instead of a count that depended on the fixture.

What I would do next, if this shape is right

co_change is the same slice with a different source — tools_cochange.go already has the presence check the classifier needs. Then the shared-node contract attribution from the second comment on #511. The routes partial case I would leave until #704 is understood, because "which files were parsed but produced no rows" is extractor bookkeeping rather than a rendering change, and guessing at it would publish a complete: false that is itself unverified.

Verification

go build ./... and go vet ./internal/mcp clean; the analyze / ownership / contract-tier test subset passes. Same caveat as my last two PRs: several ./internal/mcp tests fail on Windows on a pristine tree (drive-letter path shapes), so I verify the affected tests rather than claiming the suite.

🤖 Generated with Claude Code

[zzet]

Requesting changes for one blocking correctness issue:

[P1] Treat same-repository partial blame coverage as partial

ownershipDataState currently treats stamped[repo] > 0 as complete coverage. That only proves one candidate symbol was mined. blame.EnrichGraph is explicitly best-effort per file and skips failed or unblameable files, so a single repository can contain both stamped and unstamped candidates.

In that state the classifier returns built; when any owner row survives, handleAnalyzeOwnership removes the caveat and publishes a silently incomplete ownership answer—the exact misleading non-empty undercount this PR is intended to prevent.

Please align candidate counting with blame eligibility and classify incomplete eligible coverage as partial, or use a durable per-repository completion marker.

Required regression coverage:

• classifier: candidates=3 and stamped=1 in the same repository => partial
• handler: one stamped and one unstamped same-repository symbol retains data_state even with owner rows
• incremental reindex after blame enrichment reports partial coverage

[tiendungdev]

Confirmed and fixed in e5d89183.

I read EnrichGraph before changing anything, and the mechanism is exactly as you describe:

for path, nodes := range byPath {
    lines, err := Run(repoRoot, path)
    if err != nil || len(lines) == 0 {
        continue          // ← file skipped, pass still returns success
    }

plus pickLatest returning nil for a symbol whose lines carry no blame data. Both leave an eligible symbol unstamped inside a repository that has other stamps. stamped[repo] > 0 proved the pass had run over the repository and nothing else — and since the handler drops the caveat once rows exist, the published answer was a silently incomplete rollup. Same failure as the one this PR is about, one level down.

The fix

Coverage compared per repository: stamped == 0 is unmined, 0 < stamped < eligible is incomplete, and both report partial with the repository named. never_built is now reserved for a scope where nothing at all is stamped.

Aligning the population, in both directions

I took the eligibility route rather than a durable marker. Two reasons, and the second is the one that decided it.

blame.Eligible(n) is exported and EnrichGraph itself now calls it, replacing the two inline guards, so the reporter and the pass cannot drift into disagreeing — they are one decision with one definition.

The direction I want to flag, because it is the trap in "just count everything unstamped": counting symbols the pass never admits would report a shortfall no enrichment could ever close. A repository containing one KindFile node would sit at partial forever, and a caveat that never clears is one a caller learns to skip — which costs more than the bug being fixed. So the population is blame's admission set, and the stamped tally is filtered the same way, so a stale stamp on a symbol that is no longer eligible cannot fill a gap it is not part of.

The caveat also now carries symbols_eligible / symbols_stamped. Deciding whether to act on a partial answer needs the size of the shortfall, not only the verdict.

Requested coverage, all three

• classifier — candidates=3, stamped=1 in one repository → partial, plus the one-short case and a partly-covered repo alongside a wholly unmined one.
• handler — TestAnalyzeOwnership_PartialCoverageInsideOneRepoKeepsTheCaveat: one stamped and one unstamped same-repository symbol, an owner row survives, data_state stays.
• incremental reindex — TestAnalyzeOwnership_ReindexAfterEnrichmentReportsPartialCoverage runs a real git init/commit, a real blame.EnrichGraph, asserts the fully-stamped scope carries no caveat, then adds a file and reindexes. The answer keeps its rows and reports partial. The clean-state assertion is deliberate: without it the test would pass on a classifier that always says partial.

Two mutants survived, and both were real test gaps

Eight mutants; six died immediately. Two did not, and neither was a spurious mutation:

• stamped ignoring eligibility survived because no test had a stamp on an ineligible symbol. That mutation lets one stale stamp make a short repository read as covered — the P1 again, through a different door. Now covered by TestAnalyzeOwnership_AStampOnAnIneligibleSymbolCannotFillTheGap.
• Eligible returning true for every kind survived because the ownership handler's own kind filter kept non-blame kinds out of reach. Now covered by a table in internal/blame asserting Eligible against shouldEnrichBlame for fifteen kinds.

Both are the lesson from the last two rounds arriving on schedule: a dying mutant proves a test binds something, not that the rows span what production can produce.

One test change worth reporting

TestAnalyzeOwnership_BuiltZeroIsReportedAsReal and ..._CompleteAnswerCarriesNoCaveat started failing, and the classifier was right: setupTestServer seeds unstamped eligible symbols, so that fixture genuinely has partial coverage and can no longer be used to assert the absence of a caveat. Both moved to an isolated empty graph rather than being relaxed.

Verification

go build ./..., go vet ./internal/mcp ./internal/blame clean; the ownership and blame suites pass.

One environment caveat, since a git-driven test is new here: TestEnrichGraph_PersonNodeRepoScoped fails intermittently on my machine with TempDir RemoveAll cleanup: unlinkat …\.git: The directory is not empty. That is a cleanup error, not an assertion, it reproduces on a pristine blame.go (1/4 runs, versus 2/4 with my change — same noise), and I traced it to my own toolchain: the git on my PATH is a fork that writes .git/ai/logs and .git/ai/working_logs on every git init, so a temp .git is not reliably empty when the test ends. Nothing to fix upstream, but worth naming in case my new git init test ever looks flaky in CI for a different reason.

[zzet]

Requesting changes. The earlier same-repository partial-coverage issue is fixed, but the new completeness contract is still unsound in multi-repository graphs: existing cross-repository blame projection can be counted as coverage and suppress data_state. Two additional state/rendering cases also need correction before this can safely claim that an ownership result is complete.

[zzet]

[P1] Cross-repository blame can be certified as complete

The new per-repository completeness check treats any blame row on a node as proof that its RepoPrefix was covered, but multi-repo enrichment calls EnrichGraph on the combined graph for each repository root. stripRepoPrefix maps a foreign node onto the current root whenever both repositories share the same relative path, so repo A blame can stamp a repo B node. The counts then match and this handler can publish built and drop data_state for repo B even though its ownership data came from repo A.

Scope EnrichGraph to the target repository prefix and add a two-repository same-relative-path regression, including the empty-prefix single-repo case.

[zzet]

[P2] never_built claims run history that is not observable

stampedTotal == 0 is reported as never_built and says enrichment is absent until it runs, but EnrichGraph is best-effort: every git-blame failure or empty result is skipped while the pass can still return success. A completed pass can therefore produce zero stamps, leaving clients with a false state and a recovery command that cannot clear it.

Use an observational state/name or persist per-repository pass outcomes before claiming never-run.

[zzet]

[P2] Partial coverage hides a simultaneous threshold zero

partial is returned before the owners > 0 threshold case is considered. When coverage is partial and min_symbols filters out every known owner, the response only reports the enrichment shortfall and even says "the rows below are real" although there are no rows.

Preserve both causes, or pass the post-threshold row state into this rendering, and add a partial-plus-threshold regression.

[tiendungdev]

All three fixed in 54ec3dc8. Every one confirmed against the source before I touched it.

P1 — cross-repository blame counted as coverage

Confirmed, and it is worse than a classifier bug: EnrichGraph was writing the wrong stamps, and my completeness check merely believed them.

stripRepoPrefix tries the node path under the current root, then retries without its leading segment. So for a pass over repo-a's root, a repo-b node at repo-b/internal/foo.go misses, gets trimmed to internal/foo.go, finds repo-a's file, and takes repo-a's authors. The result is a stamp that is well-formed, plausible, and about a different file.

EnrichGraph(g, root, repoPrefix) now admits only nodes carrying that prefix, mirroring cochange.EnrichGraph. Both call sites already had the prefix in hand and were dropping it — handleAnalyzeBlame was iterating for prefix, root := range roots and passing only root.

Regression as requested, plus the direction that keeps the daemon honest:

• two repositories sharing internal/foo.go: after a repo-a pass, repo-a's node is stamped and repo-b's is not;
• the empty-prefix single-repo case still stamps — "" had to keep meaning "this graph's only repository", not "no repository";
• a prefixed pass does not reach the unprefixed shared-externals bucket.

P2 — never_built named a history I cannot see

You are right, and the fix is a rename rather than a reword, because the same objection applies to the state at the other end. Both named a pass; neither named an observation:

was	is	what it means
never_built	absent	nothing in scope carries this data
partial	partial	some does, some does not
built	complete	every symbol the pass admits carries it

I renamed built too on my own initiative — say the word and I will put it back. My reasoning: leaving it would keep one history claim in a trio the other two no longer make, and complete is what the code can actually check.

The absent note no longer says enrichment "is absent until it runs", and the recovery now says what a state that survives the pass means: the remaining files are ones git cannot blame — an unborn commit, an untracked or generated file — not ones nobody has enriched. Without that sentence the recovery is a loop.

TestOwnershipDataStateNamesNoRunHistory asserts the note contains none of "never ran", "never been", "until it runs", "has not run", so the claim cannot come back through a later reword.

P2 — the threshold zero was hidden behind the shortfall

Confirmed: partial returned before the owners > 0 case, so an answer that was both short on coverage and emptied by min_symbols reported one cause and, worse, said "the rows below are real" with no rows below.

The classifier now takes the post-threshold row count and composes both causes rather than choosing one. The partial wording no longer promises rows either — "whatever rows it returns are real" holds whether there are none or many.

The mutant that survived

Seven mutants, six died at once. The seventh — thresholdEmptied := rows == 0, dropping the owners > 0 half — survived, and it was a real gap: with no owners at all, that mutation appends "owners were found and min_symbols removed every one of them" to the absent note. A false sentence, in the one state where a caller has the least information to check it against. Now asserted in both the absent and the genuinely-empty-complete cases.

One test-harness note

The new blame tests read authorship from the sidecar as well as node Meta. My first version checked Meta only, and every assertion passed for the wrong reason: a store implementing BlameEnrichmentWriter writes the rows and never touches Meta, so both the "stamped" and "not stamped" checks read empty. The positive assertions caught it — which is the argument for always pairing a negative assertion with a positive control in the same test.

Verification

go build ./... and go vet ./internal/mcp ./internal/blame ./cmd/gortex clean; the blame and ownership suites pass.

The mutant run for internal/blame is scoped to the new scope tests plus the eligibility tables. The package's older git-heavy cases carry a TempDir cleanup flake on this machine — a git fork on my PATH writes .git/ai/logs after the test body ends — and including them would let a mutant read as BINDS for a reason unrelated to the mutation.