feat: Static model scanning UI #4368
Conversation
TestGru AssignmentSummary
Files
Tip You can |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| : path.resolve(process.cwd(), expandedPath); | ||
|
|
||
| // Check if path exists | ||
| if (!fs.existsSync(absolutePath)) { |
There was a problem hiding this comment.
Using synchronous fs.existsSync and fs.statSync may block the event loop; consider using asynchronous alternatives if scalability is a concern.
| }); | ||
|
|
||
| // Run the scan | ||
| const modelAudit = spawn('modelaudit', args); |
There was a problem hiding this comment.
Consider adding a timeout or kill mechanism to the spawn call if the scan hangs.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
18 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| if (!response.ok) { | ||
| const errorData = await response.json(); | ||
| throw new Error(errorData.error || 'Failed to scan models'); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| setScanResults(data); |
There was a problem hiding this comment.
logic: Attempting to parse response body twice - once for error and once for success. This will fail as the body stream can only be consumed once.
| if (!response.ok) { | |
| const errorData = await response.json(); | |
| throw new Error(errorData.error || 'Failed to scan models'); | |
| } | |
| const data = await response.json(); | |
| setScanResults(data); | |
| const data = await response.json(); | |
| if (!response.ok) { | |
| throw new Error(data.error || 'Failed to scan models'); | |
| } | |
| setScanResults(data); |
| label="Add pattern" | ||
| value={blacklistInput} | ||
| onChange={(e) => setBlacklistInput(e.target.value)} | ||
| onKeyPress={(e) => e.key === 'Enter' && handleAddBlacklist()} |
There was a problem hiding this comment.
style: onKeyPress is deprecated in React. Use onKeyDown instead
| onKeyPress={(e) => e.key === 'Enter' && handleAddBlacklist()} | |
| onKeyDown={(e) => e.key === 'Enter' && handleAddBlacklist()} |
| {localOptions.blacklist.map((pattern, index) => ( | ||
| <Chip key={index} label={pattern} onDelete={() => handleRemoveBlacklist(index)} /> | ||
| ))} |
There was a problem hiding this comment.
style: Using array index as key prop can cause issues with React's reconciliation if items are reordered. Consider using pattern string as key
| } | ||
|
|
||
| export interface ScanIssue { | ||
| severity: 'error' | 'warning' | 'info' | 'debug'; |
There was a problem hiding this comment.
style: severity should be defined as a type literal 'const severityType = "error" | "warning" | "info" | "debug"' to enable reuse
| const [blacklistInput, setBlacklistInput] = useState(''); | ||
| const [localOptions, setLocalOptions] = useState(scanOptions); |
There was a problem hiding this comment.
logic: Initial state is set from props but won't update if scanOptions prop changes. Consider using useEffect to sync
| {scanResults.scannedFilesList.map((file, index) => ( | ||
| <ListItem key={index} sx={{ py: 0.5 }}> |
There was a problem hiding this comment.
style: Using array index as key prop can cause issues with React reconciliation if list items are reordered or deleted. Consider using file path as key since it should be unique.
| {scanResults.scannedFilesList.map((file, index) => ( | |
| <ListItem key={index} sx={{ py: 0.5 }}> | |
| {scanResults.scannedFilesList.map((file, index) => ( | |
| <ListItem key={file} sx={{ py: 0.5 }}> |
| {paths.map((path, index) => ( | ||
| <ListItem key={index} sx={{ py: 0.5 }}> |
There was a problem hiding this comment.
style: Similarly, use path.path as key instead of array index for stable list rendering.
| {paths.map((path, index) => ( | |
| <ListItem key={index} sx={{ py: 0.5 }}> | |
| {paths.map((path, index) => ( | |
| <ListItem key={path.path} sx={{ py: 0.5 }}> |
| const getSeverityIcon = (severity: string) => { | ||
| switch (severity) { | ||
| case 'error': | ||
| return <ErrorIcon fontSize="small" />; | ||
| case 'warning': | ||
| return <WarningIcon fontSize="small" />; | ||
| case 'info': | ||
| return <InfoIcon fontSize="small" />; | ||
| default: | ||
| return <InfoIcon fontSize="small" />; | ||
| } | ||
| }; |
There was a problem hiding this comment.
style: getSeverityIcon function returns InfoIcon for both 'info' and default case. Consider adding 'debug' case or using a different icon for default.
| {issue.details && ( | ||
| <Collapse in={true}> | ||
| <Paper sx={{ p: 2, mt: 2, bgcolor: 'grey.50' }} variant="outlined"> | ||
| <Typography | ||
| variant="caption" | ||
| component="pre" | ||
| sx={{ fontFamily: 'monospace', whiteSpace: 'pre-wrap' }} | ||
| > | ||
| {JSON.stringify(issue.details, null, 2)} | ||
| </Typography> | ||
| </Paper> | ||
| </Collapse> | ||
| )} |
There was a problem hiding this comment.
style: Collapse component always has in={true}. Remove the Collapse wrapper if no dynamic collapsing is needed.
| {issue.details && ( | |
| <Collapse in={true}> | |
| <Paper sx={{ p: 2, mt: 2, bgcolor: 'grey.50' }} variant="outlined"> | |
| <Typography | |
| variant="caption" | |
| component="pre" | |
| sx={{ fontFamily: 'monospace', whiteSpace: 'pre-wrap' }} | |
| > | |
| {JSON.stringify(issue.details, null, 2)} | |
| </Typography> | |
| </Paper> | |
| </Collapse> | |
| )} | |
| {issue.details && ( | |
| <Paper sx={{ p: 2, mt: 2, bgcolor: 'grey.50' }} variant="outlined"> | |
| <Typography | |
| variant="caption" | |
| component="pre" | |
| sx={{ fontFamily: 'monospace', whiteSpace: 'pre-wrap' }} | |
| > | |
| {JSON.stringify(issue.details, null, 2)} | |
| </Typography> | |
| </Paper> | |
| )} |
| if (code === 0 || code === 1) { | ||
| try { | ||
| // Find JSON in the output (it might be mixed with other log messages) | ||
| const jsonMatch = stdout.match(/\{[\s\S]*\}/); |
There was a problem hiding this comment.
logic: Regex pattern /{[\s\S]*}/ is too greedy and may match wrong JSON if output contains multiple JSON objects. Use a more specific pattern or JSON stream parser.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @typpo - I've reviewed your changes - here's some feedback:
- The PathSelector component is over 400 lines and handles too many concerns—consider breaking it into smaller subcomponents (e.g. RecentScans, TabPanels, InputField) to improve readability and testability.
- In the scan endpoint you’re mixing synchronous fs operations (fs.existsSync, fs.statSync) and promisified exec on every request, which can block the event loop under load—migrate to async fs methods and cache the modelaudit installation check to reduce overhead.
- The spawn-based scan handler wires both 'error' and 'close' events without guarding against multiple responses—ensure you remove or guard listeners so only one response is sent per request.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // Check if path exists | ||
| if (!fs.existsSync(absolutePath)) { | ||
| res.json({ exists: false, type: null }); | ||
| return; | ||
| } | ||
|
|
||
| // Get path stats | ||
| const stats = fs.statSync(absolutePath); | ||
| const type = stats.isDirectory() ? 'directory' : 'file'; |
There was a problem hiding this comment.
suggestion (performance): Use async fs APIs instead of sync methods to avoid blocking the event loop.
Using synchronous fs methods can block the event loop. Please switch to fs.promises.access and fs.promises.stat or other async alternatives.
| // Check if path exists | |
| if (!fs.existsSync(absolutePath)) { | |
| res.json({ exists: false, type: null }); | |
| return; | |
| } | |
| // Get path stats | |
| const stats = fs.statSync(absolutePath); | |
| const type = stats.isDirectory() ? 'directory' : 'file'; | |
| // Check if path exists | |
| try { | |
| await fs.promises.access(absolutePath); | |
| } catch { | |
| res.json({ exists: false, type: null }); | |
| return; | |
| } | |
| // Get path stats | |
| const stats = await fs.promises.stat(absolutePath); | |
| const type = stats.isDirectory() ? 'directory' : 'file'; |
| ).length; | ||
|
|
||
| res.json({ | ||
| path: resolvedPaths[0], |
There was a problem hiding this comment.
issue: Only the first resolved path is returned in results.
Consider returning all resolved paths or aggregating results if multiple paths are scanned, instead of just the first one.
| onKeyPress={(e) => { | ||
| if (e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| handleAddPath(pathInput); | ||
| } | ||
| }} |
There was a problem hiding this comment.
suggestion: Use onKeyDown instead of onKeyPress for reliable Enter key handling.
onKeyDown is recommended since onKeyPress is deprecated and may not work consistently.
| onKeyPress={(e) => { | |
| if (e.key === 'Enter') { | |
| e.preventDefault(); | |
| handleAddPath(pathInput); | |
| } | |
| }} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter') { | |
| e.preventDefault(); | |
| handleAddPath(pathInput); | |
| } | |
| }} |
| onOptionsChange, | ||
| }: AdvancedOptionsDialogProps) { | ||
| const [blacklistInput, setBlacklistInput] = useState(''); | ||
| const [localOptions, setLocalOptions] = useState(scanOptions); |
There was a problem hiding this comment.
issue (bug_risk): localOptions state won't update if scanOptions prop changes.
Consider syncing localOptions with scanOptions when the dialog opens, such as with a useEffect on the open prop, to prevent stale state.
| </Stack> | ||
| </DialogTitle> | ||
| <DialogContent> | ||
| {scanResults?.scannedFilesList && scanResults.scannedFilesList.length > 0 ? ( |
There was a problem hiding this comment.
issue (bug_risk): scannedFilesList is never populated by the API.
Since scannedFilesList is not present in the API response, this UI branch is unreachable. Update the API to include it or modify the UI to use available data.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughThis change introduces a new "Model Audit" feature to the application. It adds a dedicated UI page, navigation link, React components, TypeScript types, Zustand store for recent scans, and a set of Express API endpoints for scanning model files for security vulnerabilities using the Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
src/app/src/App.tsx(2 hunks)src/app/src/components/Navigation.tsx(1 hunks)src/app/src/pages/model-audit/ModelAudit.styles.ts(1 hunks)src/app/src/pages/model-audit/ModelAudit.tsx(1 hunks)src/app/src/pages/model-audit/ModelAudit.types.ts(1 hunks)src/app/src/pages/model-audit/components/AdvancedOptionsDialog.tsx(1 hunks)src/app/src/pages/model-audit/components/ConfigurationTab.tsx(1 hunks)src/app/src/pages/model-audit/components/InstallationCheck.tsx(1 hunks)src/app/src/pages/model-audit/components/PathSelector.tsx(1 hunks)src/app/src/pages/model-audit/components/ResultsTab.tsx(1 hunks)src/app/src/pages/model-audit/components/ScanStatistics.tsx(1 hunks)src/app/src/pages/model-audit/components/ScannedFilesDialog.tsx(1 hunks)src/app/src/pages/model-audit/components/SecurityFindings.tsx(1 hunks)src/app/src/pages/model-audit/components/index.ts(1 hunks)src/app/src/pages/model-audit/page.tsx(1 hunks)src/app/src/pages/model-audit/store.ts(1 hunks)src/server/routes/modelAudit.ts(1 hunks)src/server/server.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/app/src/App.tsx (1)
src/app/src/pages/model-audit/page.tsx (1)
ModelAuditPage(9-15)
src/server/server.ts (1)
src/server/routes/modelAudit.ts (1)
modelAuditRouter(13-13)
src/app/src/pages/model-audit/page.tsx (1)
src/app/src/pages/model-audit/ModelAudit.tsx (1)
ModelAudit(23-202)
src/app/src/pages/model-audit/components/AdvancedOptionsDialog.tsx (1)
src/app/src/pages/model-audit/ModelAudit.types.ts (1)
ScanOptions(7-12)
src/app/src/pages/model-audit/components/ResultsTab.tsx (3)
src/app/src/pages/model-audit/ModelAudit.types.ts (1)
ScanResult(22-31)src/app/src/pages/model-audit/components/ScanStatistics.tsx (1)
ScanStatistics(19-91)src/app/src/pages/model-audit/components/SecurityFindings.tsx (1)
SecurityFindings(35-227)
src/app/src/pages/model-audit/store.ts (1)
src/app/src/pages/model-audit/ModelAudit.types.ts (1)
ScanPath(1-5)
⏰ Context from checks skipped due to timeout of 90000ms (23)
- GitHub Check: Tusk Test Runner
- GitHub Check: Share Test
- GitHub Check: Redteam Custom Enterprise Server
- GitHub Check: Redteam
- GitHub Check: webui tests
- GitHub Check: Build Docs
- GitHub Check: Test on Node 24.x and macOS-latest
- GitHub Check: Test on Node 24.x and ubuntu-latest
- GitHub Check: Test on Node 22.x and macOS-latest
- GitHub Check: Build on Node 24.x
- GitHub Check: Test on Node 22.x and ubuntu-latest
- GitHub Check: Build on Node 22.x
- GitHub Check: Test on Node 20.x and macOS-latest
- GitHub Check: Style Check
- GitHub Check: Build on Node 20.x
- GitHub Check: Test on Node 20.x and windows-latest
- GitHub Check: Build on Node 18.x
- GitHub Check: Test on Node 20.x and ubuntu-latest
- GitHub Check: Test on Node 18.x and macOS-latest
- GitHub Check: Test on Node 18.x and windows-latest
- GitHub Check: Tusk Tester
- GitHub Check: Test on Node 18.x and ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
src/app/src/components/Navigation.tsx (1)
184-184: Model Audit link integrated in navigation
The new navigation item for “Model Audit” is correctly added and will be highlighted when the route matches.src/server/server.ts (2)
36-36: Import modelAuditRouter
The router for model audit endpoints is properly imported.
184-184: Register modelAuditRouter under/api/model-audit
The new API routes are mounted correctly, matching existing routing patterns.src/app/src/App.tsx (2)
21-21: Import ModelAuditPage
TheModelAuditPagecomponent is correctly imported for routing.
66-66: Add/model-auditroute
The new route rendersModelAuditPageand is included alongside existing routes.src/app/src/pages/model-audit/page.tsx (1)
1-15: Introduce ModelAuditPage with error boundary
The page component correctly wrapsModelAuditin anErrorBoundary, ensuring robust error handling on the new route.src/app/src/pages/model-audit/components/InstallationCheck.tsx (1)
1-32: LGTM! Clean and accessible implementation.The component is well-structured with proper Material UI usage and security attributes on the external link. The implementation effectively guides users through the installation process.
src/app/src/pages/model-audit/ModelAudit.styles.ts (2)
3-24: Excellent severity badge implementation with proper theming.The
SeverityBadgecomponent correctly maps severity levels to theme colors and uses appropriate alpha transparency for backgrounds. The styling is consistent and accessible.
26-35: Well-designed interactive card with smooth transitions.The
StatCardcomponent provides good user feedback with the hover effect and maintains accessibility with the pointer cursor. The transition timing is appropriate for smooth interactions.src/app/src/pages/model-audit/components/ScannedFilesDialog.tsx (2)
19-24: Well-defined TypeScript interface with clear prop types.The interface properly defines all required props with appropriate types, including the union type for
ScanResult | nullwhich handles the case when no scan has been performed yet.
84-88: Good fallback handling for empty states.The component appropriately handles the case when no scan results are available with a clear, user-friendly message.
src/app/src/pages/model-audit/components/ResultsTab.tsx (2)
7-10: Clean interface design with essential props.The interface correctly defines the required props, including the callback for opening the files dialog that gets passed through to the statistics component.
12-33: Excellent state coordination between child components.The component effectively manages local UI state (
selectedSeverityandshowRawOutput) and correctly passes the appropriate props and callbacks to bothScanStatisticsandSecurityFindingscomponents. The implementation is clean and follows React best practices.src/app/src/pages/model-audit/components/ConfigurationTab.tsx (1)
19-69: Well-structured configuration component.The component properly handles all the required functionality with good separation of concerns, proper error handling, and intuitive UX patterns.
src/app/src/pages/model-audit/components/SecurityFindings.tsx (1)
188-190:❌ Incorrect review comment
Sanitize JSON output to prevent XSS.
When displaying user-controlled data in JSON format, ensure it's properly sanitized to prevent potential XSS attacks.
The JSON.stringify output is displayed in a pre element. Verify that the issue details cannot contain malicious content that could lead to XSS:
🏁 Script executed:
#!/bin/bash # Search for any HTML/script content handling in the codebase rg -A 5 -B 5 "innerHTML|dangerouslySetInnerHTML|eval|Function" --type tsx --type tsLength of output: 113
No XSS risk with JSON.stringify in React
React automatically escapes any string content rendered via JSX expressions. Since we’re using{JSON.stringify(issue.details, null, 2)}(notdangerouslySetInnerHTMLor directinnerHTML), there’s no injected HTML context and thus no XSS vector. You can safely ignore this suggestion.Likely an incorrect or invalid review comment.
src/app/src/pages/model-audit/store.ts (1)
54-54: Consider removing skipHydration for better SSR compatibility.The
skipHydration: truesetting may cause issues if the application uses server-side rendering. Consider whether this is necessary for your use case.Are there specific hydration issues that require
skipHydration: true, or can this be removed for better SSR support?src/server/routes/modelAudit.ts (2)
198-199: Verify file count fallback logic accuracy.Using
resolvedPaths.lengthas a fallback for file counts may not accurately represent the actual number of files scanned, especially when scanning directories.The fallback logic assumes one-to-one mapping between input paths and scanned files, which may not be accurate when directories are involved. Consider either:
- Implementing a more accurate file counting mechanism
- Clearly documenting this limitation
- Making the fallback more conservative (e.g., return
nullinstead of an estimate)Also applies to: 268-268
161-161:❌ Incorrect review comment
Potential command injection in spawn arguments.
While the current implementation appears safe, the spawn call should validate arguments to prevent potential command injection if the options parsing is extended.
Verify that all user inputs in the
argsarray are properly sanitized:
🏁 Script executed:
#!/bin/bash # Search for any dynamic argument building that might be vulnerable rg -A 5 -B 5 "args\.push|spawn.*args" --type tsLength of output: 4501
🏁 Script executed:
#!/bin/bash rg "import.*spawn" -n src/server/routes/modelAudit.tsLength of output: 96
No command injection risk with
spawnand argument arraysNode’s
child_process.spawndoes not invoke a shell by default, so passing user-supplied values in theargsarray cannot trigger OS-level command injection. You only need to ensure that file paths (resolvedPaths) are legitimately resolved (e.g., viapath.resolveagainst an approved base directory) to prevent path-traversal—no extra “shell-style” sanitization is required here.
- File:
src/server/routes/modelAudit.ts
spawn call:uses an args array withconst modelAudit = spawn('modelaudit', args);shell: falseby default.Likely an incorrect or invalid review comment.
| export { default as AdvancedOptionsDialog } from './AdvancedOptionsDialog'; | ||
| export { default as ConfigurationTab } from './ConfigurationTab'; | ||
| export { default as InstallationCheck } from './InstallationCheck'; | ||
| export { default as PathSelector } from './PathSelector'; | ||
| export { default as ResultsTab } from './ResultsTab'; | ||
| export { default as ScanStatistics } from './ScanStatistics'; | ||
| export { default as ScannedFilesDialog } from './ScannedFilesDialog'; | ||
| export { default as SecurityFindings } from './SecurityFindings'; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Centralize component exports
A clean index file that re-exports all model audit components. Consider ordering these exports alphabetically to improve maintainability.
🤖 Prompt for AI Agents
In src/app/src/pages/model-audit/components/index.ts lines 1 to 8, the component
exports are not ordered alphabetically. Reorder the export statements
alphabetically by component name to improve maintainability and make it easier
to locate exports.
| <Paper sx={{ p: 3, bgcolor: 'grey.100', maxWidth: 500, mx: 'auto', my: 3 }}> | ||
| <Typography variant="body1" fontFamily="monospace" fontWeight={500}> | ||
| pip install modelaudit | ||
| </Typography> | ||
| </Paper> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving accessibility for the installation command.
The installation command could benefit from better accessibility by adding an aria-label or role attribute to help screen readers identify it as a code snippet.
<Paper sx={{ p: 3, bgcolor: 'grey.100', maxWidth: 500, mx: 'auto', my: 3 }}>
<Typography
variant="body1"
fontFamily="monospace"
fontWeight={500}
+ role="code"
+ aria-label="Installation command"
>
pip install modelaudit
</Typography>
</Paper>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Paper sx={{ p: 3, bgcolor: 'grey.100', maxWidth: 500, mx: 'auto', my: 3 }}> | |
| <Typography variant="body1" fontFamily="monospace" fontWeight={500}> | |
| pip install modelaudit | |
| </Typography> | |
| </Paper> | |
| <Paper sx={{ p: 3, bgcolor: 'grey.100', maxWidth: 500, mx: 'auto', my: 3 }}> | |
| <Typography | |
| variant="body1" | |
| fontFamily="monospace" | |
| fontWeight={500} | |
| role="code" | |
| aria-label="Installation command" | |
| > | |
| pip install modelaudit | |
| </Typography> | |
| </Paper> |
🤖 Prompt for AI Agents
In src/app/src/pages/model-audit/components/InstallationCheck.tsx around lines
16 to 20, the installation command inside the Typography component lacks
accessibility attributes. Add an appropriate aria-label or role attribute to the
Typography element to indicate it is a code snippet, such as role="code" or
aria-label="Installation command", to improve screen reader identification and
accessibility.
| {scanResults?.scannedFilesList && scanResults.scannedFilesList.length > 0 ? ( | ||
| <List sx={{ maxHeight: 400, overflow: 'auto' }}> | ||
| {scanResults.scannedFilesList.map((file, index) => ( | ||
| <ListItem key={index} sx={{ py: 0.5 }}> | ||
| <ListItemIcon> | ||
| <FileIcon fontSize="small" /> | ||
| </ListItemIcon> | ||
| <ListItemText | ||
| primary={file} | ||
| primaryTypographyProps={{ fontSize: '0.875rem', fontFamily: 'monospace' }} | ||
| /> | ||
| </ListItem> | ||
| ))} | ||
| </List> |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider virtualization for large file lists.
The current implementation works well, but for directories with thousands of files, the list could cause performance issues since all items are rendered at once.
For better performance with large file lists, consider using Material UI's virtualized list or adding pagination:
- <List sx={{ maxHeight: 400, overflow: 'auto' }}>
+ <List sx={{ maxHeight: 400, overflow: 'auto' }}>
+ {/* Consider react-window or similar for virtualization */}
{scanResults.scannedFilesList.map((file, index) => (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {scanResults?.scannedFilesList && scanResults.scannedFilesList.length > 0 ? ( | |
| <List sx={{ maxHeight: 400, overflow: 'auto' }}> | |
| {scanResults.scannedFilesList.map((file, index) => ( | |
| <ListItem key={index} sx={{ py: 0.5 }}> | |
| <ListItemIcon> | |
| <FileIcon fontSize="small" /> | |
| </ListItemIcon> | |
| <ListItemText | |
| primary={file} | |
| primaryTypographyProps={{ fontSize: '0.875rem', fontFamily: 'monospace' }} | |
| /> | |
| </ListItem> | |
| ))} | |
| </List> | |
| {scanResults?.scannedFilesList && scanResults.scannedFilesList.length > 0 ? ( | |
| <List sx={{ maxHeight: 400, overflow: 'auto' }}> | |
| {/* Consider react-window or similar for virtualization */} | |
| {scanResults.scannedFilesList.map((file, index) => ( | |
| <ListItem key={index} sx={{ py: 0.5 }}> | |
| <ListItemIcon> | |
| <FileIcon fontSize="small" /> | |
| </ListItemIcon> | |
| <ListItemText | |
| primary={file} | |
| primaryTypographyProps={{ fontSize: '0.875rem', fontFamily: 'monospace' }} | |
| /> | |
| </ListItem> | |
| ))} | |
| </List> |
🤖 Prompt for AI Agents
In src/app/src/pages/model-audit/components/ScannedFilesDialog.tsx around lines
43 to 56, the current rendering of scannedFilesList maps all files directly,
which can cause performance issues with large lists. To fix this, replace the
List and map approach with a virtualized list component from Material UI or
implement pagination to render only a subset of files at a time, improving
rendering performance and responsiveness for large directories.
| export interface ScanIssue { | ||
| severity: 'error' | 'warning' | 'info' | 'debug'; | ||
| message: string; | ||
| location?: string | null; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid using string | null when string | undefined is more appropriate.
The location field uses string | null but TypeScript optional properties typically use undefined by convention. This creates inconsistency with the optional operator.
- location?: string | null;
+ location?: string;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| location?: string | null; | |
| location?: string; |
🤖 Prompt for AI Agents
In src/app/src/pages/model-audit/ModelAudit.types.ts at line 17, change the type
of the optional property 'location' from 'string | null' to 'string | undefined'
to align with TypeScript conventions for optional properties. This means
removing the explicit 'null' type and relying on the optional property to
represent undefined values.
| timeout: number; | ||
| maxFileSize?: number; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add validation constraints for numeric fields.
Consider adding JSDoc comments or using branded types to specify valid ranges for timeout and maxFileSize to prevent negative values.
export interface ScanOptions {
blacklist: string[];
- timeout: number;
- maxFileSize?: number;
+ /** Timeout in seconds, must be positive */
+ timeout: number;
+ /** Maximum file size in bytes, must be positive */
+ maxFileSize?: number;
verbose: boolean;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| timeout: number; | |
| maxFileSize?: number; | |
| export interface ScanOptions { | |
| blacklist: string[]; | |
| /** Timeout in seconds, must be positive */ | |
| timeout: number; | |
| /** Maximum file size in bytes, must be positive */ | |
| maxFileSize?: number; | |
| verbose: boolean; | |
| } |
🤖 Prompt for AI Agents
In src/app/src/pages/model-audit/ModelAudit.types.ts at lines 9 to 10, the
numeric fields timeout and maxFileSize lack validation constraints. Add JSDoc
comments above these fields specifying their valid ranges, such as timeout must
be non-negative and maxFileSize, if present, must also be non-negative.
Alternatively, define branded types or use TypeScript utility types to enforce
these constraints at the type level to prevent negative values.
| // Check if modelaudit is installed | ||
| modelAuditRouter.get('/check-installed', async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| await execAsync('python -c "import modelaudit"'); |
There was a problem hiding this comment.
Security risk: Command injection vulnerability.
Both instances execute Python commands without proper input validation or sanitization. While these specific cases use hardcoded import statements, this pattern could be exploited if extended carelessly.
Consider using a safer approach:
- await execAsync('python -c "import modelaudit"');
+ await execAsync('python', ['-c', 'import modelaudit']);Alternatively, use a dedicated Python validation function that doesn't rely on string concatenation.
Also applies to: 79-79
🤖 Prompt for AI Agents
In src/server/routes/modelAudit.ts at lines 18 and 79, the use of execAsync with
a Python command string poses a command injection risk. Replace the string
command execution with a safer method, such as using a child process spawn with
arguments passed as an array to avoid shell interpretation, or implement a
dedicated Python validation function that does not rely on string concatenation.
This prevents injection vulnerabilities by avoiding direct execution of
concatenated command strings.
| // Handle home directory expansion | ||
| let expandedPath = inputPath; | ||
| if (expandedPath.startsWith('~/')) { | ||
| expandedPath = path.join(os.homedir(), expandedPath.slice(2)); | ||
| } | ||
|
|
||
| const absolutePath = path.isAbsolute(expandedPath) | ||
| ? expandedPath | ||
| : path.resolve(process.cwd(), expandedPath); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract path resolution logic to reduce code duplication.
The path expansion and resolution logic is duplicated between the check-path and scan endpoints.
Create a utility function:
function resolvePath(inputPath: string): string {
let expandedPath = inputPath;
if (expandedPath.startsWith('~/')) {
expandedPath = path.join(os.homedir(), expandedPath.slice(2));
}
return path.isAbsolute(expandedPath)
? expandedPath
: path.resolve(process.cwd(), expandedPath);
}Then use it in both endpoints to eliminate duplication.
Also applies to: 95-103
🤖 Prompt for AI Agents
In src/server/routes/modelAudit.ts around lines 35 to 43 and 95 to 103, the path
expansion and resolution logic is duplicated. Extract this logic into a new
utility function named resolvePath that takes inputPath as a parameter, expands
the home directory if the path starts with '~/,' and returns the absolute path.
Replace the duplicated code in both the check-path and scan endpoints by calling
this new resolvePath function with the input path to eliminate redundancy.
| // Run model scan | ||
| modelAuditRouter.post('/scan', async (req: Request, res: Response): Promise<void> => { | ||
| try { | ||
| const { paths, options } = req.body; | ||
|
|
||
| if (!paths || !Array.isArray(paths) || paths.length === 0) { | ||
| res.status(400).json({ error: 'No paths provided' }); | ||
| return; | ||
| } | ||
|
|
||
| // Check if modelaudit is installed | ||
| try { | ||
| await execAsync('python -c "import modelaudit"'); | ||
| } catch { | ||
| res.status(400).json({ | ||
| error: 'ModelAudit is not installed. Please install it using: pip install modelaudit', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| // Resolve paths to absolute paths | ||
| const resolvedPaths: string[] = []; | ||
| for (const inputPath of paths) { | ||
| // Skip empty paths | ||
| if (!inputPath || inputPath.trim() === '') { | ||
| continue; | ||
| } | ||
|
|
||
| // Handle home directory expansion | ||
| let expandedPath = inputPath; | ||
| if (expandedPath.startsWith('~/')) { | ||
| expandedPath = path.join(os.homedir(), expandedPath.slice(2)); | ||
| } | ||
|
|
||
| const absolutePath = path.isAbsolute(expandedPath) | ||
| ? expandedPath | ||
| : path.resolve(process.cwd(), expandedPath); | ||
|
|
||
| // Check if path exists | ||
| if (!fs.existsSync(absolutePath)) { | ||
| res | ||
| .status(400) | ||
| .json({ error: `Path does not exist: ${inputPath} (resolved to: ${absolutePath})` }); | ||
| return; | ||
| } | ||
|
|
||
| resolvedPaths.push(absolutePath); | ||
| } | ||
|
|
||
| if (resolvedPaths.length === 0) { | ||
| res.status(400).json({ error: 'No valid paths to scan' }); | ||
| return; | ||
| } | ||
|
|
||
| // Build command arguments | ||
| const args = ['scan']; | ||
|
|
||
| // Add resolved paths | ||
| args.push(...resolvedPaths); | ||
|
|
||
| // Add options | ||
| if (options.blacklist && Array.isArray(options.blacklist)) { | ||
| options.blacklist.forEach((pattern: string) => { | ||
| args.push('--blacklist', pattern); | ||
| }); | ||
| } | ||
|
|
||
| // Always use JSON format for API responses | ||
| args.push('--format', 'json'); | ||
|
|
||
| if (options.timeout) { | ||
| args.push('--timeout', String(options.timeout)); | ||
| } | ||
|
|
||
| if (options.maxFileSize) { | ||
| args.push('--max-file-size', String(options.maxFileSize)); | ||
| } | ||
|
|
||
| if (options.verbose) { | ||
| args.push('--verbose'); | ||
| } | ||
|
|
||
| logger.info(`Running model scan on: ${resolvedPaths.join(', ')}`); | ||
|
|
||
| // Track the scan | ||
| await telemetry.recordAndSend('webui_api', { | ||
| event: 'model_scan', | ||
| pathCount: paths.length, | ||
| hasBlacklist: options.blacklist?.length > 0, | ||
| timeout: options.timeout, | ||
| verbose: options.verbose, | ||
| }); | ||
|
|
||
| // Run the scan | ||
| const modelAudit = spawn('modelaudit', args); | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
|
|
||
| modelAudit.stdout.on('data', (data) => { | ||
| stdout += data.toString(); | ||
| }); | ||
|
|
||
| modelAudit.stderr.on('data', (data) => { | ||
| stderr += data.toString(); | ||
| }); | ||
|
|
||
| modelAudit.on('error', (error) => { | ||
| logger.error(`Failed to start modelaudit: ${error.message}`); | ||
| res.status(500).json({ | ||
| error: 'Failed to start model scan. Make sure Python and modelaudit are installed.', | ||
| }); | ||
| }); | ||
|
|
||
| modelAudit.on('close', (code) => { | ||
| // ModelAudit returns exit code 1 when it finds issues, which is expected | ||
| if (code === 0 || code === 1) { | ||
| try { | ||
| // Find JSON in the output (it might be mixed with other log messages) | ||
| const jsonMatch = stdout.match(/\{[\s\S]*\}/); | ||
| if (!jsonMatch) { | ||
| throw new Error('No JSON found in output'); | ||
| } | ||
|
|
||
| // Parse JSON output | ||
| const scanResults = JSON.parse(jsonMatch[0]); | ||
|
|
||
| // Transform the results into our expected format | ||
| const transformedResults = { | ||
| path: resolvedPaths[0], // Primary resolved path | ||
| issues: scanResults.issues || [], | ||
| success: true, | ||
| scannedFiles: scanResults.files_scanned || resolvedPaths.length, | ||
| totalFiles: scanResults.files_total || resolvedPaths.length, | ||
| duration: scanResults.scan_duration || null, | ||
| rawOutput: stdout, // Always include raw output for debugging | ||
| }; | ||
|
|
||
| res.json(transformedResults); | ||
| } catch (parseError) { | ||
| logger.debug(`Failed to parse JSON from stdout: ${parseError}`); | ||
| logger.debug(`stdout: ${stdout}`); | ||
| logger.debug(`stderr: ${stderr}`); | ||
| // If JSON parsing fails, parse text output | ||
| const issues: Array<{ severity: string; message: string; location?: string }> = []; | ||
|
|
||
| // Parse the log format output | ||
| const lines = stdout.split('\n'); | ||
| lines.forEach((line) => { | ||
| // Parse lines like: "2025-06-08 20:46:58,090 - modelaudit.scanners - WARNING - [WARNING] (/path/to/file): Message" | ||
| const warningMatch = line.match(/\[WARNING\]\s*\(([^)]+)\):\s*(.+)/); | ||
| const errorMatch = line.match(/\[ERROR\]\s*\(([^)]+)\):\s*(.+)/); | ||
| const infoMatch = line.match(/\[INFO\]\s*\(([^)]+)\):\s*(.+)/); | ||
|
|
||
| if (warningMatch) { | ||
| issues.push({ | ||
| severity: 'warning', | ||
| message: warningMatch[2], | ||
| location: warningMatch[1], | ||
| }); | ||
| } else if (errorMatch) { | ||
| issues.push({ | ||
| severity: 'error', | ||
| message: errorMatch[2], | ||
| location: errorMatch[1], | ||
| }); | ||
| } else if (infoMatch) { | ||
| issues.push({ | ||
| severity: 'info', | ||
| message: infoMatch[2], | ||
| location: infoMatch[1], | ||
| }); | ||
| } else if (line.includes(' - WARNING - ')) { | ||
| // Fallback for other warning formats | ||
| const parts = line.split(' - WARNING - '); | ||
| if (parts[1]) { | ||
| issues.push({ | ||
| severity: 'warning', | ||
| message: parts[1], | ||
| }); | ||
| } | ||
| } else if (line.includes(' - ERROR - ')) { | ||
| // Fallback for other error formats | ||
| const parts = line.split(' - ERROR - '); | ||
| if (parts[1]) { | ||
| issues.push({ | ||
| severity: 'error', | ||
| message: parts[1], | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // Count scanned files from the output | ||
| const scannedFiles = lines.filter( | ||
| (line) => line.includes('Scanning file:') || line.includes('Scanning directory:'), | ||
| ).length; | ||
|
|
||
| res.json({ | ||
| path: resolvedPaths[0], | ||
| issues, | ||
| success: true, | ||
| scannedFiles: scannedFiles || resolvedPaths.length, | ||
| rawOutput: stdout, | ||
| }); | ||
| } | ||
| } else { | ||
| // Only treat codes other than 0 and 1 as actual errors | ||
| logger.error(`Model scan failed with code ${code}: ${stderr}`); | ||
| res.status(500).json({ | ||
| error: `Model scan failed with exit code ${code}: ${stderr || stdout || 'Unknown error'}`, | ||
| code, | ||
| }); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| logger.error(`Error in model scan: ${error}`); | ||
| res.status(500).json({ error: String(error) }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider refactoring the scan endpoint for better maintainability.
The scan endpoint is quite long (200+ lines) and handles multiple responsibilities. This makes it harder to test and maintain.
Consider breaking it down into smaller functions:
validateScanPaths(paths: string[])buildScanCommand(paths: string[], options: any)executeScan(command: string[])parseScanOutput(stdout: string, stderr: string)
This would improve testability and readability while maintaining the same functionality.
🤖 Prompt for AI Agents
In src/server/routes/modelAudit.ts from lines 67 to 285, the scan endpoint is
too long and handles multiple responsibilities, making it hard to maintain and
test. Refactor by extracting smaller functions: create validateScanPaths(paths:
string[]) to check and resolve input paths; buildScanCommand(paths: string[],
options: any) to construct the command arguments array; executeScan(command:
string[]) to run the scan process and collect output; and
parseScanOutput(stdout: string, stderr: string) to parse and transform the scan
results. Replace the inline code in the endpoint with calls to these functions
to improve readability and maintainability without changing functionality.
| const jsonMatch = stdout.match(/\{[\s\S]*\}/); | ||
| if (!jsonMatch) { | ||
| throw new Error('No JSON found in output'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve JSON extraction robustness.
The current regex \{[\s\S]*\} may match incomplete JSON or fail with nested objects. This could lead to parsing errors in edge cases.
Use a more robust JSON extraction method:
- const jsonMatch = stdout.match(/\{[\s\S]*\}/);
- if (!jsonMatch) {
- throw new Error('No JSON found in output');
- }
+ // Try to find the last complete JSON object in the output
+ let jsonStart = stdout.lastIndexOf('{');
+ if (jsonStart === -1) {
+ throw new Error('No JSON found in output');
+ }
+
+ // Extract from the last '{' to the end and try to parse
+ const jsonCandidate = stdout.substring(jsonStart);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/server/routes/modelAudit.ts around lines 185 to 188, the current regex
used to extract JSON from stdout is too simplistic and may incorrectly capture
incomplete or nested JSON objects. Replace this regex approach with a more
robust method, such as using a JSON parser that attempts to find and parse the
first valid JSON object in the string, or implement a balanced brace matching
algorithm to accurately extract the full JSON content before parsing.
| modelAudit.on('error', (error) => { | ||
| logger.error(`Failed to start modelaudit: ${error.message}`); | ||
| res.status(500).json({ | ||
| error: 'Failed to start model scan. Make sure Python and modelaudit are installed.', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling for spawned processes.
The spawned modelaudit process lacks timeout protection, which could lead to hanging requests if the external tool becomes unresponsive.
Add process timeout handling:
+ const timeoutMs = (options.timeout || 300) * 1000; // Convert to milliseconds
+ const processTimeout = setTimeout(() => {
+ modelAudit.kill('SIGTERM');
+ res.status(408).json({ error: 'Scan operation timed out' });
+ }, timeoutMs);
+
modelAudit.on('error', (error) => {
+ clearTimeout(processTimeout);
logger.error(`Failed to start modelaudit: ${error.message}`);
res.status(500).json({
error: 'Failed to start model scan. Make sure Python and modelaudit are installed.',
});
});
modelAudit.on('close', (code) => {
+ clearTimeout(processTimeout);
// ... rest of the logic📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| modelAudit.on('error', (error) => { | |
| logger.error(`Failed to start modelaudit: ${error.message}`); | |
| res.status(500).json({ | |
| error: 'Failed to start model scan. Make sure Python and modelaudit are installed.', | |
| }); | |
| }); | |
| const timeoutMs = (options.timeout || 300) * 1000; // Convert to milliseconds | |
| const processTimeout = setTimeout(() => { | |
| modelAudit.kill('SIGTERM'); | |
| res.status(408).json({ error: 'Scan operation timed out' }); | |
| }, timeoutMs); | |
| modelAudit.on('error', (error) => { | |
| clearTimeout(processTimeout); | |
| logger.error(`Failed to start modelaudit: ${error.message}`); | |
| res.status(500).json({ | |
| error: 'Failed to start model scan. Make sure Python and modelaudit are installed.', | |
| }); | |
| }); | |
| modelAudit.on('close', (code) => { | |
| clearTimeout(processTimeout); | |
| // ... rest of the logic | |
| }); |
🤖 Prompt for AI Agents
In src/server/routes/modelAudit.ts around lines 173 to 178, the spawned
modelaudit process does not have timeout handling, risking hanging requests if
the process becomes unresponsive. Add a timeout mechanism to the spawned process
by setting a timer that kills the process and returns an error response if it
exceeds a reasonable duration. Ensure the timer is cleared if the process exits
normally to avoid premature termination.
|
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 50.6%, saving 313.25 KB.
273 images did not require optimisation. |
❌ Generated 60 tests - 54 passed, 6 failed (4034aa0) View tests ↗Tip New to Tusk? Learn more here. Tusk generated some tests - view Tusk's output and incorporate tests. Test Summary
ResultsTusk test suite shows good coverage of the new Model Audit UI, but there are several failing tests that need attention. The tests validate core functionality like path selection, security findings display, and advanced options configuration. Most components are working as expected, but there are issues with handling edge cases in the Key Points
The tests cover critical user flows like configuring scans, viewing results, filtering by severity, and exporting findings. The Navigation component correctly shows the new Model Audit link, and the store functions for managing recent scans work properly. Most components handle edge cases well, but the failing tests highlight areas where error handling and input validation need improvement before release. View check history
|
No description provided.