-
-
Notifications
You must be signed in to change notification settings - Fork 273
feat: Add ClawScanner security integration #104
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
base: main
Are you sure you want to change the base?
Conversation
- Add skillScan.ts for malware/vulnerability scanning - Integrate scanner into skill publish flow - Block malicious uploads before publishing - Add tests for scan result handling
|
@Nwosu-Ihueze is attempting to deploy a commit to the Amantus Machina Team on Vercel. A member of the Team first needs to authorize it. |
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.
3 files reviewed, 4 comments
convex/lib/skillScan.ts
Outdated
| if (!SCANNER_URL) { | ||
| console.warn('ClawScanner not configured, skipping security scan') | ||
| return { | ||
| verdict: 'clean', |
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.
[P1] SCANNER_URL is a non-empty constant, so the if (!SCANNER_URL) branch is dead code and the scan will never be skipped.
If the intent is to allow disabling the scanner or switching environments, this should be driven by config/env rather than a hard-coded URL; otherwise the warning/log and “disabled” ScanResult are misleading and untestable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/skillScan.ts
Line: 46:49
Comment:
[P1] `SCANNER_URL` is a non-empty constant, so the `if (!SCANNER_URL)` branch is dead code and the scan will never be skipped.
If the intent is to allow disabling the scanner or switching environments, this should be driven by config/env rather than a hard-coded URL; otherwise the warning/log and “disabled” ScanResult are misleading and untestable.
How can I resolve this? If you propose a fix, please make it concise.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.
"Fixed in commit 65363d7"
| return { | ||
| verdict: 'blocked', | ||
| findings: [ | ||
| { | ||
| rule_id: 'scanner/unavailable', | ||
| engine: 'system', | ||
| severity: 'critical', | ||
| message: 'Security scanner unavailable - upload blocked for safety', | ||
| }, |
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.
[P1] scanSkillFiles converts any scanner/network error into a blocked verdict (see catch block) which will hard-fail skill publishing whenever the external API is down/slow or returns malformed JSON.
If the desired behavior is “fail closed”, that’s fine, but it’s a reliability/availability trade-off worth making explicit (and maybe differentiating between transient errors vs actual blocked verdicts) since it can prevent all publishes during an outage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/skillScan.ts
Line: 88:96
Comment:
[P1] `scanSkillFiles` converts *any* scanner/network error into a `blocked` verdict (see catch block) which will hard-fail skill publishing whenever the external API is down/slow or returns malformed JSON.
If the desired behavior is “fail closed”, that’s fine, but it’s a reliability/availability trade-off worth making explicit (and maybe differentiating between transient errors vs actual `blocked` verdicts) since it can prevent all publishes during an outage.
How can I resolve this? If you propose a fix, please make it concise.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.
"Fixed in commit 65363d7"
convex/lib/skillPublish.ts
Outdated
|
|
||
| const scanResult = await scanSkillFiles(ctx, safeFiles) | ||
| handleScanResult(scanResult) | ||
|
|
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.
[P2] handleScanResult(scanResult) is called but its return value (rule IDs for flagged verdicts) is ignored, so the “publish succeeds but skill is marked for review” behavior described in the PR won’t happen.
If flagged skills are meant to be marked/reviewed, the returned flags need to be persisted (e.g., stored on the version/skill or passed into insertVersion) or used to enqueue moderation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/skillPublish.ts
Line: 117:120
Comment:
[P2] `handleScanResult(scanResult)` is called but its return value (rule IDs for `flagged` verdicts) is ignored, so the “publish succeeds but skill is marked for review” behavior described in the PR won’t happen.
If flagged skills are meant to be marked/reviewed, the returned flags need to be persisted (e.g., stored on the version/skill or passed into `insertVersion`) or used to enqueue moderation.
How can I resolve this? If you propose a fix, please make it concise.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.
"Fixed in commit 65363d7"
| describe('handleScanResult', () => { | ||
| it('returns empty array for clean verdict', () => { | ||
| const result: ScanResult = { | ||
| verdict: 'clean', | ||
| findings: [], | ||
| scanned_at: new Date().toISOString(), | ||
| duration_ms: 50, | ||
| files_scanned: 3, | ||
| scanner_version: '0.1.0', | ||
| summary: { critical: 0, high: 0, total: 0 }, | ||
| } | ||
| const flags = handleScanResult(result) | ||
| expect(flags).toEqual([]) |
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.
[P3] This test file uses 4-space indentation and double quotes are avoided elsewhere, but the repo style guide (AGENTS.md) specifies 2-space indentation + single quotes (Biome). Running formatting/lint in CI may fail or will produce noisy diffs.
Consider running the formatter on this file to match project conventions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: convex/lib/skillScan.test.ts
Line: 4:16
Comment:
[P3] This test file uses 4-space indentation and double quotes are avoided elsewhere, but the repo style guide (AGENTS.md) specifies 2-space indentation + single quotes (Biome). Running formatting/lint in CI may fail or will produce noisy diffs.
Consider running the formatter on this file to match project conventions.
How can I resolve this? If you propose a fix, please make it concise.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.
"Fixed in commit 65363d7"
- Remove dead if-branch for scanner URL check - Add explicit comment explaining fail-closed is intentional - Capture and log flagged skills for moderation visibility - Fix test file formatting (2-space indent) - Validate files added before scanning (Vercel review)
Summary
Adds ClawScanner security integration to scan skills for malware, dangerous code patterns, and vulnerabilities before publishing.
Changes
How it works
Testing
Working on open sourcing the scanner, you can test as a standalone here: clawscanner.xyz
Greptile Overview
Greptile Summary
This PR adds a ClawScanner integration to the skill publish flow by introducing a small scanner client (
convex/lib/skillScan.ts) and invoking it duringpublishVersionForUser(convex/lib/skillPublish.ts) to block uploads with critical/high findings. It also updates Convex generated API typings and lockfile, and adds unit tests for scan result handling.Confidence Score: 3/5
(2/5) Greptile learns from your feedback when you react with thumbs up/down!
Context used:
dashboard- AGENTS.md (source)