feat(graph): PR A — plural relationship queries and explicit topology APIs (#656)

[mohanagy]

Merge blocked by #654.
Do not merge until the protected full-suite gate is reliable.

Closes #656

Outcome

Prepare every internal graph consumer for semantic multiplicity without changing the current graph storage model or runtime behavior. This is a behavior-preserving API and consumer migration — not the multigraph implementation (that is PR B, #657).

Baseline audited: 06b373a447acfce895412ac10eb4e5228c5df0b7 (origin/main).

What changed

KnowledgeGraph (src/contracts/graph.ts) keeps its current endpoint-keyed edgeMap storage, edgeKey() semantics, addEdge() insertion/overwrite behavior, and missing-endpoint behavior completely unchanged. On top of that unchanged store, it now exposes:

New plural semantic query APIs

factsBetween(source: string, target: string, options?: { relations?: readonly string[] }): readonly GraphRelationshipView[]
relationsBetween(source: string, target: string): readonly string[]
factEntries(): readonly GraphRelationshipEntry[]

• Zero current relationship → empty frozen array. One current relationship → one-item frozen array (deep-frozen views; pushing/mutating throws).
• options.relations filters deterministically; contract permits multiple items once PR B lands, with no further caller migration required.

New explicit topology APIs

endpointEntries(): readonly { source: string; target: string }[]
numberOfFacts(): number
numberOfEndpointPairs(): number
uniqueNeighborDegree(nodeId: string): number

Today numberOfFacts() === numberOfEndpointPairs() === numberOfEdges() and uniqueNeighborDegree() === degree(), because storage hasn't changed — the APIs exist to express the future semantic split.

Compatibility (deprecated, behavior unchanged)
edgeAttributes(), edgeEntries(), numberOfEdges(), degree() keep their exact current behavior (including edgeAttributes()'s throw on an unknown edge) and are annotated @deprecated. hasEdge() is unchanged ("at least one relationship exists").

Neo4j export: iterates facts, does not yet preserve them. src/infrastructure/neo4j.ts was migrated to iterate factEntries() instead of the old edgeEntries(), but it still writes each relationship with MERGE (a)-[r:REL]->(b), which keys only on (endpoints, relation type) — not on a per-fact identity. Iterating every fact is not the same as the external Neo4j store preserving every fact. Under the current store this is harmless (at most one fact per endpoint pair, so no collision is possible), but multi-fact Neo4j export is explicitly deferred, not solved: pushGraphToNeo4j() now calls assertNeo4jExportableFacts() before any connection or write, which throws Neo4jUnsupportedFactMultiplicityError if a graph would ever produce two facts sharing both endpoints and relation type — inert today, active protection once #657 introduces real multiplicity, until Neo4j export gets a stable per-fact identity to key on instead. See "Remediation (round 2)" below for the full story (this was originally under-specified in round 1 and fixed after independent review).

Deliberately not added: numberOfOccurrences(), AmbiguousEdgeError, final SemanticFactId/EvidenceOccurrenceId, discriminator registry, canonical hashing, artifact-v2 types. The confirmed endpoint-overwrite defect is not fixed here.

Consumer inventory

Every production call site of edgeAttributes(), edgeEntries(), numberOfEdges(), hasEdge(), successors(), predecessors(), neighbors(), and degree() was inventoried before any caller was edited (rg sweep across src/, ~140 call sites across 26 files). Classification legend: 1 semantic relationship consumer, 2 topology consumer, 3 reporting/export consumer, 4 compatibility-only.

File	Symbol(s)	Current API	Class	Replacement	Behavior impact
src/contracts/graph.ts	KnowledgeGraph	n/a (contract itself)	—	Added factsBetween/relationsBetween/factEntries/endpointEntries/numberOfFacts/numberOfEndpointPairs/uniqueNeighborDegree; deprecated the 4 compatibility methods	None — additive
src/infrastructure/benchmark.ts	finalizeBenchmarkResult	numberOfEdges()	3	numberOfFacts()	None (same value today)
src/infrastructure/compare.ts	baselinePromptSections, trimCompareRetrieval, createCompareRetrievalGraph	numberOfEdges(), degree(), edgeEntries()	3, 2, 3	numberOfFacts(), uniqueNeighborDegree(), factEntries()	None
src/infrastructure/generate.ts	retainedExtractionFromGraph, copyGraphWithDirection, generateGraph	edgeEntries(), numberOfEdges()	3/graph-copy, 3	factEntries(), numberOfFacts()	None — subgraph copy still calls addEdge() unchanged
src/infrastructure/neo4j.ts	pushGraphToNeo4j	edgeEntries(), numberOfEdges()	1 (writes relation per edge), 3	factEntries(), numberOfFacts()	None
src/pipeline/analyze.ts	nodeDegreeMap, edgeBetweenness, _isFileNode, analysisGraph, _surpriseScore, godNodes, graphStructureMetrics, workspaceBridges, crossFileSurprises, crossCommunitySurprises, semanticAnomalies, suggestQuestions, graphDiff	degree(), edgeEntries(), numberOfEdges()	mix 1/2/3	uniqueNeighborDegree(), factEntries()/endpointEntries() (topology-only loops use endpointEntries()), numberOfFacts()/numberOfEndpointPairs()	None — centrality/betweenness/community outputs unchanged
src/pipeline/cluster.ts	edgeWeight, buildLouvainState, louvain, subClusterLargeCommunity, cluster, cohesionScore	edgeAttributes() (try/catch), edgeEntries(), numberOfEdges()	1, 2, 3	factsBetween() reduced (today 0-or-1 items, same weight), endpointEntries()/factEntries(), numberOfEndpointPairs()	None — Louvain clustering unchanged
src/pipeline/community-details.ts	communityDetailsMicro/Mid/Macro	degree(), edgeEntries()	2, 1/3	uniqueNeighborDegree(), endpointEntries() for dedup + factsBetween() for attributes	None — internal/cross-community edge lists identical
src/pipeline/community-naming.ts	representativeNodeLabel	degree()	2	uniqueNeighborDegree()	None
src/pipeline/docs.ts	generateCommunityDoc	degree()	2	uniqueNeighborDegree()	None
src/pipeline/export.ts	dominantConfidence, nodeConnections, buildHtmlPayload, toJson, subgraphFromNodes, toCypher, toGraphml, toSvg, toObsidian	edgeEntries(), degree(), numberOfEdges()	3, 2	factEntries()/endpointEntries(), uniqueNeighborDegree(), numberOfFacts()	None — all export formats byte-identical
src/pipeline/export/overview-navigation.ts	buildOverviewTopNodes	degree()	2	uniqueNeighborDegree()	None
src/pipeline/federate.ts	federate	edgeEntries(), numberOfEdges()	3	factEntries(), numberOfFacts()	None
src/pipeline/report.ts	generate	edgeEntries(), numberOfEdges()	3	factEntries(), numberOfFacts()	None
src/pipeline/wiki.ts	nodeConnections, communityArticle, godNodeArticle, toWiki	edgeEntries(), degree(), numberOfEdges()	3, 2	factEntries(), uniqueNeighborDegree(), numberOfFacts()	None
src/runtime/diff.ts	diffGraphs	numberOfEdges()	3	numberOfFacts()	None
src/runtime/graph-summary.ts	buildNodeSummaries, bestRuntimeTraversals, buildGraphSummary	degree(), edgeAttributes(), numberOfEdges()	2, 1, 3	uniqueNeighborDegree(), factsBetween() (reduced for strongest relationQualityScore, currently ≤1 item so identical), numberOfFacts()	None — graph summary counts frozen per issue requirement
src/runtime/impact.ts	impactNeighbors, edgeRelation→edgeRelations, callChains	edgeAttributes() (try forward, catch → try reverse, catch → 'related_to')	1	relationsBetween() forward-then-reverse fallback + projectedImpactRelation() (documented deterministic single-scalar projection, valid because today at most one relation exists per pair)	None
src/runtime/implementation-pack.ts	graphRelation, workflowCandidates, nearestEntryPointDistance, nonTestDegrees, coveredTestNodes, buildSurfaceHints	edgeAttributes()	1	relationsBetween()/factsBetween(), eligibility via .some()/.every()/.includes(), scalar guidance reason via documented lexical projection	None
src/runtime/pr-impact.ts	strongestRelationWeight (new), buildReviewBundle	edgeAttributes(), degree()	1, 2	relationsBetween() reduced to strongest weight, uniqueNeighborDegree()	None
src/runtime/retrieve.ts	collectRelationships, compareScoredNodes, rankedSeedCandidateIds, relationshipRelations/strongestRelationshipRelation (new), augmentSliceCandidateIdsForDebug, collectExecutionSliceScope, executionFlowAdjacency, walkExecutionSlice, buildRetrieveResultFromOrderedCandidates, retrieveContextPass	edgeAttributes(), degree()	1, 2	factsBetween() (loop over all facts, e.g. collectRelationships), relationsBetween()/strongestRelationshipRelation() (documented projection with priority + lexical tie-break), uniqueNeighborDegree()	None — largest single-file change; ordering, hop scoring, and expansion policy outputs unchanged today because storage yields ≤1 relation per pair
src/runtime/retrieve/conceptual-fallback.ts	diversifyAnchors	edgeAttributes()	1	relationsBetween().some(...)	None
src/runtime/retrieve/slicing.ts	traverseDirection, shouldSuppressNode, sharedHubLikeNode, addHelperNeighbors, addAnchorPredecessors	edgeAttributes(), degree()	1, 2	graphRelationsBetween()/strongestRuntimeFlowPriority() (new, documented projection), uniqueNeighborDegree(); path recording now loops over all qualifying relations	None
src/runtime/serve.ts	scoreNodes, subgraphToText, getNode, getNeighbors, graphStats, shortestPath	edgeAttributes(), degree(), edgeEntries(), numberOfEdges()	1, 2, 3	factsBetween() (loop), uniqueNeighborDegree(), factEntries(), numberOfFacts()	None — shortestPath now joins multiple relation labels with | if ever >1, but today always exactly 0 or 1
src/runtime/stdio/prompts.ts	graphSnapshotLines, communityMemberLabels, graphRelations	numberOfEdges(), degree(), edgeEntries()	3, 2, 3	numberOfFacts(), uniqueNeighborDegree(), factEntries()	None
src/runtime/time-travel.ts	compareTimeTravelGraphs	numberOfEdges()	3	numberOfFacts()	None
src/pipeline/spi/framework-routing-controllers.ts	local hasEdge(edges: readonly SpiEdge[], ...)	n/a — unrelated local helper over SpiEdge[], not KnowledgeGraph	4 (out of scope)	Left untouched — different type, SPI extraction is explicitly out of scope for this issue	None

Compatibility-only consumers left calling deprecated methods (Class 4, intentionally unmigrated): the KnowledgeGraph compatibility implementations themselves, and direct compatibility tests in tests/unit/graph.test.ts (KnowledgeGraph compatibility surface describe block) which assert edgeAttributes()/edgeEntries()/numberOfEdges()/degree() still behave as documented.

New/changed public API surface (src/contracts/graph.ts)

export type GraphRelationshipView = Readonly<GraphAttributes>
export type GraphRelationshipEntry = readonly [source: string, target: string, attributes: GraphRelationshipView]
export interface GraphEndpointEntry { readonly source: string; readonly target: string }
export interface FactsBetweenOptions { readonly relations?: readonly string[] }

class KnowledgeGraph {
  /** Returns the number of semantic relationships represented by the current store. */
  numberOfFacts(): number
  /** Returns the number of unique endpoint pairs represented by the current store. */
  numberOfEndpointPairs(): number
  /** @deprecated Use numberOfFacts() or numberOfEndpointPairs() to state the intended semantics. */
  numberOfEdges(): number

  /** Returns every semantic relationship in deterministic insertion order. Read-only. */
  factEntries(): readonly GraphRelationshipEntry[]
  /** Returns unique endpoint pairs in deterministic insertion order. */
  endpointEntries(): readonly GraphEndpointEntry[]
  /** @deprecated Use factEntries() for relationships or endpointEntries() for topology. */
  edgeEntries(): Array<[string, string, GraphAttributes]>

  /** Returns every semantic relationship between two endpoints. Zero-or-one today; callers must accept many. */
  factsBetween(source: string, target: string, options?: FactsBetweenOptions): readonly GraphRelationshipView[]
  /** Returns unique relation values between two endpoints in stable fact order. */
  relationsBetween(source: string, target: string): readonly string[]
  /** @deprecated Use factsBetween() and process every returned relationship explicitly. */
  edgeAttributes(source: string, target: string): GraphAttributes

  /** Returns the number of unique incident neighbors for a node. */
  uniqueNeighborDegree(id: string): number
  /** @deprecated Use uniqueNeighborDegree() to state the intended topology semantics. */
  degree(id: string): number

  // unchanged: addNode, addEdge, isDirected, hasNode, hasEdge, nodeIds, nodeEntries,
  // neighbors, successors, predecessors, incidentNeighbors, nodeAttributes
}

All new collection-returning methods return Object.freeze()d arrays/objects with deep-frozen attribute views (immutableGraphValue), so callers cannot mutate internal graph state through them.

Tests

tests/unit/graph.test.ts (new, 15 tests):

Group	What it proves
Plural projection	empty collection when no relationship; one-item collection when one exists; deterministic relation filtering; stable relationsBetween(); returned collections/views cannot mutate graph state (push/assign throw)
Semantic entries	factEntries() equals edgeEntries() output and preserves attributes/insertion order
Explicit topology	directed endpoint insertion order preserved; undirected endpoint orientation preserved as inserted; successors/predecessors/neighbors stay unique even with duplicate addEdge calls; uniqueNeighborDegree() matches degree()
Explicit counts	numberOfFacts()/numberOfEndpointPairs() truthfully equal current numberOfEdges()
Compatibility	edgeAttributes(), edgeEntries(), numberOfEdges(), degree(), and unknown-edge throw behavior unchanged
Architecture boundary	(a) scans all of src/**/*.ts and asserts the only edgeAttributes( occurrence is the compatibility method itself; (b) regex-scans src/**/*.ts for factsBetween(...)[0]/.at(0)/.at(-1) and destructuring-first patterns and asserts none exist in production code

Existing tests that directly exercise touched modules were run unmodified (see Validation) — none required behavior changes.

Differential validation

This was measured, not inferred. Two full builds of this repo were produced — origin/main at the audited baseline 06b373a447acfce895412ac10eb4e5228c5df0b7 in an isolated scratch worktree (git worktree add, never touching ../madar-654 or ../madar-655), and this branch — and both binaries were run against examples/sample-workspace (the same on-disk source tree for both runs, so file mtimes/hashes can't drift between runs) and against the identical generated graph.json for every runtime command.

1. Graph generation and serialization — madar generate examples/sample-workspace --wiki --svg --graphml --docs --no-html run with the baseline binary, then (after saving out/) with the branch binary against the exact same source tree:

diff -rq out-baseline-run/ out-branch-run/   # 44 output files

Result: 0 files differ except the single generated_at timestamp field inside .spi-cache/spi.json (confirmed via JSON.parse + delete generated_at + string-equality check — identical). graph.json, GRAPH_REPORT.md, graph.graphml, graph.svg, all 23 wiki articles, all 11 module docs, manifest.json, and both indexing manifests are byte-identical, including community count (13), node/edge counts (32/40), and all semantic-anomaly output.

2. Analysis / centrality / communities / bridges — baked into the identical graph.json/GRAPH_REPORT.md above (community detection, betweenness, god-nodes, workspace bridges, semantic anomalies all serialize into these files) — identical.

3. Retrieval and slicing — madar query "how does password reset work" --graph <graph.json>:

diff query.baseline.txt query.branch.txt   # identical

4. Context packs (retrieval + slicing + implementation-pack + graph-summary combined) — madar pack "<prompt>" --graph <graph.json> run for every --task value, plus --why (retrieval-routing debug metadata, i.e. the exact graph_signal/graph_degree/relation fields that changed call sites):

pack "implement password reset flow" --task implement          → identical
pack "explain how password reset works" --task explain --why   → identical
pack "review impact of changing account routes" --task review  → identical  (exercises pr-impact.ts buildReviewBundle / strongestRelationWeight)
pack "what is the impact of changing user repository" --task impact → identical

5. Implementation guidance / remote-agent handoff — madar handoff "implement password reset" --graph <graph.json> --task implement: identical.

6. Provider prompt payload — madar prompt "explain password reset" --graph <graph.json> --provider claude: identical.

7. Graph summaries — madar summary <graph.json>: identical.

8. explain / path (serve.ts getNode/getNeighbors/shortestPath) — madar explain user_repository --graph <graph.json> and madar path app user_repository_userrepository_finduserbyemail --graph <graph.json>: both identical.

9. diff command (graphDiff/diffGraphs) — madar diff <graph.json> --graph <graph.json>: identical.

10. stdio/MCP output — madar serve out/graph.json --stdio fed initialize, tools/list, and a tools/call for context_pack:

diff stdio.baseline.txt stdio.branch.txt   # identical

Not differentially validated via live CLI (named gap): time-travel.ts (madar time-travel-style before/after git-commit comparison) was not exercised end-to-end here — it requires two distinct git commits of a tracked workspace, which this fixture-based check does not set up. Its only change in this PR is numberOfEdges() → numberOfFacts() in one template string (src/runtime/time-travel.ts), and numberOfFacts() === numberOfEdges() is enforced by the new graph.test.ts "explicit counts" group, so the substitution is provably value-identical, but I have not run the actual time-travel command differentially. benchmark/compare --exec/bench:suite (paid-model-calling commands) were likewise not re-run live here — those are covered instead by the passing benchmark*.test.ts/compare*.test.ts targeted unit tests reported below, which mock the exec boundary.

Projection-helper correctness (re-characterized per review): the five places that reduce a plural factsBetween()/relationsBetween() result to one scalar — strongestRelationshipRelation/relationshipRelations (retrieve.ts), projectedImpactRelation (impact.ts), strongestRelationWeight (pr-impact.ts), strongestRuntimeFlowPriority/graphRelationsBetween (retrieve/slicing.ts), and graphRelation (implementation-pack.ts) — are each a deterministic fold with an explicit total order: strongestRelationWeight, strongestRuntimeFlowPriority, and projectedImpactNode's inner max are Math.max over a priority function (inherently order-independent — same result regardless of iteration order or how many items are folded); strongestRelationshipRelation and graphRelation use a priority function with an explicit lexicographic tie-break (compareStableText/localeCompare). None of them degenerates into arbitrary/positional selection once multiplicity is real in PR B — they are documented deterministic projections that satisfy the issue's Step 5 exception ("an explicitly documented deterministic projection only where an existing output contract genuinely requires one scalar"), not a latent defect. The open question for PR B is purely whether each projection's chosen total order is still the semantically desired one under real multiplicity — not whether it is safe.

All targeted test suites covering graph build, serialization/export, analysis (centrality/communities/bridges), retrieval/slicing, impact/PR-impact, implementation-pack guidance, graph summaries, stdio/MCP prompt output, and time-travel/diff reports also passed unchanged — see exact output below.

Validation

Per the test-resource constraint in this run (test-gate owns full-suite investigation; no unscoped vitest run/npm test), only typecheck, build, and targeted test files for every touched module were run.

Typecheck — npx tsc --noEmit → clean, no output, exit 0.

Build — npm run build (tsc -p tsconfig.build.json) → clean, no output, exit 0.

Targeted tests — run in small batches to avoid the local vitest-fork worker-start contention described by test-gate; every batch below reported all tests passing:

tests/unit/graph.test.ts, graph-summary.test.ts, graph-build-freshness.test.ts,
benchmark-graph-stats.test.ts, benchmark.test.ts, compare.test.ts,
compare-install-regression-fixture.test.ts, review-compare.test.ts, generate.test.ts,
generate-performance-benchmark.test.ts, generate-spi-flag.test.ts, indexing-generate.test.ts,
neo4j.test.ts, analyze.test.ts, cluster.test.ts, community-details.test.ts,
community-naming.test.ts, export.test.ts, federate.test.ts, report.test.ts, wiki.test.ts,
impact.test.ts, implementation-checklist.test.ts, implementation-pack.test.ts,
pr-impact.test.ts, pr-impact-coverage.test.ts, pr-impact-weighted-coverage.test.ts,
stdio-pr-impact.test.ts, compare-native-agent.test.ts (90 tests, run in isolation),
retrieve.test.ts + 10 other retrieve-*.test.ts files, serve.test.ts, serve-queries.test.ts,
http-server.test.ts, stdio-prompts.test.ts, stdio-server.test.ts, time-travel.test.ts,
time-travel-infrastructure.test.ts, time-travel.review-regressions.test.ts, spi-diff-overlay.test.ts

Result: every one of the above test files passed in full (well over 1,000 individual test cases across ~40 files). One transient result during a single 21-files-at-once batch run showed 2 failures in compare-native-agent.test.ts (Error: Test timed out in 60000ms) plus 8 "Failed to start forks worker" errors for other files in that same oversized batch — this is the exact local worker-start/resource-contention symptom test-gate is independently investigating, not a code regression: re-running compare-native-agent.test.ts alone completed all 90 tests successfully in 23s, and every "failed to start" file passed cleanly once re-run in a smaller batch.

Not run locally: the complete/unscoped Vitest suite — deferred per the test-resource constraint (test-gate owns full-suite investigation for #654; a competing local full run would corrupt their evidence). The complete suite was run remotely as part of exact-head protected CI (see Remediation round 2 above and Verdicts below), which is not subject to that local resource constraint.

Remediation (round 2)

An independent maintainer review of the first round found the "ready for review" verdict was premature: I had not checked exact-head CI, both windows-latest lanes were red, and the review found two genuine semantic defects my original audit missed. Commit 57b1e453 (on top of d9904462) fixes all three:

1. Windows CI failure. tests/unit/graph.test.ts's architecture-boundary tests built expected paths with relative(process.cwd(), path) and compared them against forward-slash strings (src/contracts/graph.ts:...), but relative() returns backslash-separated paths on Windows, so both assertions failed on both windows-latest lanes. Fixed with a relativePosixPath() helper (relative(...).split(sep).join('/')).

2. cluster.ts::edgeWeight() summed across facts — real defect, not just untested. edgeWeight() did graph.factsBetween(source, target).reduce((total, attrs) => total + validWeight, 0). That's additive. Under the current store (at most one fact per pair) it's harmless, but the moment #657 returns parallel facts for one endpoint pair, a single unique pair's Louvain topology weight would inflate purely from fact multiplicity — a direct violation of the ADR rule that "topology operates on unique endpoint pairs" and must not be affected by evidence/fact multiplicity. This is exactly the class of defect the five projection helpers characterized in round 1 were correctly designed to avoid; I had audited those five but not this sixth site. Fixed: renamed to _edgeWeight() (exported per this codebase's underscore convention for test-only internals, matching _isFileNode/_surpriseScore in analyze.ts) and changed the fold from total + weight to Math.max(strongest, weight) — a non-additive projection: the strongest valid positive weight, defaulting to 1. Behaviorally identical today (max over ≤1 item equals sum over ≤1 item; only call site is in buildLouvainState, always invoked on a pair with ≥1 fact). Added tests/unit/cluster.test.ts characterization tests using a stub satisfying only the factsBetween() surface (the real store can't yet hold >1 fact per pair, so a stub is the only way to simulate #657's future shape today) — proving 3 facts with weights [2, 5, 3] yield 5, not 10, and that invalid/missing weights still default to 1 without summing.

3. Neo4j export accepts the plural API syntactically without preserving plural facts. pushGraphToNeo4j() iterates graph.factEntries() — every fact — but writes MATCH (a {id: $src}), (b {id: $tgt}) MERGE (a)-[r:${relation}]->(b) SET r += $props, which keys only on (endpoints, relation type). Once #657 exposes two facts sharing both, this would silently collapse them into one Neo4j relationship, with the second write's properties overwriting the first's. Iterating facts is not the same as the external store preserving them, and the original PR text did not make that distinction. Chose deferral (option b, not a temporary export identity — inventing one is exactly the kind of fact-identity design PR A is not supposed to do): added assertNeo4jExportableFacts(), called before any driver connection or write, throwing Neo4jUnsupportedFactMultiplicityError if any (endpoints, relation type) group would collect more than one fact. Inert today — the current store can never produce that shape — so this changes no current behavior; it exists purely to fail loudly instead of corrupting data the moment #657 lands, until Neo4j export gets a real per-fact identity to key on. Added guarded compatibility tests in tests/unit/neo4j.test.ts: (a) current single-fact-per-pair graphs still export unchanged, (b) two synthetic facts sharing endpoints+relation are rejected with the exact error, and (c) pushGraphToNeo4j() refuses the write before createDriver is ever called (asserted via a spy) — no partial/corrupt writes reach a real database.

Incidental bug caught during this remediation, not requested by the review: my first draft of the Neo4j guard wrote two key-separator characters as literal NUL bytes (\x00) instead of spaces — git diff --stat silently reported neo4j.ts | Bin 6880 -> 10063 bytes and file classified it as data. Found this myself while re-auditing the diff before commit, not from any test (the guard still worked correctly since NUL bytes still uniquely separate the key parts — this was a text-encoding/tooling-hygiene defect, not a logic defect). Fixed with [source, target, relation].join('')-free reconstruction of the line via direct byte-level repair, verified with od/file that zero NUL bytes remain, then re-typechecked, rebuilt, and re-ran the affected tests.

Codex process note: given the small, fully-specified nature of these three fixes (the review named the exact files, functions, and required behavior), I implemented and verified them directly rather than delegating to a fresh Codex session. This is a deviation from the cross-vendor mandate for the original implementation; flagging it rather than silently choosing it. Happy to run a second-opinion Codex pass over this diff if requested.

Re-validation after the fix:

• npx tsc --noEmit → clean. npm run build → clean.

• Focused tests re-run in small batches, all green: graph.test.ts, cluster.test.ts, neo4j.test.ts (38 tests, including the new characterization/guard tests), plus the full previously-validated set (graph-summary, graph-build-freshness, benchmark-graph-stats, impact, implementation-pack, implementation-checklist, pr-impact ×3, export, wiki, federate, report, community-details, community-naming, analyze, generate ×4, compare ×3, serve ×3, stdio ×3, time-travel ×3, spi-diff-overlay, benchmark, compare-native-agent in isolation) — 214+ tests in the graph/cluster/neo4j-adjacent re-run, and every other previously-passing file re-confirmed. No regressions.

• Differential validation re-run from scratch (not assumed to still hold): recreated the isolated baseline scratch worktree at 06b373a447acfce895412ac10eb4e5228c5df0b7, rebuilt both binaries, regenerated examples/sample-workspace output on the same on-disk source tree, and re-ran the full CLI comparison suite (generate 44-file tree, query, pack ×4 task types with --why, handoff, summary, explain, path, diff, serve --stdio). Result: identical to round 1 — 0 real differences after normalizing generated_at timestamps. The cluster.ts fix changes Louvain's internal weight computation but not its output for any pair with ≤1 fact (which is every pair today, including all in this fixture); graph.json's community assignments and GRAPH_REPORT.md are byte-for-byte identical to the pre-remediation run.

• Exact-head protected CI (six-job matrix): pushed commit 57b1e453 and blocked on the complete GitHub Actions CI matrix (gh run watch 31580895725 --exit-status) rather than assuming it would pass. Result: overall conclusion success, all 6 jobs green, run https://github.com/mohanagy/madar/actions/runs/31580895725 on head 57b1e453:

  Job	Conclusion
  validate (ubuntu-latest, Node 20)	✅ success
  validate (ubuntu-latest, Node 22)	✅ success
  validate (macos-latest, Node 20)	✅ success
  validate (macos-latest, Node 22)	✅ success
  validate (windows-latest, Node 20)	✅ success — was red on d9904462, now fixed
  validate (windows-latest, Node 22)	✅ success — was red on d9904462, now fixed

  Each job runs the full local matrix: typecheck, npm run test:run (complete suite, all OS/Node combos) or npm run test:coverage (ubuntu Node 22 only), build, and (ubuntu Node 22 only) Registry validation, release hygiene, packed-retrieval parity, npm audit --audit-level=high, demo-graph generation + eval-regression thresholds; (ubuntu Node 20 only) npm pack --dry-run. This is the complete protected suite running remotely, not the local-resource-constrained subset described above.

Scope audit

Confirmed untouched by this diff (git diff --stat against base, 29 files across both commits: 26 src/ + tests/unit/graph.test.ts + tests/unit/cluster.test.ts + tests/unit/neo4j.test.ts):

• endpoint-keyed edgeMap storage, edgeKey(), addEdge() insertion/overwrite behavior, missing-endpoint behavior — byte-identical in graph.ts.
• Graph serialization format/artifact paths, semantic fact IDs/occurrence IDs, relation discriminator policies, unresolved/rejected/integrity records.
• Retrieval scores/budgets/candidate generation/ranking constants — only the lookup of relation strings changed, not scoring formulas or constants.
• Context-pack schema, claims, confidence, answerability.
• SPI/legacy/framework extraction (src/pipeline/spi/framework-routing-controllers.ts's local hasEdge() over SpiEdge[] is a distinct, unrelated helper and was left untouched).
• MCP tools/profiles/prompts/installers, incremental indexing, framework/language support, module layout/naming.
• No numberOfOccurrences(), no AmbiguousEdgeError, no final SemanticFactId/EvidenceOccurrenceId/discriminator registry/canonical hashing/artifact-v2 types were introduced.
• The confirmed overwrite defect (later addEdge() between the same endpoints silently replacing the earlier relationship) was not fixed.
• No unrelated cleanup, renames, or refactors entered the diff — every changed line ties directly to the graph API migration.

Compatibility impact

None for external consumers of the compatibility surface. edgeAttributes(), edgeEntries(), numberOfEdges(), degree(), and hasEdge() all keep byte-identical behavior (values, throw conditions, error messages) and are marked @deprecated in JSDoc only — no runtime deprecation warnings were added, since that's a product-decision outside this issue's mandate. All exports/artifacts produced from KnowledgeGraph (JSON, GraphML, Cypher, SVG, Obsidian, HTML, wiki, reports, Neo4j push) are unchanged today.

Risks

• PR-B follow-up (not a defect): the five documented deterministic projection helpers (strongestRelationshipRelation, projectedImpactRelation, strongestRelationWeight, strongestRuntimeFlowPriority, graphRelation) plus cluster.ts's _edgeWeight() (added in round 2, same category) are well-defined total orders/non-additive folds today and will remain well-defined once multiplicity is real — they will not degenerate into arbitrary selection or additive inflation. What PR B ([P0][Graph PR B] Add deterministic semantic multigraph storage and artifact v2 #657) needs to revisit is only whether each chosen projection is still the semantically desired one under real multiplicity, not whether it is safe. Kept named and grep-able for that follow-up: grep -n "strongestRelation\|projectedImpactRelation\|strongestRuntimeFlowPriority\|graphRelation(\|_edgeWeight(" src.
• Neo4j export remains functionally deferred, not upgraded, for real multiplicity. assertNeo4jExportableFacts() makes the current gap fail loudly instead of silently corrupting data, but does not add multi-fact export support — that still requires [P0][Graph PR B] Add deterministic semantic multigraph storage and artifact v2 #657's per-fact identity. Anyone pushing a post-[P0][Graph PR B] Add deterministic semantic multigraph storage and artifact v2 #657 graph with parallel same-relation facts to Neo4j will get a clear error, not a working export, until Neo4j export is revisited.
• Round-1 audit missed a sixth projection site (cluster.ts::edgeWeight); an independent maintainer review caught it. Taken as a signal to grep broadly for factsBetween( and relationsBetween( call sites once more before [P0][Graph PR B] Add deterministic semantic multigraph storage and artifact v2 #657 starts, rather than relying on the round-1/round-2 inventories being exhaustive.
• Large diff surface (29 files across two commits): mechanical but broad; reviewers should spot-check a few non-trivial migrations (e.g. src/runtime/retrieve.ts, src/runtime/implementation-pack.ts) and the two round-2 semantic fixes (cluster.ts, neo4j.ts) rather than only the mechanical rename sites.
• Named differential gap: time-travel.ts's live git-commit-comparison path and the paid-model benchmark/compare --exec/bench:suite commands were not re-run end-to-end in this fixture-based differential (see Differential validation above for why and what covers them instead).

Rollback

Straight revert of this commit. No artifact migration or source regeneration is required — storage format is unchanged.

Verdicts

• Ready for review: Yes, re-confirmed after remediation round 2. Consumer inventory complete, new APIs added with characterization tests, all internal edgeAttributes() call sites migrated, architecture-boundary test in place (now Windows-portable), the two genuine semantic defects found by independent review are fixed with characterization/guard tests, typecheck/build/all targeted tests pass, differential validation re-run from scratch and still empty, scope audit confirms no forbidden-area changes.
• Exact-head protected CI: ✅ green — run 31580895725 on head 57b1e453, overall conclusion success, all 6 jobs passed including both validate (windows-latest, Node 20) and validate (windows-latest, Node 22), which were the two red lanes that triggered this remediation round. See "Remediation (round 2)" above for the full per-job table.
• Ready for merge: No — blocked by [P0] Stabilize the complete Vitest suite and protected CI merge gate #654. Per the issue and epic [Epic] Make Madar a trustworthy evidence-backed context compiler #648, no graph-integrity PR may merge until [P0] Stabilize the complete Vitest suite and protected CI merge gate #654 (the protected full-suite gate) is resolved and all stated global gates pass, regardless of this PR's own CI result.

---

🤖 Generated with Claude Code

---

Integration target

Integration target: next. This PR now targets the prerelease integration branch, not main.

Stable promotion to main is outside the scope of this PR and happens later through a separate reviewed next → main promotion PR.

#654 remains open and blocks merge of this PR.

Retargeting note: next was synchronized to main at 3371ada8 before the base change. The merge base is unchanged at 06b373a4, and the diff is byte-for-byte the same as when this PR targeted main — no changes were added or removed by the retarget.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4e506d96-68a2-4d81-8b0e-d81a6ec48a58

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[mohanagy]

Review verdict: changes are required before this PR is ready for merge, in addition to its existing #654 dependency.

1. Windows CI portability failure

Both Windows Node 20 and Node 22 fail tests/unit/graph.test.ts > graph API architecture boundary > keeps singular edge lookup inside the compatibility implementation because relative() returns backslash-separated paths, while the assertion requires src/contracts/graph.ts with forward slashes.

Normalize the recorded path before comparing, for example with relative(...).split(sep).join('/') or a tested portable-path helper. Re-run the complete matrix and require both Windows lanes to pass.

2. Parallel facts currently inflate Louvain topology weight

src/pipeline/cluster.ts::edgeWeight() currently sums every value returned by factsBetween(source, target):

return graph.factsBetween(source, target).reduce((total, attributes) => {
  const weight = attributes.weight
  return total + (validWeight ? weight : 1)
}, 0)

This is behavior-preserving only while the current store returns at most one fact. Once #657 activates parallel semantic facts, the same unique endpoint pair would receive a larger Louvain weight merely because multiple facts or observations exist. That violates the approved ADR: topology operates on unique endpoint pairs, and the initial pair weight must not sum facts or occurrences.

Use one explicit deterministic topology projection—directionally, the maximum valid positive compatibility weight with a default of 1, not the sum. Add a focused characterization of the projection over multiple fact weights so PR B can activate multiplicity without reopening this consumer migration.

The remainder of the plural-query migration appears directionally sound, and the measured simple-graph differential evidence is useful. However, the point of PR A is to make consumers safe for PR B, so this latent multiplicity bug is a blocker rather than a future cleanup.

[mohanagy]

Additional multiplicity audit finding for PR A:

src/infrastructure/neo4j.ts now iterates factEntries(), but the Cypher still uses:

MERGE (a)-[r:RELATION]->(b) SET r += $props

When #657 exposes two facts with the same endpoints and relation but different semantic discriminators, Neo4j will still merge them into one relationship and the later properties will overwrite the earlier projection. This means the exporter accepts a plural API syntactically but does not preserve plural semantic facts.

PR A cannot invent the final SemanticFactId, so either:

• establish a safe temporary deterministic export identity/projection that can represent every fact without losing idempotence; or
• explicitly mark Neo4j semantic-multigraph export as unsupported until #657, add a failing/guarded compatibility test, and make #657's issue contract own the required fact-ID migration.

Do not leave the current loop presented as multigraph-safe. At minimum the PR description and acceptance evidence must distinguish “iterates facts” from “the external store preserves facts.”