Commit graph

7 commits

Author SHA1 Message Date
azizur100389
f64cc8b7a8
feat(group): add GraphQL cross-repo contracts (#3070)
* feat(group): add GraphQL contract extraction

* fix(group): tighten GraphQL contract guards

* fix(group): complete GraphQL review hardening

* fix(group): isolate bounded GraphQL reads

* fix(group): harden GraphQL contract extraction
2026-08-29 08:39:09 +01:00
DuduPhudu
0f793558ad
fix(group)!: stop group sync claiming matching it never did (#3020)
* fix(group)!: remove the matching cascade that was advertised but never built

`gitnexus group create` wrote `matching.bm25_threshold` and
`matching.embedding_threshold` into every generated group.yaml, and no matcher
ever read either one. That was not the whole of it — an entire feature surface
described a BM25/embedding cascade that does not exist:

- `matching.bm25_threshold` / `matching.embedding_threshold` — parsed, persisted,
  unread
- `detect.embedding_fallback` — defaulted and templated, unread
- `MatchType` declared `'bm25' | 'embedding'`; both variants unreachable
- `SyncOptions.skipEmbeddings` — declared in sync.ts and never read
- `gitnexus group sync --skip-embeddings` — accepted, threaded through
  GroupService, ignored
- CLI help in en and zh-CN promised "Exact + BM25 only (no embedding fallback)"
- the MCP `group_sync` schema exposed `skipEmbeddings`, described as
  "Exact + BM25 only (Demo PR: same as default exact path)"

`sync.ts` imports exactly `buildProviderIndex`, `runExactMatch` and
`runWildcardMatch`, and the printed cascade has one stage. An operator whose
links do not match reaches for those thresholds first, and turning either knob
changes nothing — config that silently does nothing is how people conclude a
feature is broken.

Evidence that the cascade should be deleted rather than implemented, from a real
backend/frontend pair: of 165 consumer contracts, 149 link exactly and 16 do not.
Nine of the sixteen are third-party APIs (Google OAuth, Apple public keys,
PostHog, image annotation) with no in-group provider by construction — similarity
matching cannot recover them, it can only invent false links. Two are verb
mismatches: the frontend calls `POST /links` and `GET /links/check-exists` while
the backend declares `GET /links` and eleven other `/links/*` routes but neither
of those, so a fuzzy path match would link a POST consumer to a GET provider. The
rest are path-extraction artifacts. Roughly none of the sixteen would be
correctly recovered, and several would be actively mis-linked.

BREAKING CHANGE: `gitnexus group sync --skip-embeddings` and the MCP `group_sync`
`skipEmbeddings` parameter are removed. Both were accepted and ignored, so no
behavior changes — but a script passing the flag now fails with `unknown option`
instead of being silently misled. Existing group.yaml files keep loading: the
removed keys are simply no longer part of the schema, and a regression test pins
that a legacy config carrying all three still parses.

Closes #3006

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(group)!: honour --exact-only, drop inert --allow-stale, report every matching stage

Addresses the review findings on #3020, all of which are the same defect the PR
itself is about: group-sync surface that describes behaviour the pipeline does
not have.

`exactOnly` was inert in exactly the way `skipEmbeddings` was — declared on
`SyncOptions`, threaded through the CLI and the MCP tool, and read by nothing —
and strictly worse, because the stage it promised to suppress DOES run and DOES
write `matchType:'wildcard'` links into contracts.json and the bridge, which
`group impact` and cross-repo `trace` then traverse. It is now honoured rather
than deleted: unlike the never-built BM25/embedding stages, the stage it names
exists, so the flag describes a real choice. The substituted result is
`{ matched: [], remaining: unmatched }`, not an empty result — `wildcard.remaining`
IS `SyncResult.unmatched`, so skipping the stage has to leave its input unmatched
rather than dropping it from the count an operator reads.

`allowStale` had no such stage to gate: `syncGroup` emits no stale warning at any
point (the `checkStaleness` call lives in `groupStatus`, a different path), so it
is removed under the same rationale as `skipEmbeddings`.

`group sync` now prints every matching stage instead of `exact` alone. The old
block printed a `Matching cascade:` header and counted only exact links while the
next line reported `result.crossLinks.length` — which also includes `manifest` and
`wildcard` — so for any group with those the two numbers disagreed with nothing on
screen explaining why. Counting is an exhaustive `Record<MatchType, number>`, so a
new MatchType fails the build here instead of going silently uncounted, and reads
through `?? 0` so a legacy registry carrying a removed matchType prints an honest
count rather than `NaN`.

Also: the MCP `group_sync` description no longer omits the wildcard stage that
always runs, `exactOnly`'s description no longer refers to a "cascade", and
bench/cross-repo-trace/verify.mjs no longer generates the removed threshold keys
into a fresh group.yaml.

Tests: `sync-exact-only.test.ts` pins both directions of the gate (mutation-verified:
removing the gate, or returning `remaining: []`, both go red). `group-tools.test.ts`
pins that the MCP schema dropped `skipEmbeddings` and kept `exactOnly`.
`group-cli.test.ts` pins that both removed flags are rejected, with `--exact-only`
as an accepted-flag control. `config-parser.test.ts` now pins that legacy keys are
PRESERVED (measured, not assumed) rather than only that parsing does not throw.

The type narrowing's fallout in test files is cleared: `tsc -p tsconfig.test.json`
is 987 errors at head against 987 measured on origin/main, with the two error sets
identical — zero net, zero new, zero masked.

Verification: `tsc --noEmit` exit 0; prettier clean; eslint 0 errors (2 warnings,
both pre-existing on base); 69 test files / 1169 tests green across
test/unit/group, test/integration/group, tools, cli-i18n and cli-index-help.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(group): reject malformed and retired group_sync parameters (U1)

`GroupService.groupSync` read `exactOnly` off an untyped MCP payload with
`Boolean(params.exactOnly)`. While the flag was inert that coercion was
harmless; now that it gates the wildcard matching stage, the string "false" --
a routine shape for an LLM caller emitting JSON -- is truthy, so a caller that
asked to KEEP wildcard matching got it suppressed and a registry with fewer
cross-links persisted to disk. The opposite of the request, written down.

Validate instead of coercing, at the service boundary: the MCP SDK does not
enforce a tool's advertised inputSchema and `callTool` is reachable directly,
so this method is the real gate. The validator mirrors `validateImpactMode`'s
`{ value } | { error }` shape -- the established idiom for this boundary, and
the one groupSync's other guards already return through.

Also refuse `skipEmbeddings` and `allowStale` by name. The CLI rejects them
outright because commander errors on an unknown option; the MCP path accepted
and silently dropped them, so an agent working from a cached tool schema was
never told. Removing them took away discoverability, not acceptance.

Both guards run before the group is read off disk, so a rejected call performs
no work. Every test asserts the sync did NOT run -- an error string alone
cannot distinguish "refused" from "refused but synced anyway".

The tool description gains the validation note AFTER the registryOutcome
paragraph: `tools.test.ts` slices that description by ordinal position of the
'preserved' / 'superseded' / 'no-prior-registry' literals, so appending past
all three leaves those slices intact (verified, 44/44).

tsc clean; 1039/1039 group unit tests pass.

* fix(group): record the matching stages a sync was told to skip (U2)

An `--exact-only` sync wrote a contracts.json and bridge with fewer cross-links
and nothing recorded that the wildcard stage had been suppressed by request.
`group_impact` and cross-repo `trace` read that registry as authoritative, so a
narrowed graph was indistinguishable from a complete one -- and because
`group_sync` is MCP-exposed, one agent call durably narrowed the shared answer
for every later reader with no signal at all.

Add `suppressedMatchStages` to ContractRegistry and SyncResult, following the
`unreadableRepos` tri-state end to end: absent means a registry written before
the field existed, `[]` is the measurement "this run suppressed nothing", and a
populated list names the stages. The writer always emits it, because omitting
the empty case is what made "measured, none" unreachable for `unreadableRepos`.

Two properties that are easy to get backwards, and are why the split matters:

- SyncResult carries the marker on EVERY outcome. The sync genuinely did skip
  the stage whatever happened to the file afterwards, and the CLI summary (U3)
  renders from this rather than re-deriving it from the caller's options.
- The PERSISTED registry stamps it only on the `written` outcome. The preserve
  path re-writes `{ ...prior }`, so a carried-forward registry keeps the marker
  of the sync that actually produced its contracts instead of being relabelled
  with this run's request. That holds by construction: the registry literal
  carrying the field is only reachable on the written path.

`loadContractRegistryResilient` gains an explicit line, because it rebuilds the
envelope field by field with no spread of the parsed root -- a new on-disk field
is silently dropped unless named there.

Its reader is `recordedMatchStages`, not the existing `recordedRepoList`: that
one validates `string[]`, which is right for repo names and one notch too weak
here. This repo has already retired MatchType members ('bm25', 'embedding'), so
a stale value on disk is a real shape, and dropping non-members keeps an unknown
stage name from reaching a caller typed as a live one.

Surfaced on `group_contracts` and on `group_sync`'s own return -- deliberately
kept separate from the truncated/truncationReason/riskEpistemic triple. That
triple reports limits a run hit by accident, whose remedy is to fix the repo; a
suppressed stage was asked for, and its remedy is to re-sync without the flag.
Conflating them would tell an agent to retry something that returns identically.

tsc clean; 1043/1043 group unit tests pass.

* fix(group): name a skipped matching stage as skipped, and pin it (U3, U4)

Two facts were printing as the same line. `wildcard: 0 cross-links` meant both
"the stage ran and matched nothing" and "the stage never ran because you passed
--exact-only" -- the same conflation this summary block was introduced to remove
one line up, reintroduced by the flag that made the block necessary.

Render a suppressed stage as `skipped (--exact-only)`, driven by the sync's own
`suppressedMatchStages` rather than by `opts.exactOnly`. The renderer reports
what the sync did, not what the caller asked for, so it stays correct on the
outcomes where the run ended without writing a registry -- which is where the
summary is least legible and a re-derivation from the options would have been
wrong.

Also drops the `?? 0` fallback and the comment justifying it. The comment
claimed a legacy registry could carry a retired matchType into this loop. It
cannot: `syncGroup` returns a freshly computed `crossLinks` array on every
outcome, and even on the preserve path the prior links go to disk while the
fresh array is returned. The code was harmless; the stated reason was false, and
a comment that explains an unreachable path is worse than no comment.

U4 pins both halves through the CLI. A manifest fixture is sufficient: the stage
counts must sum to the total on the `Wrote contracts.json (…)` line, and the
skipped rendering does not need a stage to have matched anything, because
--exact-only records the suppression whatever the fixture holds. That is why
this coverage did not need indexed gRPC/Thrift fixture repos.

Verified by mutation, not assertion: removing the skipped-rendering branch turns
`names a stage it was told to skip as skipped` red and leaves the other 22
green. A control case pins the opposite direction -- the same group without the
flag still reports the stage as zero -- so `skipped` cannot be printed
unconditionally and pass.

U3 and U4 land together: the test has no value without the renderer, so one
commit keeps a revert clean. Both depend on U2, which introduced the field they
read.

tsc clean; 1066/1066 across the group unit and CLI integration suites.

* fix(group): make every description of --exact-only match what it does (U5)

Two descriptions this branch wrote or touched still misstated behavior.

The MCP `exactOnly` description carries "Manifest links still apply." The CLI
help and both locale strings, rewritten in the same commit, omit it -- so the
surface most operators read understated what still runs. Manifest cross-links
are computed before the gate and are genuinely unaffected by the flag, so the
caveat is the accurate half and the CLI now says it too.

The `group_sync` tool description opened with "extract HTTP contracts". That
clause was carried forward byte-identical while only the trailing cross-linking
half was rewritten, and it is wrong: the detect config has six non-HTTP
extraction toggles, and this branch's own new test fixture is Thrift.

`help-i18n.ts` is deliberately untouched. It maps an option to its translation
key and that key already exists; only the commander string and the two locale
values carry text, so a text-only change does not reach it.

The tool-description edit sits ahead of the registryOutcome paragraph, leaving
the relative order of the 'preserved' / 'superseded' / 'no-prior-registry'
literals intact -- `tools.test.ts` slices that description by their positions.

tsc clean; 64/64 across the locale-parity, help-registration, tool-schema and
group-tool suites.

* fix(group)!: remove max_candidates_per_step and shared_libs (U6)

Both keys were declared, defaulted, written into every generated group.yaml,
and read by nothing -- the same three-station dead surface this PR removed for
bm25_threshold, embedding_threshold and detect.embedding_fallback. Every other
DetectConfig field gates a real extractor in sync.ts; shared_libs gates nothing,
because 'lib' contracts come only from the operator-declared manifest extractor.
MatchingConfig reaches matching.ts solely through buildNoisyContractFilter,
which reads exclude_links_paths and exclude_links_param_only_paths and nothing
else.

Existing group.yaml files keep loading and keep their keys. parseGroupConfig
spreads the raw block over its defaults, so a key the schema no longer knows
about survives into the returned config -- which matters because `group add` and
`group remove` round-trip the operator's file through loadGroupConfig ->
yaml.dump -> write, so anything the parser dropped would be deleted from their
checked-in file. The legacy-config test now pins both keys in the same cast form
as its three siblings, and the fixture carries shared_libs so that assertion is
not vacuous.

Two stations that are easy to miss and are swept here:

- gitnexus/bench/cross-repo-trace/verify.mjs GENERATES a fresh group.yaml. It is
  not a preserve-path fixture, so "leave YAML fixtures alone" does not cover it;
  the repo has two generators and both are updated. It is a .mjs file outside
  tsconfig's include, so no type gate would have caught it.
- config-parser.test.ts asserted the removed default at runtime, which vitest
  DOES run. That assertion is gone from the defaults case (the key no longer has
  a default) and re-formed as a preserve assertion in the legacy case.

Verification gate, corrected: "zero net new errors against origin/main" would
have measured the whole branch delta and been red through no fault of this
commit. Measured instead against the branch tip immediately before it --
tsc -p tsconfig.test.json --noEmit reports 989 before and 989 after. Twenty-four
typed-literal sites across ten test files, none of them CI-gated, plus the two
runtime sites above which are.

Note the deliberate side effect: removing a key from the defaults also stops the
group add round-trip from re-adding it to a file that never carried it. Nothing
in src reads either key, so no behavior changes.

BREAKING CHANGE: `matching.max_candidates_per_step` and `detect.shared_libs` are
no longer part of the group.yaml schema and are no longer written into generated
templates. Existing files carrying them continue to parse and retain them.

src tsc clean; 1189/1189 across the group unit, group integration, locale-parity,
help-registration and tool-schema suites.

* docs(group): map PR #3020 review findings to the commits that close them

Retitles the ledger to hold one section per reviewed PR and adds #3020's ten
findings. Two things are stated rather than claimed away: `abda0d041` closes
three findings because they are one code block plus the test that pins it, and
the suppressed-stage marker is a coupled set because the renderer consumes the
field the earlier commit introduces.

Also records what is NOT closed here -- the PR description's false claim about
`max_candidates_per_step` lives outside this branch.

* refactor(group): apply simplify-pass findings

Four cleanup agents (reuse, simplification, efficiency, altitude) over this
run's diff. Efficiency was clean. The rest found five things worth fixing, two
of which were real gaps rather than style.

`recordedMatchStages` filtered unknown values instead of rejecting the list.
That inverted the tri-state on the one field built to prevent exactly this
conflation: a stale `['bm25']` -- the scenario its own comment cites as the
motivation -- survived as `[]`, which on this field MEANS "measured, nothing was
suppressed". A confident clean answer manufactured from a value we could not
read. Now all-or-nothing, matching `recordedRepoList`.

`gitnexus group contracts` showed nothing after an exact-only sync. The human
renderer destructures a fixed field list and gates its incompleteness warning on
`truncated`, so the marker reached the MCP payload and the JSON output but not
the listing an operator actually reads. It now warns, separately from the
`truncated` warning, because the remedies differ: one says fix the repo, this
one says re-run without the flag.

`verbose` was still coerced with `Boolean()` in the same call whose tool
description this branch changed to promise "PARAMETERS ARE VALIDATED". Validated
now, and added to the tool schema -- it was read by the backend and advertised
nowhere.

Reuse: the thrift wildcard-matchable pair existed twice, near-verbatim, in
`sync-exact-only` and `registry-suppressed-stages`. Both now call a shared
`makeWildcardPair` fixture, so the shape `runWildcardMatch` fires on is defined
once.

Simplification: dropped a `Set` built per sync over a list that only ever holds
zero or one entries; iterating `Object.keys(STAGE_COUNTS) as MatchType[]` also
keeps the exhaustiveness the `Record` was built for, which `Object.entries` had
discarded.

Deliberately not done, with reasons: a schema-driven unknown-parameter layer at
the MCP chokepoint (five parameters are read by backends and declared in no
schema, so a strict layer rejects working calls today, and it cannot produce the
"was removed" message finding 3 is about); folding the marker into
`GROUP_IMPACT_TRUNCATION_REASONS` (reverses a recorded plan decision and the
bridge scope is an open question for the maintainer); a per-stage suppression
cause `Record` (no second suppressor exists -- speculative); collapsing the six
`detect` extractor branches into a table (a real generalization, but a refactor
outside this diff); and converging an untouched pre-existing CLI test onto the
new manifest helper (it captures a value the helper does not return, so the
change risks more than the duplication costs).

tsc clean; eslint 0 errors (1 pre-existing warning); 1085/1085.

* docs(group): remove REVIEW-FINDINGS-MAP.md

Removes the findings-to-commits ledger from the source tree.

Note for anyone reading this in history: the file was introduced on main by
#3012 and carried that PR's findings map; this branch had appended a #3020
section. Deleting it drops both. #3012's content is recoverable with
`git show 2c0fb7753:gitnexus/src/core/group/REVIEW-FINDINGS-MAP.md`.

* fix(group): stop cross-repo impact and trace claiming a narrowed graph is complete

Closes the half of the suppressed-stage finding that was deferred. The reviewers
were right that deferring it was the weak point: the motivating harm was named
as `group_impact` and cross-repo `trace` traversing a graph missing real edges,
and those were exactly the surfaces left uncovered.

The deferral rested on an assumption that does not hold. "It is already blind to
this, so we do not make it worse" is false: `--exact-only` was inert before this
PR, so the number of narrowed registries in the world goes from zero to nonzero
exactly when this lands. The blindness was harmless only while narrowing was
impossible. And silence there is not neutral -- `cross-impact.ts` documents
`truncated: false` as an affirmative completeness claim, so those tools were
about to start asserting a complete answer over a knowingly short graph.

`suppressedMatchStages` now rides the bridge the same way `unreadableRepos`
does: persisted in meta.json (no BRIDGE_SCHEMA_VERSION bump -- meta fields have
this precedent), read back all-or-nothing, and carried across the preserve path
through `refreshPreservedBridgeMeta`'s diagnostics so a preserved bridge keeps
the marker of the sync that actually built it.

`crossRepoCompleteness` folds it in, which is what makes this one change reach
all three surfaces -- that function is by design the ONE computation behind the
truncation triple. Precedence is explicit: an unreadable or unaccounted repo
outranks a suppressed stage, because it is the more serious structural gap and
its remedy has to be the one reported.

`'suppressed-stage'` is a new member of the truncation-reason union rather than
a reuse of `'incomplete-sync'`. The earlier decision not to touch that union was
about not conflating remedies -- telling an agent to repair a repo that read
fine, for a narrowing it requested. A distinct member preserves that reasoning
while letting the answer stop claiming completeness, which is what reusing the
existing member would have destroyed.

The union's guard test did its job: adding a member failed the check that every
reason is explained on the agent-facing surface, so the impact tool description
now names this one and its distinct remedy (re-run WITHOUT the flag; nothing
failed to read).

`group status` and its CLI renderer surface it too, on the populated case only
-- absent is a registry predating the field and empty is the ordinary clean
sync; neither earns a line.

Deliberately still not done, and why: a repo-wide unknown-parameter layer for
every MCP tool. Five parameters are read by backends and declared in no schema
(`subgroupExact`, `unmatchedOnly`, `showClusters`, `showProcesses`, and
`verbose` until this branch declared it), and three tools dispatch with no
schema entry at all, so a strict layer rejects working calls until each is
reconciled. That reconciliation is the work; the layer is the cheap part. It
also cannot produce the "was removed and is no longer accepted" message the
retired-parameter guard exists to give.

tsc clean; eslint 0 errors (2 pre-existing warnings); 1159/1159 across the group
unit, group integration and tool-schema suites.

* fix(group): make the suppressed-stage signal actually reach its readers

Applies the mechanical findings from the code review of the previous commit.
That commit claimed cross-repo impact and trace stop reporting a narrowed graph
as complete. Trace did; impact did not, and two operator-facing messages said
something false. Four reviewers plus the cross-model pass converged on the same
two defects, and the untested seams were exactly where they were.

`runGroupImpact` recomputed the truncation reason and hardcoded its fallback, so
it could never emit 'suppressed-stage' -- the value the previous commit added to
the union and documented in the tool description. Every narrowed-but-readable
bridge was reported as 'incomplete-sync', telling the caller to repair a repo
that read fine. It now propagates the bridge's own reason, as cross-trace.ts
already did.

The preserve path stamped this run's request onto an older bridge. When no repo
can be read the database and registry are kept from an earlier sync, so
meta.json has to keep describing that sync; instead `{ ...existing,
...diagnostics }` overwrote its marker, leaving contracts.json, meta.json and
bridge.lbug describing three different runs. Currently masked by unreadable-repo
precedence, one loosened condition from a live wrong verdict.

`group contracts` printed "the last sync did not record which repos it could
read" after any exact-only sync: truncated was set with both repo lists empty,
so the message fell through to the wrong branch. It is now gated on the reason,
not the flag. `group impact` likewise blamed the local walk for a floor the flag
caused.

The tri-state reader is now defined once, in the leaf module whose own comment
says it exists so this exact duplication cannot recur -- it had been copied into
bridge-db.ts within one commit of that comment being true.

Both agent-facing descriptions now name the field. The previous commit added it
to three payloads and documented it on none.

Tests cover what shipped green: the preserve path for both artifacts (verified
by mutation -- reintroducing the stamp turns exactly one test red), and the
reason's REACHABILITY. The existing guard only asserted each reason is
described, which is why a documented-but-unemittable value passed it.

Also corrects a comment that said the marker is deliberately not folded into the
truncation triple. True when written; false one commit later.

tsc clean; eslint 0 errors; 1163/1163 across the group unit, group integration
and tool-schema suites.

* fix(group): drop verbose from the MCP surface, fail a superseded bridge closed

Two maintainer-directed findings from the review.

verbose is removed from the group_sync MCP schema and from GroupService, and
kept on the CLI. The parameter never did what either description claimed: the
gates emit workspace-dependency discovery stats and one aggregate manifest line,
not "each cross-link". Worse, they emit them through the server's logger, which
an MCP caller cannot read at all -- so advertising it introduced precisely the
kind of knob this PR exists to delete, in the PR that deletes them. SyncOptions
keeps the field and the CLI keeps --verbose, because a CLI user really can see
that output; its help now says "Show additional sync diagnostics", which is
what it shows. It was added to the MCP schema earlier in this same PR, so there
is no published compatibility burden in taking it back out. A caller that still
sends it is ignored rather than refused: it was never a documented parameter,
and the retired-name guard is reserved for ones this tool actually withdrew.

The second fixes a split-brain the completeness work made materially worse. When
contracts.json commits and the bridge write then fails, the previous database
stays in place describing an EARLIER sync. Until now it kept vouching for
itself, so group_impact could traverse the superseded graph and call its answer
complete while group_contracts reported the advanced registry -- two public
surfaces making contradictory epistemic claims out of one sync. That was
tolerable when the disagreement was about counts. It is not, now that
suppressed-stage makes completeness a correctness property.

markBridgeProvenanceUnknown withdraws the claim without touching the database:
bridgeMetaMatchesFile already gives provenanceUnknown highest precedence and
refuses to vouch for the pair, so cross-repo answers downgrade to a floor until
a sync succeeds. Deliberately not a re-stamp -- the metadata still describes the
database it was written for, and saying otherwise recreates the mis-pairing the
preserve path avoids. Deliberately not a delete -- the old graph is still worth
having as a floor, it just stops being called complete. Best-effort, because it
runs inside a failure handler and must not replace a reported bridge failure
with an unrelated one; the warning now states which of the two happened.

Shared registry+bridge generation identity is the architectural fix and is
deliberately NOT attempted here. This is the PR-sized containment.

Verified by mutation, both directions: neutering the withdrawal turns the new
test red, and a control pins that a healthy sync does not withdraw provenance --
otherwise every successful run would report its own answers as a floor.

tsc clean; eslint 0 errors; 1185/1185.

* refactor(group): apply simplify-pass findings

Four cleanup agents over the last five commits. Efficiency was clean and traced
why: the containment helper is failure-path only, the reason ternary sits after
the fan-out loop, and the tri-state readers run once per artifact read.

The strongest finding was one the diff itself proved. `refreshPreservedBridgeMeta`
enforced the never-persisted rule for `repoListsUnreadable` and
`pairedWithDatabase` with two deletes in its own body, under a comment noting it
was the only code that read metadata and wrote it back. That held exactly as
long as there was one such caller. `markBridgeProvenanceUnknown` made it two,
and inherited nothing. The strip now lives in `writeBridgeMeta`, so every writer
gets it and no future one can forget; `pairedWithDatabase` is the dangerous one,
because persisted it tells every later reader the pair was verified when nothing
verified it.

`group impact` still printed "fan-out stopped early" whenever `truncatedRepos`
was non-empty — but the bridge's incomplete repos are unioned into that list
even when zero crossings were attempted, so a structural gap was reported as a
runtime one, with the only working remedy omitted. That is the same false-cause
shape the contract listing was re-gated for one commit ago, left live one
command over because the new reason was bolted in front of the old branch rather
than replacing the thing it branched on. Now keyed on the reason.

The `?? 'incomplete-sync'` arm in cross-impact was unreachable: reaching it
needs `truncated` true with all three of its inputs false, which
`truncated = runtimeTruncated || bridge.truncated` forbids. Flattened.

Also: a `recordedMatchStages` insert had split `crossRepoCompleteness` from its
own JSDoc; one new test was a strict subset of another; and the bridge-failure
warning interleaved concatenation with a mid-chain ternary.

The new invariant assertion was caught being VACUOUS by mutation before it
shipped — seeded with a valid repo list, `readBridgeMeta` never sets the
reader-only field, so it passed with or without the strip. The fixture now seeds
an unreadable list, and both it and the pre-existing assertion go red when the
strip is removed.

Deliberately skipped, with reasons: a shared `firstTruncated` fold over
`TruncationFields` (the right altitude, but it changes cross-trace's return
assembly and that surface separately documents a 'timeout' rung it cannot emit —
a behavior change, not a cleanup); a reason-keyed `explainFloor` helper across
all four CLI renderers (real, but a four-site refactor); narrowing the persisted
stage vocabulary to a `SuppressibleStage` alias (would be undone by the very
extension the field was modelled as a list to allow); moving `verbose` to
`logger.debug` and deleting `SyncOptions.verbose` (the maintainer explicitly
directed keeping both); and merging the two tri-state readers behind a predicate
(they are adjacent in one file now, so a tightening applies to both by
inspection — the duplication the comment warned about was cross-FILE).

tsc clean; eslint 0 errors; 1164/1164.

* fix(group): address gitnexus-check findings

Seven bot comments across two review rounds; five distinct after dedup. Four
were valid and are fixed, two were already resolved by later commits the bot
had not seen.

The validator could throw from its own error path. `JSON.stringify` is the right
renderer there — it is what distinguishes the string "false" from the boolean,
which is the entire point of the message — but it throws on a BigInt and on a
cyclic object. So a validator promising a structured `{ error }` instead
rejected, and `callTool` is reachable directly, so neither input is
hypothetical. Guarded, keeping the distinction and falling back for the shapes
that cannot serialize.

An unreadable suppression record read as "nothing was suppressed".
`recordedMatchStages` is all-or-nothing by design, so garbage collapses to
`undefined` — and the consumer treated `undefined` as an empty measurement,
throwing that safety away and reporting a registry it could not parse as
complete. Present-but-unreadable now forces the floor, while absent stays
legitimate: a registry written before the field existed has no opinion and
should not be dragged to a floor for it.

Two test-side findings, both real and both invisible to CI because
`tsconfig.json` is src-only. Three `mock.calls[0][1]` accesses did not
type-check against a zero-arg mock, and four assertions read `truncationReason`
/ `riskEpistemic` straight off `CrossRepoCompleteness`, which is a discriminated
union carrying them on one arm. Also removed a `StoredContract` import that went
dead when those fixtures moved to `makeWildcardPair`.

Worth recording: U6 set a test-config gate at 989 errors and later commits
walked it to 994 without anyone re-measuring — the bot caught three of the five.
Now 987, below the original baseline.

Already fixed, not by this commit: the preserve-path stamp the bot flagged
against 6ceac8b1f (fixed in 1fbe0dc6b) and the displaced completeness JSDoc
(fixed in 2d2ef8c47).

Both behavior fixes are mutation-verified: restoring the unguarded stringify
turns the new unserializable-value test red, and a control pins that an absent
record still reads as complete so the fails-closed change cannot pass by forcing
every registry to a floor.

src tsc clean; eslint clean; 1167/1167.

* chore(autofix): apply prettier + eslint fixes via /autofix command

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
2026-08-27 18:27:32 +01:00
Dinh Huy
dbd4e1c9fb
feat(group): Support Django route extraction for multi-repo (#1836)
* [+] Add django route discovery to create cross-link for multi-repo

* [+] Update ingestion

* [~] Fix bugs and abstraction violation

* feat(python-http): add keyword url= and variable propagation for consumer detection

- Add REQUESTS_KEYWORD_URL_PATTERNS for requests.get(url='...') keyword args
- Add WRAPPER_URI_PATTERNS for generic wrapper.fetch(uri='...') calls
- Add WRAPPER_URI_VAR_PATTERNS + buildLocalStringMap for uri=variable propagation
- Add LOCAL_STRING_ASSIGNMENTS to track uri='...' assignments
- Wire both direct-string and variable-propagation loops in scan()
- Add normalizeConsumerPath() helper

Note: Automatic cross-link detection remains limited for runtime-computed URLs
(URLs built via .format(), string concat, or module constants). Manual
manifest links needed for known cross-repo contracts.

* [+] add extract uri and url keywork pattern for request http

* feat(python-http): add variable propagation for uri=/url= consumer patterns

Re-add LOCAL_STRING_ASSIGNMENTS, WRAPPER_URI_VAR_PATTERNS,
buildLocalStringMap(), and normalizeConsumerPath() lost during
cherry-pick merge of upstream keyword-URL commit.

Together with the upstream WRAPPER_URI_PATTERNS and
REQUESTS_KEYWORD_URL_PATTERNS, we now detect:
- requests.get(url='literal') keyword args
- wrapper.fetch(uri='literal') keyword args
- wrapper.fetch(uri=variable) where variable was assigned a string literal

* fix(group): discover Django roots relative to manage.py dir + multi-project (#1836 R1)

A Django project not at the repo root (e.g. backend/manage.py) discovered
zero routes: the settings module path was resolved repo-root-relative only,
so backend/myproj/settings.py was never found and discovery returned null.

Resolve settings, star-imported base settings, ROOT_URLCONF, and the root
urls.py against the manage.py's own directory first, then the repo root
(resolvedSettingsPath is now project-dir-aware so relative imports anchor
correctly). Iterate every manage.py so a monorepo with several Django
projects yields each project's root — the provider hook becomes plural
(discoverRootRouteFiles → string[]) and the main-thread pass loops over all
roots (inner-scoped continues, parser hoisted once per language).

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(group): remove dead code in Django root discovery (#1836 R9)

- Collapse the identical if/else in extractStarImports to one push.
- Drop the unreachable baseModule.startsWith('.') branch (baseModule is
  always a resolved slash-path or a bare absolute module — never dot-prefixed).
- Import DjangoFileReader from django.ts instead of re-declaring the type.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): walk Django includes once per prefix, not per file (#1836 R2)

The include() recursion guard was keyed on file path alone and shared across
the whole walk, so a urlconf included under two prefixes (a "diamond" — the
same app mounted at /v1/ and /v2/) emitted routes for only the first mount.

Key the guard on (resolvedFilePath, accumulatedPrefix) at all three sites
(function entry, path()-wrapped include, bare include) so a file reached
under two distinct prefixes is walked once per prefix while a genuine cycle
(same file + same prefix) still terminates — null/'' prefixes collapse to one
key so a no-prefix re-entry is treated as a cycle. MAX_INCLUDE_DEPTH remains
the backstop. Adds diamond + self-include-cycle tests.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): extract Django routes from non-list urlpatterns (#1836 R3)

findUrlpatternsLists only accepted a list-literal RHS, so common shapes
yielded zero routes: concatenation (urlpatterns = a + b), wrapper calls
(format_suffix_patterns([...]), i18n_patterns, staticfiles_urlpatterns), and
tuples.

Add collectUrlpatternContainers to descend binary_operator operands, known
wrapper-call list arguments, and tuples. Inherently-dynamic forms (DRF
router.urls, comprehensions, bare names) still yield nothing but now emit a
debug log so the silent-zero case is observable rather than mysterious.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* refactor(group): thread Django parser explicitly, drop module singleton (#1836 R4)

extractDjangoRoutes relied on a module-level _djangoParser set via
setDjangoParser before each call — hidden state that would break if a second
language ever used the include re-parse path, and an easy-to-forget contract.

Pass the tree-sitter parser as an explicit parameter of extractDjangoRoutes
(the extractRoutes provider hook already receives it) and delete the global
plus its setter. The Python provider wires it directly; tests pass the parser
in place of the removed setDjangoParser() call.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(ingestion): isolate a throwing extractRoutes in the cross-file route pass (#1836 R5)

The main-thread cross-file route pass called provider.extractRoutes without a
guard, so a throw (e.g. a future grammar edge case in the include() walk) would
propagate out of the parse phase and abort the entire analyze — unlike the
worker, which isolates per-file failures.

Wrap the per-root extractRoutes call in try/catch that logs a warning and
continues to the next root. Export extractCrossFileRoutes and add a unit test
driving a stub provider whose extractRoutes throws, asserting the pass returns
[] and does not propagate.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* perf(ingestion): bucket only route-capable languages in cross-file pass (#1836 R6)

extractCrossFileRoutes runs in the deferred band on every analyze (incl. warm
all-cache-hit runs). It now derives the set of languages whose provider exposes
the cross-file route hooks once, returns early if none do, and buckets only
those languages' paths — so a non-framework repo no longer pays to bucket the
languages it doesn't use here.

Route results are intentionally not persisted across runs, so a Django repo
still re-derives its routes each analyze; documented inline that cross-run
route caching is a deliberate follow-up rather than implemented here.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* style(group): prettier-format http-patterns/python.ts (#1836 R7)

The file was not formatted to the root .prettierrc (the consumer-path
normalizer used single-line try/catch and method chains), so the CI
quality/format check (`prettier --check .`) failed. Reflow only — no logic
change (`git diff -w` confines the change to normalizeConsumerPath's layout).

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): dedup Python URI detections by byte offset, not line arithmetic (#1836 R8)

The wrapper-URI dedup key was lineNum*1000+methodRow, which can collide for
distinct calls in files over 1000 lines (carry into the row term) and can
fail to dedup a genuine duplicate when a node straddles a line boundary.

Key on node byte offsets (`${pathNode.startIndex}:${methodNode.startIndex}`),
matching the sibling seenVarDetections dedup a few lines below.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* test(ingestion): end-to-end Django cross-file route extraction (#1836 R10)

Adds an integration test that runs runPipelineFromRepo against a Django
fixture whose project lives under backend/, asserting the resulting Route
graph nodes (/health, /api/items, /api/items/<int:pk>). This exercises the
previously-untested main-thread orchestration glue (discovery → parse →
extractRoutes → allExtractedRoutes → Route nodes) and, because the project is
in a subdirectory, regresses the subdir-discovery fix (R1) — a repo-root-only
resolver would discover nothing and emit zero Route nodes.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): anchor Django include() resolution at the project root (#1836 review F1)

resolveIncludedFile tried the bare repo-root candidate (app/urls.py) before the
project-relative one, so in a monorepo with both a repo-root app/ and a
backend/ Django project that also has an app/, include('app.urls') from the
backend project resolved to the WRONG service's routes.

Probe up-tree from the root urls.py for the nearest manage.py (the Django
project root / sys.path entry) and try that-anchored candidate first. Absolute
module paths like `app.urls` now resolve to <projectRoot>/app/urls.py
unambiguously. When no manage.py is reachable (e.g. unit tests with a urls-only
reader) the prior strategy order is preserved. Adds a monorepo wrong-app test.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): drop bogus Django provider source-scan, use graph routes (#1836 review F2)

The DJANGO_PATH_PATTERNS / DJANGO_URL_PATTERNS source scan emitted an HTTP
provider contract for every path()/re_path()/url() string literal, without
checking it was inside urlpatterns, without skipping include() mount points,
and without composing the include() prefix across files. For
`path('api/', include('app.urls'))` + child `path('items/', view)` it emitted
providers for `/api` (a mount, not a route) and `/items` (un-prefixed) — which
survived the exact-contract-ID dedup alongside the correct graph route
`/api/items`, polluting cross-repo matching with false providers.

Remove the Django provider patterns and their scan blocks. Django provider
contracts come from the graph Route nodes, which the ingestion route extractor
builds with includes already composed (and now correctly, per the other fixes).
Python HTTP *consumer* patterns (requests/wrapper) are unaffected.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(group): match method-agnostic Django providers to any-method consumers (#1836 review F3)

Django function views are method-agnostic, so extractDjangoRoutes emits
httpMethod '*'. That '*' was dropped by normalizeRouteMethod and then defaulted
to GET by the contract extractor, while the matcher only expanded wildcard
*consumers* — so a `POST /api/items` consumer never matched the Django
provider that was silently narrowed to GET.

- routes.ts: preserve '*' as a method-agnostic marker on the Route node, so the
  contract layer emits a wildcard provider (http::*::path) instead of GET.
- matching.ts: make findMatchingKeys symmetric — a specific-method consumer
  now matches an exact-method provider OR a wildcard (http::*::) provider on the
  same path, mirroring the existing wildcard-consumer expansion.

Co-Authored-By: HuyNguyenDinh <61400397+HuyNguyenDinh@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Dinh Huy <huynd86@fpt.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-06-21 20:11:30 +01:00
1PLee
05ca80ea30
feat(ingestion): add thrift contracts impl (#1234) 2026-05-06 09:19:10 +01:00
Ivan Uzun
46586a8319
fix(group): add configurable cross-link path exclusions to reduce false positives (#1093)
* fix(group): add configurable cross-link path exclusions to reduce false positives

Add matching.exclude_links_paths and matching.exclude_links_param_only_paths
to group.yaml config. These filter out noisy HTTP contracts (health checks,
param-only catch-all routes) from cross-link matching while preserving them
in the contract registry for documentation purposes.

Defaults are empty/false for backward compatibility — no behavior change
unless the operator explicitly configures exclusions.

* fix(group): address review findings — filter unmatched, normalize trailing slash, add tests

- Excluded contracts no longer inflate SyncResult.unmatched (isNoisy guard)
- pathPart in buildNoisyContractFilter strips trailing slashes before comparison
- 8 new unit tests for buildNoisyContractFilter covering all code paths
- Config-parser test asserts defaults for new matching fields

* fix(group): normalize configured exclusion paths and add root-path test

- Strip trailing slashes from configured exclude_links_paths at Set-build
  time so root path '/' (which normalizes to '') matches correctly
- Add test: exclude_links_paths: ['/'] suppresses http::GET::/ contracts
- Add new matching fields as commented examples in fixture group.yaml (DoD §2.4)

* docs(group): document exclude_links_paths and exclude_links_param_only_paths config fields

Add JSDoc to MatchingConfig interface, update the microservices guide
YAML example and field notes, and scaffold the new fields (commented out)
in the group create template.
2026-04-28 08:22:14 +01:00
ivkond
1ff324ca16
feat(group): bridge.lbug storage + contract matching expansion (1/4 of #606 split) (#795)
* feat(group): bridge.lbug storage + contract matching expansion

Part 1 of 4 in the split of #606 (ticket: #791, closes #790 with a
revised plan per @magyargergo's request).

## What changed

Adds the LadybugDB-backed bridge storage infrastructure and extends
the contract matching algorithm with wildcard support. All changes are
additive: storage.ts, sync.ts, service.ts, cli/group.ts, mcp/tools.ts
are left on their upstream main versions and will migrate to the new
bridge in follow-up PRs (#792, #793, #794).

### Files

**New (844 LOC prod):**
- `gitnexus/src/core/group/bridge-db.ts` — atomic write-to-temp with
  `retryRename` for Windows EBUSY/EPERM, per-item write tolerance via
  `WriteBridgeReport`, `findContractNode` with three-tier symbol
  lookup (uid → filePath+name → filePath)
- `gitnexus/src/core/group/bridge-schema.ts` — schema DDL
- `gitnexus/src/core/group/normalization.ts` — contract ID
  canonicalization + `dedupeContracts` / `dedupeCrossLinks` helpers
  used by both matching and bridge write

**Modified (+137 LOC prod):**
- `gitnexus/src/core/group/matching.ts` — adds `runWildcardMatch` for
  `grpc::Service/*` wildcard consumers, `buildProviderIndex` helper,
  and canonical gRPC ID handling in `normalizeContractId`
- `gitnexus/src/core/group/types.ts` — `MatchType` gains `'wildcard'`;
  new `BridgeHandle` and `BridgeMeta` interfaces

**New tests (658 LOC):**
- `gitnexus/test/unit/group/bridge-db.test.ts` — core write/read round
  trip, `WriteBridgeReport` shape, dropped-links counter, retryRename
  behavior on EBUSY/ENOENT/EPERM/EACCES
- `gitnexus/test/unit/group/bridge-db-edge.test.ts` — edge cases
  (malformed meta, missing contract nodes, concurrent access)

**Modified tests (+225 LOC):**
- `gitnexus/test/unit/group/matching.test.ts` — wildcard consumer
  matching, gRPC canonical ID handling, same-service guard

### Self-review fixes folded in

Carried forward from the original #606 self-review:
- `writeBridge` try/finally handle lifecycle + `handleClosed` sentinel
- `openBridgeDbReadOnly` partial-handle cleanup
- `writeBridgeMeta` uses `retryRename` for Windows consistency
- `retryRename` unit tests (was zero coverage)
- Per-item try/catch around every CREATE loop so one malformed contract
  doesn't abort the whole write
- Dropped cross-link counter (`linksDroppedMissingNode`)

### Why now

magyargergo asked for the #606 PR to be split so we can iterate with
confidence (https://github.com/abhigyanpatwari/GitNexus/pull/606#issuecomment-4229612271).
This is the foundational layer — pure infra, no user-facing surface,
no callers of the new APIs in this PR. Later PRs wire it in.

### How to verify

- `cd gitnexus && npx tsc --noEmit`
- `cd gitnexus && npx vitest run test/unit/group/bridge-db.test.ts --pool=forks`
- `cd gitnexus && npx vitest run test/unit/group/bridge-db-edge.test.ts --pool=forks`
- `cd gitnexus && npx vitest run test/unit/group/matching.test.ts --pool=forks`
- Pre-commit hook runs clean

### Risk / rollback

**Low.** All new code sits under `src/core/group/` in new files plus a
minimal `+16/-1` diff to `types.ts` and a `+136/-0` diff to `matching.ts`
(both purely additive). No existing callers reference the new APIs
(bridge-db, openBridgeOrFallback, runWildcardMatch) — the PRs that wire
them in come later in the split chain. Rollback = `git revert` of the
merge commit; no state introduced, no schema migration triggered.

### Scope discipline (per GUARDRAILS.md)

- Only the 8 files listed above are touched; no drive-by refactors
- No CI/release/security config changes
- No secrets, tokens, or machine-specific paths
- Content is lifted from the #606 branch which already passed CI 11/11
  green on `d15b8cb` (before the split)

### Dependencies

- **Base:** `main` (no dependencies on other split PRs)
- **Blocks:** extractor expansion (#792), sync pipeline (#793),
  cross-impact feature (#794)
- **Related ticket:** #791

Co-authored-by: Claude <noreply@anthropic.com>

* fix(group): address @claude review on #795

Addresses the findings from the automated review on PR #795
(https://github.com/abhigyanpatwari/GitNexus/pull/795#issuecomment-4229770000
— posted by @magyargergo / claude-code Action run).

### Medium severity (reviewer flagged as blockers)

- **bridge-db.ts `openBridgeDbReadOnly` bak recovery** — the `.bak`
  recovery path used bare `fsp.rename(bakPath, dbPath)`, which is
  exactly the scenario most likely to hit Windows EBUSY/EPERM (an
  interrupted writer still holding the handle for a few ms). Switched
  to `retryRename` for consistency with the rest of the file's
  Windows-safe rename path.
- **bridge-db.ts `ensureBridgeSchema` error detection** — the inline
  `msg.includes('already exists')` substring match has been lifted
  into a named constant `LBUG_ALREADY_EXISTS_MSG` with a comment
  documenting the coupling to LadybugDB's error message wording and
  why we can't use `IF NOT EXISTS` (LadybugDB DDL doesn't support it)
  or typed errors (LadybugDB's JS driver doesn't expose error codes).
  Also tightened the `catch (err: any)` to `catch (err: unknown)`.
- **bridge-db.ts `findContractNode` — extracted out of writeBridge**
  — the 35-line async closure living inside `writeBridge` has been
  lifted to three module-level functions: `createContractLookupIndex`,
  `indexContract`, and `findContractNode`. `findContractNode` is now
  a pure synchronous function taking a prebuilt index instead of
  doing its own DB queries. The `writeBridge` cross-link loop is now
  ~25 lines instead of ~100.
- **bridge-db.ts `findContractNode` — N+1 query elimination** — the
  old inner-closure version issued up to 6 DB round-trips per
  cross-link (2 endpoints × up to 3 tiers of fallback queries). For a
  group with 1000 cross-links, that's up to 6000 DB queries just to
  resolve endpoints. The new version consults an in-memory
  `ContractLookupIndex` built incrementally as contracts are inserted
  (`indexContract` called AFTER each successful insert so failed
  inserts don't poison the index). Cross-link resolution is now
  O(1) per link instead of O(3) DB queries per link, with zero DB
  round-trips during the cross-link loop.

### Minor severity

- **bridge-db.ts `queryBridge` empty-array guard** — if LadybugDB
  ever returns an empty `QueryResult[]` at the top level (shouldn't
  happen with single-statement calls, but driver contract isn't
  explicit), the old code would call `.getAll()` on `undefined` and
  crash with a confusing stack. Added an `unwrapQueryResult` helper
  that throws an explicit `'empty QueryResult array'` error instead,
  making a potential driver regression visible immediately.
- **normalization.ts `contractRichness` weights** — added a
  block-level comment documenting the weight ordering (+3 for
  symbolUid, +2 for each symbol-identifying field, +1 for service
  tag or non-manifest origin) and explicitly noting that the
  absolute numbers don't matter, only the relative ordering. Matches
  the "comment for contributors" suggestion in the review.
- **bridge-schema.ts `BRIDGE_SCHEMA_VERSION` migration comment** —
  added a 4-point contract explaining what bumping the constant
  means ("discard and re-sync" strategy for V1, no in-place
  migration yet, new migration logic should live in a separate
  `bridge-migrations.ts` module when it becomes necessary).
- **test/unit/group/fixtures.ts** — extracted the `makeContract`
  helper previously copy-pasted between `bridge-db.test.ts` and
  `bridge-db-edge.test.ts` into a shared fixtures module. Both test
  files now import from `./fixtures.js`. Kept the scope minimal:
  fixtures is NOT a general-purpose factory module, just the shared
  baseline contract builder.

### New tests

Added 9 pure-function unit tests for the now-extracted
`findContractNode` in `bridge-db.test.ts`:
  - returns null on empty index
  - tier 1 (symbolUid) match, including repo-scope and role-scope
    isolation
  - tier 2 (filePath + symbolName) fallback when symbolUid is empty
    or mismatches
  - tier 3 (filePath only) when exactly one contract lives in the
    file, and refusal when multiple do
  - priority ordering when multiple tiers could resolve

These are fully isolated — no DB, no temp directories, no native
LadybugDB binding — so they run in &lt;10ms total and are
immediately trustworthy as a regression safety net.

### Deliberately deferred (reviewer marked as "fine for now")

- `BridgeHandle._db` / `._conn` typing to `unknown` with casts in
  `bridge-db.ts` — reviewer's note: "The typing is fine for now."
- Batch inserts via `UNWIND` — needs LadybugDB support confirmation,
  tracked as a follow-up; the per-item pattern remains.
- `queryBridge` prepared-statement lifecycle — the current pattern
  (prepare → execute → GC) relies on LadybugDB's internals, worth
  verifying against their docs in a separate audit.

### Scope discipline (per `GUARDRAILS.md`)

- Only files touched by this PR (`bridge-db.ts`, `bridge-schema.ts`,
  `normalization.ts`, both bridge test files, new `fixtures.ts`) —
  no drive-by refactors
- No CI/release/security config changes
- No secrets

### Test + typecheck status

- `npx tsc --noEmit` clean
- `bridge-db.test.ts`: added 9 `findContractNode` tests, all pass in
  isolation. The full-file run still hits the pre-existing native
  LadybugDB cleanup segfault that flakes the reported count — same
  as every prior commit on this branch, not a regression.
- `bridge-db-edge.test.ts`: 4/4 pass
- `matching.test.ts`: 28/28 pass
- `types.test.ts`: 5/5 pass
- `retryRename` tests (4/4) and `findContractNode` tests (9/9)
  verified in isolation via `-t` filter

Co-authored-by: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
2026-04-11 19:46:12 +01:00
ivkond
52277247fe feat(group): add group infrastructure and contract matching
Core foundation for repository group analysis:
- Type system: ContractType, ExtractedContract, StoredContract, CrossLink
  with optional `service` field for intra-repo matching
- Config parser for group.yaml (repos, detection flags, matching thresholds)
- Contract registry storage with atomic writes
- Exact matching engine with per-type normalization (HTTP, gRPC, topic)
  and intra-repo support (different services within same repo can match)
- Extract LadybugDB pool-adapter from MCP backend for reuse by sync pipeline
- Git staleness checker for group status reporting

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2026-04-02 00:39:43 +03:00