fix: improve error handling when packfile cannot be read (fixes #1163) by costajohnt · Pull Request #2295 · isomorphic-git/isomorphic-git

costajohnt · 2026-03-12T19:44:25Z

Summary

Fixes #1163 — GitPackIndex throws confusing TypeError: Cannot read properties of null (reading 'slice') when fs.read() returns null for a packfile (e.g. ERR_FS_FILE_TOO_LARGE, missing file, permission error).

Root cause: FileSystem.read() catches all errors and returns null. In GitPackIndex.readSlice(), the guard if (!this.pack) checks a Promise object (always truthy), so when the Promise resolves to null, the code falls through to (await this.pack).slice(start) which throws an unhelpful TypeError.

Fix:

Await the Promise before null-checking in readSlice() so the guard actually works
Add an earlier null check in readObjectPacked.js with a descriptive error message that includes the packfile path
Clear the cached promise on failure (p.pack = null) so transient errors don't become permanent failures
Add test verifying a descriptive InternalError is thrown when a packfile cannot be read

Test plan

New test: should throw descriptive error when packfile cannot be read — deletes .pack file, verifies InternalError with message containing packfile path
Existing packfile integrity tests still pass (5/5)
test-readObject (17/17) and test-readBlob (10/10) unaffected

Summary by CodeRabbit

Bug Fixes
- Improved error handling and clearer messages when packfile data cannot be read or is missing, aiding diagnosis of corruption or absent files.
- Added guards that surface a specific internal error when pack data cannot be loaded, preventing silent failures.
Tests
- Added tests to verify descriptive error reporting and handling for packfile read failures.

FileSystem.read() silently returns null on errors (including ERR_FS_FILE_TOO_LARGE). In GitPackIndex.readSlice(), the null check tested the Promise object (always truthy) instead of the resolved value, causing a confusing TypeError on .slice(null). - Await the promise before null-checking in readSlice() - Add null check in readObjectPacked after resolving pack data - Clear cached promise on failure to allow retry on transient errors - Add test verifying descriptive InternalError on missing packfile

coderabbitai · 2026-03-12T19:44:45Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8e803251-a454-4cfd-9e7c-2ef617d02dd7

📥 Commits

Reviewing files that changed from the base of the PR and between 3c6a312 and 108fff9.

📒 Files selected for processing (1)

.github/workflows/ci.yml

📝 Walkthrough

Walkthrough

The changes add defensive guards and consolidate async pack loading to prevent operations on a missing packfile, plus a test that asserts a descriptive InternalError when the .pack file cannot be read.

Changes

Cohort / File(s)	Summary
Test Coverage `__tests__/test-packfileIntegrity.js`	Added a test that removes the `.pack` (leaving `.idx`) and asserts `InternalError` with a message containing "Could not read packfile" when reading a packed object.
PackIndex readSlice `src/models/GitPackIndex.js`	Consolidated awaiting `this.pack` into a local `pack` variable, use it for existence checks and slicing, and updated error messaging for pack read failure.
Pack read guard `src/storage/readObjectPacked.js`	Derives `packFile` path from indexFile and throws a descriptive `InternalError` if `p.pack` is falsy after awaiting, referencing the concrete pack file path and possible causes.
CI workflow `.github/workflows/ci.yml`	Changed the CLI test step to run `test.node` instead of `test.jest`.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

fix: add packfile integrity validation (fixes #2288) #2289 — Edits to src/storage/readObjectPacked.js and packfile error handling that overlap with this PR.

Poem

🐰 I sniffed a pack and found it thin,
An absent file — oh what a din!
I hopped, I checked, I threw a clue,
"Could not read packfile" — now we knew.
Thump-thump, debugged, the rabbit grins.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The CI workflow change from test.jest to test.node appears unrelated to packfile error handling and may be out of scope for the stated objective.	Clarify whether the CI workflow change is necessary for this fix, or revert it to a separate PR focused on CI test configuration.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main fix: improving error handling when packfile cannot be read, and references the fixed issue `#1163`.
Linked Issues check	✅ Passed	The PR directly addresses all coding requirements from issue `#1163`: null-checking the resolved pack value in readSlice, adding descriptive error handling in readObjectPacked, and adding a test verifying InternalError with packfile path.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📝 Coding Plan

Generate coding plan for human review comments

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

You can get early access to new features in CodeRabbit.

Enable the early_access setting to enable early access features such as new models, tools, and more.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@__tests__/test-packfileIntegrity.js`:
- Around line 153-179: Update the assertion to ensure the thrown InternalError
message includes the actual packfile path so regressions that strip the path
fail; after deleting the pack file (variables packDir and packFile) change the
existing expectation on error.data.message (used with readObjectPacked and
Errors.InternalError) to assert that it contains the concrete path (e.g.,
packDir + '/' + packFile or at least packFile) rather than only the generic
"Could not read packfile" prefix.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 569fc30e-f11c-428c-b90b-63023add5053

📥 Commits

Reviewing files that changed from the base of the PR and between b55bd5f and 3c6a312.

📒 Files selected for processing (3)

__tests__/test-packfileIntegrity.js
src/models/GitPackIndex.js
src/storage/readObjectPacked.js

coderabbitai · 2026-03-12T19:51:53Z

+    // Delete the .pack file but keep the .idx file
+    // This causes fs.read() to return null (file not found)
+    const packDir = gitdir + '/objects/pack'
+    const files = await fs.readdir(packDir)
+    const packFile = files.find(f => f.endsWith('.pack'))
+    if (!packFile) {
+      throw new Error('No packfile found in ' + packDir)
+    }
+    await fs.rm(packDir + '/' + packFile)
+
+    // Test - should throw InternalError with descriptive message
+    let error = null
+    try {
+      await readObjectPacked({
+        fs,
+        cache,
+        gitdir,
+        oid,
+      })
+    } catch (e) {
+      error = e
+    }
+
+    expect(error).not.toBeNull()
+    expect(error instanceof Errors.InternalError).toBe(true)
+    expect(error.data.message).toContain('Could not read packfile')
+  })


⚠️ Potential issue | 🟡 Minor

Assert the packfile path too.

Right now this only checks the generic prefix, so a regression that drops the actual path from the message would still pass.

🧪 Suggested test update

- await fs.rm(packDir + '/' + packFile) + const packPath = packDir + '/' + packFile + await fs.rm(packPath) ... expect(error.data.message).toContain('Could not read packfile') + expect(error.data.message).toContain(packPath)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed. In `@__tests__/test-packfileIntegrity.js` around lines 153 - 179, Update the assertion to ensure the thrown InternalError message includes the actual packfile path so regressions that strip the path fail; after deleting the pack file (variables packDir and packFile) change the existing expectation on error.data.message (used with readObjectPacked and Errors.InternalError) to assert that it contains the concrete path (e.g., packDir + '/' + packFile or at least packFile) rather than only the generic "Could not read packfile" prefix.

jcubic

Looks good

The CI workflow referenced `test.jest` which does not exist in package-scripts.cjs. The correct script name is `test.node`.

jcubic · 2026-03-13T10:14:55Z

🎉 This PR is included in version 1.37.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

costajohnt marked this pull request as ready for review March 12, 2026 19:44

coderabbitai Bot reviewed Mar 12, 2026

View reviewed changes

jcubic approved these changes Mar 12, 2026

View reviewed changes

fix(ci): correct test script name in CI workflow

108fff9

The CI workflow referenced `test.jest` which does not exist in package-scripts.cjs. The correct script name is `test.node`.

jcubic merged commit 7be8309 into isomorphic-git:main Mar 13, 2026
8 of 14 checks passed

jcubic added the released label Mar 13, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: improve error handling when packfile cannot be read (fixes #1163)#2295

fix: improve error handling when packfile cannot be read (fixes #1163)#2295
jcubic merged 2 commits into
isomorphic-git:mainfrom
costajohnt:fix/packindex-null-check

costajohnt commented Mar 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Mar 12, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Mar 12, 2026

Uh oh!

jcubic left a comment

Uh oh!

Uh oh!

jcubic commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

costajohnt commented Mar 12, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (2 warnings)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

jcubic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcubic commented Mar 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

costajohnt commented Mar 12, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Mar 12, 2026 •

edited

Loading