Skip to content

fix(ENG-12594): remove per-tool sanitization rules#42

Merged
hiskudin merged 4 commits intomainfrom
refactor/remove-tool-rules
Apr 8, 2026
Merged

fix(ENG-12594): remove per-tool sanitization rules#42
hiskudin merged 4 commits intomainfrom
refactor/remove-tool-rules

Conversation

@hiskudin
Copy link
Copy Markdown
Collaborator

@hiskudin hiskudin commented Apr 8, 2026

Summary

  • Removes the ToolSanitizationRule system entirely — per-tool rules were redundant with the existing DEFAULT_RISKY_FIELDS.toolOverrides which already handles per-tool field selection
  • All tools now treated uniformly with defaultRiskLevel: "medium" and risk escalation driven by Tier 1 patterns + encoding detection
  • Net deletion of 286 lines of dead/redundant code

Why

What tool rules provided What already exists
skipFields (skip id, url, etc.) DEFAULT_RISKY_FIELDS allowlist already excludes non-risky fields
maxFieldLengths Dead code — getMaxFieldLength() was never called
riskyFields override Dead code — never used in implementation
sanitizationLevel (e.g. email → high) defaultRiskLevel: "medium" for all; encoding escalation handles encoded payloads dynamically
cumulativeRiskThresholds per tool Global defaults are sufficient (medium: 3, high: 1, patterns: 3)

What was removed

  • ToolSanitizationRule interface and public type export
  • DEFAULT_TOOL_RULES constant (6 per-tool rule objects)
  • getToolRule(), shouldSkipField(), getMaxFieldLength() utilities
  • useDefaultToolRules option from PromptDefenseOptions
  • toolRule parameter threaded through all recursive sanitization methods
  • toolRules from PromptDefenseConfig and ToolResultSanitizerConfig
  • 5 tool-rule-specific integration/unit tests

What stays (covers the same functionality)

  • DEFAULT_RISKY_FIELDS.toolOverrides — per-tool field selection (e.g. gmail_*[subject, body, snippet])
  • DEFAULT_TRAVERSAL_CONFIGmaxDepth: 10, maxSize: 10MB
  • defaultRiskLevel: "medium" — uniform base risk for all tools
  • Global cumulative risk thresholds

Test plan

  • npm test — 181/181 passing (down from 186; 5 removed tests were tool-rule-specific)
  • npm run lint — clean
  • npm run build — clean
  • All existing Tier 1, Tier 2, sanitizer, and integration tests pass unchanged

🤖 Generated with Claude Code


Summary by cubic

Removed per‑tool sanitization rules (ENG-12594) and unified all tools under a medium base risk. Escalation now comes only from Tier 1 patterns, encoding checks, and Tier 2 scoring; per‑tool field scanning stays via DEFAULT_RISKY_FIELDS.toolOverrides.

  • Refactors

    • Deleted the ToolSanitizationRule system and all related code (DEFAULT_TOOL_RULES, config toolRules, useDefaultToolRules, and utils like getToolRule/shouldSkipField); removed its public type export.
    • Simplified sanitizer internals to drop rule threading and tool‑specific thresholds; cumulative risk tracker now explicitly uses global thresholds (comment updated).
    • Kept DEFAULT_RISKY_FIELDS.toolOverrides for per‑tool scanned fields.
    • Updated README to remove tool rules references, add “Risky Field Detection,” and clarify riskLevel starts at medium and only escalates via detections.
    • Removed 6 tool‑rule tests.
  • Migration

    • Remove all imports/usages of ToolSanitizationRule.
    • Delete PromptDefenseOptions.useDefaultToolRules and any config toolRules references.
    • Construct PromptDefense without tool rules; rely on defaultRiskLevel: 'medium' and detection‑based escalation.
    • Email tools no longer default to high base risk; blocking depends on detections and global thresholds.

Written for commit 38d52f1. Summary will update on new commits.

Tool rules (ToolSanitizationRule) were redundant with the existing
DEFAULT_RISKY_FIELDS system which already handles per-tool field
selection via toolOverrides (e.g. gmail_* → [subject, body, snippet]).

What tool rules provided vs what already exists:
- skipFields → redundant: risky fields allowlist already excludes non-risky
  fields like id, url, created_at
- maxFieldLengths → dead code: getMaxFieldLength() was never called
- riskyFields override → dead code: never used in implementation
- sanitizationLevel → only email used "high", everything else was "medium"
  (same as defaultRiskLevel). Encoding-based risk escalation handles the
  email case via containsSuspiciousEncoding.
- cumulativeRiskThresholds → minor tuning, global defaults are sufficient

Removed:
- ToolSanitizationRule interface (types.ts)
- DEFAULT_TOOL_RULES constant (config.ts)
- toolRules from PromptDefenseConfig and ToolResultSanitizerConfig
- getToolRule, shouldSkipField, getMaxFieldLength (field-detection.ts)
- useDefaultToolRules option (prompt-defense.ts)
- ToolSanitizationRule export from public API (index.ts)
- toolRule parameter threaded through all recursive sanitization methods
- 5 tool-rule-specific tests

All tools now treated uniformly with defaultRiskLevel: "medium" and
risk escalation driven by Tier 1 patterns + encoding detection.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@hiskudin hiskudin requested a review from a team as a code owner April 8, 2026 10:20
Copilot AI review requested due to automatic review settings April 8, 2026 10:20
@hiskudin hiskudin changed the title refactor: remove per-tool sanitization rules fix(ENG-12594): remove per-tool sanitization rules Apr 8, 2026
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Requires human review: This refactor removes a core configuration system and changes default risk behaviors for specific tools, which impacts security logic and requires human review.

- Remove useDefaultToolRules from Quick Start, API reference, and
  Vercel AI SDK integration examples
- Remove per-tool base risk level table (gmail_* → high, etc.)
- Replace "Tool-Specific Rules" section with "Risky Field Detection"
  section documenting which fields are scanned per tool (this uses
  DEFAULT_RISKY_FIELDS.toolOverrides which is still active)
- Simplify riskLevel explanation — starts at medium, escalated by
  detections

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
OMauriStkOne
OMauriStkOne previously approved these changes Apr 8, 2026
Copy link
Copy Markdown

@OMauriStkOne OMauriStkOne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 8 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="specs/integration.spec.ts">

<violation number="1" location="specs/integration.spec.ts:289">
P2: The new test does not assert the intended default `medium` risk level; it only excludes `high`/`critical`, so a regression to `low` would go undetected.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread specs/integration.spec.ts Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 1 file (changes from recent commits).

Requires human review: Auto-approval blocked by 1 unresolved issue from previous reviews.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Removes the dedicated per-tool sanitization rule system and simplifies sanitization so all tools share a uniform default risk level, relying on risky-field selection via DEFAULT_RISKY_FIELDS.toolOverrides plus Tier 1 detections for escalation.

Changes:

  • Deleted ToolSanitizationRule types/utilities and default per-tool rule config (DEFAULT_TOOL_RULES)
  • Simplified ToolResultSanitizer to remove tool-rule threading and apply uniform defaults
  • Updated/removed tests that asserted tool-rule behavior

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/utils/field-detection.ts Removes tool-rule helper APIs (getToolRule, shouldSkipField, getMaxFieldLength) leaving risky-field detection + wildcard matching.
src/types.ts Removes ToolSanitizationRule and toolRules from public configuration types.
src/index.ts Stops exporting ToolSanitizationRule from the package entrypoint.
src/core/tool-result-sanitizer.ts Removes tool-specific rules, sets uniform base risk, and uses only global cumulative-risk thresholds.
src/core/prompt-defense.ts Drops useDefaultToolRules option and stops passing tool rules into the sanitizer.
src/config.ts Removes DEFAULT_TOOL_RULES and eliminates toolRules from default config creation.
specs/utils.spec.ts Removes getToolRule tests and related imports.
specs/integration.spec.ts Removes integration coverage for useDefaultToolRules/custom tool rules and adds a uniform-default-risk assertion.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/core/tool-result-sanitizer.ts
Comment thread src/core/tool-result-sanitizer.ts Outdated
Comment thread src/core/prompt-defense.ts
Comment thread src/core/tool-result-sanitizer.ts
- Remove defendToolResult test that only asserted default medium risk
  level — redundant with existing integration tests, no longer
  tool-rule-specific
- Improve createCumulativeRiskTracker docstring per review feedback

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 issues found across 2 files (changes from recent commits).

Requires human review: This PR is a major refactor of core security logic, changing how tool risk is calculated and removing parts of the public API. It requires human review to ensure no security regression.

@hiskudin hiskudin merged commit eb4664b into main Apr 8, 2026
5 checks passed
@hiskudin hiskudin deleted the refactor/remove-tool-rules branch April 8, 2026 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants