kg: index git submodules in code-graph builds and report coverage honestly - #557
Open
NikashPrakash wants to merge 15 commits into
Open
NikashPrakash wants to merge 15 commits into
NikashPrakash wants to merge 15 commits into
Conversation
`git ls-files` reports a submodule as a single gitlink entry, never the files inside it, so a superproject build parsed only root-level source and still reported success — measured live at 47 nodes / 2 files where the workspace held 5,946 nodes / 885 files. Enumeration is now submodule-aware. DiscoverSubmodules reads the gitlink entries out of the git index (including nested ones), PlanWorkspace resolves the roots a build will cover, and BuildReport builds the superproject plus every initialized submodule. A submodule that cannot be indexed (uninitialized, unreadable, or excluded via NoRecurseSubmodules) is recorded with a reason rather than dropped. Submodule graphs are merged into the superproject graph under a per-repository scope: node qualified names and BOTH edge endpoints gain a `<submodule-path>::` prefix. CRG resolves edges by qualified name, so without that discriminator a merged graph linked every `Button` to every other `Button` and impact radius reported edges no build could produce. Superproject rows stay unscoped, so a single-repo graph is unchanged. Postprocess ordering is now structural: each root builds with postprocess deferred, the merges land, and one postprocess pass rebuilds flows, communities, and the FTS index over the merged rows. Merging after a postprocess pass is what left a merged graph with populated base tables and an empty search index that still looked healthy. Status attributes rows per root and downgrades a graph that is missing a repository to the new `incomplete` state (ready=false) naming the root and why, so a 98%-incomplete index can no longer read as READY.
`kg code-status` now prints the node/file counts per repository root and flags any root present in the checkout but absent from the graph, so the operator sees what the totals actually cover. `kg build` warns instead of printing a success box when the resulting graph is incomplete, and --require-graph consumers refuse an incomplete graph: partial results are harder to notice than empty ones. `kg build --no-recurse-submodules` opts out of indexing submodules. The exclusion is reported in the build summary and the readiness status — the opt-out is a visible choice, never the silent default it used to be. `kg update` covers the superproject only (git diff does not descend into gitlinks); it now says so rather than leaving the operator to assume a refresh covered the whole workspace.
Records the resolution for all three gaps plus the cross-repo false-edge finding, the deliberate precision-over-recall trade in scoped edge resolution, and what stays out of scope (incremental update still covers the superproject only).
The workspace build's postprocess pass went through Postprocess, which execs the CRG console script directly. Builds go through runCaptured, which wraps a Python entrypoint in the interpreter and forces SQLite autocommit — the wrapper this pass needs most, since it writes the derived tables. Verified against a real code-review-graph install: a console script with a stale shebang failed the whole build at the postprocess step even though both per-root builds and the merge had succeeded. The flag translation is now shared by both paths. Also widens per-root row attribution to match a file path stored with either native separators or slashes, so status counts are correct on Windows and against graphs written by an external aggregator.
Adversarial review of the workspace build surfaced four defects; this is their fix. The merge now owns its namespace: it deletes the scope's existing rows before copying the source in. `nodes` would have survived on its unique qualified_name, but `edges` carries no unique constraint, so re-merging a submodule duplicated every one of its edges and inflated impact radius and flow detection. Replacing rather than inserting also drops symbols the submodule deleted instead of stranding them forever. Readiness gained a way out. A submodule the build deliberately excluded, or one it indexed and found no symbols in, is not a missing root — but status runs long after the build and had no way to tell either case from a root that was never looked at, so both reported incomplete forever and --require-graph could never pass again. A build now records the roots it covered and the ones it was told to skip in a sidecar next to the graph; status reads it and downgrades only for roots that are genuinely absent. Row attribution is one exclusive pass. Each row lands in its innermost root (CASE arms ordered deepest-path-first), so a nested submodule's rows are no longer counted under both roots — arithmetic that could drive the superproject's reported share negative. The single scan also replaces one full table scan per submodule on a hot path, and comparing slash-normalized paths makes attribution behave the same on Windows. Also: each submodule merge gets its own connection with the busy_timeout this package's other writers use, so a concurrent reader makes the merge wait rather than discarding every per-root build that preceded it; gitlink paths from an untrusted index are rejected if they escape the checkout; and an enumeration failure is reported instead of silently degrading the build to single-root behavior.
The opt-out and the coverage record changed what readiness reports: an excluded or symbol-free root is named but no longer makes the graph incomplete. Bring COMMANDS, the changelog, and the proposal's resolution in line, and record the limitation that remains — readiness detects a wholly missing root, not one that indexed a fraction of its own files.
The containment check used filepath.IsAbs alone, which on Windows is false for a rooted-but-driveless path: an index entry spelled `/etc/passwd` came through as `\etc\passwd`, passed the check, and would have been walked as a submodule root. Caught by the windows-latest CI job. Rooted and volume-qualified paths (`\x`, `C:x`, `\\host\share\x`) are now rejected alongside absolute ones.
SonarCloud flagged five dynamically formatted SQL statements in the merge. Two were plain constant concatenation (ATTACH/DETACH of a fixed alias) and are now literals. The other three assemble identifiers — a table name from a two-value package constant set, and a column list read back from the databases' own schema and double-quote-escaped. No caller input reaches the statement text; the repository scope and the source path are bound parameters. S2077 cannot see that, so it is suppressed for that one file with the reasoning recorded next to the existing e2e suppression.
Sonar's new-code gate flagged three issues on the previous push. BuildReport carried the whole outcome mapping inline (cognitive complexity 18); the workspace attachment and the state→summary mapping are now their own methods on the report. The unusable-sidecar test's fixture writing moved to a helper, and the incomplete-graph warning title is a single constant rather than three copies. No behavior change.
Replace the shelled-out `git submodule`, `git ls-files`, and `git diff` reads in the code-graph lane with in-process go-git equivalents: submodule and index enumeration off the worktree, plus a native merge-base changed-files diff. A superproject build now covers the root and every initialized submodule, merges the submodule graphs under a per-repository scope, and postprocesses once over the merged result. Drop the S2077 suppression: the graph merge assembles its statements from package constants rather than identifiers read back from the databases' own schema, so there is no dynamic SQL left to exempt.
Add an AST gate that requires every os/exec and execabs Command, CommandContext, and LookPath site in shipping Go to carry an exact ledger record naming the file, function, executable, and why no in-process API exists. Git is categorically unallowable inside the cutover-locked packages (internal/graphstore, internal/codegraph, internal/gitwt, internal/gitremote) now that go-git is vendored and carries those reads. Wire it into CI ahead of the coverage gate so a re-added shell-out cannot ride a green test suite into master.
goreleaser 2.18.1 rejects `homebrew_casks.url.verified` as deprecated, so `goreleaser check` exits non-zero and the Linux test job fails at "Validate GoReleaser config". The `url` mapping held nothing but `verified`, so the anchor and its one alias reference go with it.
`git commit` spawns `git maintenance run --auto --quiet --detach`, and that
DETACHED process keeps creating and unlinking transient paths under .git
(objects/maintenance.lock, multi-pack-index, bitmap-ref-tips_*) well after
the commit returns. Two fixtures lose that race:
* copyGitTemplate walks the shared template repo, so a path it enumerated
can vanish before it is opened — the macOS "open .../.git/objects/
maintenance.lock: no such file or directory" failure in commands/sync.
* a fixture that commits into its own t.TempDir races the teardown
RemoveAll, which rmdir's .git while maintenance is repopulating it.
Pin maintenance.auto=false and gc.auto=0 in each fixture's git config before
the first commit. Copies inherit the template's config, so one setting covers
every repo cloned from it. Measured over 180 stage+commit+RemoveAll rounds:
~5% teardown failures before, zero after, and `git commit` now spawns no
child process at all.
Sonar blocked the PR on 12 new issues in this new package: nine S1192 for file paths and one replacement note repeated three or more times in the ledger, and three S3776 for scanners over the cognitive-complexity limit. Name each multi-record file path and the catch-all git-wrapper note as a constant, so a file's records share one spelling and a rename is one edit. Then lift the shared "walk a GenDecl's (name, value) pairs" loop out of scanFile and literalConsts into eachValueSpecPair, give the func-valued-var branch its own funcValueSites, and move propagateParams' git-argument index build into gitArgPositions. Scanner complexity drops 19/16/20 to 3/7/7; the package stays at 100% statement coverage and the gate's verdicts are 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.
Fixes the open proposal
kg-code-graph-submodule-blindness:da kg buildenumerated files withgit ls-files, which reports a submodule as a single gitlink and none of the files inside it. In the payout monorepo that meant 47 nodes / 2 files indexed where the workspace held 5,946 nodes / 885 files — andcode-statussaid READY. Every downstream consumer degraded silently: impact radius came back empty, flows and communities found nothing, and code-side KG queries were indistinguishable from "this topic genuinely isn't here".This clears a §11.4 trust blocker for t6-bridge-decommission: parity and cutover decisions cannot be read off a graph that silently covers 2% of the workspace.
What changed
1. Enumeration is submodule-aware (
internal/graphstore/submodule.go, new)DiscoverSubmodulesreads gitlink entries out of the git index (nested submodules included; the index is ground truth, so a gitlink with no.gitmodulesentry still counts).PlanWorkspaceresolves the roots a build will cover.kg buildbuilds the superproject and every initialized submodule. Recursion is the default;--no-recurse-submodulesopts out and the exclusion is reported, never silent. A root that can't be indexed (uninitialized, unreadable) is recorded with an actionable reason instead of dropped.2. Readiness is honest — and escapable (
crg.go,coverage.go)CRGStatusgained a per-root breakdown (roots[]: path, nodes, files, indexed, note) and a newincompletestate. A repository present in the checkout but absent from the graph makesreadyfalse, names the root, and is refused by--require-graph— partial results are harder to notice than empty ones.Two cases are deliberately not incomplete: a root the build indexed and found no symbols in (docs-only, or a language CRG can't parse), and a root excluded on purpose. Status runs long after the build and can't otherwise tell either from "never looked at", so both would have been permanently un-ready with no action that could fix it. Each build records the roots it covered in
.code-review-graph/da-workspace.json; status reads it.Row attribution is a single exclusive pass: CASE arms ordered deepest-path-first, so a nested submodule's rows land in their innermost root instead of being counted under both (which could drive the superproject's reported share negative), and paths are compared slash-normalized so Windows and external aggregators attribute the same.
3. The merge routes through postprocess (
internal/graphstore/merge.go, new)Ordering is now structural: each root builds with postprocess deferred → submodule graphs merge → one postprocess pass rebuilds flows, communities, and the FTS index over the merged rows. This closes the recorded trap where a merged graph had populated base tables next to an empty search index and still looked healthy. The merge is transactional and authoritative for its scope — it clears the scope's rows before copying, so a re-merge neither duplicates edges (
edgeshas no unique constraint, unlikenodes) nor strands symbols deleted upstream. Base tables only; derived tables are rebuilt, never copied.4. Cross-repo false edges are gone
CRG resolves edge endpoints by qualified name, so a merged graph linked every
Buttonto every otherButton. Merged rows are namespaced per repository (<submodule-path>::<qualified_name>, applied to node names and both edge endpoints). Superproject rows stay unscoped, so a single-repo graph is unchanged.Deliberate trade-off: scoping trades recall for precision. A genuine parent→submodule reference that only resolved by bare name no longer forms an edge — but under the old behavior it was indistinguishable from a fabricated one, so impact radius on a merged graph is now a lower bound rather than an unquantified upper bound.
Verification
Real end-to-end run against an actual
code-review-graphinstall and a realgit submodule addfixture (superproject + submodule, both definingButton):ls-filesREADY(silently 1 of 3 files)READY, roots:. 4 nodes+vendor/lib 5 nodesnodes_fts = 9,flows = 2,communities = 3— postprocess demonstrably ran over the merged rows, not just the root's.Buttonsymbols coexist (…/main.go::Buttonandvendor/lib::…/widget.go::Button).kg impact vendor/lib/pkg/widget.goreturns a 3-node blast radius where it previously returned nothing.git clone): build reportsincompleteand names it with the fix.--no-recurse-submodules: ready, withvendor/lib: SKIPPED (excluded by --no-recurse-submodules)in the summary and the per-root breakdown; a latercode-statusreads the coverage record and still reports READY.Tests — real git fixtures throughout (
git init+git submodule addover a local path), since the defect is entirely about what git's own enumeration does with a gitlink. Includes a test that pins the old naive merge fabricating a cross-repo edge, next to one proving the scoped merge doesn't. Nested-submodule attribution, rebuild-over-existing-graph, uninitialized/unreadable/excluded roots, and the transaction rollback paths are all covered.go test ./internal/graphstore/... ./commands/kg/...✅ (graphstore 96.2%, commands/kg 96.3%)go test ./... -skip TestConfigLoadSave✅ (parity gate green)gofmt/go vet/fsguard/importguard/scripts/verify.sh(111 passed) ✅submodule.go,merge.go,coverage.go) at 100% statement coverage — the per-file ratchet.An adversarial review pass ran against the first draft and produced four HIGH findings; all are fixed here (the
--jsonstdout corruption, the unreachable-readytrap, edge duplication on re-merge, and the Windows path comparison), along with the nested double-counting, the missingbusy_timeout, and gitlink path containment.Merge ordering
feat/crg-native-cutoveris concurrently re-pointingcommands/kgfrom the Python bridge to the native adapter. This PR keeps its weight at the enumeration/store/merge layer (internal/graphstore) so the two compose, but it does touch shared call sites incommands/kg/sync_code_warm_link.go—runKGBuild(newNoRecurseSubmodulesoption),runKGUpdate(post-update workspace notice),runKGCodeStatus(per-root breakdown), andcheckCRGReadiness(theincompletearm). Those four functions are the expected conflict surface if the cutover lands first.Known limitation, recorded in the proposal: readiness detects a wholly missing root, not a root that indexed a fraction of its own files — that needs a language-aware denominator per root and is separate work.
kg updatestill refreshes the superproject only (git diff does not descend into gitlinks); it now says so.