From b059ab3541ea68c2ce292955fc367a5de04b39ea Mon Sep 17 00:00:00 2001 From: Przemek Poppe <139065333+p-poppe@users.noreply.github.com> Date: Fri, 28 Aug 2026 19:47:41 +0200 Subject: [PATCH] PHP: detect generated-client Request(method, host . resourcePath) consumer shape (#3079) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(group/php): detect generated-client Request(method, host . resourcePath) openapi-generator-php / swagger-codegen PHP clients build every operation as `$resourcePath = '/foo/bar'; ...; new Request($method, $host . $resourcePath);` — a shape the PHP consumer patterns didn't cover (only `$client->verb('/path')` literal calls were matched), documented in the module's own docblock as a follow-up ("constant-folding the surrounding scope"). Adds a pattern for `new [Qualified\]Request(...)` constructor calls, with a conservative, single-scope backward constant fold: the last variable in the path argument's concatenation chain (generated clients build ` . `) is resolved to a `$var = '';` assignment in the same enclosing function/method body (or file scope) if one exists earlier in the same scope. No interprocedural resolution — a miss just leaves the endpoint undetected, never a wrong one. The HTTP verb is often itself a parameter in these generated clients (not a literal at the call site), so when it can't be resolved to a literal the detection reports a wildcard method (`'*'`), consistent with this project's existing manifest-link convention for a contract whose verb isn't pinned. `hasConsumerSignals` is widened to stay a proven superset of what `scan()` now detects (required by its own contract, checked by `http-consumer-signals.test.ts`). The docblock notes this is a deliberately narrow, single-scope fallback, not this language's entry into the shared cross-file constant-fold the other languages use (`constant-resolver.ts`, wired in via `java-const-resolver.ts` / `python-const-resolver.ts` / `js-const-resolver.ts`) — PHP has no such binding yet; adding one is a separate, larger project (this repo's PHP import resolution for `use`-statements is its own multi-file subsystem built for symbol/scope resolution, not constant extraction) and is out of scope here. Tests: 7 scan()-level cases (resolution across a member-access host, fully-qualified class name, purely literal call, negative — different function scope, negative — non-Request constructor, negative — non-HTTP literal, picking the LAST var in a 3-part concatenation), plus 2 new hasConsumerSignals cases and a negative. `tsc --noEmit` clean, `eslint` clean, full test/unit/group green (1035+/1037; 2 pre-existing native EBUSY failures on a `.lbug` file in bridge-meta-swap-window.test.ts, unrelated subsystem, reproduces in isolation on unchanged upstream too). * fix(group/php): fix two code-review findings in guzzle-request-ctor 1. lastConcatVariable silently returned the WRONG variable for a parenthesized right operand: `$host . ($resourcePath . $suffix)` fell through to the left operand (unhandled parenthesized_expression) and returned $host instead of looking inside the parens. Restored parenthesis unwrapping (present in an earlier draft, dropped during a simplification pass that didn't account for this fallthrough). 2. resolveLocalStringLiteral stopped at the nearest compound_statement, so a `new Request(...)` call nested in `if`/`try`/`foreach` inside the same function couldn't see an assignment made just above that block — despite the docblock's claim of covering the "enclosing function/method body". Now widens level by level (search the immediate block's preceding statements, then its own enclosing block, and so on), stopping at `program` so it still never crosses into a different function or the containing class body — verified by a regression test asserting exactly that boundary. Also documents the line-number choice (path argument, not the `new Request(` call site — the two differ for this pattern's characteristically multi-line calls) inline, matching the other three consumer patterns' convention in this file. 4 new regression tests (35 total in this file's suite): parenthesized right operand no longer mismatches, enclosing-block resolution across an `if`, and a negative case proving the widened search still respects the function boundary. tsc --noEmit clean, eslint clean. * fix(group/php): address gitnexus-check bot review on PR #3079 1. resolveLocalStringLiteral fell through an intervening non-literal reassignment: `$v = '/old'; $v = buildPath(); new Request(..., $v)` resolved to '/old' even though $v never holds that literal at the call site. The NEAREST assignment to the target variable now decides the outcome unconditionally — a non-string RHS stops the search (returns null) instead of letting the scan continue past it to an older, shadowed literal. This was a real "wrong answer", not a miss, directly contradicting the function's own documented invariant. 2. lastConcatVariable recursed into every binary_expression regardless of operator, so `$host && $resourcePath`, `$host + $resourcePath`, and `$host ?? $resourcePath` were walked exactly like `.` concatenation. Now checks operator === '.' before recursing. 3. hasConsumerSignals matches case-insensitively (`/i`), correctly, since PHP class names are case-insensitive at the language level — but scan() compared the resolved class name to 'Request' case-sensitively, so a valid `new request(...)` / `new \NS\REQUEST(...)` call would pass the parse-skip gate as a signal and then be silently dropped by scan() itself. Both sides now agree (case-insensitive compare in scan() too). 4. The first test's own PHP source assigned `$method = 'POST';` as a local variable but asserted `method: '*'` with a comment calling it "a parameter" — it wasn't; it was exactly the same locally-resolvable shape as $resourcePath. Fixed by (a) rewriting that test's source to show $method as a genuine function parameter (the shape generated clients actually use — the verb is fixed by the caller of the builder method), which is what the test intended to demonstrate, and (b) actually implementing symmetric resolution: method now resolves through the same resolveLocalStringLiteral fold as path when it IS a local variable, with a new test proving that case resolves to a literal method instead of a wildcard. 5 new regression tests (39 total in this file's suite, up from 35): non-literal-reassignment shadowing, non-concatenation operator rejected, case-insensitive class name match, and local-variable method resolution. tsc --noEmit clean, eslint clean. * fix(group/php): address second round of gitnexus-check bot review 1. Backward fold missed reassignments nested inside a preceding if/foreach/ try/switch: the scan only recognized direct expression_statement siblings as candidate assignments, so `if ($cond) { $v = '/new'; }` right before the call was invisible, and an OLDER, now-shadowed literal outside that block was returned instead — a real wrong answer whenever that branch runs. Any non-assignment sibling that contains an assignment to the target ANYWHERE inside it now stops the search (miss) rather than being skipped over, since whether that branch ran is unknown. 2. Level-by-level scope widening crossed anonymous-function boundaries without checking PHP's actual capture rule: closures capture NOTHING automatically, only variables listed in `use (...)` are visible inside — unlike arrow functions, which auto-capture everything and have no `compound_statement` body (never seen as a scope by this walk at all). Widening past a closure's body now checks its `use (...)` clause first; real PHP would throw "Undefined variable" for anything not captured, not resolve to a value from the enclosing scope. 3. lastConcatVariable still fell through to the LEFT operand whenever the right one wasn't a variable-or-nestable-expression — `new Request($m, $host . '/users')` (a trailing string literal, not a variable) resolved to $host instead of recognizing there's simply nothing to resolve at that position. Removed the left-operand fallback entirely: the rightmost position decides, full stop, matching the function's own "single lookup, not a fallback list" docblock (which the previous round already stated but the code didn't yet fully honor for this case). Also strengthened a test that the bot correctly flagged as non-diagnostic: "ignores an unrelated constructor" used an unresolvable $resourcePath, so it would have passed even with the class-name filter deleted. Now uses a fully resolvable path so the class-name filter is what the assertion actually exercises. 4 new regression tests (43 total, up from 39): shadowed-by-conditional- reassignment, closure boundary without use()-capture (negative), closure boundary WITH use()-capture (positive control), and trailing-literal concatenation no longer mistaken for the host variable. tsc --noEmit clean, eslint clean. * chore: trigger re-review (previous gitnexus-check report cited stale line numbers) * fix(group/php): stop scope widening at a function/method boundary The digest posted on PR #3079 (verified against the current file, not the stale HEAD it was generated from — three of its four findings were already fixed in prior commits) reproduced a real, still-present fourth issue: after exhausting a method's own body, widening continued straight to `program` (file/script scope) and could resolve a top-level variable into a class method — but PHP methods (and plain functions) have NO access to file-level variables without an explicit `global $v;`, which this resolver intentionally never adds support for. A file-level `$resourcePath = '/x';` could therefore leak into an unrelated method's `new Request(...)` as a real, wrong answer. Widening now stops unconditionally at a `function_definition` or `method_declaration` boundary — these get no automatic capture and no implicit global in PHP, unlike closures (already handled: an `anonymous_function` boundary stops unless `$target` is `use()`-captured). The call-site-at-file-scope case still resolves correctly, since `program` is reached directly there with no boundary to cross. 3 new regression tests (46 total): file-scope variable does not leak into a class method, does not leak into a plain top-level function either, and a positive control confirming file-scope-to-file-scope resolution still works when there's no function boundary at all. tsc --noEmit clean, eslint clean. --------- Co-authored-by: Gergő Magyar --- .../group/extractors/http-patterns/php.ts | 296 ++++++++++++++- .../unit/group/http-consumer-signals.test.ts | 6 + .../group/php-guzzle-request-ctor.test.ts | 352 ++++++++++++++++++ 3 files changed, 643 insertions(+), 11 deletions(-) create mode 100644 gitnexus/test/unit/group/php-guzzle-request-ctor.test.ts diff --git a/gitnexus/src/core/group/extractors/http-patterns/php.ts b/gitnexus/src/core/group/extractors/http-patterns/php.ts index bf2eb7aba..65a1896c9 100644 --- a/gitnexus/src/core/group/extractors/http-patterns/php.ts +++ b/gitnexus/src/core/group/extractors/http-patterns/php.ts @@ -15,20 +15,36 @@ import type { HttpDetection, HttpLanguagePlugin } from './types.js'; * Providers: * - Laravel `Route::get/post/...` * - * Consumers (string-literal URLs only): + * Consumers (string-literal URLs only, unless noted): * - Laravel HTTP client: `Http::get/post/put/delete/patch($url)` * - Guzzle / generic object method: `$client->get/post/...($url)` * - `file_get_contents($url)` + * - `new Request($method, $host . $resourcePath)` — the openapi-generator-php + * / swagger-codegen client shape. `$resourcePath` is resolved via a + * single-scope backward constant fold (see `resolveLocalStringLiteral`), + * not a string literal at the call site itself. * * The pipeline already uses `PHP.php_only` for ingesting plain `.php` * files (see `core/tree-sitter/parser-loader.ts`), and we do the same * here so Laravel route files are parsed with the right grammar dialect. * - * Scope notes: consumer patterns match string literals only. URLs built - * via binary concatenation (`$base . '/path'`), `sprintf`, or config - * lookup (`config('services.foo.base').'/path'`) are intentionally left - * for a follow-up — they require constant-folding the surrounding - * scope to be meaningful. + * Scope notes: consumer patterns match string literals only, with one + * narrow exception (above). URLs built via `sprintf`, config lookup + * (`config('services.foo.base').'/path'`), or a variable resolved from + * outside its own function/method body are intentionally left for a + * follow-up — they require constant-folding beyond one local scope to + * be meaningful. + * + * That narrow exception (`resolveLocalStringLiteral`) is a temporary, + * single-scope fallback, not this language's entry into the shared + * cross-file constant-fold used by the other languages in this plugin + * (`constant-resolver.ts`, wired in via `java-const-resolver.ts` / + * `python-const-resolver.ts` / `js-const-resolver.ts`). PHP has no such + * binding yet — adding one is a real, separate project (this repo's PHP + * import resolution for `use`-statements is its own multi-file subsystem + * under `ingestion/import-resolvers/php.ts`, built for symbol/scope + * resolution, not constant extraction) and is intentionally out of scope + * here. Tracked as a follow-up, not silently punted. */ const LARAVEL_ROUTE_SPEC: PatternSpec> = { @@ -71,11 +87,31 @@ const FILE_GET_CONTENTS_SPEC: PatternSpec> = { `, }; +/** + * `new Request($method, $host . $resourcePath)` — the shape swagger-codegen / + * openapi-generator-php emit for every operation of a generated API client + * (Guzzle's `\GuzzleHttp\Psr7\Request`, or a bare `Request` behind a `use` + * import). Matches both `(name)` and `(qualified_name)` class references; + * `scan()` below filters to the last path segment being exactly `Request` + * and resolves the concatenated path argument (see `resolveLocalStringLiteral`). + */ +const GUZZLE_REQUEST_CTOR_SPEC: PatternSpec> = { + meta: {}, + query: ` + (object_creation_expression + [(name) (qualified_name)] @class + (arguments + . (argument (_) @methodArg) + . (argument (_) @pathArg))) + `, +}; + interface PhpPatternBundle { laravelRoute: CompiledPatterns>; httpFacade: CompiledPatterns>; guzzleMember: CompiledPatterns>; fileGetContents: CompiledPatterns>; + guzzleRequestCtor: CompiledPatterns>; } const mk = (spec: PatternSpec>, suffix: string) => @@ -90,6 +126,7 @@ const PHP_PATTERNS: PhpPatternBundle = { httpFacade: mk(HTTP_FACADE_SPEC, 'http-facade'), guzzleMember: mk(GUZZLE_MEMBER_SPEC, 'guzzle-member'), fileGetContents: mk(FILE_GET_CONTENTS_SPEC, 'file-get-contents'), + guzzleRequestCtor: mk(GUZZLE_REQUEST_CTOR_SPEC, 'guzzle-request-ctor'), }; /** @@ -129,6 +166,183 @@ function isHttpUrlLiteral(path: string): boolean { return path.startsWith('http://') || path.startsWith('https://'); } +/** + * Last identifier segment of a class-name reference: `(name)` returns its + * own text, `(qualified_name)` returns the text of its last child (the + * unqualified class name — `\GuzzleHttp\Psr7\Request` → `Request`). + */ +function lastNameSegment(node: import('tree-sitter').SyntaxNode): string { + if (node.type === 'qualified_name') { + const last = node.child(node.childCount - 1); + return last ? last.text : node.text; + } + return node.text; +} + +/** + * Return the variable at the LAST position of a `.`-concatenation + * expression, if (and only if) that position is a plain variable — + * generated clients build ` . `, so the path segment + * is the one closest to the end. + * + * No fallback to an earlier operand: if the rightmost position is anything + * other than a variable, a parenthesized sub-expression, or a nested `.` + * concatenation (a literal, a function call, ...), that position is a real + * value we simply can't resolve — falling back to an EARLIER operand would + * silently substitute a different value (e.g. the host) for the one that's + * actually there. `null` here is a miss, not a signal to keep looking. + */ +function lastConcatVariable( + node: import('tree-sitter').SyntaxNode, +): import('tree-sitter').SyntaxNode | null { + if (node.type === 'variable_name') return node; + if (node.type === 'parenthesized_expression') { + const inner = node.namedChild(0); + return inner ? lastConcatVariable(inner) : null; + } + if (node.type === 'binary_expression') { + const operator = node.childForFieldName('operator'); + if (!operator || operator.text !== '.') return null; // not concatenation + const right = node.childForFieldName('right'); + return right ? lastConcatVariable(right) : null; + } + return null; +} + +/** + * True if `node`'s subtree assigns to `$target` ANYWHERE inside it, at any + * depth (including inside nested functions — deliberately over-broad: a + * false positive here only costs a miss in the caller, never a wrong + * answer, so there's no need to be precise about scoping inside the probe + * itself). + */ +function containsAssignmentTo(node: import('tree-sitter').SyntaxNode, target: string): boolean { + if (node.type === 'assignment_expression') { + const lhs = node.childForFieldName('left'); + if (lhs && lhs.type === 'variable_name' && lhs.text === target) return true; + } + for (let i = 0; i < node.namedChildCount; i++) { + const child = node.namedChild(i); + if (child && containsAssignmentTo(child, target)) return true; + } + return false; +} + +/** + * True if an `anonymous_function` node's `use (...)` clause lists + * `$target`. PHP closures capture NOTHING automatically — only variables + * named in `use (...)` are visible inside — unlike arrow functions + * (`fn() => ...`), which auto-capture everything by value and have no + * `compound_statement` body of their own, so they're never seen as a + * `scope` by the walk below in the first place. + */ +function anonymousFunctionCaptures( + anonFn: import('tree-sitter').SyntaxNode, + target: string, +): boolean { + for (let i = 0; i < anonFn.namedChildCount; i++) { + const child = anonFn.namedChild(i); + if (!child || child.type !== 'anonymous_function_use_clause') continue; + for (let j = 0; j < child.namedChildCount; j++) { + const v = child.namedChild(j); + if (v && v.type === 'variable_name' && v.text === target) return true; + } + return false; // has a use(...) clause, but $target isn't in it + } + return false; // no use(...) clause at all — nothing is captured +} + +/** + * Best-effort, single-scope constant fold: given a `variable_name` node + * referenced inside a `new Request(...)` argument, walk BACKWARD through + * the preceding statements of its immediately enclosing function/method + * body (or file scope, for top-level script code) looking for the nearest + * `$var = '';` assignment. + * + * "Enclosing body" is resolved level by level, not just the nearest + * `compound_statement` — a call site nested in `if`/`foreach`/`try` inside + * that function is still within the same function/method body, and a + * preceding assignment above that conditional must still be found. Each + * level searches only its own preceding siblings, then the search + * continues from the enclosing block itself one level up, UNLESS that + * block IS the body of a function/method/closure: + * - a regular `function_definition` or `method_declaration` boundary + * always stops the search — PHP gives a function or method no access + * to anything outside its own body (no automatic capture, no implicit + * global), so widening past one into the containing class or + * file-level scope would resolve a variable the call site could never + * actually see at runtime; + * - an `anonymous_function` boundary stops UNLESS `$target` is + * explicitly captured via `use (...)` — closures capture nothing + * automatically either. + * It stops at `program` regardless, for the case where the call site was + * at file/script scope all along. + * + * A preceding sibling that ISN'T a plain assignment but might reassign the + * target somewhere inside itself (an `if`/`foreach`/`try`/`switch`, ...) + * stops the search rather than being skipped over: whether that branch ran + * is unknown, so an older literal further back can't be trusted either. + * + * Deliberately conservative and bounded — no interprocedural resolution, + * no constant/property lookups. A miss just means the endpoint stays + * undetected, never a wrong one: this is exactly the class of case the + * module docblock flags as in-scope only for one local scope. + */ +function resolveLocalStringLiteral(varNode: import('tree-sitter').SyntaxNode): string | null { + const target = varNode.text; // includes the `$` sigil, e.g. "$resourcePath" + let cursor: import('tree-sitter').SyntaxNode = varNode; + + for (;;) { + let scope: import('tree-sitter').SyntaxNode | null = cursor.parent; + while (scope && scope.type !== 'compound_statement' && scope.type !== 'program') { + scope = scope.parent; + } + if (!scope) return null; + + let stmt: import('tree-sitter').SyntaxNode | null = cursor; + while (stmt && stmt.parent !== scope) stmt = stmt.parent; + if (!stmt) return null; + + let sibling = stmt.previousNamedSibling; + while (sibling) { + if (sibling.type === 'expression_statement') { + const inner = sibling.namedChild(0); + if (inner && inner.type === 'assignment_expression') { + const lhs = inner.childForFieldName('left'); + if (lhs && lhs.type === 'variable_name' && lhs.text === target) { + // The NEAREST assignment to this variable wins, full stop — an + // older literal further back is shadowed by this one even when + // this one isn't itself a resolvable string (`$v = f();`). + const rhs = inner.childForFieldName('right'); + return rhs && rhs.type === 'string' ? phpStringText(rhs) : null; + } + } + } else if (containsAssignmentTo(sibling, target)) { + return null; // reassigned somewhere inside a conditional/loop/try + } + sibling = sibling.previousNamedSibling; + } + + if (scope.type === 'program') return null; + const enclosing = scope.parent; + if (enclosing && enclosing.type === 'anonymous_function') { + // Closures capture nothing automatically — only what's use()'d. + if (!anonymousFunctionCaptures(enclosing, target)) return null; + } else if ( + enclosing && + (enclosing.type === 'function_definition' || enclosing.type === 'method_declaration') + ) { + // A regular function or method boundary — NOT a closure. PHP gives + // these no access to anything outside their own body (no automatic + // capture, no implicit global): widening past one into the + // containing class body or file-level scope would resolve a + // variable the call site could never actually see at runtime. + return null; + } + cursor = scope; // one block up: search resumes from this block's own position + } +} + export const PHP_HTTP_PLUGIN: HttpLanguagePlugin = { name: 'php-http', language: PHP.php_only, @@ -136,12 +350,16 @@ export const PHP_HTTP_PLUGIN: HttpLanguagePlugin = { // ingestion, so the graph is authoritative for PHP providers (#2138 Part 2). routeCoverage: 'complete', // Consumer signals scan() can detect: Laravel `Http::`, Guzzle client - // `->get/post/.../request(...)`, and `file_get_contents` of an HTTP URL. A - // provider-covered file with any of these must still be parsed (ingestion - // emits no FETCHES for PHP). Conservative — the `->verb(` shape over-matches - // ordinary method calls, which only costs a parse, never data. + // `->get/post/.../request(...)`, `file_get_contents` of an HTTP URL, and a + // generated-client `new ...Request(...)` constructor call. A provider-covered + // file with any of these must still be parsed (ingestion emits no FETCHES for + // PHP). Conservative — the `->verb(`/`new ...Request(` shapes over-match + // ordinary method calls and unrelated constructors, which only costs a + // parse, never data. hasConsumerSignals(content) { - return /Http::|file_get_contents|->\s*(get|post|put|delete|patch|request)\s*\(/i.test(content); + return /Http::|file_get_contents|->\s*(get|post|put|delete|patch|request)\s*\(|new\s+[\\\w]*Request\s*\(/i.test( + content, + ); }, scan(tree) { const out: HttpDetection[] = []; @@ -222,6 +440,62 @@ export const PHP_HTTP_PLUGIN: HttpLanguagePlugin = { }); } + for (const match of runCompiledPatterns(PHP_PATTERNS.guzzleRequestCtor, tree)) { + const classNode = match.captures.class; + const methodArg = match.captures.methodArg; + const pathArg = match.captures.pathArg; + if (!classNode || !methodArg || !pathArg) continue; + // PHP class names are case-insensitive at the language level, and + // `hasConsumerSignals` above matches case-insensitively (`/i`) for + // the same reason — this comparison must agree with it, or a valid + // `new request(...)` / `new \NS\REQUEST(...)` call would be waved + // through the parse-skip gate as a signal and then silently dropped + // here. + if (lastNameSegment(classNode).toLowerCase() !== 'request') continue; + + // Path: a direct string literal, or the last variable in a + // concatenation chain (see `lastConcatVariable`) resolved to a + // locally-assigned literal. + let path: string | null = null; + if (pathArg.type === 'string') { + path = phpStringText(pathArg); + } else { + const lastVar = lastConcatVariable(pathArg); + path = lastVar ? resolveLocalStringLiteral(lastVar) : null; + } + if (path === null || !isHttpClientPath(path)) continue; + + // The HTTP verb is a literal, a local variable resolved the same way + // as the path (see `resolveLocalStringLiteral` above), or — commonly + // in generated clients — a parameter of the enclosing builder method + // fixed by ITS caller, not by this call site. That last case needs + // the same interprocedural reach the module docblock rules out, so it + // falls through to a wildcard verb, matching this project's own + // convention for a contract whose verb isn't pinned (see manifest + // links, `http::*::`). + let method: string | null = null; + if (methodArg.type === 'string') { + method = phpStringText(methodArg); + } else if (methodArg.type === 'variable_name') { + method = resolveLocalStringLiteral(methodArg); + } + + out.push({ + role: 'consumer', + framework: 'guzzle-request-ctor', + method: method ? method.toUpperCase() : '*', + path, + name: null, + // Line of the path ARGUMENT, not the `new Request(` call — same + // choice the other three consumer patterns in this file make, but + // this is the one pattern where the two routinely differ (generated + // clients wrap the call across multiple lines). Line-span + // containment still resolves to the right symbol either way. + line: pathArg.startPosition.row + 1, + confidence: 0.6, + }); + } + return out; }, }; diff --git a/gitnexus/test/unit/group/http-consumer-signals.test.ts b/gitnexus/test/unit/group/http-consumer-signals.test.ts index 087524cfe..6d98f44c7 100644 --- a/gitnexus/test/unit/group/http-consumer-signals.test.ts +++ b/gitnexus/test/unit/group/http-consumer-signals.test.ts @@ -46,6 +46,8 @@ describe('PHP hasConsumerSignals — superset of scan() consumer idioms', () => ['Laravel Http facade', "Http::get('/api/x');"], ['Guzzle member call', "$client->post('/api/x', []);"], ['file_get_contents', "file_get_contents('https://x/api');"], + ['bare Request constructor', 'new Request($method, $host . $resourcePath);'], + ['namespaced Request constructor', "new Foo\\Bar\\Request('GET', $url);"], ])('detects %s', (_label, src) => { expect(has(PHP_HTTP_PLUGIN, src)).toBe(true); }); @@ -53,6 +55,10 @@ describe('PHP hasConsumerSignals — superset of scan() consumer idioms', () => it('returns false for a pure Laravel route file (provider only)', () => { expect(has(PHP_HTTP_PLUGIN, "Route::get('/api/a/list', 'AController@list');")).toBe(false); }); + + it('returns false for an unrelated constructor call (not *Request)', () => { + expect(has(PHP_HTTP_PLUGIN, 'new Response($body, 200);')).toBe(false); + }); }); describe('Python hasConsumerSignals — superset of scan() consumer idioms', () => { diff --git a/gitnexus/test/unit/group/php-guzzle-request-ctor.test.ts b/gitnexus/test/unit/group/php-guzzle-request-ctor.test.ts new file mode 100644 index 000000000..9e6f44b4a --- /dev/null +++ b/gitnexus/test/unit/group/php-guzzle-request-ctor.test.ts @@ -0,0 +1,352 @@ +import { describe, expect, it } from 'vitest'; +import Parser from 'tree-sitter'; +import PHP from 'tree-sitter-php'; +import { PHP_HTTP_PLUGIN } from '../../../src/core/group/extractors/http-patterns/php.js'; + +const parser = new Parser(); +parser.setLanguage(PHP.php_only); + +const scan = (src: string) => PHP_HTTP_PLUGIN.scan(parser.parse(src)); +const consumers = (src: string) => scan(src).filter((d) => d.role === 'consumer'); + +describe('PHP guzzle-request-ctor pattern', () => { + it('resolves a locally-assigned $resourcePath concatenated with a member-access host, method is a real parameter', () => { + // $method is a FUNCTION PARAMETER here (the shape openapi-generator-php + // actually emits — the verb is fixed by the caller of this builder + // method, not assigned inside its body), not a local variable that + // happens to share the resolver's single-scope shape. A local + // `$method = 'POST';` immediately before the call would in fact resolve + // via the same fold as `$resourcePath` — that's a different case, + // covered separately below. + const src = `operationHost . $resourcePath + ); + return $this->client->send($request); + } +} +`; + const found = consumers(src); + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ + framework: 'guzzle-request-ctor', + method: '*', // $method is a parameter, not a literal at this call site + path: '/payments/pay', + }); + }); + + it('resolves a fully-qualified GuzzleHttp/Psr7/Request with a literal verb', () => { + // Built via join(), not a literal backslash in this source file: a + // template-literal backslash-escape is easy to mis-transcribe (dropped + // silently by the JS/TS escape rules for an unrecognized `\`), so + // this sidesteps that entirely and is robust regardless of how the file + // itself gets written to disk. + const bs = String.fromCharCode(92); + const qualified = ['', 'GuzzleHttp', 'Psr7', 'Request'].join(bs); + const src = [ + ' { + const src = ` { + const src = `host . $resourcePath); + return $request; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('ignores an unrelated constructor whose class name does not end in "Request"', () => { + // $resourcePath IS resolvable here (unlike an earlier version of this + // test) — the class-name filter must be the reason this produces no + // detection, not an incidental miss elsewhere in the pipeline. Without + // a resolvable path, this test would pass even with the class-name + // filter deleted. + const src = `host . $resourcePath); + return $response; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('rejects a resolved literal that is not an HTTP-looking path', () => { + const src = `host . $resourcePath); + return $request; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('prefers the LAST variable in a 3-part concatenation (path, not an earlier segment)', () => { + const src = ` { + // Regression: an earlier version of lastConcatVariable treated an + // unhandled `parenthesized_expression` as "no variable here" and fell + // through to the LEFT operand — silently returning $host instead of + // failing to find anything inside the parens. + const src = ` { + // Regression: resolveLocalStringLiteral stopped at the nearest + // compound_statement (the `if` block), not the enclosing function body, + // so an assignment made just above the `if` — in the same function — + // was invisible to a `new Request(...)` call nested inside it. + const src = `isValid()) { + $request = new Request($method, $this->host . $resourcePath); + return $request; + } + return null; +} +`; + const found = consumers(src); + expect(found).toHaveLength(1); + expect(found[0].path).toBe('/payments/pay'); + }); + + it('still does not cross into a sibling function even when searching level by level', () => { + // The level-by-level widening must stop at `program` / the enclosing + // function boundary — it must not walk into a DIFFERENT function's body + // just because that function is a preceding sibling statement. + const src = `host . $resourcePath); + return $request; + } +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('does not resolve through an intervening non-literal reassignment (last write wins)', () => { + // Regression: the backward scan used to skip PAST an assignment whose + // RHS wasn't a string literal, landing on an older literal that the + // variable no longer holds at the call site — a wrong answer, not a + // miss. The nearest assignment must decide the outcome, full stop. + const src = `host . $resourcePath); + return $request; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('does not treat a non-concatenation binary expression as a path candidate', () => { + // Regression: lastConcatVariable recursed into ANY binary_expression + // without checking the operator, so `$host ?? $resourcePath` (or `&&`, + // `+`, ...) was walked exactly like `.` concatenation. + const src = `host ?? $resourcePath); + return $request; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('matches a lowercase "request" constructor — PHP class names are case-insensitive', () => { + const src = ` { + const src = `host . $resourcePath); + return $request; +} +`; + const found = consumers(src); + expect(found).toHaveLength(1); + expect(found[0]).toMatchObject({ method: 'POST', path: '/payments/pay' }); + }); + + it('does not use a stale literal shadowed by a reassignment inside a preceding if block', () => { + // Regression: the backward scan only inspected direct expression_statement + // siblings, so `$resourcePath = '/new';` nested inside an `if` right + // before the call was invisible, and the OLDER `/old` (outside the `if`) + // was returned instead — an unconditional wrong answer whenever that + // branch runs, not a miss. + const src = `host . $resourcePath); + return $request; +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('does not resolve a variable across an anonymous-function boundary it was not use()-captured into', () => { + // Regression: level-by-level widening climbed straight from the + // closure's body to the enclosing method's body without checking PHP's + // actual capture rule (closures capture NOTHING unless listed in + // `use (...)`), resolving a variable the closure can't actually see — + // real PHP would throw "Undefined variable" here, not build this path. + const src = `host . $resourcePath); + }; + return $build(); +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('DOES resolve a variable across an anonymous-function boundary it WAS use()-captured into', () => { + const src = `host . $resourcePath); + }; + return $build(); +} +`; + const found = consumers(src); + expect(found).toHaveLength(1); + expect(found[0].path).toBe('/payments/pay'); + }); + + it('does not fall back to the host when the concatenation ends in a string literal, not a variable', () => { + // Regression: lastConcatVariable fell through to the LEFT operand when + // the right one wasn't a variable, so `$host . '/users'` resolved to + // $host instead of recognizing the trailing literal isn't a variable at + // all — if $host happened to be an HTTP URL locally, that URL would be + // emitted as the path instead of a miss. + const src = ` { + // Regression: after exhausting a method's own body, widening went + // straight to `program` (file scope) and found the top-level literal — + // but PHP methods have NO access to file-level variables without an + // explicit `global $v;`, which this resolver deliberately never adds + // support for. This produced a wrong contract, not a miss. + const src = `host . $resourcePath); + return $request; + } +} +`; + expect(consumers(src)).toHaveLength(0); + }); + + it('does not resolve a variable from file scope into a plain top-level function either', () => { + const src = ` { + const src = `