bedrock provider cleanup #1276
bedrock provider cleanup #1276
Conversation
|
DescriptionSummary By MatterAI
🔄 What ChangedThis pull request modifies 🔍 Impact of the ChangeThis change significantly improves the cross-runtime compatibility of the utility functions, particularly for environments like Cloudflare Workers where the Node.js 📁 Total Files Changed
🧪 Test AddedN/A 🔒Security VulnerabilitiesN/A MotivationThis PR addresses runtime compatibility issues by ensuring that Node.js-specific modules like Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
Sequence DiagramsequenceDiagram
participant EnvUtil as src/utils/env.ts
participant HonoAdapter as hono/adapter
participant NodeRuntime as Node.js Runtime
participant OtherRuntime as Other Runtime (e.g., Workers)
EnvUtil->>HonoAdapter: getRuntimeKey()
alt Runtime is 'node'
HonoAdapter-->>EnvUtil: 'node'
EnvUtil->>+NodeRuntime: import('path')
NodeRuntime-->>-EnvUtil: path module
else Runtime is not 'node'
HonoAdapter-->>EnvUtil: 'workerd' (or other)
Note over EnvUtil: 'path' remains undefined
end
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | ||
| if (!value || ignore) return value; | ||
|
|
||
| try { | ||
| // Check if value looks like a file path | ||
| if ( | ||
| value.startsWith('/') || | ||
| value.startsWith('./') || | ||
| value.startsWith('../') | ||
| ) { | ||
| // Resolve the path (handle relative paths) | ||
| const resolvedPath = path.resolve(value); | ||
|
|
||
| // Check if file exists | ||
| if (fs.existsSync(resolvedPath)) { | ||
| // File exists, read and return its contents | ||
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | ||
| } | ||
| } | ||
|
|
||
| // If not a file path or file doesn't exist, return value as is | ||
| return value; | ||
| } catch (error: any) { | ||
| console.log(`Error reading file at ${value}: ${error.message}`); | ||
| // Return the original value if there's an error | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security Issue Fix
Issue: Path traversal vulnerability - the function accepts any file path without validation, potentially allowing access to sensitive files outside intended directories
Fix: Add path validation to restrict file access to safe directories
Impact: Prevents potential security breach through malicious file path injection
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | |
| if (!value || ignore) return value; | |
| try { | |
| // Check if value looks like a file path | |
| if ( | |
| value.startsWith('/') || | |
| value.startsWith('./') || | |
| value.startsWith('../') | |
| ) { | |
| // Resolve the path (handle relative paths) | |
| const resolvedPath = path.resolve(value); | |
| // Check if file exists | |
| if (fs.existsSync(resolvedPath)) { | |
| // File exists, read and return its contents | |
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | |
| } | |
| } | |
| // If not a file path or file doesn't exist, return value as is | |
| return value; | |
| } catch (error: any) { | |
| console.log(`Error reading file at ${value}: ${error.message}`); | |
| // Return the original value if there's an error | |
| return value; | |
| } | |
| } | |
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | |
| if (!value || ignore) return value; | |
| try { | |
| // Check if value looks like a file path | |
| if ( | |
| value.startsWith('/') || | |
| value.startsWith('./') || | |
| value.startsWith('../') | |
| ) { | |
| // Resolve the path (handle relative paths) | |
| const resolvedPath = path.resolve(value); | |
| // Security: Validate path is within allowed directories | |
| const allowedPaths = [process.cwd(), '/etc/ssl/certs', '/var/secrets']; | |
| const isPathAllowed = allowedPaths.some(allowedPath => | |
| resolvedPath.startsWith(path.resolve(allowedPath)) | |
| ); | |
| if (!isPathAllowed) { | |
| console.warn(`Access denied to path outside allowed directories: ${resolvedPath}`); | |
| return value; | |
| } | |
| // Check if file exists | |
| if (fs.existsSync(resolvedPath)) { | |
| // File exists, read and return its contents | |
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | |
| } | |
| } | |
| // If not a file path or file doesn't exist, return value as is | |
| return value; | |
| } catch (error: any) { | |
| console.log(`Error reading file at ${value}: ${error.message}`); | |
| // Return the original value if there's an error | |
| return value; | |
| } | |
| } |
| const model = params.foundationModel || params.model || ''; | ||
| if (g1EmbedModels.includes(model)) return undefined; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Potential null/undefined access - params.foundationModel and params.model could be undefined, causing the includes() check to fail
Fix: Add proper null/undefined checks before accessing model properties
Impact: Prevents runtime errors when model parameters are missing
| const model = params.foundationModel || params.model || ''; | |
| if (g1EmbedModels.includes(model)) return undefined; | |
| const model = params?.foundationModel || params?.model || ''; | |
| if (model && g1EmbedModels.includes(model)) return undefined; |
| headers: async ({ providerOptions, fn }) => { | ||
| const { | ||
| apiKey, | ||
| azureExtraParams, | ||
| azureExtraParameters, | ||
| azureDeploymentName, | ||
| azureAdToken, | ||
| azureAuthMode, | ||
| } = providerOptions; |
There was a problem hiding this comment.
📝 Documentation Gap
Issue: Missing JSDoc documentation for the azureExtraParameters field and its purpose
Fix: Add comprehensive documentation explaining the parameter's usage
Impact: Improves code maintainability and developer understanding
| headers: async ({ providerOptions, fn }) => { | |
| const { | |
| apiKey, | |
| azureExtraParams, | |
| azureExtraParameters, | |
| azureDeploymentName, | |
| azureAdToken, | |
| azureAuthMode, | |
| } = providerOptions; | |
| headers: async ({ providerOptions, fn }) => { | |
| const { | |
| apiKey, | |
| /** | |
| * Azure extra parameters configuration | |
| * Controls how extra parameters are handled in the request | |
| * @default 'drop' - drops extra parameters not supported by the model | |
| */ | |
| azureExtraParameters, | |
| azureDeploymentName, | |
| azureAdToken, | |
| azureAuthMode, | |
| } = providerOptions; |
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
narengogi
force-pushed
the
chore/bedrock-cleanup
branch
from
bc360ce to
7a18e60
Compare
| const getMethod = ( | ||
| fn: endpointStrings, | ||
| transformedRequestUrl: string, | ||
| c: Context | ||
| ) => { | ||
| if (fn === 'proxy') { | ||
| return c.req.method; | ||
| } |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Missing null/undefined check for Context parameter - if c is null/undefined, accessing c.req.method will throw a runtime error
Fix: Add null check before accessing Context properties
Impact: Prevents runtime crashes when Context is not properly initialized
| const getMethod = ( | |
| fn: endpointStrings, | |
| transformedRequestUrl: string, | |
| c: Context | |
| ) => { | |
| if (fn === 'proxy') { | |
| return c.req.method; | |
| } | |
| const getMethod = ( | |
| fn: endpointStrings, | |
| transformedRequestUrl: string, | |
| c: Context | |
| ) => { | |
| if (fn === 'proxy') { | |
| return c?.req?.method || 'GET'; | |
| } |
| finalizing_at: new Date(batch.endTime).getTime(), | ||
| expires_at: new Date(batch.jobExpirationTime).getTime(), |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Removed null checks for Date constructor - if batch.endTime or batch.jobExpirationTime are null/undefined, new Date(null) returns invalid date and getTime() returns NaN
Fix: Restore null checks to handle missing timestamps safely
Impact: Prevents NaN values in API responses which could break client applications
| finalizing_at: new Date(batch.endTime).getTime(), | |
| expires_at: new Date(batch.jobExpirationTime).getTime(), | |
| finalizing_at: batch.endTime | |
| ? new Date(batch.endTime).getTime() | |
| : undefined, | |
| expires_at: batch.jobExpirationTime | |
| ? new Date(batch.jobExpirationTime).getTime() | |
| : undefined, |
| usage: { | ||
| prompt_tokens: 0, | ||
| completion_tokens: 0, | ||
| total_tokens: 0, | ||
| prompt_tokens: response.usage.input_tokens, | ||
| completion_tokens: response.usage.output_tokens, | ||
| total_tokens: | ||
| response.usage.input_tokens + response.usage.output_tokens, |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Missing null/undefined check for response.usage - if the usage object is missing, accessing response.usage.input_tokens will throw a runtime error
Fix: Add null check before accessing usage properties
Impact: Prevents runtime crashes when usage data is not provided by the API
| usage: { | |
| prompt_tokens: 0, | |
| completion_tokens: 0, | |
| total_tokens: 0, | |
| prompt_tokens: response.usage.input_tokens, | |
| completion_tokens: response.usage.output_tokens, | |
| total_tokens: | |
| response.usage.input_tokens + response.usage.output_tokens, | |
| usage: { | |
| prompt_tokens: response.usage?.input_tokens || 0, | |
| completion_tokens: response.usage?.output_tokens || 0, | |
| total_tokens: | |
| (response.usage?.input_tokens || 0) + (response.usage?.output_tokens || 0), | |
| }, |
| const getMethod = ( | ||
| fn: endpointStrings, | ||
| transformedRequestUrl: string, | ||
| c: Context | ||
| ) => { | ||
| if (fn === 'proxy') { | ||
| return c.req.method; | ||
| } |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Inconsistent parameter naming - using 'c' for Context parameter while other functions use more descriptive names
Fix: Use consistent parameter naming for better code readability
Impact: Improves code maintainability and follows established patterns
| const getMethod = ( | |
| fn: endpointStrings, | |
| transformedRequestUrl: string, | |
| c: Context | |
| ) => { | |
| if (fn === 'proxy') { | |
| return c.req.method; | |
| } | |
| const getMethod = ( | |
| fn: endpointStrings, | |
| transformedRequestUrl: string, | |
| context: Context | |
| ) => { | |
| if (fn === 'proxy') { | |
| return context?.req?.method || 'GET'; | |
| } |
| let path: any; | ||
| if (isNodeInstance) { | ||
| path = await import('path'); | ||
| } |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Using any type defeats TypeScript's type safety - path module methods won't have proper type checking and IDE support
Fix: Use proper typing for the dynamically imported path module
Impact: Enables type checking and prevents runtime errors from incorrect method usage
| let path: any; | |
| if (isNodeInstance) { | |
| path = await import('path'); | |
| } | |
| let path: typeof import('path') | undefined; | |
| if (isNodeInstance) { | |
| path = await import('path'); | |
| } |
| let path: any; | ||
| if (isNodeInstance) { | ||
| path = await import('path'); | ||
| } |
There was a problem hiding this comment.
⚡️ Performance Improvement
Issue: Top-level await in module can block other imports and cause initialization delays
Fix: Move dynamic import to function scope where path is actually used
Impact: Reduces module initialization time and prevents blocking other imports
| let path: any; | |
| if (isNodeInstance) { | |
| path = await import('path'); | |
| } | |
| let pathModule: typeof import('path') | undefined; | |
| const getPath = async () => { | |
| if (isNodeInstance && !pathModule) { | |
| pathModule = await import('path'); | |
| } | |
| return pathModule; | |
| }; |
Changes included in this PR:
Changes not included:
Support for IRSA and IMDS based auth