feat: add node-vault util functions#6796
Conversation
WalkthroughAdds an internal TypeScript Vault client (createVaultClient, VaultClient, VaultError, related types), comprehensive tests, public exports, and removes the external Changes
Sequence DiagramsequenceDiagram
participant Client
participant VaultClient
participant Axios
participant VaultServer as Vault Server
Client->>VaultClient: createVaultClient(config)
VaultClient-->>Client: VaultClient instance
Client->>VaultClient: approleLogin(role_id, secret_id?)
VaultClient->>Axios: POST /v1/auth/<mount>/login with payload
Axios->>VaultServer: HTTP POST
VaultServer-->>Axios: { auth: { client_token: "..." } }
Axios-->>VaultClient: Response
VaultClient-->>Client: { auth: { client_token } }
Client->>VaultClient: read("secret/data/mykey")
VaultClient->>Axios: GET /v1/<apiVersion>/secret/data/mykey with headers
Axios->>VaultServer: HTTP GET
VaultServer-->>Axios: { data: { data: {...} } }
Axios-->>VaultClient: Response
VaultClient-->>Client: { data: { data: {...} } }
Client->>VaultClient: tokenRenewSelf(increment?)
VaultClient->>Axios: POST /v1/auth/token/renew-self with body
Axios->>VaultServer: HTTP POST
VaultServer-->>Axios: { auth: {...} } / error
Axios-->>VaultClient: Response / Error
alt success
VaultClient-->>Client: auth response
else failure
VaultClient-->>Client: throws VaultError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @packages/bruno-requests/src/utils/node-vault.spec.ts:
- Around line 449-466: The test "should throw VaultError with response
structure" can silently pass if no error is thrown; update the test to assert
the expected rejection explicitly by either adding expect.assertions(3) at the
start of the test or (preferably) refactor to use Jest's promise assertions:
replace the try/catch with await
expect(vault.read('secret/data/hello')).rejects.toEqual(expect.objectContaining({
constructor: VaultError, message: 'internal server error', response: {
statusCode: 500, status: 500, body: { errors: ['internal server error'] } } })),
or use rejects.toThrow/VaultError plus a separate assertion for the response
property on the caught error to ensure the vault.read call actually rejects;
reference the test case name and the vault.read and VaultError symbols when
making the change.
- Around line 477-489: The test "should handle network errors" lacks an
assertion guard and uses try/catch, so replace the try/catch with an explicit
rejection assertion: call mockedAxios.mockRejectedValueOnce(networkError) as
before and then use await
expect(vault.read('secret/data/hello')).rejects.toMatchObject({ message:
'Network Error', code: 'ECONNREFUSED' }) (or add expect.assertions(2) at the top
and keep the try/catch but add a fail() after the await to ensure the catch path
runs); target symbols: mockedAxios.mockRejectedValueOnce, vault.read, and the
test block named "should handle network errors".
🧹 Nitpick comments (3)
packages/bruno-requests/src/utils/node-vault.ts (3)
106-129: Consider adding explicit handling for 429 (rate limiting) responses.The current implementation treats any non-200/204 status as an error. While this works, Vault can return 429 for rate limiting. Depending on usage patterns, you may want to surface this distinctly for retry logic.
Also, on line 116, regex match for health endpoint could use a more robust check.
💡 Optional: Simplify health endpoint check
- if (path.match(/sys\/health/) !== null) { + if (path.includes('sys/health')) { return body; }
206-226: Silent failure on proxy URL parsing may hide configuration issues.When proxy URL parsing fails (line 222-225), the error is only logged to debug and the request proceeds without proxy. This could cause silent failures in environments requiring a proxy.
Consider emitting a warning or documenting this behavior explicitly.
♻️ Suggested improvement
} catch (e) { // If proxy URL parsing fails, pass it as-is for backward compatibility - debug('Failed to parse proxy URL:', mergedOptions.proxy); + debug('Failed to parse proxy URL, request will proceed without proxy:', mergedOptions.proxy); + // Note: Unlike node-vault, invalid proxy URLs are silently ignored }
300-303: Unused parameter intokenLookupSelf.The
argsparameter is declared but never used. Per the coding guidelines, consider removing it or documenting why it's kept for API compatibility.♻️ Suggested fix
- async tokenLookupSelf(args?: any): Promise<any> { + async tokenLookupSelf(_args?: any): Promise<any> { debug('tokenLookupSelf'); return request('GET', '/auth/token/lookup-self'); },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/bruno-js/package.jsonpackages/bruno-requests/src/index.tspackages/bruno-requests/src/utils/node-vault.spec.tspackages/bruno-requests/src/utils/node-vault.ts
💤 Files with no reviewable changes (1)
- packages/bruno-js/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-requests/src/utils/node-vault.spec.tspackages/bruno-requests/src/utils/node-vault.tspackages/bruno-requests/src/index.ts
🧠 Learnings (2)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
packages/bruno-requests/src/utils/node-vault.spec.ts
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-requests/src/utils/node-vault.tspackages/bruno-requests/src/index.ts
🧬 Code graph analysis (1)
packages/bruno-requests/src/utils/node-vault.ts (1)
packages/bruno-requests/src/index.ts (4)
VaultConfig(8-8)VaultRequestOptions(8-8)VaultError(7-7)VaultClient(8-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
🔇 Additional comments (14)
packages/bruno-requests/src/utils/node-vault.ts (7)
1-3: LGTM - Imports are appropriate.Clean imports for axios and node:https. Good use of the
node:prefix for built-in modules.
4-43: Interface definitions look good.Well-documented interfaces with JSDoc comments. The
[key: string]: anyindex signature onVaultRequestOptions(line 24) provides flexibility for additional options while maintaining type safety for known properties.
44-68: VaultError class implementation is solid.Good approach providing both
statusCodeandstatusfor compatibility with both node-vault and axios error handling patterns.
70-84: VaultClient interface is well-defined.Clear interface contract matching the node-vault API surface. Mutable properties support the direct assignment pattern documented in the example.
86-104: LGTM - HTTPS agent creation.Correctly handles
strictSSLand custom CA certificates. Agent is only instantiated when needed.
228-250: Error handling logic is sound.Good separation between server responses (handled via
handleVaultResponse) and network-level errors. Thecodeproperty preservation for network errors maintains compatibility.Minor: Line 234 cast to
AxiosErroris redundant afterisAxiosErrorcheck.💅 Optional cleanup
if (axios.isAxiosError(error)) { - const axiosError = error as AxiosError; + const axiosError = error; if (axiosError.response) {
252-320: Client implementation is clean and well-documented.Good use of JSDoc comments for all public methods. The mutable properties pattern correctly mimics node-vault behavior. Environment variable fallbacks are appropriately handled.
packages/bruno-requests/src/utils/node-vault.spec.ts (6)
1-20: Test setup is well-structured.Clean mock setup for axios with proper
isAxiosErrorimplementation. Good practice clearing environment variables inbeforeEachto ensure test isolation.
22-83: Good coverage of initialization and client property tests.Tests properly validate default values, environment variable usage, config overrides, and mutable property assignments.
85-170: Read operation tests are comprehensive.Good coverage of success path, namespace headers, endpoint updates, 404 errors, and 204 responses.
172-327: Write and AppRole tests cover the key scenarios.LDAP login, custom mount points, and authentication errors are all tested appropriately.
511-610: Request options tests are solid.Good coverage of strictSSL, CA certificates (via httpsAgent), and proxy configurations with/without authentication.
633-651: Add test for endpoint with trailing slash.The existing test covers endpoints without trailing slash, but there's no test for endpoints with a trailing slash (e.g.,
http://localhost:8200/). Without trailing slash handling in the implementation, this would result in double slashes in the constructed URL (http://localhost:8200//v1/secret/data/hello).packages/bruno-requests/src/index.ts (1)
7-8: LGTM - Clean export structure.Correctly re-exports the default as a named export (
createVaultClient) and usesexport typefor type-only exports. This maintains a consistent public API surface.
| }; | ||
|
|
||
| // Build URL | ||
| const uri = `${client.endpoint}/${client.apiVersion}${path}`; |
There was a problem hiding this comment.
| const uri = `${client.endpoint}/${client.apiVersion}${path}`; | |
| const normalizedPath = !path.startsWith('/')?'/'+path.slice(1):path | |
| const uri = `${client.endpoint}/${client.apiVersion}${normalizedPath}`; |
There was a problem hiding this comment.
Allows for the user input to be verified and cleaned up as needed, let's say we have .read('/x' then it'd make it // with the current logic
| const agent = createHttpsAgent(mergedOptions); | ||
| if (agent) { | ||
| axiosConfig.httpsAgent = agent; | ||
| } |
There was a problem hiding this comment.
nit: we get undefined from the createHttpsAgent, can avoid the if we don't see that changing
| const agent = createHttpsAgent(mergedOptions); | |
| if (agent) { | |
| axiosConfig.httpsAgent = agent; | |
| } | |
| axiosConfig.httpsAgent = createHttpsAgent(mergedOptions); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @packages/bruno-requests/src/utils/node-vault.spec.ts:
- Around line 519-531: In the test "should handle network errors" in
node-vault.spec.ts, add expect.assertions(2) as the first statement of the test
so that both assertions inside the catch block
(expect(error.message).toBe('Network Error') and
expect(error.code).toBe('ECONNREFUSED')) are guaranteed to run when invoking
vault.read('secret/data/hello').
- Around line 491-508: The test can pass silently if no error is thrown; either
add expect.assertions(3) at the top of this test to ensure the three assertions
in the catch block run, or refactor the try/catch to use Jest's rejects matcher:
replace the try/catch around vault.read('secret/data/hello') with await
expect(vault.read('secret/data/hello')).rejects.toBeInstanceOf(VaultError) and
additional awaits like await
expect(vault.read('secret/data/hello')).rejects.toHaveProperty('message','internal
server error') and await
expect(vault.read('secret/data/hello')).rejects.toHaveProperty('response',{
statusCode:500, status:500, body:{ errors:['internal server error'] } }) so the
test fails if no rejection occurs.
In @packages/bruno-requests/src/utils/node-vault.ts:
- Around line 224-227: The current catch for proxy URL parsing quietly logs with
debug and continues, causing silent fallback to no-proxy; update the catch in
node-vault.ts (the block handling mergedOptions.proxy parsing) to surface the
failure by throwing a descriptive Error that includes mergedOptions.proxy and
the caught exception (e) so callers cannot proceed under a false assumption of a
configured proxy; ensure the thrown error message clearly names
mergedOptions.proxy and the parse error to aid debugging.
- Around line 186-192: The axios request config in node-vault (the axiosConfig
object used in the request function) is missing a timeout, which can cause
requests to hang; update axiosConfig to include a timeout value (use a sensible
default like 5000ms) and make it configurable by adding an optional timeout
property to the VaultRequestOptions interface so callers can override it; ensure
the timeout value from VaultRequestOptions (if provided) is applied to
axiosConfig when building the request (and continue to preserve existing fields
like method and url/uri).
🧹 Nitpick comments (4)
packages/bruno-requests/src/utils/node-vault.ts (4)
30-35: Unusedrolefield inApproleLoginArgs.The
rolefield (line 31) is declared but not used in theapproleLoginimplementation (lines 289-299). Consider removing it if unnecessary, or document its intended purpose.♻️ Suggested fix
export interface ApproleLoginArgs { - role?: string; role_id: string; secret_id?: string; mount_point?: string; }
82-82: Unused parameter intokenLookupSelf.The
args?: anyparameter is declared but not used in the implementation (line 304-306). Consider removing it for a cleaner API.♻️ Suggested fix
- tokenLookupSelf(args?: any): Promise<any>; + tokenLookupSelf(): Promise<any>;
116-118: Preferincludes()over regex for simple substring match.Using
path.includes('sys/health')would be cleaner and slightly more efficient than a regex match for this simple case.♻️ Suggested fix
- if (path.match(/sys\/health/) !== null) { + if (path.includes('sys/health')) { return body; }
267-271: Path normalization is duplicated.The path normalization logic (
path.startsWith('/') ? path : \/${path}`) is repeated in bothreadandwrite` methods. Consider extracting to a helper function.♻️ Suggested refactor
Add a helper function:
function normalizePath(path: string): string { return path.startsWith('/') ? path : `/${path}`; }Then simplify the methods:
async read(path: string, requestOptions?: VaultRequestOptions): Promise<any> { - path = path.startsWith('/') ? path : `/${path}`; + path = normalizePath(path); debug('read', path); return request('GET', path, undefined, requestOptions); }, async write(path: string, data: any, requestOptions?: VaultRequestOptions): Promise<any> { - path = path.startsWith('/') ? path : `/${path}`; + path = normalizePath(path); debug('write', path, data); return request('POST', path, data, requestOptions); },Also applies to: 279-283
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-requests/src/utils/node-vault.spec.tspackages/bruno-requests/src/utils/node-vault.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-requests/src/utils/node-vault.tspackages/bruno-requests/src/utils/node-vault.spec.ts
🧠 Learnings (2)
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-requests/src/utils/node-vault.ts
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
packages/bruno-requests/src/utils/node-vault.spec.ts
🧬 Code graph analysis (1)
packages/bruno-requests/src/utils/node-vault.spec.ts (2)
packages/bruno-requests/src/utils/node-vault.ts (2)
VaultClient(73-84)VaultError(48-68)packages/bruno-requests/src/index.ts (2)
VaultClient(8-8)VaultError(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (8)
packages/bruno-requests/src/utils/node-vault.ts (3)
48-68: LGTM!The
VaultErrorclass is well-structured with proper inheritance, dual status codes for compatibility, and clear documentation.
89-104: LGTM!Efficient conditional agent creation - only instantiates when custom SSL settings are needed.
255-260: LGTM!Clean implementation of mutable client properties with sensible environment variable fallbacks. The pattern supports direct property assignment like the original node-vault library.
packages/bruno-requests/src/utils/node-vault.spec.ts (5)
4-9: LGTM!Clean axios mock setup with proper
isAxiosErrorhelper implementation.
22-57: LGTM!Thorough coverage of initialization scenarios including defaults, environment variables, and config overrides.
85-212: LGTM!Comprehensive read operation tests covering success paths, error handling, and URL edge cases (leading/trailing slashes).
553-652: LGTM!Solid coverage of request options including SSL settings, HTTPS agent behavior for different protocols, and proxy configuration with authentication.
695-715: LGTM!Good test for the special
sys/healthendpoint behavior that doesn't throw on non-200 status codes.
| it('should throw VaultError with response structure', async () => { | ||
| mockedAxios.mockResolvedValueOnce({ | ||
| status: 500, | ||
| data: { errors: ['internal server error'] } | ||
| }); | ||
|
|
||
| try { | ||
| await vault.read('secret/data/hello'); | ||
| } catch (error) { | ||
| expect(error).toBeInstanceOf(VaultError); | ||
| expect((error as VaultError).message).toBe('internal server error'); | ||
| expect((error as VaultError).response).toEqual({ | ||
| statusCode: 500, | ||
| status: 500, | ||
| body: { errors: ['internal server error'] } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Test may pass silently if no error is thrown.
The try/catch pattern without expect.assertions() could cause the test to pass silently if no exception is thrown. Consider using rejects.toThrow() pattern or adding expect.assertions(n).
💡 Suggested fix using rejects pattern
it('should throw VaultError with response structure', async () => {
mockedAxios.mockResolvedValueOnce({
status: 500,
data: { errors: ['internal server error'] }
});
- try {
- await vault.read('secret/data/hello');
- } catch (error) {
- expect(error).toBeInstanceOf(VaultError);
- expect((error as VaultError).message).toBe('internal server error');
- expect((error as VaultError).response).toEqual({
- statusCode: 500,
- status: 500,
- body: { errors: ['internal server error'] }
- });
- }
+ await expect(vault.read('secret/data/hello')).rejects.toMatchObject({
+ name: 'VaultError',
+ message: 'internal server error',
+ response: {
+ statusCode: 500,
+ status: 500,
+ body: { errors: ['internal server error'] }
+ }
+ });
});📝 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.
| it('should throw VaultError with response structure', async () => { | |
| mockedAxios.mockResolvedValueOnce({ | |
| status: 500, | |
| data: { errors: ['internal server error'] } | |
| }); | |
| try { | |
| await vault.read('secret/data/hello'); | |
| } catch (error) { | |
| expect(error).toBeInstanceOf(VaultError); | |
| expect((error as VaultError).message).toBe('internal server error'); | |
| expect((error as VaultError).response).toEqual({ | |
| statusCode: 500, | |
| status: 500, | |
| body: { errors: ['internal server error'] } | |
| }); | |
| } | |
| }); | |
| it('should throw VaultError with response structure', async () => { | |
| mockedAxios.mockResolvedValueOnce({ | |
| status: 500, | |
| data: { errors: ['internal server error'] } | |
| }); | |
| await expect(vault.read('secret/data/hello')).rejects.toMatchObject({ | |
| name: 'VaultError', | |
| message: 'internal server error', | |
| response: { | |
| statusCode: 500, | |
| status: 500, | |
| body: { errors: ['internal server error'] } | |
| } | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In @packages/bruno-requests/src/utils/node-vault.spec.ts around lines 491 - 508,
The test can pass silently if no error is thrown; either add
expect.assertions(3) at the top of this test to ensure the three assertions in
the catch block run, or refactor the try/catch to use Jest's rejects matcher:
replace the try/catch around vault.read('secret/data/hello') with await
expect(vault.read('secret/data/hello')).rejects.toBeInstanceOf(VaultError) and
additional awaits like await
expect(vault.read('secret/data/hello')).rejects.toHaveProperty('message','internal
server error') and await
expect(vault.read('secret/data/hello')).rejects.toHaveProperty('response',{
statusCode:500, status:500, body:{ errors:['internal server error'] } }) so the
test fails if no rejection occurs.
| it('should handle network errors', async () => { | ||
| const networkError = new Error('Network Error'); | ||
| (networkError as any).isAxiosError = true; | ||
| (networkError as any).code = 'ECONNREFUSED'; | ||
| mockedAxios.mockRejectedValueOnce(networkError); | ||
|
|
||
| try { | ||
| await vault.read('secret/data/hello'); | ||
| } catch (error) { | ||
| expect((error as any).message).toBe('Network Error'); | ||
| expect((error as any).code).toBe('ECONNREFUSED'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Same issue: test may pass silently.
Add expect.assertions(2) at the start of this test to ensure both expectations are evaluated.
💡 Suggested fix
it('should handle network errors', async () => {
+ expect.assertions(2);
const networkError = new Error('Network Error');
(networkError as any).isAxiosError = true;
(networkError as any).code = 'ECONNREFUSED';
mockedAxios.mockRejectedValueOnce(networkError);
try {
await vault.read('secret/data/hello');
+ fail('Expected error to be thrown');
} catch (error) {
expect((error as any).message).toBe('Network Error');
expect((error as any).code).toBe('ECONNREFUSED');
}
});📝 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.
| it('should handle network errors', async () => { | |
| const networkError = new Error('Network Error'); | |
| (networkError as any).isAxiosError = true; | |
| (networkError as any).code = 'ECONNREFUSED'; | |
| mockedAxios.mockRejectedValueOnce(networkError); | |
| try { | |
| await vault.read('secret/data/hello'); | |
| } catch (error) { | |
| expect((error as any).message).toBe('Network Error'); | |
| expect((error as any).code).toBe('ECONNREFUSED'); | |
| } | |
| }); | |
| it('should handle network errors', async () => { | |
| expect.assertions(2); | |
| const networkError = new Error('Network Error'); | |
| (networkError as any).isAxiosError = true; | |
| (networkError as any).code = 'ECONNREFUSED'; | |
| mockedAxios.mockRejectedValueOnce(networkError); | |
| try { | |
| await vault.read('secret/data/hello'); | |
| fail('Expected error to be thrown'); | |
| } catch (error) { | |
| expect((error as any).message).toBe('Network Error'); | |
| expect((error as any).code).toBe('ECONNREFUSED'); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In @packages/bruno-requests/src/utils/node-vault.spec.ts around lines 519 - 531,
In the test "should handle network errors" in node-vault.spec.ts, add
expect.assertions(2) as the first statement of the test so that both assertions
inside the catch block (expect(error.message).toBe('Network Error') and
expect(error.code).toBe('ECONNREFUSED')) are guaranteed to run when invoking
vault.read('secret/data/hello').
| // Build axios config | ||
| const axiosConfig: AxiosRequestConfig = { | ||
| method: method as any, | ||
| url: uri, | ||
| headers, | ||
| validateStatus: () => true // Don't throw on non-2xx status | ||
| }; |
There was a problem hiding this comment.
Missing request timeout configuration.
The axios config lacks a timeout setting. Without a timeout, requests could hang indefinitely on network issues. Consider adding a configurable timeout with a sensible default.
🔧 Suggested fix
const axiosConfig: AxiosRequestConfig = {
method: method as any,
url: uri,
headers,
- validateStatus: () => true // Don't throw on non-2xx status
+ validateStatus: () => true, // Don't throw on non-2xx status
+ timeout: mergedOptions.timeout ?? 30000 // Default 30s timeout
};Also update the VaultRequestOptions interface:
export interface VaultRequestOptions {
strictSSL?: boolean;
ca?: string | Buffer | Array<string | Buffer>;
proxy?: string;
+ timeout?: number;
[key: string]: any;
}📝 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.
| // Build axios config | |
| const axiosConfig: AxiosRequestConfig = { | |
| method: method as any, | |
| url: uri, | |
| headers, | |
| validateStatus: () => true // Don't throw on non-2xx status | |
| }; | |
| // Build axios config | |
| const axiosConfig: AxiosRequestConfig = { | |
| method: method as any, | |
| url: uri, | |
| headers, | |
| validateStatus: () => true, // Don't throw on non-2xx status | |
| timeout: mergedOptions.timeout ?? 30000 // Default 30s timeout | |
| }; |
| // Build axios config | |
| const axiosConfig: AxiosRequestConfig = { | |
| method: method as any, | |
| url: uri, | |
| headers, | |
| validateStatus: () => true // Don't throw on non-2xx status | |
| }; | |
| export interface VaultRequestOptions { | |
| strictSSL?: boolean; | |
| ca?: string | Buffer | Array<string | Buffer>; | |
| proxy?: string; | |
| timeout?: number; | |
| [key: string]: any; | |
| } |
🤖 Prompt for AI Agents
In @packages/bruno-requests/src/utils/node-vault.ts around lines 186 - 192, The
axios request config in node-vault (the axiosConfig object used in the request
function) is missing a timeout, which can cause requests to hang; update
axiosConfig to include a timeout value (use a sensible default like 5000ms) and
make it configurable by adding an optional timeout property to the
VaultRequestOptions interface so callers can override it; ensure the timeout
value from VaultRequestOptions (if provided) is applied to axiosConfig when
building the request (and continue to preserve existing fields like method and
url/uri).
| } catch (e) { | ||
| // If proxy URL parsing fails, pass it as-is for backward compatibility | ||
| debug('Failed to parse proxy URL:', mergedOptions.proxy); | ||
| } |
There was a problem hiding this comment.
Silent failure on proxy URL parsing may hide configuration issues.
When proxy URL parsing fails, the error is logged but the request proceeds without proxy configuration. This could lead to unexpected behavior where users believe proxy is configured but it silently falls back to direct connection.
Consider either throwing an error or using a more prominent warning mechanism.
💡 Suggested improvement
} catch (e) {
- // If proxy URL parsing fails, pass it as-is for backward compatibility
- debug('Failed to parse proxy URL:', mergedOptions.proxy);
+ // If proxy URL parsing fails, warn and continue without proxy
+ debug('WARNING: Failed to parse proxy URL, proceeding without proxy:', mergedOptions.proxy);
+ // Consider throwing here if strict proxy configuration is required
}🤖 Prompt for AI Agents
In @packages/bruno-requests/src/utils/node-vault.ts around lines 224 - 227, The
current catch for proxy URL parsing quietly logs with debug and continues,
causing silent fallback to no-proxy; update the catch in node-vault.ts (the
block handling mergedOptions.proxy parsing) to surface the failure by throwing a
descriptive Error that includes mergedOptions.proxy and the caught exception (e)
so callers cannot proceed under a false assumption of a configured proxy; ensure
the thrown error message clearly names mergedOptions.proxy and the parse error
to aid debugging.
* feat: add `node-vault` util functions * fix: review comment fixes
jira
Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.