refactor: use git-warp API properly — eliminate pass-through wrappers#296
refactor: use git-warp API properly — eliminate pass-through wrappers#296flyingrobots merged 6 commits intomainfrom
Conversation
- Remove getNodes(graph) wrapper — callers use graph.getNodes() directly - Remove hasNode(graph, id) wrapper — callers use graph.hasNode() directly - Remove saveGraph(graph) — dead code with zero call sites - Replace all loadGraph() calls with initGraph() (they were identical) - Keep loadGraph as deprecated alias for public API backward compat
- Remove queryEdges() — inline filter at CLI call site, tests use graph.getEdges() directly - Remove getNodesByPrefix() — inline filter at single CLI call site - Keep getNode() — adds real value (Map→object, prefix classification)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughRemoves five pass-through wrapper exports (getNodes, hasNode, saveGraph, queryEdges, getNodesByPrefix), marks Changes
Sequence Diagram(s)(Skipped — changes are API surface removals and call-site replacements; no new multi-component interaction flow added that requires sequence visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 660e297da5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
package.json
Outdated
| { | ||
| "name": "@neuroglyph/git-mind", | ||
| "version": "4.0.1", | ||
| "version": "4.1.0", |
There was a problem hiding this comment.
This release removes previously exported symbols (saveGraph, queryEdges, getNodes, hasNode, getNodesByPrefix) from src/index.js, so existing consumers on ^4.0.0 that import any of these names will fail module loading after upgrading (does not provide an export named ...). Since the package version here is only bumped to 4.1.0, this is a SemVer-breaking change shipped as a minor; either keep deprecated re-exports for the 4.x line or bump to 5.0.0.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@CHANGELOG.md`:
- Around line 10-23: Add a link definition for the new release tag [4.1.0] at
the bottom link section to match other versions (place it alongside the existing
definitions, e.g., before the `[3.1.0]` link block) so the `[4.1.0]` reference
becomes a proper hyperlink; also rename the "Removed" section heading to
"Breaking" (or add a "Breaking" section) in the `[4.1.0]` release notes to
reflect that the removed wrappers (getNodes(graph), hasNode(graph, id),
saveGraph(graph), queryEdges(graph, filter), getNodesByPrefix(graph, prefix))
are breaking changes while keeping the `loadGraph()`/`initGraph()` note as-is.
In `@package.json`:
- Line 3: Version bump in package.json is incorrect for a breaking change:
update the "version" field from "4.1.0" to "5.0.0" to reflect removal of public
exports; specifically ensure consumers of removed symbols (getNodes, hasNode,
saveGraph, queryEdges, getNodesByPrefix) see a major version increase and update
any changelog/release notes referencing src/index.js to document the removed API
surface.
In `@src/index.js`:
- Around line 6-8: The public API was reduced by removing exports saveGraph,
queryEdges, getNodes, hasNode, and getNodesByPrefix; update consumer docs by
adding a concise MIGRATION.md (and update README/API docs) that lists each
removed symbol and the exact migration snippet: replace queryEdges(...) with
graph.getEdges(... /* apply inline filter */), replace getNodes() with
graph.getNodes(), replace hasNode(id) with graph.hasNode(id), replace
getNodesByPrefix(prefix) with graph.getNodes().filter(id => id.startsWith(prefix
+ ':')), and note saveGraph is removed (dead code) with recommended alternative
or confirmation it's unnecessary; reference the CHANGELOG entry and include
minimal example code for each mapping.
In `@test/review.test.js`:
- Around line 95-96: Extract a small async test helper to avoid repeating the
(await graph.getEdges()).filter(...) pattern: add a helper function named
findEdge (or findEdges) inside the top of the describe/test file that accepts
(graph, from, to, label), calls graph.getEdges(), and returns the filtered
array; then replace the seven occurrences (e.g., the current inline uses in
assertions like const edges = (await graph.getEdges()).filter(e => e.from === X
&& e.to === Y && e.label === Z)) with calls to the helper (await findEdge(graph,
'task:a', 'spec:b', 'implements')) so tests remain identical but duplication is
removed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
CHANGELOG.mdpackage.jsonsrc/cli/commands.jssrc/diff.jssrc/edges.jssrc/graph.jssrc/index.jssrc/nodes.jstest/edges.test.jstest/graph.test.jstest/hooks.test.jstest/merge.test.jstest/nodes.test.jstest/remote.test.jstest/resolve-context.test.jstest/review.test.js
💤 Files with no reviewable changes (2)
- src/edges.js
- src/nodes.js
CHANGELOG.md
Outdated
| ## [4.1.0] - 2026-02-25 | ||
|
|
||
| ### Removed | ||
|
|
||
| - **`getNodes(graph)` wrapper** — Use `graph.getNodes()` directly (#295) | ||
| - **`hasNode(graph, id)` wrapper** — Use `graph.hasNode(id)` directly (#295) | ||
| - **`saveGraph(graph)` wrapper** — Dead code with zero call sites (#295) | ||
| - **`queryEdges(graph, filter)` wrapper** — Use `graph.getEdges()` with inline filter (#295) | ||
| - **`getNodesByPrefix(graph, prefix)` wrapper** — Use `graph.getNodes()` with `startsWith()` filter (#295) | ||
|
|
||
| ### Changed | ||
|
|
||
| - **All internal `loadGraph()` calls replaced with `initGraph()`** — `loadGraph` kept as deprecated alias for public API backward compatibility (#295) | ||
|
|
There was a problem hiding this comment.
Missing [4.1.0] link definition at end of file.
Other versions (3.1.0, 3.0.0, etc.) have corresponding link definitions at the bottom of the file (e.g., line 374-380). The [4.1.0] reference on line 10 will render as plain text, not a hyperlink.
Additionally, if you agree this is a breaking change (removal of public exports), the "Removed" section should be a "Breaking" section to match the pattern used in [3.0.0] (line 151).
Add link definition
Add at line ~374 (before [3.1.0]):
+[4.1.0]: https://github.com/neuroglyph/git-mind/releases/tag/v4.1.0
[3.1.0]: https://github.com/neuroglyph/git-mind/releases/tag/v3.1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 10 - 23, Add a link definition for the new release
tag [4.1.0] at the bottom link section to match other versions (place it
alongside the existing definitions, e.g., before the `[3.1.0]` link block) so
the `[4.1.0]` reference becomes a proper hyperlink; also rename the "Removed"
section heading to "Breaking" (or add a "Breaking" section) in the `[4.1.0]`
release notes to reflect that the removed wrappers (getNodes(graph),
hasNode(graph, id), saveGraph(graph), queryEdges(graph, filter),
getNodesByPrefix(graph, prefix)) are breaking changes while keeping the
`loadGraph()`/`initGraph()` note as-is.
package.json
Outdated
| { | ||
| "name": "@neuroglyph/git-mind", | ||
| "version": "4.0.1", | ||
| "version": "4.1.0", |
There was a problem hiding this comment.
Semver violation: removing public exports requires a MAJOR version bump, not MINOR.
The PR removes getNodes, hasNode, saveGraph, queryEdges, and getNodesByPrefix from the public API (src/index.js). Consumers importing these symbols will encounter runtime errors upon upgrading from 4.0.x to 4.1.0. Per Semantic Versioning, this constitutes a breaking change and mandates a major version increment.
This should be 5.0.0, not 4.1.0.
Proposed fix
- "version": "4.1.0",
+ "version": "5.0.0",📝 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.
| "version": "4.1.0", | |
| "version": "5.0.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 3, Version bump in package.json is incorrect for a
breaking change: update the "version" field from "4.1.0" to "5.0.0" to reflect
removal of public exports; specifically ensure consumers of removed symbols
(getNodes, hasNode, saveGraph, queryEdges, getNodesByPrefix) see a major version
increase and update any changelog/release notes referencing src/index.js to
document the removed API surface.
| export { initGraph, loadGraph } from './graph.js'; | ||
| export { createEdge, removeEdge, EDGE_TYPES } from './edges.js'; | ||
| export { getNode, setNodeProperty, unsetNodeProperty } from './nodes.js'; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Breaking API surface reduction — ensure migration docs are adequate.
Five exports removed:
saveGraph— dead code (per CHANGELOG)queryEdges→graph.getEdges()with inline filtergetNodes→graph.getNodes()hasNode→graph.hasNode(id)getNodesByPrefix→graph.getNodes()+filter(id => id.startsWith(prefix + ':'))
The CHANGELOG documents the migration paths. Confirm the README or API docs are updated for consumers, or consider adding a MIGRATION.md for this release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.js` around lines 6 - 8, The public API was reduced by removing
exports saveGraph, queryEdges, getNodes, hasNode, and getNodesByPrefix; update
consumer docs by adding a concise MIGRATION.md (and update README/API docs) that
lists each removed symbol and the exact migration snippet: replace
queryEdges(...) with graph.getEdges(... /* apply inline filter */), replace
getNodes() with graph.getNodes(), replace hasNode(id) with graph.hasNode(id),
replace getNodesByPrefix(prefix) with graph.getNodes().filter(id =>
id.startsWith(prefix + ':')), and note saveGraph is removed (dead code) with
recommended alternative or confirmation it's unnecessary; reference the
CHANGELOG entry and include minimal example code for each mapping.
- Removing public API exports is a breaking change per semver - Sync package-lock.json version with package.json - Update edges.js module header (removed "querying")
- Rename "Removed" → "Breaking" heading in CHANGELOG for 5.0.0 - Add [5.0.0] link definition at bottom of CHANGELOG - Extract findEdges() test helper in review.test.js to reduce duplication
Addressing Review Feedback✅ Semver violation (package.json) — CodeRabbit Critical + Codex P1Already fixed in 496e636 — version bumped to ✅ CHANGELOG link definition + "Breaking" heading — CodeRabbit MinorFixed in 398ea7d:
✅ Test helper extraction (test/review.test.js) — CodeRabbit TrivialFixed in 398ea7d — extracted ✅ Migration docs (src/index.js) — CodeRabbit TrivialThe CHANGELOG |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/edges.js (1)
65-69:⚠️ Potential issue | 🟠 Major
removeEdgelacks input validation — this is a correctness bug, not just an API asymmetry.
createEdgevalidatessource,target,type, andconfidenceby callingvalidateEdge()before touching the graph.removeEdgeaccepts raw parameters with zero validation. This means:
- Typos in edge types (e.g.,
"implments"instead of"implements") silently succeed or fail unpredictably.- Malformed node IDs bypass the
prefix:identifiergrammar check entirely.- Callers get no error feedback if they attempt to remove a non-existent edge.
Fix: reuse the existing
validateEdge()function to matchcreateEdge's defensive posture.🛡️ Corrected fix
/** * Remove an edge from the graph. * * `@param` {import('@git-stunts/git-warp').default} graph * `@param` {string} source * `@param` {string} target * `@param` {string} type * `@returns` {Promise<void>} */ export async function removeEdge(graph, source, target, type) { + const result = validateEdge(source, target, type); + if (!result.valid) { + throw new Error(result.errors.join('; ')); + } const patch = await graph.createPatch(); patch.removeEdge(source, target, type); await patch.commit(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/edges.js` around lines 65 - 69, removeEdge currently mutates the graph without validating inputs; update removeEdge to call the existing validateEdge(...) (the same function used by createEdge) on source, target and type before creating a patch so malformed node IDs or typos are rejected early; if validateEdge requires a confidence param, pass undefined or an appropriate default; only proceed to graph.createPatch() and patch.removeEdge(...) after validation succeeds so invalid input throws instead of silently producing unpredictable behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 10: The changelog references inline link labels like [5.0.0], [4.0.1],
[4.0.0], [3.3.0], and [3.2.0] but lacks the corresponding reference link
definitions, so add Markdown link definitions for each missing version (e.g.,
`[5.0.0]: <URL>` etc.) near the existing link-definition section—insert them
just before the existing `[3.1.0]` link definition to keep ordering consistent;
ensure each definition points to the correct release URL or commit/compare URL
and uses the exact label names shown in the file.
---
Outside diff comments:
In `@src/edges.js`:
- Around line 65-69: removeEdge currently mutates the graph without validating
inputs; update removeEdge to call the existing validateEdge(...) (the same
function used by createEdge) on source, target and type before creating a patch
so malformed node IDs or typos are rejected early; if validateEdge requires a
confidence param, pass undefined or an appropriate default; only proceed to
graph.createPatch() and patch.removeEdge(...) after validation succeeds so
invalid input throws instead of silently producing unpredictable behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CHANGELOG.mdpackage.jsonsrc/edges.js
|
@coderabbitai review please 🥕 |
|
✅ Semver bump to 5.0.0 — proper breaking-change signal I'll conduct a comprehensive review of the refactoring now. Initiating GOD-TIER PEDANTIC MODE 🥕 🧠 Learnings used✅ Actions performedReview triggered.
|
Round 2 feedback✅ Missing CHANGELOG link definitions — Fixed in 89d6e36. Added
|
|
@coderabbitai review please 🥕 |
|
Round 2 feedback disposition: Scanning the full refactoring surface across ~45 call sites:
Executing comprehensive review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
|
@coderabbitai review please 🥕 |
|
Prior rounds recap: Executing comprehensive incremental review of all changes since last scan:
Deploying MAXIMUM SCRUTINY now. 🧠 Learnings used✅ Actions performedReview triggered.
|
1 participant
Summary
getNodes,hasNode,saveGraph,queryEdges,getNodesByPrefixloadGraph()calls withinitGraph()(they were identical —WarpGraph.openis idempotent)loadGraphas deprecated alias for public API backward compatProblem Statement
git-mind wraps
@git-stunts/git-warpin thin abstraction layers that obscure the real API and duplicate capabilities the graph engine already provides. Functions likegetNodes(graph)that literally returngraph.getNodes()add indirection with zero value.ADR Compliance (Required)
Relevant ADR(s)
Compliance Declaration
Architecture Laws Checklist (Hard Gates)
Canonical Truth & Context
--at,--observer,--trust) or deterministically defaulted.Determinism & Provenance
Artifact Hygiene
Contracts & Compatibility
Extension/Effects Safety (if applicable)
Scope Control
Backward Compatibility
getNodes,hasNode,saveGraph,queryEdges,getNodesByPrefix). All have direct WARP API replacements documented in CHANGELOG.Test Plan (Required)
Unit
npx vitest run # 579 tests pass, 29 filesIntegration
npx vitest run # includes integration testsDeterminism
Contract/Schema
Policy Gates
# Pre-commit hooks run automaticallySecurity / Trust Impact
Performance Impact
Observability / Debuggability
Operational Notes
Linked Issues / Milestones