critique-builtin-tool

name	critique-builtin-tool
description	Audit and critique builtin MCP tool implementations in LibrAgent. Use when auditing existing builtin MCP server implementations for compliance, reviewing pull requests that add or modify builtin tools, validating tool implementations against the Tool Design Manifesto v2.1, or identifying potential issues before they reach production.

Critique Builtin MCP Tool Skill

When to Use This Skill

Use this skill when:

• Auditing existing builtin MCP server implementations for compliance
• Reviewing pull requests that add or modify builtin tools
• Validating tool implementations against the Tool Design Manifesto v2.1
• Providing constructive feedback on tool design and implementation
• Identifying potential issues before they reach production

Prerequisites: Familiarity with Tool Design Manifesto v2.1, Rust, and MCP protocol basics.

---

Audit Methodology

Step 1: Understand the Tool Design Manifesto Rules

Before auditing, internalize these critical rules:

Rule 1: The Immutable ID Rule (Schema Design)

• Never expose system-critical IDs as input for CREATE operations
• CREATE tools: System generates ID, agent receives it
• UPDATE/DELETE tools: ID is required input (validated before use)

Rule 2: The Hallucination Firewall (Execution Logic)

• Never trust agent-provided IDs without validation
• Check existence BEFORE any database/state mutation
• Return logic errors (not DB errors) with recovery hints

Rule 3: The Dual-Channel Response Rule (Output)

• Text content (what AI sees): Complete narrative with IDs, status, next steps
• Structured content (what UI sees): JSON for rendering tables/graphs
• Critical IDs MUST be in BOTH channels

Rule 4: AI-Native Descriptions (Input)

• Use data operation terms: "extract", "use", "target"
• Avoid human UI actions: "click", "type", "copy", "paste"
• Document prerequisites explicitly
• Show workflow patterns, not button clicks

Rule 5: The "Success Hint" Pattern (Error Handling)

• Every error includes path to success
• Suggest recovery tools from same tool group
• Format: "❌ Problem. 💡 Use toolName() to fix"
• Never raw "Not Found" without context

Rule 6: The "Zero TMI" Context Rule (Information Architecture)

• Never leak internal system architectures (e.g., Rust struct names, routing mechanisms) to the AI agent
• Keep context prompts, descriptions, and hints focused ONLY on what the agent needs to act
• Make service context prompts as static as possible; avoid injecting volatile data (like system-wide tool counts) to maximize LLM prompt caching

Rule 7: Outcome-Conditioned Augmentation (Guidance)

• Next-action hints must depend on post-call outcome class, not a fixed template
• Steady progress: Prefer empty/minimal hints when context already enables the next step
• Phase boundary: When state shows a natural milestone (queue drained, goal satisfied, batch finished), escalate to a consolidating/review action in the same domain
• Stuck / repeated failure: Escalate to diagnosis or strategy-reset instead of repeating the same recovery hint
• Intentional escalation is not bloat; unconditional "use toolX next" on every call is

---

Step 2: Gather Code Artifacts

Collect these files for analysis:

src-tauri/src/mcp/builtin/your_server/
├── mod.rs              # Server struct + trait impl
├── tools/*.rs          # Tool schema definitions
├── handlers/*.rs       # Tool execution handlers
└── types.rs            # Domain types (if any)

Critical Files to Examine:

1. Tool Schemas - Check for ID parameters in CREATE operations
2. Tool Handlers - Check for validation before mutations
3. Response Building - Check dual-channel compliance
4. Error Messages - Check for recovery hints

---

Step 3: Rule-by-Rule Audit Process

✅ Auditing Rule 1: Immutable ID Rule

What to Look For:

// ❌ VIOLATION: ID parameter in CREATE tool
pub fn create_resource_tool() -> MCPTool {
    let mut props = HashMap::new();
    props.insert(
        "id".to_string(),  // ❌ Agent can hallucinate this!
        string_prop(Some(1), Some(50), Some("Resource ID (optional)")),
    );
    // ...
}

// ✅ COMPLIANT: No ID parameter
pub fn create_resource_tool() -> MCPTool {
    let mut props = HashMap::new();
    props.insert(
        "name".to_string(),  // ✅ Only business data
        string_prop(Some(1), Some(100), Some("Resource name")),
    );
    // No "id" field at all
}

Audit Checklist:

• All CREATE tools have NO id parameter in schema
• UPDATE/DELETE tools have REQUIRED id parameter
• IDs are generated server-side (UUID/CUID/domain-specific)
• Generated IDs are returned in responses

Common False Positives:

• File paths are NOT system IDs (user-controlled, not DB PKs) ✅ OK
• Session IDs passed as options (not creation parameters) ✅ OK

---

⚠️ Auditing Rule 2: Hallucination Firewall

What to Look For:

// ❌ VIOLATION: Direct database access without validation
pub async fn handle_update_resource(args: Value) -> Result<MCPResult, String> {
    let args: UpdateArgs = serde_json::from_value(args)?;

    // ❌ No existence check - agent can hallucinate ID
    db.resources.update(&args.id, data).await?;

    Ok(success_result())
}

// ✅ COMPLIANT: Validation before mutation
pub async fn handle_update_resource(args: Value) -> Result<MCPResult, String> {
    let args: UpdateArgs = serde_json::from_value(args)?;

    // ✅ Hallucination firewall
    if !db.resources.exists(&args.id).await? {
        return Ok(operation_failed_error(
            "Update Resource",
            &format!("Resource '{}' not found", args.id),
            vec![
                "Use listResources() to find the correct ID".to_string(),
                "IDs are case-sensitive".to_string(),
            ],
            ToolGroup::YourServer
        ));
    }

    // Safe to proceed
    db.resources.update(&args.id, data).await?;
    Ok(success_result())
}

Audit Checklist:

• All ID-based operations validate existence FIRST
• Validation happens BEFORE database writes
• Invalid IDs return logic errors (not DB constraint errors)
• Error messages suggest how to find valid IDs

Common Pitfalls:

• Registry lookups that assume ID exists ❌
• Direct .get(id).unwrap() without checking ❌
• Generic not_found_error without context ⚠️ (functional but not ideal)

---

🔍 Auditing Rule 3: Dual-Channel Response (CRITICAL)

This is the most commonly violated rule. Check carefully!

What to Look For:

// ❌ VIOLATION: ID only in structured_content (AI can't see it!)
let result_text = "Process started successfully";
let data = json!({
    "process_id": process_id  // ❌ INVISIBLE to AI
});

MCPResult {
    content: vec![text(result_text)],  // ❌ No ID in text!
    structured_content: Some(data),
}

// ✅ COMPLIANT: ID in BOTH channels
let result_text = format!(
    "Process started successfully (ID: {}).\n\n\
     Use pollProcess(\"{}\") to check status",
    process_id,  // ✅ Visible to AI
    process_id
);
let data = json!({
    "process_id": process_id  // ✅ Also in JSON for UI
});

MCPResult {
    content: vec![text(result_text)],
    structured_content: Some(data),
}

Audit Process:

1. Find all response building code - Search for MCPResult, SuccessHint, to_mcp_result
2. Extract the text content - What does the AI actually see?
3. Check for critical IDs - Are process IDs, resource IDs, session IDs visible?
4. Verify in structured_content - Is the same data in JSON for UI?

Audit Checklist:

• All generated IDs appear in text content (AI-visible)
• Text content is self-sufficient (no dependency on JSON)
• Critical values repeated in structured_content for UI
• No orphaned IDs (only in JSON, not in text)

Testing Trick:

Read only the text field. Can an agent:
1. Know what happened?
2. Extract the ID for next operation?
3. Understand the current state?

If NO to any → Violation of Rule 3

---

📖 Auditing Rule 4: AI-Native Descriptions

What to Look For:

// ❌ VIOLATION: Human UI-centric language
description: "Click the resource to select it, then copy the ID and paste it into the update tool"

// ✅ COMPLIANT: AI-native workflow description
description: "Extract resource ID from listResources() output.
Use the ID as input to updateResource() for modifications.

WORKFLOW:
1. Call listResources() to view available resources
2. Identify target resource and extract its ID
3. Pass ID to updateResource(id, newData)

PREREQUISITE: Resource must exist (created via createResource)"

Audit Checklist:

• No human UI verbs: click, type, copy, paste, drag, select
• Uses data operation verbs: extract, use, pass, call, retrieve
• Prerequisites explicitly documented
• Workflow shows tool call sequences
• Examples demonstrate actual usage patterns

Red Flags:

• References to "UI", "button", "dialog", "form", "screen"
• Phrases like "enter your input", "click to confirm"
• Missing prerequisite tools in workflow

---

💡 Auditing Rule 5: Success Hint Pattern

What to Look For:

// ❌ VIOLATION: Raw error, no recovery path
.ok_or_else(|| format!("Process '{}' not found", process_id))?

// ⚠️ PARTIAL: Generic hints (not context-specific)
return Ok(not_found_error("Process", process_id, ToolGroup::Workspace));
// Uses default hints like "Use listDirectory" ❌ Wrong for processes

// ✅ COMPLIANT: Context-specific recovery hints
return Ok(operation_failed_error(
    "Poll Process",
    &format!("Process '{}' not found", process_id),
    vec![
        "Use listProcesses() to see all active processes".to_string(),
        "Process IDs are case-sensitive and must match exactly".to_string(),
        "Process may have finished - check with readProcessOutput()".to_string(),
    ],
    ToolGroup::Workspace
));

Audit Checklist:

• All errors include 2-3 actionable recovery steps
• Suggested tools are from same tool group
• Error format includes ✗ marker and 💡 hints
• No raw Err() returns without context
• Generic helpers used appropriately (or replaced with specific hints)

Common Issues:

• Using not_found_error helper for different resource types
• Missing tool suggestions for recovery
• Hints reference wrong tool group (browser hints for workspace errors)

---

🔕 Auditing Rule 6: The "Zero TMI" Context Rule

What to Look For:

// ❌ VIOLATION: Exposing internal Rust structs and volatile system state
let structured_state = json!({
    "mode": "session-isolated",
    "note": "External servers are managed per-session through MCPServiceProxy"
});

let hint = "Session-level attachment should also reference this Server ID because of the proxy hierarchy.";

// ✅ COMPLIANT: Simple, actionable, static context
let structured_state = json!({ "status": "ready" });

let hint = "To enable this server, add its Server ID to an agent using agent__update()";

Audit Checklist:

• Context prompts do not mention backend classes, structural abstractions, or implementation code names
• Tool descriptions use practical, actionable terms rather than system-level architectural jargon (e.g., avoid "platform/server inventory gating mechanism")
• Service configurations favor static text over dynamically injecting metrics like total tool counts (which harms prompt cache performance)
• Hints emphasize how to use the tool rather than explaining how the tool works inside

Common Issues:

• Explaining why a constraint exists by describing the backend architecture
• Injecting counts or timestamps into static context prompts just for the sake of completeness

---

📈 Auditing Rule 7: Outcome-Conditioned Augmentation

Hints should change with what just happened, not always advertise the same follow-up tool.

What to Look For:

// ❌ VIOLATION: Fixed success hint regardless of state
let hint = SuccessHint::new(
    format!("Updated item #{}", id),
    vec!["Use getState to see the list".to_string()], // always, even when list is already in context
);

// ❌ VIOLATION: Same recovery forever on repeated failure
return Ok(operation_failed_error(
    "Apply Patch",
    "Patch failed",
    vec!["Retry applyPatch with corrected anchors".to_string()], // never escalates
    ToolGroup::Workspace,
));

// ✅ COMPLIANT: Condition hints on outcome class
let next = match outcome {
    Outcome::Steady => vec![], // context already sufficient
    Outcome::PhaseComplete => vec![
        "Milestone reached — run the domain review/consolidate action before continuing."
            .to_string(),
    ],
    Outcome::RepeatedFailure => vec![
        "Same failure repeated — stop retrying; diagnose root cause or reset strategy."
            .to_string(),
    ],
};

Outcome classes (apply across any tool group):

Class	When	Hint posture
Steady progress	Ordinary success; agent can continue from context	Empty or one concrete fact (e.g. count), no tool promotion
Phase boundary	Natural milestone / drained queue / terminal batch	Escalate to consolidating or review action in-domain
Stuck / repeated failure	Identical error signature or N consecutive failures	Escalate to diagnosis / strategy-reset; do not restate the same retry

Audit Checklist:

• Success hints vary by post-call state (not a single always-on template)
• Steady-path successes avoid redundant "use list/get/read" promotions when context already has the data
• Phase-boundary successes escalate once to a higher-level in-domain action
• Repeated/identical failures escalate instead of looping the same recovery sentence
• Escalation targets stay actionable and domain-appropriate (not architectural lectures)

Common Issues:

• Treating every success like a tutorial (always promote sibling tools)
• Confusing Rule 7 escalation with hint bloat — fixed spam is bloat; state-gated escalation is not
• Error recovery that never changes after N identical failures

---

Step 4: Document Findings

Compliance Matrix Template

## Compliance Audit: [ServerName] Builtin Tools

| Rule                                | Status   | Grade | Evidence  |
| ----------------------------------- | -------- | ----- | --------- |
| 1. Immutable ID Rule                | ✅/⚠️/🔴 | A-F   | [Details] |
| 2. Hallucination Firewall           | ✅/⚠️/🔴 | A-F   | [Details] |
| 3. Dual-Channel Response            | ✅/⚠️/🔴 | A-F   | [Details] |
| 4. AI-Native Descriptions           | ✅/⚠️/🔴 | A-F   | [Details] |
| 5. Success Hint Pattern             | ✅/⚠️/🔴 | A-F   | [Details] |
| 6. Zero TMI Context Rule            | ✅/⚠️/🔴 | A-F   | [Details] |
| 7. Outcome-Conditioned Augmentation | ✅/⚠️/🔴 | A-F   | [Details] |

**Overall Grade:** [A-F] - [Summary]

Grading Rubric

A (Excellent): Fully compliant, exemplary implementation B (Good): Compliant with minor improvements possible C (Acceptable): Functional but has non-critical issues D (Needs Improvement): Has violations that impact UX F (Critical Issues): Blocking issues, not production-ready

---

Step 5: Provide Constructive Feedback

Feedback Template

### [Priority Level] [Rule Name] - [Title]

**Problem:** [Clear description of what's wrong]

**Location:** `file.rs` lines X-Y

**Current Code:**

```rust
// Show the problematic code
```

Issue: [Why this violates the manifesto]

Recommended Fix:

// Show the corrected code

Impact: [How this affects AI agents]

Priority: P0 (Blocker) / P1 (High) / P2 (Medium) / P3 (Low)

### Priority Guidelines

**P0 (Blocker):** Must fix before production
- Rule 3 violations (IDs invisible to AI)
- Rule 1 violations (ID input on create)
- Missing validation causing crashes

**P1 (High):** Should fix soon
- Rule 2 partial violations (poor error messages)
- Rule 5 violations (no recovery hints)
- Rule 7 violations (fixed retry loops / missing phase-boundary escalation)

**P2 (Medium):** Nice to have
- Rule 4 improvements (better descriptions)
- Inconsistent error formatting
- Rule 7 steady-path hint noise (redundant list/get promotions)

**P3 (Low):** Optional polish
- Documentation improvements
- Code organization

---

## Real-World Example: Workspace Tools Audit

### Initial Assessment (Incorrect)

```markdown
## 📊 Compliance Score

| Rule | Grade | Issue |
|------|-------|-------|
| 3. Dual-Channel Response | D 🔴 | Process IDs invisible to AI |

**Finding:** Process IDs only in structured_content, agents can't see them.

**Evidence:** Assumed text content didn't include IDs based on quick scan.

Corrected Assessment (After Deep Analysis)

## 📊 Compliance Score (CORRECTED)

| Rule                     | Grade | Evidence                        |
| ------------------------ | ----- | ------------------------------- |
| 3. Dual-Channel Response | A ✅  | Process IDs ARE in text content |

**Finding:** IDs are properly visible in BOTH channels.

**Evidence from code (lines 1075-1090):**

```rust
let hint = SuccessHint::new(
    format!(
        "Background process started successfully

• Process ID: {}  // ✅ VISIBLE
• Command: {}

💡 Next Steps:
Use pollProcess(\"{}\") to check status",
        process_id, command, process_id
    ),
    // ...
);
```

Lesson: Always verify by reading actual response text, not just scanning for patterns.

---

## Common Audit Mistakes to Avoid

### ❌ Mistake 1: Scanning Instead of Reading

**Wrong Approach:**

Search for "process_id" in structured_content → Found! Assume it's not in text → Mark as violation ❌

**Right Approach:**

1. Find response building code
2. Extract literal text content
3. Read what AI actually sees
4. Then check structured_content
5. Verify both channels have critical data ✅

### ❌ Mistake 2: Assuming Generic Helpers Are Wrong

**Wrong Assumption:**

Code uses not_found_error() → Must be bad ❌

**Right Analysis:**

1. Check what not_found_error() returns
2. Read the default hints for this tool group
3. Verify hints match the resource type
4. If hints are generic → Suggest improvement ⚠️
5. If hints are wrong → Mark as issue 🔴

### ❌ Mistake 3: Missing Context

**Wrong Critique:**

"File operations lack validation" ❌ (Actually, they use SecureFileManager with validation)

**Right Critique:**

1. Trace validation through call stack
2. Check if validation helper exists
3. Verify validation catches edge cases
4. Only flag if genuinely missing ✅

---

## Validation Checklist

Before submitting audit findings:

- [ ] Verified code by reading actual implementations (not assumptions)
- [ ] Checked if violations are real or helper-abstracted
- [ ] Provided specific line numbers for issues
- [ ] Included code examples for both violation and fix
- [ ] Graded each rule independently
- [ ] Assigned appropriate priorities
- [ ] Tested recommended fixes compile (if providing code)
- [ ] Acknowledged what's already good (not just problems)

---

## Audit Report Template

```markdown
# [ServerName] Builtin Tools Audit Report

**Auditor:** [Name/System]
**Date:** [YYYY-MM-DD]
**Version:** [Code version/commit]

## Executive Summary

[Overall grade and key findings in 2-3 sentences]

## Detailed Analysis

### Rule 1: Immutable ID Rule - [Grade]

**Status:** ✅ Compliant / ⚠️ Partial / 🔴 Violation

**Findings:**
- [Finding 1]
- [Finding 2]

**Evidence:**
```rust
// Code examples

[Repeat for Rules 2-6]

Priority Fixes

P0 (Blocker)

1. [Issue title] - [File location]
   • Impact: [Description]
   • Fix: [Code example]

P1 (High)

[Similar format]

P2 (Medium)

[Similar format]

What's Already Good

• ✅ [Strength 1]
• ✅ [Strength 2]
• ✅ [Strength 3]

Recommendations

1. [Recommendation 1]
2. [Recommendation 2]

Conclusion

[Final assessment and next steps]

---

## Testing Your Audit

### Self-Validation Questions

1. **Did I read actual code or make assumptions?**
   - ✅ Traced through implementation
   - ❌ Assumed based on patterns

2. **Are my examples accurate?**
   - ✅ Copy-pasted from source
   - ❌ Paraphrased or invented

3. **Did I verify fixes would work?**
   - ✅ Tested or checked against working examples
   - ❌ Suggested theoretical fixes

4. **Am I being fair?**
   - ✅ Acknowledged what's good
   - ❌ Only listed problems

5. **Is my feedback actionable?**
   - ✅ Specific files, lines, and code examples
   - ❌ Vague "improve error handling"

---

## Advanced: Automated Audit Patterns

### Grep Patterns for Quick Scan

```bash
# Rule 1: Find CREATE tools with ID parameters
rg -A 10 'fn create_.*_tool' | rg 'props.insert.*"id"'

# Rule 2: Find direct database access without validation
rg 'db\.\w+\.(update|delete|insert)' | rg -v 'if.*exists'

# Rule 3: Find response building
rg 'MCPResult|SuccessHint::new|to_mcp_result'

# Rule 5: Find raw Err returns
rg 'Err\(format!\(".*not found'

Code Review Checklist

When reviewing PR:

• New CREATE tools have no ID parameter
• UPDATE/DELETE operations validate before mutation
• Response text includes all critical IDs
• Error messages suggest recovery tools
• Tool descriptions are AI-native and avoid architectural jargon
• No human UI verbs in descriptions
• Success/error hints are context-specific and actionable (Zero TMI)
• Outcome-conditioned augmentation: steady path stays lean; milestones escalate; repeated failures escalate (not same retry forever)
• Shared context prompts are static to maximize LLM cache performance
• Structured content mirrors text content

---

Summary

Key Principles for Effective Audits:

1. Read Code, Don't Assume - Verify by tracing actual execution
2. Context Matters - Generic helpers may be appropriate
3. Be Specific - Provide exact locations and examples
4. Be Balanced - Acknowledge strengths and weaknesses
5. Be Actionable - Give clear fixes, not vague suggestions
6. Validate Yourself - Double-check before publishing findings

Common Audit Flow:

1. Gather code artifacts
2. Apply rule-by-rule checklist
3. Document findings with evidence
4. Prioritize issues (P0-P3)
5. Provide specific fixes
6. Validate audit accuracy
7. Submit constructive feedback

When in Doubt:

• Read the actual code path
• Check if helpers abstract the pattern correctly
• Look for working examples in the codebase
• Verify your recommended fix would compile
• Ask for second review if unsure

The goal is constructive improvement, not finding fault. Good audits help teams build better AI agent tools!