-
Notifications
You must be signed in to change notification settings - Fork 9
fix(metadata): prevent framework misclassification in codebase detection #96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import path from 'path'; | ||
| import { promises as fs } from 'fs'; | ||
| import { parse, type TSESTree } from '@typescript-eslint/typescript-estree'; | ||
| import type { | ||
| AnalysisResult, | ||
|
|
@@ -225,33 +226,59 @@ export class ReactAnalyzer implements FrameworkAnalyzer { | |
| category: categorizeDependency(name) | ||
| }) | ||
| ); | ||
| metadata.framework = { | ||
| name: 'React', | ||
| version: normalizeAnalyzerVersion(packageInfo.allDependencies.react), | ||
| type: 'react', | ||
| variant: 'unknown', | ||
| stateManagement: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['@reduxjs/toolkit', 'redux'], | ||
| ['redux', 'redux'], | ||
| ['zustand', 'zustand'], | ||
| ['jotai', 'jotai'], | ||
| ['recoil', 'recoil'], | ||
| ['mobx', 'mobx'] | ||
| ]), | ||
| uiLibraries: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['tailwindcss', 'Tailwind'], | ||
| ['@mui/material', 'MUI'], | ||
| ['styled-components', 'styled-components'], | ||
| ['@radix-ui/react-slot', 'Radix UI'] | ||
| ]), | ||
| testingFrameworks: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['vitest', 'Vitest'], | ||
| ['jest', 'Jest'], | ||
| ['@testing-library/react', 'Testing Library'], | ||
| ['playwright', 'Playwright'], | ||
| ['cypress', 'Cypress'] | ||
| ]) | ||
| }; | ||
|
|
||
| // Collect evidence indicators before claiming framework. | ||
| const indicators: string[] = []; | ||
| if (packageInfo.allDependencies.react) indicators.push('dep:react'); | ||
| if (packageInfo.allDependencies['react-dom']) indicators.push('dep:react-dom'); | ||
| if (packageInfo.allDependencies['@types/react']) indicators.push('dep:@types/react'); | ||
| if (packageInfo.allDependencies['react-native']) indicators.push('dep:react-native'); | ||
|
Comment on lines
+231
to
+235
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The React analyzer collects only four dep-based indicators ( Before this PR the old Possible fixes:
|
||
|
|
||
| // Add disk-based indicators for plain JS React projects (CRA, Vite) | ||
| try { | ||
| await fs.stat(path.join(rootPath, 'src')); | ||
| indicators.push('disk:src-directory'); | ||
| } catch { | ||
| /* absent */ | ||
| } | ||
| try { | ||
| await fs.stat(path.join(rootPath, 'public', 'index.html')); | ||
| indicators.push('disk:public-index-html'); | ||
| } catch { | ||
| /* absent */ | ||
| } | ||
|
|
||
| // Only claim React when the react package is an actual dependency. | ||
| if (indicators.includes('dep:react')) { | ||
| metadata.framework = { | ||
| name: 'React', | ||
| version: normalizeAnalyzerVersion(packageInfo.allDependencies.react), | ||
| type: 'react', | ||
| variant: 'unknown', | ||
| stateManagement: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['@reduxjs/toolkit', 'redux'], | ||
| ['redux', 'redux'], | ||
| ['zustand', 'zustand'], | ||
| ['jotai', 'jotai'], | ||
| ['recoil', 'recoil'], | ||
| ['mobx', 'mobx'] | ||
| ]), | ||
| uiLibraries: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['tailwindcss', 'Tailwind'], | ||
| ['@mui/material', 'MUI'], | ||
| ['styled-components', 'styled-components'], | ||
| ['@radix-ui/react-slot', 'Radix UI'] | ||
| ]), | ||
| testingFrameworks: detectDependencyList(packageInfo.allDependencies, [ | ||
| ['vitest', 'Vitest'], | ||
| ['jest', 'Jest'], | ||
| ['@testing-library/react', 'Testing Library'], | ||
| ['playwright', 'Playwright'], | ||
| ['cypress', 'Cypress'] | ||
| ]), | ||
| indicators | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| if (!isFileNotFoundError(error)) { | ||
| console.warn('Failed to read React project metadata:', error); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,7 @@ import ignore from 'ignore'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CodebaseMetadata, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CodeChunk, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| FrameworkInfo, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IndexingProgress, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IndexingStats, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| IndexingPhase, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1243,7 +1244,7 @@ export class CodebaseIndexer { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| rootPath: incoming.rootPath || base.rootPath, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| languages: [...new Set([...base.languages, ...incoming.languages])], // Merge and deduplicate | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| dependencies: this.mergeDependencies(base.dependencies, incoming.dependencies), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| framework: base.framework || incoming.framework, // Framework from higher priority analyzer wins | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| framework: this.selectFramework(base.framework, incoming.framework), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| architecture: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type: incoming.architecture?.type || base.architecture.type, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| layers: this.mergeLayers(base.architecture.layers, incoming.architecture?.layers), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1265,6 +1266,44 @@ export class CodebaseIndexer { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Select the best framework claim from two candidates. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Requires at least MIN_INDICATORS evidence signals — prevents analyzers from claiming | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * a framework when only one weak signal is present (e.g. a single import). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Priority-order callers (highest priority first) ensure the first passing candidate wins. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private selectFramework( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| base: FrameworkInfo | undefined, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| incoming: FrameworkInfo | undefined | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): FrameworkInfo | undefined { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const MIN_INDICATORS = 3; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const passes = (f: FrameworkInfo | undefined): f is FrameworkInfo => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !!f && (f.indicators?.length ?? 0) >= MIN_INDICATORS; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (passes(base) && passes(incoming)) return base; // higher-priority analyzer wins | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (passes(base)) return base; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (passes(incoming)) return incoming; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Debug: log dropped framework claims to aid production diagnosis | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const claimed = []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (base) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claimed.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `${(base as FrameworkInfo).type}(${(base as FrameworkInfo).indicators?.length ?? 0})` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (incoming) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claimed.push( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| `${(incoming as FrameworkInfo).type}(${(incoming as FrameworkInfo).indicators?.length ?? 0})` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (claimed.length > 0) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.debug( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| '[selectFramework] dropped %d claim(s) below MIN_INDICATORS=%d: %s', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claimed.length, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MIN_INDICATORS, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| claimed.join(', ') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1278
to
+1305
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When A single
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private mergeDependencies(base: Dependency[], incoming: Dependency[]): Dependency[] { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const seen = new Set(base.map((d) => d.name)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const result = [...base]; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -143,4 +143,92 @@ describe('CodebaseIndexer.detectMetadata', () => { | |
| expect(typeof metadata.customMetadata).toBe('object'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('framework misclassification guards', () => { | ||
| it('does not claim framework for plain Node project', async () => { | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'package.json'), | ||
| JSON.stringify({ name: 'plain-node', dependencies: { zod: '^3' } }) | ||
| ); | ||
|
|
||
| const indexer = new CodebaseIndexer({ rootPath: tempDir }); | ||
| const metadata = await indexer.detectMetadata(); | ||
|
|
||
| expect(metadata.framework).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('drops React claim when indicators are below threshold', async () => { | ||
| // react alone is only 1 indicator — should not meet the >=3 threshold | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'package.json'), | ||
| JSON.stringify({ name: 'thin-react', dependencies: { react: '^18' } }) | ||
| ); | ||
|
|
||
| const indexer = new CodebaseIndexer({ rootPath: tempDir }); | ||
|
Comment on lines
+155
to
+167
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The comment on this test says "react alone is only 1 indicator — should not meet the >=3 threshold." The symmetric case — it('drops React claim for react + react-dom without @types/react (2 indicators)', async () => {
await fs.writeFile(
path.join(tempDir, 'package.json'),
JSON.stringify({ name: 'js-react', dependencies: { react: '^18', 'react-dom': '^18' } })
);
const indexer = new CodebaseIndexer({ rootPath: tempDir });
const metadata = await indexer.detectMetadata();
// Decide: should this be undefined (current) or 'react' (intended)?
expect(metadata.framework?.type).toBe('react'); // currently fails
}); |
||
| const metadata = await indexer.detectMetadata(); | ||
|
|
||
| expect(metadata.framework).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('preserves Next.js preference over React when both pass threshold', async () => { | ||
| // next + react + react-dom + app/ = 4 Next.js indicators; react + react-dom = 2 React indicators | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'package.json'), | ||
| JSON.stringify({ | ||
| name: 'next-project', | ||
| dependencies: { next: '^14.1.0', react: '^18.2.0', 'react-dom': '^18.2.0' }, | ||
| }) | ||
| ); | ||
| await fs.mkdir(path.join(tempDir, 'app'), { recursive: true }); | ||
|
|
||
| const indexer = new CodebaseIndexer({ rootPath: tempDir }); | ||
| const metadata = await indexer.detectMetadata(); | ||
|
|
||
| expect(metadata.framework?.type).toBe('nextjs'); | ||
| }); | ||
|
|
||
| it('detects React when sufficient indicators are present', async () => { | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'package.json'), | ||
| JSON.stringify({ | ||
| name: 'react-app', | ||
| dependencies: { react: '^18', 'react-dom': '^18' }, | ||
| devDependencies: { '@types/react': '^18' }, | ||
| }) | ||
| ); | ||
|
|
||
| const indexer = new CodebaseIndexer({ rootPath: tempDir }); | ||
| const metadata = await indexer.detectMetadata(); | ||
|
|
||
| expect(metadata.framework?.type).toBe('react'); | ||
| expect(metadata.framework?.indicators).toContain('dep:react'); | ||
| }); | ||
|
|
||
| it('detects Angular library project with @angular/core in peerDependencies + ng-package.json', async () => { | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'package.json'), | ||
| JSON.stringify({ | ||
| name: 'my-angular-lib', | ||
| peerDependencies: { | ||
| '@angular/core': '^17.0.0', | ||
| '@angular/common': '^17.0.0', | ||
| }, | ||
| devDependencies: { | ||
| '@angular/compiler-cli': '^17.0.0', | ||
| } | ||
| }) | ||
| ); | ||
| await fs.writeFile( | ||
| path.join(tempDir, 'ng-package.json'), | ||
| JSON.stringify({ lib: { entryFile: 'src/public-api.ts' } }) | ||
| ); | ||
|
|
||
| const indexer = new CodebaseIndexer({ rootPath: tempDir }); | ||
| const metadata = await indexer.detectMetadata(); | ||
|
|
||
| expect(metadata.framework?.type).toBe('angular'); | ||
| expect(metadata.framework?.indicators).toContain('dep:@angular/core'); | ||
| expect(metadata.framework?.indicators).toContain('disk:ng-package-json'); | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dep:@angular/coreis the only guaranteed indicator in this block — the rest are conditional. An Angular library project that lists@angular/coreinpeerDependencies(no@angular/common, and no CLI-generatedangular.jsonortsconfig.app.json) ends up with just 1 indicator, well belowMIN_INDICATORS = 3. Unlike the oldbase.framework || incoming.frameworkpath,selectFrameworkwill silently returnundefined.Before this PR,
angularCoreVersionalone was sufficient to classify a project as Angular (which is a much tighter guard than justdep:react). The same threshold now uniformly penalises Angular, which had a more reliable single-dep signal to begin with.Suggested mitigations:
ng-package.json(Angular library projects) andtsconfig.jsonso library projects get extra indicators.