fix: add null safety guards in config-manager and cache-utils#614
fix: add null safety guards in config-manager and cache-utils#614ashsolei wants to merge 1 commit intoczlonkowski:mainfrom
Conversation
config-manager.ts: Guard JSON.parse result before accessing properties. If the config file contains 'null' (valid JSON), this.config would be null and accessing .userId would throw a TypeError. Now validates the parsed value is a non-null object before assignment. cache-utils.ts: Initialize lastError with a sentinel Error instead of leaving it uninitialized. If withRetry() is called with maxAttempts=0, the for-loop never executes and accessing lastError.message would throw a TypeError. Now provides a meaningful error message.
There was a problem hiding this comment.
Pull request overview
Improves runtime safety in telemetry config loading and retry logic by handling edge cases where previously the code could throw due to null/uninitialized values.
Changes:
- Add a runtime guard to reject non-object JSON values (e.g.,
null) when loadingtelemetry.json. - Initialize
withRetry()’slastErrorto avoidundefinedaccess when no attempts run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/utils/cache-utils.ts |
Initializes lastError in withRetry() to handle maxAttempts: 0 without a TypeError. |
src/telemetry/config-manager.ts |
Validates parsed JSON is an object before assigning to this.config, preventing null dereference. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Guard against malformed config (e.g. null is valid JSON) | ||
| if (!parsed || typeof parsed !== 'object') { | ||
| throw new Error('Telemetry config is not a valid object'); | ||
| } | ||
|
|
||
| this.config = parsed; |
There was a problem hiding this comment.
The new guard rejects null and primitives, but it still accepts arrays because typeof [] === 'object'. An array is not a valid TelemetryConfig, and because JSON.stringify ignores non-index properties on arrays, later this.config.userId = ... and lastModified may never persist to disk. Consider tightening the check to require a non-array plain object (e.g., !Array.isArray(parsed)) before assigning to this.config.
| const parsed = JSON.parse(rawConfig); | ||
|
|
||
| // Guard against malformed config (e.g. null is valid JSON) | ||
| if (!parsed || typeof parsed !== 'object') { | ||
| throw new Error('Telemetry config is not a valid object'); | ||
| } |
There was a problem hiding this comment.
loadConfig() now explicitly treats parsed null/non-object JSON as invalid. Since there are already unit tests for TelemetryConfigManager.loadConfig() (tests/unit/telemetry/config-manager.test.ts), it would be good to add a test case for a config file containing the literal JSON null (and optionally []) to verify the fallback behavior stays stable.
| let lastError: Error = new Error('No retry attempts were made'); | ||
|
|
||
| for (let attempt = 0; attempt < config.maxAttempts; attempt++) { | ||
| try { |
There was a problem hiding this comment.
withRetry() can now throw the sentinel error when config.maxAttempts is 0/negative, but the function will still proceed to the "All retry attempts exhausted" error log path. That message is inaccurate for 0 attempts and may produce noisy error logs for an invalid config. Consider validating maxAttempts (e.g., require >= 1) and throwing early with a clearer error, or explicitly handling the 0-attempt case with different logging.
| context?: string | ||
| ): Promise<T> { | ||
| let lastError: Error; | ||
| let lastError: Error = new Error('No retry attempts were made'); |
There was a problem hiding this comment.
There are existing unit tests for withRetry() (see tests/unit/utils/cache-utils.test.ts), but none cover the newly handled edge case where maxAttempts is 0. Adding a test for maxAttempts: 0 would lock in the intended behavior/message and prevent regressions (previously this case threw due to lastError being uninitialized).
|
Closing — will maintain fixes in a private mirror instead. Thank you for the review. |
Two null safety fixes:
config-manager.ts: After JSON.parse, the parsed result could be
null(valid JSON). The code immediately accessesthis.config.userIdvia non-null assertion, which would throw a TypeError. Added a guard to validate the parsed value is a non-null object before assignment, falling through to defaults on invalid config.cache-utils.ts:
lastErrorinwithRetry()is declared but never initialized. If called withmaxAttempts: 0, the for-loop never executes andlastError.messageaccesses undefined. Initialized with a sentinel Error for a meaningful message.