fix(bedrock): migrate from ai-sdk to native AWS SDK implementation - #134
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughRemoves multiple compatibility/design docs and AWS credential test suites; adds hpagent dependency. Overhauls Amazon Bedrock provider to use AWS SDK directly with health checks, streaming, and tool execution; adjusts provider registry signature. Tweaks conversation summarization inputs/prompt. Ensures explicit provider selection bypasses health gating. Deletes AWS credential provider/tester modules. Minor docs formatting/content additions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Registry as ProviderRegistry
participant Bedrock as AmazonBedrockProvider
participant AWS as AWS Bedrock SDK
App->>Registry: getProvider("bedrock", model)
Registry->>Bedrock: new AmazonBedrockProvider(model, sdk)
Bedrock->>AWS: performInitialHealthCheck() [ListFoundationModels]
AWS-->>Bedrock: Health result (ok/error)
Bedrock-->>Registry: instance
App->>Bedrock: generate(prompt/options)
alt Streaming enabled
Bedrock->>AWS: ConverseStream (messages, tools)
AWS-->>Bedrock: Stream events (content, toolUse, stop)
Bedrock-->>App: Async stream (tokens/tool calls)
else Non-streaming
Bedrock->>AWS: Converse (messages, tools)
AWS-->>Bedrock: Response (content/toolUse)
Bedrock-->>App: EnhancedGenerateResult
end
sequenceDiagram
autonumber
actor App
participant Utils as providerUtils.getBestProvider
participant Health as ProviderHealth
App->>Utils: getBestProvider(requestedProvider="bedrock")
Utils->>Health: check("bedrock") (log only)
Health-->>Utils: healthy/unhealthy/error
note right of Utils: Explicit provider always returned\nregardless of health status
Utils-->>App: "bedrock"
sequenceDiagram
autonumber
actor App
participant Mem as ConversationMemoryManager
participant Summ as Summarizer
App->>Mem: storeConversationTurn(msg)
Mem->>Mem: check threshold/max tokens
alt Needs summarization
Mem->>Summ: summarize({provider, model, disableTools}, prompt)
Summ-->>Mem: summary text
Mem->>Mem: replace history with [system summary + recent]
else No summarization
Mem->>Mem: keep messages
end
Mem-->>App: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull Request Overview
This pull request migrates AWS Bedrock provider from the AI SDK to a native AWS SDK implementation. The change removes external AI SDK dependencies and implements conversation management using native AWS Bedrock Converse API with streaming support and permission fallback.
Key changes:
- Replace @ai-sdk/amazon-bedrock with direct @aws-sdk/client-bedrock-runtime integration
- Remove custom AWS credential providers and authentication modules
- Implement native conversation management with tool support
Reviewed Changes
Copilot reviewed 38 out of 40 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/providers/amazonBedrock.ts | Complete rewrite to use native AWS SDK with conversation management |
| src/lib/providers/aws/credentialProvider.ts | Removed custom credential provider implementation |
| src/lib/providers/aws/credentialTester.ts | Removed AWS credential testing utilities |
| test/providers/aws/authentication.test.ts | Removed AWS authentication test suite |
| test/providers/aws/credentialSources.test.ts | Removed credential sources compatibility tests |
| src/lib/factories/providerRegistry.ts | Updated constructor call to remove credential config parameter |
| package.json | Added hpagent dependency |
| src/lib/utils/providerUtils.ts | Enhanced provider selection logic with better explicit provider handling |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
package.json (2)
139-139: Remove@ai-sdk/amazon-bedrockfrom all code and tests
- In
package.json, drop the line- "@ai-sdk/amazon-bedrock": "^1.0.0",- In
tools/testing/providerValidator.js, remove or replace the mappingso tests don’t reference the removed SDK.- "aws-bedrock": "@ai-sdk/amazon-bedrock",- In
neurolink-demo/package.json, remove- "@ai-sdk/amazon-bedrock": "^2.2.10",Lockfile entries will update automatically on next install.
124-137: Add missing “./dynamic-models” export in package.json
The docs import from “@juspay/neurolink/dynamic-models” but that subpath isn’t listed underexports. In package.json, underexports, add:"exports": { ".": { "types": "./dist/index.d.ts", "svelte": "./dist/index.js", "import": "./dist/index.js", "default": "./dist/index.js" }, + "./dynamic-models": { + "types": "./dist/dynamic-models.d.ts", + "import": "./dist/dynamic-models.js", + "default": "./dist/dynamic-models.js" + }, "./package.json": "./package.json", "./cli": { "types": "./dist/cli/index.d.ts", "import": "./dist/cli/index.js", "default": "./dist/cli/index.js" } },docs/advanced/dynamic-models.md (1)
25-31: Update docs import to use main package entryThe subpath “@juspay/neurolink/dynamic-models” isn’t exported in package.json. Change the import in docs/advanced/dynamic-models.md to:
-import { dynamicModelProvider } from "@juspay/neurolink/dynamic-models"; +import { dynamicModelProvider } from "@juspay/neurolink";src/lib/core/types.ts (2)
66-71: Don’t hardcode an account-scoped ARN for Claude 3.7 Sonnet; use the inference profile ID instead.
BedrockModels.CLAUDE_3_7_SONNETcurrently embeds aninference-profileARN with a specific AWS account (225681119357). Inference profile ARNs are account-scoped; hardcoding someone else’s account will fail for most users. Bedrock’s Converse API accepts the inference profile ID (e.g.,us.anthropic.claude-3-7-sonnet-20250219-v1:0) as themodelId, or an ARN in your own account. Prefer the ID to keep this enum portable. (aws.amazon.com, docs.aws.amazon.com)Apply this minimal fix:
export enum BedrockModels { CLAUDE_3_SONNET = "anthropic.claude-3-sonnet-20240229-v1:0", CLAUDE_3_HAIKU = "anthropic.claude-3-haiku-20240307-v1:0", CLAUDE_3_5_SONNET = "anthropic.claude-3-5-sonnet-20240620-v1:0", - CLAUDE_3_7_SONNET = "arn:aws:bedrock:us-east-2:225681119357:inference-profile/us.anthropic.claude-3-7-sonnet-20250219-v1:0", + // Use inference profile ID (works with Converse API as modelId) + CLAUDE_3_7_SONNET = "us.anthropic.claude-3-7-sonnet-20250219-v1:0", }If you want to support both foundation model IDs and inference profile IDs, consider branding the type or adding a runtime discriminator instead of mixing ARNs and IDs in a single enum. (aws.amazon.com)
294-307: PassinferenceProfileArninto allConversecalls in AmazonBedrockProvider.
In src/lib/providers/amazonBedrock.ts, bothConverseCommandInput(around line 271) andConverseStreamCommandInput(around line 1014) only setmodelId,messages, andsystem. You must addinferenceProfileArn: this.options.inferenceProfileArn,to each command input object so on-demand models (e.g. CLAUDE_3_7_SONNET) invoke via their inference profiles. Region routing and use of
Converse(noInvokeModel) are already correct.src/lib/utils/providerUtils.ts (3)
138-153: Ollama availability check ignores configured model.Hard-coding llama3.2:latest rejects working setups using a different OLLAMA_MODEL.
- const defaultOllamaModel = "llama3.2:latest"; - return models.some((m: UnknownRecord) => m.name === defaultOllamaModel); + const want = + process.env.OLLAMA_MODEL || "llama3.2:latest"; + return models.some((m: UnknownRecord) => m.name === want);
155-161: Health probe uses outdated generate() signature and may incur unintended costs.Most providers now expect { input: { text } }. Also add a short timeout and disable tools to avoid file/network side-effects.
- const provider = await AIProviderFactory.createProvider(providerName); - await provider.generate({ prompt: "test", maxTokens: 1 }); + const provider = await AIProviderFactory.createProvider(providerName); + await provider.generate({ + input: { text: "healthcheck" }, + maxTokens: 1, + disableTools: true, + timeoutMs: 2000, + });
466-473: Bedrock env gating conflicts with new AWS SDK default credential chain.Requiring AWS_ACCESS_KEY_ID/SECRET prevents role-based creds (SSO, EC2/ECS roles, shared config), undermining this PR’s “native AWS SDK” objective.
- case "aws": - return !!( - process.env.AWS_ACCESS_KEY_ID && process.env.AWS_SECRET_ACCESS_KEY - ); + case "aws": + // Do not gate on env vars; AWS SDK will resolve from default chain. + return true;Optionally, rely on ProviderHealthChecker for actual auth checks.
docs/demos/interactive.md (1)
9-24: External demo links currently unreachable (all returning status 000); add an offline fallback note or correct the URLs before publishing.docs/analysis/VERIFICATION_RESULTS.md (1)
29-30: Remove personal/local filesystem paths from docs.Absolute paths (e.g., /Users/sachinsharma/...) leak PII and environment details. Replace with repo-relative paths.
-**File Path**: `/Users/sachinsharma/Developer/temp/neurolink-fork/neurolink/README.md` +**File Path**: `README.md`Apply similarly across the file.
Also applies to: 111-112, 241-243, 340-341, 405-406, 489-490, 571-574
🧹 Nitpick comments (47)
package.json (2)
176-176: Remove unusedhpagentdependency
Search across the codebase (including AWS SDK configurations) shows no imports or references tohpagent,HttpProxyAgent, orHttpsProxyAgent. Drop"hpagent": "^1.2.0"frompackage.json(and runpnpm installto prune it frompnpm-lock.yaml).
152-154: Remove unused AWS SDK credential provider package
package.json:152–154 – no static imports of either credential provider were found; drop the unneeded dependency (e.g. remove@aws-sdk/credential-providersif you only require the Node provider).docs/development/package-overrides.md (1)
11-16: Minor grammar fix for clarityTweak sentence for correctness.
Apply:
- - Should be removed when dependencies update to safer versions + - Should be removed when dependencies are updated to safer versionsdocs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md (1)
235-239: Grammar nit: add missing article.Tiny wording fix improves clarity.
- - [ ] Ensure context data flows to analytics system + - [ ] Ensure context data flows to the analytics systemdocs/reference/troubleshooting.md (1)
823-829: Expand allowlist for Bedrock to match AWS SDK/Converse endpoints.Bedrock runtime calls hit region-scoped domains; include control plane and runtime (and agents if used).
- - `bedrock.amazonaws.com` (Bedrock) + - `bedrock.<region>.amazonaws.com` (Bedrock control plane) + - `bedrock-runtime.<region>.amazonaws.com` (Bedrock runtime / Converse API) + - `bedrock-agent-runtime.<region>.amazonaws.com` (Anthropic/Amazon Agents; if applicable)docs/MCP-CONFIGURATION-LOCATIONS.md (1)
124-126: Hyphenate compound modifier for clarityChange “Project/workspace specific” to “Project/workspace-specific” for grammatical correctness; optional to keep others as-is.
- - Project/workspace specific configs + - Project/workspace-specific configsdocs/TESTING.md (3)
246-257: Fix nested list indentation under numbered itemsIndent sub-bullets by 4 spaces to render consistently across CommonMark parsers.
- - Check model name in .env file - - Use `gemini-2.5-pro` instead of deprecated models - - Verify API key is valid + - Check model name in .env file + - Use `gemini-2.5-pro` instead of deprecated models + - Verify API key is valid
253-257: Same indentation issue (NaN Token Counts)Apply 4-space indent to these sub-bullets too.
- - Usually indicates provider API failure - - Check model configuration and API keys - - Test with `--debug` flag for detailed logs + - Usually indicates provider API failure + - Check model configuration and API keys + - Test with `--debug` flag for detailed logs
259-263: Same indentation issue (Enhancement Data Missing)Use 4-space indent for consistent rendering.
- - Ensure using `--debug` flag to see enhancement output - - Verify enhancement flags are correctly specified - - Check that provider is working (not falling back) + - Ensure using `--debug` flag to see enhancement output + - Verify enhancement flags are correctly specified + - Check that provider is working (not falling back)docs/demos/screenshots.md (4)
155-160: Normalize nested list indentation under “Quick Start Workflow”Indent sub-bullets by 4 spaces to ensure proper nesting.
- - Initial setup and configuration - - First generation command - - Provider status verification + - Initial setup and configuration + - First generation command + - Provider status verification
161-166: Same indentation fix under “Batch Processing”- - Multiple prompt processing - - Performance comparison - - Results compilation + - Multiple prompt processing + - Performance comparison + - Results compilation
175-179: Same indentation fix under “VS Code Integration”- - Extension interface - - Code generation in editor - - MCP server discovery + - Extension interface + - Code generation in editor + - MCP server discovery
181-185: Same indentation fix under “Terminal Workflows”- - Command completion - - Real-time streaming - - Error handling examples + - Command completion + - Real-time streaming + - Error handling examplessrc/lib/utils/conversationMemoryUtils.ts (3)
40-47: Stronger type guard for options.context before Record castGuard that context is a non-null object to avoid edge-case throws if a non-object sneaks in.
- if (!conversationMemory || !options.context) { + if (!conversationMemory || !options.context) { return []; } - const sessionId = (options.context as Record<string, unknown>)?.sessionId; + if (typeof options.context !== "object" || options.context === null) { + return []; + } + const sessionId = (options.context as Record<string, unknown>)?.sessionId;
80-87: Repeat context object guard for store pathMirror the stricter guard here to prevent type issues.
- const context = originalOptions.context as Record<string, unknown>; + if (typeof originalOptions.context !== "object" || originalOptions.context === null) { + return; + } + const context = originalOptions.context as Record<string, unknown>;
89-102: Use a single prompt variable for storage and loggingEnsures logged length matches what’s stored.
- await conversationMemory.storeConversationTurn( + const promptUsed = + originalOptions.originalPrompt ?? originalOptions.prompt ?? ""; + await conversationMemory.storeConversationTurn( sessionId, userId, - originalOptions.originalPrompt || originalOptions.prompt || "", + promptUsed, result.content, ); logger.debug("Conversation turn stored", { sessionId, userId, - promptLength: originalOptions.prompt?.length || 0, + promptLength: promptUsed.length, responseLength: result.content.length, });docs/development/cli-factory-impact-assessment.md (2)
197-208: Nested list indentation under numbered sectionsIndent sub-bullets by 4 spaces for consistent rendering.
- - All existing commands work identically - - Flag compatibility maintained - - Output formats preserved + - All existing commands work identically + - Flag compatibility maintained + - Output formats preserved
204-240: Apply same 4-space indent across all “Test Coverage Areas” subsectionsRepeat the indentation fix for sections 2–8 to avoid renderer-specific list glitches.
docs/getting-started/environment-variables.md (2)
301-306: Align Bedrock ARN example with configured regionThe ARN uses us-east-2 while the required variables section sets AWS_REGION to us-east-1. This will confuse users. Align the example with the region above.
- BEDROCK_MODEL="arn:aws:bedrock:us-east-2:<account_id>:inference-profile/us.anthropic.claude-3-7-sonnet-20250219-v1:0" + BEDROCK_MODEL="arn:aws:bedrock:us-east-1:<account_id>:inference-profile/us.anthropic.claude-3-7-sonnet-20250219-v1:0"Also, please confirm whether <account_id> should be the caller’s account or AWS-managed account for inference profiles in your flow; add a one-line note if needed.
292-296: Call out AWS SDK default credential chain (keys optional in many deployments)Since the PR migrates to native AWS SDK, add a short NOTE stating that the SDK automatically uses the default credential chain (IAM role for EC2/Lambda/ECS/EKS, SSO, shared config/credentials, environment variables). Recommend roles over long‑lived keys in production.
docs/DYNAMIC-MODELS.md (1)
231-236: Expose cache TTL as an environment variableYou mention a 5‑minute cache here. Consider documenting a MODEL_CONFIG_CACHE_TTL (seconds or ms) env to tune this without code changes, and note its default.
docs/test-reports/phase-1-2-completion-report.md (1)
61-65: Tighten bullet grammar for consistencyAdd terminal periods and keep parallel structure.
- - Multiple language support (JavaScript, TypeScript, Python, Java) - - Framework-specific configurations (Jest, Mocha, Vitest, Pytest) - - Coverage options (comprehensive, edge cases, happy path) + - Multiple language support (JavaScript, TypeScript, Python, Java). + - Framework-specific configurations (Jest, Mocha, Vitest, Pytest). + - Coverage options (comprehensive, edge cases, happy path). - - Multi-goal optimization (readability, maintainability, performance) - - Language-aware refactoring patterns - - Best practices enforcement + - Multi-goal optimization (readability, maintainability, performance). + - Language-aware refactoring patterns. + - Best-practices enforcement. - - Multiple formats (Markdown, JSDoc, Docstring, HTML) - - Audience-specific content generation - - API reference and usage guide options + - Multiple formats (Markdown, JSDoc, Docstring, HTML). + - Audience-specific content generation. + - API reference and usage guide options.Also applies to: 67-71, 73-77
docs/visual-content/phase-1-2-workflow-tools-plan.md (1)
66-68: Plan looks solid; add a quick-start commandConsider adding a one-liner to start the demo server on the documented port and a capture script hint.
1. **Ensure Demo Server Running** - Server should be on port 9876 - All 4 Phase 1.2 tools integrated + - Example: `npm run demo:server -- --port 9876` 3. **Capture Screenshots** - Use browser or Playwright for consistent captures - Save to `docs/visual-content/screenshots/phase-1-2-workflow/` + - Example: `npm run screenshots:phase1-2`Also applies to: 71-73, 76-78, 81-83
docs/visual-content/phase-1-2-visual-content-achievement.md (1)
16-19: Nice elaborations; minor punctuation for polishEnd sub-bullets with periods for consistency.
- - Shows all 4 tools in professional grid layout - - Displays performance metrics (100% test coverage, <1ms execution) - - Green theme highlighting Phase 1.2 distinction + - Shows all 4 tools in professional grid layout. + - Displays performance metrics (100% test coverage, <1ms execution). + - Green theme highlighting Phase 1.2 distinction. - - JavaScript function example with discount calculation - - Framework selection showing Jest, Mocha, Vitest, Pytest - - Coverage type options (comprehensive, edge cases, happy path) + - JavaScript function example with discount calculation. + - Framework selection showing Jest, Mocha, Vitest, Pytest. + - Coverage type options (comprehensive, edge cases, happy path). - - Original code snippet being refactored - - Multi-goal optimization checkboxes (readability, maintainability, performance) - - Successful refactoring output displayed + - Original code snippet being refactored. + - Multi-goal optimization checkboxes (readability, maintainability, performance). + - Successful refactoring output displayed. - - UserAuthentication class being documented - - Documentation type and format selection - - Generated JSDoc output with comprehensive details + - UserAuthentication class being documented. + - Documentation type and format selection. + - Generated JSDoc output with comprehensive details. - - React component debugging scenario - - Analysis depth options (detailed, quick, comprehensive) - - Issues and recommendations displayed + - React component debugging scenario. + - Analysis depth options (detailed, quick, comprehensive). + - Issues and recommendations displayed. - - Tabbed interface showing 5-step workflow - - Original code → Refactor → Document → Test → Debug - - All tools working together seamlessly + - Tabbed interface showing 5‑step workflow. + - Original code → Refactor → Document → Test → Debug. + - All tools working together seamlessly.Also applies to: 22-25, 28-31, 34-37, 40-43, 46-49
docs/tracking/IMMEDIATE_WORK_PLAN.md (2)
81-85: Add bounded parallelism to avoid provider stampede.Promise.allSettled over many providers can spike outbound calls. Recommend a small concurrency cap (e.g., 3–5) in the snippet.
Example (docs snippet):
-const results = await Promise.allSettled( - providers.map((provider) => checkProviderStatus(provider)), -); +import pLimit from "p-limit"; +const limit = pLimit(5); +const results = await Promise.allSettled( + providers.map((p) => limit(() => checkProviderStatus(p))), +);
126-131: Call out moduleResolution follow-up explicitly.Per team learnings, note the planned TS moduleResolution update (extensionless imports) so perf changes don’t regress NodeNext ESM at runtime.
src/lib/utils/providerUtils.ts (3)
270-296: Treat missing AWS env vars as warnings, not validity failures.AWS creds may come from profiles/roles. Flag as warning but keep result.isValid true if other checks pass.
- if (!accessKeyId) { - result.missingVars.push("AWS_ACCESS_KEY_ID"); - } + if (!accessKeyId) result.warnings.push("AWS_ACCESS_KEY_ID not set (role/profile may supply creds)");Apply similarly for AWS_SECRET_ACCESS_KEY.
224-227: Dead switch branch due to toLowerCase()."azureOpenai" will never match once lowercased. Use "azureopenai".
- case "azureOpenai": + case "azureopenai":
539-561: Provider alias drift (e.g., “googlevertex”) not represented.isValidProvider() misses common aliases used elsewhere (e.g., conversationMemoryManager maps vertex→googlevertex). Centralize alias mapping or extend list.
Proposed: introduce normalizeProviderName(name) and use it across utils, CLI, and summarization.
docs/demos/interactive.md (1)
506-516: Privacy statement: good. Consider rate-limit and data-retention notes.Add a one-liner about abuse limits and ephemeral storage to set expectations.
src/lib/core/conversationMemoryManager.ts (4)
113-116: Exposing internal array invites accidental mutation.Returning the stored array by reference lets callers mutate session state.
- return session ? session.messages : []; + return session ? [...session.messages] : [];
155-167: Hard-coded provider alias risks mismatch (“vertex” → “googlevertex”).Alias differs from the canonical list elsewhere. Centralize normalization (e.g., providerUtils.normalizeProviderName) and validate before use.
- let mappedProvider = providerName; - if (providerName === "vertex") { - mappedProvider = "googlevertex"; - } + const mappedProvider = normalizeProviderName(providerName); + if (!isValidProvider(mappedProvider)) { + logger.error(`[ConversationMemory] Unknown summarization provider: ${String(providerName)}`); + return; + }(Assumes adding normalizeProviderName/isValidProvider import.)
169-175: New NeuroLink instance per summarization may be costly.Consider reusing a single summarizer instance or injecting one to avoid repeated provider registry setup.
251-255: Stats: totalTurns skewed by system summary message.Compute turns from user messages for accuracy.
- const totalTurns = sessions.reduce( - (sum, session) => sum + session.messages.length / MESSAGES_PER_TURN, - 0, - ); + const totalTurns = sessions.reduce( + (sum, s) => sum + s.messages.filter(m => m.role === "user").length, + 0, + );docs/analysis/VERIFICATION_RESULTS.md (1)
1-20: Scope/claims in verification doc are great—mark as internal or preface with “work-in-progress”.Prevents external readers mistaking gaps for regressions if this lands in a public release.
src/lib/providers/amazonBedrock.ts (12)
232-239: Remove duplicate error log in conversationLoop catchSame error is logged twice; keep one to reduce noise.
- logger.error( - `[AmazonBedrockProvider] Error in conversation loop:`, - error, - ); - logger.error( - `[AmazonBedrockProvider] Error in conversation loop:`, - error, - ); + logger.error( + `[AmazonBedrockProvider] Error in conversation loop:`, + error, + );
750-759: Downgrade TRACE logs from error→debug (avoid log noise and false-alerting)TRACE markers are logged with
logger.error, which will pollute error channels. Usedebugfor these traces; keep real errors aserror.-logger.error("🟢 [TRACE] executeStream ENTRY - starting streaming attempt"); +logger.debug("🟢 [TRACE] executeStream ENTRY - starting streaming attempt"); ... -logger.error("🟢 [TRACE] executeStream TRY block - about to call streamingConversationLoop"); +logger.debug("🟢 [TRACE] executeStream TRY block - about to call streamingConversationLoop"); ... -logger.error("🟢 [TRACE] executeStream - calling streamingConversationLoop NOW"); +logger.debug("🟢 [TRACE] executeStream - calling streamingConversationLoop NOW"); ... -logger.error("🟢 [TRACE] executeStream - streamingConversationLoop SUCCESS, returning result"); +logger.debug("🟢 [TRACE] executeStream - streamingConversationLoop SUCCESS, returning result"); ... -logger.error("🔴 [TRACE] executeStream CATCH - error caught from streamingConversationLoop"); +logger.debug("🔴 [TRACE] executeStream CATCH - error caught from streamingConversationLoop"); ... -logger.error("🔴 [TRACE] executeStream CATCH - checking if permission error"); +logger.debug("🔴 [TRACE] executeStream CATCH - checking if permission error"); ... -logger.error( +logger.debug( "🟡 [TRACE] executeStream CATCH - PERMISSION ERROR DETECTED, starting fallback", ); ... -logger.error("🟦 [TRACE] streamingConversationLoop ENTRY"); +logger.debug("🟦 [TRACE] streamingConversationLoop ENTRY"); ... -logger.error("🟦 [TRACE] streamingConversationLoop - ReadableStream start() called"); +logger.debug("🟦 [TRACE] streamingConversationLoop - ReadableStream start() called"); ... -logger.error( +logger.debug( "🔴 [TRACE] streamingConversationLoop - CATCH block hit in ReadableStream", ); ... -logger.error( +logger.debug( "🔴 [TRACE] streamingConversationLoop - first streaming call FAILED, throwing", );Also applies to: 775-783, 784-805, 807-866, 874-890, 895-899, 943-946, 957-961
210-213: Guard verbose payload logging; avoid leaking prompts/responses at higher log levelsFull JSON responses and raw prompts can contain sensitive data. Keep them only at
debugand consider redacting long texts before logging.Also applies to: 771-773
111-120: Close BedrockClient used for health probe to free socketsThe one-off
BedrockClientinperformInitialHealthCheckis never destroyed. Calldestroy()after use.- await bedrockClient.send(command); + await bedrockClient.send(command); const responseTime = Date.now() - startTime; logger.debug( `[AmazonBedrockProvider] Health check PASSED - credentials valid, connectivity good, responseTime: ${responseTime}ms`, ); - } catch (error) { + } catch (error) { ... - } + } finally { + try { bedrockClient.destroy(); } catch {} + }Also applies to: 125-141
1291-1301: Also destroy the healthCheckClient in checkBedrockHealth()Avoid leaving the client open after the probe.
- const response = await healthCheckClient.send(command, { + const response = await healthCheckClient.send(command, { abortSignal: controller.signal, }); ... - } finally { - clearTimeout(timeoutId); - } + } finally { + clearTimeout(timeoutId); + try { healthCheckClient.destroy(); } catch {} + }Also applies to: 1345-1346
278-282: Honor caller-provided options (temperature, maxTokens, systemPrompt) instead of hardcoded defaultsBoth sync and streaming paths hardcode
maxTokens: 4096andtemperature: 0.7. Wire through values fromTextGenerationOptions/StreamOptionsto keep behavior consistent with other providers.Also applies to: 1019-1025
525-526: Handle unknown stop reasons gracefullyThrowing on unknown
stopReasoncan break for newly introduced Bedrock reasons (e.g., guardrail interventions). Treat as end-of-turn with logging.- } else { - throw new Error(`Unknown stop reason: ${stopReason}`); - } + } else { + logger.warn(`[AmazonBedrockProvider] Unrecognized stop reason "${stopReason}", ending conversation.`); + return { shouldContinue: false, text: "" }; + }
256-263: Region logging may print a function; resolve provider if needed
this.bedrockClient.config.regioncan be a provider. Consider resolving it for accurate logs.Example:
const region = typeof this.bedrockClient.config.region === "function" ? await this.bedrockClient.config.region() : this.bedrockClient.config.region; logger.info(`🔧 [AmazonBedrockProvider] Client region: ${region}`);
651-680: Tool schema conversion is a stub; convert Zod → JSON Schema properlyChecking
"_def"and returning an empty schema drops parameter validation. Usezod-to-json-schema(or your existing utility) to preserve shape and required fields.
894-899: Unused “firstChunkProcessed” flagThe variable is never set to true and provides no guard; remove it to simplify.
- const firstChunkProcessed = false; + // (removed) no longer needed
824-857: Node stream types: consider importing from node:stream/web for portabilityIf
domlibs are not guaranteed in tsconfig, importReadableStreamfromnode:stream/weband avoid annotatingReadableStreamDefaultControllerexplicitly (useanyor the generic).Also applies to: 894-949, 964-984, 1050-1054
368-381: Sanitize environment diagnostics for secretsGood that you only print set/unset. Consider also hiding
AWS_PROFILEvalue in production logs to avoid indirect account leakage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
BEDROCK_MCP_CONNECTOR_COMPLETE_ANALYSIS.md(0 hunks)COMPREHENSIVE_COMPATIBILITY_MATRIX.md(0 hunks)NEUROLINK_BEDROCK_COMPATIBILITY_ANALYSIS.md(0 hunks)docs/CONTEXT-SUMMARIZATION.md(1 hunks)docs/DYNAMIC-MODELS.md(1 hunks)docs/MCP-CONFIGURATION-LOCATIONS.md(1 hunks)docs/PERFORMANCE-OPTIMIZATION.md(1 hunks)docs/TESTING.md(1 hunks)docs/TROUBLESHOOTING.md(1 hunks)docs/advanced/dynamic-models.md(1 hunks)docs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md(3 hunks)docs/analysis/VERIFICATION_RESULTS.md(7 hunks)docs/demos/interactive.md(4 hunks)docs/demos/screenshots.md(2 hunks)docs/development/cli-factory-impact-assessment.md(1 hunks)docs/development/package-overrides.md(1 hunks)docs/development/testing.md(1 hunks)docs/getting-started/environment-variables.md(4 hunks)docs/reference/troubleshooting.md(2 hunks)docs/test-reports/phase-1-2-completion-report.md(1 hunks)docs/test-reports/visual-content-documentation-update-summary.md(1 hunks)docs/tracking/CLI_OPTIMIZATION_TRACKING.md(2 hunks)docs/tracking/IMMEDIATE_WORK_PLAN.md(6 hunks)docs/visual-content/phase-1-2-visual-content-achievement.md(1 hunks)docs/visual-content/phase-1-2-workflow-tools-plan.md(1 hunks)examples/sagemaker/README.md(1 hunks)package.json(1 hunks)src/lib/core/conversationMemoryManager.ts(6 hunks)src/lib/core/types.ts(1 hunks)src/lib/factories/providerRegistry.ts(0 hunks)src/lib/providers/amazonBedrock.ts(1 hunks)src/lib/providers/aws/credentialProvider.ts(0 hunks)src/lib/providers/aws/credentialTester.ts(0 hunks)src/lib/types/generateTypes.ts(1 hunks)src/lib/utils/conversationMemoryUtils.ts(1 hunks)src/lib/utils/logger.ts(1 hunks)src/lib/utils/providerUtils.ts(2 hunks)test/providers/aws/authentication.test.ts(0 hunks)test/providers/aws/credentialSources.test.ts(0 hunks)
💤 Files with no reviewable changes (8)
- src/lib/providers/aws/credentialTester.ts
- NEUROLINK_BEDROCK_COMPATIBILITY_ANALYSIS.md
- BEDROCK_MCP_CONNECTOR_COMPLETE_ANALYSIS.md
- test/providers/aws/authentication.test.ts
- src/lib/factories/providerRegistry.ts
- src/lib/providers/aws/credentialProvider.ts
- COMPREHENSIVE_COMPATIBILITY_MATRIX.md
- test/providers/aws/credentialSources.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T06:38:07.850Z
Learnt from: sinha-sahil
PR: juspay/neurolink#81
File: todos/refactor/01-global-imports.md:10-22
Timestamp: 2025-08-19T06:38:07.850Z
Learning: The team plans to update TypeScript's moduleResolution settings to support extensionless imports across the codebase, addressing potential NodeNext ESM runtime issues during the refactor process.
Applied to files:
docs/tracking/IMMEDIATE_WORK_PLAN.md
🧬 Code graph analysis (3)
src/lib/utils/providerUtils.ts (1)
src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)
src/lib/providers/amazonBedrock.ts (5)
src/lib/neurolink.ts (2)
neurolink(4879-4879)stream(2322-2390)src/lib/core/types.ts (2)
TextGenerationOptions(177-210)EnhancedGenerateResult(217-220)src/lib/types/tools.ts (2)
ToolArgs(33-38)ToolDefinition(97-105)src/lib/types/streamTypes.ts (2)
StreamOptions(74-132)StreamResult(138-168)src/lib/core/baseProvider.ts (1)
stream(114-232)
src/lib/core/conversationMemoryManager.ts (3)
src/lib/config/conversationMemoryConfig.ts (2)
DEFAULT_MAX_TURNS_PER_SESSION(11-11)MESSAGES_PER_TURN(21-21)src/lib/utils/logger.ts (2)
logger(341-380)error(223-225)src/lib/types/conversationTypes.ts (1)
ChatMessage(81-87)
🪛 LanguageTool
docs/development/package-overrides.md
[grammar] ~14-~14: There might be a mistake here.
Context: ... (potential code injection during build) - Should be removed when dependencies upda...
(QB_NEW_EN)
docs/tracking/IMMEDIATE_WORK_PLAN.md
[grammar] ~128-~128: There might be a mistake here.
Context: ... Loading**: - Analyze which modules take longest to load - Identify unnecessa...
(QB_NEW_EN)
[grammar] ~129-~129: There might be a mistake here.
Context: ... load - Identify unnecessary imports in CLI startup path - Identify modules ...
(QB_NEW_EN)
docs/development/testing.md
[grammar] ~250-~250: There might be a mistake here.
Context: ...-pro` instead of deprecated models - Verify API key is valid 2. **NaN Token Counts...
(QB_NEW_EN)
docs/demos/screenshots.md
[grammar] ~157-~157: There might be a mistake here.
Context: ...** - Initial setup and configuration - First generation command - Provider status...
(QB_NEW_EN)
[grammar] ~158-~158: There might be a mistake here.
Context: ...figuration - First generation command - Provider status verification 2. **Batch...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ... Integration** - Extension interface - Code generation in editor - MCP serve...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...interface - Code generation in editor - MCP server discovery 2. **Terminal Work...
(QB_NEW_EN)
docs/MCP-CONFIGURATION-LOCATIONS.md
[grammar] ~124-~124: There might be a mistake here.
Context: ... - Project/workspace specific configs - User/global configs - Default/fallbac...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...pecific configs - User/global configs - Default/fallback configs 3. **Platform ...
(QB_NEW_EN)
docs/test-reports/phase-1-2-completion-report.md
[grammar] ~62-~62: There might be a mistake here.
Context: ...t (JavaScript, TypeScript, Python, Java) - Framework-specific configurations (Jest,...
(QB_NEW_EN)
[grammar] ~63-~63: There might be a mistake here.
Context: ...igurations (Jest, Mocha, Vitest, Pytest) - Coverage options (comprehensive, edge ca...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...adability, maintainability, performance) - Language-aware refactoring patterns -...
(QB_NEW_EN)
[grammar] ~69-~69: There might be a mistake here.
Context: ... - Language-aware refactoring patterns - Best practices enforcement 3. **generat...
(QB_NEW_EN)
examples/sagemaker/README.md
[grammar] ~326-~326: There might be a mistake here.
Context: ...erify endpoint name spelling - Check if endpoint is in the correct region - ...
(QB_NEW_EN)
[grammar] ~333-~333: There might be a mistake here.
Context: ...issions - Check AWS credentials - Ensure endpoint allows access from your accoun...
(QB_NEW_EN)
docs/demos/interactive.md
[grammar] ~33-~33: There might be a mistake here.
Context: ...t Generation** - Simple prompt input - Provider selection - Response analysi...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...ple prompt input - Provider selection - Response analysis 2. **Advanced Feature...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...ive Dashboard** - Strategic analysis - Performance reporting - Decision supp...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...egic analysis - Performance reporting - Decision support 2. **Marketing Workflo...
(QB_NEW_EN)
[grammar] ~283-~283: There might be a mistake here.
Context: ... Beginner Level - Basic concepts - Simple examples - Guided exercises 2...
(QB_NEW_EN)
[grammar] ~284-~284: There might be a mistake here.
Context: ... - Basic concepts - Simple examples - Guided exercises 2. **Intermediate Leve...
(QB_NEW_EN)
[grammar] ~468-~468: There might be a mistake here.
Context: ...Your Path** - Quick demo (5 minutes) - Full tutorial (30 minutes) - Specific...
(QB_NEW_EN)
[grammar] ~469-~469: There might be a mistake here.
Context: ...minutes) - Full tutorial (30 minutes) - Specific use case 2. **No Setup Require...
(QB_NEW_EN)
docs/visual-content/phase-1-2-visual-content-achievement.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...flow tools page - Shows all 4 tools in professional grid layout - Displays ...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...nction example with discount calculation - Framework selection showing Jest, Mocha,...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...tion showing Jest, Mocha, Vitest, Pytest - Coverage type options (comprehensive, ed...
(QB_NEW_EN)
[grammar] ~34-~34: There might be a mistake here.
Context: ...serAuthentication class being documented - Documentation type and format selection ...
(QB_NEW_EN)
[grammar] ~35-~35: There might be a mistake here.
Context: ... Documentation type and format selection - Generated JSDoc output with comprehensiv...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...options (detailed, quick, comprehensive) - Issues and recommendations displayed 6....
(QB_NEW_EN)
docs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md
[grammar] ~236-~236: There might be a mistake here.
Context: ...xt` option support (VERIFIED as missing) - [ ] Ensure context data flows to analyti...
(QB_NEW_EN)
[grammar] ~237-~237: There might be a mistake here.
Context: ...e context data flows to analytics system - [ ] Update help documentation 2. **Fix ...
(QB_NEW_EN)
docs/TESTING.md
[grammar] ~250-~250: There might be a mistake here.
Context: ...-pro` instead of deprecated models - Verify API key is valid 2. **NaN Token Counts...
(QB_NEW_EN)
docs/development/cli-factory-impact-assessment.md
[grammar] ~211-~211: There might be a mistake here.
Context: ...2 tests) - Context parameter support - Invalid context error handling 4. **Out...
(QB_NEW_EN)
🪛 GitHub Check: 🛡️ Code Quality & Security Gate
src/lib/providers/amazonBedrock.ts
[failure] 29-29:
Cannot find module '@smithy/types' or its corresponding type declarations.
[failure] 8-8:
'ToolConfiguration' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 7-7:
'ConverseStreamCommandInput' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 5-5:
'ConverseCommandOutput' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
[failure] 4-4:
'ConverseCommandInput' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled.
🪛 GitHub Check: test (20)
src/lib/providers/amazonBedrock.ts
[failure] 29-29:
Cannot find module '@smithy/types' or its corresponding type declarations.
🪛 GitHub Check: build-check
src/lib/providers/amazonBedrock.ts
[failure] 29-29:
Cannot find module '@smithy/types' or its corresponding type declarations.
🪛 GitHub Check: test (18)
src/lib/providers/amazonBedrock.ts
[failure] 29-29:
Cannot find module '@smithy/types' or its corresponding type declarations.
🪛 GitHub Actions: CI
src/lib/providers/amazonBedrock.ts
[error] 4-4: TS1484: 'ConverseCommandInput' is a type and must be imported using a type-only import when 'verbatimModuleSyntax' is enabled. (Command: npx tsc --noEmit --strict --project tsconfig.ci.json)
🔇 Additional comments (32)
docs/CONTEXT-SUMMARIZATION.md (1)
79-80: LGTM: readability tweak onlyWhitespace change improves readability; no semantic impact.
docs/test-reports/visual-content-documentation-update-summary.md (1)
46-47: LGTM: formatting-only changeWhitespace addition is harmless and improves list readability.
docs/development/package-overrides.md (1)
18-22: LGTM: content and structureThe override rationale is clear; no semantic changes.
src/lib/core/types.ts (3)
9-12: LGTM on import reformat.Format-only; no API/behavior change.
167-173: StreamingOptions looks fine.Schema is coherent with provider configs; no blockers spotted.
200-206: Conversation memory types import/use is consistent.No functional changes; approve.
examples/sagemaker/README.md (1)
324-337: Spacing tweak improves readability.The added blank lines after issue headings render better in most markdown viewers.
src/lib/types/generateTypes.ts (1)
2-2: Import consolidation only — OK.No semantic change; proceed.
src/lib/utils/logger.ts (1)
411-414: Doc example whitespace — OK.No runtime impact.
docs/TROUBLESHOOTING.md (1)
1067-1067: Minor markdown spacing — OK.The blank line after “allowlist:” reads better.
docs/analysis/MASTER_NEUROLINK_COMPLETE_ANALYSIS.md (3)
241-245: Formatting-only change looks fine.No content issues spotted.
256-260: Checklist section: OK.Reads clearly and matches surrounding style.
271-275: Section header/entries: OK.No edits needed.
docs/tracking/CLI_OPTIMIZATION_TRACKING.md (2)
520-522: Whitespace tweak is fine.Keeps Risk section readable.
529-531: Whitespace tweak is fine.Consistent with adjacent sections.
docs/reference/troubleshooting.md (1)
494-501: Verify Vertex AI Anthropic model identifier.Example uses a likely invalid model name/version. Suggest updating to a known-good identifier; please confirm the current Vertex Anthropic naming.
- npx @juspay/neurolink generate "test" --provider vertex --model claude-sonnet-4@20250514 + # Example (verify against current Vertex Anthropic catalog) + npx @juspay/neurolink generate "test" --provider vertex --model claude-3-5-sonnet@20240620docs/development/testing.md (3)
247-251: Whitespace/readability: OK.List renders cleaner.
253-257: Whitespace/readability: OK.No content changes.
259-263: Whitespace/readability: OK.All good.
docs/PERFORMANCE-OPTIMIZATION.md (2)
508-513: Spacing improvement: OK.Keeps “Common Issues” bullets tidy.
515-520: Spacing improvement: OK.Consistent with preceding section.
docs/MCP-CONFIGURATION-LOCATIONS.md (1)
129-133: List formatting OKBlank line before the platform bullets renders fine; no action needed.
src/lib/utils/conversationMemoryUtils.ts (1)
49-58: buildContextMessages is synchronous; no change required
ConversationMemoryManager.buildContextMessages(sessionId: string) returns ChatMessage[] synchronously; wrapping inawait Promise.resolve(...)is purely optional future-proofing.docs/getting-started/environment-variables.md (1)
546-546: Formatting reads better with the new blank linesMinor spacing improvements help scan-ability. LGTM.
Also applies to: 558-558, 681-681, 687-687, 710-710
docs/DYNAMIC-MODELS.md (1)
20-24: Good additions: server capability + client caching/validation bulletsConcise, useful details. LGTM.
Also applies to: 26-31
docs/test-reports/phase-1-2-completion-report.md (1)
16-19: Verify test-count consistency across docs (36/36 vs 26/26)This report states 36/36 tests passing; the visual achievement doc lists 26/26 in one place. Please reconcile to a single source of truth.
docs/visual-content/phase-1-2-visual-content-achievement.md (1)
50-55: Fix tests-passing count to match the rest of the docsElsewhere you state 36/36 tests passing. Update this metric here or confirm the correct figure.
- - 26/26 Tests Passing status + - 36/36 Tests Passing statusdocs/tracking/IMMEDIATE_WORK_PLAN.md (1)
35-39: Whitespace-only readability tweaks: LGTM.No semantic changes; improved spacing helps scanability.
Also applies to: 68-72, 127-131, 133-137, 156-160, 162-166, 181-185, 194-198
src/lib/utils/providerUtils.ts (1)
140-144: No action needed: Node ≥18.0.0 covers global fetch and AbortSignal.timeout
package.json already specifies"node": ">=18.0.0", and global fetch became stable in Node 18.0.0 (nodejs.org) whileAbortSignal.timeout()was added in v17.3.0 (backported to v16.14.0) (simonplend.com).src/lib/providers/amazonBedrock.ts (3)
154-156: Default model string drift checkEnsure
anthropic.claude-3-sonnet-20240229-v1:0is available in your targeted regions/tenancy. If not, read from env only and fail fast with a helpful error.
651-654: No action needed:aidependency present
Theaipackage is listed in dependencies (v4.3.16) and allimport("ai").Tooltype references resolve correctly.
266-269: No action needed:getAllTools()is already implemented on BaseProvider
The protected asyncgetAllTools()method is defined at src/lib/core/baseProvider.ts:943, so calls in amazonBedrock.ts won’t throw.
- Replace @ai-sdk/amazon-bedrock with direct @aws-sdk/client-bedrock-runtime integration - Remove custom AWS credential provider and authentication logic - Delete redundant AWS credential testing and authentication modules - Implement native Bedrock Converse API with streaming support and permission fallback - Add comprehensive error handling and logging for AWS SDK operations - Remove obsolete MCP connector analysis documentation files - Simplify provider architecture by using AWS SDK's built-in credential handling
05fc681 to
2fbbcbd
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Pull Request
Description
Type of Change
Related Issues
Changes Made
AI Provider Impact
Component Impact
Testing
Test Environment
Performance Impact
Breaking Changes
Screenshots/Demo
Checklist
Additional Notes
Summary by CodeRabbit