refactor(ports): narrow persistence JSDoc to focused ports (B145)#60
Merged
flyingrobots merged 8 commits intomainfrom Mar 2, 2026
Merged
refactor(ports): narrow persistence JSDoc to focused ports (B145)#60flyingrobots merged 8 commits intomainfrom
flyingrobots merged 8 commits intomainfrom
Conversation
…nfigPort from composite (B145) Domain services now declare only the focused port intersections they actually use (CommitPort & BlobPort, etc.) instead of the 23-method GraphPersistencePort composite. ConfigPort removed from the composite (23 → 21 methods); concrete adapters still define configGet/configSet on their prototypes. Zero behavioral change.
- Writer.js: widen field type to CommitPort & BlobPort & TreePort & RefPort (passes persistence to PatchBuilderV2 which needs all four) - patch.methods.js: cast through unknown for ConfigPort access (FullPersistence no longer includes ConfigPort) - CLI files (shared.js, info.js, materialize.js): cast Persistence to FullPersistence via unknown (CLI typedef is intentionally narrow) - Test mocks (serve, syncAuth, syncWith): cast partial mocks via any (test mocks only implement methods under test)
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGraphPersistencePort is narrowed: ConfigPort removed from its exported composite and replaced across codebase JSDoc/type annotations with focused ports (CommitPort, BlobPort, TreePort, RefPort / FullPersistence / CorePersistence). config access in patch.methods is delegated via a ConfigPort-cast object; CLI/tests include lightweight type casts. No runtime behavior changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
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.
🧹 Nitpick comments (1)
src/domain/warp/patch.methods.js (1)
272-275: Add an explicit ConfigPort capability guard before calling config methods.At lines 274, 275, and 339, the code casts
_persistencetoConfigPortdespite the declaredFullPersistencetype not includingConfigPort. While all concrete adapters (GitGraphAdapter, InMemoryGraphAdapter) implementconfigGet/configSet, missing methods would fail as a genericTypeError. A guard function with a clear domain error would improve debuggability:💡 Suggested hardening
+function requireConfigPort(persistence) { + if (typeof persistence?.configGet !== 'function' || typeof persistence?.configSet !== 'function') { + throw new Error('ConfigPort methods (configGet/configSet) are required for this operation'); + } + return /** `@type` {import('../../ports/ConfigPort.js').default} */ (/** `@type` {unknown} */ (persistence)); +} + /** `@type` {import('../../ports/ConfigPort.js').default} */ -const config = /** `@type` {import('../../ports/ConfigPort.js').default} */ (/** `@type` {unknown} */ (this._persistence)); +const config = requireConfigPort(this._persistence); ... -await /** `@type` {import('../../ports/ConfigPort.js').default} */ (/** `@type` {unknown} */ (this._persistence)).configSet(configKey, freshWriterId); +await requireConfigPort(this._persistence).configSet(configKey, freshWriterId);Also applies to: 339–339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/warp/patch.methods.js` around lines 272 - 275, The code blindly casts this._persistence to ConfigPort and calls configGet/configSet; add an explicit capability guard that verifies this._persistence implements the ConfigPort methods (e.g., has callable configGet and configSet) before calling them and throw a clear domain error if not present. Replace the direct casts used by the local wrappers named configGet and configSet with a small guard helper that checks this._persistence (or FullPersistence) for those methods and returns a typed ConfigPort or throws; apply the same guard where the config calls occur again (the other occurrence around the symbol at line ~339) so missing adapter implementations produce a descriptive error instead of a TypeError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/domain/warp/patch.methods.js`:
- Around line 272-275: The code blindly casts this._persistence to ConfigPort
and calls configGet/configSet; add an explicit capability guard that verifies
this._persistence implements the ConfigPort methods (e.g., has callable
configGet and configSet) before calling them and throw a clear domain error if
not present. Replace the direct casts used by the local wrappers named configGet
and configSet with a small guard helper that checks this._persistence (or
FullPersistence) for those methods and returns a typed ConfigPort or throws;
apply the same guard where the config calls occur again (the other occurrence
around the symbol at line ~339) so missing adapter implementations produce a
descriptive error instead of a TypeError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.mdbin/cli/commands/info.jsbin/cli/commands/materialize.jsbin/cli/shared.jssrc/domain/WarpGraph.jssrc/domain/services/CheckpointService.jssrc/domain/services/HealthCheckService.jssrc/domain/services/PatchBuilderV2.jssrc/domain/services/SyncProtocol.jssrc/domain/services/WormholeService.jssrc/domain/types/WarpPersistence.jssrc/domain/warp/PatchSession.jssrc/domain/warp/Writer.jssrc/domain/warp/patch.methods.jssrc/ports/GraphPersistencePort.jstest/unit/domain/WarpGraph.serve.test.jstest/unit/domain/WarpGraph.syncAuth.test.jstest/unit/domain/WarpGraph.syncWith.test.jstest/unit/ports/GraphPersistencePort.test.js
- Delete BACKLOG.md (retired — all intake goes to ROADMAP.md) - Restructure ROADMAP.md (archive completed work, keep actionable) - Add docs/ROADMAP/COMPLETED.md for archived milestones - Add design RFCs: persistence-port-narrowing, joinreducer-split, warpgraph-decomposition - Add docs/HEX_AUDIT.convo.txt
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
docs/design/persistence-port-narrowing.md (2)
138-140: Update this section to reflect the current WarpGraph typing reality.This says WarpGraph “continues to accept the full composite,” but in this PR stream WarpGraph typing is already narrowed to focused ports/
FullPersistence. Keeping this wording will confuse follow-up work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/persistence-port-narrowing.md` around lines 138 - 140, Update the text to reflect that WarpGraph is now typed to the narrowed persistence interface (FullPersistence / focused ports) rather than accepting the full composite; change the statement referencing "continues to accept the full composite" to state that WarpGraph's own typing has been narrowed (so references to this._persistence on WarpGraph already use the focused ports/FullPersistence), and mention that the narrowing applies both to services it constructs and to the WarpGraph type itself (referencing WarpGraph and FullPersistence/this._persistence so readers can locate the relevant types).
221-223: Tighten the grep check to the exact forbidden call shape.The command at Line 221 searches any
configGet/configSetusage, not specifically_persistence.configGet/Set. That can create false failures. Use a targeted pattern for_persistence\.config(Get|Set)\(.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/persistence-port-narrowing.md` around lines 221 - 223, Update the first grep check to only match the exact forbidden call shape `_persistence.configGet(` or `_persistence.configSet(`: replace the broad `grep -r 'configGet\|configSet' src/` with a targeted regex search like `_persistence\.config(Get|Set)\(` (e.g., using grep -E) so only uses of `_persistence.configGet/Set` are flagged; keep the second grep unchanged (`GraphPersistencePort`) as-is.docs/design/warpgraph-decomposition.md (2)
439-439: Avoid hardcoded stale test counts in verification gates.
4217+ testsis already stale relative to current PR validation numbers. Prefer wording like “full suite must pass” without embedding volatile counts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/warpgraph-decomposition.md` at line 439, Replace the hardcoded, stale test count in the docs line that reads "`npm run test:local` — full unit + integration suite (4217+ tests)" with a non-volatile phrase such as "`npm run test:local` — full unit + integration suite (must pass all tests)" or similar; update the string in the docs entry so it no longer embeds a specific test count and instead describes that the full suite must pass.
190-190: Align host typedef persistence type with the narrowed port model.Both typedefs still point to
GraphPersistencePort. Given the current direction, this should reference the focused composition (WarpPersistence/intersection) to avoid reintroducing the broad composite in new design docs.Also applies to: 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/warpgraph-decomposition.md` at line 190, Update the JSDoc typedefs that currently reference GraphPersistencePort to use the narrowed persistence type (e.g., WarpPersistence or the intended intersection type) so the host property annotation for _persistence and any other typedefs point to the focused composition; find occurrences of the import or type name GraphPersistencePort and replace the typedef reference with WarpPersistence (or the explicit intersection type) in the host typedefs and property annotations (notably the _persistence property and the other typedef noted in the file).docs/design/joinreducer-split.md (1)
293-293: Performance statement is too absolute for an RFC.“V8 inlines across module boundaries” is stronger than what we can guarantee in all runtime conditions. Recommend phrasing as an expectation backed by the benchmark gate already listed.
Suggested doc fix
-| Performance regression from module boundary overhead | Negligible | V8 inlines across module boundaries. The functions are called O(ops) times, not in tight inner loops. | +| Performance regression from module boundary overhead | Negligible | Expected to be minimal in practice; confirm with `npm run benchmark`. Functions are called O(ops) times, not in tight inner loops. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/joinreducer-split.md` at line 293, The performance claim "V8 inlines across module boundaries" in the table row "Performance regression from module boundary overhead" is too absolute for an RFC; change the wording to a softer, expectation-based statement (e.g., "Expected to be negligible, subject to benchmark verification") and explicitly reference the existing benchmark gate as the evidence/requirement for the expectation so readers know this is contingent on measured results rather than a hard guarantee.
🤖 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/design/joinreducer-split.md`:
- Line 281: Replace the hard-coded "4217+ tests" wording with a non-numeric
phrase (e.g., "the full test suite" or "all tests") to avoid drift as tests
change; update both occurrences that reference the test run (the phrase on Line
281 and the related phrase on Line 300) so the instructions read something like
"Run `npm run test:local` — the full test suite must pass" and similarly in the
verification sentence.
- Line 291: The risk table arrow is reversed: change the entry that reads
"DiffCalculator → ReceiptBuilder" to "ReceiptBuilder → DiffCalculator" (or
equivalent wording) to reflect that ReceiptBuilder imports from DiffCalculator
via the buildDotToElement symbol; update any related text to match this
direction to avoid confusion.
In `@docs/design/warpgraph-decomposition.md`:
- Around line 12-15: The RFC claims WarpGraph has 37 instance fields but the
inventory lists 42 private fields plus the traverse method; update the document
so the header/table and all mentions are consistent: either change the header
count from 37 to 42 (or remove traverse from the field count if intended) and
adjust the inventory text to match, and ensure every other occurrence that
references the field count (the WarpGraph constructor/field list and the table
describing instance fields and the traverse entry) is updated to the same
corrected number.
---
Nitpick comments:
In `@docs/design/joinreducer-split.md`:
- Line 293: The performance claim "V8 inlines across module boundaries" in the
table row "Performance regression from module boundary overhead" is too absolute
for an RFC; change the wording to a softer, expectation-based statement (e.g.,
"Expected to be negligible, subject to benchmark verification") and explicitly
reference the existing benchmark gate as the evidence/requirement for the
expectation so readers know this is contingent on measured results rather than a
hard guarantee.
In `@docs/design/persistence-port-narrowing.md`:
- Around line 138-140: Update the text to reflect that WarpGraph is now typed to
the narrowed persistence interface (FullPersistence / focused ports) rather than
accepting the full composite; change the statement referencing "continues to
accept the full composite" to state that WarpGraph's own typing has been
narrowed (so references to this._persistence on WarpGraph already use the
focused ports/FullPersistence), and mention that the narrowing applies both to
services it constructs and to the WarpGraph type itself (referencing WarpGraph
and FullPersistence/this._persistence so readers can locate the relevant types).
- Around line 221-223: Update the first grep check to only match the exact
forbidden call shape `_persistence.configGet(` or `_persistence.configSet(`:
replace the broad `grep -r 'configGet\|configSet' src/` with a targeted regex
search like `_persistence\.config(Get|Set)\(` (e.g., using grep -E) so only uses
of `_persistence.configGet/Set` are flagged; keep the second grep unchanged
(`GraphPersistencePort`) as-is.
In `@docs/design/warpgraph-decomposition.md`:
- Line 439: Replace the hardcoded, stale test count in the docs line that reads
"`npm run test:local` — full unit + integration suite (4217+ tests)" with a
non-volatile phrase such as "`npm run test:local` — full unit + integration
suite (must pass all tests)" or similar; update the string in the docs entry so
it no longer embeds a specific test count and instead describes that the full
suite must pass.
- Line 190: Update the JSDoc typedefs that currently reference
GraphPersistencePort to use the narrowed persistence type (e.g., WarpPersistence
or the intended intersection type) so the host property annotation for
_persistence and any other typedefs point to the focused composition; find
occurrences of the import or type name GraphPersistencePort and replace the
typedef reference with WarpPersistence (or the explicit intersection type) in
the host typedefs and property annotations (notably the _persistence property
and the other typedef noted in the file).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
BACKLOG.mdROADMAP.mddocs/HEX_AUDIT.convo.txtdocs/ROADMAP/COMPLETED.mddocs/design/joinreducer-split.mddocs/design/persistence-port-narrowing.mddocs/design/warpgraph-decomposition.md
💤 Files with no reviewable changes (1)
- BACKLOG.md
✅ Files skipped from review due to trivial changes (1)
- docs/HEX_AUDIT.convo.txt
Release Preflight
If you tag this commit as |
- Delete BACKLOG.md (retired — all intake goes to ROADMAP.md) - Restructure ROADMAP.md (archive completed work, keep actionable) - Add docs/ROADMAP/COMPLETED.md for archived milestones - Add design RFCs: persistence-port-narrowing, joinreducer-split, warpgraph-decomposition - Add docs/HEX_AUDIT.convo.txt
- Delete dead WarpPersistence typedef (now identical to CorePersistence) - Replace FullPersistence import from WarpGraph.js with CorePersistence from WarpPersistence.js types module (removes implementation coupling) - Reorder FullPersistence to canonical CommitPort & BlobPort & TreePort & RefPort - Add inline comment on Writer._persistence explaining wider type - Update CorePersistence description (no longer contrastive with config variant) - Break long materializeOneGraph open() call into multi-line format
… into refactor/persistence
- Replace hard-coded test count "4217+" with "full suite" (joinreducer-split.md) - Fix reversed dependency arrow: ReceiptBuilder → DiffCalculator (joinreducer-split.md) - Correct field count from 37 to 42 matching actual inventory (warpgraph-decomposition.md)
Release Preflight
If you tag this commit as |
Member
Author
|
@coderabbitai review please 🥕 |
|
This looks like a well-structured architectural refactoring that narrows the persistence layer dependencies according to the Interface Segregation Principle. The changes move from a monolithic 23-method GraphPersistencePort to focused port intersections tailored to each domain service's actual needs, and removes ConfigPort from the composite (reducing it to 21 methods). I can see you've included:
Let me perform a full review of all the changes. ✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/design/warpgraph-decomposition.md (1)
335-337:⚠️ Potential issue | 🟡 MinorField/slot baseline count is internally inconsistent.
Earlier, the RFC states 43 total instance slots (Line 68), but this section uses 38 as the pre-extraction baseline. Please reconcile the baseline numbers in one convention throughout the document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/warpgraph-decomposition.md` around lines 335 - 337, The document uses inconsistent baseline slot counts for WarpGraph (43 earlier vs 38 here); pick one canonical baseline (e.g., the 43 total instance slots used at Line 68) and update this paragraph and any related calculations to match it: change "38 slots → 29 slots (24 own + 5 delegated refs)" to the reconciled numbers derived from the chosen baseline, update the delegated/own breakdown and the effective cognitive-load reduction percentage, and scan other mentions of "38" or "43" to ensure all WarpGraph slot counts and percentage reductions (the sentence with "37% reduction") are consistent with the chosen baseline.
🧹 Nitpick comments (1)
docs/design/warpgraph-decomposition.md (1)
190-190: Align host typedef examples with narrowed persistence ports.These typedefs still reference
GraphPersistencePort, which conflicts with the B145 direction (focused port intersections /FullPersistence). Please update these doc snippets so implementers don’t reintroduce the old composite type.Also applies to: 284-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/warpgraph-decomposition.md` at line 190, The JSDoc typedefs currently reference GraphPersistencePort but should use the narrowed persistence type used by the new design (FullPersistence or the specific intersection type used by the B145 direction); update the typedef import and the `@property` declaration that mentions GraphPersistencePort to reference FullPersistence (or the explicit intersection of the required persistence ports) and do the same for the second occurrence around the later typedef so implementers don't reintroduce the old composite type.
🤖 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/design/joinreducer-split.md`:
- Line 206: The fenced code block containing the reduceV5() example should
include a language tag to satisfy markdownlint MD040; update the opening
triple-backtick before the reduceV5() snippet to include a language (e.g., add
"text" so it reads ```text) and leave the closing triple-backtick unchanged so
the block is properly fenced and lint-compliant.
---
Duplicate comments:
In `@docs/design/warpgraph-decomposition.md`:
- Around line 335-337: The document uses inconsistent baseline slot counts for
WarpGraph (43 earlier vs 38 here); pick one canonical baseline (e.g., the 43
total instance slots used at Line 68) and update this paragraph and any related
calculations to match it: change "38 slots → 29 slots (24 own + 5 delegated
refs)" to the reconciled numbers derived from the chosen baseline, update the
delegated/own breakdown and the effective cognitive-load reduction percentage,
and scan other mentions of "38" or "43" to ensure all WarpGraph slot counts and
percentage reductions (the sentence with "37% reduction") are consistent with
the chosen baseline.
---
Nitpick comments:
In `@docs/design/warpgraph-decomposition.md`:
- Line 190: The JSDoc typedefs currently reference GraphPersistencePort but
should use the narrowed persistence type used by the new design (FullPersistence
or the specific intersection type used by the B145 direction); update the
typedef import and the `@property` declaration that mentions GraphPersistencePort
to reference FullPersistence (or the explicit intersection of the required
persistence ports) and do the same for the second occurrence around the later
typedef so implementers don't reintroduce the old composite type.
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
CommitPort & BlobPort, etc.) in JSDoc instead of the 23-method compositeGraphPersistencePortConfigPortfrom theGraphPersistencePortcomposite (23 → 21 methods); concrete adapters still defineconfigGet/configSeton their prototypesDetails
Phase 1 — JSDoc narrowing (8 domain files):
CheckpointService→CommitPort & BlobPort & TreePortHealthCheckService→CommitPortPatchBuilderV2→CommitPort & BlobPort & TreePort & RefPortSyncProtocol→CommitPort & BlobPortWormholeService→CommitPort & BlobPortPatchSession→RefPortWriter→CommitPort & BlobPort & TreePort & RefPortWarpGraph→FullPersistencetypedef (no more GPP import)WarpPersistence→ explicit port intersection (no more GPP alias)Phase 2 — ConfigPort removal from composite:
GraphPersistencePort.jsdrops ConfigPort import + array entrypatch.methods.jsgets explicit ConfigPort casts atconfigGet/configSetsitesTS fixups:
Persistence→FullPersistenceviaunknown(CLI typedef intentionally narrow)any(partial mocks)Test plan
npm run lint— ESLint cleantsc --noEmit— zero errorstsc --noEmit -p test/type-check/tsconfig.json— consumer types cleannpm run test:local— 249 files, 4562 tests passWarpGraph.noCoordination.test.jsgreengrep -rn GraphPersistencePort src/domain/— zero hits in services, warp, types, WarpGraph.jsRFC:
docs/design/persistence-port-narrowing.mdSummary by CodeRabbit
Refactor
Documentation
Tests