fix: shape_check false positives — quoted keys, DOM leaks, errorKeys#501
Conversation
…orKeys display
Three bugs caused shape_check to report false MISMATCH results:
1. DB array elements returned with wrapping single quotes ('key' vs key),
causing Set comparison to fail. Fixed by stripping quotes in
fetchRoutesWithConsumers and normalizing in shapeCheck/apiImpact.
2. DOM methods (appendChild, removeChild, etc.) leaked into consumer
accessedKeys via the broad property-access regex. Fixed by expanding
RESPONSE_METHOD_BLOCKLIST with common DOM manipulation methods.
3. Consumer accessing error-path keys (e.g., .error from errorKeys)
was not distinguished from true mismatches. Added errorPathKeys
field to consumer output for visibility.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@marxo126 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 4220 tests passed 1 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
marxo126
force-pushed
the
fix/shape-check-false-positives
branch
from
9fb37a6 to
5ba02c3
Compare
Verification ResultsTested against a Next.js project with 30 API routes and 11 routes with response shapes + consumers: Before fix: 6 false MISMATCH reports
After fix: 0 false mismatches
Unit tests: 9 new tests passing, 3882 total tests with 0 regressions. |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce false positives in the route “shape_check” flow by normalizing response key formats, preventing DOM-related property accesses from being misattributed as API response keys, and improving mismatch reporting for error-path accesses.
Changes:
- Added unit tests covering response-shape extraction and consumer accessed-key extraction (including DOM-related code paths).
- Normalized/stripped wrapping quotes from
responseKeys/errorKeys/middlewarewhen reading from the local DB and when computing shape-check results. - Expanded the consumer key extractor’s blocklist to avoid leaking DOM methods into
accessedKeys.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
gitnexus/test/unit/shape-check.test.ts |
Adds unit coverage for response shape extraction and accessed-key extraction scenarios. |
gitnexus/src/mcp/local/local-backend.ts |
Strips wrapping quotes from key arrays returned by the DB; adds errorPathKeys to consumer output. |
gitnexus/src/core/ingestion/call-processor.ts |
Expands the response-method blocklist to filter DOM method/property leaks from accessed-key extraction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('extracts keys from .json() with quoted property names', () => { | ||
| const content = `return NextResponse.json({ 'courses': coursesData, 'articles': articlesData });`; | ||
| const result = extractResponseShapes(content); | ||
| // Keys should not contain quotes regardless of source format | ||
| expect(result.responseKeys).toBeDefined(); | ||
| if (result.responseKeys) { | ||
| for (const key of result.responseKeys) { | ||
| expect(key).not.toMatch(/['"]/); | ||
| } | ||
| } |
There was a problem hiding this comment.
extractResponseShapes currently only detects unquoted identifier keys (it starts a key when it sees /[a-zA-Z_$]/ and skips over string literals), so this test using quoted property names ({ 'courses': ... }) will return responseKeys as undefined and fail. Either update extractResponseShapes to also extract string-literal property names, or adjust the test to match the current extractor behavior.
| it('extracts keys from .json() with quoted property names', () => { | |
| const content = `return NextResponse.json({ 'courses': coursesData, 'articles': articlesData });`; | |
| const result = extractResponseShapes(content); | |
| // Keys should not contain quotes regardless of source format | |
| expect(result.responseKeys).toBeDefined(); | |
| if (result.responseKeys) { | |
| for (const key of result.responseKeys) { | |
| expect(key).not.toMatch(/['"]/); | |
| } | |
| } | |
| it('does not extract keys from .json() with quoted property names (current limitation)', () => { | |
| const content = `return NextResponse.json({ 'courses': coursesData, 'articles': articlesData });`; | |
| const result = extractResponseShapes(content); | |
| // Current extractor skips string-literal property names, so no responseKeys are returned. | |
| expect(result.responseKeys).toBeUndefined(); |
| console.log(data.items); | ||
| console.log(data.total); |
There was a problem hiding this comment.
This test declares const result = await fetch(...).then(r => r.json()); but then accesses data.items/data.total. That mismatch makes the fixture confusing and less representative of real-world usage (it would be clearer to access result.items/result.total or rename the variable to data).
| console.log(data.items); | |
| console.log(data.total); | |
| console.log(result.items); | |
| console.log(result.total); |
| 'classList', 'className', 'style', 'dataset', | ||
| 'parentNode', 'parentElement', 'childNodes', 'children', | ||
| 'nextSibling', 'previousSibling', 'firstChild', 'lastChild', | ||
| 'click', 'focus', 'blur', 'submit', 'reset', | ||
| 'href', 'download', 'target', 'rel', 'type', |
There was a problem hiding this comment.
The DOM-leak mitigation is implemented by expanding RESPONSE_METHOD_BLOCKLIST, but several newly added entries (e.g. href, download, target, rel, type, style, dataset) are not methods and can be legitimate JSON keys. This will cause extractConsumerAccessedKeys to silently drop those keys when accessed as data.href/data.type, reducing mismatch detection accuracy. Consider fixing the root cause by tightening propAccessPattern (e.g., avoid matching receivers that are part of a larger member chain like document.body.*), and keep the blocklist focused on actual methods (or rename it if it’s intended to block properties too).
| 'classList', 'className', 'style', 'dataset', | |
| 'parentNode', 'parentElement', 'childNodes', 'children', | |
| 'nextSibling', 'previousSibling', 'firstChild', 'lastChild', | |
| 'click', 'focus', 'blur', 'submit', 'reset', | |
| 'href', 'download', 'target', 'rel', 'type', | |
| 'classList', 'className', | |
| 'parentNode', 'parentElement', 'childNodes', 'children', | |
| 'nextSibling', 'previousSibling', 'firstChild', 'lastChild', | |
| 'click', 'focus', 'blur', 'submit', 'reset', |
| // Strip wrapping quotes from DB array elements — CSV COPY stores ['key'] which | ||
| // LadybugDB may return as "'key'" rather than "key" | ||
| const stripQuotes = (keys: string[] | null): string[] | null => | ||
| keys ? keys.map(k => k.replace(/^['"]|['"]$/g, '')) : null; | ||
| const responseKeys = stripQuotes(row.responseKeys ?? row[3] ?? null); | ||
| const errorKeys = stripQuotes(row.errorKeys ?? row[4] ?? null); | ||
| const middleware = stripQuotes(row.middleware ?? row[5] ?? null); |
There was a problem hiding this comment.
stripQuotes is defined inside the for (const row of rows) loop, creating a new function per row. Since it’s pure and reused 3 times, consider hoisting it outside the loop (or making it a small private helper) to reduce repetition and improve readability.
|
@claude can you review this PR for completeness and try to find and identify performance issues and gaps in the implementation. |
|
Claude finished @magyargergo's task in 3m 0s —— View job PR Review: fix/shape-check-false-positives
The PR correctly identifies three real issues and ships targeted fixes. CI is green and the approach is sound, but there are several gaps worth addressing before merge. Performance Issues1. for (const row of rows) {
const stripQuotes = (keys: string[] | null): string[] | null => // ← new closure every row
keys ? keys.map(k => k.replace(/^['"]|['"]$/g, '')) : null;
...
}A new function object is allocated for every row returned by the query. For a repo with hundreds of routes this creates unnecessary GC pressure. Hoist it above the loop (or make it a module-level helper). Copilot also flagged this. 2. Double quote-stripping across three methods
// shapeCheck — redundant, fetchRoutesWithConsumers already stripped these
const strip = (k: string) => k.replace(/^['"]|['"]$/g, '');
const responseKeys = (r.responseKeys ?? []).map(strip); // already clean
const errorKeys = (r.errorKeys ?? []).map(strip); // already cleanThe same pattern appears in Implementation Gaps3. The fix normalizes what's read back from the DB, but the extractor itself has a blind spot. When a handler contains: return NextResponse.json({ 'courses': data });…the character parser at line 79 sets 4. Blocklist over-blocks legitimate API response fields ( The newly added entries include The root issue is that 5. Test fixture uses undeclared variable ( const result = await fetch('/api/test').then(r => r.json());
console.log(data.items); // ← `data` is not declared in this scope
console.log(data.total);The test passes because 6. No test for The PR adds Minor / Cosmetic
Summary
The most impactful items to fix before merge are #3 (the extractor gap means the original ingestion problem is only half-solved) and #4 (the overly broad blocklist introduces new false negatives). |
|
@marxo126 Can you please have a look at these findings? |
…ocklist - Hoist stripQuotes above the loop to avoid per-row closure allocation - Remove redundant strip calls in shapeCheck/apiImpact (fetchRoutesWithConsumers already normalizes) - Extract single/double-quoted property keys in response-shapes.ts parser - Remove over-broad blocklist entries (href, target, rel, type, style, dataset) that would cause false negatives for legitimate API fields - Fix test fixture (undeclared variable) and add tests for quoted key extraction and legitimate API field names Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…location - Rename RESPONSE_METHOD_BLOCKLIST → RESPONSE_ACCESS_BLOCKLIST (blocks properties too, not just methods) and add category comments - Remove redundant errorKeySet — derive error-path keys from allKnownKeys + responseKeySet instead - Replace content.slice().trimStart() with char-scan loop to avoid allocating a substring just to check one character - Add backtick-quoted key test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addressed review findingsPushed two commits addressing the review: 47bef6a — fix: address review
a8f3632 — refactor: simplify
12 tests passing, 3877 total, 0 regressions. |
|
Can you please add integration tests as well? 🙏 |
Runs the full ingestion pipeline on a fixture with: - Quoted property keys in .json() calls (single-quoted) - Error vs success response separation by HTTP status - Consumer files with DOM manipulation after fetch - Consumer accessing error-path keys (.error from errorKeys) Verifies: - Route nodes created with correct responseKeys (no wrapping quotes) - FETCHES edges contain accessedKeys in reason field - DOM methods filtered from consumer accessedKeys - Error-path keys captured alongside success keys Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Added integration tests (85014da):
Total: 18 tests covering this PR, 3883 suite-wide passes. |
|
@claude Let's run a final review if this PR is production ready and ready to be merged. |
|
Claude finished @magyargergo's task in 2m 11s —— View job Final Production-Readiness Review
Verdict: ✅ Ready to merge CI is green (4143 passed / 0 failed), all 6 previously identified issues have been resolved, and both unit and integration test coverage is solid. Previous Review Issues — Status
Remaining Minor Gaps (non-blocking)1. Missing regression test for The 2.
Code Quality AssessmentThe implementation is clean and well-structured:
The two remaining gaps are minor enough that they should not block this merge. They can be tracked as follow-up improvements. |
|
Can you please add those missing regressions into integration tests ? 🙏 |
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Nice fix overall. I found one test issue that looks like it is asserting a syntax the runtime parser will never actually see.
This added case is probably invalid JS/TS object syntax:
return NextResponse.json({ `users`: data, `total`: count });Backticks are not valid quoted property keys in object literals. They only work for template literals / computed property expressions like { [users]: data }, which this parser does not appear to support either.
So the test can pass while covering a non-parsable source shape, and it may give a false sense that backtick-quoted keys are supported in real handler code.
I would either:
- drop that test entirely, or
- replace it with a computed-property case if you explicitly want to support that syntax, and then teach the extractor to parse it.
Everything else in the branch looked directionally good.
|
Thanks for the thorough reviews. Here's what I'll add for the missing regression integration tests: 1.
|
|
@claude Let's run a final review if this PR is production ready and ready to be merged. |
|
Claude finished @magyargergo's task in 1m 57s —— View job Final Production-Readiness Review
Verdict: The core logic is correct and CI is green (4149 tests, 0 failures). All previous performance and correctness issues from prior reviews are properly addressed. However, two open items remain unresolved: 1. Backtick-quoted key test covers invalid JavaScript syntaxThe test at it('extracts backtick-quoted property keys from .json() call', () => {
const content = 'return NextResponse.json({ `users`: data, `total`: count });';
2. Promised regression tests for
|
| # | Issue | Status |
|---|---|---|
| 1 | stripQuotes closure inside loop |
✅ Hoisted above loop (local-backend.ts:1658) |
| 2 | Double-stripping in shapeCheck/apiImpact |
✅ Removed; comments confirm normalization is in fetchRoutesWithConsumers |
| 3 | Extractor couldn't handle quoted source keys | ✅ Parser handles ', ", ` at response-shapes.ts:79-101 |
| 4 | Blocklist over-blocked type, href, target, style |
✅ Removed; unit test at line 119-132 verifies they're captured |
| 5 | Undeclared data variable in test fixture |
✅ Fixed; test declares const data = await fetch(...) |
| 6 | errorPathKeys ∉ mismatched logic |
✅ Logic at local-backend.ts:1775 is correct — allKnownKeys covers both success and error keys |
The two items above are small — the backtick test is a one-liner removal, and the errorPathKeys integration test should be straightforward given the existing fixture. Addressing these would make the PR clean for merge.
Addresses review request: errorPathKeys must not appear in mismatched, and blocklisted DOM-like fields must not suppress legitimate API fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Added regression integration testsPushed 1.
|
|
@claude Let's run a final review if this PR is production ready and ready to be merged |
|
Claude finished @magyargergo's task in 1m 55s —— View job Final Production-Readiness Review
Verdict: ✅ Ready to merge — with one minor note (non-blocking) CI is green (4149 passed / 0 failed). All correctness and performance issues from prior reviews have been resolved. The two regression test scenarios promised by @marxo126 are now present and well-structured. Previous Open Items — Final Status
Regression Tests — Quality Check ✅The new
Both scenarios follow the established Remaining Minor Item (non-blocking)Backtick test ( it('extracts backtick-quoted property keys from .json() call', () => {
const content = 'return NextResponse.json({ `users`: data, `total`: count });';
This is a test quality issue only — there is no runtime impact. Since it's been explicitly flagged twice with no resolution, it's worth a one-line fix (remove the test case or document the limitation), but it should not hold up this merge. Everything Else: Confirmed ✅
|
|
Can you please make sure tests are passing? |
The shape-check-regression test uses withTestLbugDB but was running in the default vitest project with parallel forks, causing LadybugDB file-lock conflicts on Windows CI. Move it to the lbug-db project (sequential execution) and exclude from default. Follows up on #501.
The shape-check-regression test uses withTestLbugDB but was running in the default vitest project with parallel forks, causing LadybugDB file-lock conflicts on Windows CI. Move it to the lbug-db project (sequential execution) and exclude from default. Follows up on #501.
…npatwari#518) The shape-check-regression test uses withTestLbugDB but was running in the default vitest project with parallel forks, causing LadybugDB file-lock conflicts on Windows CI. Move it to the lbug-db project (sequential execution) and exclude from default. Follows up on abhigyanpatwari#501.
Summary
'courses'vscourses), causing false MISMATCHes. Fixed by stripping quotes infetchRoutesWithConsumersand normalizing inshapeCheck/apiImpact.appendChild,removeChild, etc. leaked into consumeraccessedKeysvia the broad property-access regex. Fixed by expandingRESPONSE_METHOD_BLOCKLIST..error) now getserrorPathKeysfield instead of being reported as unknown mismatch.Test plan
extractResponseShapesandextractConsumerAccessedKeysshape_check🤖 Generated with Claude Code