fix: align skill parameters with actual MCP tool schemas#323
Conversation
713e7f6 to
26bc875
Compare
Review: parameter name corrections look good — but 4 issues remainGreat direction on this PR. The core fixes ( Bug 1 —
|
Root cause: skills documented wrong parameter names that don't exist in the MCP tool schemas. The LLM followed skill instructions, sent invalid params (e.g. severity='Critical'), and the MCP server rejected them. Meanwhile vulnerability__get_system_cves worked because the LLM read its simpler schema directly. Parameter corrections verified from MCP schema dump + production logs: - vulnerability__get_cves: severity→impact (numeric string: 7=Critical, 5=Important, 3=Moderate, 1=Low), affecting→advisory_available, known_exploit is string not boolean, add cvss_from/cvss_to - vulnerability__get_system_cves: only has system_uuid, filter_, limit, offset, sort (no severity/status/remediation) - inventory__list_hosts: limit→per_page (default 10), offset→page, operating_system does not exist, add staleness/order_by enums - Cross-referenced with upstream agentic-plugins parameter reference Also includes: - response-formatting: add Synopsis + CVSS to CVE tables, sort hosts by display_name, per_page=10 default - Debug logging: tool schema props at startup, skill versions at startup Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 3 new eval cases (IDs 4-6) testing: - Multi-severity CVE queries (impact param, advisory_available) - Advisor recommendations (correct tool selection) - System listing (per_page/page params, sort by display_name) Update skill-snapshot/ to pre-fix baseline for comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
iteration-3: initial eval with wrong severity→impact mapping iteration-4: final eval with correct MCP parameter names 5 discriminating assertions improved, 0 regressions: - uses_impact_not_severity (impact='5,7' not severity='Critical') - uses_correct_param_names (advisory_available not affecting) - uses_advisory_available (string 'true' not boolean) - uses_correct_inventory_params (per_page not limit) - sorts_by_display_name (order_by guidance added) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
advisor__get_active_rules has many params with MISSING types in the MCP schema. The schema sanitizer defaults them to string, but the LLM passes boolean True instead of string 'true', causing MCP server rejection. Document all advisor params with explicit string types and add a 'String-typed booleans' section warning against this pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… v1.4 - CVE queries: emphasize always including sort and advisory_available - Inventory listings: emphasize always including order_by=display_name - Fix multi-impact example: 3,4 → 5,7 (correct numeric IDs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ggering v1.5 - Description: make triggering more assertive (ALWAYS load before MCP calls) - Argument Formatting: remove contradicting boolean guidance, add early warning about string-typed booleans - String-typed booleans section: add CRITICAL label, add explicit correct/wrong examples with concrete param names Root cause: Argument Formatting said 'booleans as true/false' which contradicted the String-typed booleans section, causing the LLM to pass impacting=True (boolean) instead of impacting="true" (string). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a 'Recommended defaults for common queries' section with full argument templates for top CVEs, system listings, and advisor queries. Provides copy-ready JSON that includes sort, limit, and all required params — more effective than bolded hints within parameter descriptions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bug 1: Moderate=4 (not 3), Low=2 (not 1) per actual MCP schema enum
Bug 2: evals.json expected_output/assertions updated to '5,7'
Bug 3: error-handling retry example now uses impact="7" instead of
severity=Critical (the exact wrong param this PR removes)
Bug 4: PR description test plan updated (via gh pr edit)
Also adds multi-impact examples: impact="4,5,7" for Moderate+,
impact="2,4,5,7" for all actionable severities.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
738a730 to
fe387e7
Compare
…e 11/17) Skill-creator evaluation results for tool-invocation-rules v1.5: - 94.1% pass rate (16/17) vs 64.7% baseline (11/17) = +29.4% - 5 discriminating assertions all pass: uses_impact_not_severity, uses_correct_param_names, uses_advisory_available, uses_impacting_as_string, uses_correct_inventory_params - 0 regressions - 1 shared failure: flags_stale_checkins (neither version flags it) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Gemini consistently loads only guardrails-safety via load_skill and skips tool-invocation-rules, so the recommended defaults, string-typed boolean rules, and correct parameter names were never in context. This change reads the SKILL.md bodies of STRICT skills at startup and injects them directly into the system prompt (static_instruction). The LLM no longer needs to call load_skill for these — the content is always available. External skill overrides via SKILLS_DIR are respected. The Skills section in AGENT_INSTRUCTION now tells the LLM not to call load_skill for the two STRICT skills since they are pre-loaded. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dmartinol
left a comment
There was a problem hiding this comment.
/lgtm all issues fixed and by pre-injecting STRICT skills into the system prompt at startup you addressed the root cause: Gemini was consistently skipping load_skill for tool-invocation-rules, meaning the correct parameter names were never in context at all.
👍
Summary
Skills documented wrong parameter names that don't exist in the MCP tool schemas. The LLM followed skill instructions, sent invalid params (e.g.,
severity="Critical"), and the MCP server rejected them — causing theError calling tool 'get_cves'failures in production.Root cause: parameter names in skills were guessed, not verified against the actual MCP tool schemas. Production logs with
AGENT_LOGGING_DETAIL=detailedrevealed the actual call args, and the schema debug logging confirmed the real parameter names.Parameter corrections
vulnerability__get_cvesseverity(Critical/Important)impact(numeric: 7=Critical, 5=Important, 4=Moderate, 2=Low)vulnerability__get_cvesaffecting(boolean)advisory_available(string"true")vulnerability__get_cvesknown_exploit(boolean)known_exploit(string"true"/"false")inventory__list_hostslimit/offsetper_page(default 10) /page(starts at 1)inventory__list_hostsoperating_systemvulnerability__get_system_cvesseverity,status,remediationsystem_uuid,filter_,limit,offset,sortChanges
Skills modified (5):
tool-invocation-rulesv1.6 — correct parameter names, types, enum values; recommended default argument sets for common queries; string-typed boolean guidance; pushy skill description for reliable loadingmulti-step-workflowsv1.2 — updated workflows to useimpact,advisory_available,system_uuidresponse-formattingv1.1 — added Synopsis + CVSS Score to CVE tables, sort hosts by display_nameefficient-countingv1.1 — correctedper_page/pagefor inventory countingerror-handlingv1.2 — corrected retry example to useimpact="7"andadvisory_available="true"Debug logging added:
schema_sanitizer.py— logs all tool parameter types and$refusage at startupagent.py— logs each skill name and version at startup for deployment verificationEval workspace:
Test plan
vulnerability__get_cvescall withsort="-cvss_score", advisory_available="true", impact="5,7"(noseverityoraffectingparam)per_page, sorted byorder_by="display_name"impacting="true"(string, not boolean),sort="-total_risk"🤖 Generated with Claude Code