release: v12.4.0 — standalone lane batch#58
Merged
flyingrobots merged 23 commits intomainfrom Feb 28, 2026
Merged
Conversation
…84+B92) - Refactor parseExportBlock to accept block body (not full source) - Add collectExportBlocks internal helper for full-source scanning - Add export class matching to extractJsExports - Add --quiet flag to suppress per-export stderr output - Add 34 unit tests for parseExportBlock, extractJsExports, extractDtsExports
…84+B92) - Refactor parseExportBlock to accept block body (not full source) - Add collectExportBlocks internal helper for full-source scanning - Add export class matching to extractJsExports - Add 34 unit tests for parseExportBlock, extractJsExports, extractDtsExports - Add 85 missing type/interface/class exports to manifest (0 warnings)
…l fix) GitAdapterError was a Git-specific class living in the domain layer, violating hexagonal architecture. The error codes (E_MISSING_OBJECT, E_REF_NOT_FOUND, E_REF_IO) are persistence-layer concepts, not Git-specific. Renamed to PersistenceError so the domain stays adapter-agnostic.
- L1: wrap listRefs in try/catch with wrapGitError for consistency - L2: gate isRefNotFoundError/isRefIoError on exit codes (128, 1) - L3: document E_MISSING_OBJECT usage for malformed commit format - I1: reorder CHANGELOG sections to Added → Changed → Refactored → Fixed - I2: remove redundant code param from PersistenceError super call - I3: upgrade NodeHttpAdapter.dispatch handler type from bare Function
|
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 (1)
📝 WalkthroughWalkthroughVersion bumped to 12.4.0 with typed persistence errors (E_MISSING_OBJECT, E_REF_NOT_FOUND, E_REF_IO), reserved graph name segment validation, optional limit parameters for listRefs(), refined HTTP adapter types, 85+ new public API exports, and simplified constructor parameter defaults across services. Comprehensive test coverage added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Move B50-B52, B55, B77, B78, B82, B84, B91-B94, B120-B122 to done. Standalone: 52 → 37, done: 8 → 23. Priority sequence updated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/services/DagTopology.js (1)
40-46:⚠️ Potential issue | 🟠 Major
traversalis optional in construction but required at runtime incommonAncestors().
commonAncestors()dereferencesthis._traversalunconditionally (Line 79+), so constructingDagTopologywithouttraversalcan crash with aTypeError. Please make it required or fail fast with a clear error.💡 Proposed fix
- * `@param` {import('./DagTraversal.js').default} [options.traversal] - Traversal service for ancestor enumeration + * `@param` {import('./DagTraversal.js').default} options.traversal - Traversal service for ancestor enumeration */ constructor(/** `@type` {{ indexReader: import('./BitmapIndexReader.js').default, logger?: import('../../ports/LoggerPort.js').default, traversal?: import('./DagTraversal.js').default }} */ { indexReader, logger = nullLogger, traversal }) { if (!indexReader) { throw new Error('DagTopology requires an indexReader'); } + if (!traversal) { + throw new Error('DagTopology requires a traversal service'); + } this._indexReader = indexReader; this._logger = logger; this._traversal = traversal; }Also applies to: 79-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/DagTopology.js` around lines 40 - 46, The constructor allows omission of traversal but commonAncestors() dereferences this._traversal, causing a runtime TypeError; update the constructor for DagTopology to require traversal and throw a clear Error if traversal is missing (i.e., validate the traversal parameter in the constructor and set this._traversal only after checking), or alternatively add an explicit guard at the start of commonAncestors() that throws a descriptive error if this._traversal is null/undefined; reference the constructor, this._traversal, and commonAncestors() when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 14: Update the release notes sentence to list all currently reserved
graph-name segments used by validateGraphName(): include `writers`,
`checkpoints`, `coverage`, `cursor`, `audit`, `trust`, and `seek-cache` so the
README matches the validation logic and prevents incomplete documentation of
reserved keywords.
In `@src/infrastructure/adapters/GitGraphAdapter.js`:
- Around line 816-821: The truthy check on `limit` lets falsy-but-invalid values
(e.g., 0, NaN) bypass validation; replace the condition so presence of the
`limit` option is detected explicitly (e.g., check `options` has a `limit`
property or `limit !== undefined && limit !== null`) then call
`this._validateLimit(limit)` and push `--count=${limit}` only when present; also
update `_validateLimit` to explicitly reject non-finite/NaN values (use
Number.isFinite or Number.isInteger as appropriate) so NaN or other invalid
numbers cannot be accepted.
In `@src/infrastructure/adapters/InMemoryGraphAdapter.js`:
- Around line 409-413: The current truthy guard skips validateLimit for provided
but falsy values; change the check to detect whether callers supplied a limit
(not whether it's truthy). Replace the `const limit = options?.limit; if (limit)
{ ... }` with a presence check such as `if (options &&
Object.prototype.hasOwnProperty.call(options, 'limit')) { const limit =
options.limit; validateLimit(limit); return sorted.slice(0, limit); }` so
validateLimit is invoked for NaN/''/null/etc.; reference symbols: validateLimit,
limit, sorted.slice.
In `@src/ports/RefPort.js`:
- Around line 45-47: The JSDoc nested param annotation for `_options.limit` is
failing lint/type-firewall; change the doc to declare the parent object
explicitly (e.g., add "@param {Object} _options - Optional parameters" and then
"@param {number} _options.limit - Maximum number of refs to return. When omitted
or 0, all matching refs are returned.") so the checker recognizes the object
shape; update the JSDoc block that documents `_options`/`_options.limit` in
RefPort.js accordingly.
In `@test/unit/domain/services/CheckpointService.edgeCases.test.js`:
- Around line 488-491: The test directly dereferences
state.edgeBirthEvent.get(edgeKey).lamport which can be undefined; change to
first store the result (e.g., const evt = state.edgeBirthEvent.get(edgeKey)),
assert it's defined (expect(evt).toBeDefined() or
expect(evt).not.toBeUndefined()), then access evt.lamport (or use a non-null
assertion if your linter allows). This uses symbols encodeEdgeKeyV5 and
state.edgeBirthEvent.get(edgeKey) and ensures safe access to lamport.
---
Outside diff comments:
In `@src/domain/services/DagTopology.js`:
- Around line 40-46: The constructor allows omission of traversal but
commonAncestors() dereferences this._traversal, causing a runtime TypeError;
update the constructor for DagTopology to require traversal and throw a clear
Error if traversal is missing (i.e., validate the traversal parameter in the
constructor and set this._traversal only after checking), or alternatively add
an explicit guard at the start of commonAncestors() that throws a descriptive
error if this._traversal is null/undefined; reference the constructor,
this._traversal, and commonAncestors() when making the change.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.gitignoreCHANGELOG.mdREADME.mdROADMAP.mdcontracts/type-surface.m8.jsonjsr.jsonpackage.jsonscripts/check-dts-surface.jsscripts/hooks/pre-pushsrc/domain/errors/PersistenceError.jssrc/domain/errors/index.jssrc/domain/services/BitmapIndexReader.jssrc/domain/services/DagPathFinding.jssrc/domain/services/DagTopology.jssrc/domain/services/DagTraversal.jssrc/domain/services/SyncAuthService.jssrc/domain/trust/TrustRecordService.jssrc/domain/types/TickReceipt.jssrc/domain/utils/RefLayout.jssrc/infrastructure/adapters/BunHttpAdapter.jssrc/infrastructure/adapters/DenoHttpAdapter.jssrc/infrastructure/adapters/GitGraphAdapter.jssrc/infrastructure/adapters/InMemoryGraphAdapter.jssrc/infrastructure/adapters/NodeHttpAdapter.jssrc/ports/HttpServerPort.jssrc/ports/RefPort.jstest/helpers/topologyHelpers.jstest/unit/domain/services/CheckpointService.edgeCases.test.jstest/unit/domain/utils/RefLayout.test.jstest/unit/infrastructure/adapters/GitGraphAdapter.coverage.test.jstest/unit/infrastructure/adapters/GitGraphAdapter.listRefs.test.jstest/unit/infrastructure/adapters/InMemoryGraphAdapter.test.jstest/unit/scripts/check-dts-surface.test.js
| - **Version namespace separation** — patch schema and checkpoint schema constants are now distinct (`PATCH_SCHEMA_V2`/`V3` vs `CHECKPOINT_SCHEMA_STANDARD`/`INDEX_TREE`). | ||
| - **ADR governance** — ADR 3 readiness gates formalize when the persisted wire-format migration may proceed, with GitHub issue template and go/no-go checklist. | ||
| - **`PersistenceError` with typed error codes** — `E_MISSING_OBJECT`, `E_REF_NOT_FOUND`, `E_REF_IO` replace brittle `message.includes()` checks, giving callers machine-readable error handling. | ||
| - **Reserved graph name validation** — `validateGraphName()` rejects ref-layout keywords (`writers`, `checkpoints`, `coverage`) as graph name segments, preventing silent data corruption. |
There was a problem hiding this comment.
Keep the reserved-keyword list complete in release notes.
Line 14 lists only three reserved segments, but current validation also reserves cursor, audit, trust, and seek-cache.
📝 Proposed doc fix
-- **Reserved graph name validation** — `validateGraphName()` rejects ref-layout keywords (`writers`, `checkpoints`, `coverage`) as graph name segments, preventing silent data corruption.
+- **Reserved graph name validation** — `validateGraphName()` rejects ref-layout keywords (`writers`, `checkpoints`, `coverage`, `cursor`, `audit`, `trust`, `seek-cache`) as graph name segments, preventing silent data corruption.📝 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.
Suggested change
| - **Reserved graph name validation** — `validateGraphName()` rejects ref-layout keywords (`writers`, `checkpoints`, `coverage`) as graph name segments, preventing silent data corruption. | |
| - **Reserved graph name validation** — `validateGraphName()` rejects ref-layout keywords (`writers`, `checkpoints`, `coverage`, `cursor`, `audit`, `trust`, `seek-cache`) as graph name segments, preventing silent data corruption. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 14, Update the release notes sentence to list all
currently reserved graph-name segments used by validateGraphName(): include
`writers`, `checkpoints`, `coverage`, `cursor`, `audit`, `trust`, and
`seek-cache` so the README matches the validation logic and prevents incomplete
documentation of reserved keywords.
Comment on lines
+816
to
+821
| const limit = options?.limit; | ||
| const args = ['for-each-ref', '--format=%(refname)']; | ||
| if (limit) { | ||
| this._validateLimit(limit); | ||
| args.push(`--count=${limit}`); | ||
| } |
There was a problem hiding this comment.
Validate non-zero limits explicitly (avoid falsey bypass).
Lines 818-820 use a truthy check, so values like NaN bypass validation and silently behave as “no limit.”
✅ Suggested fix
- if (limit) {
+ if (limit !== undefined && limit !== 0) {
this._validateLimit(limit);
args.push(`--count=${limit}`);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/adapters/GitGraphAdapter.js` around lines 816 - 821, The
truthy check on `limit` lets falsy-but-invalid values (e.g., 0, NaN) bypass
validation; replace the condition so presence of the `limit` option is detected
explicitly (e.g., check `options` has a `limit` property or `limit !== undefined
&& limit !== null`) then call `this._validateLimit(limit)` and push
`--count=${limit}` only when present; also update `_validateLimit` to explicitly
reject non-finite/NaN values (use Number.isFinite or Number.isInteger as
appropriate) so NaN or other invalid numbers cannot be accepted.
Comment on lines
+409
to
+413
| const limit = options?.limit; | ||
| if (limit) { | ||
| validateLimit(limit); | ||
| return sorted.slice(0, limit); | ||
| } |
There was a problem hiding this comment.
Validate limit when provided; current truthy check misses invalid falsy values.
On Line 410, if (limit) skips validateLimit() for values like NaN, '', or null, which then silently behave as “no limit” instead of failing.
✅ Proposed fix
const sorted = result.sort();
const limit = options?.limit;
- if (limit) {
- validateLimit(limit);
- return sorted.slice(0, limit);
- }
- return sorted;
+ if (limit === undefined || limit === 0) {
+ return sorted;
+ }
+ validateLimit(limit);
+ return sorted.slice(0, limit);📝 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.
Suggested change
| const limit = options?.limit; | |
| if (limit) { | |
| validateLimit(limit); | |
| return sorted.slice(0, limit); | |
| } | |
| const limit = options?.limit; | |
| if (limit === undefined || limit === 0) { | |
| return sorted; | |
| } | |
| validateLimit(limit); | |
| return sorted.slice(0, limit); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/infrastructure/adapters/InMemoryGraphAdapter.js` around lines 409 - 413,
The current truthy guard skips validateLimit for provided but falsy values;
change the check to detect whether callers supplied a limit (not whether it's
truthy). Replace the `const limit = options?.limit; if (limit) { ... }` with a
presence check such as `if (options &&
Object.prototype.hasOwnProperty.call(options, 'limit')) { const limit =
options.limit; validateLimit(limit); return sorted.slice(0, limit); }` so
validateLimit is invoked for NaN/''/null/etc.; reference symbols: validateLimit,
limit, sorted.slice.
Comment on lines
+45
to
47
| * @param {{ limit?: number }} [_options] - Optional parameters | ||
| * @param {number} [_options.limit] - Maximum number of refs to return. When omitted or 0, all matching refs are returned. | ||
| * @returns {Promise<string[]>} Array of matching ref names |
There was a problem hiding this comment.
JSDoc for _options.limit is currently failing lint/type-firewall.
The nested param annotation is being rejected by the current rule set. Use an explicit object parent param shape to satisfy the checker.
🛠️ Proposed fix
- * `@param` {{ limit?: number }} [_options] - Optional parameters
+ * `@param` {object} [_options] - Optional parameters
* `@param` {number} [_options.limit] - Maximum number of refs to return. When omitted or 0, all matching refs are returned.📝 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.
Suggested change
| * @param {{ limit?: number }} [_options] - Optional parameters | |
| * @param {number} [_options.limit] - Maximum number of refs to return. When omitted or 0, all matching refs are returned. | |
| * @returns {Promise<string[]>} Array of matching ref names | |
| * `@param` {object} [_options] - Optional parameters | |
| * `@param` {number} [_options.limit] - Maximum number of refs to return. When omitted or 0, all matching refs are returned. | |
| * `@returns` {Promise<string[]>} Array of matching ref names |
🧰 Tools
🪛 GitHub Check: lint
[failure] 46-46:
Qualified name '_options.limit' is not allowed without a leading '@param {object} _options'.
🪛 GitHub Check: preflight
[failure] 46-46:
Qualified name '_options.limit' is not allowed without a leading '@param {object} _options'.
🪛 GitHub Check: type-firewall
[failure] 46-46:
Qualified name '_options.limit' is not allowed without a leading '@param {object} _options'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ports/RefPort.js` around lines 45 - 47, The JSDoc nested param annotation
for `_options.limit` is failing lint/type-firewall; change the doc to declare
the parent object explicitly (e.g., add "@param {Object} _options - Optional
parameters" and then "@param {number} _options.limit - Maximum number of refs to
return. When omitted or 0, all matching refs are returned.") so the checker
recognizes the object shape; update the JSDoc block that documents
`_options`/`_options.limit` in RefPort.js accordingly.
Comment on lines
+488
to
+491
| const edgeKey = encodeEdgeKeyV5('x', 'y', 'rel'); | ||
| expect(state.edgeBirthEvent.has(edgeKey)).toBe(true); | ||
| expect(state.edgeBirthEvent.get(edgeKey).lamport).toBe(0); | ||
| }); |
There was a problem hiding this comment.
Fix strict-null failure on edgeBirthEvent.get(edgeKey) access.
Line 490 directly dereferences .lamport from Map.get(...), which can be undefined, and this is currently breaking lint/type-firewall/preflight.
✅ Proposed fix
const edgeKey = encodeEdgeKeyV5('x', 'y', 'rel');
expect(state.edgeBirthEvent.has(edgeKey)).toBe(true);
- expect(state.edgeBirthEvent.get(edgeKey).lamport).toBe(0);
+ const birthEvent = state.edgeBirthEvent.get(edgeKey);
+ if (!birthEvent) {
+ throw new Error(`Missing edgeBirthEvent for key: ${edgeKey}`);
+ }
+ expect(birthEvent.lamport).toBe(0);🧰 Tools
🪛 GitHub Check: lint
[failure] 490-490:
Object is possibly 'undefined'.
🪛 GitHub Check: preflight
[failure] 490-490:
Object is possibly 'undefined'.
🪛 GitHub Check: type-firewall
[failure] 490-490:
Object is possibly 'undefined'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/domain/services/CheckpointService.edgeCases.test.js` around lines
488 - 491, The test directly dereferences
state.edgeBirthEvent.get(edgeKey).lamport which can be undefined; change to
first store the result (e.g., const evt = state.edgeBirthEvent.get(edgeKey)),
assert it's defined (expect(evt).toBeDefined() or
expect(evt).not.toBeUndefined()), then access evt.lamport (or use a non-null
assertion if your linter allows). This uses symbols encodeEdgeKeyV5 and
state.edgeBirthEvent.get(edgeKey) and ensures safe access to lamport.
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
package.json,jsr.json,CHANGELOG.md,README.md,ROADMAP.mdPersistenceErrortyped error codes, reserved graph name validation,listRefslimit parameter,WARP_QUICK_PUSHenv var, type surface manifest completeness (0 errors, 0 warnings)listRefserror wrapping consistency, CHANGELOG section ordering, JSDoc type upgrades)Test plan
node scripts/check-dts-surface.js— 0 errors, 0 warningsnpx vitest run— all unit tests passnpm run lint— cleanpackage.json==jsr.json== CHANGELOG headingnpm run release:preflightuntil after merge — that's the tag stepSummary by CodeRabbit
v12.4.0 Release Notes
New Features
Bug Fixes
Documentation