mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-01 02:01:24 +00:00
fix(impact): fail closed on id-less targets and follow ??/||/?: callable values (#3354)
#3354 reports `impact` returning a byte-identical 1037/CRITICAL/`exact` result for three unrelated targets, with only `target.name` differing. The reporter's `target` had no `id` and no `filePath`, which the traversal path always emits, and a synthetic reproduction of their monorepo (pnpm, Cloudflare worker-configuration.d.ts in five packages, Hono, a Durable Object) resolves every target correctly on main. So the identical result was not reproduced. The repro did surface two real gaps and one hardening point: - `_runImpactBFS` now throws when the target has no node id. Every caller already catches, so impact reports `impactedCount: null, risk: UNKNOWN` instead of a normal-looking blast radius that cannot be about this symbol. - Callable-value flow followed only a single designator on the RHS, so `const sweep = env.__sweep ?? runSweep; await sweep(env)` produced no flow and `scheduled` was missing as a caller of `runSweep`, while the result still claimed `epistemic: exact`. Each branch of `??`, `||`, `or`, and `?:` now flows into the binding (language-neutral: operator and field names, no language checks). Parse cache bumped 104 -> 112 (105-111 are claimed by open PR #3326). - A whitespace-only `target_uid` (strict adapters materialize omitted optional strings) is treated as omitted and falls back to the name. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
3d24743241
commit
533af3dafd
9 changed files with 183 additions and 6 deletions
|
|
@ -216,7 +216,15 @@ export function synthesizeCallableFlowCaptures(
|
|||
|
||||
const out: CaptureMatch[] = [];
|
||||
for (const assignment of assignments) {
|
||||
emitAssignmentFact(assignment, knownCallableNames, valueBindings, options, out);
|
||||
for (const source of valueAlternatives(assignment.source)) {
|
||||
emitAssignmentFact(
|
||||
{ ...assignment, source },
|
||||
knownCallableNames,
|
||||
valueBindings,
|
||||
options,
|
||||
out,
|
||||
);
|
||||
}
|
||||
}
|
||||
for (const fn of functions) emitFormalFacts(fn, options, out);
|
||||
for (const node of nodes) {
|
||||
|
|
@ -587,6 +595,34 @@ function assignmentParts(
|
|||
return [{ container: node, destination, source }];
|
||||
}
|
||||
|
||||
/** Operators whose result is one of their operands, not a computed value. */
|
||||
const VALUE_SELECTING_OPERATORS = new Set(['??', '||', 'or', '?:']);
|
||||
|
||||
/**
|
||||
* The operands a value-selecting expression can evaluate to (#3354):
|
||||
* `a ?? b`, `a || b`, `a or b`, and `c ? a : b` each yield one of their
|
||||
* branches, so each branch flows into the destination. Anything else is its
|
||||
* own single alternative, which leaves every other source shape untouched.
|
||||
*/
|
||||
function valueAlternatives(node: SyntaxNode): SyntaxNode[] {
|
||||
let inner = node;
|
||||
while (inner.type.includes('parenthesized') && inner.namedChildCount === 1) {
|
||||
inner = inner.namedChild(0)!;
|
||||
}
|
||||
const consequence = inner.childForFieldName('consequence');
|
||||
const alternative = inner.childForFieldName('alternative');
|
||||
if (consequence !== null && alternative !== null && inner.childForFieldName('condition')) {
|
||||
return [...valueAlternatives(consequence), ...valueAlternatives(alternative)];
|
||||
}
|
||||
const left = inner.childForFieldName('left');
|
||||
const right = inner.childForFieldName('right');
|
||||
const operator = inner.childForFieldName('operator')?.type;
|
||||
if (left !== null && right !== null && operator && VALUE_SELECTING_OPERATORS.has(operator)) {
|
||||
return [...valueAlternatives(left), ...valueAlternatives(right)];
|
||||
}
|
||||
return [node];
|
||||
}
|
||||
|
||||
function emitAssignmentFact(
|
||||
assignment: AssignmentParts,
|
||||
knownCallableNames: ReadonlySet<string>,
|
||||
|
|
|
|||
|
|
@ -4431,7 +4431,10 @@ export class LocalBackend {
|
|||
}
|
||||
| { kind: 'not_found' }
|
||||
> {
|
||||
const { uid, name, include_content } = query;
|
||||
const { name, include_content } = query;
|
||||
// A blank uid is an omitted optional field (strict adapters send " "/""),
|
||||
// not a lookup key — fall through to the name instead of `not_found`.
|
||||
const uid = query.uid?.trim();
|
||||
const selectClause = `n.id AS id, n.name AS name, labels(n)[0] AS type, n.filePath AS filePath, n.startLine AS startLine, n.endLine AS endLine${include_content ? ', n.content AS content' : ''}`;
|
||||
|
||||
// Direct UID — zero-ambiguity path.
|
||||
|
|
@ -7194,7 +7197,7 @@ export class LocalBackend {
|
|||
);
|
||||
|
||||
if (outcome.kind === 'not_found') {
|
||||
const missing = params.target_uid ?? target;
|
||||
const missing = params.target_uid?.trim() || target;
|
||||
// not_found = no resolved symbol, so the envelope keeps the partial-but-
|
||||
// typed target (typed PdgImpactTarget — there is no id/type/filePath yet).
|
||||
const notFoundTarget: PdgImpactTarget = { name: target };
|
||||
|
|
@ -8017,6 +8020,14 @@ export class LocalBackend {
|
|||
const confidenceFilter = safeMinConfidence > 0 ? ' AND r.confidence >= $minConfidence' : '';
|
||||
|
||||
const symId = sym.id || sym[0];
|
||||
// #3354: a walk with no anchor id cannot say anything about THIS symbol,
|
||||
// yet it still ships a normal-looking `exact` result with the target's
|
||||
// name echoed back. Throw so every caller's catch reports UNKNOWN instead.
|
||||
if (!symId) {
|
||||
throw new Error(
|
||||
`Impact target '${sym.name || sym[1] || '?'}' resolved without a node id; refusing to report a blast radius`,
|
||||
);
|
||||
}
|
||||
|
||||
// #1858 — kick off the epistemic boundary probe concurrently with the BFS.
|
||||
// It depends only on symId/symType/symName (all known now) and touches no
|
||||
|
|
|
|||
|
|
@ -778,7 +778,11 @@ import { copyV8CacheIfPresent, tryLoadV8Cache, writeV8CacheFile } from './v8-sid
|
|||
// v104 (#3339 review): TS/JS pair-HOC queries now name object-pair
|
||||
// `mutation(withAuth(arrow))` handlers. Warm caches replay the pre-fix
|
||||
// capture set (anonymous arrows, no Function name), so both stores re-extract.
|
||||
const SCHEMA_BUMP = 104;
|
||||
// v112 (#3354): callable-value flow now follows each branch of `a ?? f`,
|
||||
// `a || f`, and `c ? f : g`. Warm caches replay the pre-fix flow facts, which
|
||||
// have no flow for those assignments, so both stores re-extract. 105-111 are
|
||||
// claimed by open PR #3326 (Elixir).
|
||||
const SCHEMA_BUMP = 112;
|
||||
const GITNEXUS_PKG_VERSION = (() => {
|
||||
try {
|
||||
// package.json sits at gitnexus/package.json — two levels up from
|
||||
|
|
|
|||
23
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/index.ts
vendored
Normal file
23
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/index.ts
vendored
Normal file
|
|
@ -0,0 +1,23 @@
|
|||
import { runSweep, runAlias, runOr, runThen, runElse } from './sweep';
|
||||
|
||||
type Handler = (env: unknown) => Promise<void>;
|
||||
|
||||
export async function aliasOnly(env: unknown) {
|
||||
const run = runAlias;
|
||||
await run(env);
|
||||
}
|
||||
|
||||
export async function nullish(env: { __sweep?: Handler }) {
|
||||
const sweep = env.__sweep ?? runSweep;
|
||||
await sweep(env);
|
||||
}
|
||||
|
||||
export async function logicalOr(env: { override?: Handler }) {
|
||||
const run = env.override || runOr;
|
||||
await run(env);
|
||||
}
|
||||
|
||||
export async function ternary(env: unknown, fast: boolean) {
|
||||
const run = fast ? runThen : runElse;
|
||||
await run(env);
|
||||
}
|
||||
5
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/sweep/index.ts
vendored
Normal file
5
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/sweep/index.ts
vendored
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
export async function runSweep(env: unknown): Promise<void> {}
|
||||
export async function runAlias(env: unknown): Promise<void> {}
|
||||
export async function runOr(env: unknown): Promise<void> {}
|
||||
export async function runThen(env: unknown): Promise<void> {}
|
||||
export async function runElse(env: unknown): Promise<void> {}
|
||||
10
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/worker.ts
vendored
Normal file
10
gitnexus/test/fixtures/lang-resolution/typescript-callable-alternatives/worker.ts
vendored
Normal file
|
|
@ -0,0 +1,10 @@
|
|||
import { runSweep } from './sweep';
|
||||
|
||||
// The reporter's shape: a Cloudflare worker's default-export object whose
|
||||
// `scheduled` handler lets tests inject a replacement sweep.
|
||||
export default {
|
||||
async scheduled(_c: unknown, env: { __sweep?: typeof runSweep }) {
|
||||
const sweep = env.__sweep ?? runSweep;
|
||||
await sweep(env);
|
||||
},
|
||||
};
|
||||
|
|
@ -0,0 +1,53 @@
|
|||
/**
|
||||
* TypeScript: a callable chosen by a value-selecting expression (#3354).
|
||||
*
|
||||
* `const sweep = env.__sweep ?? runSweep; await sweep(env)` is how the
|
||||
* reporter's Cloudflare worker made its sweep injectable in tests. The
|
||||
* callable-value flow only accepted a single designator on the right-hand
|
||||
* side, so the `??` produced no flow, `scheduled` never showed up as a caller
|
||||
* of `runSweep`, and `impact` answered with one caller fewer while still
|
||||
* claiming `epistemic: "exact"`. Each branch of `??`, `||`, and `?:` can be
|
||||
* the value that is later invoked, so each branch is a flow into the binding.
|
||||
*/
|
||||
import { describe, it, expect, beforeAll } from 'vitest';
|
||||
import path from 'path';
|
||||
import {
|
||||
FIXTURES,
|
||||
getRelationships,
|
||||
edgeSet,
|
||||
runPipelineFromRepo,
|
||||
type PipelineResult,
|
||||
} from './helpers.js';
|
||||
|
||||
describe('TypeScript callable chosen by ?? / || / ?:', () => {
|
||||
let result: PipelineResult;
|
||||
|
||||
beforeAll(async () => {
|
||||
result = await runPipelineFromRepo(
|
||||
path.join(FIXTURES, 'typescript-callable-alternatives'),
|
||||
() => {},
|
||||
);
|
||||
}, 60000);
|
||||
|
||||
const calls = () => edgeSet(getRelationships(result, 'CALLS'));
|
||||
|
||||
it('control: a plain alias reaches its callee', () => {
|
||||
expect(calls()).toContain('aliasOnly → runAlias');
|
||||
});
|
||||
|
||||
it('`a ?? fn` reaches fn', () => {
|
||||
expect(calls()).toContain('nullish → runSweep');
|
||||
});
|
||||
|
||||
it('`a ?? fn` inside an object-literal method reaches fn (worker `scheduled`)', () => {
|
||||
expect(calls()).toContain('scheduled → runSweep');
|
||||
});
|
||||
|
||||
it('`a || fn` reaches fn', () => {
|
||||
expect(calls()).toContain('logicalOr → runOr');
|
||||
});
|
||||
|
||||
it('`c ? f : g` reaches both branches', () => {
|
||||
expect(calls()).toEqual(expect.arrayContaining(['ternary → runThen', 'ternary → runElse']));
|
||||
});
|
||||
});
|
||||
|
|
@ -622,6 +622,38 @@ describe('LocalBackend.callTool', () => {
|
|||
});
|
||||
});
|
||||
|
||||
it('reports UNKNOWN instead of a blast radius when the target resolves without a node id (#3354)', async () => {
|
||||
// Every query returns the same id-less row: the resolver picks it as the
|
||||
// single match, and the frontier query would answer for no symbol at all.
|
||||
(executeParameterized as any).mockResolvedValue([{ name: 'runSweep', type: 'Function' }]);
|
||||
|
||||
const result = await backend.callTool('impact', { target: 'runSweep', direction: 'upstream' });
|
||||
|
||||
expect(result).toMatchObject({
|
||||
target: { name: 'runSweep' },
|
||||
impactedCount: null,
|
||||
risk: 'UNKNOWN',
|
||||
});
|
||||
expect(result.error).toMatch(/without a node id/);
|
||||
expect(result).not.toHaveProperty('byDepthCounts');
|
||||
});
|
||||
|
||||
it('treats a whitespace target_uid as omitted and resolves the name (#3354)', async () => {
|
||||
(executeParameterized as any).mockResolvedValue([]);
|
||||
|
||||
const result = await backend.callTool('impact', {
|
||||
target: 'validate',
|
||||
target_uid: ' ',
|
||||
direction: 'upstream',
|
||||
});
|
||||
|
||||
// Name resolution ran (no rows → not found by NAME), not a lookup of uid ' '.
|
||||
expect(result.error).toBe("Target 'validate' not found");
|
||||
const boundParams = (executeParameterized as any).mock.calls.map((c: unknown[]) => c[2]);
|
||||
expect(boundParams).not.toContainEqual(expect.objectContaining({ uid: expect.anything() }));
|
||||
expect(boundParams).toContainEqual(expect.objectContaining({ symName: 'validate' }));
|
||||
});
|
||||
|
||||
it('normalizes impact aliases before @group forwarding', async () => {
|
||||
resolveAtMemberMock.mockResolvedValue({ ok: true, repoPath: '/tmp/test-project' });
|
||||
const groupImpactSpy = vi
|
||||
|
|
|
|||
|
|
@ -288,8 +288,10 @@ describe('PARSE_CACHE_VERSION', () => {
|
|||
// Moved 103 -> 104 for #3339 review: pair-HOC queries name
|
||||
// `mutation(withAuth(arrow))` object-pair handlers. Warm caches replay
|
||||
// anonymous arrows, so both stores re-extract.
|
||||
it('pins SCHEMA_BUMP to 104 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339)', () => {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(104);
|
||||
// Moved 104 -> 112 for #3354: callable-value flow follows `??`/`||`/`?:`
|
||||
// branches. 105-111 are claimed by open PR #3326.
|
||||
it('pins SCHEMA_BUMP to 112 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354)', () => {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(112);
|
||||
expect(PARSE_CACHE_BUCKET_COUNT).toBe(128);
|
||||
// The PREVIOUS version must fail the reuse gate, not merely differ from the
|
||||
// current one — a hardcoded number outside the conflict hunk rebases cleanly
|
||||
|
|
@ -298,6 +300,7 @@ describe('PARSE_CACHE_VERSION', () => {
|
|||
for (const taken of [
|
||||
59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81,
|
||||
82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103,
|
||||
104, 105, 106, 107, 108, 109, 110, 111,
|
||||
]) {
|
||||
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(taken);
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue