Skip to content

feat(group): add cross-repo call trace with pluggable SymbolResolver#1583

Open
hongtewu-dotcom wants to merge 4 commits into
abhigyanpatwari:mainfrom
hongtewu-dotcom:feat/group-trace-mcp-tool
Open

feat(group): add cross-repo call trace with pluggable SymbolResolver#1583
hongtewu-dotcom wants to merge 4 commits into
abhigyanpatwari:mainfrom
hongtewu-dotcom:feat/group-trace-mcp-tool

Conversation

@hongtewu-dotcom

@hongtewu-dotcom hongtewu-dotcom commented May 14, 2026

Copy link
Copy Markdown

Adds group_trace as a first-class MCP tool and introduces the SymbolResolver interface to keep the BFS engine framework-agnostic.

What's included

Core: cross-repo BFS trace engine (src/core/group/trace.ts)

  • runGroupTrace / runGroupTraceWithResolver: BFS within each repo's LadybugDB graph, following crossLinks from contracts.json across repos
  • SymbolResolver interface: pluggable symbol-resolution strategy so framework-specific naming conventions live outside the engine
  • DefaultSymbolResolver: generic reference implementation covering ClassName.methodName patterns, hintFilePath pinning, Service/Impl variants
  • mtime-based cache for group.yaml and contracts.json (no repeated disk I/O when MCP server is long-running)
  • SegmentOptions struct: replaces 11-arg processOneSegment signature
  • Seeds included in BFS result at depth=0 for complete trace output
  • Topic crossLink direction correctly reversed vs RPC
  • Topic dedup key scoped to contractId to allow multiple consumers per repo

MCP tool (src/mcp/tools.ts, src/mcp/local/local-backend.ts)

  • New group_trace MCP tool exposing runGroupTrace with full parameter schema (direction, maxDepth, maxCrossDepth, includeTests, minConfidence, etc.)
  • Uses MCP server's persistent LadybugDB connection pool — eliminates cold-start cost on repeated trace calls

Tests (test/unit/group/trace.test.ts)

  • 49 tests: parameter validation, group/repo lookup errors, single-repo BFS, cross-repo RPC hops, topic hops, deduplication, unresolvable symbol skipping, includeTests filter
  • Pure-function unit tests for isTestFilePath, isClientModulePath, isUtilOrDto, buildCrossLinksIndex, findCrossRepoHopsFromRegistry, DefaultSymbolResolver.scoreCandidate (no lbug required)
  • lbug mocked via vi.mock so all tests run without indexed fixtures
  • trace.ts line coverage: ~80%

Summary

Motivation / context

Areas touched

  • gitnexus/ (CLI / core / MCP server)
  • gitnexus-web/ (Vite / React UI)
  • .github/ (workflows, actions)
  • eval/ or other tooling
  • Docs / agent config only (AGENTS.md, CLAUDE.md, .cursor/, llms.txt, etc.)

Scope & constraints

In scope

Explicitly out of scope / not done here

Implementation notes

Testing & verification

  • cd gitnexus && npm test
  • cd gitnexus && npm run test:integration (if core/indexing/MCP paths changed)
  • cd gitnexus && npx tsc --noEmit
  • cd gitnexus-web && npm test (if web changed)
  • cd gitnexus-web && npx tsc -b --noEmit (if web changed)
  • Manual / Playwright E2E (note environment — see gitnexus-web/e2e/)

Risk & rollout

Checklist

  • PR body meets repo minimum length (workflow may label short descriptions)
  • If AGENTS.md / overlays changed: headers, scope block, and changelog updated per project conventions
  • No secrets, tokens, or machine-specific paths committed

@vercel

vercel Bot commented May 14, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

@hongtewu-dotcom
hongtewu-dotcom force-pushed the feat/group-trace-mcp-tool branch 2 times, most recently from bcdc475 to f7f93e0 Compare May 14, 2026 13:35
@magyargergo

Copy link
Copy Markdown
Collaborator

Can you please benchmark the performance of the graph queries? 🙏

@hongtewu-dotcom
hongtewu-dotcom force-pushed the feat/group-trace-mcp-tool branch 2 times, most recently from 9b2df3b to bcdc475 Compare May 15, 2026 09:07
@hongtewu-dotcom

Copy link
Copy Markdown
Author

Thanks for the request! I've added scripts/bench-group-trace.ts — a
benchmark that uses only the public API (runGroupTraceWithResolver + a
minimal GroupToolPort shim backed by the local registry), so it's fully
reproducible without internal knowledge of the codebase.

Tested against a real indexed group of 83 repos.


  1. Cold start vs warm (maxCrossDepth=1, 5 iters)

┌─────────────────────────────────────────────────────┬──────────┐
│ │ Time │
├─────────────────────────────────────────────────────┼──────────┤
│ Cold start (empty module cache, no open lbug conns) │ ~1989 ms │
├─────────────────────────────────────────────────────┼──────────┤
│ Warm p50 │ ~1095 ms │
├─────────────────────────────────────────────────────┼──────────┤
│ Warm p95 │ ~2123 ms │
├─────────────────────────────────────────────────────┼──────────┤
│ Cache speedup │ 1.8× │
└─────────────────────────────────────────────────────┴──────────┘

The warm stddev (±611 ms) reflects occasional LRU eviction in the lbug
connection pool — when BFS opens more repos than the pool limit, the
least-recently-used connection is closed and reopened. The pool auto-expands
during a trace (setMaxPoolSize) to amortise this.


  1. maxCrossDepth sweep (warm cache)

┌───────────────┬──────────┬────────────────────┐
│ maxCrossDepth │ Time │ Cross-hops visited │
├───────────────┼──────────┼────────────────────┤
│ 1 │ 517 ms │ 159 │
├───────────────┼──────────┼────────────────────┤
│ 2 │ 8032 ms │ 1925 │
├───────────────┼──────────┼────────────────────┤
│ 3 │ 13883 ms │ 3014 │
├───────────────┼──────────┼────────────────────┤
│ 4 │ 26479 ms │ 7165 │
├───────────────┼──────────┼────────────────────┤
│ 5 │ 28531 ms │ 7899 │
└───────────────┴──────────┴────────────────────┘

The jump from depth 1 (517 ms, 159 hops) to depth 2 (8 s, 1925 hops) is
expected: each additional hop expands the frontier into N new repo graphs,
so visited cross-hops grow roughly multiplicatively. This is inherent to BFS
over a real call graph, not an engine inefficiency.

The default cap (maxCrossDepth=5) is intentionally conservative; MCP callers
should lower it for interactive use.


  1. Multi-target warm (maxCrossDepth=1, 5 iters each)

┌────────────────────────┬─────────┬─────────┬─────────┬────────────┐
│ Entry point │ p50 │ p95 │ σ │ Cross-hops │
├────────────────────────┼─────────┼─────────┼─────────┼────────────┤
│ shallow fan-out │ 801 ms │ 1540 ms │ ±404 ms │ 159 │
├────────────────────────┼─────────┼─────────┼─────────┼────────────┤
│ wide fan-out │ 8906 ms │ 9338 ms │ ±403 ms │ 2562 │
├────────────────────────┼─────────┼─────────┼─────────┼────────────┤
│ different repo cluster │ 4813 ms │ 5061 ms │ ±169 ms │ 605 │
└────────────────────────┴─────────┴─────────┴─────────┴────────────┘

Performance scales with cross-hops visited, not group size — a shallow entry
point in an 83-repo group responds in ~800 ms warm.


Run it yourself:
BENCH_GROUP=
BENCH_REPO_A= BENCH_TARGET_A=
npx tsx scripts/bench-group-trace.ts

@github-actions

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 6m 29s —— View job


Production-Readiness Review: PR #1583 feat(group): add cross-repo call trace with pluggable SymbolResolver

Task List

  • Post initial comment
  • Read repo context files (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, ARCHITECTURE.md, package.json, CI workflows)
  • Inspect all changed files (trace.ts, trace-resolver.ts, group.ts, service.ts, tools.ts, orm.ts, parse-worker.ts, local-backend.ts, tests)
  • Inspect related files (impact.ts, contracts.ts, config.ts, types.ts, lbug, mcp/server, cli/index.ts)
  • Run hygiene checks (Unicode, git diff --check)
  • Check CI/workflow status
  • Assess Lane A: Cross-repo group trace engine
  • Assess Lane B: CLI and output contract
  • Assess Lane C: MCP/local backend/service schema
  • Assess Lane D: MyBatis ORM ingestion
  • Assess Lane E: CI/security/workflow validation
  • Assess Lane F: Branch hygiene and PR scope
  • Write final verdict

Review Bar

Adversarial production-readiness review against GitNexus DoD. All findings are grounded in file evidence at specific lines. CI status verified via direct workflow inspection.


Current PR State

  • Head SHA: c54ed3e5ce74e6465a9ebfd626b248a8d3f3f55a
  • Commits: 7 (including 1 merge-from-main commit)
  • CI: Not verified — GitHub reports "Checks 0" / "Workflow runs completed with no jobs" on current head SHA. The fork-PR workflow requires maintainer approval to run.
  • Reviews: None
  • Changed files: 13

Branch Hygiene Assessment

Classification: rebase/split required

This PR contains two unrelated production domains:

  1. group_trace — BFS engine, MCP tool, CLI command, SymbolResolver interface
  2. MyBatis ORM ingestion — XML mapper scanning, SQL table extraction, QUERIES edges

The merge-from-main commit (c54ed3e) is harmless and merge-safe, but combined with the dual-domain scope, the branch requires splitting before final review.


Understanding of the Change

The PR adds:

  • trace.ts (907 lines): cross-repo BFS engine, runGroupTrace / runGroupTraceWithResolver, mtime-cached config loading, CrossLinksIndex pre-build, topic/RPC direction handling
  • trace-resolver.ts (291 lines): SymbolResolver interface, DefaultSymbolResolver with scoring/drillDown/resolve
  • cli/group.ts (+223 lines): new group trace subcommand with --json, --verbose, buildTraceSummary
  • service.ts (+43 lines): groupTrace() method with slim/verbose branching
  • tools.ts (+63 lines): group_trace MCP tool definition
  • local-backend.ts (+2 lines): group_trace case dispatch
  • orm.ts (+180 lines): MyBatis XML mapper extraction, SQL regex, namespace→file resolution
  • parse-worker.ts (+7 lines): mybatis added to ExtractedORMQuery.orm discriminated union

Findings


A. Cross-repo Group Trace Engine

Finding A-1 — CRITICAL — Missing trace-resolver-meituan.ts breaks all production entry points

  • Severity: Critical
  • File: gitnexus/src/core/group/trace.ts:663
  • Evidence:
    export async function runGroupTrace(...) {
      const { MeituanSymbolResolver } = await import('./trace-resolver-meituan.js'); // ← this file does NOT exist
      return runGroupTraceWithResolver(deps, params, new MeituanSymbolResolver());
    }
    Verified with filesystem search: gitnexus/src/core/group/trace-resolver-meituan* returns no files. The directory listing of gitnexus/src/core/group/ confirms the file is absent. Both the MCP tool (group_trace) and the CLI (group trace) route through service.ts:groupTrace()runGroupTrace(...). Every call will throw MODULE_NOT_FOUND before any validation or BFS runs. The unit tests that call runGroupTrace (lines 123–613 of trace.test.ts) are also broken — the dynamic import is the first statement in the function body, before the name/repo/target validation, so parameter-validation tests would throw instead of returning { error: ... }.
  • Risk: The entire feature is non-functional as shipped. Users who call group_trace via MCP or gitnexus group trace via CLI will always get a runtime exception.
  • Fix: Replace the MeituanSymbolResolver import with DefaultSymbolResolver from trace-resolver.ts in runGroupTrace, or ship trace-resolver-meituan.ts as part of this PR. The safest public default is DefaultSymbolResolver. Fix this →
  • Blocks merge: yes

Finding A-2 — High — maxDepth=0 is the default (unlimited intra-repo BFS)

  • Severity: High
  • File: gitnexus/src/core/group/trace.ts:186, trace.ts:359
  • Evidence:
    const DEFAULT_MAX_DEPTH = 0; // 0 = unlimited (BFS terminates when frontier is empty)
    // ...
    for (let depth = 1; (maxDepth === 0 || depth <= maxDepth) && frontier.length > 0; depth++) {
    The MCP schema (tools.ts:595) says Default: 0, and the CLI defaults are '0'. On a dense Java service graph with thousands of CALLS edges, the default will traverse the entire connected component, potentially returning 40MB+ of nodes per segment. The verbose: true path (always used by CLI) returns all nodes without truncation.
  • Risk: Resource exhaustion (memory, time, MCP response size) for production MCP callers on large repos. The benchmarks in the PR comment show depth=2 costs 8s and produces 1925 cross-hops — that is the cross-depth, not intra-repo depth. A single 50k-symbol repo with maxDepth=0 could traverse all nodes.
  • Fix: Change the default to a bounded value (e.g., DEFAULT_MAX_DEPTH = 10) for interactive use. Add explicit documentation that 0 = unlimited. Add a MCP schema maximum or a warning when maxDepth=0 is used.
  • Blocks merge: yes (unlimited default is unsafe for interactive MCP use on real repos)

Finding A-3 — Medium — Topic deduplication collapses valid multiple consumers on same contractId

  • Severity: Medium
  • File: gitnexus/src/core/group/trace.ts:524, test line 409
  • Evidence:
    const key = `topic::${link.contractId}::${remoteEndpoint.repo}`;
    The dedup key is contractId + remoteRepo. If two consumer groups in the same target repo listen to the same Kafka topic (same contractId), only the first hop is kept. The PR description says "Topic dedup key scoped to contractId to allow multiple consumers per repo" — but the test at line 409 only tests different contractIds, not multiple consumers sharing the same one. The claim in the PR description is incorrect for same-contractId consumers.
  • Risk: Group traces that span message queues with multiple consumer groups on the same topic silently omit real consumers.
  • Fix: Include the consumer symbolRef.filePath or symbolUid in the topic dedup key, or add a test that validates same-contractId multi-consumer behavior and document the intended dedup semantics.
  • Blocks merge: maybe

Finding A-4 — Medium — No cache invalidation test; module-level caches grow unboundedly

  • Severity: Medium
  • File: gitnexus/src/core/group/trace.ts:41–70
  • Evidence:
    const _groupConfigCache = new Map<string, CacheEntry<GroupConfig>>();
    const _contractRegistryCache = new Map<string, CacheEntry<...>>();
    These are module-level Maps with no maximum size or LRU eviction. A long-running MCP server serving many groups will accumulate entries indefinitely. More importantly, the mtime-based invalidation logic has no test — trace.test.ts searches return zero hits for "cache", "mtime", "stale", or "invalid". If stat() throws (e.g., contracts.json deleted mid-run), the catch block falls through to uncached readContractRegistry, which is correct behavior — but this path is also untested.
  • Risk: Memory leak over time (minor for small installations; significant for enterprise with 100+ groups). Silent serving of stale contracts if the mtime comparison silently breaks.
  • Fix: Add a test for cache invalidation (write file, read, modify mtime, verify re-read). Consider an LRU cap if the MCP server is expected to serve many groups.
  • Blocks merge: maybe

B. CLI and Output Contract

Finding B-1 — High — maxCrossDepth=NaN and negative values allow unlimited cross-repo traversal

  • Severity: High
  • File: gitnexus/src/cli/group.ts:401–402
  • Evidence:
    const maxDepth = parseInt(String(opts.maxDepth ?? '0'), 10);
    const maxCrossDepth = parseInt(String(opts.maxCrossDepth ?? '10'), 10);
    parseInt("abc") returns NaN. In the trace engine: maxCrossDepth > 0 evaluates to NaN > 0 = false, so the truncation guard is never triggered. maxCrossDepth = -5 also satisfies !(-5 > 0) and allows unbounded cross-repo traversal. The MCP schema has minimum: 0 but no enforcement in the CLI layer. The minConfidence parsing (parseFloat(...) || 0) silently defaults to 0 on invalid input, which is safe.
  • Risk: A user passing --max-cross-depth abc or --max-cross-depth -1 gets an unbounded trace.
  • Fix: Add isNaN(n) || n < 0 guards and exit with a user-friendly error. Fix this →
  • Blocks merge: yes (combines with Finding A-2 — both can trigger unbounded traversal)

Finding B-2 — Medium — Stats inconsistency: CLI totalRepos vs MCP totalRepos

  • Severity: Medium
  • File: gitnexus/src/cli/group.ts:578–583, gitnexus/src/core/group/service.ts:344–345
  • Evidence:
    CLI buildTraceSummary:
    const repoSet = new Set<string>();
    for (const hop of dedupHops) {
      repoSet.add(hop.from.repo);
      repoSet.add(hop.to.repo);
    }
    // totalRepos: repoSet.size  ← repos appearing in crossHops only
    Service slim response:
    totalRepos: new Set(full.segments?.map((s: any) => s.repoPath) ?? []).size,
    // ← all traversed segment repos
    A trace visiting 5 repos with only 3 appearing in cross-hops reports totalRepos=3 from CLI and totalRepos=5 from MCP. The entry repo (which always has a segment) may or may not appear in cross-hops.
  • Risk: Users querying via MCP and comparing notes with CLI output see different numbers for the same trace. Confusing for automation.
  • Fix: Define totalRepos semantics once, share the buildTraceSummary helper between CLI and service, or document the difference explicitly.
  • Blocks merge: maybe (confusing but not incorrect behavior)

C. MCP/Local Backend/Service Schema

Finding C-1 — High — params as any in service.ts bypasses TypeScript safety

  • Severity: High
  • File: gitnexus/src/core/group/service.ts:313
  • Evidence:
    const result = await runGroupTrace({ port: this.port, gitnexusDir: getDefaultGitnexusDir() }, params as any);
    params is Record<string, unknown>. The cast to any means the TypeScript compiler cannot catch mismatched types if TraceParams changes. Combined with Finding A-1, this also means that if runGroupTrace is eventually fixed to use runGroupTraceWithResolver, the params forwarding bypasses all static typing.
  • Fix: Parse and validate params into a TraceParams value explicitly (same pattern used by groupImpact in cross-impact.ts). Fix this →
  • Blocks merge: maybe

Finding C-2 — Medium — MCP tool description says "segments" but default response omits them

  • Severity: Medium
  • File: gitnexus/src/mcp/tools.ts:572
  • Evidence:
    Tool description: "Returns: segments (per-repo BFS nodes), cross-repo hops, skipped repos, and truncation flag." Default (non-verbose) response returns crossHops (deduplicated), stats, skippedRepos, truncated — no segments. Only verbose: true includes segments.
  • Risk: MCP clients reading the description expect segments in the response and write code against it, then break on non-verbose output.
  • Fix: Update the description to reflect the default slim response shape and explicitly state that segments are only included with verbose: true.
  • Blocks merge: no

Finding C-3 — Medium — No MCP contract test for slim/verbose response shape

  • Severity: Medium
  • Evidence: No test in the test suite verifies that groupTrace({ verbose: false }) returns the slim shape, or that groupTrace({ verbose: true }) returns full segments. No test exercises the MCP tool dispatch path (local-backend.ts:3320).
  • Risk: Schema and runtime shape can silently diverge on future changes.
  • Fix: Add at least one integration-level test exercising groupService.groupTrace with verbose: false and asserting the slim response keys.
  • Blocks merge: maybe

D. MyBatis ORM Ingestion

Finding D-1 — High — MyBatis domain is unrelated to group_trace; should be split

  • Severity: High (branch hygiene)
  • Files: gitnexus/src/core/ingestion/pipeline-phases/orm.ts (+180 lines), parse-worker.ts (+7 lines), 3 new test fixtures, test/integration/orm-dataflow.test.ts (+64 lines)
  • Evidence: orm.ts is the ingestion pipeline's ORM phase (pipeline DAG: scan → structure → parse → [routes, tools, orm]). The group_trace feature operates entirely on the group/ module and LadybugDB query layer — it does not call ORM ingestion code. The PR contains no code path connecting MyBatis ingestion to runGroupTrace. The PR description's "Areas touched" and "Motivation/context" are both blank.
  • Risk: MyBatis ingestion and group_trace have different risk profiles, different reviewers, and different rollback strategies. Mixing them makes the PR harder to revert, review, and bisect.
  • Fix: Split MyBatis ORM changes into a separate PR.
  • Blocks merge: yes (per DoD: "If MyBatis ORM changes are unrelated to group trace and remain in the PR, choose rebase/split required")

Finding D-2 — Medium — TABLE_REF_RE misses schema-qualified table names

  • Severity: Medium
  • File: gitnexus/src/core/ingestion/pipeline-phases/orm.ts:62
  • Evidence:
    const TABLE_REF_RE = /\b(?:FROM|INTO|UPDATE|JOIN)\s+`?([a-zA-Z_][a-zA-Z0-9_]*)`?/gi;
    The capture group ([a-zA-Z_][a-zA-Z0-9_]*) stops at ., so FROM db_schema.order_info matches db_schema, not order_info. Enterprise MyBatis XML files commonly use schema-qualified table names. The table filter list !/^(select|dual|values|set)$/ doesn't help here since db_schema is a valid identifier.
  • Fix: Extend the regex to optionally match schema.table: ([a-zA-Z_][a-zA-Z0-9_]*)(?:\.([a-zA-Z_][a-zA-Z0-9_]*))? and use the second group when present.
  • Blocks merge: maybe (correctness issue for schema-qualified SQL)

Finding D-3 — Medium — All .xml files scanned regardless of project type

  • Severity: Medium
  • File: gitnexus/src/core/ingestion/pipeline-phases/orm.ts:137
  • Evidence:
    const xmlPaths = allPaths.filter((p) => p.endsWith('.xml'));
    For a Node.js or Python repo, this will read every XML file (Maven POM fragments, Spring configs, log4j2, IntelliJ project files, etc.) before the quick-check if (!content.includes('<mapper')). For very large repos with many XML files, this adds unnecessary I/O on every analysis run.
  • Fix: Add a path-level heuristic before the content read: check for mapper/ in the path, or only scan XML files in repos with Java sources.
  • Blocks merge: no (performance, not correctness)

Finding D-4 — Low — Overload-suffix lookup limited to #0/#1; misses type-hash variants

  • Severity: Low
  • File: gitnexus/src/core/ingestion/pipeline-phases/orm.ts:229–236
  • Evidence:
    const candidates = [
      generateId('Method', `${q.filePath}:${qualifiedMethod}#1`),
      generateId('Method', `${q.filePath}:${qualifiedMethod}#0`),
      generateId('Method', `${q.filePath}:${qualifiedMethod}`),
    ];
    As documented in ARCHITECTURE.md § Overloaded method resolution, IDs can include type-hash suffixes like ~int,String when arity collision exists. The lookup misses those variants, falling back to the File node.
  • Fix: Accept the File-level fallback as a documented limitation, or use a graph prefix-scan for Method nodes with the qualified method name prefix.
  • Blocks merge: no

E. CI/Security/Workflow Validation

Finding E-1 — Critical — CI has not run on current head SHA

  • Severity: Critical
  • Evidence: PR is from fork hongtewu-dotcom:feat/group-trace-mcp-tool. GitHub Actions requires maintainer approval for first-time fork contributors. The PR metadata states "Checks 0" / "Workflow runs completed with no jobs." The ci.yml workflow dispatches: TypeScript typecheck (ci-quality.yml), unit + integration tests (ci-tests.yml), security scans (Gitleaks, CodeQL, dependency review). None of these have run on c54ed3e5. The author's "49 tests / ~80% coverage" claim is unverified.
  • Risk: TypeScript typecheck has not validated that import('./trace-resolver-meituan.js') compiles (it does compile — dynamic imports to non-existent files are not caught at compile time). Integration tests have not run against the orm-dataflow fixture. Gitleaks has not scanned the PR.
  • Fix: Maintainer must approve CI run, or author must provide verified local test output (with exact commit SHA and npm test / npx tsc --noEmit output).
  • Blocks merge: yes

F. Hidden Unicode and Repository Hygiene

Finding F-1 — Clear — No hidden/bidirectional Unicode controls

  • Evidence: Ran grep -P "[\x{202A}-\x{202E}\x{2066}-\x{2069}\x{200B}-\x{200D}\x{FEFF}]" across all changed files. Result: No bidi Unicode. The non-ASCII characters present are em dashes (—) and arrows (→) in comments — visible Unicode, consistent with the existing repo style.
  • Blocks merge: no

Finding F-2 — Medium — Merge-from-main commit present

  • Severity: Medium
  • Evidence: Commit c54ed3e Merge branch 'main' into feat/group-trace-mcp-tool brings in commits through 7d50039 (ladybug read-only enforcement), bdc0439 (git worktrees), 105efd0 (wiki --lang), 4938272 (analyze heap). These are unrelated to group_trace or MyBatis. The merge itself is clean, but it adds maintenance churn in the diff and complicates bisect.
  • Fix: Not required if maintainers accept the combined scope, but a rebase onto main would produce a cleaner history for the final merge.
  • Blocks merge: no (harmless but messy)

PR-Specific Assessment Summary

Lane Status Blocks Merge
A — Trace engine Broken (missing Meituan resolver, unlimited default depth) Yes
B — CLI contract Issues (NaN/negative depth, stats inconsistency) Yes
C — MCP/service schema Issues (params as any, doc mismatch, no contract test) Maybe
D — MyBatis ingestion Wrong PR (unrelated domain, regex gaps) Yes
E — CI/validation Not run (fork PR, 0 checks) Yes
F — Hygiene Mixed (no bidi Unicode ✓, merge commit present, dual domain) Yes

Final Verdict

not production-ready

The PR has three independent blockers, any one of which would prevent merge:

  1. trace-resolver-meituan.ts does not exist. Both the MCP group_trace tool and the CLI group trace command route through runGroupTrace() which dynamically imports this missing module. Every production call will throw MODULE_NOT_FOUND. The feature is entirely non-functional as shipped, and the unit tests that appear to validate it are also broken for the same reason.

  2. CI has not run. A PR touching the ingestion pipeline, MCP tool layer, group service, and CLI output contract must pass TypeScript typecheck, unit tests, and integration tests on the current head SHA before merge. GitHub currently shows zero checks.

  3. MyBatis ORM ingestion is a separate production domain with no demonstrated causal connection to group_trace. It should be split into its own PR where it can be reviewed on its own risk profile, tested independently, and reverted independently if necessary.

Beyond these blockers, the default maxDepth=0 (unlimited intra-repo BFS) and unvalidated CLI numeric inputs for maxCrossDepth create resource-exhaustion risks for real multi-repo codebases that must be addressed before this ships to production MCP users.

@github-actions

Copy link
Copy Markdown
Contributor

✨ PR Autofix

Found fixable formatting / unused-import issues across 1258 changed lines. Comment /autofix on this PR to apply them, or run npm run lint:fix && npm run format locally.

{"schema":"gitnexus.pr-autofix/v2","state":"fixes-available","pr_number":1583,"changed_lines":1258,"head_sha":"c54ed3e5ce74e6465a9ebfd626b248a8d3f3f55a","run_id":"26022408824","apply_command":"/autofix"}

// Strip CDATA wrapper
const clean = sql.replace(/<!\[CDATA\[([\s\S]*?)]]>/g, '$1');
// Strip XML comments
const noComments = clean.replace(/<!--[\s\S]*?-->/g, '');
.join('\n');

fs.writeFileSync(
path.join(groupDir, 'group.yaml'),
contracts: any[] = [],
): void {
fs.writeFileSync(
path.join(groupDir, 'contracts.json'),
Comment thread gitnexus/test/unit/group/trace.test.ts Outdated
try {
writeContractsJson(groupDir); // empty crossLinks

const { executeParameterized, executeQuery } = await import(
Comment thread gitnexus/test/unit/group/trace.test.ts Outdated
},
]);

const { executeParameterized, executeQuery } = await import(
Comment thread gitnexus/test/unit/group/trace.test.ts Outdated
try {
writeContractsJson(groupDir);

const { executeParameterized, executeQuery } = await import(
@hongtewu-dotcom
hongtewu-dotcom force-pushed the feat/group-trace-mcp-tool branch 3 times, most recently from 6d96056 to f5d7857 Compare May 18, 2026 11:24
@hongtewu-dotcom
hongtewu-dotcom force-pushed the feat/group-trace-mcp-tool branch 3 times, most recently from 4d106d6 to ed3c89e Compare May 25, 2026 13:23
wuhongteng added 2 commits May 25, 2026 21:27
- trace.ts: BFS engine across repos via CALLS edges + contracts.json crossLinks.
  Pluggable SymbolResolver interface; DefaultSymbolResolver as reference implementation.
  runGroupTrace uses DefaultSymbolResolver; runGroupTraceWithResolver accepts any resolver.
- trace-resolver.ts: SymbolResolver interface + DefaultSymbolResolver (generic, no
  framework-specific logic). Subclass to add custom symbol resolution.
- service.ts: groupTrace() with typed TraceParams construction, NaN guards, depth clamping
  (maxCrossDepth cap 50). Slim MCP response by default; verbose flag for full data.
- cli/group.ts: group trace subcommand with NaN-safe parseInt, negative-value guards.
- tools.ts: group_trace MCP tool with complete schema (relationTypes, maxCrossDepth max 50).
- local-backend.ts: route group_trace to GroupService.
- Use fs.mkdtempSync for secure temp directory creation (CWE-377)
- Remove unused executeQuery destructuring (3 instances)
@hongtewu-dotcom
hongtewu-dotcom force-pushed the feat/group-trace-mcp-tool branch from ed3c89e to 4e239e1 Compare May 25, 2026 13:27
@hongtewu-dotcom

Copy link
Copy Markdown
Author

Hi @magyargergo , the CodeQL findings have been addressed:

  • Replaced insecure temp dir creation with fs.mkdtempSync (CWE-377)
  • Removed unused executeQuery destructuring (3 instances)
  • The orm.ts alert is a stale scan — that code path no longer exists on this branch

The branch has also been rebased onto main to remove the merge commit.

Could you approve the workflow run so CI can verify the fixes? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants