fix: surface silent update-check parse failures#384
Conversation
The update checker silently swallowed invalid package metadata and cached JSON parse failures. Keep the same fallback behavior, but log those cases at debug level so malformed local state is observable during diagnosis.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughThis is a minor severity observability improvement that adds debug-level logging to the update checker's error handling paths while preserving all existing fallback behavior. The fix surfaces previously-silent parse failures in local package metadata and cache JSON reading, enabling visibility into malformed local state during diagnosis without changing any functional behavior or exported APIs. No security or data-loss risks are introduced; regression tests were added to verify the debug logging behavior (59 new test lines, full suite passes 3315 tests), and the architectural approach is straightforward—capturing exceptions in existing catch blocks and logging them before returning fallback values. Walkthroughenhanced error handling in auto-update-checker by adding debug logging to two catch blocks. when version reading or cache loading fails, errors are now logged before returning fallback values. tests added to verify logging behavior on parse/read errors. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
review notes:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/auto-update-checker.test.ts`:
- Around line 242-261: In the "logs debug details when package metadata cannot
be parsed" test, add an assertion that the update fetch still runs by checking
globalThis.fetch was called; update the test around the checkForUpdates(true)
invocation (the test function and its mocks for fs.readFileSync and
globalThis.fetch) to include expect(globalThis.fetch).toHaveBeenCalled() after
the existing assertions so the fallback update path (checkForUpdates) is
verified to continue even when package parsing fails.
- Around line 287-310: Add a new vitest case in test/auto-update-checker.test.ts
that simulates Windows-style readFileSync failures for the update cache: mock
fs.existsSync to true and mock fs.readFileSync to throw a NodeJS.ErrnoException
(set the name/message/code to "EBUSY" or "EPERM") when the path includes
"update-check-cache.json" while still returning package.json for that path; call
checkForUpdates() and assert that logger.debug was invoked with a message like
"Failed to load update cache" containing an error object that includes the
errno/code string, and also assert that globalThis.fetch was called to verify
fallback behavior in the auto-update-checker checkForUpdates path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 69b037a0-4f30-4858-938f-c02e77b60d6e
📒 Files selected for processing (2)
lib/auto-update-checker.tstest/auto-update-checker.test.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/auto-update-checker.ts
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/auto-update-checker.test.ts
🔇 Additional comments (3)
lib/auto-update-checker.ts (2)
44-47: good observability on package version fallback path.this keeps behavior stable while making failures diagnosable in
getCurrentVersion(lib/auto-update-checker.ts:44-47), and it is covered by vitest attest/auto-update-checker.test.ts:242-261. no new concurrency risk introduced here.
57-60: cache parse/read failures are now visible without breaking fallback.the debug log in
loadCache(lib/auto-update-checker.ts:57-60) preserves the null fallback and is validated bytest/auto-update-checker.test.ts:287-310. this also improves windows fs diagnostics without exposing tokens/emails.test/auto-update-checker.test.ts (1)
15-19: logger mock wiring is solid and deterministic.the
vi.doMocksetup and per-test logger reset intest/auto-update-checker.test.ts:15-19andtest/auto-update-checker.test.ts:28-35are clean and stable under vitest fake timers.Also applies to: 28-35
| it("logs debug details when package metadata cannot be parsed", async () => { | ||
| vi.mocked(fs.readFileSync).mockImplementation((path: unknown) => { | ||
| if (String(path).includes("package.json")) { | ||
| return "{"; | ||
| } | ||
| throw new Error("File not found"); | ||
| }); | ||
| vi.mocked(globalThis.fetch).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => ({ version: "5.0.0" }), | ||
| } as Response); | ||
|
|
||
| const result = await checkForUpdates(true); | ||
|
|
||
| expect(result.currentVersion).toBe("0.0.0"); | ||
| expect(logger.debug).toHaveBeenCalledWith( | ||
| "Failed to read current package version", | ||
| expect.objectContaining({ error: expect.any(String) }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
add an assertion that update fetch still runs after package parse failure.
this regression currently checks fallback version and debug log, but it does not assert continuation of the update path in test/auto-update-checker.test.ts:242-261. add expect(globalThis.fetch).toHaveBeenCalled() so the fallback behavior is fully locked in.
proposed test hardening
const result = await checkForUpdates(true);
expect(result.currentVersion).toBe("0.0.0");
+ expect(globalThis.fetch).toHaveBeenCalled();
expect(logger.debug).toHaveBeenCalledWith(
"Failed to read current package version",
expect.objectContaining({ error: expect.any(String) }),
);As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
📝 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("logs debug details when package metadata cannot be parsed", async () => { | |
| vi.mocked(fs.readFileSync).mockImplementation((path: unknown) => { | |
| if (String(path).includes("package.json")) { | |
| return "{"; | |
| } | |
| throw new Error("File not found"); | |
| }); | |
| vi.mocked(globalThis.fetch).mockResolvedValue({ | |
| ok: true, | |
| json: async () => ({ version: "5.0.0" }), | |
| } as Response); | |
| const result = await checkForUpdates(true); | |
| expect(result.currentVersion).toBe("0.0.0"); | |
| expect(logger.debug).toHaveBeenCalledWith( | |
| "Failed to read current package version", | |
| expect.objectContaining({ error: expect.any(String) }), | |
| ); | |
| }); | |
| it("logs debug details when package metadata cannot be parsed", async () => { | |
| vi.mocked(fs.readFileSync).mockImplementation((path: unknown) => { | |
| if (String(path).includes("package.json")) { | |
| return "{"; | |
| } | |
| throw new Error("File not found"); | |
| }); | |
| vi.mocked(globalThis.fetch).mockResolvedValue({ | |
| ok: true, | |
| json: async () => ({ version: "5.0.0" }), | |
| } as Response); | |
| const result = await checkForUpdates(true); | |
| expect(result.currentVersion).toBe("0.0.0"); | |
| expect(globalThis.fetch).toHaveBeenCalled(); | |
| expect(logger.debug).toHaveBeenCalledWith( | |
| "Failed to read current package version", | |
| expect.objectContaining({ error: expect.any(String) }), | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/auto-update-checker.test.ts` around lines 242 - 261, In the "logs debug
details when package metadata cannot be parsed" test, add an assertion that the
update fetch still runs by checking globalThis.fetch was called; update the test
around the checkForUpdates(true) invocation (the test function and its mocks for
fs.readFileSync and globalThis.fetch) to include
expect(globalThis.fetch).toHaveBeenCalled() after the existing assertions so the
fallback update path (checkForUpdates) is verified to continue even when package
parsing fails.
| it("logs debug details when cached update JSON is unreadable", async () => { | ||
| vi.mocked(fs.existsSync).mockReturnValue(true); | ||
| vi.mocked(fs.readFileSync).mockImplementation((path: unknown) => { | ||
| if (String(path).includes("package.json")) { | ||
| return JSON.stringify(mockPackageJson); | ||
| } | ||
| if (String(path).includes("update-check-cache.json")) { | ||
| return "{"; | ||
| } | ||
| throw new Error("File not found"); | ||
| }); | ||
| vi.mocked(globalThis.fetch).mockResolvedValue({ | ||
| ok: true, | ||
| json: async () => ({ version: "5.0.0" }), | ||
| } as Response); | ||
|
|
||
| await checkForUpdates(); | ||
|
|
||
| expect(logger.debug).toHaveBeenCalledWith( | ||
| "Failed to load update cache", | ||
| expect.objectContaining({ error: expect.any(String) }), | ||
| ); | ||
| expect(globalThis.fetch).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
missing windows read-error regression for cache load path.
test/auto-update-checker.test.ts:287-310 covers malformed cache json, but not windows-style fs read failures (like EBUSY/EPERM) on readFileSync. add a case that throws NodeJS.ErrnoException with those codes and assert debug logging plus fetch fallback. this is the windows edge case for the exact path changed in lib/auto-update-checker.ts:57-60. no new concurrency bug is visible, but this platform regression is still missing.
proposed regression case
+ it.each(["EBUSY", "EPERM"] as const)(
+ "logs debug details when cache read fails on windows-style lock (%s)",
+ async (code) => {
+ vi.mocked(fs.existsSync).mockReturnValue(true);
+ vi.mocked(fs.readFileSync).mockImplementation((path: unknown) => {
+ if (String(path).includes("package.json")) {
+ return JSON.stringify(mockPackageJson);
+ }
+ if (String(path).includes("update-check-cache.json")) {
+ const error = new Error("locked") as NodeJS.ErrnoException;
+ error.code = code;
+ throw error;
+ }
+ throw new Error("File not found");
+ });
+ vi.mocked(globalThis.fetch).mockResolvedValue({
+ ok: true,
+ json: async () => ({ version: "5.0.0" }),
+ } as Response);
+
+ await checkForUpdates();
+
+ expect(logger.debug).toHaveBeenCalledWith(
+ "Failed to load update cache",
+ expect.objectContaining({ error: expect.any(String) }),
+ );
+ expect(globalThis.fetch).toHaveBeenCalled();
+ },
+ );As per coding guidelines, test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/auto-update-checker.test.ts` around lines 287 - 310, Add a new vitest
case in test/auto-update-checker.test.ts that simulates Windows-style
readFileSync failures for the update cache: mock fs.existsSync to true and mock
fs.readFileSync to throw a NodeJS.ErrnoException (set the name/message/code to
"EBUSY" or "EPERM") when the path includes "update-check-cache.json" while still
returning package.json for that path; call checkForUpdates() and assert that
logger.debug was invoked with a message like "Failed to load update cache"
containing an error object that includes the errno/code string, and also assert
that globalThis.fetch was called to verify fallback behavior in the
auto-update-checker checkForUpdates path.
|
Superseded by #387, which rebuilds the full open PR stack onto one reviewed integration branch. |
|
Closing in favor of #387. |
1 participant
Problem
The update checker silently swallowed invalid local package metadata and unreadable cached JSON.
The fallback behavior was safe, but those failures were invisible during diagnosis.
Fix
Keep the same fallback behavior, but log those silent parse/read failures at debug level.
Changes
lib/auto-update-checker.tspackage.jsonfailstest/auto-update-checker.test.tsValidation
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
this pr adds
log.debugcalls in the two previously-silentcatch {}blocks insidegetCurrentVersion()andloadCache(), surfacing parse and read failures for diagnosis without changing the existing fallback behavior ("0.0.0"/null). tests are added for both paths using avi.doMocklogger stub that correctly intercepts the module-levelcreateLogger("update-checker")call before the module is dynamically imported.Confidence Score: 5/5
safe to merge — no behavior change, only adds debug observability to two existing catch blocks
minimal diff, fallback values are identical to before, both new catch paths are exercised by dedicated tests, and the pattern matches the existing log.debug call in fetchLatestVersion()
no files require special attention
Important Files Changed
Sequence Diagram
sequenceDiagram participant C as checkForUpdates() participant GV as getCurrentVersion() participant LC as loadCache() participant FS as node:fs participant LOG as log (debug) participant NPM as npm registry C->>GV: call GV->>FS: readFileSync(package.json) alt read/parse succeeds FS-->>GV: version string GV-->>C: "4.12.0" else read/parse fails (new) FS-->>GV: throws GV->>LOG: debug("Failed to read current package version", {error}) GV-->>C: "0.0.0" end C->>LC: call LC->>FS: existsSync(cache) alt cache absent LC-->>C: null else cache present LC->>FS: readFileSync(cache) alt parse succeeds FS-->>LC: UpdateCheckCache LC-->>C: cache object else parse fails (new) FS-->>LC: throws LC->>LOG: debug("Failed to load update cache", {error}) LC-->>C: null end end alt cache valid and not expired C-->>C: return cached result else cache missing/expired/forced C->>NPM: fetchLatestVersion() NPM-->>C: latestVersion C->>FS: saveCache(...) C-->>C: return fresh result endReviews (1): Last reviewed commit: "fix: surface silent update-check parse f..." | Re-trigger Greptile