Skip to content

Make MCP retrieval and writes graph aware - #64

Open
blast-hardcheese wants to merge 15 commits into
mainfrom
graph-search-selection
Open

blast-hardcheese wants to merge 15 commits into
mainfrom
graph-search-selection

Conversation

@blast-hardcheese

@blast-hardcheese blast-hardcheese commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Preserve automatic repository .kin/ selection and selected-store default captures. Default MCP retrieval includes the selected project and effective configured global graph, including nondefault data directories. Explicit profiles remain isolated.
  • Use rank-based federation that preserves each graph's native hybrid ordering. A single contributing graph retains its original order and scores; multi-graph output labels the federation score explicitly. Equal ranks prefer the configured outer graph.
  • Carry session-bound, graph-qualified references through search, context, task dependencies, node details, and mutation results. Follow-on operations route to the referenced store. Derived captures accept qualified source_refs, with global precedence for mixed evidence; cross-store links and task dependencies are rejected before mutation.
  • Apply graph scopes consistently across search, tasks, context/ask/prime, listings, diagnostics, and read resources. Per-store topology stays separate. graph="project" permits contained reads without opening a broken secondary.
  • Build global store policy from user configuration, and prevent repository edit-policy overrides from governing global or profile stores. Secondary reads remain SQLite read-only, never migrate or stamp the database, and preserve grounding/trust warnings.
  • Return typed secondary-store failures and preserve the primary exception during cleanup. Resolve title/alias collisions canonically before contextual writes and keep globally routed project tasks visible in the default project view.

Default behavior and graph identity

Automatic local graph support remains intentional, per the requester's direction; this PR does not add an opt-in requirement. A source-free capture belongs to the selected store and is readable there through default MCP retrieval. Derivation cannot be inferred from prose: callers provide qualified evidence references when they intend source-based routing.

Qualified references authorize operations only within the current MCP store-selection session. They are not durable database identifiers. Broader sibling-worktree discovery, archive reconstruction, and persistent graph identity are outside this PR. SQLite edges and task dependencies cannot span stores.

Validation

  • Final full suite on c327820: 3,058 passed, 4 skipped, 9 subtests passed (Node 24; ambient Codex session identifiers cleared).
  • Integration acceptance/config/MCP/retrieval suite: 432 passed. The final test-contract correction passed 208 focused tests plus an independent regression rerun.
  • GitHub CI passed on the final head. kin policy check --event pre-commit and git diff --check pass.

Regression coverage includes real load_config/MCP round trips with a nondefault configured global directory, default capture/search/link, global-derived captures, duplicate IDs and title/alias collisions, contextual write prevalidation, task dependency round trips, native-ranking parity, scoped retrieval, grounding/trust warnings, stale schemas, profile mismatch cleanup, and edit-policy isolation.

Delivery scope

No release, tag, deployment, or sibling-worktree discovery is included. The branch incorporates current main's already-landed release metadata and unrelated upstream fixes without changing their behavior.

@adaptcom adaptcom Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confidence Score: 1/5

Summary

Extends MCP retrieval and writes across project and global graphs, but global policy isolation and reference round-tripping have reproducible defects. Secondary-store error handling and grounding warnings also need correction.

Important Files Changed

File Overview
README.md Documents cross-graph retrieval and write routing.
src/kindex/config.py Preserves the user-level data directory before project overrides.
src/kindex/mcp_server.py Adds cross-graph discovery and routing with policy-isolation and reference-handling defects.
src/kindex/store.py Adds read-only connections; cleanup masks profile-mismatch errors.
src/kindex/tasks.py Formats graph-qualified task references.
src/kindex/vectors.py Checks existing vector compatibility without schema writes.
tests/test_mcp_cross_graph.py Tests basic routing but misses policy isolation, aliases, dependency round-trips, and secondary-store failures.

↻ Re-run review · View in Adapt

Comment thread src/kindex/mcp_server.py Outdated
Comment thread src/kindex/mcp_server.py Outdated
Comment thread src/kindex/mcp_server.py Outdated
Comment thread src/kindex/mcp_server.py Outdated
Comment thread src/kindex/mcp_server.py Outdated
Comment thread src/kindex/store.py Outdated
@jmc-wander

Copy link
Copy Markdown
Contributor

Independent review at head 6dd9ba2, executed in a throwaway worktree. I came to this PR from the live defect it appears to target, so let me lead with that.

Does it fix the two-store defect? No.

Reproduced from inside first. Three stores on this machine:

store nodes
~/Personal/Conv/kindex.db (configured data_dir) 27,770
<repo>/.kin/local/kindex/kindex.db 1
~/.kindex/kindex.db (package default) 0

search reads the first, add/link/status write the second, so search hands back node ids that link then cannot find.

Mechanical cause, src/kindex/config.py:1003-1027 (identical on main and this head): store selection keys on the mere file presence of <git-root>/.kin/local/*.db, and presence outranks the user's configured data_dir. Proven on main:

A) plain                  -> …/repoA/.kin/local/kindex     # global data_dir silently discarded
B) .kin/config data_dir:  -> …/repoA/.kin/local/kindex     # explicit project config ALSO loses
C) KIN_PROFILE=personal   -> …/HOME-GRAPH                  # the only lever that pins it

And the file that flips the switch is created by a different lane: src/kindex/integrations.py:70-99 (modern_codebase_store) unconditionally mkdirs <repo>/.kin/local/kindex and opens a Store there. A separate MCP server's side effect silently re-points this one. That is the one-authority-per-fact break — store selection is decided by an artifact nobody announced.

At this PR's head:

ADD  -> Created node: aac7c6cd0d39      in local store? True   in global? False
LINK bare -> Target node not found: 418b002e7145          # the live symptom, unchanged
LINK qual -> Error: cross-graph links are not supported   # new permanent refusal

add with default arguments still writes to the repo-local store (_derived_write_store, mcp_server.py:381-390, defaults graph="project"). The PR makes global nodes visible to search, which raises the odds an agent finds one and then discovers it cannot link its capture to it at all.

Blocking

1. search silently re-ranks for every user, including single-graph users. mcp_server.py:666-687. The merge block is unconditional — it runs even when home is None, which is the majority configuration. It replaces hybrid RRF ordering with 0.35*confidence + 0.30/(rank+1) + 0.35*literal_term_coverage (:675), while the displayed score at :718 still prints rrf_score when home is None. Executed, single store, no global graph involved:

=== MAIN ===                     === PR64 ===
1. Release checklist  (0.786)    1. Release checklist  (0.786)
2. Deploy guide       (0.429)    2. Release release…   (0.383)  <- up
3. Release release…   (0.383)    3. Deploy guide       (0.429)  <- down

Two harms. Ordering changes for all existing users with no flag and no migration — and the PR's risk note says reordering happens "relative to a single-graph search", which is the opposite of what it does. And the 0.35*coverage term systematically demotes vector and graph-BFS hits carrying none of the query's literal tokens, which is exactly what hybrid retrieval exists to surface. Displayed scores are now non-monotonic, so the output looks broken.

2. Default writes still land in the store the reader will not read. mcp_server.py:381-390, 744/793, 1610, 2497. Routing to global requires the caller to pass graph="global" or source_refs="global:<id>"; nothing enforces it, and the only mechanism is a sentence in the instruction blob at :72-73. Concretely: an agent starts in a repo, captures a decision before searching (the documented workflow — "capture as you go, don't batch"), the write lands in the 1-node store, and the next session's search shows 27,770 nodes none of which link to it. The routed path genuinely works when used, but this is a behavioural contract on an LLM, not a mechanism — and it cannot cover the first write of a session, because no source_refs exists yet. link has no graph parameter at all.

3. A git-tracked .kin/config from a cloned repo now governs writes into the personal graph. mcp_server.py:301 does home_config = config.model_copy(deep=True), copying the project-merged config including edit_policy — which isn't in _PROJECT_LAYER_UNTRUSTED_KEYS (config.py:874-880) because until now it could only govern that repo's own store. Executed:

.kin/config:  edit_policy: {decision: editable, constraint: editable}
  -> ignored_project_keys == []
  -> edit("global:<decision-id>", content="REWRITTEN by the repo")  => succeeded
  baseline: "Error: Node type 'decision' is additive — use supersede"

A cloned repository can destroy the additive-immutability invariant that supersede and the whole decision/constraint model rest on, in the user's global graph. Same class this codebase already defends against in project_store.tracked_store_refusal ("files a clone delivers are not a local trusted database"). Build home_config from user-level layers, not config.model_copy.

High

4. A stale global graph breaks project-only operations with a protocol error. store.py:386-390 raises SchemaMigrationPending (a RuntimeError); callers wrap only except ValueError; _safe_output re-raises. With home at schema_version 3, both edit(<project node>) and search("local") raise RuntimeError. The collision probe in _routed_ref (:356-367) opens the global store for any bare id matching locally, so a second store the user never asked to involve takes down the primary. _tool's own docstring promises "a broken store turns into a typed tool result on every tool, never a protocol error."

5. Profile mismatch raises AttributeError instead of the real error. store.py:392-395 calls self._conn.close() in cleanup, but _check_profile_stamp (:450-451) already set _conn = None. The non-read-only path at :417-419 gets this right. Result: RuntimeError: 'NoneType' object has no attribute 'close' instead of the profile-stamp message.

Medium and below

6. A routed global write permanently stamps the user's primary database (:309-311 clears _stamp_on_open only for reads); first add(graph="global") writes meta.kin_profile, after which any other profile name hard-refuses. Irreversible without SQL.

7. Retrieval containment changes with no opt-out: search (:590) and task_list (:2533) gained no graph parameter while the write tools did, and ties prefer global (:677). Once any repo has a .kin/local, every search there merges the personal graph in — asymmetric with the write API, and it removes the separation the repo store exists to provide.

8. Grounding verdicts dropped for global hits — :631-634 omits grounding=grounding, so a search returning 100% global results prints no [grounding: uncalibrated] banner precisely when nothing was evaluated.

9. context (:996), ask (:1519), prime, list_nodes, suggest, graph_stats, status, changelog are not graph-aware. prime is the SessionStart primer — so search reports 27,770 nodes while prime reports 1, and an agent trusting prime concludes memory is empty.

10. Global task dependencies returned unqualified (:2698-2703, :2733-2738), so a follow-on task_get(dep) resolves against the wrong graph. 11. The collision guard (:356-367) probes with peek_node + exact-title SQL while the write uses resolve_node_for_write → _nodes_named (title or alias), so the guard has false negatives.

All six Adapt findings are real. I disagree with their severity ordering — they rated the edit_policy leak as a config-construction nit; it's a clone-controlled trust-boundary break. They missed items 1, 2, 6, 7 and 9, including the single-graph reordering that affects every user.

Tests

tests/test_mcp_cross_graph.py (209 lines, 13 tests) does not test store selection. The fixture (:13-30) monkeypatches server._store and server._config directly, bypassing load_config — it asserts behaviour given a correctly-chosen pair of stores, which is the one thing that isn't broken in the field. The only selection test (:191-209) asserts the current defective outcome: cfg.data_path == local_dir.

Nothing would fail if a write went to the wrong store. Absent: a default add() followed by a read that must find it; link(<new>, <bare id existing only in global>); single-graph ordering parity with main (would have caught 1); a stale or profile-mismatched global store (4, 5); edit_policy isolation (3); context/ask/prime parity (9).

What this gets right

The diagnosis is correct and the missing concept is named in the right place: the MCP surface had one implicit store and no vocabulary for a second, and global:<id> is the right primitive. Once a reference carries its authority, show, edit, supersede, verify, invalidate, link, graph_merge, watch_resolve, lock_*, stale_check --rebind and task_* all route consistently — and the PR wires every one, not a convenient subset.

It refuses rather than fakes the impossible case. SQLite edges can't span databases, and instead of inventing a shadow edge table or silently dropping the link, the cross-store paths fail with an explicit message. That's the correct answer.

The read-only secondary store is defensively right: store.py:377-396 uses a real mode=ro URI plus PRAGMA query_only=ON — enforcement, not convention. It refuses to migrate, stamp, or create a missing database (:318); vectors.ensure_vec_table gets a matching guard (vectors.py:615-630) so a search can't rebuild vector state in a graph it's only visiting; and store.py:2061 suppresses the last_accessed write so reading the global graph doesn't perturb its decay signal. The test at :58-76 proves all of it, including that the connection rejects an UPDATE.

The hard boundary is preserved — _global_store returns None when an explicit profile is active (:298-300), so anyone who deliberately sequestered their graphs sees byte-identical behaviour.

And the comment at :315-317 — "Fail visibly if an existing secondary graph is unreadable; silently returning only project results would recreate the original false negative" — shows the author understood the actual hazard class. The intent is right even where the implementation overshoots.

Gate item 1 behind if home is not None, build home_config from user-level layers, return typed results for 4 and 5, and default add to the graph search is reading, and this is a good PR.

Verdict: CHANGES_REQUESTED

Right diagnosis, right primitive — but it doesn't fix the live defect, it silently changes search ranking for every existing user including single-graph ones, and it lets a cloned repo's .kin/config override mutability policy on the user's personal graph.

Upstream fixes worth making regardless of this PR

  1. Presence must stop being an authority. config.py:1003-1027 should require an explicit opt-in — a store: project key in .kin/config, or KIN_PROJECT_STORE=1 — before a repo-local database outranks the configured data_dir. A file created by a different lane is not a user decision.
  2. integrations.modern_codebase_store must not create a store that silently re-points another lane.
  3. Make the write default follow the read. If search merges two graphs, add with no graph must not silently pick the one with 1 node.
  4. Add the missing test: default add, then search/link for that node, across the full load_config path — not a monkeypatched store pair.

@blast-hardcheese

Copy link
Copy Markdown
Collaborator Author

@jmc-wander The correctness findings in your review have been addressed and pushed, including the broader read/write consistency issues beyond the six Adapt findings.

  • Native hybrid ordering and scores are preserved for a single contributing graph. Federation uses per-graph ranks, keeps semantic/graph hits in their native order, and labels its score explicitly.
  • Default retrieval includes the selected project graph, so default captures remain discoverable. Context, ask, prime, listings, diagnostics, and read resources now follow the same graph scope. Explicit project reads do not open the secondary graph.
  • Qualified references survive result and task-dependency round trips. Source-based captures route to their evidence's graph, with configured-global precedence for mixed evidence. Ambiguous IDs/titles/aliases and cross-store links/dependencies fail before mutation; contextual writes prevalidate their targets.
  • User/global policy is isolated from repository overrides, including when a profile or external store is the primary. Secondary reads remain read-only; routed global writes do not introduce a profile stamp. Stale schemas produce typed failures, cleanup preserves the original error, and global grounding/trust warnings remain visible.
  • New integration tests exercise the real configuration loader with a nondefault global directory: default add → search → link, global-derived captures, policy boundaries, and failure containment. Additional regressions cover the affected MCP surfaces.

One proposed policy change is deliberately not adopted: the requester explicitly wants automatic repository .kin/ support, so local presence continues to select useful project information by default. Source-free captures remain in that selected store; explicit qualified evidence supplies derivation rather than guessing it from prose. The read/write consistency fix is implemented around that policy.

Published head: c327820cc99d41d6ea8f907eee411a973e1eb7a7, including current main. Full local suite: 3,058 passed, 4 skipped, 9 subtests passed; current-head CI is green. The PR remains ready for review.

Session-qualified refs remain session-bound authority, not persistent graph IDs. Sibling-worktree discovery and archive reconstruction remain separate scope. No release or deployment was performed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants