fix: improve tool reliability — sql_execute crash, edit context drift, webfetch failures#474
Open
anandgupta42 wants to merge 3 commits intomainfrom
Open
fix: improve tool reliability — sql_execute crash, edit context drift, webfetch failures#474anandgupta42 wants to merge 3 commits intomainfrom
anandgupta42 wants to merge 3 commits intomainfrom
Conversation
**#469 — `sql_execute` crash: `getStatementTypes is not a function`** - Safe-import `@altimateai/altimate-core` with `typeof` guard - Regex fallback classifier when napi binary unavailable - Null/undefined input guards on `classify()` and `classifyAndCheck()` - 42 adversarial tests (fallback parity, ReDoS, bypass attempts) **#470 — `edit` tool: "Could not find oldString" context drift** - `buildNotFoundMessage()` finds closest-matching line via Levenshtein - Error now shows line number + 5-line snippet of actual file content - Tells model to re-read the file instead of retrying blindly - 14 adversarial tests (similarity scoring, truncation, edge cases) **#471 — `webfetch`: 934 daily failures from invalid/broken URLs** - `URL` constructor validation before fetch (catches malformed URLs) - Session-level 404/410/451 failure cache with 5-min TTL - Actionable error messages per HTTP status (404: "Do NOT retry", 429: includes `Retry-After`, 500: "transient — retry once") - 24 tests (validation, cache TTL, error messages, edge cases) **#473 — Bump `yaml` 2.8.2 → 2.8.3** - Stack overflow fix during node composition (security) Closes #469, Closes #470, Closes #471, Closes #473 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughSQL classification now guards native N-API usage and falls back to a regex classifier when unavailable; edit tooling reports richer "not found" diagnostics via a new helper; webfetch adds URL validation, status-aware errors, and a session-scoped failure cache; multiple tests and a pre-release NAPI export check were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebFetch as WebFetchModule
participant Cache as FailureCache
participant Remote as RemoteServer
Client->>WebFetch: request(url)
WebFetch->>Cache: check(url)
alt cached permanent failure
Cache-->>WebFetch: cached error
WebFetch-->>Client: throw cached error
else not cached
WebFetch->>Remote: fetch(url)
Remote-->>WebFetch: response(status, headers, body)
alt response.ok === false
WebFetch->>Cache: record(url) if status in (404,410,451)
WebFetch-->>Client: throw status-aware error (may include Retry-After)
else response.ok
WebFetch-->>Client: return body
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/opencode/test/tool/webfetch-validation.test.ts (2)
11-12: Risk of test drift from duplicated logic.Replicating private functions means tests may pass while actual implementation diverges. Consider either:
- Exporting the helpers (even if marked
@internal)- Adding integration tests that exercise the actual tool with mocked HTTP responses
The current approach is acceptable for initial coverage, but keep this drift risk in mind during future changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/webfetch-validation.test.ts` around lines 11 - 12, The test duplicates private helper logic which risks drift; fix by either exporting the internal helper functions used by the webfetch validation (make the helpers exportable—mark with `@internal` if desired) so the test in webfetch-validation.test.ts can import and assert real behavior, or replace/augment the unit-style duplication with an integration test that invokes the actual webfetch tool (the webfetch entry/function used by the tool) against mocked HTTP responses to validate behavior end-to-end; update imports in the test to use the exported helpers or call the real webfetch function accordingly.
146-149: Shared cache across tests risks flaky behavior.The
cacheMap is shared across all tests in this describe block without cleanup. While unique URLs prevent immediate collisions, tests that modify the cache (lines 167-207) accumulate state. Consider clearing the cache before each test for isolation.Suggested fix
describe("URL failure cache", () => { // Replicate cache logic for testing const cache = new Map<string, { status: number; timestamp: number }>() const TTL = 5 * 60 * 1000 + + // Clear cache before each test for isolation + beforeEach(() => { + cache.clear() + })Also add the import:
-import { describe, test, expect } from "bun:test" +import { describe, test, expect, beforeEach } from "bun:test"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/tool/webfetch-validation.test.ts` around lines 146 - 149, The shared Map named cache (and its TTL constant) in the describe block "URL failure cache" causes cross-test state leakage; add a test isolation step to clear/reset the cache before each test (e.g., a beforeEach that calls cache.clear()) so tests modifying cache entries (the tests around lines that update the cache) start with a clean map; locate the Map by the identifier cache and the describe block name to implement the cleanup.packages/opencode/src/tool/webfetch.ts (1)
21-22: Consider adding a cache size limit to prevent unbounded memory growth.The cache only cleans up entries lazily (on access after TTL expires). In a long-running process fetching many unique failing URLs, the map could grow indefinitely. Consider adding a max size with LRU eviction, or periodic cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/tool/webfetch.ts` around lines 21 - 22, The failure cache (failedUrls) can grow unbounded because entries are only expired lazily using FAILURE_CACHE_TTL; update the logic to enforce a max size and evict least-recently-used entries (or run periodic cleanup): introduce a MAX_FAILURE_CACHE_SIZE constant and either replace Map failedUrls with an LRU structure or maintain access-order by touching entries on read and evicting oldest when size > MAX_FAILURE_CACHE_SIZE, and/or add a periodic timer that purges entries older than FAILURE_CACHE_TTL; ensure you update all usages of failedUrls (look for references to failedUrls and FAILURE_CACHE_TTL) to use the new eviction/cleanup behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 28-29: The HARD_DENY_PATTERN check is only applied to the start of
the normalized SQL and normalization currently only strips block comments, so
inputs with leading line comments or multiple statements (e.g. "-- note\nDROP
DATABASE..." or "SELECT 1; DROP DATABASE ...") bypass the deny. Update the
normalization in sql-classify.ts to remove leading/trailing whitespace, strip
both block (/* */) and line (--) comments, then split the input into individual
statements by semicolon (or conservatively detect multiple statements) and apply
HARD_DENY_PATTERN to every trimmed statement; alternatively, reject any input
containing multiple statements outright. Also ensure the code path used by
sql-execute.ts that relies on the blocked flag uses this tightened
classification.
- Around line 25-27: The current fallback uses WRITE_PATTERN and defaults
anything not matching that small write-prefix list to "read", which
under-prompts ambiguous statements; change the fallback to use a positive read
allowlist instead and treat everything else as "write". Specifically, replace or
augment the WRITE_PATTERN usage with a READ_PATTERN that only matches clearly
read-only statements (e.g., SELECT, WITH ... SELECT, EXPLAIN,
DESCRIBE/SHOW/PRAGMA/SET variants you deem safe) and update the fallback logic
(the code that currently returns "read" when WRITE_PATTERN fails) to return
"write" by default unless READ_PATTERN matches; keep the WRITE_PATTERN checks
for explicit write-prefixes
(INSERT/UPDATE/DELETE/MERGE/CREATE/ALTER/DROP/TRUNCATE/GRANT/REVOKE/CALL/EXEC)
to still classify obvious writes.
In `@packages/opencode/src/tool/edit.ts`:
- Around line 639-642: The code currently uses the literal first line of
oldString which treats inputs like "\n\nactual text" as empty; update the logic
that sets firstLine in edit.ts to scan oldString's lines and pick the first
non-empty trimmed line (e.g., split by "\n", trim each, find first with length >
0) and only fall back to the empty/whitespace warning if no non-empty line
exists; adjust the subsequent check that returns base + " The oldString appears
to be empty or whitespace-only." to use this new first non-empty trimmed line so
nearest-match hints work for strings with leading blank lines.
In `@packages/opencode/src/tool/webfetch.ts`:
- Around line 50-54: The 429 handling currently appends "s" to the Retry-After
header unconditionally; change the logic in the case 429 block (variables
retryAfter and wait in packages/opencode/src/tool/webfetch.ts) to detect whether
headers?.get("retry-after") is a numeric-second value or a HTTP-date string: if
it's a positive integer use " (retry after Xs)", if it's a non-numeric/date
string include it verbatim like " (retry after <value>)" or omit the unit, and
ensure the message remains valid by not appending "s" to date values; keep the
rest of the message about the hostname and suggested actions intact.
- Around line 18-22: The comment for the failedUrls cache incorrectly states it
is session-level, but it is actually process-level because it is a module-scoped
Map reused across all sessions. To fix this, update the comment above the
failedUrls declaration to accurately describe it as a process-level URL failure
cache that persists for the process lifetime and is cleared only when the
process restarts. Remove or revise any language implying it is scoped per user
session.
In `@packages/opencode/test/altimate/tools/sql-classify-fallback.test.ts`:
- Around line 68-83: Test reimplements WRITE_PATTERN/HARD_DENY_PATTERN and
classifyFallback locally, so it doesn't exercise the module's real fallback
branch; instead import the actual fallback patterns or force a real import
failure so the production fallback is exercised. Update the test to either (a)
import the exported fallback regexes/utility from sql-classify.ts (referencing
the module's exported symbols like WRITE_PATTERN, HARD_DENY_PATTERN or a helper
such as getFallbackPatterns/classifyFallbackFromModule) and use those in
assertions, or (b) simulate a real N-API load failure by mocking the
require/import of '@altimateai/altimate-core' to throw (using your test
framework's module-mocking API) before importing sql-classify.ts so the module's
fallback path runs and the tests assert against the module's actual fallback
implementation.
---
Nitpick comments:
In `@packages/opencode/src/tool/webfetch.ts`:
- Around line 21-22: The failure cache (failedUrls) can grow unbounded because
entries are only expired lazily using FAILURE_CACHE_TTL; update the logic to
enforce a max size and evict least-recently-used entries (or run periodic
cleanup): introduce a MAX_FAILURE_CACHE_SIZE constant and either replace Map
failedUrls with an LRU structure or maintain access-order by touching entries on
read and evicting oldest when size > MAX_FAILURE_CACHE_SIZE, and/or add a
periodic timer that purges entries older than FAILURE_CACHE_TTL; ensure you
update all usages of failedUrls (look for references to failedUrls and
FAILURE_CACHE_TTL) to use the new eviction/cleanup behavior.
In `@packages/opencode/test/tool/webfetch-validation.test.ts`:
- Around line 11-12: The test duplicates private helper logic which risks drift;
fix by either exporting the internal helper functions used by the webfetch
validation (make the helpers exportable—mark with `@internal` if desired) so the
test in webfetch-validation.test.ts can import and assert real behavior, or
replace/augment the unit-style duplication with an integration test that invokes
the actual webfetch tool (the webfetch entry/function used by the tool) against
mocked HTTP responses to validate behavior end-to-end; update imports in the
test to use the exported helpers or call the real webfetch function accordingly.
- Around line 146-149: The shared Map named cache (and its TTL constant) in the
describe block "URL failure cache" causes cross-test state leakage; add a test
isolation step to clear/reset the cache before each test (e.g., a beforeEach
that calls cache.clear()) so tests modifying cache entries (the tests around
lines that update the cache) start with a clean map; locate the Map by the
identifier cache and the describe block name to implement the cleanup.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 300a36f9-8f7a-4c67-a29e-8aa9cfc70efd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
packages/opencode/src/altimate/tools/sql-classify.tspackages/opencode/src/tool/edit.tspackages/opencode/src/tool/webfetch.tspackages/opencode/test/altimate/tools/sql-classify-fallback.test.tspackages/opencode/test/tool/edit-error-context.test.tspackages/opencode/test/tool/webfetch-validation.test.ts
Comment on lines
+25
to
+27
| // Regex fallback: patterns that indicate write operations (case-insensitive, anchored to statement start) | ||
| const WRITE_PATTERN = | ||
| /^\s*(INSERT|UPDATE|DELETE|MERGE|CREATE|ALTER|DROP|TRUNCATE|GRANT|REVOKE|CALL|EXEC)\b/i |
There was a problem hiding this comment.
Fallback now under-prompts on ambiguous SQL.
This branch returns "read" for anything outside the small write-prefix list, so fallback mode will silently skip the sql_execute_write prompt for statements the comment above explicitly calls ambiguous, like SHOW, SET, and USE, plus forms like WITH ... DELETE. Since this is the degraded path, false positives are safer than false negatives here — please switch to a positive read allowlist and default everything else to "write".
Minimal safe direction
-const WRITE_PATTERN =
- /^\s*(INSERT|UPDATE|DELETE|MERGE|CREATE|ALTER|DROP|TRUNCATE|GRANT|REVOKE|CALL|EXEC)\b/i
+const SAFE_READ_PATTERN = /^\s*SELECT\b/i
...
- const queryType = WRITE_PATTERN.test(trimmed) ? "write" : "read"
+ const queryType = SAFE_READ_PATTERN.test(trimmed) ? "read" : "write"Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 25 - 27,
The current fallback uses WRITE_PATTERN and defaults anything not matching that
small write-prefix list to "read", which under-prompts ambiguous statements;
change the fallback to use a positive read allowlist instead and treat
everything else as "write". Specifically, replace or augment the WRITE_PATTERN
usage with a READ_PATTERN that only matches clearly read-only statements (e.g.,
SELECT, WITH ... SELECT, EXPLAIN, DESCRIBE/SHOW/PRAGMA/SET variants you deem
safe) and update the fallback logic (the code that currently returns "read" when
WRITE_PATTERN fails) to return "write" by default unless READ_PATTERN matches;
keep the WRITE_PATTERN checks for explicit write-prefixes
(INSERT/UPDATE/DELETE/MERGE/CREATE/ALTER/DROP/TRUNCATE/GRANT/REVOKE/CALL/EXEC)
to still classify obvious writes.
Comment on lines
+28
to
+29
| const HARD_DENY_PATTERN = | ||
| /^\s*(DROP\s+(DATABASE|SCHEMA)\b|TRUNCATE(\s+TABLE)?\s)/i |
There was a problem hiding this comment.
Fallback hard-deny is still bypassable.
HARD_DENY_PATTERN is only checked against the start of the normalized string, and the normalization step only removes block comments. In degraded mode, inputs like -- note\nDROP DATABASE prod or SELECT 1; DROP DATABASE prod still return blocked: false. packages/opencode/src/altimate/tools/sql-execute.ts:16-30 relies on this flag to stop execution, so the fallback needs to strip leading line comments and inspect every apparent statement (or conservatively reject multi-statement input) before returning.
Also applies to: 36-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/altimate/tools/sql-classify.ts` around lines 28 - 29,
The HARD_DENY_PATTERN check is only applied to the start of the normalized SQL
and normalization currently only strips block comments, so inputs with leading
line comments or multiple statements (e.g. "-- note\nDROP DATABASE..." or
"SELECT 1; DROP DATABASE ...") bypass the deny. Update the normalization in
sql-classify.ts to remove leading/trailing whitespace, strip both block (/* */)
and line (--) comments, then split the input into individual statements by
semicolon (or conservatively detect multiple statements) and apply
HARD_DENY_PATTERN to every trimmed statement; alternatively, reject any input
containing multiple statements outright. Also ensure the code path used by
sql-execute.ts that relies on the blocked flag uses this tightened
classification.
Comment on lines
+639
to
+642
| // Find the first line of oldString and search for it in the file | ||
| const firstLine = oldString.split("\n")[0].trim() | ||
| if (!firstLine) return base + " The oldString appears to be empty or whitespace-only." | ||
|
|
There was a problem hiding this comment.
Use the first non-empty line from oldString, not the literal first line.
This currently misclassifies inputs like "\n\nactual text" as whitespace-only and skips nearest-match hints. Please scan for the first non-empty trimmed line before fallback.
Proposed fix
- const firstLine = oldString.split("\n")[0].trim()
+ const firstLine = oldString
+ .split("\n")
+ .map((line) => line.trim())
+ .find((line) => line.length > 0) ?? ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/tool/edit.ts` around lines 639 - 642, The code
currently uses the literal first line of oldString which treats inputs like
"\n\nactual text" as empty; update the logic that sets firstLine in edit.ts to
scan oldString's lines and pick the first non-empty trimmed line (e.g., split by
"\n", trim each, find first with length > 0) and only fall back to the
empty/whitespace warning if no non-empty line exists; adjust the subsequent
check that returns base + " The oldString appears to be empty or
whitespace-only." to use this new first non-empty trimmed line so nearest-match
hints work for strings with leading blank lines.
Comment on lines
+18
to
+22
| // altimate_change start — session-level URL failure cache (#471) | ||
| // Prevents repeated fetches to URLs that already returned 404/410 in this session. | ||
| // Keyed by URL string. Cleared when the process restarts (new session). | ||
| const failedUrls = new Map<string, { status: number; timestamp: number }>() | ||
| const FAILURE_CACHE_TTL = 5 * 60 * 1000 // 5 minutes |
There was a problem hiding this comment.
Misleading comment: cache is process-level, not session-level.
Based on the context snippet from tool.ts, Tool.define() is called once at module load time and the same tool instance is reused across all sessions. The failedUrls Map is module-scoped and persists across all sessions within the same process, not just a single session.
This could cause unintended behavior where a 404 cached from one user's session affects another user's session in the same process.
-// altimate_change start — session-level URL failure cache (`#471`)
-// Prevents repeated fetches to URLs that already returned 404/410 in this session.
-// Keyed by URL string. Cleared when the process restarts (new session).
+// altimate_change start — process-level URL failure cache (`#471`)
+// Prevents repeated fetches to URLs that already returned 404/410 in this process.
+// Keyed by URL string. Shared across all sessions; cleared when the process restarts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/tool/webfetch.ts` around lines 18 - 22, The comment for
the failedUrls cache incorrectly states it is session-level, but it is actually
process-level because it is a module-scoped Map reused across all sessions. To
fix this, update the comment above the failedUrls declaration to accurately
describe it as a process-level URL failure cache that persists for the process
lifetime and is cleared only when the process restarts. Remove or revise any
language implying it is scoped per user session.
Comment on lines
+50
to
+54
| case 429: { | ||
| const retryAfter = headers?.get("retry-after") | ||
| const wait = retryAfter ? ` (retry after ${retryAfter}s)` : "" | ||
| return `HTTP 429: Rate limited by ${new URL(url).hostname}${wait}. Wait before fetching from this domain again, or use a different source.` | ||
| } |
There was a problem hiding this comment.
retry-after header can be a date string, not just seconds.
Per HTTP spec, the Retry-After header can be either a number of seconds or an HTTP-date (e.g., "Fri, 31 Dec 2026 23:59:59 GMT"). Appending "s" unconditionally produces malformed messages for date values.
Suggested fix
case 429: {
const retryAfter = headers?.get("retry-after")
- const wait = retryAfter ? ` (retry after ${retryAfter}s)` : ""
+ let wait = ""
+ if (retryAfter) {
+ // retry-after can be seconds or HTTP-date; only append "s" for numeric values
+ wait = /^\d+$/.test(retryAfter)
+ ? ` (retry after ${retryAfter}s)`
+ : ` (retry after ${retryAfter})`
+ }
return `HTTP 429: Rate limited by ${new URL(url).hostname}${wait}. Wait before fetching from this domain again, or use a different source.`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case 429: { | |
| const retryAfter = headers?.get("retry-after") | |
| const wait = retryAfter ? ` (retry after ${retryAfter}s)` : "" | |
| return `HTTP 429: Rate limited by ${new URL(url).hostname}${wait}. Wait before fetching from this domain again, or use a different source.` | |
| } | |
| case 429: { | |
| const retryAfter = headers?.get("retry-after") | |
| let wait = "" | |
| if (retryAfter) { | |
| // retry-after can be seconds or HTTP-date; only append "s" for numeric values | |
| wait = /^\d+$/.test(retryAfter) | |
| ? ` (retry after ${retryAfter}s)` | |
| : ` (retry after ${retryAfter})` | |
| } | |
| return `HTTP 429: Rate limited by ${new URL(url).hostname}${wait}. Wait before fetching from this domain again, or use a different source.` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/tool/webfetch.ts` around lines 50 - 54, The 429
handling currently appends "s" to the Retry-After header unconditionally; change
the logic in the case 429 block (variables retryAfter and wait in
packages/opencode/src/tool/webfetch.ts) to detect whether
headers?.get("retry-after") is a numeric-second value or a HTTP-date string: if
it's a positive integer use " (retry after Xs)", if it's a non-numeric/date
string include it verbatim like " (retry after <value>)" or omit the unit, and
ensure the message remains valid by not appending "s" to date values; keep the
rest of the message about the hostname and suggested actions intact.
Comment on lines
+68
to
+83
| describe("regex fallback — simulated napi failure", () => { | ||
| // We test the fallback logic by directly calling the internal regex patterns. | ||
| // Since we can't mock require() in the already-loaded module, we replicate | ||
| // the fallback logic from the source to verify regex correctness. | ||
|
|
||
| const WRITE_PATTERN = | ||
| /^\s*(INSERT|UPDATE|DELETE|MERGE|CREATE|ALTER|DROP|TRUNCATE|GRANT|REVOKE|CALL|EXEC)\b/i | ||
| const HARD_DENY_PATTERN = | ||
| /^\s*(DROP\s+(DATABASE|SCHEMA)\b|TRUNCATE(\s+TABLE)?\s)/i | ||
|
|
||
| function classifyFallback(sql: string): { queryType: "read" | "write"; blocked: boolean } { | ||
| const trimmed = sql.replace(/\/\*[\s\S]*?\*\//g, "").trim() | ||
| const blocked = HARD_DENY_PATTERN.test(trimmed) | ||
| const queryType = WRITE_PATTERN.test(trimmed) ? "write" : "read" | ||
| return { queryType, blocked } | ||
| } |
There was a problem hiding this comment.
These tests can stay green without exercising the implementation paths they claim to cover.
The “simulated napi failure” block reimplements the regex locally instead of forcing sql-classify.ts through its real fallback branch, and the parity suite never proves the AST path is actually active. On a host where @altimateai/altimate-core fails to load, both suites can still pass while the shipped fallback logic drifts. Please share the real helper/patterns with the test, or load the module under an actual import failure so these assertions are coupled to production code.
Also applies to: 244-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/tools/sql-classify-fallback.test.ts` around
lines 68 - 83, Test reimplements WRITE_PATTERN/HARD_DENY_PATTERN and
classifyFallback locally, so it doesn't exercise the module's real fallback
branch; instead import the actual fallback patterns or force a real import
failure so the production fallback is exercised. Update the test to either (a)
import the exported fallback regexes/utility from sql-classify.ts (referencing
the module's exported symbols like WRITE_PATTERN, HARD_DENY_PATTERN or a helper
such as getFallbackPatterns/classifyFallbackFromModule) and use those in
assertions, or (b) simulate a real N-API load failure by mocking the
require/import of '@altimateai/altimate-core' to throw (using your test
framework's module-mocking API) before importing sql-classify.ts so the module's
fallback path runs and the tests assert against the module's actual fallback
implementation.
Prevents stale `@altimateai/altimate-core` platform binaries from reaching production. Two new defense layers: 1. **CI test** (`napi-exports.test.ts`): verifies all 40+ expected function exports exist in the installed binary. Fails CI if a version bump doesn't update the lock file. 2. **Pre-release check** (step 2b): validates 17 critical napi exports before `bun run pre-release` passes. Blocks release tagging when the binary is stale. Together with the runtime validation in altimate-core-internal PR #113, this creates 3 layers of defense: - Build-time: validate-exports.js patches index.js (core-internal) - CI: napi-exports.test.ts fails on missing exports (altimate-code) - Pre-release: pre-release-check.ts blocks stale binaries (altimate-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/script/pre-release-check.ts`:
- Around line 70-75: The pre-release export gate only lists a subset of required
N-API exports; update the CRITICAL_EXPORTS array to include the full export
contract used elsewhere so missing symbols can't slip through; locate the
CRITICAL_EXPORTS constant and add every exported function name that the N-API
consumer/validator expects (i.e., mirror the complete list of exports validated
in the N-API contract) so this file's check and the N-API validation are
aligned.
In `@packages/opencode/test/altimate/napi-exports.test.ts`:
- Around line 104-113: The test for getStatementTypes only checks top-level keys
but must also verify the nested statements shape used at runtime; update the
"getStatementTypes returns expected shape" test (and any helper calls to
core.getStatementTypes) to assert that result.statements is an array, that its
length matches result.statement_count, and that each item in result.statements
has a statement_type property (and optionally type/category fields used
downstream), and keep the existing checks for statement_count and categories
(e.g., ensure at least one statement has statement_type === "query").
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f1cce881-864d-45dd-a6b0-54a1e5f6a02a
📒 Files selected for processing (2)
packages/opencode/script/pre-release-check.tspackages/opencode/test/altimate/napi-exports.test.ts
Comment on lines
+70
to
+75
| const CRITICAL_EXPORTS = [ | ||
| "getStatementTypes", "formatSql", "lint", "validate", "transpile", | ||
| "extractMetadata", "columnLineage", "trackLineage", "diffSchemas", | ||
| "importDdl", "exportDdl", "optimizeContext", "pruneSchema", | ||
| "compareQueries", "classifyPii", "checkQueryPii", "parseDbtProject", | ||
| ] |
There was a problem hiding this comment.
Pre-release export gate is too narrow and can miss broken releases.
This list covers only a subset of the N-API contract validated elsewhere, so a binary with missing non-listed exports can still pass this pre-release check.
🔧 Proposed alignment with full export contract
const CRITICAL_EXPORTS = [
- "getStatementTypes", "formatSql", "lint", "validate", "transpile",
- "extractMetadata", "columnLineage", "trackLineage", "diffSchemas",
- "importDdl", "exportDdl", "optimizeContext", "pruneSchema",
- "compareQueries", "classifyPii", "checkQueryPii", "parseDbtProject",
+ "transpile",
+ "formatSql",
+ "extractMetadata",
+ "extractOutputColumns",
+ "getStatementTypes",
+ "compareQueries",
+ "optimizeContext",
+ "optimizeForQuery",
+ "pruneSchema",
+ "diffSchemas",
+ "importDdl",
+ "exportDdl",
+ "schemaFingerprint",
+ "introspectionSql",
+ "lint",
+ "scanSql",
+ "isSafe",
+ "classifyPii",
+ "checkQueryPii",
+ "resolveTerm",
+ "analyzeTags",
+ "columnLineage",
+ "diffLineage",
+ "trackLineage",
+ "complete",
+ "rewrite",
+ "generateTests",
+ "analyzeMigration",
+ "parseDbtProject",
+ "correct",
+ "evaluate",
+ "explain",
+ "fix",
+ "validate",
+ "checkEquivalence",
+ "checkPolicy",
+ "checkSemantics",
+ "initSdk",
+ "resetSdk",
+ "flushSdk",
]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const CRITICAL_EXPORTS = [ | |
| "getStatementTypes", "formatSql", "lint", "validate", "transpile", | |
| "extractMetadata", "columnLineage", "trackLineage", "diffSchemas", | |
| "importDdl", "exportDdl", "optimizeContext", "pruneSchema", | |
| "compareQueries", "classifyPii", "checkQueryPii", "parseDbtProject", | |
| ] | |
| const CRITICAL_EXPORTS = [ | |
| "transpile", | |
| "formatSql", | |
| "extractMetadata", | |
| "extractOutputColumns", | |
| "getStatementTypes", | |
| "compareQueries", | |
| "optimizeContext", | |
| "optimizeForQuery", | |
| "pruneSchema", | |
| "diffSchemas", | |
| "importDdl", | |
| "exportDdl", | |
| "schemaFingerprint", | |
| "introspectionSql", | |
| "lint", | |
| "scanSql", | |
| "isSafe", | |
| "classifyPii", | |
| "checkQueryPii", | |
| "resolveTerm", | |
| "analyzeTags", | |
| "columnLineage", | |
| "diffLineage", | |
| "trackLineage", | |
| "complete", | |
| "rewrite", | |
| "generateTests", | |
| "analyzeMigration", | |
| "parseDbtProject", | |
| "correct", | |
| "evaluate", | |
| "explain", | |
| "fix", | |
| "validate", | |
| "checkEquivalence", | |
| "checkPolicy", | |
| "checkSemantics", | |
| "initSdk", | |
| "resetSdk", | |
| "flushSdk", | |
| ] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/script/pre-release-check.ts` around lines 70 - 75, The
pre-release export gate only lists a subset of required N-API exports; update
the CRITICAL_EXPORTS array to include the full export contract used elsewhere so
missing symbols can't slip through; locate the CRITICAL_EXPORTS constant and add
every exported function name that the N-API consumer/validator expects (i.e.,
mirror the complete list of exports validated in the N-API contract) so this
file's check and the N-API validation are aligned.
Comment on lines
+104
to
+113
| test("getStatementTypes returns expected shape", () => { | ||
| if (!core || typeof core.getStatementTypes !== "function") return | ||
| const result = (core.getStatementTypes as (sql: string) => any)("SELECT 1") | ||
| expect(result).toHaveProperty("statements") | ||
| expect(result).toHaveProperty("statement_count") | ||
| expect(result).toHaveProperty("types") | ||
| expect(result).toHaveProperty("categories") | ||
| expect(result.statement_count).toBe(1) | ||
| expect(result.categories).toContain("query") | ||
| }) |
There was a problem hiding this comment.
Strengthen the contract check for getStatementTypes nested statement shape.
Current assertions only validate top-level keys. Runtime code also depends on statements[].statement_type, so this test can miss a breaking N-API shape change.
🔧 Proposed test hardening
test("getStatementTypes returns expected shape", () => {
if (!core || typeof core.getStatementTypes !== "function") return
const result = (core.getStatementTypes as (sql: string) => any)("SELECT 1")
expect(result).toHaveProperty("statements")
expect(result).toHaveProperty("statement_count")
expect(result).toHaveProperty("types")
expect(result).toHaveProperty("categories")
expect(result.statement_count).toBe(1)
expect(result.categories).toContain("query")
+ expect(Array.isArray(result.statements)).toBe(true)
+ expect(result.statements.length).toBeGreaterThan(0)
+ expect(result.statements[0]).toHaveProperty("statement_type")
+ expect(typeof result.statements[0].statement_type).toBe("string")
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("getStatementTypes returns expected shape", () => { | |
| if (!core || typeof core.getStatementTypes !== "function") return | |
| const result = (core.getStatementTypes as (sql: string) => any)("SELECT 1") | |
| expect(result).toHaveProperty("statements") | |
| expect(result).toHaveProperty("statement_count") | |
| expect(result).toHaveProperty("types") | |
| expect(result).toHaveProperty("categories") | |
| expect(result.statement_count).toBe(1) | |
| expect(result.categories).toContain("query") | |
| }) | |
| test("getStatementTypes returns expected shape", () => { | |
| if (!core || typeof core.getStatementTypes !== "function") return | |
| const result = (core.getStatementTypes as (sql: string) => any)("SELECT 1") | |
| expect(result).toHaveProperty("statements") | |
| expect(result).toHaveProperty("statement_count") | |
| expect(result).toHaveProperty("types") | |
| expect(result).toHaveProperty("categories") | |
| expect(result.statement_count).toBe(1) | |
| expect(result.categories).toContain("query") | |
| expect(Array.isArray(result.statements)).toBe(true) | |
| expect(result.statements.length).toBeGreaterThan(0) | |
| expect(result.statements[0]).toHaveProperty("statement_type") | |
| expect(typeof result.statements[0].statement_type).toBe("string") | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/test/altimate/napi-exports.test.ts` around lines 104 - 113,
The test for getStatementTypes only checks top-level keys but must also verify
the nested statements shape used at runtime; update the "getStatementTypes
returns expected shape" test (and any helper calls to core.getStatementTypes) to
assert that result.statements is an array, that its length matches
result.statement_count, and that each item in result.statements has a
statement_type property (and optionally type/category fields used downstream),
and keep the existing checks for statement_count and categories (e.g., ensure at
least one statement has statement_type === "query").
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes 4 issues that collectively caused 1,206 tool failures per day:
sql_executeeditwebfetchyaml2.8.2 → 2.8.3 (stack overflow fix)#469 —
sql_execute:getStatementTypes is not a functionrequire("@altimateai/altimate-core")withanytype and no guard. When napi binary fails to load,core.getStatementTypesisundefined.typeofcheck, regex fallback classifier, null input guards.#470 —
edit: "Could not find oldString" (context drift)oldString.buildNotFoundMessage()uses Levenshtein to find closest match, shows 5-line snippet with line numbers, tells model to re-read the file.#471 —
webfetch: 934 failures (55% of all tool failures)URLconstructor validation, session-level 404/410/451 cache (5-min TTL), HTTP-status-specific error messages ("Do NOT retry" for 404, "Rate limited" withRetry-Afterfor 429).#473 —
yaml2.8.2 → 2.8.3Type of change
Issue for this PR
Closes #469, Closes #470, Closes #471, Closes #473
How did you verify your code works?
sql-classify-fallback.test.ts: 42 tests — regex fallback parity, ReDoS protection, null inputs, bypass attemptsedit-error-context.test.ts: 14 tests — Levenshtein matching, snippet bounds, edge caseswebfetch-validation.test.ts: 24 tests — URL validation, cache TTL, error messages, edge casesChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests