kg: cut da kg over to the kg-native CRG backend (bridge kept as rollback) - #553
Open
NikashPrakash wants to merge 24 commits into
Open
NikashPrakash wants to merge 24 commits into
NikashPrakash wants to merge 24 commits into
Conversation
Publish the eight §11.1 parity rows (build, update, status, impact-radius, flows, communities, postprocess, detect-changes) plus the two bulk-export reads as one interface, so the CRG cutover is a provider swap rather than a rewrite. CRGBridge is asserted against it; NativeGraphDBPath fixes the kg-native graph's repo-local location next to the bridge's CRGDBPath so the two backends can never share a database file. Add NewMCPServerWithProvider so the MCP server takes an explicitly selected backend instead of hard-coding NewCRGBridge, and fall back to the backend's own graph when the warm mirror holds no code rows (impact-radius seed resolution, semantic search, graph stats, review-context impact). The warm store is a mirror populated by `kg warm --include-code`, not the source of truth, so without the fallback the skill-critical tools report an empty graph after the cutover.
Add internal/codegraph, an Engine satisfying graphstore.CodeGraphProvider with no subprocess of any kind. Two halves: - scan.go ingests the repository with go/ast into the normalized symbol corpus the kg-native crg adapter models: Function/Type symbols named <pkgPath>.<Name>, and CALLS / IMPORTS / TESTED_BY references resolved conservatively (same-package and aliased-import hits win; a bare member name resolves only when exactly one declaration carries it, so an ambiguous name yields no edge rather than a wrong one). - engine.go / query.go persist through the published graphstore contract into <repo>/.dot-agents/code-graph.db and answer every derived view by READING THAT STORAGE BACK through the crg adapter's parity-verified derivations (flows, communities, risk index, FTS, impact radius). Nothing is served from the in-memory scan, so a divergent write stays visible — the same readback discipline the §11.6 parity oracles use. namespace.go is the projection that makes this work: persisted nodes/edges are lowered through crg.Corpus.ToGraph, so the note/edge field names and the symbol-id derivation have exactly one definition, shared with the adapter's own ingestion path. NullProvider is the `none` adapter's backend: every operation succeeds with an empty, well-formed result, so "this project consumes no graph backend" is a first-class configuration rather than a nil every call site must guard. One deliberate improvement over the bridge: an unresolvable diff base (the common `HEAD~1` on a repository whose only commit is the root commit) falls back to every tracked file instead of hard-erroring, which is what made build_or_update_graph_tool unusable on a fresh repo.
Register the CRG family in both production registries via crgbridge.RegisterCRGFamily — not two bare Register calls, because that entry point also runs registry.EnforceReadsFrom, without which the §11.2 migration_only gate is registered but inert. Add commands/kg/backend.go as the single backend-selection chokepoint: DA_KG_GRAPH_BACKEND, then .agentsrc.json kg.graph_backend, then the kg-native default. The ref resolves through the built-in adapter registry, so an unregistered or version-incompatible ref is rejected rather than silently defaulting, and a bare adapter name is accepted and expanded. Re-point every consumer off NewCRGBridge onto codeGraphProvider: sync_code_warm_link.go (build, update, code-status, impact, flows, communities, postprocess, changes, readiness, warm code import), bridge.go (change_analysis, community_context) and `da kg serve`. Graceful degradation is preserved and generalized: the bridge-era DiscoverCRGBin probe becomes errBackendUnavailable, so the post_tool_use update hook, the warm code lane and the readiness checks still no-op instead of failing a session when the selected backend's tooling is absent. The bridge is kept and stays reachable behind explicit config — the §11.4 rollback path. Existing subprocess-routing tests are pinned to it rather than deleted, so they remain the regression coverage for that path.
Update docs/crg-bridge-consumer-audit.md to the post-cutover state: the consumer table, how backend selection works, where the kg-native graph lives, and a delta table naming every observable difference from the Python bridge (Go-only ingestion, degree-centrality risk in the review-context composite, empty community descriptions with structural cohesion, positional flow ids, no-op skip flags, postprocess as a recompute-and-stamp pass, and kg_crg.* SDK persistence still being Phase B work). Teach scripts/crg-bridge-consumer-audit.sh to derive §11.4 condition 3 instead of asserting a hardcoded "no kg-native replacement wired", and fix the graceful-degrade probe to match the new hook message, so --check stays a truthful drift gate. Add a §11.4 progress note recording that condition 3 is met and that the condition 1/2 soak clock restarts, because the parity matrix must now soak against a native path that is actually serving production. The four gate conditions themselves are unchanged.
Pin the upstream tool surface, prompts and release metadata as a generated, byte-stable contract with a capability map that records, per tool, whether the native backend serves it or the bridge does and why.
Route release tool calls through the bridge, expose the derived community, flow and impact views the native handlers read, and record the upstream release identity alongside the build and update reports.
Add the native tool handlers, the build/update lifecycle, and the language capability contract that decides which sources the native scanner indexes. The embedded language inventory mirrors the generated release fixture and a drift test pins the two together.
Validate every tools/call against the release schema, then answer natively where the engine reproduces the release and via the bridge otherwise, and report the split through a graph capability diagnostic.
goreleaser 2.18.1 errors on homebrew_casks.url.verified: Homebrew removed the stanza and goreleaser no longer writes it. The block held nothing else, so the anchor and its alias go with it.
`git commit` spawns a detached `git maintenance run --auto` that keeps creating and unlinking transient paths under .git after the commit returns, racing both the template copy's WalkDir and a fixture's own TempDir teardown. Disabling it in the template config covers every repo cloned from it.
- codegraph: a NAMED int64 reached writePyJSON's reflect fallback and rendered as a quoted string while encoding/json spells it as a number, so a payload carrying such a field would have shifted every context_savings denominator without changing the payload. - graphstore: NewCRGToolBridge now resolves the interpreter pythonBin falls back to. The fallback is a bare name that is never empty, so a release whose virtualenv lost its interpreter reported the bridge as available and failed on the first call with a raw exec error instead of naming what was missing. - codegraph: flowRowToMap returns the decoded node-id path, so a flow's stored path is decoded once per row instead of twice, and flowSteps no longer re-checks an error its caller already returned on. - codegraph/crg: releaseBlobEquals' error result was always nil and round4/roundTo4 guard a ParseFloat that cannot reject FormatFloat's own output; both guards are removed rather than left unreachable.
Behaviour-driven tests for the release contract, the native tool handlers, the capability router and the retained bridge: the published 18/12 native-vs-bridge split and its named reasons, the release's own argument validation vocabulary, every query_type mode and its payload, the flow, community, impact and review-context views with their result bounds, the native scanner's node identity and call-target resolution, changed-path discovery over real git repositories, and the lazy store's derived-view delegators. The nine files that remain short are allowlisted with a per-file rationale: each holds a defensive guard on a condition the surrounding control flow has already excluded. The ratchet moves 179 -> 188.
- kg sync runs git with maintenance.auto=false and gc.auto=0. git otherwise spawns a DETACHED `git maintenance run --auto` that inherits the CLI's stdout and outlives the pull, so a caller reading that handle keeps waiting after `da kg sync` has returned. - Release-contract fixtures substitute the SLASH-NORMALIZED repository root. Node identity is slash-normalized by construction (scan.go), so splicing a native Windows root produced a hybrid `C:\...\001/pkg/...` expectation the product never emits; it also fed a backslash into the JSON-encoded critical_path, where the encoder correctly escaped it and the fixture did not. Both suites now match the one convention the rest of the package uses. - assertRowsEqual prints the differing field and both values; "row N mismatch" cost a CI round trip to diagnose. - A working-tree fixture named `git~1` at a repository root resolves onto .git's NTFS 8.3 alias; it moves under a subdirectory, where go-git's tree path validator still refuses it, so the assertion is unchanged. - -timeout goes back to 1200s. `da kg` now builds the graph in-process instead of shelling to the Python release, so commands/kg does real parse and sqlite work per test; under -race on windows-latest that passed 600s.
captureStdout read the pipe only AFTER fn returned, so fn blocked forever the moment it printed more than one pipe buffer. Windows os.Pipe buffers far less than Linux's 64KiB, and the cutover made `da kg` print materially more, which is why the package wedged on windows-latest with no goroutine dump: the runtime's own timeout output went into the same abandoned pipe. The restore was also inline after fn. fn is test code that can t.Skip (the POSIX-shim guard does exactly that on Windows), t.Fatal or panic, each of which unwinds the frame — leaving os.Stdout pointing at a dead pipe for every following test in the package. 117 call sites, 133 Fatal/Skip sites inside them. The reader now runs concurrently and the restore is deferred, and the two hand-rolled copies of the same pattern route through the helper.
The generator shells out to an operator-named executable and rewrites an operator-named tree, so it now refuses to do either for a value it was not designed for: --crg-bin must resolve to a runnable code-review-graph (the basename is matched against an allowlist and the path re-derived from it), and --out must resolve inside the working directory. Both reject with an actionable message before anything is written or spawned. Also splits the two functions that had grown past the complexity gate and hoists the fixture path repeated thirteen times into a constant. Verified reproducible: regenerating into a scratch tree yields all 120 fixture files byte-identical to the committed ones.
replaceRows assembled `DELETE FROM ` + table, the one dynamically formatted statement in the store. A table name cannot be a bound parameter and the set of tables this path may truncate is closed, so each caller now issues its own literal truncation inside the shared transaction and the helper only owns the transaction and the row count. Alongside: ReplaceFlows' generation swap and insert split into named helpers, the repeated "invalid params" and "rev-parse" literals become constants, SetNodeCommunity's parameter list groups its two int64s across the interface and all three implementations, and the blank embed import says why it is there.
Every handler and helper the complexity gate flagged is split into named, doc-commented pieces along the seams it already had: the transitive-test collector into input expansion plus a direct, a bare-name and a call-hop pass; hybrid search into ranking, scoring and bounding; the traversal into frame pop, visit and neighbour append; the architecture overview into its community index, cross-edge aggregation and coupling warnings; the scanner's call-target resolution into a declaration index and receiver candidates. Extraction only — no logic, ordering or payload changes, and the shapes that would have added an untested branch were deliberately left as they were. The byte-for-byte release-fixture comparisons and both per-file coverage gates are unchanged. Also hoists four repeated literals into constants and explains the three intentionally empty bodies.
Fat test bodies and recursive payload comparators move into named helpers so the t.Run closures stay thin, and `max` stops shadowing the builtin. No case, assertion or message is dropped — the extraction is mechanical, which is why statement coverage is unchanged.
|
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
1 participant
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.
CRG decommission Phase A, workstream 1: cut
da kgover from the Pythoncode-review-graphsubprocess bridge to the kg-native adapter. The bridge iskept and stays reachable behind explicit config — this PR deletes nothing.
Task:
graph-backend-adapter-contract / t6-bridge-decommissionSpec:
.agents/workflow/specs/graph-backend-adapter-contract/design.md§11.4(progress note added; the four gate conditions are unchanged)
Audit:
docs/crg-bridge-consumer-audit.mdWhat changed
1. The backend is now a published contract.
graphstore.CodeGraphProvider(internal/graphstore/provider.go) is exactlythe eight §11.1 parity rows — build, update, status, impact-radius, flows,
communities, postprocess, detect-changes — plus the two bulk-export reads the
warm-link sync needs.
*CRGBridgeand the new native engine both satisfy it,so the cutover is a provider swap rather than a rewrite.
2. The kg-native engine (
internal/codegraph).scan.goingests the repo withgo/astinto the normalized symbol corpusthe
crgadapter models:Function/Typesymbols named<pkgPath>.<Name>and
CALLS/IMPORTS/TESTED_BYreferences. Resolution is conservative:same-package and aliased-import hits win, and a bare member name resolves
only when exactly one declaration carries it, so an ambiguous name produces
no edge rather than a wrong one.
engine.go/query.gopersist through the publishedgraphstore.CodeGraphWritercontract into<repo>/.dot-agents/code-graph.dband compute every derived view by reading that storage back through the
crg adapter's parity-verified derivations. Nothing is served from the
in-memory scan, so a divergent write stays visible — the same readback
discipline the §11.6 oracles use, and the reason the CLI and the parity gate
cannot drift apart.
namespace.golowers persisted nodes/edges throughcrg.Corpus.ToGraph, sothe note/edge field names and symbol-id derivation have one definition,
shared with the adapter's own ingestion path.
NullProviderbacks thenoneadapter, so "no graph backend" is afirst-class configuration rather than a nil every call site must guard.
3. Backend selection.
commands/kg/backend.gois the single chokepoint:dotagents-builtin:graph/crg@^1.0codegraph.Engine(in-process)dotagents-builtin:graph/crg-bridge@^0.1graphstore.CRGBridge(Python)dotagents-builtin:graph/none@^1.0codegraph.NullProviderResolution order:
DA_KG_GRAPH_BACKEND→.agentsrc.jsonkg.graph_backend→ kg-native default. The ref is resolved through the built-in adapter
registry, so an unregistered or version-incompatible ref is rejected instead
of silently defaulting; bare names (
crg,crg-bridge,none) are expanded.Both registries now register the family via
crgbridge.RegisterCRGFamily—not two bare
Registercalls, because that entry point also runsregistry.EnforceReadsFrom, without which the §11.2migration_onlygate isregistered but inert.
4. Consumers re-pointed.
sync_code_warm_link.go(11 ref-lines) andbridge.go(2) no longer referenceCRGBridgeat all;da kg serveuses thenew
NewMCPServerWithProvider. Graceful degradation is preserved andgeneralized: the bridge-era
DiscoverCRGBinprobe becomeserrBackendUnavailable, so thepost_tool_useupdate hook, the warm codelane and the readiness checks still no-op rather than failing a session when
the selected backend's tooling is absent.
5. MCP fallbacks. Several tools read the warm store
(
$KG_HOME/ops/graphstore.db), which is only populated by an explicitkg warm --include-codemirror pass. With the code graph now living with thebackend,
get_impact_radius,semantic_search_nodes,list_graph_statsandget_review_context's impact section fall back to the backend's own graphwhen the mirror is empty — otherwise they would have started reporting an
empty graph.
Tool-shape deltas (documented, not silent)
Full table in
docs/crg-bridge-consumer-audit.md. The load-bearing ones:languages; the native ingester is Go-only today. Non-Go repos build an
empty graph on the native backend; the rollback backend still covers them.
Closing this is prerequisite to t6d deleting the bridge.
get_review_context_tool— same JSON keys and value types, butchanged_symbolsis derived from the persisted graph (degree-centralityrisk normalized 0–1, caller counts,
TESTED_BY-derived test gaps) ratherthan the CRG composite's own heuristic + LLM-assisted summaries.
semantic_search_nodes_tool— unchanged by this PR (it already read thewarm store, not the bridge). The vector-search richness gap is pre-existing;
this PR only adds a backend fallback so it stops returning empty.
description— empty (the bridge's were LLM-authored);cohesionis now a structural connected-pair ratio. Members, size,dominant language and ids keep their shape.
keys a flow by its entry-point symbol id, a string). §11.6 already compares
flows by
(flow_id, member_id, position)set equality, not by id.--skip-flows/--skip-postprocess— accepted, no effect: derivedviews are computed on demand, so there is no materialization pass to skip.
postprocess— recomputes the derived views and records their sizes asstore metadata rather than rebuilding tables.
kg_crg.*SDK namespace — the engine persists throughCodeGraphWriter, not a SQL-backedsdk.Store. Building that store soingestion literally lands in
kg_crg.*is Phase B; the parity-verifiedderivation code is shared either way.
One deliberate improvement: an unresolvable diff base (
HEAD~1on arepository whose only commit is the root commit) falls back to every tracked
file instead of hard-erroring, which is what made
build_or_update_graph_toolunusable on a fresh repo.
Soak clock
This restarts the §11.4 condition 1/2 soak clock. The hermetic parity gate
has been green for weeks, but against a native path nothing was calling; from
this merge the matrix soaks against a native path that is actually serving
production. Condition 3 is now MET; 1, 2 and 4 remain open, so the audit's
verdict stays
NOT-READY — KEEP THE BRIDGE.Explicitly out of scope (Phase B / t6d): deleting any bridge code, the CI
.venvcode-review-graphinstall, or the parity gate.Verification
go test ./...(skipping the known-hangingTestConfigLoadSave) — green.go test -race -count=1 -run '^(TestPostprocessParity_TenCommitDualRead|TestPostprocessParity_CatchesDivergence|TestRegression_CrossPathParity_ClampsAreTheSameNumbers)$' ./internal/adapters/builtin/crg ./internal/graphstorego vet ./...andfsguardclean.internal/codegraph97.9%,commands/kg/backend.go100%,commands/kg95.7%,internal/graphstore95.8%.bash scripts/crg-bridge-consumer-audit.sh --check docs/crg-bridge-consumer-audit.mdpasses (doc and script back in lockstep).
No-Python e2e, sandboxed
HOME/AGENTS_HOME/KG_HOME, throwaway gitfixture,
PATHscrubbed ofcode-review-graph:Then flipping the backend, both via env and via
.agentsrc.jsonkg.graph_backend: crg-bridge, routes to the Python bridge and degrades asdesigned (
code-statusreports the reason,builderrors, theupdatehookno-ops at exit 0) — the routing is what is under test, and the CLI is absent
in the sandbox by construction.
All eight MCP tools exercised over stdio on the native backend, same names and
shapes: