fix(group): detect httpx async consumers#1408
Conversation
|
@juyua9 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
|
Claude finished @magyargergo's task in 4m 34s —— View job PR #1408 Review — fix(group): detect httpx async consumersReview bar for this PR
Current PR state
Understanding of the changeBefore this PR, the Python HTTP plugin scanned for:
This PR adds a two-phase approach:
Out of scope from #1184: NestJS Findings[MEDIUM] Test framework assertion is semantically over-broad
[LOW] No negative test coverage for false-positive prevention
[LOW]
|
| Pattern | Reason |
|---|---|
from httpx import AsyncClient; client = AsyncClient() |
RHS is call(identifier), not call(attribute) |
import httpx as hx; client = hx.AsyncClient() |
Module text is "hx", fails #eq? "httpx" |
c = self._client; await c.get("/x") |
Alias chain — c not in tracked set |
return httpx.AsyncClient().get("/x") |
Inline call, no assignment captured |
client = make_httpx_client() |
Factory function, RHS not httpx.AsyncClient(...) |
All are acceptable false negatives for a targeted PR; the more common patterns (attribute-held and async-with alias) are covered.
Consumer detection assessment
Verb pattern (HTTPX_ASYNC_CLIENT_VERB_PATTERNS, lines 122–137):
(call
function: (attribute object: (_) @client attribute: (identifier) @method (#match? ...))
arguments: (argument_list . (string) @path))
- The
argument_list .anchor (first-child positional anchor in tree-sitter) requires the path string to be the first positional argument. This works forclient.get("/x")andclient.post("/x", json=data)(keyword args follow the positional first arg). It does NOT work for pure keyword-arg formclient.get(url="/x")— acceptable out-of-scope limitation. method.text.toUpperCase()produces the canonical HTTP method. ✓- The
httpxAsyncClients.has(clientNode.text)guard prevents any untracked object from producing a detection —dict.get("/x"),response.get("/foo"),service.request("POST", "/x")are all rejected. ✓
Generic pattern (HTTPX_ASYNC_CLIENT_GENERIC_PATTERNS, lines 140–155):
(call
function: (attribute object: (_) @client attribute: ... (#eq? "request"))
arguments: (argument_list . (string) @http_method (string) @path))
- Requires both method and path to be positional string literals.
unquoteLiteralstrips quotes;methodRaw.toUpperCase()normalizes. Non-literal method/path → bothunquoteLiteralresults are null → skipped. ✓ - Keyword form
client.request(method="POST", url="/x")is not supported — acceptable.
Static-analysis precision / recall assessment
Expected true positives — all correctly detected:
self._client = httpx.AsyncClient()
await self._client.get("/topic") # ✓
async with httpx.AsyncClient() as client:
await client.post("/questions/dup-check") # ✓
await self._client.request("POST", "/questions/import") # ✓False positives — correctly prevented:
data = {}
data.get("/x") # "data" not in tracked set → skip ✓
service.request("POST", "/x") # "service" not in tracked set → skip ✓
client = something_else.AsyncClient() # module != "httpx" → not tracked ✓
await client.get("/x") # "client" not in set → skip ✓Known false negatives (acceptable scope):
- Direct import:
from httpx import AsyncClient+ bareAsyncClient() - Aliased import:
import httpx as hx+hx.AsyncClient() - Reassigned alias:
c = self._client; await c.get("/x") - Keyword-only args:
client.get(url="/x") - Typed annotation:
client: httpx.AsyncClient = httpx.AsyncClient()
All are edge cases relative to the issue's target patterns. A follow-up issue is recommended.
HTTP contract / group matching assessment
Emitted HttpDetection objects:
role: 'consumer'✓framework: 'python-httpx'✓ (distinct from'python-requests','fastapi')method: uppercased from capture ✓path: raw literal → fed to orchestrator'snormalizeConsumerPath✓- Absolute URL
https://svc.local/questions/duplicate-check→new URL(...).pathname→/questions/duplicate-check→http::POST::/questions/duplicate-check✓ - Relative path
/topic→http::GET::/topic✓
- Absolute URL
name: null— consistent with all other consumer plugins ✓confidence: 0.7— consistent with all other source-scan consumer plugins ✓
Group matching interaction: The orchestrator uses method + normalized path for contract ID generation, not framework. python-httpx consumers will correctly match providers from FastAPI, Spring, NestJS, etc. when method + path align. Framework field is stored in meta only. Confidence 0.7 does not suppress matching. dedupeContracts uses contractId|filePath|name as key — duplicate calls in the same file are de-duplicated. ✓
No schema changes, no new contract types, no provider contracts emitted. ✓
Test assessment
The new test (http-route-extractor.test.ts:431–464) covers:
- ✅ Attribute-held client:
self._client = httpx.AsyncClient()+await self._client.get("/topic") - ✅ Generic
.request():await self._client.request("POST", "/questions/import") - ✅
async withalias:async with httpx.AsyncClient() as client:+await client.post("https://svc.local/questions/duplicate-check") - ✅ Absolute URL normalization:
https://svc.local/questions/duplicate-check→http::POST::/questions/duplicate-check - ✅ Framework metadata assertion (currently correct, though over-broad — see Finding 1)
- ✅ Three expected contract IDs verified
Would tests fail before this PR? Yes — the three expect(...).toBeDefined() assertions would all fail on the unpatched code since no httpx consumer patterns existed.
Missing test coverage:
- ❌ Local variable client (only attribute
self._clientis tested;client = httpx.AsyncClient()is not a separate case in the test) - ❌ Negative: arbitrary
.get()/.request()on untracked objects - ❌
.put,.delete,.patchverb coverage - ❌ Non-literal method/path → skipped (document this guarantee)
- ❌ Additional positional kwargs:
client.post("/x", json=payload)still matches (good behavior, untested) - ❌
from httpx import AsyncClient→ correctly undetected (a negative case worth documenting)
Scope / issue hygiene assessment
- PR body correctly scopes to "Python
httpx.AsyncClientslice of [bug][group] HTTP consumer extractor misses httpx.AsyncClient, NestJS HttpService, and project axios wrappers #1184" ✓ - No claim of fixing NestJS
HttpServiceor axios wrappers ✓ - No unrelated extractor changes ✓
- No generated/package-lock/release changes ✓
- Issue [bug][group] HTTP consumer extractor misses httpx.AsyncClient, NestJS HttpService, and project axios wrappers #1184 must remain open — two of three slices (NestJS HttpService, project axios wrappers) are unaddressed. If GitHub auto-closes [bug][group] HTTP consumer extractor misses httpx.AsyncClient, NestJS HttpService, and project axios wrappers #1184 via
Fixes #1184, the maintainers should reopen it or split into sub-issues.
Hidden Unicode / hygiene assessment
grep -Pn "[\x{202A}-\x{202E}\x{2066}-\x{2069}]" python.ts test.ts
→ No bidi chars found ✓
grep -Pn "[^\x00-\x7F]" python.ts test.ts
→ Matches: section comment separator lines only (─ U+2500, — U+2014)
All non-ASCII matches are:
─(U+2500 BOX DRAWINGS LIGHT HORIZONTAL) in section comment separators on lines 27, 46, 64, 83, 102, 121, 139 ofpython.ts—(U+2014 EM DASH) indescribe()block strings in the test file
These are consistent with the existing codebase style (pre-existing sections in python.ts use the same separator), are in comments and string labels only, and are entirely absent from tree-sitter query strings, regex patterns, and test fixture Python source. No bidi control characters. No invisible whitespace. No block.
The GitHub "hidden Unicode" warning triggered by these decorative separators is a false alarm relative to the security concern.
Performance / maintainability assessment
collectHttpxAsyncClients(tree)runs 2 compiled pattern bundles in a single pass — O(AST match count). No full-file string search.- The 4 new compiled pattern bundles (
ASSIGN,WITH_ALIAS,VERB,GENERIC) are compiled once at module load time viacompilePatterns— zero per-file compilation overhead. ✓ - Consumer scan is O(call nodes) per file — well within normal tree-sitter performance bounds.
- Pattern names (
HTTPX_ASYNC_CLIENT_ASSIGN_PATTERNS, etc.) are clear and follow the existing naming convention. ✓ - Section comments match the style of existing sections in the same file. ✓
- Future extensions (sync
httpx.Client, direct imports, aliased imports) can be added by appending patterns tocollectHttpxAsyncClientswithout changing the consumer scan loops. Architecture supports clean extension. ✓
Final verdict
production-ready with minor follow-ups
The core implementation is architecturally sound. The two-phase tracked-client approach correctly prevents false positives from arbitrary .get() / .request() calls — the httpxAsyncClients.has(clientNode.text) guard is the right mechanism and it works correctly for both attribute-held clients and async with aliases. Path and method normalization reuse the existing normalizeConsumerPath pipeline unchanged. Contract IDs, confidence, and framework metadata are consistent with the rest of the consumer plugin surface.
The outstanding items are test coverage gaps (negative cases, .put/.delete/.patch verbs, framework assertion scope) and undocumented false negatives for direct and aliased imports. None of these constitute a correctness defect in the stated scope — the patterns the PR claims to detect are detected correctly, and the patterns it should not detect are correctly rejected. Before merge, it is worth confirming CI completes green, and the maintainers should ensure issue #1184 is not auto-closed since the NestJS and axios slices remain open.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@juyua9 please look into these findings 🙏 |
|
Thanks @magyargergo — I looked into the review findings and pushed Follow-up changes:
Validation:
|
|
Thanks — I pushed Change:
Validation:
|
|
Claude finished @magyargergo's task in 5m 37s —— View job PR #1408 Review — fix(group): detect httpx async consumersReview bar for this PR
Current PR state
Branch hygiene assessmentVerdict: merge-from-main commit present but harmless and merge-safe. The merge commit Understanding of the changeBefore this PR, the Python HTTP plugin scanned for Group matching needs consumer contracts in Why attribute-held clients matter: Production service wrappers typically look like Why async-with aliases matter: One-off request patterns ( Why Why false positives from arbitrary Deliberately out of scope: Findings[MEDIUM] File-global client tracking — no test for same-name reuse across scopes
[LOW] PR body
|
| Scenario | Behavior |
|---|---|
client in function A (httpx) / client.get() in function B (dict) |
False positive if variable text matches exactly |
self._client in class A (httpx) / self._client.get() in class B |
Controlled false positive — self._client text is identical; same-class reuse is rare |
c = self._client; await c.get("/x") |
False negative — alias chain not tracked |
from httpx import AsyncClient; client = AsyncClient() |
False negative — documented in NOTE comment |
import httpx as hx; client = hx.AsyncClient() |
False negative — documented in NOTE comment |
False-positive control proven in tests:
data.get("/nope")→datanot in tracked set → no emission ✓service.request("POST", "/nope")→servicenot in tracked set → no emission ✓
Gap: No test proves that same-name reuse across scopes within the same file is controlled. This is the main open precision question — acceptable for the current DoD bar but worth tracking.
HTTP contract semantics assessment
Emitted HttpDetection objects pass through extractConsumersSourceScan:
role: 'consumer'✓framework: 'python-httpx'✓ (distinct from'python-requests','fastapi'; stored inmetaonly)method: uppercased from capture ✓path: raw literal →normalizeConsumerPathin orchestrator:- Absolute
https://svc.local/questions/duplicate-check→new URL(...).pathname→/questions/duplicate-check→http::POST::/questions/duplicate-check✓ - Relative
/topic→http::GET::/topic✓
- Absolute
name: null— consistent with all consumer plugins ✓confidence: 0.7— consistent with source-scan consumer plugins ✓dedupeContractsusescontractId|filePath|namekey — duplicate calls in the same file are de-duplicated ✓
Group matching compatibility: The orchestrator uses method + normalized path for contract ID generation; framework is meta only. python-httpx consumers will correctly match FastAPI, Spring, NestJS, Express providers when method + path align. No schema changes, no new contract types. ✓
base_url from httpx.AsyncClient(base_url=...) is not merged into consumer paths. Relative paths like "/topic" remain relative. This is consistent with existing extractor policy — base URL merging is explicitly not in scope and is the correct behavior since the group matching normalizes on path segments, not full URLs.
Scope / issue alignment assessment
The PR title, body, and code correctly scope to the Python httpx.AsyncClient slice only. The NOTE comment at lines 83–85 explicitly documents the unsupported forms:
// NOTE: This targeted detector only tracks explicit `httpx.AsyncClient(...)`
// construction. Direct imports (`from httpx import AsyncClient`) and module
// aliases (`import httpx as hx`) are intentionally left for a follow-up.The PR body correctly states "Fixes... for the Python httpx.AsyncClient slice." However, the Fixes #1184 keyword will trigger GitHub's auto-close behavior regardless of the qualifying text. The maintainers should:
- Edit the PR body to use
Partially fixes #1184, or - Reopen [bug][group] HTTP consumer extractor misses httpx.AsyncClient, NestJS HttpService, and project axios wrappers #1184 after merge, or
- Create sub-issues for NestJS HttpService and axios wrapper slices before merging.
No claim of broader httpx support (sync httpx.Client, direct imports, aliased imports) appears in the code or docs. ✓
Test assessment
Positive cases covered:
- ✅ Attribute-held client:
self._client = httpx.AsyncClient()+await self._client.get("/topic") - ✅ Generic
.request():await self._client.request("POST", "/questions/import") - ✅
.delete()verb:await self._client.delete("/topic") - ✅
async withalias:async with httpx.AsyncClient() as client:+await client.post(...) - ✅ Absolute URL normalization:
https://svc.local/questions/duplicate-check→http::POST::/questions/duplicate-check - ✅ Framework metadata assertion: per-contractId (not over-broad global assertion)
Negative cases covered:
- ✅
data.get("/nope")— untracked object, no emission - ✅
service.request("POST", "/nope")— untracked object, no emission
Regression coverage:
- ✅ FastAPI
@app.get("/users")provider extraction tested (lines 649–675) - ✅
requests.post(...)consumer extraction tested (lines 411–430) - ✅ Existing 27 tests (27 + 1 new = 28 total) cover the full range of existing providers/consumers
Gaps (non-blocking):
- ❌ No test for same-name local variable reused in a different function (file-global scope leakage)
- ❌ No test explicitly proving
from httpx import AsyncClient; client = AsyncClient()does NOT emit (absence is implicitly correct; negative test would document the boundary) - ❌
.putand.patchverb coverage absent from the httpx test (only.get,.post,.deletetested); the.put/.patchpath throughHTTPX_ASYNC_CLIENT_VERB_PATTERNSis implicitly validated by the REQUESTS_VERB_PATTERNS tests but not separately exercised for httpx
Temp directory safety:
The change from a shared Date.now() path to per-test fs.mkdtempSync(...) is correct and fixes the CodeQL insecure-tempfile finding. tmpDir is declared as let, assigned in beforeEach, used in afterEach — all scoped correctly. Each test gets a unique private directory; parallel test runs cannot collide. ✓
Would tests have failed before this PR? Yes — all four expect(consumer).toBeDefined() assertions in the new httpx test block would have failed on the unpatched code since no httpx patterns existed.
CI / release-readiness assessment
| Check | Status |
|---|---|
| quality/format | ✅ passed |
| quality/lint | ✅ passed |
| quality/typecheck | ✅ passed |
| quality/typecheck-web | ✅ passed |
| scope-parity (4 languages) | ✅ passed |
| e2e | ✅ passed |
| tests/ubuntu/coverage | ⏳ in progress |
| tests/windows-latest | ⏳ in progress |
| tests/macos-latest | ⏳ in progress |
| CodeQL | ✅ passed |
| Gitleaks | ✅ passed |
| Dependency Review | ✅ passed |
| Docker Build & Push | ⏳ in progress |
Typecheck passes on both gitnexus and gitnexus-shared packages. The three platform test runs are still in progress — do not merge until they complete green.
Hidden Unicode / hygiene assessment
Bidi control characters (U+202A–U+202E, U+2066–U+2069, U+200B–U+200F, U+FEFF): None found in either changed file. ✓
Non-ASCII characters:
python.tslines 27, 46, 64, 82, 104, 123, 141:─(U+2500 BOX DRAWINGS LIGHT HORIZONTAL) in section comment separators — consistent with the pre-existing style used inpython.tsbefore this PR, and present throughout the codebase. Not in query strings, regexes, or executable code. ✓http-route-extractor.test.ts:—(U+2014 EM DASH) indescribe()block label strings — pre-existing style throughout the test file. Not in test fixture Python source code, query strings, or assertions. ✓
No block. The GitHub "hidden Unicode" warning is a false alarm triggered by decorative comment separators.
Elegance / maintainability assessment
The implementation is focused, local, and follows the existing plugin pattern exactly. The four pattern bundles follow the naming convention of existing bundles (FASTAPI_PATTERNS, REQUESTS_VERB_PATTERNS, REQUESTS_GENERIC_PATTERNS). All four are compiled at module load time via compilePatterns. collectHttpxAsyncClients is a clear, 14-line function with no external dependencies. The two consumer scan loops follow the identical structure of the existing REQUESTS_* loops.
Section comments use the same ─── Name ────── style as the pre-existing sections. The NOTE comment accurately states the scoping decision without overpromising.
Future extensions (sync httpx.Client, direct imports, aliased imports, aiohttp) can be added by appending patterns to collectHttpxAsyncClients without modifying the consumer scan loops. The architecture supports clean extension. ✓
No broad extractor refactor, no new types, no schema changes, no unrelated file touches.
Final verdict
production-ready with minor follow-ups
The core implementation is architecturally sound and achieves its stated goal. The two-phase approach (collect tracked clients, then filter call detections) correctly prevents false positives from arbitrary .get() / .request() calls — the httpxAsyncClients.has(clientNode.text) guard works for both attribute-held clients and async with aliases, and the test suite confirms this with both positive detections and negative data.get("/nope") / service.request("POST", "/nope") assertions. The tree-sitter queries are correct for the pinned tree-sitter-python 0.23.4, path normalization reuses the existing normalizeConsumerPath pipeline, and the emitted contracts are fully compatible with group exact matching.
The two open items are: (1) the Fixes #1184 PR body keyword will auto-close an issue that has unresolved slices — the maintainers should handle this explicitly before or immediately after merge; and (2) the file-global client-name tracking has no regression test for same-name reuse across scopes, which is an accepted approximation documented in the NOTE comment but untested. Neither constitutes a correctness defect for the declared scope of this PR.
Do not merge until the tests/ubuntu/coverage, tests/windows-latest, and tests/macos-latest runs complete green. All quality, typecheck, scope-parity, CodeQL, Gitleaks, and dependency checks are already green.
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 8672 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
Thanks — I pushed Changes:
Validation:
|
|
Claude finished @magyargergo's task in 6m 29s —— View job PR #1408 Review — fix(group): detect httpx async consumersTasks:
Review bar for this PR
Current PR state
Branch hygiene assessmentVerdict: merge-from-main commits present but harmless and merge-safe. The branch has three merge-from-main commits ( Understanding of the changeThe PR adds a two-phase approach to Python HTTP consumer extraction:
This correctly handles: attribute-held clients in class methods, Findings[MEDIUM] Module-scope
|
|
@juyua9 Can you please look into claude's findings? 🙏 |
Prevent module-scope httpx.AsyncClient tracking from matching same-name local variables inside functions. Also documents the intentionally unsupported direct-import, alias, and typed-assignment forms, and extends the httpx extractor regression fixture to cover module-scope shadowing while keeping module-scope calls detected.
|
Thanks — I looked into Claude's latest findings and pushed What changed:
Validation run locally:
I did not expand scope to the remaining #1184 slices (NestJS |
3 participants
What problem does this PR solve?
Partially fixes #1184 for the Python
httpx.AsyncClientslice. The NestJSHttpServiceand project-specific axios wrapper slices remain follow-up work.The source-scan HTTP extractor currently detects Python
requests.*consumers, but misses async service clients built withhttpx.AsyncClient. That means group contract extraction can omit real Python consumer call sites such as attribute-held clients (self._client.get(...)) andasync withaliases (client.post(...)).Type of change
Changes
httpx.AsyncClient(...)clients assigned to variables/attributes andasync with ... as clientaliases..get/.post/.put/.delete/.patch(...)calls on those clients..request("METHOD", "url")on those clients.Validation
cd gitnexus-shared && npx tsccd gitnexus && npx tsccd gitnexus && npx vitest run test/unit/group/http-route-extractor.test.ts— 28 passed