fix: immediate standalone items + backlog reconciliation#56
Merged
flyingrobots merged 5 commits intomainfrom Feb 28, 2026
Merged
fix: immediate standalone items + backlog reconciliation#56flyingrobots merged 5 commits intomainfrom
flyingrobots merged 5 commits intomainfrom
Conversation
- B46: ESLint `no-restricted-syntax` bans Date.now() in src/domain/. Legitimate wall-clock uses annotated with eslint-disable. fork.methods.js and MaterializedViewService.js switched to Math.random(). - B47: orsetAdd() validates dot shape before encodeDot(). - B26: ED25519_SPKI_PREFIX constant with RFC 8410 reference in TrustCrypto. - B71: PatchBuilderV2.removeNode() warn mode routes through this._logger instead of console.warn. Writer now forwards logger to PatchBuilderV2. - B126: ESLint no-empty rule with allowEmptyCatch: false. Also includes BACKLOG→ROADMAP reconciliation (B119–B129 promoted) and M13 design problem statement (MIGRATION_PROBLEM.md).
- patch.methods.js: coerce null logger to undefined for Writer constructor - deleteGuardEnforce tests: add missing `child` method to mock loggers
|
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 (5)
📝 WalkthroughWalkthroughThe PR consolidates backlog items into ROADMAP, introduces ESLint rules enforcing domain purity (banning Date usage), refactors logging to use LoggerPort throughout the Writer/PatchBuilder flow, updates fork name generation from timestamp-based to random-based, adds a comprehensive design document for edge property schema migration, and includes validation enhancements and boundary safeguards. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Release Preflight
If you tag this commit as |
Ban new Date() in domain code, fix Writer logger fallback, fix delete-guard test console.warn spy, pad fork name suffix, and relocate MIGRATION_PROBLEM.md to docs/design/.
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
eslint.config.js (1)
273-289: Close theDate()loophole in domain-purity rules.
Date.now()andnew Date()are blocked, butDate()(function-call form) still allows wall-clock access in domain code.🔧 Suggested rule addition
"no-restricted-syntax": ["error", { "selector": "CallExpression[callee.object.name='Date'][callee.property.name='now']", "message": "Date.now() is banned in domain code. Use ClockPort / ClockAdapter instead.", }, + { + "selector": "CallExpression[callee.name='Date']", + "message": "Date() is banned in domain code. Use ClockPort / ClockAdapter instead.", + }, { "selector": "NewExpression[callee.name='Date']", "message": "new Date() is banned in domain code. Use ClockPort / ClockAdapter instead.", }, ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.js` around lines 273 - 289, Add a third restriction to the existing "no-restricted-syntax" array to ban the function-call form Date() in domain code: add an object with selector "CallExpression[callee.name='Date']" and the same message "Date() is banned in domain code. Use ClockPort / ClockAdapter instead." so the rule block that defines "no-restricted-syntax" (around the files: ["src/domain/**/*.js"] entry) covers Date.now(), new Date(), and Date().test/unit/domain/WarpGraph.deleteGuardEnforce.test.js (1)
298-299: Consider using themockLoggerhelper for consistency.These inline logger objects duplicate the same structure as
mockLogger. Using the helper would reduce duplication and ensure consistent mock structure across all tests.♻️ Suggested refactor
it('warn mode works through writer().commitPatch()', async () => { repo = await createGitRepo('delguard'); const warnSpy = vi.fn(); - const logger = { info: vi.fn(), warn: warnSpy, error: vi.fn(), debug: vi.fn(), child: () => logger }; + const logger = mockLogger(warnSpy); const graph = await WarpGraph.open({Note: You'd need to either move
mockLoggerto a shared scope or extract it to a test utility.Also applies to: 333-334
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/domain/WarpGraph.deleteGuardEnforce.test.js` around lines 298 - 299, The test creates an inline logger (warnSpy and logger) duplicating the existing mockLogger helper; replace the inline logger instances with the shared mockLogger to reduce duplication and keep mocks consistent—locate uses of warnSpy and logger in WarpGraph.deleteGuardEnforce.test.js (and the similar occurrences around the other lines mentioned) and swap them to use the mockLogger helper (or import/extract mockLogger into a shared test-utility if not in scope), adjusting any expectations to reference mockLogger.warn or mockLogger.child as needed.
🤖 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/MIGRATION_PROBLEM.md`:
- Around line 197-205: The fenced code block in the MIGRATION_PROBLEM.md snippet
lacks a language tag (MD040); update the opening fence for that block to include
a language identifier (for example "text") so the block begins with ```text and
keep the closing fence unchanged, ensuring the fence uses three backticks and no
extra indentation; this will satisfy the linter and preserve the exact code
block contents in the Op normalization layer section.
In `@ROADMAP.md`:
- Around line 516-520: The inventory table in ROADMAP.md is inconsistent: the
"Standalone" row lists items including B26, B46, B47, B71, and B126 while the
"Standalone (done)" row still shows only three done items; update the table so
counts and ID lists match the DONE markers — increment the "Standalone (done)"
count and move the appropriate IDs (B26, B46, B47, B71, B126 as applicable) from
the "Standalone" list into the "Standalone (done)" list (or remove them if
already listed elsewhere), and then adjust the "Standalone" and "Total tracked"
counts to remain internally consistent with those ID lists.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 273-289: Add a third restriction to the existing
"no-restricted-syntax" array to ban the function-call form Date() in domain
code: add an object with selector "CallExpression[callee.name='Date']" and the
same message "Date() is banned in domain code. Use ClockPort / ClockAdapter
instead." so the rule block that defines "no-restricted-syntax" (around the
files: ["src/domain/**/*.js"] entry) covers Date.now(), new Date(), and Date().
In `@test/unit/domain/WarpGraph.deleteGuardEnforce.test.js`:
- Around line 298-299: The test creates an inline logger (warnSpy and logger)
duplicating the existing mockLogger helper; replace the inline logger instances
with the shared mockLogger to reduce duplication and keep mocks
consistent—locate uses of warnSpy and logger in
WarpGraph.deleteGuardEnforce.test.js (and the similar occurrences around the
other lines mentioned) and swap them to use the mockLogger helper (or
import/extract mockLogger into a shared test-utility if not in scope), adjusting
any expectations to reference mockLogger.warn or mockLogger.child as needed.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
BACKLOG.mdCHANGELOG.mdROADMAP.mddocs/design/MIGRATION_PROBLEM.mdeslint.config.jssrc/domain/crdt/ORSet.jssrc/domain/services/AuditReceiptService.jssrc/domain/services/AuditVerifierService.jssrc/domain/services/BoundaryTransitionRecord.jssrc/domain/services/MaterializedViewService.jssrc/domain/services/PatchBuilderV2.jssrc/domain/services/SyncAuthService.jssrc/domain/trust/TrustCrypto.jssrc/domain/utils/defaultClock.jssrc/domain/warp/Writer.jssrc/domain/warp/fork.methods.jssrc/domain/warp/patch.methods.jstest/unit/domain/WarpGraph.deleteGuardEnforce.test.jstest/unit/domain/WarpGraph.fork.test.js
- Ban Date() function-call form in domain ESLint rule - Add text language tag to MIGRATION_PROBLEM.md fenced block - Hoist mockLogger helper to outer describe scope - Reconcile ROADMAP inventory table with DONE markers
Member
Author
|
All 4 review findings addressed in 7118b71:
All 4430 tests pass, lint clean, IRONCLAD gates green. @coderabbitai review please 🥕 |
|
The commit 7118b71 looks comprehensive:
With all tests passing and gates green, I'll review the changes now. ✅ Actions performedReview triggered.
|
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
ED25519_SPKI_PREFIXconstant with RFC 8410 reference in TrustCrypto.jsno-restricted-syntaxbansDate.now()insrc/domain/**/*.js. Legitimate wall-clock uses annotated with eslint-disable. Fork name and verify seed switched toMath.random().orsetAdd()validates dot shape{writerId: string, counter: integer}beforeencodeDot().PatchBuilderV2.removeNode()warn mode routes throughthis._logger.warn()instead ofconsole.warn.Writernow accepts and forwardsloggertoPatchBuilderV2.no-emptywithallowEmptyCatch: false.MIGRATION_PROBLEM.mddocumenting the edge property schema migration problem in detail.Test plan
noCoordination.test.jspassesconsole.warnSummary by CodeRabbit
Bug Fixes
Documentation
Tests