From aa8c567126b7657f3f06751ce21a6e1913f5a4b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Sun, 21 Jun 2026 15:25:56 +0100 Subject: [PATCH 1/2] fix(lbug): stop --pdg analyze double-free (skip LadybugDB close-destructor crash) + harden connection serialization (#2264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lbug): serialize singleton connection to stop --pdg analyze double-free LadybugDB is single-writer and its Connection is NOT safe for concurrent query execution. The WAL-checkpoint driver (5s setInterval) issued `conn.query('CHECKPOINT')` on the same module-singleton `conn` the analyze pipeline used for COPY. With --pdg the extra BasicBlock / REACHING_DEF / CDG / POST_DOMINATE / TAINTED / CALL_SUMMARY / TAINT_PATH table COPYs outlast the 5s tick, so a checkpoint executed concurrently with an in-flight COPY on one connection -> two libuv workers mutate shared native state -> heap corruption ("double free or corruption (out)" / SIGABRT, detected at the final "Saving metadata..." free). Fix: add conn-lock.ts (`withConnLock`, a promise-chain mutex) and run every singleton-`conn` helper's full query + result-drain inside it: queryAndDrain (when targetConn === conn), executePrepared, executeWithReusedStatement, flushWAL, tryFlushWAL, getLbugStats, deleteAllInterprocTaintPaths, deleteAllCallSummaries. Add an `if (inflight) return` reentrancy guard to the driver tick so overdue ticks don't stack checkpoints. streamQuery is intentionally NOT wrapped (read path, re-entrant per-row callback). Reproduced the crash with concurrent queries on one raw Connection (serial = stable); verified the fix drives the same overlap through the locked adapter without crashing. Tests: conn-lock serialization (no overlap / FIFO / throw-releases) and driver reentrancy guard. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): lock deleteAllCommunitiesAndProcesses against the WAL driver (#2264) The count + DETACH DELETE ran raw conn.query on the singleton connection during incremental --pdg writeback while the WAL-checkpoint driver was live — the same concurrent CHECKPOINT-vs-write double-free this branch fixes elsewhere. Wrap the body in withConnLock, mirroring the already-wrapped deleteAllInterprocTaintPaths. Adds test/integration/lbug-conn-serialization.test.ts (call-through withConnLock spy) asserting the helper now acquires the lock, wired into the lbug-db vitest project (and excluded from the default project so it doesn't run twice). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): lock queryImporters against the WAL driver (#2264) queryImporters issued a raw conn.query on the singleton connection inside the importer-BFS loop of incremental --pdg writeback, while the WAL-checkpoint driver could fire a concurrent CHECKPOINT — the same double-free class. Wrap the read (query + getAll + drain) in withConnLock. Extends lbug-conn-serialization.test.ts with a routing assertion. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): lock deleteNodesForFile count query on the singleton path (#2264) The per-table count read used a raw targetConn.query while the sibling DETACH DELETE already routed through the locked queryAndDrain — an asymmetry that left the count racing the WAL-checkpoint driver during incremental --pdg writeback. Gate the count through withConnLock when targetConn === conn (the singleton), matching queryAndDrain; per-query/temp connections stay lock-free. Test asserts the count loop takes the lock once per filePath-bearing node table. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): drain DELETE results in the deleteAll* helpers (#2264) deleteAllInterprocTaintPaths, deleteAllCallSummaries, and deleteAllCommunitiesAndProcesses awaited conn.query(...DELETE...) but dropped the returned QueryResult (only the count result was closed), leaking a native result handle and violating the helpers' own "query + drain inside the lock" contract. Close each delete result via closeQueryResults, matching the count handling. Adds a seeded drain test (closeQueryResults fires for the DELETE result). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): make the conn-lock non-reentrancy invariant enforced, not just documented (#2264) A future withConnLock-wrapped helper calling another wrapped helper would await its own holder's tail and hang silently. Add an AsyncLocalStorage-based re-entry guard: withConnLock throws a clear error when invoked from within a holding fn's async context. A boolean flag can't do this — a legitimately-queued top-level caller also runs while the lock is held; only AsyncLocalStorage distinguishes a true nested call from normal contention. Tests: re-entry throws (not deadlocks); sequential and concurrent top-level calls do NOT false-fire; the lock releases after a re-entry throw. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(lbug): rename __resetConnLockForTests to _resetConnLockForTests (#2264) Match the repo's single-underscore test-seam convention (_initLockPathForTest). Pure rename of the @internal export and its sole importer. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(lbug): fix stale CHECKPOINT guard regex after the c.query refactor (#2264) lbug-checkpoint.test.ts asserted exactly two CHECKPOINT sites by grepping the literal `conn.query('CHECKPOINT')`. The connection-serialization refactor changed flushWAL/tryFlushWAL to capture `const c = conn` and call `c.query('CHECKPOINT')` inside withConnLock, so the literal grep found 0 and the test failed (expected 2). Make the regex receiver-agnostic (`.query('CHECKPOINT')`) — preserves the guard's intent (exactly two authorized CHECKPOINT sites; a third is a regression) while tolerating the captured-receiver form. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): skip native close on CLI exit to dodge LadybugDB destructor double-free (#2264) THE actual fix for the `analyze --pdg` crash. gdb shows the abort is a double-free inside LadybugDB's own destructor during conn.close(): "double free or corruption (out)" -> abort lbug::main::ClientContext::~ClientContext() lbug::main::Connection::~Connection() NodeConnection::Close(...) <- conn.close() from safeClose It reproduces with the WAL driver OFF and with serial load, so it is NOT the checkpoint/COPY concurrency the rest of this branch serialized — it's a native LadybugDB engine bug (@ladybugdb/core 0.17.1, latest stable) triggered by the larger --pdg write set, firing during teardown AFTER a fully-written, checkpointed index. Fix: closeLbug({ skipNativeClose }) CHECKPOINTs for durability (flushWAL) then skips conn.close()/db.close(), leaving the handles referenced so no GC finalizer re-runs the destructor. The CLI analyze command (success, error, and SIGINT paths all process.exit) opts in via skipNativeCloseOnExit; long-lived callers (MCP server, tests) keep the real close. Mirrors the pool adapter's fire-and-forget native close and the ONNX native-cleanup philosophy. Validated end-to-end: `analyze --pdg --force` now exits 0 with a 193,876-node index; re-opening it (no --force) reads clean and reports up-to-date, proving the CHECKPOINT-only persistence is durable without db.close(). Workaround for an upstream LadybugDB bug (ClientContext destructor double-free). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): keep conn.close()/db.close() literals out of the closeLbug comment (#2264 review P1-1) The skipNativeClose comment in closeLbug contained the literal `conn.close()`/ `db.close()`, which the structural guard test (lbug-checkpoint.test.ts:52-53 — "closeLbug must not inline conn.close()/db.close()") greps for and fails on. Reword the comment to describe the native close without the literal tokens; the code already delegates close exclusively to safeClose, so the guard's intent holds. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): real close on the analyze error path to avoid a hang under skipNativeClose (#2264 review P1-2) The CLI error handler soft-returns (process.exitCode = 1) instead of forcing exit, relying on the released native handles to let Node terminate. The earlier commit made runFullAnalysis's error-path closeLbug skip the native close, leaving live LadybugDB handles that keep the event loop alive forever — a post-init analyze failure would hang. Only the SUCCESS path (which guarantees a following process.exit) skips the native close; the error path now always closes for real. A late-error close could still abort in the destructor, but that terminates the process — it does not hang. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): skip native close in the analyze worker to avoid the LadybugDB destructor crash (#2264 review P2-3) The forked server analyze worker runs runFullAnalysis then force-exits (process.exit(0)). With a real native close inside runFullAnalysis, the LadybugDB ClientContext destructor can double-free after --pdg writes and abort the worker BEFORE it sends 'complete', failing the parent's analyze. Pass skipNativeCloseOnExit: true so the worker checkpoints for durability and lets its process.exit reclaim the handles — same about-to-exit contract as the CLI. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(cli): force exit on a soft error-return when LadybugDB handles are open (#2264 review P1) The full-analysis success path skip-closes LadybugDB (handles left open, reclaimed by process.exit). If a post-finalize step (assertAnalysisFinalized) then throws, the outer catch soft-returns (process.exitCode = 1) — and with native handles open the event loop never drains, so the process HANGS instead of exiting 1. Guard once at the analyzeCommand wrapper, after the try/finally: if isLbugReady() (handles still open) the analyze actually ran and we must force the exit. The success path never reaches here (analyzeCommandImpl process.exit(0)s itself); early-validation errors and unit tests that mock runFullAnalysis never open the DB (isLbugReady() false), so the soft return is preserved. Adds analyze-finalize-failure-exits.test.ts (force-exits when handles open; does NOT when they aren't). The analyze-*.test.ts that mock lbug-adapter now also mock isLbugReady (vitest throws on accessing an undefined export of a mocked module). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): skip the native close on the analyze error path too (#2264 review P2) A real conn.close() on the error path after large --pdg writes can itself hit the LadybugDB ClientContext destructor double-free → SIGABRT, degrading an actionable exit-1 error into a raw native abort. Switch the error-path close to skipNativeClose (mirroring the success path). Safe now that the CLI catch force-exits when isLbugReady() (the prior commit): handles left open are reclaimed by that guaranteed process.exit, so the process terminates without the abort and without hanging. flushWAL keeps the partial index durable. Depends on the prior commit (CLI force-exit guard). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): run loadCachedEmbeddings reads under withConnLock (#2264 review P2) loadCachedEmbeddings issued raw conn.query reads on the singleton connection outside withConnLock — safe today only because it runs before the WAL-checkpoint driver starts, an ordering invariant not enforced by code. Wrap the whole read in withConnLock so a future reorder can't race a CHECKPOINT on the connection. Leaf read; no nested wrapped helpers. Adds a routing assertion to lbug-conn-serialization.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(lbug): de-brittle the close/CHECKPOINT structural guard (#2264 review P2) lbug-checkpoint.test.ts grepped the adapter SOURCE (comments included) for conn.close()/db.close()/.query('CHECKPOINT') literals, coupling a passing test to comment wording — a prior commit had to reword a comment just to keep it green. Strip comments from the read source before the structural assertions so they reflect code only; the invariant (exactly two CHECKPOINT sites; close calls only in safeClose) is preserved and no longer breaks on a comment edit. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(lbug): rel COPY uses the captured writeConn, matching node COPY (#2264 review P3) The relationship COPY passed the module-level `conn` to copyCsvWithRetry while the node COPY uses the captured `writeConn`. Use `writeConn` for both — one captured reference for the whole bulk load, removing the latent identity dependency. Same object during analyze (`conn` is only reassigned at open/close under the session lock), so the queryAndDrain `targetConn === conn` lock gate still engages. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): make the analyze worker's IPC send() failure-safe (#2264 review P3) The worker's send() used `process.send?.(msg)` — the `?.` guards an undefined channel but not a throw from an already-closed one (ERR_IPC_CHANNEL_CLOSED). A throw in the catch-branch send() would escape the message handler and skip the scheduled `setTimeout(process.exit(0))`, stranding the worker (with skip-close leaving native handles open, #2264). Wrap process.send in try/catch so the exit always fires; a vanished child is a failure to the parent regardless. Not unit-tested: send() is module-private and importing the worker registers process signal handlers; the change is a defensive try/catch around one call. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(cli): bound the SIGINT cleanup CHECKPOINT so Ctrl-C stays responsive (#2264 review P3) The SIGINT handler calls closeLbug({skipNativeClose:true}), whose flushWAL CHECKPOINT queues behind the connection lock held by an in-flight COPY — so a single Ctrl-C during a long --pdg COPY appeared hung until the COPY released. Race the cleanup against a 2s timeout before process.exit(130); the WAL replays on the next analyze. The double-Ctrl-C escape hatch is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): report analyze-worker errors over IPC, never swallow (#2264 P3) The worker's send() swallowed IPC failures (and a prior pass logged them to stderr). Per review, all worker errors must be reported back to the parent over the existing IPC channel (send({ type: 'error' })) and nothing silently dropped. - send() no longer catches: a dead channel (ERR_IPC_CHANNEL_CLOSED) throws instead of being swallowed. - Every handler (uncaughtException, unhandledRejection, SIGTERM, the analysis message handler) reports its error via send() in try and schedules process.exit in finally, so a throw from send() can no longer skip the exit and wedge the worker — the P3 'schedule the exit so it always fires' fix, without a swallow. - SIGTERM cleanup failures are now reported to the parent instead of an empty catch {}. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(workers): report every caught parse-worker error over IPC (#2264) The parse worker swallowed or only-locally-logged several caught errors, so they never reached the pool: the per-language-group catch was an empty catch {} that silently dropped the whole group on any throw (not just an unavailable grammar), the per-file parse/query-execution catches only logger.warn'd (worker-thread local), and the C++ template-constraint catch swallowed silently. Route all work-path catches through a new reportWarning() helper that posts { type: 'warning', message } to the pool (which logs it on the main thread AND resets the worker idle timer, so a worker grinding through failing files isn't falsely idle-evicted), with a logger.warn fallback for the non-worker path. The existing inline warning sites (query-compilation, the extractParsedFile callback, CFG build) are migrated to the same helper. The 4 optional-grammar module-load guards (Swift/Dart/Kotlin/C) stay silent: they run before the 'ready' handshake and their absence is already surfaced via result.skippedLanguages + the isLanguageAvailable gate. Fatal/group-aborting errors continue to flow through the message handler's { type: 'error', errorStack }. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(cli): harden finalize-failure test against forked-worker death (#2264) analyzeCommand calls installFatalHandlers(), which registers global unhandledRejection/uncaughtException handlers that call the REAL process.exit(1). Across this file's vi.resetModules() reimports they accumulate on `process`, and under CI timing a stray async rejection fired one while no process.exit spy was active — killing the forked vitest worker ("Worker exited unexpectedly"), which only surfaced once the full test lanes finished (they were pending at review time). Keep process.exit spied for the whole file (beforeAll/afterAll) so a fatal handler can never really exit mid-run, strip the handlers installFatalHandlers added in afterAll (preserving vitest's own, snapshotted up front) before restoring the real process.exit, and reset process.exitCode so the worker exits clean. Passes in isolation and grouped; behavior under test is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(analyze): don't take the up-to-date fast path for an unregistered repo (#2264) A prior 'analyze --name X' that hit a registry name collision writes meta.json (meta-save runs before registerRepo) but fails before registering — leaving the index up-to-date but UNREGISTERED. A later 'analyze --name X --allow-duplicate-name' then matched the up-to-date gate and early-returned WITHOUT registering, so the repo stayed invisible to list_repos/MCP and the CLI's assertAnalysisFinalized rejected it. --allow-duplicate-name could never heal it. This was latent on main, masked by the very close-hang this PR fixes: the lingering process pushed the cli-e2e #829 step-3 analyze past its 60s spawn timeout (status===null → the test's vacuous early-return). With the hang gone the analyze exits promptly, exit 1 surfaces, and the bug becomes deterministic on all platforms. Fix: the up-to-date fast path now short-circuits only when the repo is actually registered (new isRepoRegistered helper, sharing assertAnalysisFinalized's exact canonical/case-folded membership check). An indexed-but-unregistered repo falls through to the pipeline, which registers it honoring allowDuplicateName. Already registered repos keep the fast path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * chore: trigger CI re-run Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(analyze): gate up-to-date self-heal on --allow-duplicate-name (#2264) The prior commit healed every up-to-date-but-unregistered repo by falling through to register it — which broke the #1169 guard: a plain `analyze` of an up-to-date repo whose registry entry is missing MUST fail loudly ("Analysis did not finalize") rather than silently register a possibly half-finalized index. Distinguish the two causes of "unregistered": - collision-rejected + user re-runs with --allow-duplicate-name → explicit intent to register, so fall through to the pipeline and register it (#829). - plain analyze, registry missing/wiped → keep the #1169 fail-loud behavior. So self-heal is gated on options.allowDuplicateName; isRepoRegistered is only read on that opt-in branch, so the common fast path keeps its single-stat cost. Both cli-e2e guards (#1169 fail-loud, #829 heal) now pass. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): skip the native close on analyze-worker SIGTERM cancellation (#2264 P2) cancelJob() / the 30-min timeout (analyze-job.ts) send SIGTERM to the forked analyze worker, but its SIGTERM handler still did a full `await closeLbug()` (native conn/db teardown) — even though normal completion now skips it via skipNativeCloseOnExit. A cancelled or timed-out --pdg server analyze could therefore still hit the LadybugDB ClientContext destructor double-free, or block behind the in-flight COPY's connection lock before exiting. Mirror the CLI SIGINT path: a best-effort CHECKPOINT with closeLbug({ skipNativeClose: true }) bounded by a 2s Promise.race timeout, then process.exit(0) (which reclaims the handles). A CHECKPOINT failure is reported to the parent over IPC rather than swallowed; the exit is in .finally so it always fires. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(cli): import analyze once in the finalize-failure test (#2264 CI) The test failed deterministically only on the ubuntu coverage lane (2/2 runs) while passing locally and in isolation, incl. with --coverage. Cause: the vi.resetModules() + per-test `await import('analyze.js')` re-instrumented the ENTIRE analyze module graph on every test; under --coverage on the memory-constrained CI runner that OOM/crashed the forked worker ("Worker exited unexpectedly" → the assertion never ran). Import analyzeCommand ONCE and drive the mocks per-test via mockReturnValue (resetModules wasn't needed — the hoisted mocks are controllable per-test). Keeps the whole-file process.exit spy + afterAll fatal-handler strip from the prior pass. Behavior under test is unchanged; passes in isolation and with --coverage. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(cli): pre-set NODE_OPTIONS heap cap so ensureHeap can't re-exec (#2264 CI) Root cause of the ubuntu-coverage-only failure (3/3 CI runs, passing locally): analyzeCommand calls ensureHeap() (analyze.ts:715), which RE-EXECS the process — spawning `node ` with vitest's argv — unless NODE_OPTIONS already carries --max-old-space-size (analyze.ts:498). That re-exec killed the forked vitest worker ("Worker exited unexpectedly" → the assertion never ran). It only reproduced on the memory-constrained CI runner because locally a high V8 heap-size-limit also short-circuits ensureHeap (analyze.ts:501). Reproduced locally with NODE_OPTIONS="--no-warnings" (no heap cap) → same failure; fixed by pre-setting --max-old-space-size in beforeAll (restored in afterAll), the same workaround cli-e2e uses. Verified: passes under the repro condition, normally, and with --coverage. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): worker asserts finalization before reporting complete (#2264 P2) The forked analyze worker reported {type:'complete'} straight after runFullAnalysis, so a server/web analyze of a half-finalized repo (meta.json written but the global registry entry missing — a prior collision-aborted run, or a wiped registry) was reported successful while the repo stayed unregistered/invisible to list_repos. The CLI already guards this with assertAnalysisFinalized; the worker did not. Extract the run -> finalize -> report contract into a side-effect-free analyze-worker-core seam (the entry module's top-level process.on handlers make it untestable directly) and call assertAnalysisFinalized before sending complete — a failure is reported as {type:'error'} instead of a false success. The seam is dependency-injected and unit-tested with fakes; the entry module wires the real deps and keeps owning process.exit. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): coordinate worker SIGTERM cancellation with completion (#2264 P3) The worker SIGTERM handler unconditionally sent {type:'error','Analysis cancelled'} and didn't coordinate with the message handler that sends complete, so a cancel near the finish line could report a cancelled job complete, or a late SIGTERM could flip an already-complete job to failed. Add a single terminal-outcome claim (createTerminalClaim) shared by the message handler and the SIGTERM handler: whoever claims it first reports its terminal message; the other skips its terminal send. Single-threaded JS makes the check-and-set atomic. The cleanup + process.exit still run regardless. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(server): make a job's terminal outcome immutable on the parent side (#2264 P3) Defense-in-depth complement to the worker terminal-claim: the launcher's message handler and the job manager's updateJob both lacked a terminal-state guard, so a late worker IPC message (a SIGTERM-driven 'error' after 'complete', or vice versa) could re-release the repo lock and flip the reported status. (Touches parent-side files outside the original PR diff — deliberate, clearly-scoped.) - analyze-job.ts updateJob: drop any update once the job is already terminal (the transition INTO terminal still applies, since status isn't terminal yet then). - analyze-launch.ts message handler: return early when the job is already terminal, mirroring its sibling exit handler. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(lbug): assert deleteAllInterprocTaintPaths + deleteAllCallSummaries route through withConnLock (#2264) The lock-routing suite covered 4 singleton-conn helpers but not these two withConnLock-wrapped delete helpers (lbug-adapter.ts), which also run during the incremental --pdg writeback window — so a revert of either wrapper would have gone uncaught. Add the two routing assertions to complete the coverage the file's header claims (every singleton-conn helper reachable during the WAL-driver window). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(lbug): assert temp-conn deleteNodesForFile skips withConnLock (negative gate, #2264) The positive case (singleton deleteNodesForFile locks each per-table count) was covered, but not the negative branch of the targetConn === conn gate: a per-file/temp connection (dbPath provided) must NOT take the singleton lock, or temp-conn callers would needlessly contend with it. Add the negative-gate assertion so a regression that unconditionally locks is caught. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * test(cli): assert the force-exit forwards process.exitCode, not a hardcoded 1 (#2264) The existing cases asserted process.exit(1), but since the error catch always sets exitCode=1 they couldn't distinguish forwarding (process.exit(process.exitCode ?? 1)) from a hardcoded 1. Add a case on the alreadyUpToDate path — which returns without setting exitCode or calling process.exit — with a pre-set exitCode=2 and isLbugReady forced true, asserting the wrapper force-exits with 2. Proves the exitCode-forwarding branch. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(lbug): replace skipNativeClose flag with a dedicated closeLbugBeforeExit() (#2264) The "skip the native close only when a process.exit is guaranteed to follow" invariant was enforced by convention across ~4 call sites via a boolean option on closeLbug — the exact foot-gun the review flagged. Encode the contract in the name instead: - New closeLbugBeforeExit() (CHECKPOINT via flushWAL, then return without the native close); closeLbug() drops the option and is the plain real-close again. - run-analyze success + error paths: options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug(). CLI SIGINT + worker SIGTERM call closeLbugBeforeExit() directly. skipNativeCloseOnExit stays on AnalyzeOptions as the caller's "I will exit" signal. - lbug-checkpoint.test: assert closeLbugBeforeExit exists + has no native close, and match `closeLbug =` precisely so it doesn't prefix-match the new function. - Retarget the conn-serialization integration case to closeLbugBeforeExit(); add the new export to the 12 analyze-*.test.ts lbug-adapter mocks. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(lbug): extract isSharedSingletonConn predicate for the lock gate (#2264) The targetConn === conn object-identity gate (decides whether an op takes withConnLock) was duplicated inline in queryAndDrain and deleteNodesForFile with its own explanatory comments. Extract a single isSharedSingletonConn(c) predicate with the rationale in one place; both sites route through it. Behavior unchanged — covered by the lock-routing tests' positive (singleton locks) and negative (temp-conn skips) cases. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(lbug): share the bounded checkpoint-then-exit cleanup (SIGINT/SIGTERM) (#2264) The CLI SIGINT handler (analyze.ts) and the worker SIGTERM handler (analyze-worker.ts) had near-identical Promise.race([closeLbugBeforeExit, timeout]).finally(exit) blocks with separately-hardcoded 2s timeouts. Extract boundedCheckpointBeforeExit into a shared shutdown-helpers module — parameterized by exit code, an optional flush-error reporter (worker reports over IPC), and an optional beforeExit hook (CLI flushes the logger). checkpoint + exit are injectable test seams, so it's unit-tested without the real LadybugDB close or process.exit. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * refactor(storage): extract registryPathEquals for the registry case-fold compare (#2264) The Windows case-insensitive / POSIX case-sensitive registry-path comparison was duplicated across 6 sites (registerRepo dedup, the fresh-merge findIndex, removeRepo/removeBranchIndex local 'matches' helpers, isRepoRegistered, and the path-match lookup). Extract a single registryPathEquals(a, b) predicate so every registry lookup/dedup/finalize check answers identically; route all 6 through it. No behavior change — repo-manager + finalize-invariant suites pass (incl. the Windows case-fold case). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * feat(lbug): runtime-guard streamQuery against the WAL-checkpoint driver (#2264) streamQuery is deliberately not wrapped in withConnLock (its per-row callback can re-enter the adapter), so its unlocked per-row reads could race a CHECKPOINT on the shared connection — the corruption window the lock serializes everything else against. That invariant was comment-only, safe today only because the serve/read path forks analyze workers. Make it enforced: - lbug-adapter: a walDriverActive flag + markWalDriverActive(bool); streamQuery throws an actionable error when the driver is active. - wal-checkpoint-driver: arm the flag on start, disarm in stop() AFTER the in-flight CHECKPOINT drains (clearing earlier would briefly allow a race). A future in-process analyze overlapping a stream now fails loud instead of corrupting native state. (reentrancy test's lbug-adapter mock gains the new export.) Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * docs(lbug): explain why closeLbugBeforeExit skips finalizeLbugSidecarsAfterClose (#2264) Document the deliberate trade-off: the skip-close path intentionally does NOT run the sidecar-finalize step that safeClose runs after a real close. It's designed for released WAL handles; running it with the connection still open risks a Windows file-lock on the in-use WAL. The CHECKPOINT already made the index durable and the next run's preflightLbugSidecars reconciles residual WAL — the deferral is the accepted cost of skipping the native close to dodge the destructor double-free. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm * fix(lbug): move WAL-driver-active flag to its own module (fix mock ripple, #2264) The streamQuery guard (4449bb4a) put markWalDriverActive in lbug-adapter, and the wal-checkpoint-driver imported it from there. That broke every test mocking lbug-adapter while loading the real driver — CI's ubuntu lane caught run-analyze-fts-repair.test.ts ('No markWalDriverActive export on the mock'). Move the one-bit shared flag to a dedicated wal-driver-state module: the driver toggles markWalDriverActive there, streamQuery reads isWalDriverActive there, and lbug-adapter no longer carries it — so mocking lbug-adapter no longer has to stub the toggle. run-analyze-fts-repair now passes untouched; the reentrancy test's mock addition is reverted (no longer needed). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm --------- Co-authored-by: Claude Opus 4.8 (1M context) --- gitnexus/src/cli/analyze.ts | 35 +- .../core/ingestion/workers/parse-worker.ts | 68 ++- gitnexus/src/core/lbug/conn-lock.ts | 66 +++ gitnexus/src/core/lbug/lbug-adapter.ts | 553 +++++++++++------- gitnexus/src/core/lbug/shutdown-helpers.ts | 53 ++ .../src/core/lbug/wal-checkpoint-driver.ts | 16 + gitnexus/src/core/lbug/wal-driver-state.ts | 25 + gitnexus/src/core/run-analyze.ts | 43 +- gitnexus/src/server/analyze-job.ts | 6 + gitnexus/src/server/analyze-launch.ts | 7 + gitnexus/src/server/analyze-worker-core.ts | 94 +++ gitnexus/src/server/analyze-worker.ts | 107 ++-- gitnexus/src/storage/repo-manager.ts | 46 +- .../lbug-conn-serialization.test.ts | 155 +++++ .../analyze-embedding-endpoint-flags.test.ts | 2 + .../unit/analyze-embeddings-limit.test.ts | 2 + .../analyze-finalize-failure-exits.test.ts | 180 ++++++ gitnexus/test/unit/analyze-gitnexusrc.test.ts | 6 +- .../test/unit/analyze-heap-respawn.test.ts | 2 + gitnexus/test/unit/analyze-job.test.ts | 32 + .../analyze-lbug-checkpoint-threshold.test.ts | 2 + .../analyze-local-embedding-error.test.ts | 2 + .../test/unit/analyze-no-stats-bridge.test.ts | 2 + .../analyze-respawn-progress-terminal.test.ts | 2 + gitnexus/test/unit/analyze-wal-error.test.ts | 2 + .../test/unit/analyze-worker-core.test.ts | 143 +++++ .../unit/analyze-worker-pool-size.test.ts | 2 + .../test/unit/analyze-worker-timeout.test.ts | 2 + gitnexus/test/unit/conn-lock.test.ts | 109 ++++ gitnexus/test/unit/lbug-checkpoint.test.ts | 39 +- .../repo-manager-finalize-invariant.test.ts | 14 + gitnexus/test/unit/shutdown-helpers.test.ts | 62 ++ .../unit/stream-query-driver-guard.test.ts | 53 ++ .../wal-checkpoint-driver-reentrancy.test.ts | 66 +++ gitnexus/vitest.config.ts | 2 + 35 files changed, 1699 insertions(+), 301 deletions(-) create mode 100644 gitnexus/src/core/lbug/conn-lock.ts create mode 100644 gitnexus/src/core/lbug/shutdown-helpers.ts create mode 100644 gitnexus/src/core/lbug/wal-driver-state.ts create mode 100644 gitnexus/src/server/analyze-worker-core.ts create mode 100644 gitnexus/test/integration/lbug-conn-serialization.test.ts create mode 100644 gitnexus/test/unit/analyze-finalize-failure-exits.test.ts create mode 100644 gitnexus/test/unit/analyze-worker-core.test.ts create mode 100644 gitnexus/test/unit/conn-lock.test.ts create mode 100644 gitnexus/test/unit/shutdown-helpers.test.ts create mode 100644 gitnexus/test/unit/stream-query-driver-guard.test.ts create mode 100644 gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 0cf17ab13..d399c5082 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -13,7 +13,8 @@ import os from 'os'; import { spawn } from 'child_process'; import v8 from 'v8'; import cliProgress from 'cli-progress'; -import { closeLbug } from '../core/lbug/lbug-adapter.js'; +import { isLbugReady } from '../core/lbug/lbug-adapter.js'; +import { boundedCheckpointBeforeExit } from '../core/lbug/shutdown-helpers.js'; import { isLbugCheckpointIoError, isWalCorruptionError, @@ -738,6 +739,17 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption } finally { restoreAnalyzeEnv(envSnap); } + // If analyzeCommandImpl returned via a soft `process.exitCode = 1` error path + // while LadybugDB native handles are still open, the event loop won't drain and + // the process would HANG (#2264 review P1). The full analyze paths skip-close the + // DB — handles are left open and reclaimed by process.exit — so a soft return + // after a real analyze must force the exit. The success path never reaches here + // (analyzeCommandImpl calls process.exit(0) itself); early-validation errors and + // unit tests that mock runFullAnalysis never open the DB, so isLbugReady() is + // false and the soft return is preserved. + if (isLbugReady()) { + process.exit(typeof process.exitCode === 'number' ? process.exitCode : 1); + } }; const analyzeCommandImpl = async ( @@ -1168,13 +1180,18 @@ const analyzeCommandImpl = async ( aborted = true; bar.stop(); console.log('\n Interrupted — cleaning up...'); - closeLbug() - .catch(() => {}) - .finally(async () => { + // Bounded CHECKPOINT-then-exit (#2264 review P3): skip the native close (the + // LadybugDB destructor can double-free after --pdg writes), but don't hang + // behind a long --pdg COPY holding the connection lock — bound it so a single + // Ctrl-C stays responsive; the WAL replays on the next analyze. A second + // Ctrl-C (`if (aborted) process.exit(1)` above) remains the escape hatch. + void boundedCheckpointBeforeExit({ + exitCode: 130, + beforeExit: async () => { const { flushLoggerSync } = await import('../core/logger.js'); flushLoggerSync(); - process.exit(130); - }); + }, + }); }; process.on('SIGINT', sigintHandler); @@ -1273,6 +1290,12 @@ const analyzeCommandImpl = async ( // Extra fetch-wrapper names from `.gitnexusrc` (#1589/#1852 residual); // forwarded to the routes phase consumer scan. fetchWrappers: options.fetchWrappers, + // The CLI always process.exit()s after this returns (success path at the + // end of analyzeCommandImpl, error/interrupt paths via process.exit too), + // so the finalize close skips the native conn/db close — it can double-free + // in LadybugDB's ClientContext destructor after --pdg writes (#2264). The + // CHECKPOINT keeps the index durable; process exit reclaims the handles. + skipNativeCloseOnExit: true, }, { onProgress: (_phase, percent, message) => { diff --git a/gitnexus/src/core/ingestion/workers/parse-worker.ts b/gitnexus/src/core/ingestion/workers/parse-worker.ts index 6c603984f..436833a5a 100644 --- a/gitnexus/src/core/ingestion/workers/parse-worker.ts +++ b/gitnexus/src/core/ingestion/workers/parse-worker.ts @@ -966,8 +966,13 @@ const processBatch = ( try { setLanguage(language, regularFiles[0].path); processFileGroup(regularFiles, language, queryString, result, onFileProcessed); - } catch { - // parser unavailable — skip this language group + } catch (err) { + // A throw here drops the whole language group — surface it to the pool + // (#2264) instead of silently skipping. The old empty catch hid real + // extractor/parser failures, not just an unavailable grammar. + reportWarning( + `Skipped ${regularFiles.length} ${language} file(s) after a processing error: ${err instanceof Error ? err.message : String(err)}`, + ); } } else { result.skippedLanguages[language] = @@ -981,8 +986,12 @@ const processBatch = ( try { setLanguage(language, tsxFiles[0].path); processFileGroup(tsxFiles, language, queryString, result, onFileProcessed); - } catch { - // parser unavailable — skip this language group + } catch (err) { + // See above — surface a tsx-group processing failure rather than + // silently dropping every file in it (#2264). + reportWarning( + `Skipped ${tsxFiles.length} ${language} (tsx) file(s) after a processing error: ${err instanceof Error ? err.message : String(err)}`, + ); } } else { result.skippedLanguages[language] = @@ -1142,6 +1151,23 @@ export function extractORMQueries( import { extractFastAPIRouterBindings } from '../route-extractors/fastapi-router-bindings.js'; +/** + * Report a non-fatal worker issue to the pool over IPC so a caught error is not + * invisible to the operator (#2264). The pool logs it on the main thread AND + * resets the worker idle timer (so a worker grinding through failing files isn't + * falsely idle-evicted). Falls back to the local logger when there's no parent — + * this code also runs on the main thread in tests / the non-worker path. Fatal, + * group-aborting errors go through the message handler's + * `{ type: 'error', errorStack }` channel instead. + */ +function reportWarning(message: string): void { + if (parentPort) { + parentPort.postMessage({ type: 'warning', message }); + } else { + logger.warn(message); + } +} + const processFileGroup = ( files: ParseWorkerInput[], language: SupportedLanguages, @@ -1154,12 +1180,9 @@ const processFileGroup = ( const lang = parser.getLanguage(); query = new Parser.Query(lang, queryString); } catch (err) { - const message = `Query compilation failed for ${language}: ${err instanceof Error ? err.message : String(err)}`; - if (parentPort) { - parentPort.postMessage({ type: 'warning', message }); - } else { - logger.warn(message); - } + reportWarning( + `Query compilation failed for ${language}: ${err instanceof Error ? err.message : String(err)}`, + ); return; } @@ -1203,7 +1226,7 @@ const processFileGroup = ( bufferSize: getTreeSitterBufferSize(parseContent), }); } catch (err) { - logger.warn( + reportWarning( `Failed to parse file ${file.path}: ${err instanceof Error ? err.message : String(err)}`, ); continue; @@ -1216,7 +1239,7 @@ const processFileGroup = ( try { matches = query.matches(tree.rootNode); } catch (err) { - logger.warn( + reportWarning( `Query execution failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, ); continue; @@ -1237,13 +1260,7 @@ const processFileGroup = ( provider, parseContent, file.path, - (message) => { - if (parentPort) { - parentPort.postMessage({ type: 'warning', message }); - } else { - logger.warn(message); - } - }, + reportWarning, tree, scopeSourceKind, ); @@ -1306,9 +1323,9 @@ const processFileGroup = ( }; } } catch (err) { - const message = `CFG build failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`; - if (parentPort) parentPort.postMessage({ type: 'warning', message }); - else logger.warn(message); + reportWarning( + `CFG build failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, + ); } } @@ -2109,7 +2126,12 @@ const processFileGroup = ( if (parsedTemplateConstraints !== undefined) { constraintsTag = templateConstraintsIdTag(parsedTemplateConstraints); } - } catch { + } catch (err) { + // Optional C++ template-constraint enrichment: fall back to no tag, but + // surface the failure (#2264) — matches the CFG-build warning above. + reportWarning( + `Template-constraint extraction failed for ${file.path}: ${err instanceof Error ? err.message : String(err)}`, + ); parsedTemplateConstraints = undefined; constraintsTag = ''; } diff --git a/gitnexus/src/core/lbug/conn-lock.ts b/gitnexus/src/core/lbug/conn-lock.ts new file mode 100644 index 000000000..7948371bc --- /dev/null +++ b/gitnexus/src/core/lbug/conn-lock.ts @@ -0,0 +1,66 @@ +/** + * Serialize every operation on the shared singleton LadybugDB connection. + * + * LadybugDB is single-writer and its `Connection` is NOT safe for concurrent + * query execution: dispatching two queries on one connection at the same time + * lets two libuv workers mutate shared native engine state at once, corrupting + * the heap. This surfaced as `double free or corruption (out)` / SIGSEGV at the + * end of `analyze --pdg`, where the periodic WAL-checkpoint driver + * (`wal-checkpoint-driver.ts`) fired `CHECKPOINT` on the same connection a + * long-running PDG-table COPY was still using. `--pdg` makes those COPYs outlast + * the driver's 5 s tick, so the overlap (rare without `--pdg`) becomes reliable. + * + * Every singleton-`conn` helper in `lbug-adapter.ts` runs its full query + + * result-drain inside this lock, so the checkpoint driver, the bulk COPY, the + * embedding writeback, and the PDG edge deletes are mutually exclusive — the + * property that makes a strictly-serial workload stable. + * + * Implementation: a promise chain. Each caller installs a fresh unresolved tail, + * awaits the previous holder's tail, runs, then releases its own in `finally` + * (so a thrown op never wedges the connection). FIFO and non-reentrant: a wrapped + * helper MUST NOT call another wrapped helper — the inner call would await its own + * holder's tail and deadlock. The re-entry guard below catches this and throws + * instead of hanging. A boolean flag can't do this: a legitimately-queued + * top-level caller also runs while the lock is held, so only AsyncLocalStorage — + * which marks the *async context* of the running `fn` — distinguishes a true + * nested call from normal contention. + */ +import { AsyncLocalStorage } from 'node:async_hooks'; + +let tail: Promise = Promise.resolve(); + +// Set (to `true`) only inside a holding `fn`'s async context. A withConnLock call +// that observes it set is a nested/re-entrant call from within a critical section. +const inCriticalSection = new AsyncLocalStorage(); + +export const withConnLock = async (fn: () => Promise): Promise => { + if (inCriticalSection.getStore()) { + throw new Error( + 'conn-lock re-entry: a withConnLock-wrapped helper called another wrapped ' + + 'helper, which would deadlock the single LadybugDB connection. Run the inner ' + + 'work outside the lock, or inline it. See src/core/lbug/conn-lock.ts.', + ); + } + const prior = tail; + let release!: () => void; + tail = new Promise((resolve) => { + release = resolve; + }); + await prior; + try { + return await inCriticalSection.run(true, fn); + } finally { + release(); + } +}; + +/** + * Test-only: reset the lock chain to a fresh resolved tail. Production code has + * no reason to call this — a leaked-but-resolved tail is harmless — but unit + * tests want a clean chain per case. + * + * @internal + */ +export const _resetConnLockForTests = (): void => { + tail = Promise.resolve(); +}; diff --git a/gitnexus/src/core/lbug/lbug-adapter.ts b/gitnexus/src/core/lbug/lbug-adapter.ts index eb04ec5a4..903493f8f 100644 --- a/gitnexus/src/core/lbug/lbug-adapter.ts +++ b/gitnexus/src/core/lbug/lbug-adapter.ts @@ -6,6 +6,8 @@ import { finished } from 'stream/promises'; import path from 'path'; import lbug from '@ladybugdb/core'; import { closeQueryResults } from './query-result-utils.js'; +import { withConnLock } from './conn-lock.js'; +import { isWalDriverActive } from './wal-driver-state.js'; import { KnowledgeGraph } from '../graph/types.js'; import { NODE_TABLES, @@ -178,6 +180,25 @@ export const splitRelCsvByLabelPair = async ( let db: lbug.Database | null = null; let conn: lbug.Connection | null = null; + +// Serialize every operation on the shared singleton `conn`. LadybugDB's +// Connection is single-writer and is NOT safe for concurrent query execution; +// the periodic WAL-checkpoint driver overlapping a long `--pdg` COPY on this +// connection corrupted native state (`double free or corruption`). Each +// singleton-`conn` helper below runs its full query + drain inside withConnLock. +// Invariant: a wrapped helper MUST NOT call another wrapped helper (re-entry +// self-deadlocks); all current holders are leaf-level. `streamQuery` is +// deliberately NOT wrapped — its per-row callback can re-enter the adapter and +// it only runs on the read path where the checkpoint driver is inactive. +// See conn-lock.ts for the full rationale. +// +// The gate that decides whether an op must take withConnLock: only operations on +// the shared singleton `conn` serialize. Per-file / temp connections (distinct +// native objects with no shared engine state) must NOT block on — or be blocked +// by — the singleton's lock. Reads the live `conn` binding at call time (it's +// reassigned only at open/close, never mid-load). +const isSharedSingletonConn = (c: lbug.Connection): boolean => c === conn; + let currentDbPath: string | null = null; let currentDbReadOnly = false; let ftsLoaded = false; @@ -472,8 +493,14 @@ const readQueryRows = async ( }; const queryAndDrain = async (targetConn: lbug.Connection, cypher: string): Promise => { - const queryResult = await targetConn.query(cypher); - await drainQueryResult(queryResult); + const run = async (): Promise => { + const queryResult = await targetConn.query(cypher); + await drainQueryResult(queryResult); + }; + // Serialize only when this runs on the shared singleton connection (the bulk + // node/relationship COPY captures `writeConn = conn`); per-file / temp + // connections skip the lock — see isSharedSingletonConn. + return isSharedSingletonConn(targetConn) ? withConnLock(run) : run(); }; const READ_ONLY_SHADOW_REPLAY_PROBE = 'MATCH (n) RETURN n LIMIT 1'; @@ -1116,7 +1143,12 @@ export const loadGraphToLbug = async ( log(`Loading edges: ${pairIdx}/${relsByPair.size} types (${fromLabel} -> ${toLabel})`); } - await copyCsvWithRetry(conn, copyQuery, (retryErr) => { + // Use the captured `writeConn` (not the module-level `conn`) for the rel + // COPY, matching the node COPY above — one captured reference for the whole + // bulk load (#2264 review P3). Same object during analyze (`conn` is only + // reassigned at open/close under the session lock, never mid-load), so the + // queryAndDrain `targetConn === conn` lock gate still engages. + await copyCsvWithRetry(writeConn, copyQuery, (retryErr) => { const retryMsg = retryErr instanceof Error ? retryErr.message : String(retryErr); warnings.push(`${fromLabel}->${toLabel} (${rows} edges): ${retryMsg.slice(0, 80)}`); failedPairEdges += rows; @@ -1498,6 +1530,18 @@ export const streamQuery = async ( cypher: string, onRow: (row: any) => void | Promise, ): Promise => { + if (isWalDriverActive()) { + // streamQuery reads rows on the singleton connection WITHOUT withConnLock; if + // the WAL-checkpoint driver is live, those reads could race a CHECKPOINT — the + // #2264 corruption window. Today the serve/read path never runs the driver + // (analyze runs in a forked worker), so this fails loud only if a future + // in-process analyze overlaps a stream. Run analysis in a worker, or stop the + // driver before streaming. See conn-lock.ts. + throw new Error( + 'streamQuery cannot run while the WAL-checkpoint driver is active (it would ' + + 'race a CHECKPOINT on the unlocked read connection — #2264).', + ); + } if (!conn) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } @@ -1535,23 +1579,27 @@ export const executePrepared = async ( cypher: string, params: Record, ): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - const stmt = await conn.prepare(cypher); - if (!stmt.isSuccess()) { - const errMsg = await stmt.getErrorMessage(); - throw new Error(`Prepare failed: ${errMsg}`); - } - const queryResult = await conn.execute(stmt, params); - return await readQueryRows(queryResult); + return withConnLock(async () => { + const stmt = await c.prepare(cypher); + if (!stmt.isSuccess()) { + const errMsg = await stmt.getErrorMessage(); + throw new Error(`Prepare failed: ${errMsg}`); + } + const queryResult = await c.execute(stmt, params); + return await readQueryRows(queryResult); + }); }; export const executeWithReusedStatement = async ( cypher: string, paramsList: Array>, ): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } if (paramsList.length === 0) return; @@ -1559,39 +1607,50 @@ export const executeWithReusedStatement = async ( const SUB_BATCH_SIZE = 4; for (let i = 0; i < paramsList.length; i += SUB_BATCH_SIZE) { const subBatch = paramsList.slice(i, i + SUB_BATCH_SIZE); - const stmt = await conn.prepare(cypher); - if (!stmt.isSuccess()) { - const errMsg = await stmt.getErrorMessage(); - throw new Error(`Prepare failed: ${errMsg}`); - } - try { - for (const params of subBatch) { - await drainQueryResult(await conn.execute(stmt, params)); + // One critical section per sub-batch: the prepare + its executes run with + // exclusive access to the connection (so the WAL checkpoint driver cannot + // interleave a CHECKPOINT mid-batch), while the lock is released between + // sub-batches to let the driver checkpoint during a long writeback. + await withConnLock(async () => { + const stmt = await c.prepare(cypher); + if (!stmt.isSuccess()) { + const errMsg = await stmt.getErrorMessage(); + throw new Error(`Prepare failed: ${errMsg}`); } - } catch (e) { - const msg = e instanceof Error ? e.message : String(e); - const queryPreview = cypher.replace(/\s+/g, ' ').slice(0, 120); - throw new Error( - `Batch execution failed for rows ${i + 1}-${i + subBatch.length}: ${msg} (${queryPreview})`, - ); - } - // Note: LadybugDB PreparedStatement doesn't require explicit close() + try { + for (const params of subBatch) { + await drainQueryResult(await c.execute(stmt, params)); + } + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + const queryPreview = cypher.replace(/\s+/g, ' ').slice(0, 120); + throw new Error( + `Batch execution failed for rows ${i + 1}-${i + subBatch.length}: ${msg} (${queryPreview})`, + ); + } + // Note: LadybugDB PreparedStatement doesn't require explicit close() + }); } }; export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> => { - if (!conn) return { nodes: 0, edges: 0 }; + const c = conn; + if (!c) return { nodes: 0, edges: 0 }; + // Called during analyze finalize while the WAL-checkpoint driver is still + // running; each count read takes the connection lock so it cannot execute + // concurrently with a driver CHECKPOINT. Per-query locking lets the driver + // checkpoint between table counts rather than waiting for the whole sweep. let totalNodes = 0; for (const tableName of NODE_TABLES) { try { - const queryResult = await conn.query( - `MATCH (n:${escapeTableName(tableName)}) RETURN count(n) AS cnt`, - ); - const nodeRows = await readQueryRows(queryResult); - if (nodeRows.length > 0) { - totalNodes += Number(nodeRows[0]?.cnt ?? nodeRows[0]?.[0] ?? 0); - } + totalNodes += await withConnLock(async () => { + const queryResult = await c.query( + `MATCH (n:${escapeTableName(tableName)}) RETURN count(n) AS cnt`, + ); + const nodeRows = await readQueryRows(queryResult); + return nodeRows.length > 0 ? Number(nodeRows[0]?.cnt ?? nodeRows[0]?.[0] ?? 0) : 0; + }); } catch { // ignore } @@ -1599,13 +1658,13 @@ export const getLbugStats = async (): Promise<{ nodes: number; edges: number }> let totalEdges = 0; try { - const queryResult = await conn.query( - `MATCH ()-[r:${REL_TABLE_NAME}]->() RETURN count(r) AS cnt`, - ); - const edgeRows = await readQueryRows(queryResult); - if (edgeRows.length > 0) { - totalEdges = Number(edgeRows[0]?.cnt ?? edgeRows[0]?.[0] ?? 0); - } + totalEdges = await withConnLock(async () => { + const queryResult = await c.query( + `MATCH ()-[r:${REL_TABLE_NAME}]->() RETURN count(r) AS cnt`, + ); + const edgeRows = await readQueryRows(queryResult); + return edgeRows.length > 0 ? Number(edgeRows[0]?.cnt ?? edgeRows[0]?.[0] ?? 0) : 0; + }); } catch { // ignore } @@ -1624,67 +1683,75 @@ export const loadCachedEmbeddings = async (): Promise<{ embeddingNodeIds: Set; embeddings: CachedEmbedding[]; }> => { - if (!conn) { + const c = conn; + if (!c) { return { embeddingNodeIds: new Set(), embeddings: [] }; } - const embeddingNodeIds = new Set(); - const embeddings: CachedEmbedding[] = []; - try { - // Schema migration detection: query with new columns to verify schema version. - // Old schema only had (nodeId, embedding); new schema adds (id, chunkIndex, startLine, endLine, contentHash). - // If the query fails (column missing), we return empty cache to force a full rebuild. + // The whole read runs inside the connection lock (#2264 review P2). It's safe + // today only by call-ordering (loadCachedEmbeddings runs before the WAL driver + // starts), but the lock makes it robust to future reordering — a concurrent + // CHECKPOINT on the singleton connection is the documented corruption trigger. + // Leaf read: no nested withConnLock-wrapped helpers inside. + return withConnLock(async () => { + const embeddingNodeIds = new Set(); + const embeddings: CachedEmbedding[] = []; try { - const check = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex LIMIT 1`, - ); - await readQueryRows(check); - } catch { - return { embeddingNodeIds: new Set(), embeddings: [] }; - } - - // Try to read contentHash alongside chunk columns - let rows: any; - let hasContentHash = true; - try { - rows = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding, e.contentHash AS contentHash`, - ); - } catch (err: any) { - // Fallback for legacy DBs without contentHash column - const msg = err?.message ?? ''; - if (isMissingColumnOrTableError(msg)) { - hasContentHash = false; - rows = await conn.query( - `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding`, + // Schema migration detection: query with new columns to verify schema version. + // Old schema only had (nodeId, embedding); new schema adds (id, chunkIndex, startLine, endLine, contentHash). + // If the query fails (column missing), we return empty cache to force a full rebuild. + try { + const check = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex LIMIT 1`, ); - } else { - throw err; + await readQueryRows(check); + } catch { + return { embeddingNodeIds: new Set(), embeddings: [] }; } - } - for (const row of await readQueryRows(rows)) { - const nodeId = String(row.nodeId ?? row[0] ?? ''); - if (!nodeId) continue; - embeddingNodeIds.add(nodeId); - const embedding = row.embedding ?? row[4]; - if (embedding) { - embeddings.push({ - nodeId, - chunkIndex: Number(row.chunkIndex ?? row[1] ?? 0), - startLine: Number(row.startLine ?? row[2] ?? 0), - endLine: Number(row.endLine ?? row[3] ?? 0), - embedding: Array.isArray(embedding) - ? embedding.map(Number) - : Array.from(embedding as any).map(Number), - contentHash: hasContentHash ? (row.contentHash ?? row[5] ?? undefined) : undefined, - }); - } - } - } catch { - /* embedding table may not exist */ - } - return { embeddingNodeIds, embeddings }; + // Try to read contentHash alongside chunk columns + let rows: any; + let hasContentHash = true; + try { + rows = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding, e.contentHash AS contentHash`, + ); + } catch (err: any) { + // Fallback for legacy DBs without contentHash column + const msg = err?.message ?? ''; + if (isMissingColumnOrTableError(msg)) { + hasContentHash = false; + rows = await c.query( + `MATCH (e:${EMBEDDING_TABLE_NAME}) RETURN e.nodeId AS nodeId, e.chunkIndex AS chunkIndex, e.startLine AS startLine, e.endLine AS endLine, e.embedding AS embedding`, + ); + } else { + throw err; + } + } + for (const row of await readQueryRows(rows)) { + const nodeId = String(row.nodeId ?? row[0] ?? ''); + if (!nodeId) continue; + embeddingNodeIds.add(nodeId); + const embedding = row.embedding ?? row[4]; + if (embedding) { + embeddings.push({ + nodeId, + chunkIndex: Number(row.chunkIndex ?? row[1] ?? 0), + startLine: Number(row.startLine ?? row[2] ?? 0), + endLine: Number(row.endLine ?? row[3] ?? 0), + embedding: Array.isArray(embedding) + ? embedding.map(Number) + : Array.from(embedding as any).map(Number), + contentHash: hasContentHash ? (row.contentHash ?? row[5] ?? undefined) : undefined, + }); + } + } + } catch { + /* embedding table may not exist */ + } + + return { embeddingNodeIds, embeddings }; + }); }; /** @@ -1768,10 +1835,13 @@ export const fetchExistingEmbeddingHashes = async ( * @see safeClose — CHECKPOINT + connection/database close */ export const flushWAL = async (): Promise => { - if (!conn) return; + const c = conn; + if (!c) return; try { - const checkpointResult = await conn.query('CHECKPOINT'); - await drainQueryResult(checkpointResult); + await withConnLock(async () => { + const checkpointResult = await c.query('CHECKPOINT'); + await drainQueryResult(checkpointResult); + }); } catch (err) { logger.debug( `GitNexus: LadybugDB CHECKPOINT skipped/failed during WAL flush: ${summarizeError(err)}`, @@ -1794,9 +1864,15 @@ export const flushWAL = async (): Promise => { * whether to retry. */ export const tryFlushWAL = async (): Promise => { - if (!conn) return false; - const checkpointResult = await conn.query('CHECKPOINT'); - await drainQueryResult(checkpointResult); + const c = conn; + if (!c) return false; + // Runs on the periodic WAL-checkpoint driver. The lock makes this CHECKPOINT + // wait for any in-flight COPY / writeback on the singleton connection instead + // of executing concurrently with it (the `analyze --pdg` heap-corruption bug). + await withConnLock(async () => { + const checkpointResult = await c.query('CHECKPOINT'); + await drainQueryResult(checkpointResult); + }); return true; }; @@ -1856,6 +1932,39 @@ export const safeClose = async (): Promise => { } }; +/** + * CHECKPOINT for durability, then DELIBERATELY skip the native connection/database + * teardown. The name encodes the contract — there is no boolean flag to misuse: + * call this ONLY from a path that guarantees a `process.exit` immediately after + * (the CLI analyze success/SIGINT paths and the forked worker). + * + * LadybugDB's ClientContext/Connection destructor can double-free after large + * --pdg writes (gdb: `double free or corruption` in ClientContext::~ClientContext + * via NodeConnection::Close), aborting the process AFTER a fully-written, + * checkpointed index. flushWAL already persisted the data; process exit reclaims + * the native handles. We leave the handles referenced and module state intact so a + * GC finalizer cannot run the same destructor before exit, and any post-analyze + * read reuses the live connection. Mirrors the pool adapter's fire-and-forget + * native teardown (pool-adapter.ts) and the ONNX native-cleanup philosophy. + * Workaround for a LadybugDB engine bug (to be reported upstream). + * + * SAFETY: only valid when a process.exit is guaranteed to follow. Long-lived + * callers (MCP server, tests) leave `skipNativeCloseOnExit` unset, so + * runFullAnalysis closes for real via {@link closeLbug} — never this. + */ +export const closeLbugBeforeExit = async (): Promise => { + await flushWAL(); + // NOTE (#2264): unlike safeClose, this deliberately does NOT run + // finalizeLbugSidecarsAfterClose. That step inspects/quarantines orphan WAL + + // sidecar files and is designed to run AFTER the native close has released the + // WAL handle; running it here — with the connection still open — would risk a + // Windows file-lock on the in-use WAL for no benefit. The CHECKPOINT above + // already made the index durable, and the next run's preflightLbugSidecars + // reconciles any residual WAL on open. The deferred sidecar housekeeping is the + // accepted trade-off of skipping the native close to dodge the destructor + // double-free. +}; + export const closeLbug = async (): Promise => { await safeClose(); currentDbPath = null; @@ -1903,12 +2012,17 @@ export const deleteNodesForFile = async ( if (tableName === 'Community' || tableName === 'Process') continue; try { - // First count how many we'll delete + // First count how many we'll delete. On the singleton connection this + // count runs inside withConnLock (incremental --pdg writeback executes + // while the WAL driver is live); per-query/temp connections skip the + // lock, matching queryAndDrain's `targetConn === conn` gate — the sibling + // DETACH DELETE below already routes through it. (#2264) const tn = escapeTableName(tableName); - const countResult = await targetConn!.query( - `MATCH (n:${tn}) WHERE n.filePath = '${escapedPath}' RETURN count(n) AS cnt`, - ); - const rows = await readQueryRows(countResult); + const countCypher = `MATCH (n:${tn}) WHERE n.filePath = '${escapedPath}' RETURN count(n) AS cnt`; + const runCount = async () => readQueryRows(await targetConn!.query(countCypher)); + const rows = isSharedSingletonConn(targetConn!) + ? await withConnLock(runCount) + : await runCount(); const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); if (count > 0) { @@ -1959,7 +2073,8 @@ export const getEmbeddingTableName = (): string => EMBEDDING_TABLE_NAME; * exports. */ export const queryImporters = async (targetFilePath: string): Promise => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } const escaped = targetFilePath.replace(/'/g, "''"); @@ -1968,22 +2083,27 @@ export const queryImporters = async (targetFilePath: string): Promise WHERE r.type = 'IMPORTS' AND b.filePath = '${escaped}' RETURN DISTINCT a.filePath AS importer `; - let queryResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - queryResult = await conn.query(cypher); - const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; - const rows = await result.getAll(); - const out: string[] = []; - for (const row of rows) { - const v = (row as { importer?: unknown }).importer; - if (typeof v === 'string' && v.length > 0) out.push(v); + // Runs inside the connection lock: queryImporters is called in the importer-BFS + // loop during incremental --pdg writeback while the WAL driver is live, so an + // unlocked conn.query here could race a concurrent CHECKPOINT on the singleton. + return withConnLock(async () => { + let queryResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + queryResult = await c.query(cypher); + const result = Array.isArray(queryResult) ? queryResult[0] : queryResult; + const rows = await result.getAll(); + const out: string[] = []; + for (const row of rows) { + const v = (row as { importer?: unknown }).importer; + if (typeof v === 'string' && v.length > 0) out.push(v); + } + return out; + } catch { + return []; + } finally { + if (queryResult) await closeQueryResults(queryResult); } - return out; - } catch { - return []; - } finally { - if (queryResult) await closeQueryResults(queryResult); - } + }); }; /** @@ -1996,28 +2116,35 @@ export const queryImporters = async (targetFilePath: string): Promise export const deleteAllCommunitiesAndProcesses = async (): Promise<{ nodesDeleted: number; }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let nodesDeleted = 0; - for (const label of ['Community', 'Process']) { - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query(`MATCH (n:${label}) RETURN count(n) AS cnt`); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH (n:${label}) DETACH DELETE n`); - nodesDeleted += count; + // count + DETACH DELETE run inside the connection lock so they cannot execute + // concurrently with the WAL-checkpoint driver's CHECKPOINT on the singleton + // connection. This runs during incremental --pdg writeback while the driver is + // live; mirrors the wrapped deleteAllInterprocTaintPaths / deleteAllCallSummaries. + return withConnLock(async () => { + let nodesDeleted = 0; + for (const label of ['Community', 'Process']) { + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query(`MATCH (n:${label}) RETURN count(n) AS cnt`); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults(await c.query(`MATCH (n:${label}) DETACH DELETE n`)); + nodesDeleted += count; + } + } catch { + // Table may not exist yet on a freshly-initialized DB — fine. + } finally { + if (countResult) await closeQueryResults(countResult); } - } catch { - // Table may not exist yet on a freshly-initialized DB — fine. - } finally { - if (countResult) await closeQueryResults(countResult); } - } - return { nodesDeleted }; + return { nodesDeleted }; + }); }; /** @@ -2036,44 +2163,51 @@ export const deleteAllCommunitiesAndProcesses = async (): Promise<{ * plain DELETE on the typed CodeRelation rows — endpoints are untouched. */ export const deleteAllInterprocTaintPaths = async (): Promise<{ edgesDeleted: number }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let edgesDeleted = 0; - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query( - `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' RETURN count(r) AS cnt`, - ); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' DELETE r`); - edgesDeleted = count; - } - } catch (err) { - // A missing table on a freshly-initialized DB is the benign, expected case - // (the count query above is what throws) — stay silent. Any OTHER failure - // (lock, disk, native error) would leave stale TAINT_PATH rows that the - // subsequent re-extract then DUPLICATES (CodeRelation has no PK), so it - // must ABORT the writeback (#2084 review P2-5): re-throw so the caller's - // crash-recovery dirty flag forces a clean full rebuild on the next run, - // rather than silently writing duplicate cross-function findings. - const msg = err instanceof Error ? err.message : String(err); - if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + // count + DELETE run as one critical section on the singleton connection so a + // concurrent WAL-checkpoint cannot corrupt native state mid-delete (#pdg). + return withConnLock(async () => { + let edgesDeleted = 0; + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query( + `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' RETURN count(r) AS cnt`, + ); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults( + await c.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'TAINT_PATH' DELETE r`), + ); + edgesDeleted = count; + } + } catch (err) { + // A missing table on a freshly-initialized DB is the benign, expected case + // (the count query above is what throws) — stay silent. Any OTHER failure + // (lock, disk, native error) would leave stale TAINT_PATH rows that the + // subsequent re-extract then DUPLICATES (CodeRelation has no PK), so it + // must ABORT the writeback (#2084 review P2-5): re-throw so the caller's + // crash-recovery dirty flag forces a clean full rebuild on the next run, + // rather than silently writing duplicate cross-function findings. + const msg = err instanceof Error ? err.message : String(err); + if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + if (countResult) await closeQueryResults(countResult); + return { edgesDeleted }; + } if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + throw new Error( + `[taint-interproc] failed to clear existing TAINT_PATH edges before incremental ` + + `re-write (${msg}) — aborting to avoid duplicate cross-function findings; ` + + `the next run will full-rebuild`, + ); } if (countResult) await closeQueryResults(countResult); - throw new Error( - `[taint-interproc] failed to clear existing TAINT_PATH edges before incremental ` + - `re-write (${msg}) — aborting to avoid duplicate cross-function findings; ` + - `the next run will full-rebuild`, - ); - } - if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + return { edgesDeleted }; + }); }; /** @@ -2088,42 +2222,49 @@ export const deleteAllInterprocTaintPaths = async (): Promise<{ edgesDeleted: nu * an unchanged function's summary from being lost. */ export const deleteAllCallSummaries = async (): Promise<{ edgesDeleted: number }> => { - if (!conn) { + const c = conn; + if (!c) { throw new Error('LadybugDB not initialized. Call initLbug first.'); } - let edgesDeleted = 0; - let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; - try { - countResult = await conn.query( - `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' RETURN count(r) AS cnt`, - ); - const result = Array.isArray(countResult) ? countResult[0] : countResult; - const rows = await result.getAll(); - const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); - if (count > 0) { - await conn.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' DELETE r`); - edgesDeleted = count; - } - } catch (err) { - // A missing table on a freshly-initialized DB is the benign, expected case - // (the count query is what throws) — stay silent. Any OTHER failure would - // leave stale rows that the re-extract then DUPLICATES (CodeRelation has no - // PK), so it must ABORT the writeback: re-throw so the caller's crash- - // recovery dirty flag forces a clean full rebuild on the next run. - const msg = err instanceof Error ? err.message : String(err); - if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + // count + DELETE run as one critical section on the singleton connection so a + // concurrent WAL-checkpoint cannot corrupt native state mid-delete (#pdg). + return withConnLock(async () => { + let edgesDeleted = 0; + let countResult: lbug.QueryResult | lbug.QueryResult[] | undefined; + try { + countResult = await c.query( + `MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' RETURN count(r) AS cnt`, + ); + const result = Array.isArray(countResult) ? countResult[0] : countResult; + const rows = await result.getAll(); + const count = Number(rows[0]?.cnt ?? rows[0]?.[0] ?? 0); + if (count > 0) { + await closeQueryResults( + await c.query(`MATCH ()-[r:CodeRelation]->() WHERE r.type = 'CALL_SUMMARY' DELETE r`), + ); + edgesDeleted = count; + } + } catch (err) { + // A missing table on a freshly-initialized DB is the benign, expected case + // (the count query is what throws) — stay silent. Any OTHER failure would + // leave stale rows that the re-extract then DUPLICATES (CodeRelation has no + // PK), so it must ABORT the writeback: re-throw so the caller's crash- + // recovery dirty flag forces a clean full rebuild on the next run. + const msg = err instanceof Error ? err.message : String(err); + if (/no table|not exist|not found|does not exist|Table .* does not exist/i.test(msg)) { + if (countResult) await closeQueryResults(countResult); + return { edgesDeleted }; + } if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + throw new Error( + `[call-summary] failed to clear existing CALL_SUMMARY edges before incremental ` + + `re-write (${msg}) — aborting to avoid duplicate summaries; ` + + `the next run will full-rebuild`, + ); } if (countResult) await closeQueryResults(countResult); - throw new Error( - `[call-summary] failed to clear existing CALL_SUMMARY edges before incremental ` + - `re-write (${msg}) — aborting to avoid duplicate summaries; ` + - `the next run will full-rebuild`, - ); - } - if (countResult) await closeQueryResults(countResult); - return { edgesDeleted }; + return { edgesDeleted }; + }); }; // ============================================================================ diff --git a/gitnexus/src/core/lbug/shutdown-helpers.ts b/gitnexus/src/core/lbug/shutdown-helpers.ts new file mode 100644 index 000000000..542709d72 --- /dev/null +++ b/gitnexus/src/core/lbug/shutdown-helpers.ts @@ -0,0 +1,53 @@ +/** + * Shared bounded "checkpoint, then exit" cleanup for interrupt/cancel signals + * (#2264). The CLI SIGINT handler and the forked worker's SIGTERM handler both + * need to: CHECKPOINT the WAL for durability (skipping the native close — see + * closeLbugBeforeExit), but NOT hang behind an in-flight COPY that holds the + * connection lock, and then exit. Bounding the CHECKPOINT with a short timeout + * keeps a single Ctrl-C / cancel responsive; the WAL replays on the next analyze. + */ +import { closeLbugBeforeExit } from './lbug-adapter.js'; + +/** Default cap so a CHECKPOINT queued behind a long COPY can't wedge the signal. */ +export const DEFAULT_EXIT_CLEANUP_TIMEOUT_MS = 2000; + +export interface BoundedCheckpointExitOptions { + /** Exit code to terminate with (130 for SIGINT, 0 for a worker SIGTERM). */ + exitCode: number; + /** Cap on the CHECKPOINT; defaults to {@link DEFAULT_EXIT_CLEANUP_TIMEOUT_MS}. */ + timeoutMs?: number; + /** Report a CHECKPOINT failure (e.g. over IPC) rather than swallowing it. */ + onFlushError?: (err: unknown) => void; + /** Run just before exit (e.g. flush the logger synchronously). */ + beforeExit?: () => void | Promise; + /** @internal test seam — defaults to {@link closeLbugBeforeExit}. */ + checkpoint?: () => Promise; + /** @internal test seam — defaults to `process.exit`. */ + exit?: (code: number) => void; +} + +/** + * Best-effort CHECKPOINT bounded by a timeout, then exit. Never rejects — the + * exit always fires (in `finally`) even if the CHECKPOINT throws. Fire-and-forget + * from a signal handler (`void boundedCheckpointBeforeExit({...})`). + */ +export async function boundedCheckpointBeforeExit( + opts: BoundedCheckpointExitOptions, +): Promise { + const timeoutMs = opts.timeoutMs ?? DEFAULT_EXIT_CLEANUP_TIMEOUT_MS; + const checkpoint = opts.checkpoint ?? closeLbugBeforeExit; + const exit = opts.exit ?? ((code: number) => process.exit(code)); + let timer: ReturnType | undefined; + try { + await Promise.race([ + checkpoint().catch((err: unknown) => opts.onFlushError?.(err)), + new Promise((resolve) => { + timer = setTimeout(resolve, timeoutMs); + }), + ]); + } finally { + if (timer !== undefined) clearTimeout(timer); + await opts.beforeExit?.(); + exit(opts.exitCode); + } +} diff --git a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts index 57dc6b2d6..458c63947 100644 --- a/gitnexus/src/core/lbug/wal-checkpoint-driver.ts +++ b/gitnexus/src/core/lbug/wal-checkpoint-driver.ts @@ -34,6 +34,7 @@ import { logger } from '../logger.js'; import { tryFlushWAL } from './lbug-adapter.js'; +import { markWalDriverActive } from './wal-driver-state.js'; import { isLbugCheckpointIoError } from './lbug-config.js'; /** @@ -162,8 +163,20 @@ export const startWalCheckpointDriver = ( let stopped = false; let inflight: Promise | null = null; + // Arm the streamQuery guard: while this driver runs, an unlocked streamQuery on + // the singleton connection could race a CHECKPOINT (#2264). Cleared in stop(). + markWalDriverActive(true); + const tick = async (): Promise => { if (stopped) return; + // Reentrancy guard: setInterval keeps firing on its fixed cadence even when + // the previous checkpoint has not settled (a CHECKPOINT can outlast the + // period during a large `--pdg` writeback). Without this, each overdue tick + // would queue another CHECKPOINT — they now serialize on the connection lock + // (lbug-adapter `withConnLock`), but letting them pile up is still pointless + // work and widens the window for a backlog at stop(). Skip while one is in + // flight; the next tick covers any WAL accumulated in the meantime. + if (inflight) return; inflight = runCheckpointWithRetry() .then(() => undefined) .catch((err) => { @@ -210,6 +223,9 @@ export const startWalCheckpointDriver = ( /* swallowed in tick() — surface path is the surrounding write */ } } + // Disarm AFTER the in-flight CHECKPOINT drains — clearing it earlier would + // briefly let a streamQuery race the still-finishing CHECKPOINT (#2264). + markWalDriverActive(false); }, }; }; diff --git a/gitnexus/src/core/lbug/wal-driver-state.ts b/gitnexus/src/core/lbug/wal-driver-state.ts new file mode 100644 index 000000000..c86022c71 --- /dev/null +++ b/gitnexus/src/core/lbug/wal-driver-state.ts @@ -0,0 +1,25 @@ +/** + * Shared "is the manual WAL-checkpoint driver running?" flag (#2264). + * + * Lives in its own tiny module — NOT in lbug-adapter — on purpose: the + * wal-checkpoint-driver toggles it and lbug-adapter's `streamQuery` reads it, and + * putting it here keeps that one-bit coupling out of the big, heavily-mocked + * lbug-adapter surface. (Importing it from lbug-adapter forced every test that + * mocks lbug-adapter + loads the real driver to also stub the toggle — a brittle + * ripple this module avoids.) + * + * streamQuery is deliberately not wrapped in withConnLock (its per-row callback can + * re-enter the adapter), so it must refuse to run while the driver is live — + * otherwise its unlocked per-row reads could race a CHECKPOINT on the shared + * connection. The serve/read path never starts the driver, so this stays false + * there. + */ +let walDriverActive = false; + +/** Toggled by the WAL-checkpoint driver's start (true) / stop (false). */ +export const markWalDriverActive = (active: boolean): void => { + walDriverActive = active; +}; + +/** True while the manual WAL-checkpoint driver is running. @see streamQuery */ +export const isWalDriverActive = (): boolean => walDriverActive; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index cf22cf09e..0b5d9c9ba 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -21,6 +21,7 @@ import { executeQuery, executeWithReusedStatement, closeLbug, + closeLbugBeforeExit, loadCachedEmbeddings, deleteNodesForFile, deleteAllCommunitiesAndProcesses, @@ -42,6 +43,7 @@ import { loadMeta, ensureGitNexusIgnored, registerRepo, + isRepoRegistered, cleanupOldKuzuFiles, INCREMENTAL_SCHEMA_VERSION, type RepoMeta, @@ -232,6 +234,15 @@ export interface AnalyzeOptions { * consumer scan unchanged. */ fetchWrappers?: string[]; + /** + * The caller will `process.exit()` immediately after this analyze returns (the + * CLI `analyze` command). When set, the finalize/error close CHECKPOINTs for + * durability but skips the native `conn.close()`/`db.close()`, which can + * double-free in LadybugDB's `ClientContext` destructor after large `--pdg` + * writes (gdb-confirmed) — aborting the process AFTER a fully-written index. + * Process exit reclaims the handles. Long-lived callers (MCP server, tests) + * leave this unset so they get a real close. See `closeLbug`. */ + skipNativeCloseOnExit?: boolean; } export interface AnalyzeResult { @@ -768,7 +779,21 @@ export async function runFullAnalysis( return true; // conservative on git failure } })(); - if (!dirty) { + // Registration wrinkle around the fast path (#2264). A prior + // `analyze --name X` that hit a name collision writes meta.json (meta-save + // runs before registerRepo) then fails before registering, leaving the + // index up-to-date but UNREGISTERED. When the user re-runs with + // --allow-duplicate-name they explicitly want it registered, so fall + // through to the pipeline (which registers it, honoring the flag) instead + // of early-returning an unregistered repo the flag could never heal. + // For a PLAIN analyze we deliberately do NOT self-heal: an up-to-date but + // unregistered repo early-returns here and the CLI's assertAnalysisFinalized + // surfaces it as a hard failure (#1169) rather than silently registering a + // possibly half-finalized index. `isRepoRegistered` is only read on the + // opt-in branch so the common fast path keeps its single-stat cost. + const healUnregistered = + options.allowDuplicateName === true && !(await isRepoRegistered(repoPath)); + if (!dirty && !healUnregistered) { await ensureGitNexusIgnored(repoPath); return { // `resolveRepoIdentityRoot` collapses worktree roots to the @@ -1549,7 +1574,11 @@ export async function runFullAnalysis( // Stop the manual checkpoint driver before closeLbug so its // in-flight CHECKPOINT cannot race the `safeClose` CHECKPOINT. await walCheckpointDriver.stop(); - await closeLbug(); + // CLI callers (about to process.exit) skip the native close to dodge a + // LadybugDB destructor double-free after --pdg writes — closeLbugBeforeExit + // CHECKPOINTs for durability then leaves the handles for process exit to + // reclaim (#2264). Long-lived callers close for real. + await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug()); progress('done', 100, 'Done'); @@ -1570,7 +1599,15 @@ export async function runFullAnalysis( /* swallow — surface path is the rethrow below */ } try { - await closeLbug(); + // Skip the native close on the error path too: a real conn.close() after + // large --pdg writes can itself abort in LadybugDB's ClientContext + // destructor (#2264 review P2), turning an actionable exit-1 into a raw + // SIGABRT. closeLbugBeforeExit leaves the handles open, but the CLI catch + // now force-exits when isLbugReady() (analyze.ts, #2264 review P1), so the + // process still terminates — no hang, no abort. flushWAL keeps the partial + // index durable; process exit reclaims the handles. Long-lived callers + // (skipNativeCloseOnExit unset) close for real. + await (options.skipNativeCloseOnExit ? closeLbugBeforeExit() : closeLbug()); } catch { /* swallow */ } diff --git a/gitnexus/src/server/analyze-job.ts b/gitnexus/src/server/analyze-job.ts index f7d97e97d..d62912abd 100644 --- a/gitnexus/src/server/analyze-job.ts +++ b/gitnexus/src/server/analyze-job.ts @@ -101,6 +101,12 @@ export class JobManager { const job = this.jobs.get(id); if (!job) return; + // Once a job is terminal (complete/failed) its outcome is immutable — drop any + // later update so a worker `complete` racing a SIGTERM-driven `error` (or vice + // versa) can't flip a reported result (#2264 P3). The transition INTO a terminal + // state still applies because `job.status` is not yet terminal at that point. + if (this.isTerminal(job.status)) return; + Object.assign(job, update); if (this.isTerminal(job.status)) { diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index 184c5a813..87a463ca0 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/gitnexus/src/server/analyze-launch.ts @@ -81,6 +81,13 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { }); child.on('message', (msg: WorkerMessage) => { + // Ignore any message once the job is terminal — a late worker message (a + // SIGTERM-driven `error` after `complete`, or vice versa) must not + // re-release the repo lock or flip the reported status. Mirrors the `exit` + // handler guard below; pairs with the worker's terminal-claim (#2264 P3). + const current = jobManager.getJob(job.id); + if (!current || current.status === 'complete' || current.status === 'failed') return; + if (msg.type === 'progress') { jobManager.updateJob(job.id, { status: 'analyzing', diff --git a/gitnexus/src/server/analyze-worker-core.ts b/gitnexus/src/server/analyze-worker-core.ts new file mode 100644 index 000000000..acb31e6d1 --- /dev/null +++ b/gitnexus/src/server/analyze-worker-core.ts @@ -0,0 +1,94 @@ +/** + * Side-effect-free core of the analyze worker's message handler. + * + * Extracted from `analyze-worker.ts` — a `fork()` entry module whose top-level + * `process.on(...)` handlers and `ready` handshake make it unsafe to import in a + * unit test. This module has no top-level side effects and takes its collaborators + * by dependency injection, so the worker's run → finalize → report contract is + * unit-testable without spawning a process. The entry module wires the real deps + * and owns the `process.exit` lifecycle. + * + * The `import type ... typeof import(...)` forms below are erased at runtime, so + * importing this module does NOT load `run-analyze`, `repo-manager`, or the entry + * worker — only the lightweight `analyze-worker-ipc` projection helper. + */ +import type { AnalyzeOptions } from '../core/run-analyze.js'; +import type { WorkerMessage } from './analyze-worker.js'; +import { projectAnalyzeResultForIpc } from './analyze-worker-ipc.js'; + +export interface WorkerAnalysisDeps { + runFullAnalysis: typeof import('../core/run-analyze.js').runFullAnalysis; + assertAnalysisFinalized: typeof import('../storage/repo-manager.js').assertAnalysisFinalized; + send: (msg: WorkerMessage) => void; + /** + * Claim the single terminal-outcome slot. Returns `true` for the first caller + * (which may then send its `complete`/`error`) and `false` for every caller + * after — so a SIGTERM cancellation and a near-simultaneous completion can't + * both report a terminal outcome (#2264 P3). See {@link createTerminalClaim}. + */ + claimTerminal: () => boolean; +} + +/** + * Run the analysis and report the outcome to the parent over IPC. Reports at most + * one terminal message (`complete` or `error`) — and none if a cancellation + * already claimed the terminal slot — and never throws; the caller schedules + * `process.exit` after this resolves. + */ +export async function runWorkerAnalysis( + repoPath: string, + options: AnalyzeOptions, + deps: WorkerAnalysisDeps, +): Promise { + let terminal: WorkerMessage; + try { + const result = await deps.runFullAnalysis( + repoPath, + // This worker force-exits right after reporting, so skip the native close + // (it can double-free in LadybugDB's ClientContext destructor after --pdg + // writes); flushWAL still persists the index, process.exit reclaims handles. + { ...options, skipNativeCloseOnExit: true }, + { + onProgress: (phase, percent, message) => + deps.send({ type: 'progress', phase, percent, message }), + onLog: (message) => deps.send({ type: 'progress', phase: 'log', percent: -1, message }), + }, + ); + // P2 (#2264): a half-finalized repo — meta.json written but the global + // registry entry missing (e.g. a prior collision-aborted run, or a wiped + // registry) — must NOT be reported as a successful analysis. Mirror the CLI's + // assertAnalysisFinalized guard so the worker surfaces it as an error instead + // of a false `complete` that leaves the repo invisible to list_repos. + await deps.assertAnalysisFinalized(repoPath); + + // Send a JSON-safe projection, NOT the raw result: the IPC channel is + // default-JSON serialization and `result.pipelineResult` carries the live + // KnowledgeGraph. See analyze-worker-ipc.ts. + terminal = { type: 'complete', result: projectAnalyzeResultForIpc(result) }; + } catch (err: unknown) { + // Report the failure to the parent over IPC (the parent surfaces the message). + const message = err instanceof Error ? err.message : 'Analysis failed'; + terminal = { type: 'error', message }; + } + + // P3 (#2264): only report if a SIGTERM cancellation hasn't already claimed the + // terminal slot — otherwise a cancel near the finish line would report the + // analysis as `complete` over the top of the cancellation. + if (deps.claimTerminal()) deps.send(terminal); +} + +/** + * Create the single-use terminal-outcome claim shared by the worker's message + * handler and its SIGTERM handler. The first call returns `true`; every later + * call returns `false`. This is the coordination point that prevents a cancel and + * a completion from both reporting a terminal status (#2264 P3). Single-threaded + * JS guarantees the check-and-set is atomic (no preemption mid-call). + */ +export function createTerminalClaim(): () => boolean { + let claimed = false; + return () => { + if (claimed) return false; + claimed = true; + return true; + }; +} diff --git a/gitnexus/src/server/analyze-worker.ts b/gitnexus/src/server/analyze-worker.ts index d14fb0d39..f8d583e71 100644 --- a/gitnexus/src/server/analyze-worker.ts +++ b/gitnexus/src/server/analyze-worker.ts @@ -12,8 +12,10 @@ */ import { runFullAnalysis, type AnalyzeOptions } from '../core/run-analyze.js'; -import { projectAnalyzeResultForIpc, type AnalyzeResultIpc } from './analyze-worker-ipc.js'; -import { closeLbug } from '../core/lbug/lbug-adapter.js'; +import { type AnalyzeResultIpc } from './analyze-worker-ipc.js'; +import { runWorkerAnalysis, createTerminalClaim } from './analyze-worker-core.js'; +import { assertAnalysisFinalized } from '../storage/repo-manager.js'; +import { boundedCheckpointBeforeExit } from '../core/lbug/shutdown-helpers.js'; interface StartMessage { type: 'start'; @@ -44,27 +46,62 @@ export interface ErrorMessage { export type WorkerMessage = ProgressMessage | CompleteMessage | ErrorMessage; function send(msg: WorkerMessage) { + // No try/catch: if the IPC channel is gone, process.send throws + // (ERR_IPC_CHANNEL_CLOSED) and that failure must NOT be swallowed. Every caller + // schedules its process.exit inside a `finally`, so a throw here still tears the + // worker down deterministically instead of wedging the event loop (#2264 P3). process.send?.(msg); } -// Catch uncaught exceptions and unhandled rejections — report to parent -process.on('uncaughtException', (err) => { - send({ type: 'error', message: err?.message || 'Uncaught exception in worker' }); - setTimeout(() => process.exit(1), 500); -}); +// Single terminal-outcome slot shared by the message handler and the SIGTERM +// handler: whoever claims it first reports its complete/error; the other skips its +// terminal send, so a cancel near the finish line can't also report success and a +// late SIGTERM can't flip an already-reported job (#2264 P3). +const claimTerminal = createTerminalClaim(); -process.on('unhandledRejection', (reason: any) => { - send({ type: 'error', message: reason?.message || 'Unhandled rejection in worker' }); - setTimeout(() => process.exit(1), 500); -}); - -// Handle graceful shutdown — notify parent before exit -process.on('SIGTERM', async () => { - send({ type: 'error', message: 'Analysis cancelled (worker received SIGTERM)' }); +// Catch uncaught exceptions and unhandled rejections — report them to the parent +// over IPC (the same channel the analysis path uses), then exit. The report runs +// in `try` and the exit in `finally` so a throw from send() on a closed channel +// can't skip the exit and leave the worker wedged (#2264 review P3). +process.on('uncaughtException', (err: unknown) => { try { - await closeLbug(); - } catch {} - process.exit(0); + const message = err instanceof Error ? err.message : 'Uncaught exception in worker'; + send({ type: 'error', message }); + } finally { + setTimeout(() => process.exit(1), 500); + } +}); + +process.on('unhandledRejection', (reason: unknown) => { + try { + const message = reason instanceof Error ? reason.message : 'Unhandled rejection in worker'; + send({ type: 'error', message }); + } finally { + setTimeout(() => process.exit(1), 500); + } +}); + +// Handle cancellation / timeout shutdown (analyze-job.ts `cancelJob` sends +// SIGTERM). Bounded CHECKPOINT-then-exit shared with the CLI SIGINT path (#2264): +// skip the native close (the LadybugDB destructor can double-free after --pdg +// writes), but don't block behind the in-flight COPY's connection lock — so a +// single cancel can't abort or hang the worker. A CHECKPOINT failure is reported +// to the parent over IPC, not swallowed; the exit always fires. +process.on('SIGTERM', () => { + // Only report the cancellation if the analysis hasn't already reported a + // terminal outcome (#2264 P3) — otherwise this would flip an already-complete + // job to failed. The cleanup + exit below run regardless. + if (claimTerminal()) { + send({ type: 'error', message: 'Analysis cancelled (worker received SIGTERM)' }); + } + void boundedCheckpointBeforeExit({ + exitCode: 0, + onFlushError: (err: unknown) => { + const message = + err instanceof Error ? err.message : 'Worker checkpoint failed during SIGTERM'; + send({ type: 'error', message }); + }, + }); }); // Listen for start command from parent — guarded against re-entry @@ -74,26 +111,20 @@ process.on('message', async (msg: StartMessage) => { started = true; try { - const result = await runFullAnalysis(msg.repoPath, msg.options, { - onProgress: (phase, percent, message) => { - send({ type: 'progress', phase, percent, message }); - }, - onLog: (message) => { - send({ type: 'progress', phase: 'log', percent: -1, message }); - }, + // The run → finalize → report contract lives in the side-effect-free + // analyze-worker-core seam (unit-testable without this entry module's + // process.on side effects). It reports exactly one terminal message and + // never throws. + await runWorkerAnalysis(msg.repoPath, msg.options, { + runFullAnalysis, + assertAnalysisFinalized, + send, + claimTerminal, }); - - // Send a JSON-safe projection, NOT the raw result: the IPC channel is - // default-JSON serialization and `result.pipelineResult` carries the live - // KnowledgeGraph (wasteful to materialize, silently corrupted by JSON, and - // a BigInt/circular value would throw and mis-report this success as a - // failure). See analyze-worker-ipc.ts. - send({ type: 'complete', result: projectAnalyzeResultForIpc(result) }); - } catch (err: any) { - send({ type: 'error', message: err?.message || 'Analysis failed' }); + } finally { + // LadybugDB's native module prevents clean exit — force it (same reason the + // CLI uses process.exit(0)). In `finally` so the exit still fires even if the + // report above throws on a closed IPC channel (#2264 review P3). + setTimeout(() => process.exit(0), 500); } - - // LadybugDB's native module prevents clean exit — force it - // (same reason the CLI uses process.exit(0)) - setTimeout(() => process.exit(0), 500); }); diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 2a63d7fb5..8b18b0a17 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -63,6 +63,15 @@ export const canonicalizePath = (p: string): string => { } }; +/** + * Compare two already-canonicalised registry paths. Case-insensitive on Windows + * (its filesystem is), case-sensitive elsewhere. Both arguments must already be + * run through {@link canonicalizePath}; this is the single comparison the registry + * lookups/dedup/finalize checks all share so they answer identically. + */ +export const registryPathEquals = (a: string, b: string): boolean => + process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + export interface RepoMeta { repoPath: string; lastCommit: string; @@ -709,7 +718,7 @@ export const registerRepo = async ( // to a stable key instead of throwing. const a = canonicalizePath(e.path); const b = canonicalInput; - return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + return registryPathEquals(a, b); }); const existing = existingIdx >= 0 ? entries[existingIdx] : null; @@ -817,9 +826,7 @@ export const registerRepo = async ( const fresh = await readRegistry(); const freshIdx = fresh.findIndex((e) => { const a = canonicalizePath(e.path); - return process.platform === 'win32' - ? a.toLowerCase() === canonicalInput.toLowerCase() - : a === canonicalInput; + return registryPathEquals(a, canonicalInput); }); const freshExisting = freshIdx >= 0 ? fresh[freshIdx] : null; let merged: RegistryEntry; @@ -858,9 +865,7 @@ export const unregisterRepo = async (repoPath: string): Promise => { // `resolveRegistryEntry` post-#1003 review. const resolved = canonicalizePath(repoPath); const entries = await readRegistry(); - const matches = (a: string, b: string) => - process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; - const filtered = entries.filter((e) => !matches(canonicalizePath(e.path), resolved)); + const filtered = entries.filter((e) => !registryPathEquals(canonicalizePath(e.path), resolved)); await writeRegistry(filtered); }; @@ -874,10 +879,8 @@ export const unregisterRepo = async (repoPath: string): Promise => { */ export const removeBranchIndex = async (repoPath: string, branch: string): Promise => { const resolved = canonicalizePath(repoPath); - const matches = (a: string, b: string) => - process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; const entries = await readRegistry(); - const idx = entries.findIndex((e) => matches(canonicalizePath(e.path), resolved)); + const idx = entries.findIndex((e) => registryPathEquals(canonicalizePath(e.path), resolved)); if (idx < 0) return false; const entry = entries[idx]; const before = entry.branches?.length ?? 0; @@ -978,6 +981,18 @@ export class AnalysisNotFinalizedError extends Error { } } +/** + * True when the global registry already contains an entry whose canonical path + * matches `repoPath`. Uses the same canonical, case-folded (Windows) comparison + * as {@link assertAnalysisFinalized} so "is it registered?" answers identically + * at the analyze fast-path gate and at the finalize assertion. Pure read. + */ +export const isRepoRegistered = async (repoPath: string): Promise => { + const entries = await readRegistry(); + const canonicalInput = canonicalizePath(path.resolve(repoPath)); + return entries.some((e) => registryPathEquals(canonicalizePath(e.path), canonicalInput)); +}; + /** * Verify that a successful `analyze` call actually produced an indexed, * registered repo on disk. Two checks, both strictly required: @@ -1002,14 +1017,7 @@ export const assertAnalysisFinalized = async (repoPath: string): Promise = throw new AnalysisNotFinalizedError(resolved, storagePath, 'meta', getGlobalRegistryPath()); } - const entries = await readRegistry(); - const canonicalInput = canonicalizePath(resolved); - const isWin = process.platform === 'win32'; - const found = entries.some((e) => { - const a = canonicalizePath(e.path); - return isWin ? a.toLowerCase() === canonicalInput.toLowerCase() : a === canonicalInput; - }); - if (!found) { + if (!(await isRepoRegistered(resolved))) { throw new AnalysisNotFinalizedError( resolved, storagePath, @@ -1125,7 +1133,7 @@ export const resolveRegistryEntry = (entries: RegistryEntry[], target: string): const pathMatch = entries.find((e) => { const a = canonicalizePath(e.path); const b = canonicalTarget; - return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + return registryPathEquals(a, b); }); if (pathMatch) return pathMatch; diff --git a/gitnexus/test/integration/lbug-conn-serialization.test.ts b/gitnexus/test/integration/lbug-conn-serialization.test.ts new file mode 100644 index 000000000..57a1ec32e --- /dev/null +++ b/gitnexus/test/integration/lbug-conn-serialization.test.ts @@ -0,0 +1,155 @@ +/** + * Integration tests: every singleton-`conn` helper reachable during the + * WAL-checkpoint-driver window must route through `withConnLock` (PR #2264 + * tri-review, P1). These helpers issued raw `conn.query` on the shared + * connection while the driver could fire a concurrent CHECKPOINT — the same + * native double-free this branch fixes, on the incremental `--pdg` path. + * + * Mirrors `lbug-core-adapter.test.ts`: one isolated temp DB via `withTestLbugDB`. + * `withConnLock` is mocked to a call-through spy so we can assert each helper + * acquires the lock while the real serialization still runs. Routing assertions + * use an empty (initialized) DB — they prove the lock is taken regardless of + * whether any rows match. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { withTestLbugDB } from '../helpers/test-indexed-db.js'; +import { NODE_TABLES } from '../../src/core/lbug/schema.js'; + +// Spy `withConnLock` while preserving its real behavior (call-through). The +// adapter imports this module, so the spy observes every lock acquisition. +vi.mock('../../src/core/lbug/conn-lock.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + withConnLock: vi.fn(actual.withConnLock) as typeof actual.withConnLock, + }; +}); +import { withConnLock } from '../../src/core/lbug/conn-lock.js'; +const lockSpy = vi.mocked(withConnLock); + +// Spy `closeQueryResults` (call-through) to prove the deleteAll* helpers now +// drain/close their DELETE result, not just the count result (P2 #2264). +vi.mock('../../src/core/lbug/query-result-utils.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + closeQueryResults: vi.fn(actual.closeQueryResults) as typeof actual.closeQueryResults, + }; +}); +import { closeQueryResults } from '../../src/core/lbug/query-result-utils.js'; +const closeSpy = vi.mocked(closeQueryResults); + +withTestLbugDB('conn-serialization', () => { + describe('singleton-conn helpers acquire withConnLock (P1 #2264)', () => { + beforeEach(() => { + // Setup (clear/seed/flush) already exercised the lock; reset so each + // assertion reflects only the helper under test. + lockSpy.mockClear(); + }); + + it('U1: deleteAllCommunitiesAndProcesses routes through withConnLock', async () => { + const { deleteAllCommunitiesAndProcesses } = + await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCommunitiesAndProcesses(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ nodesDeleted: 0 }); + }); + + it('U2: queryImporters routes through withConnLock', async () => { + const { queryImporters } = await import('../../src/core/lbug/lbug-adapter.js'); + const importers = await queryImporters('any/path.ts'); + expect(lockSpy).toHaveBeenCalled(); + expect(importers).toEqual([]); + }); + + it('U3: deleteNodesForFile (singleton) locks every per-table count query', async () => { + const { deleteNodesForFile } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteNodesForFile('any/path.ts'); + // One locked count per filePath-bearing node table (Community/Process are + // skipped), proving the count read — not just the already-locked DELETE — + // now serializes. Baseline (count unlocked) would show ~1 lock call. + const filePathTables = NODE_TABLES.filter((t) => t !== 'Community' && t !== 'Process'); + expect(lockSpy.mock.calls.length).toBeGreaterThanOrEqual(filePathTables.length); + expect(result).toMatchObject({ deletedNodes: 0 }); + }); + + it('closeLbugBeforeExit() checkpoints but leaves the connection open (#2264 close-crash)', async () => { + const adapter = await import('../../src/core/lbug/lbug-adapter.js'); + await adapter.closeLbugBeforeExit(); + // The native conn/db are deliberately NOT torn down — that avoids LadybugDB's + // ClientContext destructor double-free after --pdg writes. The connection + // stays ready and queryable (the CHECKPOINT made the index durable; process + // exit reclaims the handles on the CLI path). + expect(adapter.isLbugReady()).toBe(true); + const rows = await adapter.executeQuery('RETURN 1 AS one'); + expect(rows).toHaveLength(1); + }); + + it('U3: loadCachedEmbeddings routes through withConnLock', async () => { + const { loadCachedEmbeddings } = await import('../../src/core/lbug/lbug-adapter.js'); + const cached = await loadCachedEmbeddings(); + expect(lockSpy).toHaveBeenCalled(); + expect(cached.embeddings).toEqual([]); + expect(cached.embeddingNodeIds.size).toBe(0); + }); + + it('U4: deleteAllInterprocTaintPaths routes through withConnLock', async () => { + const { deleteAllInterprocTaintPaths } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllInterprocTaintPaths(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ edgesDeleted: 0 }); + }); + + it('U4: deleteAllCallSummaries routes through withConnLock', async () => { + const { deleteAllCallSummaries } = await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCallSummaries(); + expect(lockSpy).toHaveBeenCalled(); + expect(result).toMatchObject({ edgesDeleted: 0 }); + }); + + it('U5: deleteNodesForFile on a temp dbPath does NOT take the lock (negative gate)', async () => { + // The targetConn === conn gate's negative branch: a per-file/temp connection + // (dbPath provided) must NOT take the singleton lock, so temp-conn callers + // can't contend with the singleton. Mirrors the positive U3 case above so a + // regression that unconditionally locks is caught. (#2264) + const { createTempDir } = await import('../helpers/test-db.js'); + const { deleteNodesForFile } = await import('../../src/core/lbug/lbug-adapter.js'); + const temp = await createTempDir('gn-negative-gate-'); + try { + lockSpy.mockClear(); + const result = await deleteNodesForFile('any/path.ts', temp.dbPath); + expect(lockSpy).not.toHaveBeenCalled(); + expect(result).toMatchObject({ deletedNodes: 0 }); + } finally { + await temp.cleanup(); + } + }); + }); +}); + +withTestLbugDB( + 'conn-serialization-drain', + () => { + describe('deleteAll* drain their DELETE result (P2 #2264)', () => { + beforeEach(() => { + closeSpy.mockClear(); + }); + + it('U4: deleteAllCommunitiesAndProcesses closes the DETACH DELETE result', async () => { + const { deleteAllCommunitiesAndProcesses } = + await import('../../src/core/lbug/lbug-adapter.js'); + const result = await deleteAllCommunitiesAndProcesses(); + // Seed has 1 Community, 0 Process. With the drain fix, closeQueryResults + // fires for: Community count, Community DETACH DELETE, Process count = 3. + // Without the fix the delete result is dropped → only 2 closes. + expect(result).toMatchObject({ nodesDeleted: 1 }); + expect(closeSpy.mock.calls.length).toBeGreaterThanOrEqual(3); + }); + }); + }, + { + seed: [ + "CREATE (c:Community {id: 'comm:drain', label: 'Drain', heuristicLabel: 'Drain', keywords: ['x'], description: 'd', enrichedBy: 'heuristic', cohesion: 0.5, symbolCount: 1})", + ], + }, +); diff --git a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts b/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts index 69c51e34c..ee1a8b22c 100644 --- a/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts +++ b/gitnexus/test/unit/analyze-embedding-endpoint-flags.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-embeddings-limit.test.ts b/gitnexus/test/unit/analyze-embeddings-limit.test.ts index 6fbc5af56..c7f12fe7f 100644 --- a/gitnexus/test/unit/analyze-embeddings-limit.test.ts +++ b/gitnexus/test/unit/analyze-embeddings-limit.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts b/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts new file mode 100644 index 000000000..c87008726 --- /dev/null +++ b/gitnexus/test/unit/analyze-finalize-failure-exits.test.ts @@ -0,0 +1,180 @@ +/** + * Regression test for the #2264 review P1: when a full analyze succeeds (which + * skip-closes LadybugDB, leaving native handles open) and a post-finalize step + * THEN throws, the CLI's outer catch soft-returns (`process.exitCode = 1`). With + * native handles open, the event loop can't drain — the process would HANG. The + * `analyzeCommand` wrapper now force-exits when `isLbugReady()` is true after the + * soft return. This test drives that exact path and asserts termination. + * + * Test-safety: when `isLbugReady()` is false (the default in every analyze unit + * test that mocks run-analyze — the DB is never opened), the wrapper must NOT + * force-exit, preserving the soft return those tests rely on. + * + * Worker-safety (#2264 CI): the module is imported ONCE and the mocks are driven + * per-test via `mockReturnValue`. The earlier `vi.resetModules()` + per-test + * `await import('analyze.js')` re-instrumented the ENTIRE analyze module graph on + * every test; under `--coverage` on the memory-constrained CI runner that + * OOM/crashed the forked worker ("Worker exited unexpectedly"), even though it + * passed locally. `analyzeCommand` also installs global fatal handlers + * (installFatalHandlers) that call the REAL process.exit(1); we keep process.exit + * spied for the whole file so one firing can't kill the worker, strip the handlers + * it added in afterAll, and reset process.exitCode so the worker exits clean. + */ +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + describe, + expect, + it, + vi, + type MockInstance, +} from 'vitest'; + +const { + runFullAnalysisMock, + assertAnalysisFinalizedMock, + isLbugReadyMock, + AnalysisNotFinalizedError, +} = vi.hoisted(() => { + class AnalysisNotFinalizedError extends Error { + storagePath = '.gitnexus'; + } + return { + runFullAnalysisMock: vi.fn(), + assertAnalysisFinalizedMock: vi.fn(), + isLbugReadyMock: vi.fn(() => false), + AnalysisNotFinalizedError, + }; +}); + +vi.mock('../../src/core/run-analyze.js', () => ({ runFullAnalysis: runFullAnalysisMock })); +vi.mock('../../src/cli/ai-context.js', () => ({ + generateAIContextFiles: vi.fn(async () => ({ files: [] as string[] })), + refreshBaseRefLine: vi.fn(async () => ({ files: [] as string[] })), +})); +vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: vi.fn() })); +vi.mock('../../src/cli/cli-message.js', () => ({ cliError: vi.fn() })); +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: isLbugReadyMock, +})); +vi.mock('../../src/storage/repo-manager.js', () => ({ + getStoragePaths: vi.fn(() => ({ storagePath: '.gitnexus', lbugPath: '.gitnexus/lbug' })), + getGlobalRegistryPath: vi.fn(() => 'registry.json'), + RegistryNameCollisionError: class RegistryNameCollisionError extends Error {}, + AnalysisNotFinalizedError, + assertAnalysisFinalized: assertAnalysisFinalizedMock, +})); +vi.mock('../../src/storage/git.js', () => ({ + getGitRoot: vi.fn(() => '/repo'), + hasGitDir: vi.fn(() => true), + getDefaultBranch: vi.fn(() => null), +})); +vi.mock('../../src/core/ingestion/utils/max-file-size.js', () => ({ + getMaxFileSizeBannerMessage: vi.fn(() => null), +})); + +// Imported ONCE (not re-imported per test) — see the worker-safety note above. +import { analyzeCommand } from '../../src/cli/analyze.js'; + +describe('analyzeCommand — finalize-failure must terminate, not hang (#2264 P1)', () => { + // Snapshot the fatal-handler listeners present BEFORE this file ran (vitest's + // own) so afterAll strips only the ones installFatalHandlers added. + const baselineUnhandled = process.listeners('unhandledRejection'); + const baselineUncaught = process.listeners('uncaughtException'); + let exitSpy: MockInstance; + let savedNodeOptions: string | undefined; + + beforeAll(() => { + // analyzeCommand calls ensureHeap(), which RE-EXECS the process — spawning + // `node ` where argv is vitest's, killing the forked + // worker — UNLESS NODE_OPTIONS already carries a heap cap (analyze.ts:498). + // Locally a high V8 heap-size-limit also short-circuits it (analyze.ts:501), + // which is why this only crashed on the memory-constrained CI runner. Pre-set + // the cap so ensureHeap returns early — the same workaround cli-e2e uses + // (#2264 CI). Restored in afterAll so a reused worker's later files are clean. + savedNodeOptions = process.env.NODE_OPTIONS; + process.env.NODE_OPTIONS = `${process.env.NODE_OPTIONS ?? ''} --max-old-space-size=8192`.trim(); + // Mock process.exit for the WHOLE file — a fatal handler firing between tests + // (after a per-test spy would have been restored) can't really exit. + exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never); + }); + + afterAll(() => { + // Strip the handlers installFatalHandlers added BEFORE restoring the real + // process.exit, so no stray rejection during teardown fires a real exit. Only + // remove non-baseline (vitest's own) listeners. + process + .listeners('unhandledRejection') + .filter((l) => !baselineUnhandled.includes(l)) + .forEach((l) => process.removeListener('unhandledRejection', l)); + process + .listeners('uncaughtException') + .filter((l) => !baselineUncaught.includes(l)) + .forEach((l) => process.removeListener('uncaughtException', l)); + exitSpy.mockRestore(); + process.env.NODE_OPTIONS = savedNodeOptions ?? ''; + process.exitCode = 0; + }); + + beforeEach(() => { + exitSpy.mockClear(); + runFullAnalysisMock.mockReset(); + // Full analysis succeeded (NOT the alreadyUpToDate fast path) → skip-closed. + runFullAnalysisMock.mockResolvedValue({ + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: false, + ftsRepairedOnly: false, + pipelineResult: { communityResult: undefined }, + }); + assertAnalysisFinalizedMock.mockReset(); + // Post-finalize check throws (the documented silent-finalize state). + assertAnalysisFinalizedMock.mockRejectedValue(new AnalysisNotFinalizedError('not finalized')); + isLbugReadyMock.mockReset(); + process.exitCode = undefined; + }); + + afterEach(() => { + // Don't leak a non-zero exit code to the forked worker's natural exit. + process.exitCode = 0; + }); + + it('force-exits when native handles are still open (isLbugReady true)', async () => { + isLbugReadyMock.mockReturnValue(true); + await analyzeCommand(undefined, {}); + expect(exitSpy).toHaveBeenCalledWith(1); + }); + + it('does NOT force-exit when no handles are open (isLbugReady false) — soft return preserved', async () => { + isLbugReadyMock.mockReturnValue(false); + await analyzeCommand(undefined, {}); + expect(exitSpy).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + }); + + it('forwards a pre-set process.exitCode rather than the hardcoded fallback', async () => { + // The alreadyUpToDate path returns WITHOUT setting process.exitCode or calling + // process.exit (unlike the error catch, which always sets exitCode=1), so the + // wrapper's force-exit must forward whatever exitCode is already set — proving + // `process.exit(process.exitCode ?? 1)` reads exitCode and doesn't hardcode 1. + // isLbugReady is forced true to drive the wrapper's force-exit on this path. + isLbugReadyMock.mockReturnValue(true); + runFullAnalysisMock.mockResolvedValue({ + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: true, + ftsRepairedOnly: false, + pipelineResult: { communityResult: undefined }, + }); + assertAnalysisFinalizedMock.mockResolvedValue(undefined); + process.exitCode = 2; + await analyzeCommand(undefined, {}); + expect(exitSpy).toHaveBeenCalledWith(2); + }); +}); diff --git a/gitnexus/test/unit/analyze-gitnexusrc.test.ts b/gitnexus/test/unit/analyze-gitnexusrc.test.ts index cd299f870..fa1b3f9fb 100644 --- a/gitnexus/test/unit/analyze-gitnexusrc.test.ts +++ b/gitnexus/test/unit/analyze-gitnexusrc.test.ts @@ -39,7 +39,11 @@ vi.mock('../../src/cli/ai-context.js', () => ({ })); vi.mock('../../src/cli/skill-gen.js', () => ({ generateSkillFiles: generateSkillFilesMock })); vi.mock('../../src/cli/cli-message.js', () => ({ cliError: cliErrorMock })); -vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined) })); +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), +})); vi.mock('../../src/storage/repo-manager.js', () => ({ getStoragePaths: vi.fn((repoPath: string) => ({ diff --git a/gitnexus/test/unit/analyze-heap-respawn.test.ts b/gitnexus/test/unit/analyze-heap-respawn.test.ts index 7d98ddbf4..e384e460c 100644 --- a/gitnexus/test/unit/analyze-heap-respawn.test.ts +++ b/gitnexus/test/unit/analyze-heap-respawn.test.ts @@ -25,6 +25,8 @@ vi.mock('os', async () => { vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); const mockSpawnExit = ({ diff --git a/gitnexus/test/unit/analyze-job.test.ts b/gitnexus/test/unit/analyze-job.test.ts index 1fca6d5ce..4f40d93b9 100644 --- a/gitnexus/test/unit/analyze-job.test.ts +++ b/gitnexus/test/unit/analyze-job.test.ts @@ -128,4 +128,36 @@ describe('JobManager', () => { it('cancelJob returns false for unknown job', () => { expect(manager.cancelJob('nonexistent')).toBe(false); }); + + // #2264 P3: a job's terminal outcome is immutable, so a late worker message (a + // SIGTERM-driven `error` after `complete`, or vice versa) cannot flip it. + describe('terminal-state immutability (#2264 P3)', () => { + it('keeps complete when a later failed update arrives', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + manager.updateJob(job.id, { status: 'analyzing' }); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' }); + expect(manager.getJob(job.id)!.status).toBe('complete'); + }); + + it('keeps failed when a later complete update arrives', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + manager.updateJob(job.id, { status: 'analyzing' }); + manager.updateJob(job.id, { status: 'failed', error: 'Analysis cancelled' }); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + const after = manager.getJob(job.id)!; + expect(after.status).toBe('failed'); + expect(after.error).toBe('Analysis cancelled'); + }); + + it('emits no further event for a post-terminal update', () => { + const job = manager.createJob({ repoUrl: 'https://github.com/user/repo' }); + const events: Array<{ phase: string }> = []; + manager.onProgress(job.id, (data) => events.push(data)); + manager.updateJob(job.id, { status: 'complete', repoName: 'repo' }); + manager.updateJob(job.id, { status: 'failed', error: 'late' }); + expect(events).toHaveLength(1); + expect(events[0].phase).toBe('complete'); + }); + }); }); diff --git a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts b/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts index de55d4e71..5e7f5190c 100644 --- a/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts +++ b/gitnexus/test/unit/analyze-lbug-checkpoint-threshold.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-local-embedding-error.test.ts b/gitnexus/test/unit/analyze-local-embedding-error.test.ts index 0b5e5de48..c90896be2 100644 --- a/gitnexus/test/unit/analyze-local-embedding-error.test.ts +++ b/gitnexus/test/unit/analyze-local-embedding-error.test.ts @@ -27,6 +27,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts b/gitnexus/test/unit/analyze-no-stats-bridge.test.ts index 629bfac02..e08098ff6 100644 --- a/gitnexus/test/unit/analyze-no-stats-bridge.test.ts +++ b/gitnexus/test/unit/analyze-no-stats-bridge.test.ts @@ -35,6 +35,8 @@ vi.mock('../../src/cli/cli-message.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts b/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts index de90ae467..a5bfb16ee 100644 --- a/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts +++ b/gitnexus/test/unit/analyze-respawn-progress-terminal.test.ts @@ -40,6 +40,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-wal-error.test.ts b/gitnexus/test/unit/analyze-wal-error.test.ts index 5264dfd3a..0cc768043 100644 --- a/gitnexus/test/unit/analyze-wal-error.test.ts +++ b/gitnexus/test/unit/analyze-wal-error.test.ts @@ -20,6 +20,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-worker-core.test.ts b/gitnexus/test/unit/analyze-worker-core.test.ts new file mode 100644 index 000000000..a5a3f20d9 --- /dev/null +++ b/gitnexus/test/unit/analyze-worker-core.test.ts @@ -0,0 +1,143 @@ +/** + * Unit tests for the analyze-worker core seam (#2264). + * + * P2: the worker must NOT report `complete` for a half-finalized repo (meta.json + * written but the global registry entry missing) — it must surface that as an + * error, mirroring the CLI's assertAnalysisFinalized guard. + * + * P3: a SIGTERM cancellation and a near-simultaneous completion must not both + * report a terminal outcome — the `claimTerminal` slot coordinates them. + * + * Driven via the side-effect-free `runWorkerAnalysis` seam with injected fakes, so + * no fork()/process.on side effects of the entry module are touched. + */ +import { describe, it, expect, vi } from 'vitest'; +import { + runWorkerAnalysis, + createTerminalClaim, + type WorkerAnalysisDeps, +} from '../../src/server/analyze-worker-core.js'; +import type { AnalyzeResult } from '../../src/core/run-analyze.js'; +import type { WorkerMessage } from '../../src/server/analyze-worker.js'; + +const baseResult: AnalyzeResult = { + repoName: 'repo', + repoPath: '/repo', + stats: {}, + alreadyUpToDate: false, + ftsRepairedOnly: false, +}; + +const okRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => baseResult); +const okFinalize: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn(async () => undefined); +const alwaysClaim: WorkerAnalysisDeps['claimTerminal'] = () => true; + +describe('runWorkerAnalysis — finalize guard (#2264 P2)', () => { + it('reports error (not complete) when finalization fails for an unregistered repo', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const assertAnalysisFinalized: WorkerAnalysisDeps['assertAnalysisFinalized'] = vi.fn( + async () => { + throw new Error('registry entry for /repo was not added'); + }, + ); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized, + send, + claimTerminal: alwaysClaim, + }, + ); + + expect(send).toHaveBeenCalledWith({ + type: 'error', + message: 'registry entry for /repo was not added', + }); + expect(send).not.toHaveBeenCalledWith(expect.objectContaining({ type: 'complete' })); + }); + + it('reports complete exactly once when finalization succeeds', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized: okFinalize, + send, + claimTerminal: alwaysClaim, + }, + ); + + const completes = send.mock.calls.filter((c) => c[0].type === 'complete'); + expect(completes).toHaveLength(1); + }); + + it('reports error when finalization passes but the analysis itself throws', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const failingRun: WorkerAnalysisDeps['runFullAnalysis'] = vi.fn(async () => { + throw new Error('boom'); + }); + // Fresh local mock (not the shared okFinalize) so the "never called" assertion + // reflects only this test. + const finalize = vi.fn(async () => undefined); + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: failingRun, + assertAnalysisFinalized: finalize, + send, + claimTerminal: alwaysClaim, + }, + ); + + expect(send).toHaveBeenCalledWith({ type: 'error', message: 'boom' }); + expect(finalize).not.toHaveBeenCalled(); + }); +}); + +describe('runWorkerAnalysis — terminal-claim coordination (#2264 P3)', () => { + it('sends NO terminal message when the slot is already claimed (cancellation won)', async () => { + const send = vi.fn<(msg: WorkerMessage) => void>(); + const alreadyClaimed: WorkerAnalysisDeps['claimTerminal'] = () => false; + + await runWorkerAnalysis( + '/repo', + {}, + { + runFullAnalysis: okRun, + assertAnalysisFinalized: okFinalize, + send, + claimTerminal: alreadyClaimed, + }, + ); + + const terminals = send.mock.calls.filter( + (c) => c[0].type === 'complete' || c[0].type === 'error', + ); + expect(terminals).toHaveLength(0); + }); +}); + +describe('createTerminalClaim (#2264 P3)', () => { + it('returns true for the first claim and false for every claim after', () => { + const claim = createTerminalClaim(); + expect(claim()).toBe(true); + expect(claim()).toBe(false); + expect(claim()).toBe(false); + }); + + it('gives independent claims separate slots', () => { + const a = createTerminalClaim(); + const b = createTerminalClaim(); + expect(a()).toBe(true); + expect(b()).toBe(true); + expect(a()).toBe(false); + }); +}); diff --git a/gitnexus/test/unit/analyze-worker-pool-size.test.ts b/gitnexus/test/unit/analyze-worker-pool-size.test.ts index 2c7a8bd3f..8af0f213f 100644 --- a/gitnexus/test/unit/analyze-worker-pool-size.test.ts +++ b/gitnexus/test/unit/analyze-worker-pool-size.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/analyze-worker-timeout.test.ts b/gitnexus/test/unit/analyze-worker-timeout.test.ts index aebe587e9..8a2dc04ef 100644 --- a/gitnexus/test/unit/analyze-worker-timeout.test.ts +++ b/gitnexus/test/unit/analyze-worker-timeout.test.ts @@ -8,6 +8,8 @@ vi.mock('../../src/core/run-analyze.js', () => ({ vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ closeLbug: vi.fn(async () => undefined), + closeLbugBeforeExit: vi.fn(async () => undefined), + isLbugReady: vi.fn(() => false), })); vi.mock('../../src/storage/repo-manager.js', () => ({ diff --git a/gitnexus/test/unit/conn-lock.test.ts b/gitnexus/test/unit/conn-lock.test.ts new file mode 100644 index 000000000..f3d2e588b --- /dev/null +++ b/gitnexus/test/unit/conn-lock.test.ts @@ -0,0 +1,109 @@ +/** + * Unit tests for the LadybugDB connection serialization lock (conn-lock.ts). + * + * This lock is the fix for the `analyze --pdg` native crash: the WAL-checkpoint + * driver's periodic CHECKPOINT was executing on the shared singleton connection + * concurrently with a long-running COPY, and LadybugDB's single-writer + * Connection corrupts native heap state under concurrent query execution + * (`double free or corruption (out)` / SIGSEGV). These tests assert the + * lock's one-at-a-time guarantee deterministically, with no native engine — + * the property that makes the otherwise-crashing overlap safe. + */ +import { afterEach, describe, expect, it } from 'vitest'; +import { withConnLock, _resetConnLockForTests } from '../../src/core/lbug/conn-lock.js'; + +afterEach(() => { + _resetConnLockForTests(); +}); + +describe('withConnLock — connection serialization', () => { + it('runs critical sections one at a time in FIFO order with no interleave', async () => { + const events: string[] = []; + const section = (id: string, yields: number) => async (): Promise => { + events.push(`${id}:enter`); + // Yield to the microtask queue repeatedly. Without serialization a later + // section's `enter` would slip in between these yields. + for (let i = 0; i < yields; i++) await Promise.resolve(); + events.push(`${id}:exit`); + }; + + // B and C are launched while A (which yields the most) is mid-flight. + await Promise.all([ + withConnLock(section('A', 5)), + withConnLock(section('B', 0)), + withConnLock(section('C', 0)), + ]); + + expect(events).toEqual(['A:enter', 'A:exit', 'B:enter', 'B:exit', 'C:enter', 'C:exit']); + }); + + it('never lets two critical sections overlap under heavy concurrency', async () => { + let active = 0; + const observedMax: number[] = []; + const op = () => async (): Promise => { + active++; + observedMax.push(active); + await Promise.resolve(); + await Promise.resolve(); + active--; + }; + + await Promise.all(Array.from({ length: 25 }, () => withConnLock(op()))); + + // The concurrency count observed at the top of every critical section was + // always exactly 1 — i.e. no two ran at once. This is precisely what stops + // the checkpoint driver from racing a COPY on the native connection. + expect(Math.max(...observedMax)).toBe(1); + }); + + it('releases the lock when a critical section throws (no permanent wedge)', async () => { + await expect( + withConnLock(async () => { + throw new Error('boom'); + }), + ).rejects.toThrow('boom'); + + // A failed op must not strand the lock — the next caller still acquires it. + await expect(withConnLock(async () => 'recovered')).resolves.toBe('recovered'); + }); + + it('returns the wrapped operation result', async () => { + await expect(withConnLock(async () => 42)).resolves.toBe(42); + }); +}); + +describe('withConnLock — re-entry guard', () => { + it('throws on a nested (wrapped-in-wrapped) call instead of deadlocking', async () => { + await expect(withConnLock(async () => withConnLock(async () => 'inner'))).rejects.toThrow( + /re-entry/, + ); + }); + + it('does NOT false-fire on sequential (non-nested) calls', async () => { + // Mirrors getLbugStats: many withConnLock calls in a loop, each awaited to + // completion before the next — distinct async contexts, never nested. + const results: number[] = []; + for (let i = 0; i < 5; i++) { + results.push(await withConnLock(async () => i)); + } + expect(results).toEqual([0, 1, 2, 3, 4]); + }); + + it('does NOT false-fire on concurrent top-level (queued) callers', async () => { + // Legitimate contention: B and C call while A holds the lock. They are + // separate async contexts (not nested in A's fn), so they queue, not throw. + const out = await Promise.all([ + withConnLock(async () => 'a'), + withConnLock(async () => 'b'), + withConnLock(async () => 'c'), + ]); + expect(out).toEqual(['a', 'b', 'c']); + }); + + it('releases the lock after a re-entry throw so later callers proceed', async () => { + await expect(withConnLock(async () => withConnLock(async () => 'inner'))).rejects.toThrow( + /re-entry/, + ); + await expect(withConnLock(async () => 'ok')).resolves.toBe('ok'); + }); +}); diff --git a/gitnexus/test/unit/lbug-checkpoint.test.ts b/gitnexus/test/unit/lbug-checkpoint.test.ts index 6eac7e73a..f898030e2 100644 --- a/gitnexus/test/unit/lbug-checkpoint.test.ts +++ b/gitnexus/test/unit/lbug-checkpoint.test.ts @@ -23,10 +23,20 @@ import { flushWAL } from '../../src/core/lbug/lbug-adapter.js'; describe('flushWAL / safeClose — consolidation guard (#1376)', () => { let adapterSource: string; + // Strip comments before the structural assertions so they reflect CODE only. + // Otherwise a `conn.close()` / `db.close()` / `.query('CHECKPOINT')` token + // mentioned in a doc comment would falsely trip (or vacuously satisfy) a guard, + // coupling the test to comment wording — exactly the brittleness flagged in the + // #2264 review (a prior commit had to reword a comment just to keep this green). + const codeOnly = (src: string): string => + src.replace(/\/\*[\s\S]*?\*\//g, '').replace(/\/\/[^\n]*/g, ''); + beforeAll(async () => { - adapterSource = await fs.readFile( - path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), - 'utf-8', + adapterSource = codeOnly( + await fs.readFile( + path.join(__dirname, '..', '..', 'src', 'core', 'lbug', 'lbug-adapter.ts'), + 'utf-8', + ), ); }); @@ -44,7 +54,8 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => { }); it('closeLbug delegates to safeClose instead of inlining conn.close/db.close', () => { - const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug')); + // Match `closeLbug =` precisely so we don't prefix-match `closeLbugBeforeExit`. + const closeLbugBody = adapterSource.slice(adapterSource.indexOf('export const closeLbug =')); expect(closeLbugBody).toMatch(/await safeClose\(\)/); // closeLbug must NOT contain its own conn.close() or db.close() — those // live exclusively inside safeClose now. @@ -53,8 +64,26 @@ describe('flushWAL / safeClose — consolidation guard (#1376)', () => { expect(closeLbugBlock).not.toMatch(/db\.close\(\)/); }); + it('exports closeLbugBeforeExit (CHECKPOINT-only, skips native close) (#2264)', () => { + expect(adapterSource).toMatch(/export const closeLbugBeforeExit/); + // closeLbugBeforeExit is declared immediately before closeLbug; its body must + // CHECKPOINT via flushWAL and NEVER do a native conn/db close (that's the + // whole point — it relies on a guaranteed process.exit). + const body = adapterSource.slice( + adapterSource.indexOf('export const closeLbugBeforeExit'), + adapterSource.indexOf('export const closeLbug ='), + ); + expect(body).toMatch(/await flushWAL\(\)/); + expect(body).not.toMatch(/conn\.close\(\)/); + expect(body).not.toMatch(/db\.close\(\)/); + }); + it('CHECKPOINT is issued only by flushWAL (best-effort) and tryFlushWAL (rethrows for the retry driver)', () => { - const matches = adapterSource.match(/conn\.query\('CHECKPOINT'\)/g) ?? []; + // Receiver-agnostic: since the connection-serialization refactor (#2264) + // both sites capture `const c = conn` and call `c.query('CHECKPOINT')` + // inside withConnLock, so match `.query('CHECKPOINT')` regardless of the + // receiver name rather than the literal `conn.query(...)`. + const matches = adapterSource.match(/\.query\('CHECKPOINT'\)/g) ?? []; // Two authorized sites: `flushWAL` (swallows errors — used by // `safeClose` and the server's best-effort flush) and `tryFlushWAL` // (rethrows so the manual checkpoint driver in `wal-checkpoint-driver.ts` diff --git a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts index c4472a6de..d0e217365 100644 --- a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts +++ b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts @@ -18,6 +18,7 @@ import fs from 'fs/promises'; import { AnalysisNotFinalizedError, assertAnalysisFinalized, + isRepoRegistered, registerRepo, saveMeta, getStoragePaths, @@ -128,4 +129,17 @@ describe('assertAnalysisFinalized (#1169)', () => { const variant = process.platform === 'win32' ? tmpRepo.dbPath.toUpperCase() : tmpRepo.dbPath; // POSIX is case-sensitive; assertion uses canonical form await expect(assertAnalysisFinalized(variant)).resolves.toBeUndefined(); }); + + // isRepoRegistered backs the analyze up-to-date fast-path gate (#2264): the + // fast path must NOT short-circuit a repo that is indexed-but-unregistered + // (e.g. a prior --name collision wrote meta.json then failed before + // registerRepo), otherwise --allow-duplicate-name could never heal it. + it('isRepoRegistered is false when the repo has no registry entry', async () => { + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(false); + }); + + it('isRepoRegistered is true once a matching entry is written', async () => { + await registerRepo(tmpRepo.dbPath, meta); + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(true); + }); }); diff --git a/gitnexus/test/unit/shutdown-helpers.test.ts b/gitnexus/test/unit/shutdown-helpers.test.ts new file mode 100644 index 000000000..e4606383c --- /dev/null +++ b/gitnexus/test/unit/shutdown-helpers.test.ts @@ -0,0 +1,62 @@ +/** + * Unit tests for the shared bounded checkpoint-then-exit cleanup (#2264) used by + * the CLI SIGINT handler and the worker SIGTERM handler. The checkpoint + exit are + * injected so the helper is testable without touching the real LadybugDB close or + * the real process.exit. + */ +import { describe, it, expect, vi } from 'vitest'; +import { boundedCheckpointBeforeExit } from '../../src/core/lbug/shutdown-helpers.js'; + +describe('boundedCheckpointBeforeExit (#2264)', () => { + it('checkpoints, runs beforeExit, then exits with the given code (in order)', async () => { + const order: string[] = []; + const exit = vi.fn<(code: number) => void>((c) => { + order.push(`exit:${c}`); + }); + + await boundedCheckpointBeforeExit({ + exitCode: 130, + checkpoint: vi.fn(async () => { + order.push('checkpoint'); + }), + beforeExit: () => { + order.push('beforeExit'); + }, + exit, + }); + + expect(exit).toHaveBeenCalledWith(130); + expect(order).toEqual(['checkpoint', 'beforeExit', 'exit:130']); + }); + + it('reports a checkpoint failure via onFlushError and still exits', async () => { + const onFlushError = vi.fn<(err: unknown) => void>(); + const exit = vi.fn<(code: number) => void>(); + const err = new Error('checkpoint boom'); + + await boundedCheckpointBeforeExit({ + exitCode: 0, + checkpoint: vi.fn(async () => { + throw err; + }), + onFlushError, + exit, + }); + + expect(onFlushError).toHaveBeenCalledWith(err); + expect(exit).toHaveBeenCalledWith(0); + }); + + it('exits via the timeout when the checkpoint hangs', async () => { + const exit = vi.fn<(code: number) => void>(); + + await boundedCheckpointBeforeExit({ + exitCode: 0, + timeoutMs: 0, + checkpoint: () => new Promise(() => {}), // never resolves + exit, + }); + + expect(exit).toHaveBeenCalledWith(0); + }); +}); diff --git a/gitnexus/test/unit/stream-query-driver-guard.test.ts b/gitnexus/test/unit/stream-query-driver-guard.test.ts new file mode 100644 index 000000000..503c23a11 --- /dev/null +++ b/gitnexus/test/unit/stream-query-driver-guard.test.ts @@ -0,0 +1,53 @@ +/** + * Unit tests for the streamQuery WAL-driver guard (#2264). streamQuery is + * deliberately NOT wrapped in withConnLock (its per-row callback re-enters the + * adapter), so it must refuse to run while the WAL-checkpoint driver is live — + * otherwise its unlocked per-row reads could race a CHECKPOINT on the shared + * connection (the corruption window the lock serializes everything else against). + * Today the serve/read path never starts the driver; this guard fails loud if a + * future in-process analyze ever overlaps a stream. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { streamQuery } from '../../src/core/lbug/lbug-adapter.js'; +import { markWalDriverActive } from '../../src/core/lbug/wal-driver-state.js'; +import { startWalCheckpointDriver } from '../../src/core/lbug/wal-checkpoint-driver.js'; + +describe('streamQuery WAL-driver guard (#2264)', () => { + beforeEach(() => { + // Manual checkpoint defaults on; pin it so the driver path is deterministic. + vi.stubEnv('GITNEXUS_WAL_MANUAL_CHECKPOINT', '1'); + }); + + afterEach(() => { + markWalDriverActive(false); + vi.unstubAllEnvs(); + }); + + it('throws when the WAL-checkpoint driver is active', async () => { + markWalDriverActive(true); + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /WAL-checkpoint driver is active/, + ); + }); + + it('passes the guard when inactive (reaching the not-initialized check)', async () => { + markWalDriverActive(false); + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /not initialized/, + ); + }); + + it('startWalCheckpointDriver arms the guard; stop() disarms it', async () => { + const driver = startWalCheckpointDriver({ periodMs: 1_000_000 }); + try { + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /WAL-checkpoint driver is active/, + ); + } finally { + await driver.stop(); + } + await expect(streamQuery('RETURN 1 AS one', () => undefined)).rejects.toThrow( + /not initialized/, + ); + }); +}); diff --git a/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts b/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts new file mode 100644 index 000000000..289a5a23b --- /dev/null +++ b/gitnexus/test/unit/wal-checkpoint-driver-reentrancy.test.ts @@ -0,0 +1,66 @@ +/** + * Reentrancy-guard test for the manual WAL checkpoint driver. + * + * `setInterval` fires on a fixed cadence regardless of whether the previous + * checkpoint has settled. During a large `--pdg` writeback a CHECKPOINT can + * outlast the period; without a guard each overdue tick would launch ANOTHER + * concurrent CHECKPOINT on the singleton connection. The guard (`if (inflight) + * return`) ensures at most one checkpoint is ever in flight. + * + * `tryFlushWAL` is mocked so we can hold a checkpoint "in flight" and drive the + * interval with fake timers — no native engine involved. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + tryFlushWAL: vi.fn(), +})); + +import { startWalCheckpointDriver } from '../../src/core/lbug/wal-checkpoint-driver.js'; +import { tryFlushWAL } from '../../src/core/lbug/lbug-adapter.js'; + +const mockedTryFlush = vi.mocked(tryFlushWAL); + +describe('startWalCheckpointDriver — reentrancy guard', () => { + let originalEnv: string | undefined; + + beforeEach(() => { + originalEnv = process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; + delete process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; // default = enabled + vi.useFakeTimers(); + mockedTryFlush.mockReset(); + }); + + afterEach(() => { + vi.useRealTimers(); + if (originalEnv === undefined) delete process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT; + else process.env.GITNEXUS_WAL_MANUAL_CHECKPOINT = originalEnv; + }); + + it('starts only one checkpoint while a prior one is still in flight, then resumes', async () => { + let resolveFirst!: () => void; + mockedTryFlush + // First checkpoint is held open until we resolve it. + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveFirst = () => resolve(true); + }), + ) + // Any later checkpoint completes immediately. + .mockImplementation(() => Promise.resolve(true)); + + const driver = startWalCheckpointDriver({ periodMs: 10 }); + + // ~5 ticks fire while the first checkpoint is still pending. + await vi.advanceTimersByTimeAsync(55); + expect(mockedTryFlush).toHaveBeenCalledTimes(1); + + // Let the first settle; subsequent ticks may now fire a new checkpoint. + resolveFirst(); + await vi.advanceTimersByTimeAsync(25); + expect(mockedTryFlush.mock.calls.length).toBeGreaterThanOrEqual(2); + + await driver.stop(); + }); +}); diff --git a/gitnexus/vitest.config.ts b/gitnexus/vitest.config.ts index d8dc68835..89719cd80 100644 --- a/gitnexus/vitest.config.ts +++ b/gitnexus/vitest.config.ts @@ -70,6 +70,7 @@ export default defineConfig({ 'test/integration/lbug-readonly-init.test.ts', 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', + 'test/integration/lbug-conn-serialization.test.ts', ], fileParallelism: false, sequence: { groupOrder: 1 }, @@ -103,6 +104,7 @@ export default defineConfig({ 'test/integration/lbug-readonly-init.test.ts', 'test/integration/analyze-wal-checkpoint-failure.test.ts', 'test/integration/lbug-non-ascii-path.test.ts', + 'test/integration/lbug-conn-serialization.test.ts', 'test/integration/skills-e2e.test.ts', ], }, From 6a571570f28f2dd98a0bb7614d60c70b24b798ae Mon Sep 17 00:00:00 2001 From: glier Date: Sun, 21 Jun 2026 18:59:00 +0300 Subject: [PATCH 2/2] feat(group): Kotlin Spring HTTP consumer extraction + provider parity with Java (#2254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(group): Kotlin Spring HTTP consumer extraction + provider parity with Java Brings the Kotlin group/contract HTTP extractor up to parity with Java for inter-service contract detection, and unifies the language-agnostic consumer logic so it is not duplicated. Consumers (new for Kotlin): - @FeignClient interface @(Get|...)Mapping methods are emitted as OpenFeign consumers (a remote call), not providers — previously mis-classified because tree-sitter-kotlin models an interface as a class_declaration. - Spring 6 HTTP Interface @(Get|...)Exchange (with optional class-level @HttpExchange(url) prefix) — added for BOTH Java and Kotlin. - Native OpenFeign @RequestLine, gated to interfaces (Feign proxies are interfaces only), mirroring java.ts's findEnclosingInterface check. Providers (Kotlin parity with java.ts scanSpringProject): - A @(Get|...)Mapping on a non-Feign interface is a route *contract*, not a served route; it is skipped in scan() so the implementing controller is the sole provider (Java drops these implicitly via interface_declaration). - scanProject inherits interface routes onto the implementing class, gated on the class being a @RestController/@Controller (kotlinClassIsController handles both the attached `modifiers` shape and the detached leading-arg-form prefix_expression shape) so non-controller implementers don't emit phantom providers. Shared module: - New spring-consumer-shared.ts holds the language-agnostic primitives (REST_TEMPLATE_/WEB_CLIENT_/EXCHANGE verb maps, joinPath, parseRequestLine, framework + confidence constants); java.ts and kotlin.ts both import it. Array-of-paths (both languages): - Route/Feign/Exchange annotation paths are `String[]`; a multi-element array registers the route under EVERY element. The class/Feign/HttpExchange prefix maps now accumulate all elements (were last-write-wins) and emission cross-products prefixes × method paths, so `@RequestMapping(["/a","/b"])` + `@GetMapping(["/x","/y"])` yields all four contract IDs. Array form is matched via a predicate-free alternation over Kotlin `collection_literal` / Java `element_value_array_initializer`. Tests: comprehensive Java + Kotlin cases incl. consumer-vs-provider classification, @*Exchange, @RequestLine (interface-only + plain-interface), interface-based controller inheritance, non-controller negative case, detached @RestController, single- and multi-element array paths (method-level and class-prefix cross-product). 109 http-route + group tests pass; tsc/eslint/ prettier clean. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(group): apply @RequestMapping prefix to Kotlin @RequestLine consumers (#2254 P2) A @RequestLine method on an interface with a class-level @RequestMapping prefix but no @FeignClient(path) dropped the prefix in Kotlin while Java applied it (java.ts merges the fallback into feignPrefixByInterfaceId). Mirror the feignPrefixByClassId ?? prefixByClassId ?? [''] chain already used by the @GetMapping-in-Feign path. Adds Kotlin twins for the @RequestMapping-prefix and @FeignClient(path)-wins cases. * fix(group): accept named-arg Kotlin @RequestLine(value=...) (#2254 P2) The positional pattern's '.' anchor only matched @RequestLine("VERB /x"), silently dropping the named @RequestLine(value = "VERB /x") form that java.ts accepts. Add a dedicated named pattern constrained to #eq? @key "value" (Java parity: non-value keys stay dropped). Adds Kotlin twins for the named-value and non-value-key cases. * fix(group): resolve Kotlin FQN annotations/supertypes by trailing segment (#2254) A fully-qualified @org…RestController / supertype : a.b.Api parses to a user_type with one type_identifier per dotted segment; kotlinAnnotationName and collectKotlinSupertypes took the FIRST ("org"/"a"), so FQN controllers were not recognised and FQN supertypes never matched their interface. Take the trailing segment. Adds FQN controller + FQN supertype inheritance twins. * refactor(group): remove dead prefix_expression branch in kotlinClassIsController (#2254) AST probe (bare and realistic package+constructor forms) confirms the arg-form @RestController("bean") attaches under the class `modifiers` as an annotation/constructor_invocation, caught by the modifiers loop — the prefix_expression sibling branch was unreachable. Remove it and correct the false grammar comments (source + the arg-form test). The existing arg-form test stays green via the modifiers branch, confirming no behavior change. * feat(group): support Kotlin arrayOf(...) annotation arrays (#2254 P3) arrayOf("/a","/b") (the explicit String[] form) parses to a call_expression, not a string_literal/collection_literal, so it was missed across all five annotation-array families. Add dedicated arrayOf query patterns (positional + named) per family via a shared arrayOfArg fragment — kept out of the existing [(string_literal) (collection_literal …)] alternation to avoid the tree-sitter 0.21.x predicate-bucket hazard. Verified one match per element (multi-element accumulates) with buildPath/produces/empty anti-overreach. * feat(group): detect WebClient long-form in Java for Kotlin parity (#2254 P3) Java deliberately deferred webClient.method(HttpMethod.X).uri(...); the Kotlin plugin proves a single structural query suffices (same field-access shape as REST_TEMPLATE_EXCHANGE). Add WEB_CLIENT_LONG_FORM_PATTERNS + scan loop so .java and .kt detect it identically. Move WEB_CLIENT_LONG_VERB_RE to the shared module (single source for both). Flip the now-obsolete java :1741 negative test to positive (verbs + no-double-emit) and add a Java var-verb anti-overreach twin. * refactor(group): share pushPrefix between java.ts and kotlin.ts (#2254) The de-duping prefix accumulator was duplicated as kotlin.ts pushKotlinPrefix and a java.ts closure. Hoist a single export pushPrefix into spring-consumer-shared.ts; both plugins import it. No behavior change. * test(group): add Kotlin interface-inheritance boundary twins (#2254) Twins for the Java inheritance-boundary cases that had no Kotlin counterpart: shared-leading-segment combine, prefix-less method overlap, ambiguous duplicate-interface-name suppression, plus a positive multi-interface implementer. These pin Kotlin's scanProject behavior before U8 extracts the shared inheritance algorithm. * refactor(group): share the Spring interface-inheritance scanProject algorithm (#2254) scanKotlinProject and scanSpringProject were ~80-line near-duplicates over structurally identical type records. Extract scanSpringInheritanceProject + SharedSpringType into spring-consumer-shared.ts; collapse KotlinTypeInfo and SpringTypeInfo into the shared type; both plugins' scanProject become thin collect-and-delegate wrappers. The ownerPrefix-carrying intermediate is owned by the shared function. Behavior-preserving — Java and Kotlin inheritance suites (incl. the new Kotlin boundary twins) byte-identical; tsc clean. * test(group): close Kotlin↔Java consumer test-parity gaps + assert confidence (#2254) Add Kotlin twins for Java-tested consumer scenarios with no Kotlin coverage: @RequestLine query-strip, mixed @RequestLine+@GetMapping, malformed-value rejection, and @FeignClient(path)-wins-when-@RequestMapping-first. Add the Java dual-role twin (interface as consumer + implementing controller as provider). Add two-sided provider confidence (0.8) assertions on the canonical Java and Kotlin interface-inheritance tests. * docs(group): document Java FQN route-annotation limitation + pin it (#2254) Per KTD6, the Java FQN route-annotation gap is documentation-first: the gap is route-string-only (FQN controllers are already recognised via hasAnnotation) and FQN-written annotations are vanishingly rare. Document the asymmetry with Kotlin in JAVA_ROUTE_ANNOTATION_PATTERNS and pin current behavior with an anti-overreach test. The scoped_identifier query change is deferred to avoid re-keying existing contracts via the predicate-bucket hazard. * test(group): add Java↔Kotlin contract set-equality parity harness (#2254) Independent per-side twins can both pass while the emitted contract SETS differ. Add a table-driven harness over the parity-critical families (@RequestLine prefix-fallback, named @RequestLine, @FeignClient(path)+@GetMapping, @HttpExchange+@GetExchange, WebClient long-form, interface inheritance) that runs matched .java/.kt fixtures through both plugins and asserts the full projected contract set (role+contractId+framework+confidence) is equal across languages AND equal to the expected set — the durable guard for the byte-identical goal. Gated on kotlinConsumerAvailable. * style(group): apply prettier formatting to #2254 changes --------- Co-authored-by: Claude Opus 4.8 (1M context) Co-authored-by: Gergő Magyar --- .../group/extractors/http-patterns/java.ts | 466 ++-- .../group/extractors/http-patterns/kotlin.ts | 687 +++++- .../http-patterns/spring-consumer-shared.ts | 269 +++ .../unit/group/http-route-extractor.test.ts | 2133 ++++++++++++++++- 4 files changed, 3236 insertions(+), 319 deletions(-) create mode 100644 gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts diff --git a/gitnexus/src/core/group/extractors/http-patterns/java.ts b/gitnexus/src/core/group/extractors/http-patterns/java.ts index 3ad66543e..5afebbace 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/java.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/java.ts @@ -11,6 +11,22 @@ import { isRouteMemberKey, findEnclosingClass, } from '../../../ingestion/route-extractors/spring-shared.js'; +import { + REST_TEMPLATE_TO_HTTP, + WEB_CLIENT_SHORT_TO_HTTP, + WEB_CLIENT_LONG_VERB_RE, + EXCHANGE_ANNOTATION_TO_HTTP, + parseRequestLine, + pushPrefix, + joinPath, + scanSpringInheritanceProject, + type SharedSpringType, + OPENFEIGN_FRAMEWORK, + HTTP_INTERFACE_FRAMEWORK, + FEIGN_CONFIDENCE, + REQUEST_LINE_CONFIDENCE, + EXCHANGE_CONFIDENCE, +} from './spring-consumer-shared.js'; import type { HttpDetection, HttpFileDetections, @@ -49,38 +65,23 @@ import type { // corrupt every route). That key filtering is done in `isRouteMemberKey`, and // all of these annotations are matched by the one `JAVA_ROUTE_ANNOTATION_PATTERNS` // query below (see its header for why the filtering lives in JS, not the query). -interface SpringRouteBinding { - method: string; - path: string; - ownerPrefix?: string; -} - -interface SpringMethodInfo { - name: string; - routes: SpringRouteBinding[]; -} - -interface SpringTypeInfo { - filePath: string; - kind: 'class' | 'interface'; - name: string; - classPrefix: string; - implementedInterfaces: string[]; - isController: boolean; - methods: SpringMethodInfo[]; -} +// The Spring class/interface view (`SharedSpringType`) and the interface-based +// controller inheritance algorithm (`scanSpringInheritanceProject`) are shared +// with kotlin.ts via spring-consumer-shared.ts so both plugins emit identical +// provider contracts. `collectSpringTypes` below produces that shared shape. // ─── Route-defining annotations (one generic query, one pass) ───────── // Every Java route-mapper annotation shares one shape: an annotation carrying a -// single string argument — positional `"..."` or named `key = "..."` — on a -// class, interface, or method. This SINGLE query matches that shape generically; -// `scanRouteAnnotations` then reads the annotation NAME (`@ann`) and declaration -// kind (`@node.type`) in its for-loop to decide what each match means. Adding a -// new framework annotation that follows this single-string-argument shape is a -// change to that loop (and the lookup maps), not to this query. Annotations with -// a different argument shape — e.g. an array value `@RequestMapping({"/a","/b"})` -// — are out of scope here (as they were for the prior queries) and would need a -// new branch. +// string argument — positional `"..."` or named `key = "..."`, each also in its +// array form `{"..."}` / `key = {"..."}` (Spring's `path`/`value` are +// `String[]`) — on a class, interface, or method. This SINGLE query matches that +// shape generically; the `@value` capture is an alternation over a bare +// `string_literal` and one nested in an `element_value_array_initializer`, so a +// single-element array yields the same `@value` (a multi-element array yields one +// match per element). `scanRouteAnnotations` then reads the annotation NAME +// (`@ann`) and declaration kind (`@node.type`) in its for-loop to decide what +// each match means. Adding a new framework annotation that follows this shape is +// a change to that loop (and the lookup maps), not to this query. // // Captures (shared across all branches; intentionally framework-agnostic): // @ann → the annotation name identifier (RequestMapping, GetMapping, RequestLine, …) @@ -96,6 +97,18 @@ interface SpringTypeInfo { // silently dropping sibling-branch matches. Keeping the query predicate-free // sidesteps that hazard entirely; all name/key discrimination lives in the // for-loop, where it reads as straight-line code. +// +// KNOWN LIMITATION — fully-qualified route annotations are not matched. `@ann` +// binds `name: (identifier)`, but a FQN annotation (`@org.springframework… +// GetMapping("/x")`) parses its name as a `scoped_identifier`, which this query +// does not match, so its route is not extracted. (The class is still recognized +// as a controller — `hasAnnotation` trailing-segment-matches the FQN — only the +// route-string extraction is missed.) In practice annotations are imported and +// written by simple name, so this is rare. It is a minor asymmetry with the +// Kotlin plugin, whose grammar models a FQN as separate `type_identifier` +// segments that its route queries DO match. Aligning Java would mean matching +// `scoped_identifier` too; that is deferred to avoid re-keying existing Java +// contracts via the predicate hazard above. Pinned by an anti-overreach test. const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ name: 'java-route-annotation', language: Java, @@ -108,12 +121,12 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) @node (interface_declaration (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value)))) @node + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) @node (class_declaration (modifiers (annotation @@ -121,7 +134,7 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))))) @node (interface_declaration (modifiers (annotation @@ -129,12 +142,12 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value))))) @node + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))))) @node (method_declaration (modifiers (annotation name: (identifier) @ann - arguments: (annotation_argument_list (string_literal) @value))) + arguments: (annotation_argument_list [(string_literal) @value (element_value_array_initializer (string_literal) @value)]))) name: (identifier) @member) @node (method_declaration (modifiers @@ -143,7 +156,7 @@ const JAVA_ROUTE_ANNOTATION_PATTERNS = compilePatterns({ arguments: (annotation_argument_list (element_value_pair key: (identifier) @key - value: (string_literal) @value)))) + value: [(string_literal) @value (element_value_array_initializer (string_literal) @value)])))) name: (identifier) @member) @node ] `, @@ -167,59 +180,10 @@ const SPRING_TYPE_DECLARATION_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns>); -// ─── Consumer: OpenFeign `@RequestLine("METHOD /path")` parsing ─────── -// OpenFeign's native annotation pairs an HTTP method and path in a single -// string literal — see https://github.com/OpenFeign/feign#interface-annotations. -// It is method-level only and is mutually exclusive with Spring MVC -// `@GetMapping` / `@PostMapping` etc. on the same method (mixing them -// requires a different Feign Contract — they are not combined). The match -// itself comes from `JAVA_ROUTE_ANNOTATION_PATTERNS`; this regex splits the -// verb from the path of the captured literal. -// -// Examples: -// @RequestLine("GET /users/{id}") -// @RequestLine("POST /users?status=active") -const REQUEST_LINE_VERB_RE = /^\s*(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s+(\S.*?)\s*$/i; - -/** - * Parse a Feign `@RequestLine` value into a method + path pair. - * - * `@RequestLine("METHOD /path[?query]")` packs both fields in one string; - * the query portion is dropped because contract IDs are method+path only - * (consistent with how other consumers like RestTemplate/WebClient drop - * query strings when their values are inline literals). - * - * Returns null if the value is not a recognized HTTP verb followed by a - * path beginning with `/`. - */ -function parseRequestLine(raw: string): { method: string; path: string } | null { - const match = REQUEST_LINE_VERB_RE.exec(raw); - if (!match) return null; - const [, verb, rest] = match; - if (typeof verb !== 'string' || typeof rest !== 'string') return null; - const queryIdx = rest.indexOf('?'); - const pathOnly = (queryIdx >= 0 ? rest.slice(0, queryIdx) : rest).trim(); - if (!pathOnly.startsWith('/')) return null; - return { method: verb.toUpperCase(), path: pathOnly }; -} - -// ─── Consumer: Spring RestTemplate (object-named + method-named) ────── -// RestTemplate.getForObject / getForEntity → GET -// RestTemplate.postForObject / postForEntity → POST -// RestTemplate.put → PUT -// RestTemplate.delete → DELETE -// RestTemplate.patchForObject → PATCH -// Source-scan only: receiver must be named exactly `restTemplate`. -// Fields, `this.restTemplate`, aliases, and other injection names are deferred. -const REST_TEMPLATE_TO_HTTP: Record = { - getForObject: 'GET', - getForEntity: 'GET', - postForObject: 'POST', - postForEntity: 'POST', - put: 'PUT', - delete: 'DELETE', - patchForObject: 'PATCH', -}; +// OpenFeign `@RequestLine` parsing (`parseRequestLine`), the RestTemplate and +// WebClient short-form verb maps, the `@*Exchange` verb map, `joinPath`, and the +// shared confidence/framework constants live in `spring-consumer-shared.ts` so +// the Java and Kotlin plugins emit identical contract IDs. interface RestTemplateMeta { framework: 'spring-rest-template'; @@ -261,14 +225,6 @@ const REST_TEMPLATE_EXCHANGE_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns); -const WEB_CLIENT_SHORT_TO_HTTP: Record = { - get: 'GET', - post: 'POST', - put: 'PUT', - delete: 'DELETE', - patch: 'PATCH', -}; - const WEB_CLIENT_SHORT_FORM_PATTERNS = compilePatterns({ name: 'java-web-client-short-form', language: Java, @@ -288,6 +244,37 @@ const WEB_CLIENT_SHORT_FORM_PATTERNS = compilePatterns({ ], } satisfies LanguagePatterns>); +// ─── Consumer: WebClient long form `webClient.method(HttpMethod.X).uri("/y")` ─ +// The fluent long form carries the verb as a `HttpMethod.X` field access through +// `.method(...)` and the path on a separate `.uri(...)` hop. A single structural +// query matches the whole chain (the same field-access shape used by +// REST_TEMPLATE_EXCHANGE_PATTERNS) — the earlier "intentionally deferred" note +// predated the Kotlin plugin proving the structural query is enough. Variable- +// bound verbs (`webClient.method(verb).uri(...)`) do NOT match: the value carries +// a bare `identifier`, not a `HttpMethod.X` field access — source-scan can't +// follow the binding (anti-overreach test pins this, parity with Kotlin). +const WEB_CLIENT_LONG_FORM_PATTERNS = compilePatterns({ + name: 'java-web-client-long-form', + language: Java, + patterns: [ + { + meta: {}, + query: ` + (method_invocation + object: (method_invocation + object: (identifier) @obj (#eq? @obj "webClient") + name: (identifier) @method_call (#eq? @method_call "method") + arguments: (argument_list + (field_access + object: (identifier) @httpMethodCls (#eq? @httpMethodCls "HttpMethod") + field: (identifier) @verb))) + name: (identifier) @uri_method (#eq? @uri_method "uri") + arguments: (argument_list . (string_literal) @path)) + `, + }, + ], +} satisfies LanguagePatterns>); + // ─── Consumer: OkHttp `new Request.Builder().url("path")` ───────────── // Note: `Request.Builder` is a `scoped_type_identifier` whose text includes // the dot, so `#eq?` against the literal string matches cleanly (no need @@ -370,38 +357,6 @@ function findEnclosingInterface(node: Parser.SyntaxNode): Parser.SyntaxNode | nu return null; } -/** - * Join a class-level prefix and a method-level path into a single URL - * path. Mirrors the semantics of the original regex implementation: - * strip trailing slashes on the prefix, then ensure a single slash - * between prefix and method path. - */ -function joinPath(prefix: string, methodPath: string): string { - const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanSub = methodPath.replace(/^\/+/, ''); - if (!cleanPrefix) return `/${cleanSub}`; - return `/${cleanPrefix}/${cleanSub}`; -} - -function joinInheritedSpringPath( - controllerPrefix: string, - inheritedPath: string, - inheritedOwnerPrefix = '', -): string { - const joined = joinPath(controllerPrefix, inheritedPath); - const cleanPrefix = controllerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanOwnerPrefix = inheritedOwnerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanInherited = inheritedPath.replace(/^\/+/, ''); - if (!cleanPrefix) return joined; - if ( - cleanPrefix === cleanOwnerPrefix && - (cleanInherited === cleanPrefix || cleanInherited.startsWith(`${cleanPrefix}/`)) - ) { - return `/${cleanInherited}`; - } - return joined; -} - function getNodeName(node: Parser.SyntaxNode): string | null { return node.childForFieldName('name')?.text ?? null; } @@ -440,14 +395,18 @@ interface RequestLineAnnotation { } interface RouteAnnotationScan { - /** Spring `@RequestMapping` URL prefix per class/interface node id (last write wins). */ - prefixByTypeId: Map; - /** OpenFeign interface prefix per interface node id; `@FeignClient(path)` wins over `@RequestMapping`. */ - feignPrefixByInterfaceId: Map; + /** Spring `@RequestMapping` URL prefixes per class/interface node id (one per array element). */ + prefixByTypeId: Map; + /** OpenFeign interface prefixes per interface node id; `@FeignClient(path)` wins over `@RequestMapping`. */ + feignPrefixByInterfaceId: Map; + /** Spring HTTP Interface `@HttpExchange(url|value)` type-level prefixes per class/interface node id. */ + httpExchangePrefixByTypeId: Map; /** One entry per resolved Spring `@(Get|...)Mapping` route — a method with N mappings yields N entries. */ methodRoutes: MethodRouteAnnotation[]; /** One entry per OpenFeign `@RequestLine` whose value parses to a verb + path. */ requestLines: RequestLineAnnotation[]; + /** One entry per Spring HTTP Interface `@(Get|...)Exchange` method — always a consumer. */ + exchangeRoutes: MethodRouteAnnotation[]; } /** @@ -468,13 +427,17 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // collectSpringTypes cross-file inheritance), while `feignPrefixByInterfaceId` // feeds the OpenFeign *consumer* path in scan(). An interface carrying both // `@RequestMapping` and `@FeignClient(path)` lands a different value in each. - const prefixByTypeId = new Map(); - const feignPrefixByInterfaceId = new Map(); + const prefixByTypeId = new Map(); + const feignPrefixByInterfaceId = new Map(); + const httpExchangePrefixByTypeId = new Map(); const methodRoutes: MethodRouteAnnotation[] = []; const requestLines: RequestLineAnnotation[] = []; + const exchangeRoutes: MethodRouteAnnotation[] = []; // Interface `@RequestMapping` prefixes rank below `@FeignClient(path)`; // collect them and apply only after the FeignClient pass below. const interfaceRequestMappingPrefixes: Array<{ id: number; prefix: string }> = []; + // `pushPrefix` (the de-duping accumulator) is shared from + // spring-consumer-shared.ts so Java and Kotlin build prefix maps identically. for (const { captures } of matches) { const annNode = captures.ann; @@ -510,6 +473,20 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { parsed, }); } + } else if (EXCHANGE_ANNOTATION_TO_HTTP[ann]) { + // Spring 6 HTTP Interface `@(Get|...)Exchange` — the path lives in the + // `url` or `value` attribute (or positionally); other attributes + // (`accept`, `contentType`, …) are not routes. + if (keyNode && keyNode.text !== 'url' && keyNode.text !== 'value') continue; + const rawPath = unquoteLiteral(valueNode.text); + if (rawPath !== null) { + exchangeRoutes.push({ + methodNode: node, + methodName: captures.member?.text ?? null, + httpMethod: EXCHANGE_ANNOTATION_TO_HTTP[ann], + rawPath, + }); + } } continue; } @@ -520,7 +497,7 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { if (!isRouteMemberKey(keyNode)) continue; const prefix = unquoteLiteral(valueNode.text); if (prefix !== null) { - prefixByTypeId.set(node.id, prefix); + pushPrefix(prefixByTypeId, node.id, prefix); if (node.type === 'interface_declaration') { interfaceRequestMappingPrefixes.push({ id: node.id, prefix }); } @@ -529,17 +506,30 @@ function scanRouteAnnotations(tree: Parser.Tree): RouteAnnotationScan { // Feign's `name`/`value` identify a service, not a path — only `path` is a prefix. if (!keyNode || keyNode.text !== 'path') continue; const prefix = unquoteLiteral(valueNode.text); - if (prefix !== null && !feignPrefixByInterfaceId.has(node.id)) { - feignPrefixByInterfaceId.set(node.id, prefix); - } + if (prefix !== null) pushPrefix(feignPrefixByInterfaceId, node.id, prefix); + } else if (ann === 'HttpExchange') { + // Spring HTTP Interface type-level prefix: the path lives in `url`/`value` + // (or positionally). Applies to its `@(Get|...)Exchange` consumer methods. + if (keyNode && keyNode.text !== 'url' && keyNode.text !== 'value') continue; + const prefix = unquoteLiteral(valueNode.text); + if (prefix !== null) pushPrefix(httpExchangePrefixByTypeId, node.id, prefix); } } + // `@RequestMapping` on a Feign interface is the fallback prefix, but only when + // the interface has no `@FeignClient(path)` of its own (path wins). for (const { id, prefix } of interfaceRequestMappingPrefixes) { - if (!feignPrefixByInterfaceId.has(id)) feignPrefixByInterfaceId.set(id, prefix); + if (!feignPrefixByInterfaceId.has(id)) pushPrefix(feignPrefixByInterfaceId, id, prefix); } - return { prefixByTypeId, feignPrefixByInterfaceId, methodRoutes, requestLines }; + return { + prefixByTypeId, + feignPrefixByInterfaceId, + httpExchangePrefixByTypeId, + methodRoutes, + requestLines, + exchangeRoutes, + }; } function collectDirectMethods(typeNode: Parser.SyntaxNode): Parser.SyntaxNode[] { @@ -578,15 +568,15 @@ function collectImplementedInterfaces(typeNode: Parser.SyntaxNode): string[] { return out; } -function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo[] { +function collectSpringTypes(filePath: string, tree: Parser.Tree): SharedSpringType[] { const { prefixByTypeId, methodRoutes } = scanRouteAnnotations(tree); - const routesByMethodId = new Map(); + const routesByMethodId = new Map>(); for (const route of methodRoutes) { const routes = routesByMethodId.get(route.methodNode.id) ?? []; routes.push({ method: route.httpMethod, path: route.rawPath }); routesByMethodId.set(route.methodNode.id, routes); } - const out: SpringTypeInfo[] = []; + const out: SharedSpringType[] = []; for (const match of runCompiledPatterns(SPRING_TYPE_DECLARATION_PATTERNS, tree)) { const typeNode = match.captures.type; @@ -598,13 +588,16 @@ function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo name: getNodeName(methodNode), routes: routesByMethodId.get(methodNode.id) ?? [], })) - .filter((method): method is SpringMethodInfo => method.name !== null); + .filter( + (method): method is { name: string; routes: Array<{ method: string; path: string }> } => + method.name !== null, + ); out.push({ filePath, kind, name: typeNameNode.text, - classPrefix: prefixByTypeId.get(typeNode.id) ?? '', + classPrefixes: prefixByTypeId.get(typeNode.id) ?? [], implementedInterfaces: kind === 'class' ? collectImplementedInterfaces(typeNode) : [], isController: kind === 'class' && hasAnnotation(typeNode, ['RestController', 'Controller']), methods, @@ -614,62 +607,13 @@ function collectSpringTypes(filePath: string, tree: Parser.Tree): SpringTypeInfo return out; } +// The interface-based-controller inheritance algorithm is shared with kotlin.ts +// (`scanSpringInheritanceProject`); this collects the `SharedSpringType` view and +// delegates so Java and Kotlin emit byte-identical provider contracts. function scanSpringProject(files: readonly HttpScanInput[]): HttpFileDetections[] { - const types = files.flatMap((file) => collectSpringTypes(file.filePath, file.tree)); - const interfaceRoutes = new Map | null>(); - - for (const type of types) { - if (type.kind !== 'interface') continue; - if (interfaceRoutes.has(type.name)) { - interfaceRoutes.set(type.name, null); - continue; - } - const methodMap = new Map(); - for (const method of type.methods) { - const routes = method.routes.map((route) => ({ - method: route.method, - path: type.classPrefix ? joinPath(type.classPrefix, route.path) : route.path, - ownerPrefix: type.classPrefix, - })); - if (routes.length > 0) methodMap.set(method.name, routes); - } - interfaceRoutes.set(type.name, methodMap); - } - - const detectionsByFile = new Map(); - for (const type of types) { - if (type.kind !== 'class' || !type.isController) continue; - for (const method of type.methods) { - if (method.routes.length > 0) continue; - const inheritedRoutes = type.implementedInterfaces.flatMap((interfaceName) => { - const routeMap = interfaceRoutes.get(interfaceName); - if (!routeMap) return []; - const routes = routeMap.get(method.name) ?? []; - return routes.map((route) => ({ - method: route.method, - path: joinInheritedSpringPath(type.classPrefix, route.path, route.ownerPrefix), - })); - }); - - for (const route of inheritedRoutes) { - const detections = detectionsByFile.get(type.filePath) ?? []; - detections.push({ - role: 'provider', - framework: 'spring', - method: route.method, - path: route.path, - name: method.name, - confidence: 0.8, - }); - detectionsByFile.set(type.filePath, detections); - } - } - } - - return [...detectionsByFile.entries()].map(([filePath, detections]) => ({ - filePath, - detections, - })); + return scanSpringInheritanceProject( + files.flatMap((file) => collectSpringTypes(file.filePath, file.tree)), + ); } export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { @@ -682,8 +626,14 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { // `scanRouteAnnotations` resolves every route-defining annotation — // class/interface prefixes, method `@(Get|...)Mapping`s and native // `@RequestLine`s — from a single `matches()` pass over the tree. - const { prefixByTypeId, feignPrefixByInterfaceId, methodRoutes, requestLines } = - scanRouteAnnotations(tree); + const { + prefixByTypeId, + feignPrefixByInterfaceId, + httpExchangePrefixByTypeId, + methodRoutes, + requestLines, + exchangeRoutes, + } = scanRouteAnnotations(tree); // A `@(Get|...)Mapping` inside a `@FeignClient` interface is an OpenFeign // *consumer* (it describes a remote call); the same annotation inside a @@ -693,28 +643,34 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { for (const route of methodRoutes) { const enclosingInterface = findEnclosingInterface(route.methodNode); if (enclosingInterface && hasAnnotation(enclosingInterface, 'FeignClient')) { - const prefix = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ''; - out.push({ - role: 'consumer', - framework: 'openfeign', - method: route.httpMethod, - path: joinPath(prefix, route.rawPath), - name: route.methodName, - confidence: 0.7, - }); + const prefixes = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: FEIGN_CONFIDENCE, + }); + } continue; } const enclosingClass = findEnclosingClass(route.methodNode); if (!enclosingClass) continue; - const prefix = prefixByTypeId.get(enclosingClass.id) ?? ''; - out.push({ - role: 'provider', - framework: 'spring', - method: route.httpMethod, - path: joinPath(prefix, route.rawPath), - name: route.methodName, - confidence: 0.8, - }); + // A multi-element class `@RequestMapping({"/a","/b"})` registers the method + // under each prefix — emit one provider per (prefix × this route). + const prefixes = prefixByTypeId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'provider', + framework: 'spring', + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: 0.8, + }); + } } // Native OpenFeign `@RequestLine("METHOD /path")`. Method-level only and @@ -730,15 +686,38 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { for (const requestLine of requestLines) { const enclosingInterface = findEnclosingInterface(requestLine.methodNode); if (!enclosingInterface) continue; - const prefix = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ''; - out.push({ - role: 'consumer', - framework: 'openfeign', - method: requestLine.parsed.method, - path: joinPath(prefix, requestLine.parsed.path), - name: requestLine.methodName, - confidence: 0.75, - }); + const prefixes = feignPrefixByInterfaceId.get(enclosingInterface.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: requestLine.parsed.method, + path: joinPath(prefix, requestLine.parsed.path), + name: requestLine.methodName, + confidence: REQUEST_LINE_CONFIDENCE, + }); + } + } + + // ─── Consumers: Spring HTTP Interface @(Get|...)Exchange ──────── + // Declarative client interfaces proxied by `HttpServiceProxyFactory` + // (over RestClient / WebClient / RestTemplate). Always a consumer — no + // provider ambiguity — with an optional type-level `@HttpExchange(url)` + // prefix. The verb comes from the annotation name (`@GetExchange` → GET). + for (const route of exchangeRoutes) { + const enclosing = + findEnclosingInterface(route.methodNode) ?? findEnclosingClass(route.methodNode); + const prefixes = enclosing ? (httpExchangePrefixByTypeId.get(enclosing.id) ?? ['']) : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: HTTP_INTERFACE_FRAMEWORK, + method: route.httpMethod, + path: joinPath(prefix, route.rawPath), + name: route.methodName, + confidence: EXCHANGE_CONFIDENCE, + }); + } } // ─── Consumers: RestTemplate ──────────────────────────────────── @@ -777,9 +756,9 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { } // ─── Consumers: WebClient.get().uri("path") short form ───────── - // Source-scan only: receiver must be named exactly `webClient`. - // The real long-form chain `webClient.method(HttpMethod.X).uri("/x")` - // needs multi-hop chain analysis and is intentionally deferred. + // Source-scan only: receiver must be named exactly `webClient`. The + // long-form chain `webClient.method(HttpMethod.X).uri("/x")` is handled + // separately below by WEB_CLIENT_LONG_FORM_PATTERNS. for (const match of runCompiledPatterns(WEB_CLIENT_SHORT_FORM_PATTERNS, tree)) { const verbNode = match.captures.verb; const pathNode = match.captures.path; @@ -798,6 +777,29 @@ export const JAVA_HTTP_PLUGIN: HttpLanguagePlugin = { }); } + // ─── Consumers: WebClient.method(HttpMethod.X).uri("path") long form ─ + // The verb is captured as the literal `HttpMethod.X` field name; gate it on + // the shared verb regex (HEAD/OPTIONS/TRACE excluded, matching the short + // form). The short-form query requires an empty inner argument list, so it + // cannot also fire on this chain — no double-emit. + for (const match of runCompiledPatterns(WEB_CLIENT_LONG_FORM_PATTERNS, tree)) { + const verbNode = match.captures.verb; + const pathNode = match.captures.path; + if (!verbNode || !pathNode) continue; + const verbText = verbNode.text; + if (!WEB_CLIENT_LONG_VERB_RE.test(verbText)) continue; + const path = unquoteLiteral(pathNode.text); + if (path === null) continue; + out.push({ + role: 'consumer', + framework: 'spring-web-client', + method: verbText, + path, + name: null, + confidence: 0.7, + }); + } + // ─── Consumers: OkHttp Request.Builder().url("path") ──────────── for (const match of runCompiledPatterns(OK_HTTP_PATTERNS, tree)) { const pathNode = match.captures.path; diff --git a/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts b/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts index 14dce0ae1..4286ba758 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/kotlin.ts @@ -1,4 +1,4 @@ -import Parser from 'tree-sitter'; +import type Parser from 'tree-sitter'; import { requireVendoredGrammar } from '../../../tree-sitter/vendored-grammars.js'; import { compilePatterns, @@ -6,7 +6,32 @@ import { unquoteLiteral, type LanguagePatterns, } from '../tree-sitter-scanner.js'; -import type { HttpDetection, HttpLanguagePlugin } from './types.js'; +import type { + HttpDetection, + HttpFileDetections, + HttpLanguagePlugin, + HttpScanInput, +} from './types.js'; +import { + METHOD_ANNOTATION_TO_HTTP, + findEnclosingClass, +} from '../../../ingestion/route-extractors/spring-shared.js'; +import { + REST_TEMPLATE_TO_HTTP, + WEB_CLIENT_SHORT_TO_HTTP, + WEB_CLIENT_LONG_VERB_RE, + EXCHANGE_ANNOTATION_TO_HTTP, + parseRequestLine, + pushPrefix, + joinPath, + scanSpringInheritanceProject, + type SharedSpringType, + OPENFEIGN_FRAMEWORK, + HTTP_INTERFACE_FRAMEWORK, + FEIGN_CONFIDENCE, + REQUEST_LINE_CONFIDENCE, + EXCHANGE_CONFIDENCE, +} from './spring-consumer-shared.js'; /** * Kotlin HTTP plugin (Spring providers + consumers). @@ -74,53 +99,37 @@ try { Kotlin = null; } -const METHOD_ANNOTATION_TO_HTTP: Record = { - GetMapping: 'GET', - PostMapping: 'POST', - PutMapping: 'PUT', - DeleteMapping: 'DELETE', - PatchMapping: 'PATCH', -}; +// The Spring `@(Get|...)Mapping` verb map, RestTemplate / WebClient short-form +// verb maps, the `@(Get|...)Exchange` verb map, `joinPath`, `parseRequestLine`, +// and the shared confidence/framework constants are imported from +// `spring-consumer-shared.ts` / `spring-shared.ts` so the Kotlin and Java +// plugins emit identical contract IDs. + +// The WebClient long-form verb gate (`WEB_CLIENT_LONG_VERB_RE`) is imported from +// `spring-consumer-shared.ts` so the Java and Kotlin long-form scans accept the +// same verb set (HEAD/OPTIONS/TRACE excluded, matching the short form). + +// The de-duping prefix accumulator (`pushPrefix`) is imported from +// `spring-consumer-shared.ts` so the Java and Kotlin plugins build their +// per-declaration prefix maps identically. /** - * RestTemplate method-name → HTTP verb. Mirrors the Java plugin's - * `REST_TEMPLATE_TO_HTTP` (java.ts) so a polyglot repo emits the - * same contract IDs from .java and .kt sources. + * Tree-sitter sub-pattern for the Kotlin `arrayOf("/a", "/b")` annotation-array + * form, capturing each element string under `cap` (`@prefix` or `@path`). + * + * Kept as a DEDICATED query fragment embedded in its own pattern — NEVER as an + * arm of the `[(string_literal) (collection_literal …)]` alternation. The + * `#eq? @arrayOf "arrayOf"` predicate, sharing a single alternation bucket with + * the string/collection arms, would evaluate FALSE for those arms (where + * `@arrayOf` is absent) and silently drop them — the tree-sitter 0.21.x hazard + * documented in `java.ts`. tree-sitter yields one match per `arrayOf` element, + * so multi-element arrays accumulate through the same loops as `collection_literal` + * (verified by AST probe). The `arrayOf` callee constraint keeps unrelated calls + * (`buildPath("/x")`) from matching. */ -const REST_TEMPLATE_TO_HTTP: Record = { - getForObject: 'GET', - getForEntity: 'GET', - postForObject: 'POST', - postForEntity: 'POST', - put: 'PUT', - delete: 'DELETE', - patchForObject: 'PATCH', -}; - -/** - * WebClient short-form verb → HTTP verb. The reactive WebClient API - * exposes `.get()`, `.post()`, `.put()`, `.delete()`, `.patch()` as - * one-liners that return a `RequestHeadersUriSpec` whose `.uri(...)` - * carries the path. We capture both pieces in a single query (see - * `WEB_CLIENT_SHORT_PATTERNS` below) and translate the verb here. - */ -const WEB_CLIENT_SHORT_TO_HTTP: Record = { - get: 'GET', - post: 'POST', - put: 'PUT', - delete: 'DELETE', - patch: 'PATCH', -}; - -/** - * Allowed HTTP verbs for the WebClient long-form path - * `webClient.method(HttpMethod.X).uri("/y")`. Compiled once at module - * load (instead of inside the scan loop) per maintainer feedback on - * PR #1884. Mirrors the keys of `WEB_CLIENT_SHORT_TO_HTTP` above — - * keeping HEAD/OPTIONS/TRACE intentionally excluded for symmetry - * with the short form and the Java plugin. - */ -const WEB_CLIENT_LONG_VERB_RE = /^(GET|POST|PUT|DELETE|PATCH)$/; +const arrayOfArg = (cap: string): string => `(call_expression + (simple_identifier) @arrayOf (#eq? @arrayOf "arrayOf") + (call_suffix (value_arguments (value_argument (string_literal) ${cap}))))`; /** * Build the plugin only if the Kotlin grammar is available. Compiling @@ -161,7 +170,7 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (constructor_invocation (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) (value_arguments - (value_argument . (string_literal) @prefix))))) + (value_argument . [(string_literal) @prefix (collection_literal (string_literal) @prefix)]))))) (type_identifier) @cls) @class `, }, @@ -176,7 +185,35 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (value_arguments (value_argument (simple_identifier) @key (#match? @key "^(path|value)$") - (string_literal) @prefix))))) + [(string_literal) @prefix (collection_literal (string_literal) @prefix)]))))) + (type_identifier) @cls) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) + (value_arguments + (value_argument . ${arrayOfArg('@prefix')}))))) + (type_identifier) @cls) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestMapping")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(path|value)$") + ${arrayOfArg('@prefix')}))))) (type_identifier) @cls) @class `, }, @@ -200,7 +237,7 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (constructor_invocation (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) (value_arguments - (value_argument . (string_literal) @path))))) + (value_argument . [(string_literal) @path (collection_literal (string_literal) @path)]))))) (simple_identifier) @method_name) @method `, }, @@ -215,7 +252,35 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { (value_arguments (value_argument (simple_identifier) @key (#match? @key "^(path|value)$") - (string_literal) @path))))) + [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) + (value_arguments + (value_argument . ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Mapping$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(path|value)$") + ${arrayOfArg('@path')}))))) (simple_identifier) @method_name) @method `, }, @@ -400,31 +465,362 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { ], } satisfies LanguagePatterns>); - /** - * Find the nearest enclosing class_declaration ancestor for a node, or - * null if the node is top-level. Mirrors the Java plugin's helper. - */ - function findEnclosingClass(node: Parser.SyntaxNode): Parser.SyntaxNode | null { - let cur: Parser.SyntaxNode | null = node.parent; - while (cur) { - if (cur.type === 'class_declaration') return cur; - cur = cur.parent; - } - return null; - } + // ─── Consumer (OpenFeign): @FeignClient interface marker + path prefix ─ + // A `@FeignClient` interface's `@(Get|...)Mapping` methods describe OUTBOUND + // calls (consumers), not routes the service serves. Pattern 1 marks the + // interface; pattern 2 captures its optional `path = "/prefix"` (the + // `name`/`value`/`url` attributes identify the remote service, not a path). + // In tree-sitter-kotlin an `interface` is a `class_declaration`, so the + // method-route loop reclassifies @*Mapping methods whose enclosing + // class_declaration is in `feignClassIds` (see scan()). + const SPRING_FEIGN_CLIENT_PATTERNS = compilePatterns({ + name: 'kotlin-spring-feign-client', + language, + patterns: [ + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "path") + [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "FeignClient")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "path") + ${arrayOfArg('@prefix')})))))) @class + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: Spring 6 HTTP Interface @(Get|...)Exchange ───────────── + // Declarative client interfaces proxied by HttpServiceProxyFactory (over + // RestClient / WebClient / RestTemplate). The path lives in `url`/`value` + // (named) or positionally. Always a consumer — no provider ambiguity. + const SPRING_EXCHANGE_PATTERNS = compilePatterns({ + name: 'kotlin-spring-http-exchange', + language, + patterns: [ + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument . [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + [(string_literal) @path (collection_literal (string_literal) @path)]))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument . ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#match? @ann "^(Get|Post|Put|Delete|Patch)Exchange$")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + ${arrayOfArg('@path')}))))) + (simple_identifier) @method_name) @method + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: HTTP Interface type-level @HttpExchange(url) prefix ───── + const SPRING_HTTP_EXCHANGE_CLASS_PATTERNS = compilePatterns({ + name: 'kotlin-spring-http-exchange-class', + language, + patterns: [ + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument . [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + [(string_literal) @prefix (collection_literal (string_literal) @prefix)])))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument . ${arrayOfArg('@prefix')})))))) @class + `, + }, + { + meta: {}, + query: ` + (class_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "HttpExchange")) + (value_arguments + (value_argument + (simple_identifier) @key (#match? @key "^(url|value)$") + ${arrayOfArg('@prefix')})))))) @class + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Consumer: OpenFeign native @RequestLine("VERB /path") ──────────── + // Two patterns mirror the positional vs named split. java.ts accepts the + // named `value` argument (java.ts:442 drops any non-`value` key); the + // positional pattern's `.` anchor only matches when the string literal is the + // first argument, so the named form needs its own pattern. Constraining + // `#eq? @key "value"` keeps non-`value` keys (`name`, etc.) dropped — Java + // parity, just enforced in the query rather than the JS loop. + const SPRING_REQUEST_LINE_PATTERNS = compilePatterns({ + name: 'kotlin-spring-request-line', + language, + patterns: [ + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestLine")) + (value_arguments + (value_argument . (string_literal) @value))))) + (simple_identifier) @method_name) @method + `, + }, + { + meta: {}, + query: ` + (function_declaration + (modifiers + (annotation + (constructor_invocation + (user_type (type_identifier) @ann (#eq? @ann "RequestLine")) + (value_arguments + (value_argument + (simple_identifier) @key (#eq? @key "value") + (string_literal) @value))))) + (simple_identifier) @method_name) @method + `, + }, + ], + } satisfies LanguagePatterns>); + + // ─── Provider via interface inheritance (Spring interface-based controller) ─ + // Pattern: `@RestController class X(...) : XApi` where the route annotations + // live on the `XApi` interface and the controller's `override fun` carries + // none. Java resolves this in `scanProject` (scanSpringProject); this is the + // Kotlin port. tree-sitter-kotlin models BOTH class and interface as + // `class_declaration`; the `interface` keyword token distinguishes them. + const KOTLIN_TYPE_DECLARATION_PATTERNS = compilePatterns({ + name: 'kotlin-type-declaration', + language, + patterns: [{ meta: {}, query: `(class_declaration (type_identifier) @name) @type` }], + } satisfies LanguagePatterns>); + + /** A `class_declaration` is an interface when it carries the `interface` keyword token. */ + const isKotlinInterface = (node: Parser.SyntaxNode): boolean => + node.children.some((c) => c.type === 'interface'); + + /** Resolve an `annotation` node's simple name: `@Foo` / `@Foo(...)` / `@a.b.Foo` → "Foo". */ + const kotlinAnnotationName = (annotation: Parser.SyntaxNode): string | null => { + const direct = annotation.namedChildren.find((c) => c.type === 'user_type'); + const ctor = annotation.namedChildren.find((c) => c.type === 'constructor_invocation'); + const userType = direct ?? ctor?.namedChildren.find((c) => c.type === 'user_type'); + // A fully-qualified annotation (`@a.b.Foo`) parses to a `user_type` carrying + // one `type_identifier` per dotted segment (`a`, `b`, `Foo`); the trailing + // one is the simple name. Taking the FIRST would resolve `@org…RestController` + // to "org" and miss the controller. + const idents = userType?.namedChildren.filter((c) => c.type === 'type_identifier') ?? []; + const ident = idents.at(-1); + return ident ? ident.text : null; + }; /** - * Join a class-level prefix and a method-level path. Identical - * semantics to the Java plugin: strip leading/trailing slashes on - * the prefix, strip leading slashes on the method path, ensure a - * single slash between them. + * Whether a `class_declaration` is a Spring `@RestController` / `@Controller`. + * All forms attach under `modifiers` as an `annotation` (confirmed against + * tree-sitter-kotlin fwcd): the bare `@RestController`, the common + * `@RestController @RequestMapping("/x")` pair, AND the arg-form + * `@RestController("beanName")` — the last parses to an `annotation` whose + * child is a `constructor_invocation` (NOT a detached sibling), which + * `kotlinAnnotationName` reads. A single pass over `modifiers` covers them all. */ - function joinPath(prefix: string, methodPath: string): string { - const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); - const cleanSub = methodPath.replace(/^\/+/, ''); - if (!cleanPrefix) return `/${cleanSub}`; - return `/${cleanPrefix}/${cleanSub}`; - } + const CONTROLLER_ANNOTATIONS = new Set(['RestController', 'Controller']); + const kotlinClassIsController = (typeNode: Parser.SyntaxNode): boolean => { + const modifiers = typeNode.namedChildren.find((c) => c.type === 'modifiers'); + for (const ann of modifiers?.namedChildren ?? []) { + if (ann.type !== 'annotation') continue; + const name = kotlinAnnotationName(ann); + if (name && CONTROLLER_ANNOTATIONS.has(name)) return true; + } + return false; + }; + + /** Supertype names from `: A, B` (`delegation_specifier` → `user_type` → `type_identifier`). */ + const collectKotlinSupertypes = (node: Parser.SyntaxNode): string[] => { + const out: string[] = []; + for (const child of node.namedChildren) { + if (child.type !== 'delegation_specifier') continue; + const userType = child.namedChildren.find((c) => c.type === 'user_type'); + // FQN supertype (`: a.b.Api`) → one `type_identifier` per segment; the + // trailing one is the simple name (taking the first would yield "a"). + const idents = userType?.namedChildren.filter((c) => c.type === 'type_identifier') ?? []; + const ident = idents.at(-1); + if (ident) out.push(ident.text); + } + return out; + }; + + /** Direct `function_declaration` members of a type (no descent into nested types). */ + const collectKotlinDirectMethods = (typeNode: Parser.SyntaxNode): Parser.SyntaxNode[] => { + const body = typeNode.namedChildren.find((c) => c.type === 'class_body'); + if (!body) return []; + return body.namedChildren.filter((c) => c.type === 'function_declaration'); + }; + + const kotlinFunctionName = (fn: Parser.SyntaxNode): string | null => + fn.namedChildren.find((c) => c.type === 'simple_identifier')?.text ?? null; + + const collectKotlinSpringTypes = (filePath: string, tree: Parser.Tree): SharedSpringType[] => { + // Class-level @RequestMapping prefixes (reuse the provider class-prefix query). + const prefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_CLASS_PREFIX_PATTERNS, tree)) { + const prefixNode = match.captures.prefix; + const classNode = match.captures.class; + if (!prefixNode || !classNode) continue; + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(prefixByClassId, classNode.id, prefix); + } + // Method @(Get|...)Mapping routes keyed by the function_declaration node id. + const routesByMethodId = new Map>(); + for (const match of runCompiledPatterns(SPRING_METHOD_ROUTE_PATTERNS, tree)) { + const annNode = match.captures.ann; + const pathNode = match.captures.path; + const methodNode = match.captures.method; + if (!annNode || !pathNode || !methodNode) continue; + const httpMethod = METHOD_ANNOTATION_TO_HTTP[annNode.text]; + if (!httpMethod) continue; + const rawPath = unquoteLiteral(pathNode.text); + if (rawPath === null) continue; + const arr = routesByMethodId.get(methodNode.id) ?? []; + arr.push({ method: httpMethod, path: rawPath }); + routesByMethodId.set(methodNode.id, arr); + } + + const out: SharedSpringType[] = []; + for (const match of runCompiledPatterns(KOTLIN_TYPE_DECLARATION_PATTERNS, tree)) { + const typeNode = match.captures.type; + const nameNode = match.captures.name; + if (!typeNode || !nameNode) continue; + const kind = isKotlinInterface(typeNode) ? 'interface' : 'class'; + const methods = collectKotlinDirectMethods(typeNode) + .map((fn) => ({ name: kotlinFunctionName(fn), routes: routesByMethodId.get(fn.id) ?? [] })) + .filter((m): m is { name: string; routes: Array<{ method: string; path: string }> } => { + return m.name !== null; + }); + out.push({ + filePath, + kind, + name: nameNode.text, + isController: kind === 'class' ? kotlinClassIsController(typeNode) : false, + classPrefixes: prefixByClassId.get(typeNode.id) ?? [], + implementedInterfaces: kind === 'class' ? collectKotlinSupertypes(typeNode) : [], + methods, + }); + } + return out; + }; + + // The interface-based-controller inheritance algorithm is shared with java.ts + // (`scanSpringInheritanceProject`); this collects the language-specific + // `SharedSpringType` view and delegates. kotlinClassIsController handles every + // controller form (bare, paired, and the arg-form `@RestController("bean")`) + // via the `modifiers` `annotation`/`constructor_invocation` shape. + const scanKotlinProject = (files: readonly HttpScanInput[]): HttpFileDetections[] => + scanSpringInheritanceProject( + files.flatMap((f) => collectKotlinSpringTypes(f.filePath, f.tree)), + ); return { name: 'kotlin-http', @@ -433,16 +829,42 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { const out: HttpDetection[] = []; // ─── Class prefixes ───────────────────────────────────────────── - const prefixByClassId = new Map(); + const prefixByClassId = new Map(); for (const match of runCompiledPatterns(SPRING_CLASS_PREFIX_PATTERNS, tree)) { const prefixNode = match.captures.prefix; const classNode = match.captures.class; if (!prefixNode || !classNode) continue; const prefix = unquoteLiteral(prefixNode.text); - if (prefix !== null) prefixByClassId.set(classNode.id, prefix); + if (prefix !== null) pushPrefix(prefixByClassId, classNode.id, prefix); } - // ─── Method routes ────────────────────────────────────────────── + // ─── OpenFeign client interfaces + HTTP Interface type prefixes ── + // In tree-sitter-kotlin an `interface` is a `class_declaration`, so a + // `@FeignClient` interface's @(Get|...)Mapping methods would otherwise be + // mis-emitted as providers. Collect the FeignClient class ids (and their + // optional `path` prefix) so the method-route loop can reclassify them. + const feignClassIds = new Set(); + const feignPrefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_FEIGN_CLIENT_PATTERNS, tree)) { + const classNode = match.captures.class; + if (!classNode) continue; + feignClassIds.add(classNode.id); + const prefixNode = match.captures.prefix; + if (prefixNode) { + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(feignPrefixByClassId, classNode.id, prefix); + } + } + const httpExchangePrefixByClassId = new Map(); + for (const match of runCompiledPatterns(SPRING_HTTP_EXCHANGE_CLASS_PATTERNS, tree)) { + const classNode = match.captures.class; + const prefixNode = match.captures.prefix; + if (!classNode || !prefixNode) continue; + const prefix = unquoteLiteral(prefixNode.text); + if (prefix !== null) pushPrefix(httpExchangePrefixByClassId, classNode.id, prefix); + } + + // ─── Method routes (Spring providers) + OpenFeign consumers ───── for (const match of runCompiledPatterns(SPRING_METHOD_ROUTE_PATTERNS, tree)) { const annNode = match.captures.ann; const pathNode = match.captures.path; @@ -454,16 +876,45 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { const rawPath = unquoteLiteral(pathNode.text); if (rawPath === null) continue; const enclosingClass = findEnclosingClass(methodNode); - const prefix = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? '') : ''; - const fullPath = joinPath(prefix, rawPath); - out.push({ - role: 'provider', - framework: 'spring', - method: httpMethod, - path: fullPath, - name: nameNode?.text ?? null, - confidence: 0.8, - }); + // A @(Get|...)Mapping inside a @FeignClient interface is an OpenFeign + // consumer (a remote call), not a route this service serves. + if (enclosingClass && feignClassIds.has(enclosingClass.id)) { + // @FeignClient(path) wins over @RequestMapping; a multi-element prefix + // yields one consumer per (prefix × this route). + const prefixes = feignPrefixByClassId.get(enclosingClass.id) ?? + prefixByClassId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: FEIGN_CONFIDENCE, + }); + } + continue; + } + // A @(Get|...)Mapping on a (non-Feign) interface declares a route + // *contract*, not a route this service serves — the implementing + // @RestController is the provider, emitted via scanProject's interface + // inheritance. Java drops these implicitly (findEnclosingClass returns + // null for an interface_declaration); tree-sitter-kotlin models an + // interface as a class_declaration, so skip it explicitly here. + if (enclosingClass && isKotlinInterface(enclosingClass)) continue; + // A multi-element class `@RequestMapping(["/a","/b"])` registers the method + // under each prefix — emit one provider per (prefix × this route). + const prefixes = enclosingClass ? (prefixByClassId.get(enclosingClass.id) ?? ['']) : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'provider', + framework: 'spring', + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: 0.8, + }); + } } // ─── Consumers: RestTemplate ──────────────────────────────────── @@ -547,8 +998,74 @@ function buildKotlinPlugin(language: unknown): HttpLanguagePlugin { }); } + // ─── Consumers: Spring HTTP Interface @(Get|...)Exchange ──────── + for (const match of runCompiledPatterns(SPRING_EXCHANGE_PATTERNS, tree)) { + const annNode = match.captures.ann; + const pathNode = match.captures.path; + const nameNode = match.captures.method_name; + const methodNode = match.captures.method; + if (!annNode || !pathNode || !methodNode) continue; + const httpMethod = EXCHANGE_ANNOTATION_TO_HTTP[annNode.text]; + if (!httpMethod) continue; + const rawPath = unquoteLiteral(pathNode.text); + if (rawPath === null) continue; + const enclosingClass = findEnclosingClass(methodNode); + const prefixes = enclosingClass + ? (httpExchangePrefixByClassId.get(enclosingClass.id) ?? ['']) + : ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: HTTP_INTERFACE_FRAMEWORK, + method: httpMethod, + path: joinPath(prefix, rawPath), + name: nameNode?.text ?? null, + confidence: EXCHANGE_CONFIDENCE, + }); + } + } + + // ─── Consumers: OpenFeign native @RequestLine("VERB /path") ───── + // Method-level only and always declared on an interface — Feign builds its + // proxy from the interface, so a `@RequestLine` on a concrete class is not + // a client call. We do NOT require an enclosing `@FeignClient` (core Feign + // uses `@RequestLine` with `Feign.builder()`, not Spring Cloud's + // `@FeignClient`); the `RequestLine` name plus the structural interface + // check keep false positives away. Mirrors java.ts's `findEnclosingInterface` + // gate — in tree-sitter-kotlin an interface is a `class_declaration`, so we + // test the `interface` keyword via isKotlinInterface. + for (const match of runCompiledPatterns(SPRING_REQUEST_LINE_PATTERNS, tree)) { + const valueNode = match.captures.value; + const nameNode = match.captures.method_name; + const methodNode = match.captures.method; + if (!valueNode || !methodNode) continue; + const raw = unquoteLiteral(valueNode.text); + const parsed = raw !== null ? parseRequestLine(raw) : null; + if (!parsed) continue; + const enclosingClass = findEnclosingClass(methodNode); + if (!enclosingClass || !isKotlinInterface(enclosingClass)) continue; + // Mirror java.ts (which pre-merges the @RequestMapping fallback into + // feignPrefixByInterfaceId, "path wins"): @FeignClient(path) wins, else + // the interface's class-level @RequestMapping prefix, else none. Without + // the prefixByClassId fallback Kotlin dropped the class prefix that Java + // applies — the same fallback chain the @GetMapping-in-Feign path uses above. + const prefixes = feignPrefixByClassId.get(enclosingClass.id) ?? + prefixByClassId.get(enclosingClass.id) ?? ['']; + for (const prefix of prefixes) { + out.push({ + role: 'consumer', + framework: OPENFEIGN_FRAMEWORK, + method: parsed.method, + path: joinPath(prefix, parsed.path), + name: nameNode?.text ?? null, + confidence: REQUEST_LINE_CONFIDENCE, + }); + } + } + return out; }, + scanProject: scanKotlinProject, }; } diff --git a/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts b/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts new file mode 100644 index 000000000..f42894da6 --- /dev/null +++ b/gitnexus/src/core/group/extractors/http-patterns/spring-consumer-shared.ts @@ -0,0 +1,269 @@ +/** + * Shared, language-agnostic primitives for Spring / OpenFeign / Spring-HTTP-Interface + * HTTP *consumer* extraction, used by BOTH the Java (`java.ts`) and Kotlin + * (`kotlin.ts`) group-layer HTTP plugins so the two cannot drift apart. + * + * These are pure value maps + string helpers — no tree-sitter AST knowledge. + * Each language plugin keeps its own grammar-specific queries and walkers and + * funnels the extracted (verb, path, prefix) facts through these helpers, so a + * polyglot repo emits byte-identical contract IDs from `.java` and `.kt`. + * + * The provider-side annotation→verb map (`METHOD_ANNOTATION_TO_HTTP`), + * `isRouteMemberKey`, and `findEnclosingClass` live in the lower + * `ingestion/route-extractors/spring-shared.ts` (shared with the ingestion + * route extractor). This module is the consumer-side counterpart and lives in + * the group layer beside the plugins that use it. + */ + +import type { HttpDetection, HttpFileDetections } from './types.js'; + +/** + * RestTemplate method-name → HTTP verb. Source-scan only: the receiver must be + * named exactly `restTemplate` (the per-language query enforces that). + */ +export const REST_TEMPLATE_TO_HTTP: Record = { + getForObject: 'GET', + getForEntity: 'GET', + postForObject: 'POST', + postForEntity: 'POST', + put: 'PUT', + delete: 'DELETE', + patchForObject: 'PATCH', +}; + +/** + * Reactive WebClient short-form verb helper → HTTP verb + * (`webClient.get().uri("/x")`, `.post()`, ...). HEAD/OPTIONS/TRACE are + * intentionally excluded for symmetry across the plugins. + */ +export const WEB_CLIENT_SHORT_TO_HTTP: Record = { + get: 'GET', + post: 'POST', + put: 'PUT', + delete: 'DELETE', + patch: 'PATCH', +}; + +/** + * Accepted HTTP verbs for the WebClient long form + * `webClient.method(HttpMethod.X).uri("/y")`. The verb is captured as the + * literal `HttpMethod.X` field name (`GET`, `POST`, …); HEAD/OPTIONS/TRACE are + * intentionally excluded for symmetry with the short form + * (`WEB_CLIENT_SHORT_TO_HTTP`). Shared so the Java and Kotlin long-form scans + * gate verbs identically. + */ +export const WEB_CLIENT_LONG_VERB_RE = /^(GET|POST|PUT|DELETE|PATCH)$/; + +/** + * Spring 6 declarative HTTP Interface shortcut annotation → HTTP verb. + * + * `@GetExchange`/`@PostExchange`/… on a service interface proxied by + * `HttpServiceProxyFactory` (over RestClient / WebClient / RestTemplate) + * describe an OUTBOUND call — i.e. a CONSUMER, the modern analogue of an + * OpenFeign `@(Get|Post|...)Mapping` interface method. The path lives in the + * annotation's `url` (or `value`) attribute, or positionally. + * + * The base `@HttpExchange(method = "GET", url = "...")` form carries its verb + * in an attribute rather than the annotation name; the shortcut annotations + * above are the overwhelmingly common case and the only ones mapped here. + */ +export const EXCHANGE_ANNOTATION_TO_HTTP: Record = { + GetExchange: 'GET', + PostExchange: 'POST', + PutExchange: 'PUT', + DeleteExchange: 'DELETE', + PatchExchange: 'PATCH', +}; + +/** + * Accumulate a route prefix (de-duped) under a class/interface declaration node + * id. A Spring route attribute is `String[]`; a multi-element array (`["/a","/b"]`, + * `arrayOf("/a","/b")`, `{"/a","/b"}`) yields one query match per element, so + * prefixes accumulate rather than overwrite. Shared by the Java and Kotlin plugins + * so both build their prefix maps identically. + */ +export const pushPrefix = (map: Map, id: number, prefix: string): void => { + const arr = map.get(id) ?? []; + if (!arr.includes(prefix)) arr.push(prefix); + map.set(id, arr); +}; + +/** Framework tags emitted on consumer detections (stable contract metadata). */ +export const OPENFEIGN_FRAMEWORK = 'openfeign'; +export const HTTP_INTERFACE_FRAMEWORK = 'spring-http-interface'; + +/** Consumer-detection confidences, shared so `.java` and `.kt` agree. */ +export const FEIGN_CONFIDENCE = 0.7; +export const REQUEST_LINE_CONFIDENCE = 0.75; +export const EXCHANGE_CONFIDENCE = 0.75; + +/** + * OpenFeign's native `@RequestLine("METHOD /path[?query]")` packs an HTTP + * method and path in a single string literal — see + * https://github.com/OpenFeign/feign#interface-annotations. This regex splits + * the verb from the path of that literal. + */ +export const REQUEST_LINE_VERB_RE = /^\s*(GET|POST|PUT|DELETE|PATCH|HEAD|OPTIONS)\s+(\S.*?)\s*$/i; + +/** + * Parse a Feign `@RequestLine` value into a method + path pair. The query + * portion is dropped because contract IDs are method+path only (consistent + * with how RestTemplate/WebClient consumers drop inline query strings). + * + * Returns null if the value is not a recognized HTTP verb followed by a path + * beginning with `/`. + */ +export function parseRequestLine(raw: string): { method: string; path: string } | null { + const match = REQUEST_LINE_VERB_RE.exec(raw); + if (!match) return null; + const [, verb, rest] = match; + if (typeof verb !== 'string' || typeof rest !== 'string') return null; + const queryIdx = rest.indexOf('?'); + const pathOnly = (queryIdx >= 0 ? rest.slice(0, queryIdx) : rest).trim(); + if (!pathOnly.startsWith('/')) return null; + return { method: verb.toUpperCase(), path: pathOnly }; +} + +/** + * Join a class/interface-level prefix and a method-level path into a single + * URL path: strip leading/trailing slashes on the prefix and leading slashes + * on the method path, then ensure exactly one slash between them. + */ +export function joinPath(prefix: string, methodPath: string): string { + const cleanPrefix = prefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanSub = methodPath.replace(/^\/+/, ''); + if (!cleanPrefix) return `/${cleanSub}`; + return `/${cleanPrefix}/${cleanSub}`; +} + +/** + * Join a controller's own class prefix with a route inherited from an interface + * (interface-based controllers, #1743). The inherited path already has the + * interface's own class prefix (`inheritedOwnerPrefix`) baked in; when the + * controller repeats that same prefix we must NOT prepend it twice (#2057). + * Shared by both plugins' `scanProject` so Java and Kotlin agree. + */ +export function joinInheritedSpringPath( + controllerPrefix: string, + inheritedPath: string, + inheritedOwnerPrefix = '', +): string { + const joined = joinPath(controllerPrefix, inheritedPath); + const cleanPrefix = controllerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanOwnerPrefix = inheritedOwnerPrefix.replace(/^\/+/, '').replace(/\/+$/, ''); + const cleanInherited = inheritedPath.replace(/^\/+/, ''); + if (!cleanPrefix) return joined; + if ( + cleanPrefix === cleanOwnerPrefix && + (cleanInherited === cleanPrefix || cleanInherited.startsWith(`${cleanPrefix}/`)) + ) { + return `/${cleanInherited}`; + } + return joined; +} + +/** + * Language-agnostic view of a Spring class/interface that each plugin's + * grammar-specific collector produces. The interface-based-controller + * inheritance algorithm (`scanSpringInheritanceProject`) operates only on this + * shape, so the Java and Kotlin plugins share one algorithm and cannot drift. + * + * `methods[].routes` carry only `{ method, path }` — the interface's own class + * prefix is applied *inside* `scanSpringInheritanceProject` (it is not part of + * the collector's output). + */ +export interface SharedSpringType { + filePath: string; + kind: 'class' | 'interface'; + name: string; + /** Class-level `@RequestMapping` prefixes — one per array element. */ + classPrefixes: string[]; + implementedInterfaces: string[]; + isController: boolean; + methods: Array<{ name: string; routes: Array<{ method: string; path: string }> }>; +} + +/** + * Resolve interface-based-controller provider routes (#1743): a concrete + * `@RestController`/`@Controller` class inherits the `@(Get|...)Mapping` routes + * declared on the interface it implements. Shared by the Java and Kotlin plugins + * so both emit byte-identical provider contracts. + * + * An interface name that resolves to two distinct interfaces is ambiguous and + * its routes are dropped (the `null` marker). The controller's own class + * prefix(es) cross-product the inherited routes; `joinInheritedSpringPath` + * avoids doubling a prefix the interface already baked in (#2057). + */ +export function scanSpringInheritanceProject(types: SharedSpringType[]): HttpFileDetections[] { + // interface name → (method name → routes). `ownerPrefix` records the + // interface's own class prefix so the controller side avoids doubling it + // (#2057). `null` marks an ambiguous (duplicated) interface name. + type InheritedRoute = { method: string; path: string; ownerPrefix: string }; + const interfaceRoutes = new Map | null>(); + for (const type of types) { + if (type.kind !== 'interface') continue; + if (interfaceRoutes.has(type.name)) { + interfaceRoutes.set(type.name, null); + continue; + } + const prefixes = type.classPrefixes.length ? type.classPrefixes : ['']; + const methodMap = new Map(); + for (const method of type.methods) { + // Cross-product the interface's class prefixes with each method route, so a + // multi-element `@RequestMapping(["/a","/b"])` interface yields N bindings. + const routes = method.routes.flatMap((route) => + prefixes.map((prefix) => ({ + method: route.method, + path: prefix ? joinPath(prefix, route.path) : route.path, + ownerPrefix: prefix, + })), + ); + if (routes.length > 0) methodMap.set(method.name, routes); + } + interfaceRoutes.set(type.name, methodMap); + } + + const detectionsByFile = new Map(); + for (const type of types) { + if (type.kind !== 'class' || !type.isController) continue; + // Cross-product the controller's own class prefixes with each inherited + // route; `['']` keeps the common no-prefix controller emitting the + // interface path unchanged. + const controllerPrefixes = type.classPrefixes.length ? type.classPrefixes : ['']; + for (const method of type.methods) { + if (method.routes.length > 0) continue; // own @*Mapping → already a provider via scan() + const inherited = type.implementedInterfaces.flatMap((iface) => { + const routeMap = interfaceRoutes.get(iface); + if (!routeMap) return []; + const routes = routeMap.get(method.name) ?? []; + return routes.flatMap((route) => + controllerPrefixes.map((controllerPrefix) => ({ + method: route.method, + path: joinInheritedSpringPath(controllerPrefix, route.path, route.ownerPrefix), + })), + ); + }); + const seen = new Set(); + for (const route of inherited) { + const key = `${route.method} ${route.path}`; + if (seen.has(key)) continue; + seen.add(key); + const detections = detectionsByFile.get(type.filePath) ?? []; + detections.push({ + role: 'provider', + framework: 'spring', + method: route.method, + path: route.path, + name: method.name, + confidence: 0.8, + }); + detectionsByFile.set(type.filePath, detections); + } + } + } + + return [...detectionsByFile.entries()].map(([filePath, detections]) => ({ + filePath, + detections, + })); +} diff --git a/gitnexus/test/unit/group/http-route-extractor.test.ts b/gitnexus/test/unit/group/http-route-extractor.test.ts index 52e6f8a49..2d688c7e8 100644 --- a/gitnexus/test/unit/group/http-route-extractor.test.ts +++ b/gitnexus/test/unit/group/http-route-extractor.test.ts @@ -950,6 +950,8 @@ public class UserController implements UserApi { expect(toPosixPath(usersRoute!.symbolRef.filePath)).toBe( 'src/controller/UserController.java', ); + expect(usersRoute!.meta.framework).toBe('spring'); + expect(usersRoute!.confidence).toBe(0.8); }); it('does not duplicate inherited Spring prefixes already present on the controller', async () => { @@ -1156,6 +1158,37 @@ public class StatusController implements StatusApi { ).toHaveLength(0); }); + it('does not extract fully-qualified Java route annotations (documented limitation #2254)', async () => { + // JAVA_ROUTE_ANNOTATION_PATTERNS binds `name: (identifier)`; a FQN route + // annotation parses its name as `scoped_identifier` and is not matched, so + // its route is not extracted (only the route string — the controller itself + // is still recognised). This pins that documented limitation / asymmetry + // with Kotlin. If FQN matching is ever added, flip this assertion. + const dir = path.join(tmpDir, 'java-fqn-route-annotation'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'FqnController.java'), + ` +@org.springframework.web.bind.annotation.RestController +@org.springframework.web.bind.annotation.RequestMapping("/api") +class FqnController { + @org.springframework.web.bind.annotation.GetMapping("/users") + Object users() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // The FQN route annotation is not extracted (documented limitation). + expect(providers.find((c) => c.contractId === 'http::GET::/api/users')).toBeUndefined(); + // And it must not over-match into a bogus contract either. + expect( + providers.filter((c) => c.symbolRef.filePath.endsWith('FqnController.java')), + ).toHaveLength(0); + }); + it('extracts Express router.get patterns', async () => { const dir = path.join(tmpDir, 'express'); fs.mkdirSync(path.join(dir, 'src/routes'), { recursive: true }); @@ -1738,7 +1771,10 @@ class ApiClient { ).toBeDefined(); }); - it('does NOT match Java WebClient long-form method(HttpMethod).uri(...) yet', async () => { + it('extracts Java WebClient long-form method(HttpMethod.X).uri(...) — #2254 parity', async () => { + // Parity with the Kotlin plugin: a single structural query matches the + // verb (HttpMethod.X field access) and path. Previously deferred on the + // Java side; PR #2254 lifts it so .java and .kt detect it identically. const dir = path.join(tmpDir, 'java-web-client-long-form'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); fs.writeFileSync( @@ -1749,6 +1785,10 @@ import org.springframework.web.reactive.function.client.WebClient; class LongFormClient { void run(WebClient webClient) { + webClient.method(HttpMethod.GET).uri("/api/get").retrieve(); + webClient.method(HttpMethod.POST).uri("/api/post").retrieve(); + webClient.method(HttpMethod.PUT).uri("/api/put").retrieve(); + webClient.method(HttpMethod.DELETE).uri("/api/delete").retrieve(); webClient.method(HttpMethod.PATCH).uri("/api/users/42").retrieve(); } } @@ -1758,11 +1798,105 @@ class LongFormClient { const contracts = await extractor.extract(null, dir, makeRepo(dir)); const consumers = contracts.filter((c) => c.role === 'consumer'); + for (const [verb, p] of [ + ['GET', '/api/get'], + ['POST', '/api/post'], + ['PUT', '/api/put'], + ['DELETE', '/api/delete'], + ['PATCH', '/api/users/{param}'], + ]) { + expect( + consumers.find( + (c) => + c.contractId === `http::${verb}::${p}` && + c.meta.framework === 'spring-web-client' && + c.confidence === 0.7, + ), + ).toBeDefined(); + } + // No double-emit: the short-form query cannot also fire on the long form. + expect(consumers.filter((c) => c.contractId === 'http::GET::/api/get')).toHaveLength(1); + }); + + it('does NOT match Java WebClient long-form with a variable-bound verb', async () => { + // The value carries a bare identifier, not a HttpMethod.X field access — + // source-scan can't follow the binding (anti-overreach, parity with Kotlin). + const dir = path.join(tmpDir, 'java-web-client-long-form-var'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'VarVerbClient.java'), + ` +import org.springframework.http.HttpMethod; +import org.springframework.web.reactive.function.client.WebClient; + +class VarVerbClient { + void run(WebClient webClient, HttpMethod verb) { + webClient.method(verb).uri("/api/users/42").retrieve(); + } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + expect( - consumers.find((c) => c.contractId === 'http::PATCH::/api/users/{param}'), + consumers.find( + (c) => c.contractId.startsWith('http::') && c.contractId.includes('/api/users'), + ), ).toBeUndefined(); }); + it('handles Java array-form annotation paths ({"/x"}, key = {"/x"})', async () => { + const dir = path.join(tmpDir, 'java-array-paths'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: array class prefix + array method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductController.java'), + ` +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RestController +@RequestMapping({"/api/products"}) +class ProductController { + @GetMapping(path = {"/{id}"}) + Product get(Integer id) { return null; } +} +`, + ); + // OpenFeign consumer with array positional path. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.java'), + ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.PostMapping; + +@FeignClient(name = "orders") +interface OrdersClient { + @PostMapping({"/orders/search"}) + Object search(); +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Array class prefix + array method path → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient with array positional path → consumer. + expect( + consumers.find( + (c) => c.contractId === 'http::POST::/orders/search' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + }); + it('extracts OpenFeign clients as consumers, not providers', async () => { const dir = path.join(tmpDir, 'java-openfeign-consumer'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -1805,6 +1939,46 @@ interface OrderClient { ).toBeUndefined(); }); + it('extracts Spring HTTP Interface @(Get|...)Exchange clients as consumers', async () => { + const dir = path.join(tmpDir, 'java-http-exchange-consumer'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryApi.java'), + ` +import org.springframework.web.service.annotation.HttpExchange; +import org.springframework.web.service.annotation.GetExchange; +import org.springframework.web.service.annotation.PostExchange; + +@HttpExchange(url = "/items") +interface InventoryApi { + @GetExchange(url = "/{id}") + Item getItem(Integer id); + + @PostExchange("/search") + Page search(ItemFilter query); +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'spring-http-interface' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + // Declarative HTTP-interface methods are consumers, never providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryApi.java')), + ).toBeUndefined(); + }); + it('extracts OpenFeign clients without an interface path prefix', async () => { const dir = path.join(tmpDir, 'java-openfeign-no-prefix'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -2251,6 +2425,64 @@ interface ReversedPrecedenceClient { expect(consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders')).toBeUndefined(); }); + it('a @FeignClient API interface implemented by a controller yields both a consumer and a provider (Java)', async () => { + // Java twin of the Kotlin dual-role case: an `api` module publishes a + // @FeignClient contract (consumer) that the service's @RestController + // implements (provider). + const dir = path.join(tmpDir, 'java-feign-api-implemented'); + fs.mkdirSync(path.join(dir, 'src/rest'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src/controller'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src/rest/WarehouseApi.java'), + ` +package com.example.rest; +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.*; + +@FeignClient(name = "catalog-service") +@RequestMapping("/warehouses") +public interface WarehouseApi { + @GetMapping("/{id}/stock") + Object listStock(); +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src/controller/WarehouseController.java'), + ` +package com.example.controller; +import com.example.rest.WarehouseApi; +import org.springframework.web.bind.annotation.*; + +@RestController +public class WarehouseController implements WarehouseApi { + @Override + public Object listStock() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.java') && + c.confidence === 0.8, + ), + ).toBeDefined(); + }); + it('extracts Java and Apache HttpClient literal request construction', async () => { const dir = path.join(tmpDir, 'java-http-client-consumer'); fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); @@ -2688,6 +2920,1903 @@ class CacheClient(private val cacheClient: SomeCache) { }, ); + // ─── Kotlin OpenFeign + Spring HTTP Interface consumers ────────────── + // `@FeignClient` interfaces (Spring MVC `@*Mapping` methods) and Spring 6 + // declarative HTTP Interfaces (`@(Get|...)Exchange`) are the dominant + // outbound-call patterns in Kotlin+Spring services. In tree-sitter-kotlin + // an `interface` is a `class_declaration`, so without a `@FeignClient` + // gate the `@*Mapping` methods would mis-classify as providers. + itKotlinConsumer( + 'extracts Kotlin @FeignClient methods as consumers, not providers', + async () => { + const dir = path.join(tmpDir, 'kotlin-feign-consumer'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "inventory-service", configuration = [InventoryFeignClientConfig::class]) +interface InventoryClient { + @GetMapping("items/{itemId}", consumes = [MediaType.APPLICATION_JSON_VALUE], produces = [MediaType.APPLICATION_JSON_VALUE]) + fun getItem(@PathVariable("itemId") itemId: Int): ItemDto + + @PostMapping("items/search", consumes = [MediaType.APPLICATION_JSON_VALUE]) + fun getItems(@RequestBody query: ItemFilter): Page +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'openfeign' && + c.confidence === 0.7, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + // The Feign interface methods must NOT leak into providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryClient.kt')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer('applies @FeignClient(path) and @RequestMapping prefixes', async () => { + // One interface per file — the real-world layout (e.g. InventoryClient.kt). + const dir = path.join(tmpDir, 'kotlin-feign-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'PrecedenceClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@FeignClient(name = "a", path = "/feign-path") +@RequestMapping("/rm-path") +interface PrecedenceClient { + @GetMapping("/orders") + fun getOrders(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@FeignClient(name = "b") +@RequestMapping(path = "/api") +interface InventoryClient { + @GetMapping("/inventory/{id}") + fun getInventory(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @FeignClient(path) wins over @RequestMapping. + expect(consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders')).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders')).toBeUndefined(); + // @RequestMapping is the fallback prefix when there is no @FeignClient(path). + expect( + consumers.find((c) => c.contractId === 'http::GET::/api/inventory/{param}'), + ).toBeDefined(); + }); + + itKotlinConsumer( + 'extracts Kotlin Spring HTTP Interface @(Get|...)Exchange consumers', + async () => { + const dir = path.join(tmpDir, 'kotlin-http-exchange'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'InventoryApi.kt'), + `package com.example +import org.springframework.web.service.annotation.GetExchange +import org.springframework.web.service.annotation.PostExchange +import org.springframework.web.service.annotation.PutExchange +import org.springframework.web.service.annotation.PatchExchange +import org.springframework.web.service.annotation.DeleteExchange + +interface InventoryApi { + @GetExchange(url = "/items/{itemId}", accept = [MediaType.APPLICATION_JSON_VALUE]) + fun obtainItem(@PathVariable itemId: Int): Any + + @PostExchange(url = "/items/search") + fun search(): Any + + @PutExchange(url = "/items") + fun create(): Any + + @PatchExchange(url = "/items/update/{itemId}") + fun update(@PathVariable itemId: Int): Any + + @DeleteExchange(url = "/items/{itemId}") + fun remove(@PathVariable itemId: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/items/{param}' && + c.meta.framework === 'spring-http-interface' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::POST::/items/search')).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::PUT::/items')).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::PATCH::/items/update/{param}'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::DELETE::/items/{param}'), + ).toBeDefined(); + // Declarative HTTP-interface methods are consumers, never providers. + expect( + providers.find((c) => c.symbolRef.filePath.endsWith('InventoryApi.kt')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'applies class-level @HttpExchange(url) prefix and a positional @GetExchange', + async () => { + const dir = path.join(tmpDir, 'kotlin-http-exchange-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ProductApi.kt'), + `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + fun get(@PathVariable id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/products/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer('extracts Kotlin OpenFeign native @RequestLine consumers', async () => { + const dir = path.join(tmpDir, 'kotlin-feign-request-line'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine("POST /ai/summarize") + fun summarize(): String + + @RequestLine("GET /ai/health") + fun health(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/ai/summarize' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect(consumers.find((c) => c.contractId === 'http::GET::/ai/health')).toBeDefined(); + }); + + itKotlinConsumer( + 'applies the @RequestMapping interface prefix to @RequestLine consumers (no @FeignClient path) — #2254 P2 parity', + async () => { + // Parity with java.ts, which merges the @RequestMapping prefix into + // feignPrefixByInterfaceId: an interface with @RequestMapping("/orders") + // and a @RequestLine method (no @FeignClient(path)) must apply the prefix. + // Kotlin previously dropped it (PR #2254 tri-review, kotlin.ts:978). + const dir = path.join(tmpDir, 'kotlin-request-line-rm-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'OrderClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + fun get(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/orders/{param}' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + // The un-prefixed form must NOT be emitted (the prefix was applied). + expect(consumers.find((c) => c.contractId === 'http::GET::/{param}')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'prefers @FeignClient(path) over @RequestMapping for @RequestLine consumers', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-feign-path-wins'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'OrderClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service", path = "/feign-path") +@RequestMapping("/rm-path") +interface OrderClient { + @RequestLine("GET /orders/{id}") + fun get(id: String): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @FeignClient(path) wins over @RequestMapping (parity with the @GetMapping path). + expect( + consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders/{param}'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders/{param}'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'extracts Kotlin @RequestLine written with the named "value" argument (#2254 P2)', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-named-value'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine(value = "POST /create") + fun create(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/create' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'ignores Kotlin @RequestLine whose named argument is not "value"', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-non-value-key'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "ai-backend") +interface AiClient { + @RequestLine(name = "GET /should-not-extract") + fun nope(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find((c) => c.contractId === 'http::GET::/should-not-extract'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'strips query strings from Kotlin @RequestLine values when forming contract IDs', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-query'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'SearchClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "search-service") +interface SearchClient { + @RequestLine("GET /search?q={query}&limit={limit}") + fun search(): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect(consumers.find((c) => c.contractId === 'http::GET::/search')).toBeDefined(); + expect( + consumers.find((c) => c.contractId.includes('?') || c.contractId.includes('limit')), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'mixes Kotlin @RequestLine and @GetMapping methods on the same @FeignClient interface', + async () => { + const dir = path.join(tmpDir, 'kotlin-feign-mixed-annotations'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MixedClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import feign.RequestLine + +@FeignClient(name = "mixed-service", path = "/api") +interface MixedClient { + @GetMapping("/spring-style") + fun springStyle(): String + + @RequestLine("GET /native-style") + fun nativeStyle(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // @GetMapping → @FeignClient(path) prefix; confidence 0.7. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/api/spring-style' && + c.meta.framework === 'openfeign' && + c.confidence === 0.7, + ), + ).toBeDefined(); + // @RequestLine → @FeignClient(path) prefix; confidence 0.75. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/api/native-style' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'ignores Kotlin @RequestLine values that are not a "VERB /path" line', + async () => { + const dir = path.join(tmpDir, 'kotlin-request-line-malformed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MalformedClient.kt'), + `package com.example +import feign.RequestLine + +interface MalformedClient { + @RequestLine("not a request line at all") + fun noVerb(): String + + @RequestLine("GET relative/no/leading/slash") + fun noLeadingSlash(): String + + @RequestLine("FETCH /unknown-verb") + fun unknownVerb(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.filter((c) => c.symbolRef.filePath.endsWith('MalformedClient.kt')), + ).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'prefers @FeignClient(path) over @RequestMapping when @RequestMapping appears first (Kotlin)', + async () => { + // Source-order independence twin: @FeignClient(path) wins even when + // @RequestMapping is the first annotation. + const dir = path.join(tmpDir, 'kotlin-feign-prefix-precedence-reversed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ReversedClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.web.bind.annotation.RequestMapping + +@RequestMapping("/rm-path") +@FeignClient(name = "order-service", path = "/feign-path") +interface ReversedClient { + @GetMapping("/orders") + fun getOrders(): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + expect( + consumers.find((c) => c.contractId === 'http::GET::/feign-path/orders'), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/rm-path/orders'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'classifies a @RestController class as provider and a @FeignClient interface as consumer', + async () => { + const dir = path.join(tmpDir, 'kotlin-controller-vs-feign'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'Mixed.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.GetMapping +import org.springframework.cloud.openfeign.FeignClient + +@RestController +class OrdersController { + @GetMapping("/orders/{id}") + fun getOrder(@PathVariable id: Int): Any = TODO() +} + +@FeignClient(name = "pricing") +interface PricingClient { + @GetMapping("/prices/{id}") + fun getPrice(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Controller method → provider (not a consumer). + expect( + providers.find( + (c) => c.contractId === 'http::GET::/orders/{param}' && c.meta.framework === 'spring', + ), + ).toBeDefined(); + expect( + consumers.find((c) => c.contractId === 'http::GET::/orders/{param}'), + ).toBeUndefined(); + // Feign interface method → consumer (not a provider). + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/prices/{param}' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + expect( + providers.find((c) => c.contractId === 'http::GET::/prices/{param}'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'emits a provider for a class implementing a route interface (interface-based controller)', + async () => { + // One interface per file (real layout). Routes live on the interface; the + // @RestController override carries none → inherited via scanProject. + const dir = path.join(tmpDir, 'kotlin-interface-based-controller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // The controller inherits the route declared on WarehouseApi → provider. + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt') && + c.meta.framework === 'spring' && + c.confidence === 0.8, + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'recognises a fully-qualified @org…RestController as a controller (#2254 FQN parity)', + async () => { + // A FQN annotation parses to a user_type with one type_identifier per + // segment; the controller gate must read the trailing segment, not "org". + const dir = path.join(tmpDir, 'kotlin-fqn-controller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example + +@org.springframework.web.bind.annotation.RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'resolves a fully-qualified supertype to its trailing segment for interface inheritance', + async () => { + // `: com.example.WarehouseApi` must resolve to "WarehouseApi" (trailing + // segment), not "com", so the inherited interface route is matched. + const dir = path.join(tmpDir, 'kotlin-fqn-supertype'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : com.example.WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'a @FeignClient API interface implemented by a controller yields both a consumer and a provider', + async () => { + // catalog-service pattern: an `api` module publishes a @FeignClient contract that + // the service's own @RestController implements. The interface is the + // client SDK (consumer); the implementing controller is the provider. + const dir = path.join(tmpDir, 'kotlin-feign-api-implemented'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@FeignClient(name = "catalog-service") +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Interface (published @FeignClient client SDK) → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // Implementing @RestController → provider (route inherited from the interface). + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'handles Kotlin array-form paths (["/x"], value = ["/x"]) for providers and consumers', + async () => { + // Spring path/value attributes are Array; the array literal form + // is common in Kotlin. Each route-bearing annotation must accept both a + // bare string and a single-element array (collection_literal). + const dir = path.join(tmpDir, 'kotlin-array-paths'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: array class prefix + array method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductsController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/api/products"]) +class ProductsController { + @GetMapping(value = ["/{id}"]) + fun get(id: Int): Any = TODO() +} +`, + ); + // OpenFeign consumer: positional array path. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "orders") +interface OrdersClient { + @PostMapping(["/orders/search"]) + fun search(): Any +} +`, + ); + // Spring HTTP Interface consumer: named array url. + fs.writeFileSync( + path.join(dir, 'src', 'PricingApi.kt'), + `package com.example +import org.springframework.web.service.annotation.GetExchange + +interface PricingApi { + @GetExchange(url = ["/pricing/{id}"]) + fun price(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // Array class prefix + array method path → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient positional array path → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/orders/search' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // @GetExchange(url = [...]) array → consumer. + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/pricing/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'handles Kotlin arrayOf("/x") annotation arrays across families (#2254 P3)', + async () => { + // arrayOf(...) is the explicit (older) form of a Kotlin String[] arg, + // distinct from the ["/x"] collection_literal. Each route-bearing + // annotation must accept it, positional and named, in all families. + const dir = path.join(tmpDir, 'kotlin-array-of'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // Provider: positional arrayOf class prefix + named arrayOf method path. + fs.writeFileSync( + path.join(dir, 'src', 'ProductsController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/api/products")) +class ProductsController { + @GetMapping(value = arrayOf("/{id}")) + fun get(id: Int): Any = TODO() +} +`, + ); + // OpenFeign consumer: named arrayOf path prefix. + fs.writeFileSync( + path.join(dir, 'src', 'OrdersClient.kt'), + `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.PostMapping + +@FeignClient(name = "orders", path = arrayOf("/feign")) +interface OrdersClient { + @PostMapping(arrayOf("/orders/search")) + fun search(): Any +} +`, + ); + // Spring HTTP Interface consumer: positional arrayOf class prefix + named arrayOf url. + fs.writeFileSync( + path.join(dir, 'src', 'PricingApi.kt'), + `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(arrayOf("/pricing")) +interface PricingApi { + @GetExchange(url = arrayOf("/{id}")) + fun price(id: Int): Any +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + const consumers = contracts.filter((c) => c.role === 'consumer'); + + // class @RequestMapping(arrayOf) + method @GetMapping(value = arrayOf) → provider. + expect( + providers.find((c) => c.contractId === 'http::GET::/api/products/{param}'), + ).toBeDefined(); + // @FeignClient(path = arrayOf) + @PostMapping(arrayOf) → consumer (path applied). + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/feign/orders/search' && + c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + // @HttpExchange(arrayOf) + @GetExchange(url = arrayOf) → consumer (prefix applied). + expect( + consumers.find( + (c) => + c.contractId === 'http::GET::/pricing/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'registers a multi-element arrayOf("/a","/b") under every element (cross-product)', + async () => { + const dir = path.join(tmpDir, 'kotlin-array-of-multi'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'MultiController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/a", "/b")) +class MultiController { + @GetMapping(arrayOf("/x", "/y")) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + // 2 prefixes × 2 method paths → 4 contract IDs. + for (const id of [ + 'http::GET::/a/x', + 'http::GET::/a/y', + 'http::GET::/b/x', + 'http::GET::/b/y', + ]) { + expect(providers.find((c) => c.contractId === id)).toBeDefined(); + } + }, + ); + + itKotlinConsumer( + 'mixes arrayOf and collection-literal arrays without cannibalising either', + async () => { + // The dedicated arrayOf pattern must not drop the sibling ["/x"] + // collection_literal match (the tree-sitter 0.21.x predicate-bucket hazard). + const dir = path.join(tmpDir, 'kotlin-array-of-mixed'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ArrayOfController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(arrayOf("/aof")) +class ArrayOfController { + @GetMapping(arrayOf("/x")) + fun get(): Any = TODO() +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'LiteralController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/lit"]) +class LiteralController { + @GetMapping(["/y"]) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + + expect(providers.find((c) => c.contractId === 'http::GET::/aof/x')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/lit/y')).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not treat a non-arrayOf call or a non-route arrayOf key as a route (anti-overreach)', + async () => { + const dir = path.join(tmpDir, 'kotlin-array-of-negative'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + // buildPath(...) is a call_expression but not arrayOf → no prefix. + // produces = arrayOf(...) is a non-route key → no route. + // arrayOf() is empty → no phantom route. + fs.writeFileSync( + path.join(dir, 'src', 'NegController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(buildPath("/built")) +class NegController { + @GetMapping(produces = arrayOf("application/json")) + fun a(): Any = TODO() + + @GetMapping(arrayOf()) + fun b(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => + c.symbolRef.filePath.endsWith('NegController.kt'), + ); + + // No route should be produced from any of the three anti-overreach forms. + expect(providers).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'does not extract @RequestLine on a Kotlin class method (Feign proxies are interfaces only)', + async () => { + // Feign builds its proxy from an interface; a @RequestLine on a concrete + // class is not a client call. Anti-overreach guard, parity with java.ts. + const dir = path.join(tmpDir, 'kotlin-request-line-class'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClientImpl.kt'), + `package com.example +import feign.RequestLine + +class AiClientImpl { + @RequestLine("GET /should-not-extract") + fun health(): String = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + expect( + contracts.find((c) => c.contractId === 'http::GET::/should-not-extract'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'extracts Kotlin @RequestLine on a plain interface without @FeignClient (Feign.builder())', + async () => { + // Core-Feign usage: a plain interface with @RequestLine wired via + // Feign.builder() — no @FeignClient. The structural interface check + // (not a @FeignClient gate) admits it, matching the Java plugin. + const dir = path.join(tmpDir, 'kotlin-request-line-plain-interface'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AiClient.kt'), + `package com.example +import feign.RequestLine + +interface AiClient { + @RequestLine("POST /ai/summarize") + fun summarize(): String + + @RequestLine("GET /ai/health") + fun health(): String +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const consumers = contracts.filter((c) => c.role === 'consumer'); + expect( + consumers.find( + (c) => + c.contractId === 'http::POST::/ai/summarize' && + c.meta.framework === 'openfeign' && + c.confidence === 0.75, + ), + ).toBeDefined(); + expect( + consumers.find( + (c) => c.contractId === 'http::GET::/ai/health' && c.meta.framework === 'openfeign', + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not emit a provider for a non-controller class implementing a route interface', + async () => { + // Only a @RestController/@Controller implementer serves the interface's + // routes. A plain service/adapter implementing the same interface must + // NOT emit phantom providers (parity with Java's isController gate). + const dir = path.join(tmpDir, 'kotlin-noncontroller-impl'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseServiceImpl.kt'), + `package com.example + +class WarehouseServiceImpl(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/warehouses/{param}/stock'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'detects a controller using the arg-form @RestController("bean")', + async () => { + // The arg-form @RestController("bean") attaches under the class + // `modifiers` as an `annotation` whose child is a `constructor_invocation` + // (NOT a detached sibling). The controller gate reads its trailing name so + // the inherited route is still emitted. + const dir = path.join(tmpDir, 'kotlin-argform-restcontroller'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WarehouseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController("warehouseController") +class WarehouseController(private val svc: Svc) : WarehouseApi { + override fun listStock(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/warehouses/{param}/stock' && + c.symbolRef.filePath.endsWith('WarehouseController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'treats @(Get|...)Exchange as a consumer even on a concrete class (parity with Java)', + async () => { + // @(Get|...)Exchange is definitionally a client (HttpServiceProxyFactory) + // annotation. Like java.ts, the extractor classifies it as a consumer + // regardless of the enclosing type — so even a (mis-)use on a concrete + // class yields a consumer, never a provider. Pins the accepted behavior. + const dir = path.join(tmpDir, 'kotlin-exchange-on-class'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'ReportClient.kt'), + `package com.example +import org.springframework.stereotype.Component +import org.springframework.web.service.annotation.GetExchange + +@Component +class ReportClient { + @GetExchange("/reports/{id}") + fun report(id: Int): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + expect( + contracts.find( + (c) => + c.role === 'consumer' && + c.contractId === 'http::GET::/reports/{param}' && + c.meta.framework === 'spring-http-interface', + ), + ).toBeDefined(); + // Never a provider. + expect( + contracts.find( + (c) => c.role === 'provider' && c.contractId === 'http::GET::/reports/{param}', + ), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'emits one contract per element of a multi-element method-level path array', + async () => { + // Spring registers `@GetMapping(["/a", "/b"])` under BOTH paths, so the + // extractor must emit N contracts (one per array element), not just one. + const dir = path.join(tmpDir, 'kotlin-multi-method-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AliasController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping("/api") +class AliasController { + @GetMapping(value = ["/primary", "/alias"]) + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/api/primary')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/api/alias')).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'emits one contract per element of a multi-element class-level prefix array', + async () => { + // Spring registers a method under EVERY class-level prefix, so a + // `@RequestMapping(["/api/v1", "/api/v2"])` controller must yield a + // contract per (prefix × method-path) combination, not just the last prefix. + const dir = path.join(tmpDir, 'kotlin-multi-prefix-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'VersionedController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RestController +@RequestMapping(["/base/one", "/base/two"]) +class VersionedController { + @GetMapping("/items") + fun get(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/base/one/items')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/base/two/items')).toBeDefined(); + }, + ); + + it('emits one contract per element of a multi-element Java path array', async () => { + // Java parity: @GetMapping({"/a", "/b"}) method array and a multi-element + // class-level @RequestMapping must both expand to N contracts. + const dir = path.join(tmpDir, 'java-multi-array'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'AliasController.java'), + `package com.example; +import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RestController +@RequestMapping({"/base/one", "/base/two"}) +public class AliasController { + @GetMapping({"/primary", "/alias"}) + public Object get() { return null; } +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + for (const id of [ + 'http::GET::/base/one/primary', + 'http::GET::/base/one/alias', + 'http::GET::/base/two/primary', + 'http::GET::/base/two/alias', + ]) { + expect(providers.find((c) => c.contractId === id)).toBeDefined(); + } + }); + + itKotlinConsumer( + 'combines a Kotlin controller class prefix with an inherited interface prefix', + async () => { + // Interface-based controller where BOTH the controller and the interface + // carry a class-level @RequestMapping: the inherited route must be prefixed + // by the controller prefix too (parity with java.ts joinInheritedSpringPath). + const dir = path.join(tmpDir, 'kotlin-controller-prefix-inherit'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'WidgetApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/v1") +interface WidgetApi { + @GetMapping("/{id}") + fun fetch(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'WidgetController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/api") +class WidgetController : WidgetApi { + override fun fetch(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find( + (c) => + c.contractId === 'http::GET::/api/v1/{param}' && + c.symbolRef.filePath.endsWith('WidgetController.kt'), + ), + ).toBeDefined(); + }, + ); + + itKotlinConsumer( + 'does not double a shared prefix when a Kotlin controller repeats the interface prefix', + async () => { + // #2057 parity: controller prefix == interface prefix must not be prepended + // twice (no /shared/shared/...). + const dir = path.join(tmpDir, 'kotlin-controller-prefix-dedup'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'LedgerApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/shared") +interface LedgerApi { + @GetMapping("/entries") + fun entries(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'LedgerController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/shared") +class LedgerController : LedgerApi { + override fun entries(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/shared/entries')).toBeDefined(); + expect( + providers.find((c) => c.contractId === 'http::GET::/shared/shared/entries'), + ).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'still combines distinct inherited Kotlin prefixes that share a leading segment', + async () => { + // Twin of the Java 'shared leading segment' case: controller @RequestMapping("/open") + // + interface @RequestMapping("/open/ai") must combine to /open/open/ai/query, NOT + // dedup to /open/ai/query (the dedup only fires on an exact prefix match). + const dir = path.join(tmpDir, 'kotlin-shared-leading-prefix'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'DataReleaseApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/open/ai") +interface DataReleaseApi { + @GetMapping("/query") + fun query(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'DataReleaseController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/open") +class DataReleaseController : DataReleaseApi { + override fun query(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/open/open/ai/query'), + ).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/open/ai/query')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'keeps a Kotlin controller prefix when a prefix-less interface method starts with the same path', + async () => { + // Twin of the Java prefix-overlap case: controller @RequestMapping("/users") + // + interface @GetMapping("/users/{id}") (no interface prefix) → + // /users/users/{param}, not deduped to /users/{param}. + const dir = path.join(tmpDir, 'kotlin-method-prefix-overlap'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'UserApi.kt'), + `package com.example +import org.springframework.web.bind.annotation.GetMapping + +interface UserApi { + @GetMapping("/users/{id}") + fun getUser(id: String): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'UserController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController +import org.springframework.web.bind.annotation.RequestMapping + +@RestController +@RequestMapping("/users") +class UserController : UserApi { + override fun getUser(id: String): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect( + providers.find((c) => c.contractId === 'http::GET::/users/users/{param}'), + ).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/users/{param}')).toBeUndefined(); + }, + ); + + itKotlinConsumer( + 'skips ambiguous inherited Kotlin routes when interfaces share a simple name', + async () => { + // Twin of the Java simple-name-collision case: two distinct interfaces both + // named StatusApi → ambiguous, so the implementing controller emits nothing. + const dir = path.join(tmpDir, 'kotlin-iface-name-collision'); + fs.mkdirSync(path.join(dir, 'src', 'a'), { recursive: true }); + fs.mkdirSync(path.join(dir, 'src', 'b'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'a', 'StatusApi.kt'), + `package com.example.a +import org.springframework.web.bind.annotation.GetMapping + +interface StatusApi { + @GetMapping("/a/status") + fun getStatus(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'b', 'StatusApi.kt'), + `package com.example.b +import org.springframework.web.bind.annotation.GetMapping + +interface StatusApi { + @GetMapping("/b/status") + fun getStatus(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'StatusController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class StatusController : StatusApi { + override fun getStatus(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/a/status')).toBeUndefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/b/status')).toBeUndefined(); + expect( + providers.filter((c) => c.symbolRef.filePath.endsWith('StatusController.kt')), + ).toHaveLength(0); + }, + ); + + itKotlinConsumer( + 'emits routes from every distinctly-named interface a Kotlin controller implements', + async () => { + // Positive multi-interface case (untested in both languages before #2254): + // a controller implementing two route interfaces emits both their routes. + const dir = path.join(tmpDir, 'kotlin-multi-iface'); + fs.mkdirSync(path.join(dir, 'src'), { recursive: true }); + fs.writeFileSync( + path.join(dir, 'src', 'Apis.kt'), + `package com.example +import org.springframework.web.bind.annotation.GetMapping + +interface OrdersApi { + @GetMapping("/orders") + fun orders(): Any +} + +interface UsersApi { + @GetMapping("/users") + fun users(): Any +} +`, + ); + fs.writeFileSync( + path.join(dir, 'src', 'GatewayController.kt'), + `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class GatewayController : OrdersApi, UsersApi { + override fun orders(): Any = TODO() + override fun users(): Any = TODO() +} +`, + ); + + const contracts = await extractor.extract(null, dir, makeRepo(dir)); + const providers = contracts.filter((c) => c.role === 'provider'); + expect(providers.find((c) => c.contractId === 'http::GET::/orders')).toBeDefined(); + expect(providers.find((c) => c.contractId === 'http::GET::/users')).toBeDefined(); + }, + ); + + // ─── Byte-identical Java↔Kotlin contract parity (set-equality harness) ───── + // Independent per-side twin tests can both pass while the emitted contract + // SETS differ (an extra contract on one side, or a confidence/framework + // drift). This harness feeds matched .java/.kt fixtures through both plugins + // and asserts the full projected contract set is equal across languages AND + // equal to the expected set — the only check that actually verifies the + // "byte-identical contract IDs" goal. Per-scenario twins above stay for + // readability and language-specific cases; this covers the parity-critical + // families. Drift in any covered family fails here directly. + describe('Java↔Kotlin contract parity (set-equality)', () => { + interface ParityFile { + name: string; + java: string; + kotlin: string; + } + interface ParityContract { + role: string; + contractId: string; + framework: unknown; + confidence: number; + } + interface ParityRow { + name: string; + files: ParityFile[]; + expected: ParityContract[]; + } + + const sortContracts = (contracts: ParityContract[]): ParityContract[] => + [...contracts].sort((a, b) => + `${a.role} ${a.contractId}`.localeCompare(`${b.role} ${b.contractId}`), + ); + + const projectContracts = ( + contracts: Awaited>, + ): ParityContract[] => + sortContracts( + contracts.map((c) => ({ + role: c.role, + contractId: c.contractId, + framework: c.meta.framework, + confidence: c.confidence, + })), + ); + + const rows: ParityRow[] = [ + { + name: '@RequestLine with @RequestMapping prefix fallback', + files: [ + { + name: 'OrderClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.RequestMapping; +import feign.RequestLine; + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + Object get(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.RequestMapping +import feign.RequestLine + +@FeignClient(name = "order-service") +@RequestMapping("/orders") +interface OrderClient { + @RequestLine("GET /{id}") + fun get(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/orders/{param}', + framework: 'openfeign', + confidence: 0.75, + }, + ], + }, + { + name: 'named @RequestLine(value=...)', + files: [ + { + name: 'CreateClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import feign.RequestLine; + +@FeignClient(name = "create-service") +interface CreateClient { + @RequestLine(value = "POST /create") + Object create(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import feign.RequestLine + +@FeignClient(name = "create-service") +interface CreateClient { + @RequestLine(value = "POST /create") + fun create(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::POST::/create', + framework: 'openfeign', + confidence: 0.75, + }, + ], + }, + { + name: '@FeignClient(path) + @GetMapping', + files: [ + { + name: 'UsersClient', + java: ` +import org.springframework.cloud.openfeign.FeignClient; +import org.springframework.web.bind.annotation.GetMapping; + +@FeignClient(name = "users-service", path = "/api") +interface UsersClient { + @GetMapping("/users") + Object users(); +} +`, + kotlin: `package com.example +import org.springframework.cloud.openfeign.FeignClient +import org.springframework.web.bind.annotation.GetMapping + +@FeignClient(name = "users-service", path = "/api") +interface UsersClient { + @GetMapping("/users") + fun users(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/api/users', + framework: 'openfeign', + confidence: 0.7, + }, + ], + }, + { + name: '@HttpExchange(url) prefix + @GetExchange', + files: [ + { + name: 'ProductApi', + java: ` +import org.springframework.web.service.annotation.HttpExchange; +import org.springframework.web.service.annotation.GetExchange; + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + Object get(); +} +`, + kotlin: `package com.example +import org.springframework.web.service.annotation.HttpExchange +import org.springframework.web.service.annotation.GetExchange + +@HttpExchange(url = "/products") +interface ProductApi { + @GetExchange("/{id}") + fun get(): Any +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/products/{param}', + framework: 'spring-http-interface', + confidence: 0.75, + }, + ], + }, + { + name: 'WebClient long-form method(HttpMethod.X).uri(...)', + files: [ + { + name: 'LongFormClient', + java: ` +import org.springframework.http.HttpMethod; +import org.springframework.web.reactive.function.client.WebClient; + +class LongFormClient { + void run(WebClient webClient) { + webClient.method(HttpMethod.GET).uri("/api/items").retrieve(); + } +} +`, + kotlin: `package com.example +import org.springframework.http.HttpMethod +import org.springframework.web.reactive.function.client.WebClient + +class LongFormClient { + fun run(webClient: WebClient) { + webClient.method(HttpMethod.GET).uri("/api/items").retrieve() + } +} +`, + }, + ], + expected: [ + { + role: 'consumer', + contractId: 'http::GET::/api/items', + framework: 'spring-web-client', + confidence: 0.7, + }, + ], + }, + { + name: 'interface-based controller inheritance', + files: [ + { + name: 'WarehouseApi', + java: ` +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.GetMapping; + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + Object listStock(); +} +`, + kotlin: `package com.example +import org.springframework.web.bind.annotation.RequestMapping +import org.springframework.web.bind.annotation.GetMapping + +@RequestMapping("/warehouses") +interface WarehouseApi { + @GetMapping("/{id}/stock") + fun listStock(): Any +} +`, + }, + { + name: 'WarehouseController', + java: ` +import org.springframework.web.bind.annotation.RestController; + +@RestController +class WarehouseController implements WarehouseApi { + @Override + public Object listStock() { return null; } +} +`, + kotlin: `package com.example +import org.springframework.web.bind.annotation.RestController + +@RestController +class WarehouseController : WarehouseApi { + override fun listStock(): Any = TODO() +} +`, + }, + ], + expected: [ + { + role: 'provider', + contractId: 'http::GET::/warehouses/{param}/stock', + framework: 'spring', + confidence: 0.8, + }, + ], + }, + ]; + + rows.forEach((row) => { + itKotlinConsumer(`emits identical contracts for ${row.name}`, async () => { + const base = path.join(tmpDir, `parity-${row.name.replace(/[^a-z0-9]+/gi, '-')}`); + const javaDir = path.join(base, 'java'); + const kotlinDir = path.join(base, 'kotlin'); + fs.mkdirSync(path.join(javaDir, 'src'), { recursive: true }); + fs.mkdirSync(path.join(kotlinDir, 'src'), { recursive: true }); + for (const file of row.files) { + fs.writeFileSync(path.join(javaDir, 'src', `${file.name}.java`), file.java); + fs.writeFileSync(path.join(kotlinDir, 'src', `${file.name}.kt`), file.kotlin); + } + + const javaContracts = projectContracts( + await extractor.extract(null, javaDir, makeRepo(javaDir)), + ); + const kotlinContracts = projectContracts( + await extractor.extract(null, kotlinDir, makeRepo(kotlinDir)), + ); + const expected = sortContracts(row.expected); + + // The two languages emit the same contract set... + expect(kotlinContracts).toEqual(javaContracts); + // ...and it is exactly the expected set (no extra/missing contracts). + expect(javaContracts).toEqual(expected); + }); + }); + }); + it('extracts Go stdlib and resty calls', async () => { const dir = path.join(tmpDir, 'go-consumer'); fs.mkdirSync(path.join(dir, 'cmd'), { recursive: true });