fix: graceful degradation when native binding fails to load#324
Open
anandgupta42 wants to merge 3 commits intomainfrom
Open
fix: graceful degradation when native binding fails to load#324anandgupta42 wants to merge 3 commits intomainfrom
anandgupta42 wants to merge 3 commits intomainfrom
Conversation
On systems with older GLIBC (e.g. Ubuntu 20.04 with GLIBC 2.31), `@altimateai/altimate-core` napi-rs binary requires GLIBC 2.35 and crashes with an unhandled stack trace on startup, blocking all CLI usage. Three changes to degrade gracefully instead of crashing: 1. `sql-classify.ts`: lazy-load `altimate-core` on first call instead of eager `require()` at import time. Falls back to "write" (safe default) when native binding is unavailable. 2. `native/index.ts`: catch `altimate-core` import failure separately so other handler modules (connections, schema, finops, dbt) still register. 3. `dispatcher.ts`: catch registration hook failures caused by native binding errors, log a clear warning, and continue. Non-native-binding errors still throw. Result: CLI starts normally. SQL analysis tools (validate, lint, transpile, lineage) are unavailable, but warehouse connections, schema indexing, dbt integration, and all other features work. Closes #310
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds fault-tolerant handling around native binding loads and lazy native registration: dynamic imports and lazy registration are wrapped in try/catch that classify and suppress native-binding-related errors (e.g., native binding, GLIBC, ERR_DLOPEN_FAILED) while rethrowing other errors, and provides safe fallbacks when the native core is unavailable. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
packages/opencode/src/altimate/native/dispatcher.ts (1)
49-50: Consolidate native-load error detection into a shared helper.The same message-matching logic is duplicated here and in
native/index.ts; centralizing it reduces drift and keeps graceful-degradation behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/dispatcher.ts` around lines 49 - 50, Extract the duplicated message-matching logic (msg.includes("native binding") || msg.includes("GLIBC") || msg.includes("ERR_DLOPEN_FAILED")) into a single exported helper like isNativeLoadError(message: string): boolean in the native module (e.g., export from native/index.ts or a small native/errors.ts), then import and use that helper from dispatcher.ts (replace the inline check in the block containing console.error) and from the other location in native/index.ts so both places call isNativeLoadError(msg) instead of duplicating the string checks.
🤖 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/native/index.ts`:
- Around line 23-28: The unguarded top-level await of "./sql/register" can throw
and stop registration of the other handlers; wrap each dynamic import (at least
"./sql/register", "./connections/register", "./schema/register",
"./finops/register", "./dbt/register", "./local/register") so a failure in one
doesn’t abort the rest — e.g., perform the imports with individual try/catch
blocks or use Promise.allSettled on the array of import() promises, log or
surface errors (including the error from the sql/register import which pulls in
`@altimateai/altimate-core`) and continue registering remaining modules.
In `@packages/opencode/src/altimate/tools/sql-classify.ts`:
- Around line 58-60: The fallback in classifyAndCheck currently returns {
queryType: "write", blocked: false } when getCore() is null, which allows
hard-deny SQL to pass; change the fallback to conservatively block by returning
{ queryType: "write", blocked: true } (or otherwise ensure blocked is true) so
that when native parsing via getCore() is unavailable, classifyAndCheck treats
statements as writes and enforces hard-deny blocking.
---
Nitpick comments:
In `@packages/opencode/src/altimate/native/dispatcher.ts`:
- Around line 49-50: Extract the duplicated message-matching logic
(msg.includes("native binding") || msg.includes("GLIBC") ||
msg.includes("ERR_DLOPEN_FAILED")) into a single exported helper like
isNativeLoadError(message: string): boolean in the native module (e.g., export
from native/index.ts or a small native/errors.ts), then import and use that
helper from dispatcher.ts (replace the inline check in the block containing
console.error) and from the other location in native/index.ts so both places
call isNativeLoadError(msg) instead of duplicating the string checks.
🪄 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: 9a4c6828-02b1-45a6-af14-6d0f59bcaafa
📒 Files selected for processing (3)
packages/opencode/src/altimate/native/dispatcher.tspackages/opencode/src/altimate/native/index.tspackages/opencode/src/altimate/tools/sql-classify.ts
Comment on lines
58
to
+60
| export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } { | ||
| const core = getCore() | ||
| if (!core) return { queryType: "write", blocked: false } |
There was a problem hiding this comment.
Hard-deny safety is bypassed when native core is unavailable.
On Line 60, fallback returns blocked: false, which allows hard-deny statements to proceed through the write-permission path instead of being unconditionally blocked. Keep a conservative hard-deny fallback even without native parsing.
🔧 Proposed fix
const HARD_DENY_TYPES = new Set(["DROP DATABASE", "DROP SCHEMA", "TRUNCATE", "TRUNCATE TABLE"])
+const HARD_DENY_FALLBACK_REGEX = /\bDROP\s+(DATABASE|SCHEMA)\b|\bTRUNCATE(?:\s+TABLE)?\b/i
@@
export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } {
const core = getCore()
- if (!core) return { queryType: "write", blocked: false }
+ if (!core) {
+ return { queryType: "write", blocked: HARD_DENY_FALLBACK_REGEX.test(sql) }
+ }📝 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
| export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } { | |
| const core = getCore() | |
| if (!core) return { queryType: "write", blocked: false } | |
| export function classifyAndCheck(sql: string): { queryType: "read" | "write"; blocked: boolean } { | |
| const core = getCore() | |
| if (!core) { | |
| return { queryType: "write", blocked: HARD_DENY_FALLBACK_REGEX.test(sql) } | |
| } |
🤖 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 58 - 60,
The fallback in classifyAndCheck currently returns { queryType: "write",
blocked: false } when getCore() is null, which allows hard-deny SQL to pass;
change the fallback to conservatively block by returning { queryType: "write",
blocked: true } (or otherwise ensure blocked is true) so that when native
parsing via getCore() is unavailable, classifyAndCheck treats statements as
writes and enforces hard-deny blocking.
…ation `sql/register`, `schema/register`, `dbt/register`, and `local/register` all transitively import `@altimateai/altimate-core`. Previously only the direct `altimate-core` import was wrapped — if any register module failed due to GLIBC mismatch, the remaining modules wouldn't load either. Now each core-dependent module is individually wrapped so a native binding failure in one doesn't prevent the others from registering. Modules that don't depend on core (`connections/register`, `finops/register`) load unconditionally. Adds CI guard tests to `build-integrity.test.ts` that verify: - `isNativeBindingError` helper exists - `altimate-core` import is wrapped in try/catch - All 4 core-dependent modules are NOT bare `await import()` calls - Both safe modules are still referenced Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/src/altimate/native/index.ts (1)
9-12: Consider centralizing native-binding error matching.
isNativeBindingErrorappears logically shared with dispatcher behavior. Moving this matcher to one shared helper will prevent drift when patterns evolve.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/native/index.ts` around lines 9 - 12, The native-binding error matcher in isNativeBindingError should be moved to a shared helper to avoid duplicated logic; extract the logic (currently in isNativeBindingError) into a single exported function (e.g., matchesNativeBindingError or isNativeBindingError in a new/shared utils module), replace the local implementation in packages/opencode/src/altimate/native/index.ts with an import from that shared helper, update the dispatcher code to import and use the same helper (remove its own matching logic), and run/adjust any tests or usages to reference the shared symbol to ensure no duplicated patterns remain.packages/opencode/test/branding/build-integrity.test.ts (1)
366-366: Consider escapingmodin the RegExp to future-proof against special characters.While the current module list contains only literal-safe names (
sql/register,schema/register,dbt/register,local/register), escaping the interpolated value guards against regex fragility if module names change to include metacharacters.Suggested hardening
+ const escapeRegExp = (value: string) => + value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") + for (const mod of coreDepModules) { test(`${mod} import is wrapped in try/catch (not bare await)`, () => { // Each core-dependent module should appear inside the coreDependent array or // a try/catch, NOT as a bare `await import("./module")`. - const barePattern = new RegExp(`^\\s*await import\\(["']\\.\\/${mod}["']\\)`, "m") + const barePattern = new RegExp( + `^\\s*await import\\(["']\\.\\/${escapeRegExp(mod)}["']\\)`, + "m", + ) expect(nativeIndex).not.toMatch(barePattern)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/branding/build-integrity.test.ts` at line 366, The RegExp construction for barePattern uses the interpolated variable mod directly which can break if mod contains regex metacharacters; update the code that builds barePattern (the RegExp(...) call that assigns barePattern) to escape mod before interpolation (e.g., implement or call an escapeRegExp utility to sanitize the mod string) and then use the escaped value in new RegExp so the pattern reliably matches literal module names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/altimate/native/index.ts`:
- Around line 9-12: The native-binding error matcher in isNativeBindingError
should be moved to a shared helper to avoid duplicated logic; extract the logic
(currently in isNativeBindingError) into a single exported function (e.g.,
matchesNativeBindingError or isNativeBindingError in a new/shared utils module),
replace the local implementation in
packages/opencode/src/altimate/native/index.ts with an import from that shared
helper, update the dispatcher code to import and use the same helper (remove its
own matching logic), and run/adjust any tests or usages to reference the shared
symbol to ensure no duplicated patterns remain.
In `@packages/opencode/test/branding/build-integrity.test.ts`:
- Line 366: The RegExp construction for barePattern uses the interpolated
variable mod directly which can break if mod contains regex metacharacters;
update the code that builds barePattern (the RegExp(...) call that assigns
barePattern) to escape mod before interpolation (e.g., implement or call an
escapeRegExp utility to sanitize the mod string) and then use the escaped value
in new RegExp so the pattern reliably matches literal module names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2deec7a3-1738-4fd8-a032-2908125d2fbb
📒 Files selected for processing (2)
packages/opencode/src/altimate/native/index.tspackages/opencode/test/branding/build-integrity.test.ts
Users seeing the GLIBC mismatch warning now get actionable fix steps: which distro versions have GLIBC >= 2.35, Docker workaround, and how to check their current GLIBC version. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Tests — All PassedTypeScript — passedcc @anandgupta42 |
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
2 participants
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?
On systems with older GLIBC (e.g. Ubuntu 20.04 with GLIBC 2.31),
@altimateai/altimate-corerequires GLIBC 2.35 and crashes with an unhandled stack trace on startup, blocking all CLI usage. This PR makes the CLI degrade gracefully instead of crashing.Before: Any CLI command crashes with
Cannot find native bindingstack trace.After: CLI starts normally. SQL analysis tools are unavailable, but everything else works. User sees a clear warning.
Type of change
Issue for this PR
Closes #310
How did you verify your code works?
getCore()returns null,classify()returns"write"(safe default) andclassifyAndCheck()returns{ queryType: "write", blocked: false }Files changed
src/altimate/tools/sql-classify.tsaltimate-coreon first call instead of eagerrequire(). Falls back to"write"when unavailablesrc/altimate/native/index.tsaltimate-coreimport failure separately so other handlers (connections, schema, finops, dbt) still registersrc/altimate/native/dispatcher.tsChecklist
🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Bug Fixes
Tests