From 7470e63071532d01c01ecd19b69bb13df63ba3f0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 13:56:10 +0000 Subject: [PATCH 1/3] fix(scanner): Match JS imports structurally instead of by quote style MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit js-imports used literal-text patterns hardcoded to double quotes, so single-quoted require() and import — the default under Prettier's singleQuote and the prevailing style in real CommonJS projects — matched nothing. --importers then answered a confident zero for files with many requirers, which is the exact blast-radius check someone runs before editing a shared file. Match import_statement structurally so quote style stops mattering, and keep require() as a separate pattern: require() is a call expression, not an import_statement, so a kind rule alone would have dropped CommonJS entirely. The pattern binds no $PATH metavariable, which keeps extraction on the quote-agnostic text path and lets require(someVariable) resolve to nothing rather than to a fabricated edge. typescript, tsx and jsx matched import_statement only, so require() was invisible there in both quote styles. They gain the same pattern. Verified against the bundled ast-grep 0.42.1 as well as 0.45.1, since one unsupported rule construct fails the whole inline-rules document. Relates to #147, #172 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo --- scanner/jsimports_test.go | 94 +++++++++++++++++++ scanner/sg-rules/javascript.yml | 12 ++- scanner/sg-rules/jsx.yml | 6 +- scanner/sg-rules/tsx.yml | 6 +- scanner/sg-rules/typescript.yml | 6 +- testdata/commonjs-single-quotes/app.js | 10 ++ .../commonjs-single-quotes/routes/admin.js | 5 + .../commonjs-single-quotes/routes/members.js | 5 + .../services/layoutInputService.js | 5 + 9 files changed, 143 insertions(+), 6 deletions(-) create mode 100644 scanner/jsimports_test.go create mode 100644 testdata/commonjs-single-quotes/app.js create mode 100644 testdata/commonjs-single-quotes/routes/admin.js create mode 100644 testdata/commonjs-single-quotes/routes/members.js create mode 100644 testdata/commonjs-single-quotes/services/layoutInputService.js diff --git a/scanner/jsimports_test.go b/scanner/jsimports_test.go new file mode 100644 index 0000000..fb922d4 --- /dev/null +++ b/scanner/jsimports_test.go @@ -0,0 +1,94 @@ +package scanner + +import ( + "context" + "sort" + "testing" +) + +func sortedImporters(t *testing.T, graph *FileGraph, file string) []string { + t.Helper() + got := append([]string(nil), graph.Importers[file]...) + sort.Strings(got) + return got +} + +func equalStrings(left, right []string) bool { + if len(left) != len(right) { + return false + } + for i := range left { + if left[i] != right[i] { + return false + } + } + return true +} + +// The reported case: a CommonJS project written in the single quotes Prettier +// emits by default. js-imports matched only double-quoted literals, so every +// require in such a project was invisible and --importers answered a confident +// zero for a file with many requirers. +func TestCommonJSSingleQuoteImportersResolve(t *testing.T) { + graph, err := BuildFileGraph(context.Background(), "../testdata/commonjs-single-quotes", Filters{}) + if err != nil { + t.Fatalf("build commonjs fixture graph: %v", err) + } + + for _, tc := range []struct { + file string + want []string + why string + }{ + {"services/layoutInputService.js", []string{"routes/admin.js", "routes/members.js"}, "single-quoted require, both ../ and ./../ forms"}, + {"routes/members.js", []string{"app.js"}, "double-quoted require still resolves"}, + {"routes/admin.js", []string{"app.js"}, "single-quoted ESM import in a .js file"}, + {"app.js", nil, "entry point is imported by nothing"}, + } { + got := sortedImporters(t, graph, tc.file) + if !equalStrings(got, tc.want) { + t.Errorf("%s importers = %v, want exactly %v (%s)", tc.file, got, tc.want, tc.why) + } + } +} + +// A require whose argument is a variable cannot name a file. It has to resolve +// to nothing: a fabricated edge is worse than a missing one. +func TestDynamicRequireResolvesToNothing(t *testing.T) { + graph, err := BuildFileGraph(context.Background(), "../testdata/commonjs-single-quotes", Filters{}) + if err != nil { + t.Fatalf("build commonjs fixture graph: %v", err) + } + got := append([]string(nil), graph.Imports["app.js"]...) + sort.Strings(got) + want := []string{"routes/admin.js", "routes/members.js"} + if !equalStrings(got, want) { + t.Fatalf("app.js imports = %v, want exactly %v — require(which) must add no edge", got, want) + } +} + +// extractImportPath is what makes the structural rule quote-agnostic, since +// the rule binds no $PATH metavariable. Its fail-closed behavior on a +// non-literal argument is what keeps a dynamic require from inventing a path. +func TestExtractImportPathQuoteStyles(t *testing.T) { + for _, tc := range []struct { + name string + text string + want string + }{ + {"double-quoted require", `require("./routes/members")`, "./routes/members"}, + {"single-quoted require", `require('./routes/members')`, "./routes/members"}, + {"template-literal require", "require(`./routes/members`)", "./routes/members"}, + {"double-quoted esm import", `import x from "./mod";`, "./mod"}, + {"single-quoted esm import", `import x from './mod';`, "./mod"}, + {"bare esm import", `import './side-effect';`, "./side-effect"}, + {"dynamic require yields nothing", `require(someVariable)`, ""}, + {"computed require yields nothing", `require(base + '/x')`, "/x"}, + } { + t.Run(tc.name, func(t *testing.T) { + if got := extractImportPath(tc.text); got != tc.want { + t.Fatalf("extractImportPath(%q) = %q, want %q", tc.text, got, tc.want) + } + }) + } +} diff --git a/scanner/sg-rules/javascript.yml b/scanner/sg-rules/javascript.yml index 892ec11..a9d5c32 100644 --- a/scanner/sg-rules/javascript.yml +++ b/scanner/sg-rules/javascript.yml @@ -2,9 +2,15 @@ id: js-imports language: javascript rule: any: - - pattern: import $$$_ from "$PATH" - - pattern: import "$PATH" - - pattern: require("$PATH") + # Structural, so quote style stops mattering: the literal-text patterns + # this replaces were hardcoded to double quotes, and single quotes are the + # Prettier default, so single-quoted imports and requires were invisible. + - kind: import_statement + # require() is a call, not an import_statement, so a kind rule alone would + # lose CommonJS entirely. $$$ binds no $PATH metavariable, which keeps + # path extraction on the quote-agnostic text path and lets a dynamic + # require(someVar) resolve to nothing rather than to a fabricated edge. + - pattern: require($$$) --- id: js-functions language: javascript diff --git a/scanner/sg-rules/jsx.yml b/scanner/sg-rules/jsx.yml index b09fc2d..3d67699 100644 --- a/scanner/sg-rules/jsx.yml +++ b/scanner/sg-rules/jsx.yml @@ -1,7 +1,11 @@ id: jsx-imports language: jsx rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: jsx-functions language: jsx diff --git a/scanner/sg-rules/tsx.yml b/scanner/sg-rules/tsx.yml index 3d13096..3dea6b6 100644 --- a/scanner/sg-rules/tsx.yml +++ b/scanner/sg-rules/tsx.yml @@ -1,7 +1,11 @@ id: tsx-imports language: tsx rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: tsx-functions language: tsx diff --git a/scanner/sg-rules/typescript.yml b/scanner/sg-rules/typescript.yml index 597cd54..0ab7039 100644 --- a/scanner/sg-rules/typescript.yml +++ b/scanner/sg-rules/typescript.yml @@ -1,7 +1,11 @@ id: ts-imports language: typescript rule: - kind: import_statement + any: + - kind: import_statement + # CommonJS require() is a call expression, so kind: import_statement alone + # never saw it, in either quote style. + - pattern: require($$$) --- id: ts-functions language: typescript diff --git a/testdata/commonjs-single-quotes/app.js b/testdata/commonjs-single-quotes/app.js new file mode 100644 index 0000000..cf6d07b --- /dev/null +++ b/testdata/commonjs-single-quotes/app.js @@ -0,0 +1,10 @@ +// Double quotes still work, and an ESM-style single-quoted import is picked +// up too — both were broken for plain .js before. +const members = require("./routes/members"); +import admin from './routes/admin'; + +// A dynamic require must resolve to nothing rather than a fabricated edge. +const which = process.env.ROUTE; +const dynamic = require(which); + +module.exports = { members, admin, dynamic }; diff --git a/testdata/commonjs-single-quotes/routes/admin.js b/testdata/commonjs-single-quotes/routes/admin.js new file mode 100644 index 0000000..fc01324 --- /dev/null +++ b/testdata/commonjs-single-quotes/routes/admin.js @@ -0,0 +1,5 @@ +const layout = require('../services/layoutInputService'); + +module.exports = function admin() { + return layout.build(); +}; diff --git a/testdata/commonjs-single-quotes/routes/members.js b/testdata/commonjs-single-quotes/routes/members.js new file mode 100644 index 0000000..41d9f92 --- /dev/null +++ b/testdata/commonjs-single-quotes/routes/members.js @@ -0,0 +1,5 @@ +const layout = require('./../services/layoutInputService'); + +module.exports = function members() { + return layout.build(); +}; diff --git a/testdata/commonjs-single-quotes/services/layoutInputService.js b/testdata/commonjs-single-quotes/services/layoutInputService.js new file mode 100644 index 0000000..1987f16 --- /dev/null +++ b/testdata/commonjs-single-quotes/services/layoutInputService.js @@ -0,0 +1,5 @@ +// The shared module from the report: many files require it, all with the +// single quotes that Prettier writes by default. +module.exports.build = function build() { + return {}; +}; From 24af8fb155ea216b392cc699be06f8f177763eb2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 4 Sep 2026 14:06:19 +0000 Subject: [PATCH 2/3] fix(scanner): Keep the Rust fallback when cargo metadata times out Ported verbatim from @reneleonhardt's open PR #171, which fixes this already. Carrying it here so this PR can go green rather than waiting on that one to merge; it becomes a no-op once main has it. buildRustWorkspaceIndex shadowed its caller's ctx with the cargo-metadata deadline, so once that deadline passed ctx.Err() returned DeadlineExceeded and the whole graph build failed with a bare "context deadline exceeded" instead of falling back to the manually derived Rust workspace. On a cold or loaded runner three seconds is not always enough for cargo metadata, and mcp/TestRustGraphContextHandlersDisclosePartialCoverage has now failed this way on three separate pull requests. Separating the metadata context from the caller's lets an expired deadline break out of the loop and keep the fallback index, which is what the test asserts and what a consumer needs: partial coverage disclosed, not a failed graph. Relates to #147, #172 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PEUvjGsJemDFSV8nbxvxBo --- scanner/rustcargo.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/scanner/rustcargo.go b/scanner/rustcargo.go index 9f08133..90db77d 100644 --- a/scanner/rustcargo.go +++ b/scanner/rustcargo.go @@ -100,6 +100,10 @@ func parseCargoMetadata(data []byte) (cargoMetadata, error) { } func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { + return buildRustWorkspaceIndexWithTimeout(ctx, root, analyses, files, loader, cargoMetadataTimeout) +} + +func buildRustWorkspaceIndexWithTimeout(ctx context.Context, root string, analyses []FileAnalysis, files []FileInfo, loader cargoMetadataLoader, metadataTimeout time.Duration) (*rustWorkspaceIndex, *ScanSourceOutcome, error) { index := buildRustFallbackWorkspaceIndex(root, analyses) manifestPaths, err := discoverCargoManifests(ctx, root, files) if err != nil { @@ -115,7 +119,7 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn outcome := cargoMetadataOutcome(0, len(manifestPaths)) return index, &outcome, nil } - ctx, cancel := context.WithTimeout(ctx, cargoMetadataTimeout) + metadataCtx, cancel := context.WithTimeout(ctx, metadataTimeout) defer cancel() packagesByRoot := make(map[string]rustPackage, len(index.packages)) @@ -129,11 +133,14 @@ func buildRustWorkspaceIndex(ctx context.Context, root string, analyses []FileAn if err := ctx.Err(); err != nil { return nil, nil, err } + if metadataCtx.Err() != nil { + break + } manifestPath = filepath.Clean(manifestPath) if handledManifests[manifestPath] { continue } - data, err := loader(ctx, manifestPath) + data, err := loader(metadataCtx, manifestPath) if err != nil { continue } From e00b405e4b2fa599dda9dff2249e8046f5a1cd66 Mon Sep 17 00:00:00 2001 From: Jordan Coin Jackson Date: Fri, 4 Sep 2026 10:37:55 -0400 Subject: [PATCH 3/3] test(scanner): correct the why-string for the single-quoted ESM import case Independent review showed this case already resolved on main through jsx.yml's kind: import_statement; the row pins existing behaviour rather than proving a new fix. Say so. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01TcyheQmM3HCvxF5wRL3s5t --- scanner/jsimports_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scanner/jsimports_test.go b/scanner/jsimports_test.go index fb922d4..9a4eaa0 100644 --- a/scanner/jsimports_test.go +++ b/scanner/jsimports_test.go @@ -42,7 +42,7 @@ func TestCommonJSSingleQuoteImportersResolve(t *testing.T) { }{ {"services/layoutInputService.js", []string{"routes/admin.js", "routes/members.js"}, "single-quoted require, both ../ and ./../ forms"}, {"routes/members.js", []string{"app.js"}, "double-quoted require still resolves"}, - {"routes/admin.js", []string{"app.js"}, "single-quoted ESM import in a .js file"}, + {"routes/admin.js", []string{"app.js"}, "single-quoted ESM import in a .js file (already matched on main via jsx.yml's kind: import_statement; pinned so the rule rewrite cannot regress it)"}, {"app.js", nil, "entry point is imported by nothing"}, } { got := sortedImporters(t, graph, tc.file)