V18 Continuum slices 6-10#94
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughAdds Continuum evidence posture and claim types, receipt-family projection and source-facts, replaces non-atomic writer-ref updates with compare-and-swap plus immediate visibility checks in commitPatch, updates test fixtures/mocks to support CAS, and adds related unit and smoke tests and design/BEARING docs. ChangesV18 Evidence Posture, Receipt Projection, CAS Visibility, and Test Updates
Sequence Diagram(s)sequenceDiagram
participant CommitPatch
participant Persistence
participant Reader
CommitPatch->>Persistence: create commit object (write tree/blob)
CommitPatch->>Persistence: compareAndSwapRef(writerRef, newCommitSha, expectedParentSha)
alt CAS success
Persistence->>Reader: readRef(writerRef) to verify visibility
alt ref == newCommitSha
Reader-->>CommitPatch: visible -> success path (onCommitSuccess)
else ref != newCommitSha
Reader-->>CommitPatch: PersistenceError.E_REF_IO (invisibility)
end
else CAS failure
Persistence->>Persistence: re-read current ref
Persistence-->>CommitPatch: WriterError (expectedSha / actualSha) -> CAS conflict
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/unit/domain/services/PatchBuilder.test.ts (1)
1-1360: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftThis test file should be split to satisfy the LOC limit.
At 1360 LOC, it substantially exceeds the repository test-file maximum and is now beyond the anti-sludge threshold.
As per coding guidelines,
Maximum file size: 500 LOC (source), 800 LOC (test), 300 LOC (scripts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/services/PatchBuilder.test.ts` around lines 1 - 1360, The test file is too large; split the monolithic PatchBuilder.test.ts into multiple smaller test files grouped by responsibility to respect the LOC limits. Create separate test files (e.g. PatchBuilder.NodeEdgeProps.test.ts for "building patch with node/edge/property" and "reads/writes provenance" groups, PatchBuilder.Versioning.test.ts for "multiple operations increment the VersionVector" and "patch context includes version vector", PatchBuilder.Commit.test.ts for all "commit()" tests including use-after-commit and blob/commit-message assertions, and PatchBuilder.StateErrors.test.ts for "empty state / removeNode/removeEdge" cases), and move the helper factories createMockState, createMockPersistence, createPatchJournal plus imports (PatchBuilder, VersionVector, ORSet, Dot, encodeEdgeKey, decodePatchMessage, decode, CborPatchJournalAdapter, CborCodec, PatchError) into a shared test-helpers file that each split test file imports; ensure each new file keeps the original describe/it blocks intact and only changes imports to reference the shared helpers so test semantics and referenced symbols (e.g., PatchBuilder, builder.addNode/removeNode/addEdge/removeEdge/setProperty/commit, and helper functions) are preserved.test/unit/domain/services/PatchBuilder.content.test.ts (1)
1-820: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this test file; it exceeds the repository LOC ceiling.
This file is now 820 LOC, above the test-file limit, and should be split into focused suites.
As per coding guidelines,
Maximum file size: 500 LOC (source), 800 LOC (test), 300 LOC (scripts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/services/PatchBuilder.content.test.ts` around lines 1 - 820, The test file is too large and should be split into smaller focused suites: extract the attachContent-related tests (describe('attachContent()' and the "with streaming input" and "with blobStorage" groups) into a new test file, extract edge-specific tests (describe('attachEdgeContent()', describe('clearEdgeContent()')) into another, and extract commit/commit-with-content tests (the "commit() with content blobs" describe) into a third; factor out shared helpers (createMockBlobStorage, createMockPersistence, createMockState, createPatchJournal) into a common test-utils module and update the new test files to import them and PatchBuilder, VersionVector, ORSet, Dot, encodeEdgeKey, CborPatchJournalAdapter, CborCodec as needed, ensuring each new file only contains the relevant describe blocks and aligned imports so no duplicated helpers remain.test/unit/domain/warp/Writer.test.ts (1)
1-823: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftFile length is over the test-file cap and should be trimmed/split.
This file is 823 LOC and exceeds the configured 800 LOC maximum for tests.
As per coding guidelines,
Maximum file size: 500 LOC (source), 800 LOC (test), 300 LOC (scripts).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/warp/Writer.test.ts` around lines 1 - 823, The test file exceeds the 800 LOC cap; split it into smaller files and extract shared helpers: move the entire "Writer (WARP schema:2)" describe block (all tests that reference Writer, beginPatch, commit, commitPatch, WriterError) into a new test file (e.g., Writer.test.ts) and move the "PatchSession operations" describe block into a separate test file (e.g., PatchSession.test.ts); extract the helper functions createMockPersistence, createPatchJournal, createPatchMessage and any shared imports (VersionVector, ORSet, Dot, encodeEdgeKey, encodePatchMessage, CborPatchJournalAdapter, CborCodec) into a test helper module and import them from both new test files; update imports in the new files to reference the helper module and ensure each test file now stays below the 800 LOC limit.
🧹 Nitpick comments (3)
test/unit/domain/services/PatchBuilder.test.ts (1)
40-42: ⚡ Quick winMake the CAS mock enforce expected-old SHA.
Line 40 currently accepts all CAS writes; it should fail when current ref differs from
expectedOidto keep commit-path tests meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/services/PatchBuilder.test.ts` around lines 40 - 42, The CAS mock currently accepts any swap; update the mock for persistence.compareAndSwapRef to read the current ref (via persistence.readRef) and enforce that the provided expected SHA (the first argument, e.g., expectedOid) matches the current ref before applying the newOid: if they match, set persistence.readRef to resolve to newOid and return success; if they differ, simulate a failed CAS by throwing or returning a failure value so tests exercising commit-path behavior fail when the expected-old SHA is wrong.test/unit/domain/warp/Writer.test.ts (1)
35-37: ⚡ Quick winAlign the mock with real CAS semantics.
Line 35 updates the ref regardless of expected-old value. Add expected-value checking so race/conflict behavior is exercised rather than assumed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/warp/Writer.test.ts` around lines 35 - 37, The compareAndSwapRef mock currently always updates the ref; update the mockImplementation for persistence.compareAndSwapRef to accept the expectedOld and newOid parameters and only set persistence.readRef to newOid (or resolve true) when expectedOld matches the current stored value (simulate current value either held in a local variable or read from persistence.readRef mock), otherwise simulate a CAS failure (resolve false or throw error consistent with real CAS behavior); reference persistence.compareAndSwapRef and persistence.readRef so test logic exercises race/conflict behavior.test/unit/domain/services/PatchBuilder.content.test.ts (1)
41-43: ⚡ Quick winModel CAS mismatches in the mock, not only success paths.
Line 41 currently ignores the expected-old ref, so CAS contract regressions can still pass these tests. Make the mock validate
expectedOidagainst current ref and reject on mismatch.Suggested change
- persistence.compareAndSwapRef.mockImplementation(async (_ref, newOid) => { - persistence.readRef.mockResolvedValue(newOid); + persistence.compareAndSwapRef.mockImplementation(async (ref, newOid, expectedOid) => { + const current = await persistence.readRef(ref); + if (current !== expectedOid) { + throw new Error(`CAS mismatch for ${ref}`); + } + persistence.readRef.mockResolvedValue(newOid); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/domain/services/PatchBuilder.content.test.ts` around lines 41 - 43, Update the test mock for persistence.compareAndSwapRef so it enforces the CAS contract instead of always succeeding: in the mockImplementation for compareAndSwapRef, first obtain the currentOid (e.g., by calling persistence.readRef or using a captured current value), compare it to the passed expectedOid, and only if they match resolve with the newOid and also set persistence.readRef to return newOid; if they do not match reject (or throw) to simulate a CAS failure. Ensure the mock checks the expectedOid parameter rather than ignoring it so tests fail on CAS mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domain/continuum/ContinuumEvidenceClaim.ts`:
- Around line 17-20: The ContinuumEvidenceClaim constructor dereferences fields
(fields.descriptor) before validating fields exists; update the constructor in
ContinuumEvidenceClaim to first guard that the incoming fields parameter is
non-null/undefined and throw a WarpError('E_VALIDATION') on invalid input, then
call requireDescriptor(fields.descriptor), normalizePosture(fields.posture) and
optionalNonEmptyString(fields.nativeWitnessProof, 'nativeWitnessProof') as
before so all downstream helpers receive validated input.
In `@src/domain/continuum/ContinuumReceiptFamilyProjection.ts`:
- Around line 65-68: The constructor of ContinuumReceiptFamilyProjection must
validate the incoming envelope object before accessing nested properties; add a
guard at the start of the constructor to check that the parameter fields is not
null/undefined (and optionally has expected shape) and throw a domain WarpError
(or use your existing validation helper) instead of letting a raw TypeError
surface; only call
requireEvidence(fields.evidence).requireTranslatedGitWarpEvidence(),
requireReceiptFamilyDescriptor(this.evidence.descriptor) and
requireSourceFacts(fields.sourceFacts) after the envelope has been validated so
the constructor enforces its invariants and fails with a domain error.
In `@src/domain/continuum/GitWarpReceiptSourceFacts.ts`:
- Around line 18-20: The constructor of GitWarpReceiptSourceFacts dereferences
fields.tickReceipt before validating fields, so update the
GitWarpReceiptSourceFacts constructor to first guard that the input object
fields is non-null/undefined and throw the appropriate WarpError('E_VALIDATION')
(or call the existing validation helper) if it's invalid; only after that, call
requireTickReceipt(this.tickReceipt = requireTickReceipt(fields.tickReceipt))
and then requireReceiptOutcomes(this.tickReceipt) to preserve the existing
invariants.
In `@test/helpers/mockPorts.ts`:
- Around line 55-57: The mock compareAndSwapRef (compareAndSwapRef) currently
unconditionally writes newOid; change it to validate the expectedOid against the
current value in the refs Map before updating: fetch current = refs.get(ref) (or
treat missing as null), compare with the provided _expectedOid, and only set
refs.set(ref, newOid) when they match; if they don't match, simulate CAS failure
by either throwing a descriptive error or returning a failure value consistent
with how real CAS is consumed in tests (ensure callers of compareAndSwapRef in
tests still get the same success/failure semantics).
In `@test/helpers/WarpGraphMockPersistence.ts`:
- Around line 59-62: The mock compareAndSwapRef currently overwrites refs
unconditionally; change its implementation (the vi.fn for compareAndSwapRef) to
read the current value from this.#refs, compare it against the provided
_expectedOid (treat missing key as null), and only set this.#refs.set(ref,
newOid) and update readRef's mockImplementation when they match; otherwise leave
refs unchanged and return a failure indicator (e.g., false). Ensure the function
returns a boolean or promise<boolean> indicating success so tests can detect CAS
conflicts, and keep references to compareAndSwapRef and readRef when making this
change.
In `@test/smoke/warpTtdReceiptFamilyProjectionSmoke.ts`:
- Line 9: The import of GitWarpAdapter in warpTtdReceiptFamilyProjectionSmoke.ts
cannot resolve the source path; update the import to reference the warp-ttd
package export (e.g., import { GitWarpAdapter } from 'warp-ttd' or from the
package's published entrypoint) or add the underlying src path to the active
tsconfig "paths"/"include" so TypeScript can find GitWarpAdapter; locate the
import statement in test/smoke/warpTtdReceiptFamilyProjectionSmoke.ts and
replace the '../../../../warp-ttd/src/adapters/gitWarpAdapter.ts' path with the
correct package entrypoint export (or adjust tsconfig) so the GitWarpAdapter
symbol resolves during typecheck.
In `@test/unit/domain/continuum/ContinuumEvidencePosture.test.ts`:
- Around line 26-95: Add two negative-path unit tests to
ContinuumEvidencePosture.test.ts that exercise ContinuumEvidenceClaim
validation: (1) construct a claim with a non-native posture (e.g.,
'translated-git-warp-evidence' or 'unproven-continuum-shape') while providing a
non-empty nativeWitnessProof and assert that the constructor/validation throws
WarpError; (2) construct a claim with posture 'native-continuum-evidence' but
pass an empty nativeWitnessProof (empty string or whitespace) and assert that it
throws WarpError. Use makeGeneratedReceiptDescriptor() to build the descriptor
and reference ContinuumEvidenceClaim, ContinuumEvidencePosture and WarpError in
the expectations so the tests cover those branches.
In `@test/unit/domain/services/PatchBuilder.cas.test.ts`:
- Around line 25-27: The current test mock for persistence.compareAndSwapRef in
PatchBuilder.cas.test.ts always "succeeds" and blindly sets persistence.readRef,
so CAS semantics aren't tested; change the mockImplementation for
persistence.compareAndSwapRef to accept (expectedOldOid, newOid), compare
expectedOldOid to the current value returned by persistence.readRef (or a local
variable capturing the simulated ref), only update the simulated ref (and
persistence.readRef mock) when they match, and otherwise return/throw a failure
(e.g., false or throw) so tests can assert true CAS success and CAS failure
paths correctly.
In `@test/unit/domain/WarpGraph.test.ts`:
- Around line 60-62: The shared CAS mock currently always succeeds by setting
persistence.readRef to newOid; change
persistence.compareAndSwapRef.mockImplementation so it enforces the
expected-head match: have the mock read the current head (via
persistence.readRef or an internal variable), compare it to the provided
expectedOid argument, and only update persistence.readRef to newOid and return
success when they match; if they don’t match return a failure (or throw)
consistent with the real compareAndSwapRef behavior. Ensure you reference the
same mock signature (compareAndSwapRef, expectedOid, newOid, and readRef) when
updating the implementation.
---
Outside diff comments:
In `@test/unit/domain/services/PatchBuilder.content.test.ts`:
- Around line 1-820: The test file is too large and should be split into smaller
focused suites: extract the attachContent-related tests
(describe('attachContent()' and the "with streaming input" and "with
blobStorage" groups) into a new test file, extract edge-specific tests
(describe('attachEdgeContent()', describe('clearEdgeContent()')) into another,
and extract commit/commit-with-content tests (the "commit() with content blobs"
describe) into a third; factor out shared helpers (createMockBlobStorage,
createMockPersistence, createMockState, createPatchJournal) into a common
test-utils module and update the new test files to import them and PatchBuilder,
VersionVector, ORSet, Dot, encodeEdgeKey, CborPatchJournalAdapter, CborCodec as
needed, ensuring each new file only contains the relevant describe blocks and
aligned imports so no duplicated helpers remain.
In `@test/unit/domain/services/PatchBuilder.test.ts`:
- Around line 1-1360: The test file is too large; split the monolithic
PatchBuilder.test.ts into multiple smaller test files grouped by responsibility
to respect the LOC limits. Create separate test files (e.g.
PatchBuilder.NodeEdgeProps.test.ts for "building patch with node/edge/property"
and "reads/writes provenance" groups, PatchBuilder.Versioning.test.ts for
"multiple operations increment the VersionVector" and "patch context includes
version vector", PatchBuilder.Commit.test.ts for all "commit()" tests including
use-after-commit and blob/commit-message assertions, and
PatchBuilder.StateErrors.test.ts for "empty state / removeNode/removeEdge"
cases), and move the helper factories createMockState, createMockPersistence,
createPatchJournal plus imports (PatchBuilder, VersionVector, ORSet, Dot,
encodeEdgeKey, decodePatchMessage, decode, CborPatchJournalAdapter, CborCodec,
PatchError) into a shared test-helpers file that each split test file imports;
ensure each new file keeps the original describe/it blocks intact and only
changes imports to reference the shared helpers so test semantics and referenced
symbols (e.g., PatchBuilder,
builder.addNode/removeNode/addEdge/removeEdge/setProperty/commit, and helper
functions) are preserved.
In `@test/unit/domain/warp/Writer.test.ts`:
- Around line 1-823: The test file exceeds the 800 LOC cap; split it into
smaller files and extract shared helpers: move the entire "Writer (WARP
schema:2)" describe block (all tests that reference Writer, beginPatch, commit,
commitPatch, WriterError) into a new test file (e.g., Writer.test.ts) and move
the "PatchSession operations" describe block into a separate test file (e.g.,
PatchSession.test.ts); extract the helper functions createMockPersistence,
createPatchJournal, createPatchMessage and any shared imports (VersionVector,
ORSet, Dot, encodeEdgeKey, encodePatchMessage, CborPatchJournalAdapter,
CborCodec) into a test helper module and import them from both new test files;
update imports in the new files to reference the helper module and ensure each
test file now stays below the 800 LOC limit.
---
Nitpick comments:
In `@test/unit/domain/services/PatchBuilder.content.test.ts`:
- Around line 41-43: Update the test mock for persistence.compareAndSwapRef so
it enforces the CAS contract instead of always succeeding: in the
mockImplementation for compareAndSwapRef, first obtain the currentOid (e.g., by
calling persistence.readRef or using a captured current value), compare it to
the passed expectedOid, and only if they match resolve with the newOid and also
set persistence.readRef to return newOid; if they do not match reject (or throw)
to simulate a CAS failure. Ensure the mock checks the expectedOid parameter
rather than ignoring it so tests fail on CAS mismatches.
In `@test/unit/domain/services/PatchBuilder.test.ts`:
- Around line 40-42: The CAS mock currently accepts any swap; update the mock
for persistence.compareAndSwapRef to read the current ref (via
persistence.readRef) and enforce that the provided expected SHA (the first
argument, e.g., expectedOid) matches the current ref before applying the newOid:
if they match, set persistence.readRef to resolve to newOid and return success;
if they differ, simulate a failed CAS by throwing or returning a failure value
so tests exercising commit-path behavior fail when the expected-old SHA is
wrong.
In `@test/unit/domain/warp/Writer.test.ts`:
- Around line 35-37: The compareAndSwapRef mock currently always updates the
ref; update the mockImplementation for persistence.compareAndSwapRef to accept
the expectedOld and newOid parameters and only set persistence.readRef to newOid
(or resolve true) when expectedOld matches the current stored value (simulate
current value either held in a local variable or read from persistence.readRef
mock), otherwise simulate a CAS failure (resolve false or throw error consistent
with real CAS behavior); reference persistence.compareAndSwapRef and
persistence.readRef so test logic exercises race/conflict behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e34bd16-dada-449e-8ceb-fca24951d9d5
📒 Files selected for processing (35)
docs/BEARING.mddocs/design/0154-v18-evidence-posture/v18-evidence-posture.mddocs/design/0155-v18-patch-commit-visibility-contract/v18-patch-commit-visibility-contract.mddocs/design/0156-v18-same-writer-concurrent-race-witness/v18-same-writer-concurrent-race-witness.mddocs/design/0157-v18-receipt-family-projection/v18-receipt-family-projection.mddocs/design/0158-v18-warp-ttd-receipt-smoke/v18-warp-ttd-receipt-smoke.mdindex.tssrc/domain/continuum/ContinuumEvidenceClaim.tssrc/domain/continuum/ContinuumEvidencePosture.tssrc/domain/continuum/ContinuumReceiptFamilyProjection.tssrc/domain/continuum/GitWarpReceiptSourceFacts.tssrc/domain/errors/WriterError.tssrc/domain/services/PatchCommitter.tstest/benchmark/detachedReadBenchmark.fixture.tstest/helpers/WarpGraphMockPersistence.tstest/helpers/mockPorts.tstest/smoke/warpTtdReceiptFamilyProjectionSmoke.tstest/unit/domain/WarpCore.snapshotHashStability.test.tstest/unit/domain/WarpGraph.conflicts.test.tstest/unit/domain/WarpGraph.invalidation.test.tstest/unit/domain/WarpGraph.observerBoundary.test.tstest/unit/domain/WarpGraph.receipts.test.tstest/unit/domain/WarpGraph.strands.test.tstest/unit/domain/WarpGraph.test.tstest/unit/domain/WarpGraph.worldline.test.tstest/unit/domain/WarpGraph.writerInvalidation.test.tstest/unit/domain/continuum/ContinuumEvidencePosture.test.tstest/unit/domain/continuum/ContinuumReceiptFamilyProjection.test.tstest/unit/domain/index.exports.test.tstest/unit/domain/services/PatchBuilder.cas.test.tstest/unit/domain/services/PatchBuilder.content.test.tstest/unit/domain/services/PatchBuilder.test.tstest/unit/domain/services/PatchCommitter.visibility.test.tstest/unit/domain/warp/Writer.sameWriterRace.test.tstest/unit/domain/warp/Writer.test.ts
Member
Author
|
@codex self-audit findings before fixes:
|
Member
Author
Activity Summary
Verification completed after the fixes:
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 4
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/PatchCommitter.ts (1)
181-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle ambiguous CAS errors when ref already equals
newCommitSha.If
compareAndSwapRefwrites successfully but throws afterward, the current logic turns that into a false CAS conflict. TreatingactualSha === newCommitShaas success avoids incorrect conflict signaling.Suggested fix
async function compareAndSwapWriterRef( persistence: PersistencePorts, writerRef: string, newCommitSha: string, expectedSha: string | null, ): Promise<void> { try { await persistence.compareAndSwapRef(writerRef, newCommitSha, expectedSha); } catch (err) { const actualSha = await persistence.readRef(writerRef); + if (actualSha === newCommitSha) { + return; + } if (actualSha !== expectedSha) { throw buildWriterCasConflict(expectedSha, actualSha); } throw err; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/domain/services/PatchCommitter.ts` around lines 181 - 189, The catch block for compareAndSwapRef currently treats any error as a CAS conflict; instead, after calling const actualSha = await persistence.readRef(writerRef), first check if actualSha === newCommitSha and if so treat the operation as successful (return/exit without throwing), otherwise if actualSha !== expectedSha throw buildWriterCasConflict(expectedSha, actualSha), and only rethrow the original err in the remaining case; reference compareAndSwapRef, readRef, writerRef, newCommitSha, expectedSha, and buildWriterCasConflict to locate and apply this logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/unit/domain/services/PatchBuilder.commit.test.ts`:
- Around line 15-16: Tests in PatchBuilder.commit.test.ts use many occurrences
of `any`/`as any`; replace them with explicit test types: declare
interfaces/types for the mock state and ORSet shapes (e.g. MockState,
NodeAliveORSet, EdgeAliveORSet) and update variables like the harness,
mockState, and helpers (the values currently cast with `as any`) to use those
types instead of `any`; ensure functions/methods referenced in tests (e.g.
PatchBuilder.commit, any makeNode/makeEdge helpers, createHarness/getMockState)
return typed values or accept typed params and import or define those types in
the test file so all `as any` casts are removed.
In `@test/unit/domain/services/PatchBuilder.contentPersistence.test.ts`:
- Around line 13-14: This test file uses blanket any and as any in its mocks and
fixtures; replace those with precise test types by (1) creating minimal typed
test doubles or using Partial<T> / jest.Mocked<T> for mocked services (e.g., the
ContentPersistence mock and any PatchBuilder inputs), (2) avoid as any casts by
typing helper factories to return the exact shape the test needs (create a
TestContentPersistence interface or use jest.Mocked<ContentPersistence>), and
(3) when unknown values are needed first assert to unknown then narrow/cast to
the real type rather than as any; update references to the mock used in tests
(the ContentPersistence mock, PatchBuilder inputs and any helper functions) to
use these concrete types everywhere you previously used any or as any.
In `@test/unit/domain/services/PatchBuilder.provenance.test.ts`:
- Around line 13-16: The tests use `any` and `as any` in the mock V5 state and
PatchBuilder fixtures—replace the JSDoc `@returns {any}` on the mock function
(e.g., createMockV5State) with the concrete type (import the correct V5
state/interface from the domain types), type the builder inputs instead of
casting (the variables passed into the PatchBuilder tests / `builder`), and give
the decoded values proper types (replace `decodedPatch as any` and `decodedOps
as any` with their real types by importing the Patch/Op types and constructing
fixtures that conform to those interfaces); remove all `as any` casts and update
assertions to use the typed values so the tests compile against the repo TS
typings.
In `@test/unit/domain/WarpGraph.test.ts`:
- Around line 117-126: The test file WarpGraph.test.ts is too large and must be
split into focused spec files: extract related groups of tests (e.g.,
persistence/compareAndSwapRef behavior, graph-construction specs, writer/ref
tests) into separate files (for example WarpGraph.persistence.test.ts,
WarpGraph.construction.test.ts) and move shared helpers/fixtures like
createMockPersistence and any common mock setup into a shared test helper module
(e.g., test/utils or test/__fixtures__) that each new spec imports; update the
affected tests (including the "test fixture compareAndSwapRef rejects
expected-head mismatches" case) to import createMockPersistence from the new
helper and ensure any beforeEach/afterEach setup is preserved in the new files
so the tests run unchanged.
---
Outside diff comments:
In `@src/domain/services/PatchCommitter.ts`:
- Around line 181-189: The catch block for compareAndSwapRef currently treats
any error as a CAS conflict; instead, after calling const actualSha = await
persistence.readRef(writerRef), first check if actualSha === newCommitSha and if
so treat the operation as successful (return/exit without throwing), otherwise
if actualSha !== expectedSha throw buildWriterCasConflict(expectedSha,
actualSha), and only rethrow the original err in the remaining case; reference
compareAndSwapRef, readRef, writerRef, newCommitSha, expectedSha, and
buildWriterCasConflict to locate and apply this logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d3ead7f-fb5b-4804-b3d0-cded7c457208
📒 Files selected for processing (25)
CHANGELOG.mddocs/BEARING.mddocs/design/0158-v18-warp-ttd-receipt-smoke/v18-warp-ttd-receipt-smoke.mdsrc/domain/continuum/ContinuumEvidenceClaim.tssrc/domain/continuum/ContinuumEvidencePosture.tssrc/domain/continuum/ContinuumReceiptFamilyProjection.tssrc/domain/continuum/GitWarpReceiptSourceFacts.tssrc/domain/services/PatchCommitter.tstest/helpers/WarpGraphMockPersistence.tstest/helpers/mockPorts.tstest/smoke/warpTtdReceiptFamilyProjectionSmoke.tstest/unit/domain/WarpGraph.patchCount.test.tstest/unit/domain/WarpGraph.test.tstest/unit/domain/continuum/ContinuumEvidencePosture.test.tstest/unit/domain/continuum/ContinuumReceiptFamilyProjection.test.tstest/unit/domain/services/PatchBuilder.cas.test.tstest/unit/domain/services/PatchBuilder.commit.test.tstest/unit/domain/services/PatchBuilder.content.test.tstest/unit/domain/services/PatchBuilder.contentPersistence.test.tstest/unit/domain/services/PatchBuilder.provenance.test.tstest/unit/domain/services/PatchBuilder.test.tstest/unit/domain/warp/PatchSession.operations.test.tstest/unit/domain/warp/Writer.test.tstest/unit/helpers/WarpGraphMockPersistence.test.tstest/unit/helpers/mockPorts.test.ts
✅ Files skipped from review due to trivial changes (4)
- CHANGELOG.md
- test/unit/domain/warp/PatchSession.operations.test.ts
- docs/design/0158-v18-warp-ttd-receipt-smoke/v18-warp-ttd-receipt-smoke.md
- docs/BEARING.md
Member
Author
|
@codex self-audit findings before the next fixes:
These are queued with the unresolved CodeRabbit threads and will be resolved with separate commits and verification. |
Member
Author
Activity Summary
Verification after these fixes:
|
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
Verification
ADR checks
Summary by CodeRabbit
New Features
Documentation
Tests