From 0e7b3d6fad9ba57e05c1af90874374586953c489 Mon Sep 17 00:00:00 2001 From: samuel mbabhazi <111171386+samuelmbabhazi@users.noreply.github.com> Date: Tue, 20 Oct 2026 09:54:52 +0200 Subject: [PATCH] fix(mcp): report cross-function explain lines 1-based (#3635) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): report cross-function explain lines 1-based The interprocedural branch of explain passed the endpoints' and hops' graph startLine through unchanged. Those are 0-based (tree-sitter rows), so every cross-function source, sink and hop sat one line above the function's declaration in an editor, while the intra-procedural findings of the same response were already 1-based. source.line, sink.line and hops[].line now go through toDisplayLine like context, query and impact (#2377), and the response note says these are declaration lines. The interproc integration block asserts the exact fixture lines: handle on line 7 of source.ts, runIt on line 6 of sink.ts. Fixes #3491 * fix(review): remove repaired accuracy allowance --------- Co-authored-by: Gergő Magyar Co-authored-by: Gergo Magyar --- gitnexus/bench/tool-accuracy/known-gaps.json | 12 ----------- gitnexus/src/mcp/local/local-backend.ts | 20 +++++++++++++++---- .../test/integration/taint-explain.test.ts | 10 ++++++++++ 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/gitnexus/bench/tool-accuracy/known-gaps.json b/gitnexus/bench/tool-accuracy/known-gaps.json index 9202c5627..ec34aff9a 100644 --- a/gitnexus/bench/tool-accuracy/known-gaps.json +++ b/gitnexus/bench/tool-accuracy/known-gaps.json @@ -8,18 +8,6 @@ "reason": "Guard-style sanitizers are a documented taint limitation (#3490).", "baselineActual": ["handleGuarded -> runGit command-injection"] }, - { - "id": "explain.cross-lines", - "issue": 3491, - "reason": "Cross-function declarations and hops are displayed 0-based (#3491).", - "baselineActual": [ - "hop 0 handleUnsafe@13", - "hop 1 blameSummary@6", - "hop 2 runGit@3", - "sink runGit@3", - "source handleUnsafe@13" - ] - }, { "id": "python.factory-qualified", "issue": 3498, diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index af5ad07f6..8e3e9857d 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -6139,15 +6139,27 @@ export class LocalBackend { const total = Number((ipCountRows[0] as any)?.total ?? (ipCountRows[0] as any)?.[0] ?? 0); const findings = ipRows.map((r: any) => { const decoded = decodeTaintPath(r.reason ?? r[5]); + // Cross-function lines are the endpoints' and hops' 0-based symbol + // `startLine` (the Function node property, and what interproc-emit + // encodes into the path codec), unlike the intra-procedural rows + // above, whose PDG statement lines are already 1-based. Convert at + // the response boundary like `context`/`query`/`impact` (#2377), so + // both layers of one response agree with the editor (#3491). const hops = decoded.ok - ? decoded.hops.map((h) => ({ function: h.variable, line: h.line })) + ? decoded.hops.map((h) => ({ function: h.variable, line: toDisplayLine(h.line) })) : []; return { interprocedural: true, file: String(r.file ?? r[0] ?? ''), sinkKind: decoded.ok ? (decoded.kind ?? 'unknown') : 'unknown', - source: { function: String(r.sourceFn ?? r[1] ?? ''), line: r.sourceLine ?? r[2] }, - sink: { function: String(r.sinkFn ?? r[3] ?? ''), line: r.sinkLine ?? r[4] }, + source: { + function: String(r.sourceFn ?? r[1] ?? ''), + line: toDisplayLine(r.sourceLine ?? r[2]), + }, + sink: { + function: String(r.sinkFn ?? r[3] ?? ''), + line: toDisplayLine(r.sinkLine ?? r[4]), + }, hops, ...(decoded.ok && decoded.truncated ? { pathIncomplete: true } : {}), }; @@ -6262,7 +6274,7 @@ export class LocalBackend { findings: pageFindings, totalFindings: totalFindings + interprocTotal, ...(truncated ? { truncated: true } : {}), - note: 'Intra-procedural (TAINTED, statement hops) AND cross-function (TAINT_PATH, function hops, `interprocedural: true`) flows are modeled, including sources read inside inline callbacks and returned handlers. Captured closure-variable, property/field, and implicit flows are NOT modeled; absence of a finding is not proof of safety. Cross-function findings are context-insensitive and may over-attribute among same-named callees. SANITIZES (kill) edges are queryable via cypher.', + note: 'Intra-procedural (TAINTED, statement hops) AND cross-function (TAINT_PATH, function hops, `interprocedural: true`) flows are modeled, including sources read inside inline callbacks and returned handlers. Captured closure-variable, property/field, and implicit flows are NOT modeled; absence of a finding is not proof of safety. Cross-function findings are context-insensitive and may over-attribute among same-named callees. All line numbers are 1-based; for cross-function findings, `source.line`, `sink.line` and `hops[].line` are the declaration lines of the functions, not statement lines. SANITIZES (kill) edges are queryable via cypher.', }; } diff --git a/gitnexus/test/integration/taint-explain.test.ts b/gitnexus/test/integration/taint-explain.test.ts index cd105c501..95768990f 100644 --- a/gitnexus/test/integration/taint-explain.test.ts +++ b/gitnexus/test/integration/taint-explain.test.ts @@ -530,6 +530,16 @@ withTestLbugDB( expect(hr!.sinkKind).toBe('command-injection'); expect(Array.isArray(hr!.hops)).toBe(true); expect((hr!.hops as unknown[]).length).toBeGreaterThan(0); + // Lines are 1-based declaration lines, as in the editor: `handle` is + // declared on line 7 of source.ts and `runIt` on line 6 of sink.ts + // (#3491 — the graph stores 0-based startLine; before the fix these + // read 6 and 5, one above every intra-procedural line in the same + // response). + expect(hr!.source).toEqual({ function: 'handle', line: 7 }); + expect(hr!.sink).toEqual({ function: 'runIt', line: 6 }); + const hops = hr!.hops as Array<{ function: string; line: number }>; + expect(hops[0]).toEqual({ function: 'handle', line: 7 }); + expect(hops.at(-1)).toEqual({ function: 'runIt', line: 6 }); }); it('symbol-anchored on the sink function surfaces the cross-function finding', async () => {