fix: throw on missing content blob oids#68
Merged
flyingrobots merged 10 commits intomainfrom Mar 14, 2026
Merged
Conversation
Release Preflight
If you tag this commit as |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMissing content blob OIDs now surface as PersistenceError(E_MISSING_OBJECT) instead of returning empty bytes; getContent()/getEdgeContent() return Uint8Array | null; GitGraphAdapter adds an existence check for empty reads; pre-push hook gains configurable launcher/binary wrappers and run_tool/command_exists helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GitGraphAdapter as Warp (GitGraphAdapter)
participant Git as "git (cat-file / cat-file -e)"
Client->>GitGraphAdapter: readBlob(oid)
GitGraphAdapter->>Git: stream blob data (cat-file -p oid)
alt blob data non-empty
Git-->>GitGraphAdapter: returns bytes
GitGraphAdapter-->>Client: return Uint8Array
else blob data empty
Git-->>GitGraphAdapter: returns zero bytes
GitGraphAdapter->>Git: cat-file -e oid (existence check)
alt object exists
Git-->>GitGraphAdapter: exit 0 (exists)
GitGraphAdapter-->>Client: return empty Uint8Array
else object missing
Git-->>GitGraphAdapter: exit non-zero / not found
GitGraphAdapter-->>Client: throw PersistenceError(E_MISSING_OBJECT, { oid })
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Release Preflight
If you tag this commit as |
![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
🧹 Nitpick comments (2)
scripts/hooks/pre-push (1)
59-60: Use configured binary name in Gate 0 skip message.The message always says “lychee not installed”, even when a custom link checker is configured, which can mislead debugging.
♻️ Suggested tweak
- echo "[Gate 0] Link check skipped (lychee not installed)" + echo "[Gate 0] Link check skipped ($LINKCHECK_BIN not installed)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-push` around lines 59 - 60, The Gate 0 skip echo is hardcoded to "lychee not installed"; update the echo in the Gate 0 skip path to interpolate the configured link-checker variable (e.g. use ${LINK_CHECKER:-lychee} or ${LINK_CHECKER_BIN:-lychee}) instead of the literal "lychee" so the message shows the actual configured binary name when skipping the link check.test/integration/api/content-attachment.test.js (1)
243-245: Prefer asserting the typed error code in integration tests.The current regex is permissive. Since this PR standardizes
E_MISSING_OBJECT, assertingerr.codehere would better protect the contract from regressions.✅ Suggested assertion style
- await expect(graph.getContent('doc:1')) - .rejects.toThrow(/Missing Git object|Blob not found|bad object/i); + await expect(graph.getContent('doc:1')) + .rejects.toMatchObject({ code: 'E_MISSING_OBJECT' }); - await expect(graph.getEdgeContent('a', 'b', 'rel')) - .rejects.toThrow(/Missing Git object|Blob not found|bad object/i); + await expect(graph.getEdgeContent('a', 'b', 'rel')) + .rejects.toMatchObject({ code: 'E_MISSING_OBJECT' });Also applies to: 263-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/api/content-attachment.test.js` around lines 243 - 245, Replace the loose regex-based rejection assertion with an assertion that checks the error's typed code (E_MISSING_OBJECT) when calling graph.getContent('doc:1') in the content-attachment integration test; specifically, change the await expect(graph.getContent(...)).rejects.toThrow(/.../) to await expect(graph.getContent(...)).rejects.toMatchObject({ code: 'E_MISSING_OBJECT' }) or await expect(graph.getContent(...)).rejects.toHaveProperty('code', 'E_MISSING_OBJECT') for both occurrences (around the getContent call and the other assertion at lines ~263-264) so the test explicitly asserts err.code instead of a permissive regex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/specs/CONTENT_ATTACHMENT.md`:
- Around line 115-117: Update the spec to explicitly state throw behavior for
getEdgeContent() to match getContent(): document that when an _edgeContent or
referenced blob OID is missing, getEdgeContent() will throw an error rather than
returning empty bytes, and mirror the guidance about consumers decoding bytes
(e.g., new TextDecoder().decode(buffer)); reference the same error semantics and
examples used for getContent() so the contract is unambiguous between
getContent() and getEdgeContent().
In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 376-382: The current logic treats any git error with exit code 1
or 128 as a missing object; narrow this by checking the git error message/output
for evidence it’s actually a missing object (e.g., strings like "bad object",
"fatal: ambiguous argument", "not found", or similar in
gitErr.message/gitErr.stderr) before mapping to
PersistenceError.E_MISSING_OBJECT; if that additional check fails, do not
classify as E_MISSING_OBJECT — instead rethrow the original error or wrap it in
a different PersistenceError (preserving cause) so repo/access errors aren’t
misreported; update the block around
wrapped/gitErr/getExitCode/PersistenceError/E_MISSING_OBJECT/oid to implement
this conditional check.
---
Nitpick comments:
In `@scripts/hooks/pre-push`:
- Around line 59-60: The Gate 0 skip echo is hardcoded to "lychee not
installed"; update the echo in the Gate 0 skip path to interpolate the
configured link-checker variable (e.g. use ${LINK_CHECKER:-lychee} or
${LINK_CHECKER_BIN:-lychee}) instead of the literal "lychee" so the message
shows the actual configured binary name when skipping the link check.
In `@test/integration/api/content-attachment.test.js`:
- Around line 243-245: Replace the loose regex-based rejection assertion with an
assertion that checks the error's typed code (E_MISSING_OBJECT) when calling
graph.getContent('doc:1') in the content-attachment integration test;
specifically, change the await
expect(graph.getContent(...)).rejects.toThrow(/.../) to await
expect(graph.getContent(...)).rejects.toMatchObject({ code: 'E_MISSING_OBJECT'
}) or await expect(graph.getContent(...)).rejects.toHaveProperty('code',
'E_MISSING_OBJECT') for both occurrences (around the getContent call and the
other assertion at lines ~263-264) so the test explicitly asserts err.code
instead of a permissive regex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67447dea-19f6-45c7-9b30-5ff78973bcc3
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdROADMAP.mddocs/specs/CONTENT_ATTACHMENT.mdscripts/hooks/pre-pushsrc/infrastructure/adapters/CasBlobAdapter.jssrc/infrastructure/adapters/GitGraphAdapter.jstest/integration/api/content-attachment.test.jstest/unit/domain/WarpGraph.content.test.jstest/unit/domain/services/GitGraphAdapter.test.jstest/unit/infrastructure/adapters/CasBlobAdapter.test.jstest/unit/scripts/pre-push-hook.test.js
Comment on lines
+115
to
117
| `getContent()` returns raw `Uint8Array` bytes. Consumers wanting text should decode with `new TextDecoder().decode(buffer)`. | ||
| If `_content` points at a missing blob OID, `getContent()` throws instead of silently returning empty bytes. | ||
|
|
There was a problem hiding this comment.
Document edge-content throw semantics explicitly too.
This section now states throw behavior for getContent(), but not getEdgeContent(). If both throw on missing blob OIDs, document both to avoid contract ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/specs/CONTENT_ATTACHMENT.md` around lines 115 - 117, Update the spec to
explicitly state throw behavior for getEdgeContent() to match getContent():
document that when an _edgeContent or referenced blob OID is missing,
getEdgeContent() will throw an error rather than returning empty bytes, and
mirror the guidance about consumers decoding bytes (e.g., new
TextDecoder().decode(buffer)); reference the same error semantics and examples
used for getContent() so the contract is unambiguous between getContent() and
getEdgeContent().
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/domain/services/GitGraphAdapter.test.js (1)
5-64: Consider adding a test for the exit-code-1 fallback path.The current tests exercise the
wrapGitErrorpattern-matching path (exit 128 + "bad object") but not theambiguousMissingObjectfallback (exit code 1 with empty stderr). While this is an edge case, adding a test would ensure the fallback logic remains correct:📝 Suggested test
it('throws E_MISSING_OBJECT for exit-1 with empty stderr (ambiguous case)', async () => { mockPlumbing.executeStream.mockResolvedValue({ collect: vi.fn().mockResolvedValue(Buffer.alloc(0)), }); const err = /** `@type` {any} */ (new Error('')); err.details = { code: 1, stderr: '' }; mockPlumbing.execute.mockRejectedValue(err); await expect(adapter.readBlob('deadbeef')) .rejects.toMatchObject({ code: 'E_MISSING_OBJECT', message: 'Missing Git object: deadbeef', }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/services/GitGraphAdapter.test.js` around lines 5 - 64, Add a unit test to cover the exit-code-1 ambiguous-missing-object fallback in GitGraphAdapter.readBlob: simulate executeStream.collect returning an empty Buffer and mock plumbing.execute to reject with an Error whose details are { code: 1, stderr: '' }, then assert adapter.readBlob('deadbeef') rejects with { code: 'E_MISSING_OBJECT', message: 'Missing Git object: deadbeef' }; place this alongside the existing readBlob tests to ensure the ambiguousMissingObject/wrapGitError fallback path is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/domain/services/GitGraphAdapter.test.js`:
- Around line 5-64: Add a unit test to cover the exit-code-1
ambiguous-missing-object fallback in GitGraphAdapter.readBlob: simulate
executeStream.collect returning an empty Buffer and mock plumbing.execute to
reject with an Error whose details are { code: 1, stderr: '' }, then assert
adapter.readBlob('deadbeef') rejects with { code: 'E_MISSING_OBJECT', message:
'Missing Git object: deadbeef' }; place this alongside the existing readBlob
tests to ensure the ambiguousMissingObject/wrapGitError fallback path is
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9284ec66-5d09-4dce-b137-43c63ec5a1c7
📒 Files selected for processing (3)
docs/specs/CONTENT_ATTACHMENT.mdsrc/infrastructure/adapters/GitGraphAdapter.jstest/unit/domain/services/GitGraphAdapter.test.js
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/hooks/pre-push (1)
55-60: Consider clarifying the skip message for non-readable scenarios.The message "lychee not installed" is technically imprecise when the skip reason is that the target file isn't readable (e.g., permissions issue). A more accurate message might be "lychee not available". However, this is a minor UX consideration and the current simplification is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/pre-push` around lines 55 - 60, Summary: The skip message "lychee not installed" can be misleading when the real reason is that lychee isn't accessible/readable; update the message to be more accurate. Change the echo in the else branch that currently prints "[Gate 0] Link check skipped (lychee not installed)" to a clearer message such as "[Gate 0] Link check skipped (lychee not available)" or "[Gate 0] Link check skipped (lychee not available or not readable)" so it covers both installation and accessibility issues; update the echo near the command_exists check for LINKCHECK_LAUNCHER and LINKCHECK_BIN that precedes run_tool to reflect this wording. Ensure only the message text is changed and the logic around command_exists, LINKCHECK_LAUNCHER, LINKCHECK_BIN, and run_tool remains untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/hooks/pre-push`:
- Around line 55-60: Summary: The skip message "lychee not installed" can be
misleading when the real reason is that lychee isn't accessible/readable; update
the message to be more accurate. Change the echo in the else branch that
currently prints "[Gate 0] Link check skipped (lychee not installed)" to a
clearer message such as "[Gate 0] Link check skipped (lychee not available)" or
"[Gate 0] Link check skipped (lychee not available or not readable)" so it
covers both installation and accessibility issues; update the echo near the
command_exists check for LINKCHECK_LAUNCHER and LINKCHECK_BIN that precedes
run_tool to reflect this wording. Ensure only the message text is changed and
the logic around command_exists, LINKCHECK_LAUNCHER, LINKCHECK_BIN, and run_tool
remains untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b6dc83e-53e8-49a7-ade5-4bd06ae6dd27
📒 Files selected for processing (4)
scripts/hooks/pre-pushsrc/domain/warp/query.methods.jstest/unit/domain/services/GitGraphAdapter.test.jstest/unit/scripts/pre-push-hook.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/domain/services/GitGraphAdapter.test.js
Release Preflight
If you tag this commit as |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GitGraphAdapter.readBlob()so swallowed missing-object reads no longer surface as empty bytesCloses #46.
Testing
Summary by CodeRabbit
Breaking Changes
Bug Fixes
Documentation
Tests
Chores