fix: unify skill and extension catalog generation to include derived suites#210
Conversation
WalkthroughCatalog parsing and suite derivation are centralized in a shared module. Extension and skills catalog builders now consume normalized data, while agent and MCP catalogs and workflow documentation adopt camelCase fields and suite-driven selection. ChangesCatalog generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BuildScripts
participant CatalogCore
participant CatalogJSON
participant OpforSetup
participant OpforRun
BuildScripts->>CatalogCore: discover and normalize evaluators and suites
CatalogCore->>BuildScripts: return derived suites and validated references
BuildScripts->>CatalogJSON: write surface-specific catalog output
OpforSetup->>CatalogJSON: read suites, evaluatorIds, and criteria
OpforRun->>OpforSetup: use source-scan and correlation metadata
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 🔧 Checkov (3.3.8).github/workflows/ci.ymlTraceback (most recent call last): package.jsonTraceback (most recent call last): 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/lib/catalog-core.mjs (2)
115-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winInvalid severity values silently become
"high".A typo like
sevirity: hgihor an unsupported string silently maps to"high"with no warning, masking config errors in the generated catalog (could misreport a finding's real severity).🛡️ Proposed fix: warn on unrecognized values
function normalizeSeverity(s) { const v = (s || "").toLowerCase(); if (v === "critical" || v === "high" || v === "medium" || v === "low") return v; + if (v) console.warn(`[catalog] unrecognized severity "${s}", defaulting to "high"`); return "high"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/catalog-core.mjs` around lines 115 - 119, Update normalizeSeverity so unrecognized non-empty severity values emit a warning before falling back to "high", while preserving the existing accepted values and fallback behavior. Include the invalid value in the warning to make configuration typos diagnosable.
154-185: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMalformed pattern entries are dropped silently.
Inline patterns missing
name/template(154-167) and file-based fallback patterns missingtemplate(168-185) are skipped with no log, unlike the strict id/name/suite validations elsewhere in this file that throw. A typo in a pattern'stemplatekey would silently shrink the evaluator's attack surface without anyone noticing.🛡️ Proposed fix: warn on skipped inline patterns
if (pName && template) { const pattern = { name: pName, template }; const judgeHint = str(item, "judge_hint").trim(); if (judgeHint) pattern.judgeHint = judgeHint; patterns.push(pattern); + } else { + console.warn(`[catalog] ${filePath}: skipping malformed inline pattern (missing name/template)`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/lib/catalog-core.mjs` around lines 154 - 185, Add warnings when malformed pattern entries are skipped in the inline parsing loop and file-based fallback loop: report the relevant pattern identity/source and the missing or invalid name/template fields before continuing. Preserve valid pattern collection and existing file-read error handling, and ensure malformed entries are no longer silently dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build-catalog.ts`:
- Around line 40-61: Update toSkillEvaluator to preserve the top-level
judge_hint field by mapping it onto the evaluator output alongside the other MCP
judging metadata. Ensure skills/*/catalog.json retains existing judge guidance
while leaving unrelated field handling unchanged.
---
Nitpick comments:
In `@scripts/lib/catalog-core.mjs`:
- Around line 115-119: Update normalizeSeverity so unrecognized non-empty
severity values emit a warning before falling back to "high", while preserving
the existing accepted values and fallback behavior. Include the invalid value in
the warning to make configuration typos diagnosable.
- Around line 154-185: Add warnings when malformed pattern entries are skipped
in the inline parsing loop and file-based fallback loop: report the relevant
pattern identity/source and the missing or invalid name/template fields before
continuing. Preserve valid pattern collection and existing file-read error
handling, and ensure malformed entries are no longer silently dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 717033c4-5cd9-4767-81ab-143e90d76735
📒 Files selected for processing (13)
AGENTS.mdeslint.config.jsrunners/extension/scripts/build-catalog.mjsscripts/build-catalog.tsscripts/lib/catalog-core.mjsskills/agent-redteaming/opfor-run/SKILL.mdskills/agent-redteaming/opfor-run/report-schema.mdskills/agent-redteaming/opfor-setup/SKILL.mdskills/agent-redteaming/opfor-setup/catalog.jsonskills/mcp-redteaming/opfor-run/SKILL.mdskills/mcp-redteaming/opfor-run/report-schema.mdskills/mcp-redteaming/opfor-setup/SKILL.mdskills/mcp-redteaming/opfor-setup/catalog.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/tests/catalog-core.test.mjs (1)
157-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGuard cleanup against partial setup failures.
If
mkdtempfails forsuitesDirduring thebeforehook,suitesDirremains undefined. Theafterhook will then unconditionally passundefinedtorm(), throwing anERR_INVALID_ARG_TYPEwhich masks the original test failure.♻️ Proposed refactor
after(async () => { - await rm(tmpRoot, { recursive: true, force: true }); - await rm(suitesDir, { recursive: true, force: true }); + if (tmpRoot) await rm(tmpRoot, { recursive: true, force: true }); + if (suitesDir) await rm(suitesDir, { recursive: true, force: true }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/catalog-core.test.mjs` around lines 157 - 160, Guard the suitesDir cleanup in the after hook so rm() is called only when mkdtemp successfully initialized suitesDir. Keep cleanup of tmpRoot unchanged and ensure partial setup failures do not trigger a secondary ERR_INVALID_ARG_TYPE that masks the original failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/tests/catalog-core.test.mjs`:
- Around line 157-160: Guard the suitesDir cleanup in the after hook so rm() is
called only when mkdtemp successfully initialized suitesDir. Keep cleanup of
tmpRoot unchanged and ensure partial setup failures do not trigger a secondary
ERR_INVALID_ARG_TYPE that masks the original failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12c5db2c-aeeb-40c9-8c4d-52a390005771
📒 Files selected for processing (8)
.github/workflows/ci.ymlpackage.jsonrunners/extension/scripts/build-catalog.mjsrunners/extension/tests/build-catalog.test.mjsscripts/build-catalog.tsscripts/lib/catalog-core.mjsscripts/tests/build-catalog.test.tsscripts/tests/catalog-core.test.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- scripts/build-catalog.ts
- runners/extension/scripts/build-catalog.mjs
- scripts/lib/catalog-core.mjs
2 participants
Problem
The three catalog generators (
core's runtime loader, the extension'sbuild-catalog.mjs, and the skills'build-catalog.ts) reimplemented the same YAML-walk-and-derive logic independently, and had drifted:skills/{agent,mcp}-redteaming/opfor-setup/catalog.json) only emitted the 4 hand-curated suites and never derived the standards-based suites (OWASP LLM/API/Agentic/MCP Top 10, MITRE ATLAS, EU AI Act, NIST AI RMF) thatcoreand the extension already derive from evaluatorstandards:tags.opfor-setup/SKILL.mdtold the agent to offer exactlyowasp-llm-top10andowasp-agentic-aias the only two suite choices — both absent from the catalog it reads, so following the skill literally was impossible.pass_criteria,scan_mode, suiteevaluators) while the extension catalog used camelCase (passCriteria,scanMode, suiteevaluatorIds) for the same data, for no functional reason.Solution
scripts/lib/catalog-core.mjs(plain ESM, nocoreimport — the skills catalog builds beforetsc -b coreruns, so nothing here can depend oncore/dist).scripts/build-catalog.ts(skills) andrunners/extension/scripts/build-catalog.mjs(extension) now both import this shared module and only encode their genuine differences:opfor-setup/opfor-runSKILL.md + report-schema.md (both agent and mcp bundles) for the new field names, and rewrote the suite-selection step to enumerate suites from the catalog dynamically instead of hardcoding two suite IDs that didn't exist in it.AGENTS.md's Key files table.runners/extension/catalog.jsonoutput is byte-for-byte unchanged — verified by diff before/after against the committed file.Changes
scripts/lib/catalog-core.mjs(new) — shared discovery/parsing/deriveStandardSuitesscripts/build-catalog.ts— rewritten to use the shared module, emit camelCase + derived suitesrunners/extension/scripts/build-catalog.mjs— rewritten to use the shared module (output unchanged)skills/{agent,mcp}-redteaming/opfor-setup/catalog.json— regenerated (camelCase, derived suites added)skills/{agent,mcp}-redteaming/opfor-{setup,run}/SKILL.md,report-schema.md— field-name updates, dynamic suite enumerationAGENTS.md,eslint.config.js(addedscripts/**/*.mjsto the existing node-globals block, needed by the new shared module)Issue
N/A — found while auditing the agent-redteaming skills for drift against the CLI/extension engine.
How to test
All of the above were run locally against a full from-scratch rebuild (deleted all
dist/,tsconfig.tsbuildinfo, and generatedcatalog.jsonfiles first).Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Chores
.mjsbuild scripts.