Conversation
…angling-ref resilience BREAKING CHANGE: autoMaterialize now defaults to true. Pass autoMaterialize: false to preserve old behavior. Completes M1.T2, M2.T3, B1, B8, B9, B10. Bumps to v10.15.0.
Add JSDoc type annotations for allowedWriters param, narrow union types in SyncAuthService.verifyWriters, and cast materialize() returns in integration tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDefaults autoMaterialize to true; adds writer whitelist enforcement and metrics; introduces Writer commit reentrancy guard; treats certain Git errors as dangling refs returning null; adds CI npm-audit step and SECURITY.md; bumps version to 11.0.0; updates docs, types, and tests to reflect these changes. Changes
Sequence DiagramssequenceDiagram
participant Client
participant HttpSyncServer
participant SyncAuthService
participant Graph
Client->>HttpSyncServer: POST /sync (patches, writerId, signature)
HttpSyncServer->>HttpSyncServer: parse body
HttpSyncServer->>SyncAuthService: verify/auth signature
SyncAuthService-->>HttpSyncServer: { ok: true } / { ok:false }
alt allowedWriters configured
HttpSyncServer->>SyncAuthService: verifyWriters(writerId)
SyncAuthService-->>HttpSyncServer: { ok:true } or { ok:false, FORBIDDEN_WRITER }
alt ok
HttpSyncServer->>Graph: graph.patch / Writer.commitPatch
Graph-->>HttpSyncServer: result
HttpSyncServer-->>Client: 200 OK
else forbidden
HttpSyncServer-->>Client: 403 Forbidden
end
else no whitelist (or log-only)
HttpSyncServer->>Graph: graph.patch / Writer.commitPatch
Graph-->>HttpSyncServer: result
HttpSyncServer-->>Client: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ROADMAP.md (1)
439-449:⚠️ Potential issue | 🟡 MinorB1 not marked DONE despite writer whitelist being implemented in this PR.
The PR objectives list "B1: Add writer whitelist for sync ingress" as a deliverable, and the
allowedWritersfeature is implemented inSyncAuthServiceandHttpSyncServer. However, B1 on line 439 is not marked~~DONE~~like B8–B10. If B1 is only partially fulfilled (e.g., "enforced signed commits" is still pending), consider noting that in the description; otherwise, mark it done for consistency.
🤖 Fix all issues with AI agents
In `@docs/GUIDE.md`:
- Around line 1749-1773: Update the GUIDE text to use the correct release
version and fix the async example calls: change the release mention "As of
v10.9.0" to "As of v11.0.0" (to match the PR bump) and update the opt-out note
to reference the prior behavior version consistently (e.g., "preserve v10.9.0
behavior" or adjust both to v11.0.0/v10.9.0 as appropriate for release notes),
and add missing await keywords to the async getNodes() calls in the examples
(both the pre-change example that calls await graph.materialize(); const nodes =
await graph.getNodes(); and the post-change example const nodes = await
graph.getNodes();), referencing WarpGraph.open, materialize(), and getNodes()
where changes are needed.
In `@SECURITY.md`:
- Around line 131-132: The SECURITY.md risk table currently lists `strip-ansi`
and `open` though they are not direct dependencies; update the table to either
remove these two rows or annotate them as transitive/inlined (e.g., note that
`strip-ansi` was inlined into src/visualization/utils/ansi.js and `open` only
appears transitively in the lockfile), and ensure any mention references the
actual artifact names (`strip-ansi`, `open`) and the inlined location
(`src/visualization/utils/ansi.js`) so auditors see they are not present in
package.json.
In `@src/domain/services/HttpSyncServer.js`:
- Around line 240-243: The constructor currently accepts allowedWriters but
initAuth returns auth:null when auth.keys is missing, so allowedWriters is
silently ignored; update the constructor in HttpSyncServer to validate that if
allowedWriters is provided but auth.keys is not configured (check the auth
parameter or result of initAuth), throw a clear configuration error instead of
silently storing this._allowedWriters, and ensure callers of initAuth/authMode
and checkWriterWhitelist (and the SyncAuthService integration) cannot bypass
enforcement; reference initAuth, this._allowedWriters, auth.keys,
checkWriterWhitelist and SyncAuthService when making the change.
🧹 Nitpick comments (5)
test/unit/domain/services/GitGraphAdapter.test.js (2)
543-543: Misleadingdescribename: tests exercisereadRef, notrefExistsdirectly.The suite is named
refExists() dangling-ref handlingbut all assertions calladapter.readRef(...). Consider renaming toreadRef() dangling-ref handlingto match what's actually being tested.
558-584: Tests only cover therefExistscode path withinreadRef, not therev-parsefallback.Since
mockPlumbing.executeis set to reject with the same error for all calls,refExists(theshow-refcall) catches the dangling error and returnsfalse, soreadRefreturnsnullat line 539–540 without ever reaching therev-parse+isDanglingObjectErrorcatch at line 551.To also cover the TOCTOU race path (ref exists during
show-refbut becomes dangling beforerev-parse), consider a test where the firstexecutecall (show-ref) resolves successfully, and the second (rev-parse) rejects with a dangling error.Example test for the rev-parse dangling path
+ it('readRef returns null when show-ref succeeds but rev-parse hits dangling object', async () => { + // show-ref succeeds (ref exists) + mockPlumbing.execute + .mockResolvedValueOnce('') // show-ref --verify --quiet + .mockRejectedValueOnce((() => { + const err = /** `@type` {any} */ (new Error('fatal: bad object')); + err.details = { code: 128, stderr: 'fatal: bad object abc123' }; + return err; + })()); + + const result = await adapter.readRef('refs/warp/test/writers/alice'); + expect(result).toBeNull(); + });src/domain/warp/Writer.js (1)
179-194: Solid reentrancy guard with properfinallycleanup.The
try/finallyensures the flag resets on both success and error paths. The synchronous flag check before anyawaitcorrectly prevents both nested and concurrentcommitPatchcalls on the same Writer instance.One minor note: the
@throwsJSDoc (line 170) doesn't list the newCOMMIT_IN_PROGRESSerror code.Add COMMIT_IN_PROGRESS to the `@throws` docs
* `@throws` {WriterError} EMPTY_PATCH if no operations were added * `@throws` {WriterError} WRITER_REF_ADVANCED if CAS fails (ref moved since beginPatch) * `@throws` {WriterError} PERSIST_WRITE_FAILED if git operations fail + * `@throws` {WriterError} COMMIT_IN_PROGRESS if called while another commitPatch is in progresssrc/domain/services/SyncAuthService.js (1)
385-395: Forbidden writer rejections are not logged.Unlike the
verify()method which routes failures throughthis._fail()(incrementingauthFailCountand logging viathis._logger.warn),verifyWriters()only increments theforbiddenWriterRejectsmetric without logging. Consider logging the forbidden writer IDs atwarnlevel so operators can identify misconfigured or malicious sync peers.♻️ Suggested logging addition
verifyWriters(writerIds) { if (!this._allowedWriters) { return { ok: true }; } const forbidden = writerIds.filter(id => !/** `@type` {Set<string>} */ (this._allowedWriters).has(id)); if (forbidden.length > 0) { this._metrics.forbiddenWriterRejects += 1; + this._logger.warn('sync auth: forbidden writers rejected', { forbidden }); return fail('FORBIDDEN_WRITER', 403); } return { ok: true }; }src/domain/WarpGraph.js (1)
1076-1081: Consider extracting the duplicated E_NO_STATE message into a constant.The same long error string (with URL) is repeated verbatim in
join(),runGC(),applySyncResponse(), and_ensureFreshState(). A shared constant would reduce drift risk if the URL or wording changes.Example
+const E_NO_STATE_MSG = 'No materialized state. Call materialize() before querying, or use autoMaterialize: true (the default). See https://github.com/git-stunts/git-warp#materialization'; + // Then in each throw site: -throw new QueryError('No materialized state. Call materialize() before querying, or use autoMaterialize: true (the default). See https://github.com/git-stunts/git-warp#materialization', { +throw new QueryError(E_NO_STATE_MSG, { code: 'E_NO_STATE', });Also applies to: 1737-1742, 2221-2226
Global git config was accidentally set to CI Bot <ci@git-cms.local>. Restored to James Ross <james@flyingrobots.dev> to match GPG signing key. Prior commits on this branch are correctly signed but show as unverified on GitHub due to the author mismatch.
Release Preflight
If you tag this commit as |
- README.md: query section no longer says to pass autoMaterialize: true - index.d.ts: document autoMaterialize default as true, add commitPatch() reentrancy @throws, add allowedWriters to SyncAuthServerOptions and serve() types - Writer.js: add missing @throws COMMIT_IN_PROGRESS to commitPatch() - GitGraphAdapter.js: readRef() JSDoc notes dangling-object handling - examples/edge-properties.js: remove redundant autoMaterialize: true
Release Preflight
If you tag this commit as |
Previously, passing allowedWriters without configuring auth.keys would silently ignore the whitelist. Now throws a clear configuration error at construction time.
- GUIDE.md migration section: v10.9.0 → v11.0.0, add missing await on getNodes(), "preserve v10.8 behavior" → "preserve pre-v11 behavior" - SECURITY.md: annotate strip-ansi as inlined and open as transitive
- Extract E_NO_STATE_MSG / E_STALE_STATE_MSG constants in WarpGraph.js - Add warn logging for forbidden writer rejections in SyncAuthService - Rename test describe "refExists()" → "readRef()" to match tested method - Add TOCTOU race path test (show-ref ok, rev-parse dangling) - Remove redundant autoMaterialize: true from README and example - Annotate ROADMAP B1 as partially done (whitelist done, signed commits open)
|
@coderabbitai Re: Writer.js |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1326
Thank you for pointing this out—the documentation was indeed already in place. ✓ |
Release Preflight
If you tag this commit as |
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/domain/services/SyncAuthService.js`:
- Around line 385-396: verifyWriters currently always returns a failure for
forbidden writer IDs; update it to respect the service auth mode like verify()
does by checking this._authMode (e.g. compare against 'log-only') before
returning fail. Specifically, in verifyWriters (and/or checkWriterWhitelist if
that helper exists) keep the metric increment and logger.warn for forbidden
writers, but if this._authMode === 'log-only' return { ok: true } instead of
fail('FORBIDDEN_WRITER', 403); only return the failure when auth mode enforces
blocking. Ensure you reference the verifyWriters function and this._authMode
property so behavior matches verify().
In `@src/domain/WarpGraph.js`:
- Around line 1741-1743: The throw in runGC() duplicates the hardcoded
E_NO_STATE message; replace the string literal with the shared constant
E_NO_STATE_MSG and pass the same error metadata (code: 'E_NO_STATE') so runGC()
throws new QueryError(E_NO_STATE_MSG, { code: 'E_NO_STATE' }) consistent with
other sites (see uses at lines referencing E_NO_STATE_MSG and the QueryError
constructor) — update the throw inside the runGC function to reference the
E_NO_STATE_MSG constant instead of the duplicated literal.
🧹 Nitpick comments (2)
src/domain/services/SyncAuthService.js (1)
176-183: Consider guarding against an emptyallowedWritersarray.An empty array
allowedWriters: []will create an emptySet, silently rejecting all writers. This is likely a misconfiguration rather than intent. A validation here would save operators debugging time.Proposed fix
if (allowedWriters) { + if (allowedWriters.length === 0) { + throw new Error('allowedWriters must be a non-empty array when provided'); + } for (const w of allowedWriters) { validateWriterId(w); }test/unit/domain/services/HttpSyncServer.test.js (1)
308-325: Tests cover the config validation; consider adding a request-level whitelist enforcement test.These two tests correctly verify the constructor guard. However, there's no unit test in this file for the runtime behavior — i.e., sending a sync request with a forbidden writer ID and asserting HTTP 403. The integration tests (B1/B9) may cover this, but a focused unit test here would improve confidence in
checkWriterWhitelist.Also a minor nit: lines 311 and 319 pass
createMockPort()(the wrapper object) ashttpPort, but the constructor expects the.portsub-object (as used in thebeforeEachon line 128). This works only because the constructor throws or completes before touchinghttpPort. UsingcreateMockPort().portwould be more accurate.
- Replace hardcoded E_NO_STATE error string in runGC() with the shared E_NO_STATE_MSG constant (CodeRabbit review 2, comment #2808672021) - Pass authMode to checkWriterWhitelist() so log-only mode correctly allows forbidden writers through instead of blocking them - Extract _validateAllowedWriters() helper to reduce constructor complexity - Validate empty allowedWriters array at SyncAuthService construction - Add test coverage for allowedWriters + log-only mode interaction and empty allowedWriters rejection
Release Preflight
If you tag this commit as |
- Convert standalone checkWriterWhitelist() to HttpSyncServer._checkWriterWhitelist()
for consistency with _checkAuth() — both are instance methods that read
this._auth and this._authMode, no parameter threading needed.
- Add JSDoc on SyncAuthService.verifyWriters() clarifying that mode
enforcement is the caller's responsibility (matching verify() pattern).
- Add mode-agnostic documentation-by-test: explicitly verify that both
verify() and verifyWriters() return { ok: false } regardless of mode,
proving mode enforcement lives in HttpSyncServer, not SyncAuthService.
Release Preflight
If you tag this commit as |
- Combine _checkAuth() and _checkWriterWhitelist() into a single
_authorize(request, parsed) method on HttpSyncServer. Both checks
share the same enforce/log-only pattern against this._auth and
this._authMode — no reason to split them across two methods.
- Simplify _handleRequest: body parsing now precedes auth (body size
DoS guard remains first), reducing the pipeline from 6 steps to 5.
- Add SyncAuthService.enforceWriters() — mode-aware convenience wrapper
around verifyWriters(). Returns { ok: true } in log-only mode after
recording metrics, eliminating the need for callers to check mode.
- Add end-to-end log-only integration test: bad signature + forbidden
writer both pass through, sync succeeds, logger.warn called.
Release Preflight
If you tag this commit as |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
- Replace manual verifyWriters() + mode check in _authorize() with enforceWriters(), which handles log-only passthrough internally. - Strengthen log-only e2e test: assert logger.warn called >= 2 times (once for auth failure, once for forbidden writer) confirming both failures are independently logged.
Release Preflight
If you tag this commit as |
1 participant
Summary
autoMaterializenow defaults totrue— query methods transparently callmaterialize()when no cached state exists. PassautoMaterialize: falseto preserve old behavior.allowedWritersoption onHttpSyncServerrejects unlisted writer IDs with HTTP 403refExists()/readRef()handle dangling-ref exit 128, returningnull/falseinstead of throwinggraph.patch()CAS integration tests with real Git persistenceWriter.commitPatch()reentrancy guard matchinggraph.patch()semanticsnpm auditCI gateE_NO_STATE/E_STALE_STATEmessages with recovery hints and docs URLs; migration guide in GUIDE.mdTest plan
npm run lint— cleannpm run typecheck— cleangit warp infosmoke test — worksSummary by CodeRabbit
Breaking Changes
New Features
Documentation
Chores
Tests
Release