fix: tool reliability improvements for sql-classify, edit, and webfetch#582
fix: tool reliability improvements for sql-classify, edit, and webfetch#582anandgupta42 merged 1 commit intomainfrom
Conversation
…ch (#581) **sql-classify.ts:** - Fix `computeSqlFingerprint` referencing undefined `core` variable after safe-import refactor — extract `extractMetadata` as module-level guard - Invert fallback classifier to whitelist reads (`READ_PATTERN`) instead of blacklisting writes — treats unknown statements as "write" for safety - Handle multi-statement SQL in fallback by splitting on semicolons - Strip `--` line comments in fallback (block comments already stripped) - Fix `HARD_DENY_PATTERN` trailing `\s` → `\b` to match `TRUNCATE;` **edit.ts:** - Add `buildNotFoundMessage` with Levenshtein nearest-match snippets for LLM self-correction when `oldString` not found - Fix substring matching to prefer exact equality over short-line matches **webfetch.ts:** - Add session-level URL failure cache (404/410/451) with 5-min TTL - Add `buildFetchError` with actionable status-specific error messages - Add `sanitizeUrl` to strip query strings from error messages - Add URL validation via `new URL()` constructor - Add `MAX_CACHED_URLS = 500` size cap with oldest-entry eviction **Tests:** 12 new tests for `buildNotFoundMessage`, `replace` error messages, `computeSqlFingerprint`, and updated webfetch assertions. Closes #581 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughThis PR addresses reliability issues across three tools by adding safe imports with fallback logic to sql-classify, implementing error message self-correction in edit, and introducing URL failure caching with actionable error messages in webfetch. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebFetch as WebFetch Tool
participant Cache as URL Failure Cache
participant Server as HTTP Server
Client->>WebFetch: fetch(url)
WebFetch->>WebFetch: validate URL
WebFetch->>Cache: check if url failed before?
alt Cached failure (404/410/451)
Cache->>WebFetch: return cached error
WebFetch->>Client: throw error immediately
else Not in cache
WebFetch->>Server: make HTTP request
Server->>WebFetch: response (status, body)
alt Success (200-299)
WebFetch->>Client: return response
else Error (404/410/451/403/429/500)
WebFetch->>Cache: cache failure + TTL
WebFetch->>WebFetch: build status-specific error message
WebFetch->>Client: throw actionable error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…refactor The failure isolation tests monkey-patched `core.getStatementTypes` and `core.extractMetadata` on the require'd module object, but #582 changed `computeSqlFingerprint` to use module-level variable copies captured at import time. Monkey-patching `core` no longer affects the function. Replace monkey-patch tests with input-driven resilience tests that exercise error paths naturally (malformed SQL, empty strings, edge cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ch (#581) (#582) **sql-classify.ts:** - Fix `computeSqlFingerprint` referencing undefined `core` variable after safe-import refactor — extract `extractMetadata` as module-level guard - Invert fallback classifier to whitelist reads (`READ_PATTERN`) instead of blacklisting writes — treats unknown statements as "write" for safety - Handle multi-statement SQL in fallback by splitting on semicolons - Strip `--` line comments in fallback (block comments already stripped) - Fix `HARD_DENY_PATTERN` trailing `\s` → `\b` to match `TRUNCATE;` **edit.ts:** - Add `buildNotFoundMessage` with Levenshtein nearest-match snippets for LLM self-correction when `oldString` not found - Fix substring matching to prefer exact equality over short-line matches **webfetch.ts:** - Add session-level URL failure cache (404/410/451) with 5-min TTL - Add `buildFetchError` with actionable status-specific error messages - Add `sanitizeUrl` to strip query strings from error messages - Add URL validation via `new URL()` constructor - Add `MAX_CACHED_URLS = 500` size cap with oldest-entry eviction **Tests:** 12 new tests for `buildNotFoundMessage`, `replace` error messages, `computeSqlFingerprint`, and updated webfetch assertions. Closes #581 Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…refactor The failure isolation tests monkey-patched `core.getStatementTypes` and `core.extractMetadata` on the require'd module object, but #582 changed `computeSqlFingerprint` to use module-level variable copies captured at import time. Monkey-patching `core` no longer affects the function. Replace monkey-patch tests with input-driven resilience tests that exercise error paths naturally (malformed SQL, empty strings, edge cases). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
What does this PR do?
Fixes 8 tool reliability issues found via 6-model consensus code review across sql-classify, edit, and webfetch tools.
sql-classify.ts:
computeSqlFingerprintreferencing undefinedcorevariable after safe-import refactor--line comments in fallbackHARD_DENY_PATTERNtrailing\s→\bedit.ts:
buildNotFoundMessagewith Levenshtein nearest-match snippets for LLM self-correctionwebfetch.ts:
buildFetchErrorsanitizeUrlto strip query strings from error messages (prevents token leakage)new URL()constructorType of change
Issue for this PR
Closes #581
How did you verify your code works?
Checklist
Summary by CodeRabbit
Bug Fixes
New Features