Repository navigation
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces centralized error detection and typed error handling across AI provider integrations, replacing generic error returns with specific error classes. Additionally, CSV processing is enhanced with column metadata analysis, data quality warnings, and quality scoring capabilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/providers/litellm.ts (1)
101-173:⚠️ Potential issue | 🟠 MajorChange
handleProviderErrorreturn type fromErrortoneveracross all providers.All 14 providers have
handleProviderError(error: unknown): Error, but every code path throws. The return type is misleading and violates TypeScript's contract. Since the method always throws (never returns), the return type should benever, and call sites should remove the outerthrow:
- Change:
public handleProviderError(error: unknown): Error→public handleProviderError(error: unknown): never- At call sites:
throw this.handleProviderError(error)→this.handleProviderError(error)This applies to all 14 providers: litellm.ts, openAI.ts, mistral.ts, openRouter.ts, huggingFace.ts, googleAiStudio.ts, ollama.ts, azureOpenai.ts, anthropic.ts, amazonSagemaker.ts, googleVertex.ts, amazonBedrock.ts, openaiCompatible.ts, anthropicBaseProvider.ts.
src/lib/providers/mistral.ts (1)
173-221:⚠️ Potential issue | 🟠 MajorSame
handleProviderErrorreturn-type / throw mismatch as ingoogleVertex.ts.This method's return type is
Errorbut every branch throws. The same fix applies: change return type tonever(pending base class update).Beyond that, the error classification logic is clean and correctly maps Mistral-specific error patterns to typed errors. The
detectProviderErrorpre-check followed by provider-specific fallbacks is a sound layered approach.Proposed fix
- public handleProviderError(error: unknown): Error { + public handleProviderError(error: unknown): never {
🤖 Fix all issues with AI agents
In `@src/lib/providers/anthropicBaseProvider.ts`:
- Around line 56-97: The handleProviderError function should have a never return
type and avoid redundant status-code branches already handled by
detectProviderError: change the signature from "protected
handleProviderError(error: unknown): Error" to "protected
handleProviderError(error: unknown): never", keep the detectProviderError(error,
this.providerName) and TimeoutError handling, and then either remove the
explicit status checks for 401 and 429 (the AuthenticationError/RateLimitError
branches) or add a clear comment that they are intentional fallbacks; ensure
thrown errors remain NetworkError, AuthenticationError, RateLimitError,
ProviderError as currently used and reference these symbols when modifying the
code.
In `@src/lib/providers/azureOpenai.ts`:
- Around line 88-130: In handleProviderError, detectProviderError is currently
called first and throws before Azure-specific checks can provide tailored
guidance; move the detectProviderError call to the end of handleProviderError
(after the AuthenticationError, RateLimitError, NetworkError, and ProviderError
checks) so provider-specific message logic in handleProviderError runs first and
only falls back to detectProviderError/detectedError if no manual pattern
matched; reference handleProviderError, detectProviderError, and
DEFAULT_ERROR_PATTERNS when making the change and preserve throwing the detected
error when it is returned at the end.
In `@src/lib/types/errors.ts`:
- Around line 84-122: The DEFAULT_ERROR_PATTERNS contains the numeric substring
patterns "401" and "429" which are too broad and cause false positives because
matchesPatterns does a substring check; remove "401" from the authentication
array and "429" from the rateLimit array in DEFAULT_ERROR_PATTERNS and rely on
extractStatusCode/statusCodes handling for numeric HTTP status detection (see
matchesPatterns and extractStatusCode references to guide locating the logic).
- Line 148: The patterns assignment currently shallow-merges
DEFAULT_ERROR_PATTERNS and customPatterns, causing whole arrays to be replaced;
update the merge so for each error category key (use DEFAULT_ERROR_PATTERNS and
customPatterns) you concatenate/merge the arrays (e.g.,
DEFAULT_ERROR_PATTERNS[key].concat(customPatterns[key]) with sensible defaults
when a key is missing) and optionally deduplicate entries, then assign the
result to patterns so providers can extend rather than overwrite defaults.
In `@src/lib/utils/csvProcessor.ts`:
- Around line 109-117: BOOLEAN_VALUES currently runs before numeric detection so
"1"/"0" get classified as boolean; change the check order to test numeric types
first by moving the INTEGER_REGEX (and FLOAT_REGEX if present) checks above the
BOOLEAN_VALUES check (i.e., run INTEGER_REGEX.test(trimmed) and any float test
before consulting BOOLEAN_VALUES), or alternatively remove "1" and "0" from
BOOLEAN_VALUES; update the logic in the type-detection function that uses
BOOLEAN_VALUES and INTEGER_REGEX accordingly so numeric strings are classified
as integer/float before being treated as boolean.
In `@test/unit/utils/csvProcessor.test.ts`:
- Around line 549-601: The test indicates 0 and 1 are being misclassified as
boolean because the type detection checks boolean before integer; update the
detection logic in detectValueType (or wherever value classification occurs in
CSVProcessor) to check integer/number types before boolean so numeric values 0
and 1 are classified as integers/floats, not booleans, then adjust or re-run the
"should consolidate integers and floats as number type" test to include natural
0/1 values if desired to confirm the fix.
🧹 Nitpick comments (8)
src/lib/utils/csvProcessor.ts (4)
277-283:Math.min(...numericValues)/Math.max(...)can throwRangeErroron large arrays.With
maxRowsup to 10,000, the spread operator passes all elements as function arguments, which can exceed the JS engine's call stack limit (typically ~65K–125K args, but behavior varies). For a fully numeric column with 10,000 rows this is likely fine, but it's a latent risk if limits are raised.A simple loop or
reduceavoids this entirely:Proposed fix
if (numericValues.length > 0) { - minValue = Math.min(...numericValues); - maxValue = Math.max(...numericValues); + minValue = numericValues.reduce((a, b) => Math.min(a, b), numericValues[0]); + maxValue = numericValues.reduce((a, b) => Math.max(a, b), numericValues[0]); avgValue = Math.round( (numericValues.reduce((a, b) => a + b, 0) / numericValues.length) * 100, ) / 100; }
686-692: Raw format re-parses the CSV solely for metadata analysis — potential performance hit.The raw path already has the CSV string available but now re-parses it through the streaming CSV parser (up to 500 rows) just to populate column metadata. For large files this roughly doubles processing time in the raw path.
Consider whether this trade-off is intentional. If metadata is not always needed, a lazy or opt-in approach would be lighter. Otherwise this is acceptable if metadata is always consumed downstream.
389-429: Quality score can be double-penalized for the same issue.
calculateDataQualityScorededucts points per warning (e.g.,high_null_ratewarning → −3 or −8) and also deducts for overall null rate (Line 419) and low type confidence (Lines 424-426). A column with high nulls thus gets penalized via both the warning deduction and the null-rate deduction. This may be intentional for weighting, but it makes the score harder to reason about and could yield unexpectedly low scores for moderately problematic data.
716-720: Raw format header detection splits on comma only — ignores other delimiters.
(limitedLines[0] || "").split(",")at Line 717 assumes comma delimiter. If the CSV uses tabs or semicolons, header detection will receive a single-element array and likely return incorrect results. ThedetectedDelimiteris also hardcoded to","at Line 720. This is consistent but worth noting if multi-delimiter support is planned.test/unit/utils/csvProcessor.test.ts (1)
804-845: Data quality score tests are adequate but could benefit from boundary testing.Consider adding a test that verifies the score is clamped to
[0, 100]— e.g., extremely problematic data shouldn't yield a negative score. The implementation doesMath.max(0, Math.min(100, score)), but there's no test asserting the lower bound explicitly for worst-case input.src/lib/types/errors.ts (1)
143-178:AuthorizationError(403) is not handled bydetectProviderError.The file defines
AuthorizationError(Line 36), butdetectProviderErrorhas no pattern category or status code check for it. HTTP 403 / "Forbidden" / "AccessDenied" errors will fall through and returnnull. The Bedrock provider manually handlesAccessDeniedExceptionafterdetectProviderErrorreturns null, but other providers may not. Consider adding anauthorizationcategory toErrorDetectionPatternswith status code[403]and patterns like"Forbidden","AccessDenied","access_denied".src/lib/providers/litellm.ts (1)
102-106: Redundant timeout detection afterdetectProviderError.
detectProviderErroralready detectsTimeoutErrorandAbortError(viaisTimeoutError) and returns a genericNetworkError("Request timed out"). The manualTimeoutError/"Timeout"checks on lines 108–126 will never execute for timeout errors becausedetectProviderErrorthrows first.If the intent is to preserve the more descriptive LiteLLM-specific message (
"LiteLLM request timed out: ..."), pass custom timeout patterns that won't match indetectProviderErrorand handle timeouts manually. Otherwise, the lines 108–126 are dead code.This same redundancy exists in other providers (HuggingFace, OpenRouter, Ollama). Not blocking, but worth being aware of.
Also applies to: 108-126
src/lib/providers/googleVertex.ts (1)
2331-2335: Potential double-throw:detectProviderErrormay already classify timeouts that the next block also checks.
detectProviderErrorcallsisTimeoutError(error)internally and returns aNetworkErrorfor timeouts. The explicitTimeoutErrorcheck at lines 2338-2346 is therefore a defensive fallback for cases wheredetectProviderError's heuristic doesn't match (e.g., a customTimeoutErrorclass with.name === "TimeoutError"). This is fine as-is but worth noting that the two checks overlap significantly. No action needed unless you want to simplify.
| public handleProviderError(error: unknown): Error { | ||
| const errorObj = error as UnknownRecord; | ||
| if ( | ||
| errorObj?.message && | ||
| typeof errorObj.message === "string" && | ||
| errorObj.message.includes("401") | ||
| ) { | ||
| return new Error("Invalid Azure OpenAI API key or endpoint."); | ||
| // Try automatic error detection first | ||
| const detectedError = detectProviderError(error, this.providerName); | ||
| if (detectedError) { | ||
| throw detectedError; | ||
| } | ||
|
|
||
| const errorObj = error as UnknownRecord; | ||
| const message = | ||
| errorObj?.message && typeof errorObj.message === "string" | ||
| ? errorObj.message | ||
| : "Unknown error"; | ||
| return new Error(`Azure OpenAI error: ${message}`); | ||
|
|
||
| if (message.includes("401") || message.includes("Unauthorized")) { | ||
| throw new AuthenticationError( | ||
| "Invalid Azure OpenAI API key or endpoint. Please check your AZURE_OPENAI_API_KEY and AZURE_OPENAI_ENDPOINT environment variables.", | ||
| this.providerName, | ||
| ); | ||
| } | ||
|
|
||
| if (message.includes("429") || message.includes("rate limit")) { | ||
| throw new RateLimitError( | ||
| "Azure OpenAI rate limit exceeded. Please try again later.", | ||
| this.providerName, | ||
| ); | ||
| } | ||
|
|
||
| if ( | ||
| message.includes("ECONNREFUSED") || | ||
| message.includes("ENOTFOUND") || | ||
| message.includes("fetch failed") | ||
| ) { | ||
| throw new NetworkError( | ||
| `Azure OpenAI connection error: ${message}`, | ||
| this.providerName, | ||
| ); | ||
| } | ||
|
|
||
| throw new ProviderError( | ||
| `Azure OpenAI error: ${message}`, | ||
| this.providerName, | ||
| ); | ||
| } |
There was a problem hiding this comment.
detectProviderError shadows all Azure-specific error messages.
Every manual check below detectProviderError (lines 101–124) matches patterns that are already in DEFAULT_ERROR_PATTERNS — e.g., "401", "Unauthorized", "429", "rate limit", "ECONNREFUSED", "ENOTFOUND". Since detectProviderError throws first with the raw error message, the carefully crafted Azure-specific guidance messages (like "check your AZURE_OPENAI_API_KEY and AZURE_OPENAI_ENDPOINT") will never reach the user.
This is a correctness concern affecting user experience — the whole point of provider-specific messages is lost. The same issue applies to other providers but is most impactful here since Azure requires multiple environment variables.
Options:
- Move
detectProviderErrorto the end as a fallback, after provider-specific checks. - Don't call
detectProviderErrorand rely solely on manual checks (since they already cover the same cases with better messages). - Have
detectProviderErrorreturn the error instead of throwing, and only throw it if no manual check matched.
Option 1: Move detectProviderError to end as fallback
public handleProviderError(error: unknown): Error {
- // Try automatic error detection first
- const detectedError = detectProviderError(error, this.providerName);
- if (detectedError) {
- throw detectedError;
- }
-
const errorObj = error as UnknownRecord;
const message = /* ... */;
if (message.includes("401") || message.includes("Unauthorized")) {
throw new AuthenticationError(/* Azure-specific message */, this.providerName);
}
// ... other manual checks ...
+ // Fallback: try automatic error detection
+ const detectedError = detectProviderError(error, this.providerName);
+ if (detectedError) {
+ throw detectedError;
+ }
+
throw new ProviderError(`Azure OpenAI error: ${message}`, this.providerName);
}🤖 Prompt for AI Agents
In `@src/lib/providers/azureOpenai.ts` around lines 88 - 130, In
handleProviderError, detectProviderError is currently called first and throws
before Azure-specific checks can provide tailored guidance; move the
detectProviderError call to the end of handleProviderError (after the
AuthenticationError, RateLimitError, NetworkError, and ProviderError checks) so
provider-specific message logic in handleProviderError runs first and only falls
back to detectProviderError/detectedError if no manual pattern matched;
reference handleProviderError, detectProviderError, and DEFAULT_ERROR_PATTERNS
when making the change and preserve throwing the detected error when it is
returned at the end.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of handleProviderError in azureOpenai.ts
rg -n "handleProviderError|detectProviderError|AuthenticationError|RateLimitError|NetworkError|ProviderError" src/lib/providers/azureOpenai.ts -A2 -B2
Length of output: 338
🏁 Script executed:
#!/bin/bash
# Get the full content of formatProviderError / handleProviderError methods
sed -n '84,140p' src/lib/providers/azureOpenai.ts
Length of output: 1933
@y-naaz, I checked the current state of src/lib/providers/azureOpenai.ts on the branch and it looks like the changes have been fully reverted to the original pre-PR implementation — the file now contains formatProviderError with plain Error returns and none of the typed errors (AuthenticationError, RateLimitError, etc.) or detectProviderError are present.
Could you clarify the intended resolution? There are a few possibilities:
- Intentionally reverted — The changes to
azureOpenai.tswere dropped from this PR entirely, in which case this comment can be dismissed. - Fix applied incorrectly — The suggested fix (moving
detectProviderErrorto the end as a fallback) was supposed to be applied to thehandleProviderErrormethod introduced in the diff, but wasn't pushed correctly.
If the intent is to apply the fix, the corrected version should look like:
public handleProviderError(error: unknown): Error {
const errorObj = error as UnknownRecord;
const message =
errorObj?.message && typeof errorObj.message === "string"
? errorObj.message
: "Unknown error";
if (message.includes("401") || message.includes("Unauthorized")) {
throw new AuthenticationError(
"Invalid Azure OpenAI API key or endpoint. Please check your AZURE_OPENAI_API_KEY and AZURE_OPENAI_ENDPOINT environment variables.",
this.providerName,
);
}
if (message.includes("429") || message.includes("rate limit")) {
throw new RateLimitError(
"Azure OpenAI rate limit exceeded. Please try again later.",
this.providerName,
);
}
if (
message.includes("ECONNREFUSED") ||
message.includes("ENOTFOUND") ||
message.includes("fetch failed")
) {
throw new NetworkError(
`Azure OpenAI connection error: ${message}`,
this.providerName,
);
}
// Fallback: try automatic error detection
const detectedError = detectProviderError(error, this.providerName);
if (detectedError) {
throw detectedError;
}
throw new ProviderError(`Azure OpenAI error: ${message}`, this.providerName);
}🧠 Learnings used
Learnt from: murdore
Repo: juspay/neurolink PR: 0
File: :0-0
Timestamp: 2026-02-22T14:18:13.480Z
Learning: In the neurolink providers, formatProviderError must return typed errors (not throw). For Google Vertex specifically, timeouts and common network errors (ECONNRESET, ENOTFOUND, ECONNREFUSED) are mapped to NetworkError for consistency across providers.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/index.ts:16-16
Timestamp: 2025-09-17T17:55:15.261Z
Learning: In src/lib/types/providers.ts, ProviderConfig was renamed to AIModelProviderConfig to deduplicate type names, as there was an existing ProviderConfig type that better suited the "ProviderConfig" name. This was an intentional breaking change for better type organization.
Learnt from: YasmeenOgo
Repo: juspay/neurolink PR: 145
File: src/lib/core/types.ts:0-0
Timestamp: 2025-09-02T13:50:42.770Z
Learning: The APIVersions enum in src/lib/core/types.ts now contains comprehensive API version constants for all major AI providers: Azure OpenAI (latest, stable, legacy), OpenAI (current, beta), Google AI (current, beta), and Anthropic (current). This centralization helps avoid API version drift across the codebase.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 173
File: src/lib/types/index.ts:58-62
Timestamp: 2025-09-17T18:14:34.960Z
Learning: RajuSudhar explained that in the Neurolink codebase, there are multiple ProviderConfig types causing inconsistency. One existing ProviderConfig type better suited the "ProviderConfig" name, so they renamed the less-suitable one to AIModelProviderConfig to free up the name. Adding backward compatibility aliases would worsen naming inconsistency rather than help. The remaining duplicates will be systematically deduplicated in the 07-Types-Module.md TODO as part of their phased refactor approach.
Learnt from: RajuSudhar
Repo: juspay/neurolink PR: 0
File: :0-0
Timestamp: 2025-11-04T22:14:18.719Z
Learning: In the juspay/neurolink repository, do not flag existing type or interface definitions located outside src/lib/types/ - these are part of a phased migration plan and will be addressed in upcoming PRs. Only enforce type centralization rules on new code going forward.
Learnt from: sudharsan-juspay
Repo: juspay/neurolink PR: 160
File: src/lib/providers/index.ts:43-44
Timestamp: 2025-09-10T08:22:11.910Z
Learning: In the Neurolink project, type deduplication across modules (like ProviderName definitions) should be handled as separate tasks rather than mixed with other refactoring efforts, as there are multiple such occurrences throughout the codebase that need systematic cleanup.
There was a problem hiding this comment.
Pull request overview
This PR aims to standardize provider error handling by introducing a shared error-detection utility and updating multiple providers to emit consistent typed errors. In addition, it introduces enhanced CSV analysis (column type detection, data quality warnings, quality scoring, and header detection) along with extensive new unit tests for the CSV processor.
Changes:
- Add
detectProviderError()and default message-pattern matching to classify provider errors into typedProviderErrorsubclasses. - Update multiple providers to use the standardized typed errors for auth/rate-limit/network/model failures.
- Add CSV column metadata analysis (types/statistics), data quality warnings/scoring, and header detection + new unit tests covering the new metadata.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/types/errors.ts |
Adds shared error pattern detection (detectProviderError) and supporting helpers/constants. |
src/lib/providers/openRouter.ts |
Switches OpenRouter error handling to standardized typed errors and auto-detection. |
src/lib/providers/ollama.ts |
Updates Ollama error mapping to standardized typed errors and auto-detection. |
src/lib/providers/mistral.ts |
Updates Mistral error mapping to standardized typed errors and auto-detection. |
src/lib/providers/litellm.ts |
Updates LiteLLM error mapping to standardized typed errors and auto-detection. |
src/lib/providers/huggingFace.ts |
Updates HuggingFace error mapping to standardized typed errors and auto-detection. |
src/lib/providers/googleVertex.ts |
Updates Vertex error mapping to standardized typed errors and auto-detection. |
src/lib/providers/azureOpenai.ts |
Updates Azure OpenAI error mapping to standardized typed errors and auto-detection. |
src/lib/providers/anthropicBaseProvider.ts |
Updates Anthropic base provider error mapping to standardized typed errors and auto-detection. |
src/lib/providers/amazonBedrock.ts |
Updates Bedrock error mapping to standardized typed errors and auto-detection. |
src/lib/utils/csvProcessor.ts |
Adds enhanced CSV metadata analysis (column typing, warnings, score, header detection). |
src/lib/types/fileTypes.ts |
Extends CSV-related metadata types to expose column metadata, warnings, score, header/delimiter info. |
test/unit/utils/csvProcessor.test.ts |
Adds comprehensive unit tests for the new CSV metadata and quality features. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public handleProviderError(error: unknown): Error { | ||
| // Try automatic error detection first | ||
| const detectedError = detectProviderError(error, this.providerName); | ||
| if (detectedError) { | ||
| throw detectedError; |
There was a problem hiding this comment.
handleProviderError() now throws (including the detectProviderError fast-path). Existing integration tests call this method directly and expect it to return an Error instance (e.g., const handledError = provider.handleProviderError(error)). To avoid breaking that contract, either revert to returning the typed errors (and let callers throw), or update the public API/tests + BaseProvider contract to make handleProviderError consistently throw across providers.
| const patterns = { ...DEFAULT_ERROR_PATTERNS, ...customPatterns }; | ||
|
|
||
| // Extract error message from various error shapes | ||
| const message = extractErrorMessage(error); | ||
| const statusCode = extractStatusCode(error); |
There was a problem hiding this comment.
customPatterns is merged via object spread. If a caller passes { authentication: undefined } (or similar), it will overwrite the default array with undefined, and subsequent matchesPatterns() calls will throw at runtime because they assume string[]. Consider per-field nullish merging (e.g. custom?.authentication ?? DEFAULT...) or sanitizing customPatterns before use.
| rateLimit?: string[]; | ||
| network?: string[]; | ||
| invalidModel?: string[]; | ||
| timeout?: string[]; | ||
| } |
There was a problem hiding this comment.
ErrorDetectionPatterns exposes a timeout pattern list, but detectProviderError() never consults patterns.timeout (it only calls isTimeoutError). Either incorporate timeout patterns into detection logic or remove the unused field/defaults to avoid a misleading API surface.
| const uniqueHeaders = new Set( | ||
| headerValues.map((v) => v?.trim().toLowerCase()), | ||
| ); | ||
| const hasUniqueHeaders = uniqueHeaders.size === headerValues.length; |
There was a problem hiding this comment.
detectHasHeaders() treats empty headers as allowed, but uniqueness is checked across all trimmed header values (including ''), and compared to headerValues.length. Multiple empty header cells will therefore force hasUniqueHeaders to false and can incorrectly return hasHeaders=false. Consider excluding empty/blank header names from the uniqueness check (or updating the comment/behavior to disallow empty headers).
| const uniqueHeaders = new Set( | |
| headerValues.map((v) => v?.trim().toLowerCase()), | |
| ); | |
| const hasUniqueHeaders = uniqueHeaders.size === headerValues.length; | |
| // Uniqueness is evaluated only among non-empty header names; empty headers are allowed. | |
| const normalizedNonEmptyHeaders = headerValues | |
| .map((v) => v?.trim().toLowerCase()) | |
| .filter((v) => v !== ""); | |
| const uniqueHeaders = new Set(normalizedNonEmptyHeaders); | |
| const hasUniqueHeaders = | |
| uniqueHeaders.size === normalizedNonEmptyHeaders.length; |
| hasHeaders: detectHasHeaders( | ||
| (limitedLines[0] || "").split(","), | ||
| undefined, | ||
| ), | ||
| detectedDelimiter: ",", |
There was a problem hiding this comment.
In the raw-format path, hasHeaders is computed via (limitedLines[0] || "").split(",") and detectedDelimiter is hardcoded to ",". This bypasses proper CSV parsing (quotes/escaped commas) and ignores sep=... metadata lines (which are detected elsewhere). Since you already parse limitedCSV into sampleForAnalysis, consider deriving headers (and delimiter, if supported) from the parser result instead of manual splitting, or rename the field to reflect that it’s an assumed delimiter.
| // Parse a sample for enhanced metadata analysis (raw format still benefits from column analysis) | ||
| const sampleForAnalysis = await this.parseCSVString( | ||
| limitedCSV, | ||
| Math.min(rowCount, 500), | ||
| ); |
There was a problem hiding this comment.
The PR title/description focuses on standardizing provider errors, but this change also adds substantial new CSV column/type analysis + data-quality scoring (and a large new test suite). Please either update the PR title/description to reflect the CSV feature, or split the CSV work into a separate PR to keep scope and risk contained.
1b748a6 to
d1923a2
Compare
|
Someone is attempting to deploy a commit to the Sachin Sharma's projects Team on Vercel. A member of the Team first needs to authorize it. |
All providers now use typed ProviderError subclasses (AuthenticationError, RateLimitError, NetworkError, InvalidModelError, AuthorizationError) instead of plain Error objects. This enables programmatic error handling and consistent error classification across all 13 providers. Changes: - Added detectProviderError() utility in errors.ts for automatic error type detection - Updated 9 providers to throw typed errors: AnthropicBase, LiteLLM, Bedrock, Azure, Vertex, Mistral, Ollama, OpenRouter, HuggingFace - All handleProviderError() methods now use throw instead of return - Provider-specific error patterns preserved with helpful error messages Fixes bug where different providers returned inconsistent error formats. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
a0af671 to
cf5f164
Compare
09ff4a5 to
405e3e5
Compare
|
Closing as part of project audit (2026-03-29). The feature this PR targets was implemented via a different approach in a later release. See docs/project-audit-2026-03-29.md for details. |
Pull Request
Description
What does this PR do?
A clear and concise description of the changes in this pull request.
Related Issues
Does this PR close any issues?
Fixes #(issue number)
Closes #(issue number)
Relates to #(issue number)
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
Provide context for reviewers:
Changes Made
What specific changes were made?
Provide a bullet-point list of the key changes:
Breaking Changes
Does this PR introduce breaking changes?
If yes, describe:
Testing
How has this been tested?
Please describe the tests you ran and their results:
Test Coverage
Manual Testing Steps
Provide steps for manual testing:
Code Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionExample:
feat(providers): add support for LiteLLM proxyDependencies
Does this PR add, update, or remove dependencies?
If yes, list dependencies and justification:
Performance Impact
Does this change affect performance?
If applicable, provide benchmark results:
Security Considerations
Are there any security implications?
If applicable, describe:
Deployment Notes
Special deployment instructions?
Screenshots / Videos
If applicable, add screenshots or videos to demonstrate changes:
[Add screenshots or videos here]
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
[Add any extra context, concerns, or questions here]
Pre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes