Skip to content

Comments

feat!: WARP-native content system (M13 VESSEL v4.0.1)#285

Merged
flyingrobots merged 7 commits intomainfrom
feat/warp-native-content
Feb 22, 2026
Merged

feat!: WARP-native content system (M13 VESSEL v4.0.1)#285
flyingrobots merged 7 commits intomainfrom
feat/warp-native-content

Conversation

@flyingrobots
Copy link
Owner

@flyingrobots flyingrobots commented Feb 22, 2026

Summary

Problem Statement

The content-on-node system in M13 VESSEL used raw git hash-object/git cat-file subprocess calls to implement CAS storage. This duplicated functionality that git-warp already provides natively via setContent()/getContent(), creating unnecessary shell invocation overhead and a wider attack surface (command injection risk from interpolated SHAs). The migration consolidates content storage into the WARP substrate where it belongs, per ADR-0004.

ADR Compliance (Required)

Relevant ADR(s)

  • ADR-0003 (Graph-Native Content, Deterministic Materialization, and Workspace Bridge)
  • None

Compliance Declaration

  • This PR is fully compliant with all checked ADRs.

Exception Request (Required if deviating)

N/A


Architecture Laws Checklist (Hard Gates)

Canonical Truth & Context

  • Graph remains canonical truth (no dual truth with generated files).
  • No hidden worktree coupling introduced in core/domain/materialization paths.
  • Context-sensitive behavior is explicit (--at, --observer, --trust) or deterministically defaulted.
  • Resolved context is surfaced in output metadata where applicable.

Determinism & Provenance

  • Pure query/materialization paths remain deterministic for identical inputs.
  • Mutations/materializations include provenance receipts/envelopes where required.
  • Cache keys (if used) are derived only from semantic inputs + pinned versions.

Artifact Hygiene

  • No forbidden generated artifact paths are tracked.
  • Any generated artifacts intentionally tracked are in allowlisted publish paths only.
  • Pre-commit/CI policy checks updated or confirmed valid.

Contracts & Compatibility

  • Machine-facing outputs are schema-versioned.
  • Breaking contract changes include version bump + migration notes.
  • Backward compatibility impact is documented below.

Extension/Effects Safety (if applicable)

N/A — no extension behavior changes.


Scope Control

  • PR is single-purpose/cohesive (no unrelated refactors).
  • Any non-essential refactor is split into separate PR(s) or explicitly justified.

Backward Compatibility

  • CLI/API contract changes: writeContent() and readContent() now use WARP's native content API. The _content.sha property is replaced by WARP's CONTENT_PROPERTY_KEY. Callers using the public API (writeContent, readContent, etc.) see no change — the interface is identical.
  • Data model/storage changes: Content blobs are stored via WARP's setContent() instead of raw git hash-object. Existing content written with the old API will not be readable without re-writing (breaking for any pre-existing content graphs on v3.x).
  • Migration required?: Yes — any graphs with content from v3.3.0 need content re-written. This is acceptable since content-on-node was introduced in v3.3.0 and has not been released to production users.
  • User-facing behavior changes: None — CLI commands behave identically.

Test Plan (Required)

Unit

  • Added/updated tests for changed logic
  • Commands:
npx vitest run test/content.test.js  # 37 tests pass

Integration

  • Added/updated integration tests
  • Commands:
npx vitest run  # 577 tests pass (29 files)

Determinism

  • Determinism assertions included for relevant paths
  • Method: Content write/read round-trip tests verify deterministic OID generation
  • Commands:
npx vitest run test/content.test.js

Contract/Schema

  • Schema validation updated/passing
  • Commands:
npx vitest run test/content.test.js  # includes schema contract tests
npx vitest run test/contracts.test.js

Policy Gates

  • Mechanical architecture gates pass
  • Commands:
# Pre-commit hooks run automatically
npx vitest run

Security / Trust Impact

  • Threat surface changed?: Reduced — eliminated all execSync/execFileSync subprocess calls from content module
  • Trust policy impact: None
  • Provenance/audit impact: None
  • New failure modes introduced: None — WARP native API has equivalent error semantics

Performance Impact

  • Hot path affected?: Content read/write now goes through WARP API instead of raw subprocess — marginal improvement (no shell fork)
  • Expected impact (latency/memory/io): Neutral to slightly improved
  • Benchmarks or profiling evidence: N/A — subprocess elimination is a clear win

Observability / Debuggability

  • Errors are actionable and include context.
  • Logs/diagnostics added or updated where needed.
  • git mind status / diagnostics updated if writeback/eventing behavior changed.

Operational Notes

  • Feature flag (if any): None
  • Rollback strategy: Revert to v3.3.0; content written with v4.x uses WARP native format
  • Operational caveats: None

Linked Issues / Milestones


Reviewer Quick Verdict Block (for maintainers)

MUST (Hard Gates)

•	PASS

SHOULD (Quality)

•	PASS

Verdict

•	APPROVE

Summary by CodeRabbit

  • Version Update

    • Released version 4.0.1.
  • Chores / Refactor

    • Content storage backend migrated to a WARP-managed OID-based system (no user-visible behavioral changes).
    • CLI content commands retain existing outputs and error handling while aligning with the new content backend.
  • Tests

    • Added coverage for storing empty-string content and updated tests to match the new content APIs.

BREAKING CHANGE: writeContent and readContent no longer take a `cwd`
parameter. The `_content.sha` property key is replaced by WARP's native
`_content` (CONTENT_PROPERTY_KEY). Version bumped to 4.0.0.

Replace execFileSync shell-outs to `git hash-object`/`git cat-file` with
WARP's native content API: patch.attachContent(), graph.getContent(),
graph.getContentOid(). This eliminates subprocess overhead, gains GC
protection, and removes redundant integrity re-hashing (git CAS
guarantees integrity by definition).

Removes: PREFIX, KEYS, SHA_RE, assertValidSha(), integrity re-hash test.
576 tests pass (36 content, 29 files).
- ROADMAP: correct stale integrity-check language to reflect WARP-native storage
- src/content.js: JSDoc SHA → OID terminology
- test/content.test.js: remove dead execSync import, add empty-content test
- Bump 4.0.0 → 4.0.1, add missing 4.0.0 changelog entry
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026

Warning

Rate limit exceeded

@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 47 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

Bumped package to 4.0.1 and migrated content storage from raw Git commands to git-warp's native Graph API. Content helpers’ signatures changed to remove the cwd/tempDir parameter; content metadata now uses CONTENT_PROPERTY_KEY (OID) plus MIME_KEY and SIZE_KEY.

Changes

Cohort / File(s) Summary
Version
package.json
Bumped package version from 3.3.0 to 4.0.1.
Content core
src/content.js
Rewrote storage to use WARP Graph API (attachContent / getContent / getContentOid). Removed direct git subprocesses and git SHA handling; switched to CONTENT_PROPERTY_KEY (OID) and MIME/SIZE keys. Updated function signatures to accept graph first (removed cwd).
CLI call sites
src/cli/commands.js
Updated callers to writeContent and readContent to match new signatures (no cwd).
Tests
test/content.test.js
Updated tests and helper calls to use graph-first signatures. Imported CONTENT_PROPERTY_KEY. Added test for empty-string content and updated metadata assertions.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant ContentModule as Content.js
  participant Graph as GitWarpGraph
  CLI->>ContentModule: contentSet(graph, nodeId, buf, {mime})
  ContentModule->>Graph: patch.attachContent(nodeId, buf, {mime})
  Graph-->>ContentModule: contentOid
  ContentModule->>Graph: graph.setProperty(nodeId, CONTENT_PROPERTY_KEY, contentOid + meta)
  ContentModule-->>CLI: WriteContentResult{ oid, mime, size }

  CLI->>ContentModule: contentShow(graph, nodeId)
  ContentModule->>Graph: graph.getContentOid(nodeId)
  Graph-->>ContentModule: contentOid
  ContentModule->>Graph: graph.getContent(contentOid)
  Graph-->>ContentModule: content bytes
  ContentModule-->>CLI: content bytes + meta
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Poem

🐇 I hopped through code, removed the old cwd,
WARP wrapped my blobs with an OID so steady,
MIME and size snug where properties be,
Tests now whisper empty strings — hooray for me! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat!: WARP-native content system (M13 VESSEL v4.0.1)' accurately describes the main breaking change—migrating to WARP's native content API—with a version indicator, making it clear and specific.
Description check ✅ Passed The PR description is comprehensive and follows the template well: includes a detailed summary, problem statement, ADR compliance declaration with checkboxes, architectural laws checklist, scope control, backward compatibility details, test plan with commands, security/performance impact, and operational notes.
Linked Issues check ✅ Passed The PR successfully implements all requirements from issue #284: replaces execFileSync calls with git-warp's native content API (attachContent/getContent), removes cwd parameters, updates property keys, eliminates legacy helpers (PREFIX, KEYS, SHA_RE, assertValidSha), and includes version bump and test updates.
Out of Scope Changes check ✅ Passed All code changes are scoped to the WARP migration: content module refactoring, CLI command updates, test suite alignment, and package version bump. Minor nit fixes (ROADMAP language, JSDoc SHA→OID, empty-content test) are directly related to the migration and do not introduce unrelated refactoring.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ 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
  • Commit unit tests in branch feat/warp-native-content

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@package.json`:
- Line 3: Ensure the installed `@git-stunts/git-warp`@11.5.0 actually exports
CONTENT_PROPERTY_KEY, patch.attachContent, graph.getContent,
graph.getContentOid, and graph.getNodeProps which src/content.js relies on; to
fix, either bump or pin the dependency in package.json to a version that exports
those symbols, or change the imports in src/content.js to the correct export
names/paths from the library, and add a short runtime guard in src/content.js
that checks for these five exports and throws a clear error listing any missing
symbols so the issue fails fast.

In `@src/content.js`:
- Around line 135-136: The hasContent function currently returns sha !== null
but graph.getContentOid may return undefined too; change hasContent to use a
truthiness/nullish check (e.g., return Boolean(sha) or return sha != null) so it
treats both null and undefined as "no content"; update the return in hasContent
(which calls graph.getContentOid(nodeId)) to use a truthy check consistent with
other callers (!sha) to avoid false positives.
- Around line 89-95: Adjust the empty-blob guard and reintroduce strict UTF-8
validation: change the throw condition to only error when contentBuf is missing
for non-empty blobs (i.e., throw if !contentBuf && meta.size > 0) and treat
missing contentBuf for meta.size === 0 as an empty Buffer/text; then validate
UTF-8 strictly before returning by decoding and re-encoding the buffer (e.g.,
decode with toString('utf8') then re-encode and compare to the original buffer)
and throw a descriptive error if the re-encoded buffer does not equal contentBuf
so non-UTF-8 blobs fail (references: contentBuf, meta.size, meta.sha, nodeId,
graph.getContent(), readContent()).
- Around line 159-163: The current deleteContent implementation calls
patch.setProperty(nodeId, CONTENT_PROPERTY_KEY, null) (and similarly for
MIME_KEY and SIZE_KEY), which stores literal null rather than removing the
property; change the implementation to either perform node-level removal using
graph.removeNode(nodeId) when you intend to fully delete the entity, or adjust
deleteContent's contract and callers to treat the operation as “set properties
to null” (i.e., document that setProperty(..., null) is used instead of removal
and do not expect the key to be absent); update references to setProperty,
CONTENT_PROPERTY_KEY, MIME_KEY, SIZE_KEY and use graph.removeNode/removeEdge
where true deletion is required.

{
"name": "@neuroglyph/git-mind",
"version": "3.3.0",
"version": "4.0.1",
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify that the WARP API surface used in this PR is present in the installed version
echo "=== Installed `@git-stunts/git-warp` version ==="
node -e "import('@git-stunts/git-warp').then(m => { console.log('Exports:', Object.keys(m)); console.log('CONTENT_PROPERTY_KEY:', m.CONTENT_PROPERTY_KEY); }).catch(e => console.error(e.message))" 2>&1 || echo "Module import check completed"

echo ""
echo "=== Checking package.json for `@git-stunts/git-warp` version ==="
cat package.json | jq -r '.dependencies["@git-stunts/git-warp"] // .devDependencies["@git-stunts/git-warp"] // "not found"'

echo ""
echo "=== Search for CONTENT_PROPERTY_KEY in warp package ==="
rg "CONTENT_PROPERTY_KEY" node_modules/@git-stunts/git-warp/ -n --include="*.js" -l 2>/dev/null | head -10

echo ""
echo "=== Search for attachContent / getContentOid / getNodeProps ==="
rg "attachContent|getContentOid|getNodeProps\b" node_modules/@git-stunts/git-warp/ -n --include="*.js" -l 2>/dev/null | head -10

echo ""
echo "=== Checking if node_modules/@git-stunts/git-warp exists ==="
ls -la node_modules/@git-stunts/ 2>/dev/null | grep git-warp || echo "@git-stunts/git-warp not in node_modules"

Repository: neuroglyph/git-mind

Length of output: 489


🏁 Script executed:

#!/bin/bash
echo "=== Checking package-lock.json for exact `@git-stunts/git-warp` version ==="
jq -r '.packages."node_modules/@git-stunts/git-warp".version // "not found"' package-lock.json 2>/dev/null || echo "package-lock.json check failed"

echo ""
echo "=== Searching for CONTENT_PROPERTY_KEY usage in codebase ==="
rg "CONTENT_PROPERTY_KEY" --include="*.js" --include="*.mjs" -n -B2 -A2 2>/dev/null | head -30

echo ""
echo "=== Searching for attachContent usage ==="
rg "attachContent\(" --include="*.js" --include="*.mjs" -n -B2 -A2 2>/dev/null | head -30

echo ""
echo "=== Searching for getContent / getContentOid usage ==="
rg "getContent|getContentOid" --include="*.js" --include="*.mjs" -n -B1 -A1 2>/dev/null | head -30

echo ""
echo "=== Searching for getNodeProps usage ==="
rg "getNodeProps" --include="*.js" --include="*.mjs" -n -B1 -A1 2>/dev/null | head -30

echo ""
echo "=== Check what this PR actually modifies ==="
git diff HEAD~1 --name-only 2>/dev/null | head -20

Repository: neuroglyph/git-mind

Length of output: 392


🏁 Script executed:

#!/bin/bash
echo "=== Full package.json ==="
cat -n package.json

echo ""
echo "=== Search for git-warp imports ==="
rg "from\s+['\"]@git-stunts/git-warp|require\(['\"]@git-stunts/git-warp" --include="*.js" --include="*.mjs" -n 2>/dev/null

echo ""
echo "=== Check src/content.js mentioned in learnings ==="
if [ -f src/content.js ]; then
  wc -l src/content.js
  echo "--- First 50 lines ---"
  head -50 src/content.js
fi

echo ""
echo "=== List all .js/.mjs files in src/ ==="
find src -type f \( -name "*.js" -o -name "*.mjs" \) 2>/dev/null | head -20

Repository: neuroglyph/git-mind

Length of output: 3465


🏁 Script executed:

#!/bin/bash
echo "=== Full src/content.js ==="
cat -n src/content.js

echo ""
echo "=== Search for method calls on graph/patch objects ==="
rg "graph\.(getContent|getContentOid|getNodeProps|attachContent)" src/content.js -n 2>/dev/null
rg "patch\.(attachContent)" src/content.js -n 2>/dev/null

Repository: neuroglyph/git-mind

Length of output: 6789


Verify @git-stunts/git-warp@11.5.0 exports the required content API surface.

The content system (src/content.js) actively uses CONTENT_PROPERTY_KEY, patch.attachContent(), graph.getContent(), graph.getContentOid(), and graph.getNodeProps(). All five are called at runtime across writeContent, readContent, getContentMeta, hasContent, and deleteContent. If any of these exports do not exist in v11.5.0, the content subsystem will fail at runtime.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 3, Ensure the installed `@git-stunts/git-warp`@11.5.0
actually exports CONTENT_PROPERTY_KEY, patch.attachContent, graph.getContent,
graph.getContentOid, and graph.getNodeProps which src/content.js relies on; to
fix, either bump or pin the dependency in package.json to a version that exports
those symbols, or change the imports in src/content.js to the correct export
names/paths from the library, and add a short runtime guard in src/content.js
that checks for these five exports and throws a clear error listing any missing
symbols so the issue fails fast.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for flagging! This is a false positive — CodeRabbit sandbox could not resolve node_modules, but the API surface is fully verified locally. All 5 symbols (CONTENT_PROPERTY_KEY, patch.attachContent, graph.getContent, graph.getContentOid, graph.getNodeProps) are exercised by the test suite. 37 content tests pass covering every code path. No action needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flyingrobots, thank you for the clarification! You're absolutely right — the sandbox limitation prevented me from verifying the node_modules, but your local test coverage with 37 passing content tests clearly confirms that all five API symbols are present and functioning correctly in @git-stunts/git-warp@11.5.0. I appreciate you taking the time to confirm this. No action needed indeed!


🧠 Learnings used
Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T17:11:37.241Z
Learning: Applies to {package.json,package-lock.json,**/*.{js,mjs}} : Use `git-stunts/git-warp` as a core dependency for CRDT graph on Git

Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.

Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 276
File: src/content.js:110-127
Timestamp: 2026-02-22T13:15:58.473Z
Learning: In the git-mind repository, the M13 content system (src/content.js) is intentionally text-only (UTF-8). Non-UTF-8 blobs are designed to fail the integrity check in readContent() as a boundary enforcement mechanism. Binary content support is deferred to a future milestone. The _content.encoding property was deliberately removed.

Comment on lines 159 to 163
const patch = await graph.createPatch();
patch.setProperty(nodeId, KEYS.sha, null);
patch.setProperty(nodeId, KEYS.mime, null);
patch.setProperty(nodeId, KEYS.size, null);
patch.setProperty(nodeId, CONTENT_PROPERTY_KEY, null);
patch.setProperty(nodeId, MIME_KEY, null);
patch.setProperty(nodeId, SIZE_KEY, null);
await patch.commit();
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# First, check if the WARP library is available in the repository
find . -name "git-warp*" -type d 2>/dev/null | head -10

Repository: neuroglyph/git-mind

Length of output: 45


🏁 Script executed:

# Also search the codebase for WARP API usage patterns
rg "\.attachContent\(|\.setProperty\(|\.getContentOid\(|deleteContent" --include="*.js" -B 2 -A 2 2>/dev/null | head -60

Repository: neuroglyph/git-mind

Length of output: 45


🏁 Script executed:

# Search for any documentation or types that might indicate WARP's content detach mechanisms
rg "detach|removeContent|deleteContent" --include="*.js" --include="*.md" --include="*.json" -B 2 -A 2 2>/dev/null | head -80

Repository: neuroglyph/git-mind

Length of output: 45


🏁 Script executed:

# Check package.json for the exact version of `@git-stunts/git-warp` being used
cat package.json | grep -A 5 -B 5 "@git-stunts/git-warp" 2>/dev/null || echo "WARP dependency not found in package.json"

Repository: neuroglyph/git-mind

Length of output: 379


🏁 Script executed:

wc -l src/content.js

Repository: neuroglyph/git-mind

Length of output: 81


🏁 Script executed:

cat -n src/content.js | sed -n '150,175p'

Repository: neuroglyph/git-mind

Length of output: 645


🏁 Script executed:

# Search for the deleteContent function definition
rg "deleteContent\s*=" src/content.js -B 5 -A 20

Repository: neuroglyph/git-mind

Length of output: 45


🏁 Script executed:

# Check for any imports or references to WARP patch methods
rg "import|require" src/content.js | head -20

Repository: neuroglyph/git-mind

Length of output: 518


🌐 Web query:

@git-stunts/git-warp v11 setProperty null deletion API documentation

💡 Result:

In @git-stunts/git-warp v11 there is no “delete/unset property” operation in the patch API—PatchBuilderV2 / PatchSession only expose .setProperty(nodeId, key, value) (and .setEdgeProperty(...)) plus node/edge add/remove. There is no .removeProperty()/.unsetProperty() method. [1][2]

What you can do:

  • Store null as the property value: property values are documented as JSON-serializable, explicitly including null, and setProperty(..., null) is valid. This does not remove the key; it sets the value to null, so you treat null as your app-level “deleted” sentinel (and you can query for it, e.g. .where({ status: null })). [3]
  • If you truly need to wipe properties, the API-supported way is to remove (tombstone) the entire node/edge (e.g. .removeNode(nodeId) or .removeEdge(...)), which makes its properties invisible as part of deletion semantics—but that’s entity deletion, not per-key deletion. [1][3]

So: setProperty(..., null) is supported, but it’s not a deletion API—it’s just setting the value to null. [2][3]

Sources:
[1] JSR docs: PatchBuilderV2 API (methods list incl. setProperty, no remove/unset)
[2] JSR package types (index.d.ts): setProperty(nodeId, key, value: unknown) (no remove/unset)
[3] GitHub GUIDE.md (raw): property values JSON-serializable incl. null; examples of querying null


🌐 Web query:

git-warp WARP detachContent removeContent API reference

💡 Result:

I couldn’t find any detachContent / removeContent APIs in the Rust git-warp crate docs (the public API is organized into modules like config, cow, git, terminal, etc., and those symbols don’t appear there). [1]

The closest match for DetachContent and RemoveContent (same names, different casing) is in Brave’s ai_chat::AssociatedContentManager (C++):

  • void RemoveContent(AssociatedContentDelegate* delegate, bool notify_updated = true); [2]
  • void DetachContent(); (private helper) [2]
  • Implementation references show DetachContent() being called to clear existing associated content, and RemoveContent(...) being used to remove items (including archived content cleanup). [3]

Sources: [1] [2] [3]


🏁 Script executed:

rg "getContentOid" src/ --include="*.js" -B 3 -A 10

Repository: neuroglyph/git-mind

Length of output: 504


🏁 Script executed:

# Also check if there's any documentation or comments about null handling
rg "CONTENT_PROPERTY_KEY" src/ --include="*.js" -B 2 -A 2

Repository: neuroglyph/git-mind

Length of output: 504


🏁 Script executed:

rg "getContentOid" src/ -B 3 -A 10

Repository: neuroglyph/git-mind

Length of output: 2513


🏁 Script executed:

rg "CONTENT_PROPERTY_KEY" src/ -B 2 -A 2

Repository: neuroglyph/git-mind

Length of output: 778


setProperty(CONTENT_PROPERTY_KEY, null) does not delete the property; it stores the literal value null.

WARP v11 has no removeProperty() API. The setProperty(key, null) call stores null as the property value rather than deleting the key. This means deleteContent returns removed: true while the property still exists (with null value), creating a semantic correctness failure. The only true deletion in WARP is via removeNode() or removeEdge() for entity-level removal. Either use node removal for actual deletion, or reconsider the function's contract and ensure callers understand that properties are set to null rather than removed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/content.js` around lines 159 - 163, The current deleteContent
implementation calls patch.setProperty(nodeId, CONTENT_PROPERTY_KEY, null) (and
similarly for MIME_KEY and SIZE_KEY), which stores literal null rather than
removing the property; change the implementation to either perform node-level
removal using graph.removeNode(nodeId) when you intend to fully delete the
entity, or adjust deleteContent's contract and callers to treat the operation as
“set properties to null” (i.e., document that setProperty(..., null) is used
instead of removal and do not expect the key to be absent); update references to
setProperty, CONTENT_PROPERTY_KEY, MIME_KEY, SIZE_KEY and use
graph.removeNode/removeEdge where true deletion is required.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the deep investigation! This is a known WARP v11 behavior pattern documented in our project notes. WARP v11 has no removeProperty() API — setProperty(key, null) is the established deletion sentinel. All our readers check values (truthiness/nullish), never key presence: getContentOid() returns null after deletion, hasContent() returns false, getContentMeta() returns null. The full delete/re-check cycle is covered by the deleteContent removes properties test. No action needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flyingrobots, thank you for the clarification! I appreciate you explaining the intentional design pattern. I've noted that WARP v11's setProperty(key, null) is the established deletion sentinel and that all reader functions check values rather than key presence. This makes the implementation correct as-is.

- readContent: use explicit `contentBuf == null` instead of `!contentBuf`
  for clearer intent on null/undefined check
- hasContent: use loose equality `sha != null` to catch both null and
  undefined from getContentOid(), consistent with other callers
@flyingrobots
Copy link
Owner Author

@coderabbitai review please 🥕

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026

@flyingrobots: I'll review the changes in PR #285 for you! 🥕

This looks like a significant migration to WARP-native content APIs with breaking changes to the content store signatures. I'll analyze the refactoring, API changes, test coverage, and ensure the migration is clean.


🧠 Learnings used
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.

Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 276
File: src/content.js:110-127
Timestamp: 2026-02-22T13:15:58.473Z
Learning: In the git-mind repository, the M13 content system (src/content.js) is intentionally text-only (UTF-8). Non-UTF-8 blobs are designed to fail the integrity check in readContent() as a boundary enforcement mechanism. Binary content support is deferred to a future milestone. The _content.encoding property was deliberately removed.

Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T17:11:37.241Z
Learning: Applies to {package.json,package-lock.json,**/*.{js,mjs}} : Use `git-stunts/git-warp` as a core dependency for CRDT graph on Git
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/content.js`:
- Around line 61-63: The call to graph.getContentOid(nodeId) in writeContent can
return null and currently is returned as sha violating the WriteContentResult
JSDoc; add a null-guard after the await (in the writeContent function) to detect
if sha is null/undefined and throw a clear Error (including nodeId and operation
context) rather than returning null, and update the JSDoc or types if needed to
reflect that callers will always receive a string sha or an exception; reference
graph.getContentOid, writeContent, nodeId and WriteContentResult when making the
change.
- Around line 83-87: The catch around graph.getContent() is too broad and hides
the original failure; change the handler to capture the error (e.g., catch
(err)) and either rethrow the original err or throw the "Content blob ... not
found" error with the original error as its cause so callers can distinguish
missing-blob vs infrastructure errors; update the block around
graph.getContent(), meta.sha and nodeId to preserve and surface the original
error (or inspect err to only map NotFound errors to the custom message).

---

Duplicate comments:
In `@src/content.js`:
- Line 95: The current return uses contentBuf.toString('utf-8') which silently
replaces invalid UTF-8 sequences; update readContent() to perform strict UTF-8
validation and throw on invalid sequences instead of returning corrupted text:
decode the Buffer with a UTF-8 decoder configured to be fatal (e.g.,
TextDecoder('utf-8', { fatal: true })) or otherwise validate the byte sequence
before converting, and if decoding fails throw a clear error (e.g., "Non-UTF-8
content encountered") so binary/octet-stream blobs fail the integrity check
rather than being silently corrupted.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/content.js`:
- Around line 61-63: The returned sha from graph.getContentOid(nodeId) is not
null-guarded, violating the WriteContentResult.sha: {string} contract; after the
call to graph.getContentOid (referencing getContentOid and the surrounding
return { nodeId, sha, mime, size }), add a defensive check that sha is a
non-empty string and if not throw a descriptive Error (or otherwise
convert/fetch a valid string) so callers never receive sha: null/undefined.
- Around line 83-87: The catch block around graph.getContent() is too broad and
hides the original error; change the catch to capture the exception (e.g., catch
(err)) and rethrow a new, distinct error that preserves the original cause (use
Error's cause option or include err.message/stack) while keeping the existing
"Content blob ... not found" message only for the explicit null/empty-return
path; specifically update the try/catch that surrounds graph.getContent()
(reference graph.getContent, meta.sha, nodeId) to differentiate a missing blob
vs an infrastructure/runtime exception and include the original error details in
the thrown error.

- writeContent: throw if getContentOid returns null after successful write
- readContent: capture and chain original error via { cause: err }
- readContent: unify error messages to "Failed to retrieve content blob"
- Update test regex to match new error wording
@flyingrobots
Copy link
Owner Author

@coderabbitai review please 🥕

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 22, 2026

@flyingrobots: Triggering the review now! 🥕


🧠 Learnings used
Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 169
File: docs/templates/code-review-feedback.md:26-33
Timestamp: 2025-09-16T17:10:27.477Z
Learning: The user (flyingrobots) uses Obsidian to view/edit Markdown files and prefers to keep Obsidian-specific syntax like collapsible callout markers (e.g., `[!NOTE]-` for collapsible callouts) even if they're not GitHub-compatible.

Learnt from: flyingrobots
Repo: neuroglyph/git-mind PR: 276
File: src/content.js:110-127
Timestamp: 2026-02-22T13:15:58.473Z
Learning: In the git-mind repository, the M13 content system (src/content.js) is intentionally text-only (UTF-8). Non-UTF-8 blobs are designed to fail the integrity check in readContent() as a boundary enforcement mechanism. Binary content support is deferred to a future milestone. The _content.encoding property was deliberately removed.

Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-20T17:11:37.241Z
Learning: Applies to {package.json,package-lock.json,**/*.{js,mjs}} : Use `git-stunts/git-warp` as a core dependency for CRDT graph on Git

Learnt from: CR
Repo: neuroglyph/git-mind PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-07T14:14:58.924Z
Learning: Conventional commits (type(scope): description) and reference issues; PRs must describe changes, link issues, include test plan, and update docs when applicable
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

- Error message conventions for content module
- Audit try/catch blocks for cause preservation
- Integration test for error.cause chain
- --verbose flag for content CLI error diagnostics
- Update stale execFileSync note (content.js has no subprocesses now)
@flyingrobots flyingrobots merged commit 6bd1e56 into main Feb 22, 2026
8 checks passed
@flyingrobots flyingrobots deleted the feat/warp-native-content branch February 22, 2026 19:03
flyingrobots added a commit that referenced this pull request Feb 22, 2026
CI/CD hygiene:
- Version consistency gate (package.json ↔ package-lock.json)
- Changelog entry validation for current version

Content system UX:
- content set from stdin
- Content health checks in git mind doctor
- --as-of guard for content commands
- Content CLI fast-path unit tests
- WARP-native migration guide
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate content system to git-warp native API

1 participant