src/storage/readObjectPacked.js (1)

48-51: InternalError is semantically misleading for data-integrity failures.

InternalError's boilerplate message ("If you're not a developer, report the bug to the developers of the application you're using … please file a bug report at isomorphic-git/issues") describes a library defect, not external data corruption or tampering. A user whose packfile is genuinely corrupted would receive a confusing directive to file a bug against isomorphic-git.

This follows the existing pattern at Line 26 (if (p.error) throw new InternalError(p.error)), so changing it is low-priority. Longer term, a dedicated PackfileIntegrityError (or similar) would allow callers to distinguish a library bug from user-data integrity failures.

Also applies to: 59-62

🤖 Prompt for AI Agents

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

In `@src/storage/readObjectPacked.js` around lines 48 - 51, The code currently
throws InternalError for packfile integrity failures which is misleading; define
a new PackfileIntegrityError (e.g., class PackfileIntegrityError extends Error)
and export it, then replace the two throws of new InternalError(...) in
readObjectPacked.js (the pack trailer mismatch throw and the similar throw
around lines 59-62) with throws of new PackfileIntegrityError(...) preserving
the original message text; also update any local imports/exports so callers can
catch PackfileIntegrityError specifically.

__tests__/test-packfileIntegrity.js (1)

109-136: Test doesn't actually assert single-verification behavior

The test name and inline comment claim to verify that _checksumVerified prevents re-verification, but the body only confirms that two consecutive reads both succeed and return the same format. It passes even if verification runs twice (or zero times). To make this meaningful, the test should inspect the _checksumVerified flag on the cached pack entry directly after the first read, or spy on the underlying SHA computation.

♻️ Suggested approach to directly assert the flag

  // First read - should verify and set the flag
  const obj1 = await readObjectPacked({ fs, cache, gitdir, oid })
  expect(obj1).not.toBeNull()

+ // Confirm the flag was set after the first read
+ const packEntries = Object.values(cache)
+ const packEntry = packEntries.find(p => p._checksumVerified !== undefined)
+ expect(packEntry).toBeDefined()
+ expect(packEntry._checksumVerified).toBe(true)

  // Second read - should use cached verification
  const obj2 = await readObjectPacked({ fs, cache, gitdir, oid })
  expect(obj2).not.toBeNull()
  expect(obj2.format).toBe(obj1.format)

🤖 Prompt for AI Agents

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

In `@__tests__/test-packfileIntegrity.js` around lines 109 - 136, The test
currently only checks that two reads succeed but doesn't assert that the pack
entry's _checksumVerified flag was set after the first read; update the test to
inspect the cache/pack index entry after the first read and assert that its
_checksumVerified (or equivalent) property is true (e.g., locate the cached pack
entry via the same cache structure used by readObjectPacked and assert
entry._checksumVerified === true), then perform the second read to ensure it
still returns the same object; alternatively, spy/mock the SHA verification
function that readObjectPacked uses and assert it was called only once.

__tests__/test-packfileIntegrity.js (2)

115-142: Test doesn't actually assert the "only once" invariant

The test description claims to verify that _checksumVerified prevents re-verification, but the assertions (obj !== null, obj2.format === obj1.format) would pass even if checksum verification ran on every read. The test only confirms two reads succeed — not that the second read skips the checksum.

To make this test meaningful, inspect the cached index entry's flag or spy on shasum:

♻️ Suggested stronger assertion (inspect cache flag)

+    // Reach into the cache to confirm the _checksumVerified flag was set
+    // readPackIndex caches by filename; find the entry that has the flag
+    const cacheEntry = Object.values(cache).find(
+      entry => entry && typeof entry._checksumVerified !== 'undefined'
+    )
+    expect(cacheEntry).toBeDefined()
+    expect(cacheEntry._checksumVerified).toBe(true)

Alternatively, wrap shasum with a spy and assert it was called exactly once across both reads.

🤖 Prompt for AI Agents

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

In `@__tests__/test-packfileIntegrity.js` around lines 115 - 142, The test for
readObjectPacked falsely only checks that two reads succeed instead of asserting
that checksum verification ran only once; update the test to either (A) inspect
the pack index/cache entry after the first read and assert the
`_checksumVerified` flag (or equivalent flag) is set and still set after the
second read, referencing the same oid used in readObjectPacked, or (B) spy/mock
the hashing function (e.g., shasum) used by readObjectPacked and assert it was
invoked exactly once across both reads; modify the test around the two
readObjectPacked calls to check the `_checksumVerified` flag on the cached index
entry or add a spy on shasum and assert call count equals 1.

---

55-69: Consider Jest's .rejects matcher for cleaner error assertions

The manual try/catch + error = e accumulation pattern works but is more verbose than necessary. Since the same structure repeats in Test 3 (lines 98–112), a small helper or Jest's native matchers would reduce noise.

♻️ Suggested alternative using expect(...).rejects

-    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('Packfile trailer mismatch')
+    const promise = readObjectPacked({ fs, cache, gitdir, oid })
+    await expect(promise).rejects.toBeInstanceOf(Errors.InternalError)
+    await expect(promise).rejects.toMatchObject({
+      data: { message: expect.stringContaining('Packfile trailer mismatch') },
+    })

Note: promise would be evaluated twice (both .rejects calls), so the second await expect(promise) re-uses the same rejected promise — this is safe in JavaScript as a rejected Promise stays rejected.

Apply the same refactor to lines 98–112.

🤖 Prompt for AI Agents

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

In `@__tests__/test-packfileIntegrity.js` around lines 55 - 69, Replace the manual
try/catch pattern around readObjectPacked({fs, cache, gitdir, oid}) with Jest's
promise matchers: call await expect(readObjectPacked({fs, cache, gitdir,
oid})).rejects.toBeInstanceOf(Errors.InternalError) and then await
expect(readObjectPacked({fs, cache, gitdir, oid})).rejects.toMatchObject({ data:
{ message: expect.stringContaining('Packfile trailer mismatch') } }); apply the
same change to the second occurrence of the try/catch in the file (the block
covering the second test). Ensure you import or reference Errors and
readObjectPacked the same way they are currently used.