fix(bisect): harden CLI validation and remove dead code (B148)#62
Merged
flyingrobots merged 17 commits intomainfrom Mar 4, 2026
Merged
fix(bisect): harden CLI validation and remove dead code (B148)#62flyingrobots merged 17 commits intomainfrom
flyingrobots merged 17 commits intomainfrom
Conversation
BREAKING CHANGE: getNodeProps() now returns Record<string, unknown>
instead of Map<string, unknown>, aligning with getEdgeProps() which
already returns a plain object.
Migration: replace .get('key') with .key or ['key'],
.has('key') with 'key' in props, .size with Object.keys(props).length.
Updated source (query.methods.js, ObserverView.js, QueryBuilder.js),
type definitions (_wiredMethods.d.ts, index.d.ts, type-surface.m8.json),
15 test files, examples, README, and GUIDE.
Replace duplicate FullPersistence typedef in WarpGraph.js with an import of CorePersistence from types/WarpPersistence.js. Both were identical four-port intersections (Commit & Blob & Tree & Ref).
Promote subscribe() and watch() to @stability stable with @SInCE 13.0.0 annotations. Fix onError callback type from (error: Error) to (error: unknown) in _wiredMethods.d.ts and index.d.ts to match runtime catch semantics. Correct watch() pattern type to string | string[].
Sequential batch helper that applies multiple patch callbacks in order. Each callback sees state from the prior commit, enabling dependent patches. Returns array of commit SHAs. Inherits CAS, eager re-materialize, and reentrancy guard from patch().
Implements causality bisect: given a known-good and known-bad commit SHA on a writer's patch chain, finds the first bad patch via binary search. Materializes state at each midpoint using ceiling-bounded materialization and delegates to a user-supplied test function. Includes 6 test vectors covering the linear chain, same-SHA, single-step, reversed-order, missing-SHA, and SHA-passthrough cases.
BisectService performs binary search over a writer's patch chain to find the first bad patch between a known-good and known-bad commit. O(log N) materializations via ceiling-based seek. CLI: git warp bisect --good <sha> --bad <sha> --test <cmd> --writer <id> Exit codes: 0=found, 2=range error. Test command receives WARP_BISECT_SHA and WARP_BISECT_GRAPH env vars. Includes 6 test vectors from BISECT_V1 spec: linear chain, same SHA, single step, reversed order, unknown SHA, and testFn validation.
Three new edge-case tests: - Multi-handler cross-unsubscribe (A unsubscribes B mid-loop) - Subscribe new handler during callback (should not fire for current diff) - Unsubscribe during onError callback (no infinite loop)
22 tests verifying shape parity between CLI trust output and AuditVerifierService.evaluateTrust() across happy path, error path, and not-configured paths. Validates CLI-specific overrides (status, source, sourceDetail) and structural invariants.
Three tests documenting the intentional null = "no value" sentinel contract: null triggers recomputation, getWithMetadata shows fromCache=false, hasValue returns false.
BREAKING CHANGES: - getNodeProps() returns Record<string, unknown> instead of Map (B100) - Removed PerformanceClockAdapter and GlobalClockAdapter exports (B140) New features: - graph.patchMany() batch patch API (B11) - git warp bisect — causality bisect CLI and BisectService (B2) - Observer API (subscribe/watch) stabilized at @SInCE 13.0.0 (B3) Refactors: - Unified CorePersistence/FullPersistence typedefs (B146) Tests: - Unsubscribe-during-callback E2E tests (B44) - Trust payload parity tests (B124) - CachedValue null-payload semantic tests (B125)
…13.0.0 - Fix WarpGraphInstance.materialize return type (void → WarpStateV5) in CLI types - Use structural BisectGraph typedef instead of WarpGraph import in BisectService - Update index.d.ts BisectService constructor to match structural type - Add BisectService and BisectResult to type-surface.m8.json manifest
- Add --writer validation to bisect CLI handler - Fix exit code constant (USAGE+1 → NOT_FOUND) - Remove unreachable candidates guard and unused BisectTestFn typedef - Add all-bad, testFn-throws, and empty-chain bisect test vectors - Remove unused imports/constants from TrustPayloadParity test - Fix stale getNodeProps return type in CHANGELOG - Archive M11 COMPASS II to COMPLETED.md
|
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. Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (44)
📝 WalkthroughWalkthroughAdds v13.0.0: introduces BisectService and CLI Changes
Sequence DiagramsequenceDiagram
rect rgba(220,240,255,0.5)
actor CLI as Client/CLI
end
rect rgba(200,255,220,0.5)
participant Bisect as BisectService
participant Graph as WarpGraph
participant History as PatchHistory
participant TestFn as Test Function
end
CLI->>Bisect: run({good, bad, writerId, testFn})
Bisect->>Graph: getWriterPatches(writerId)
Graph->>History: fetch writer patches
History-->>Graph: patches[]
Graph-->>Bisect: patches slice (candidates)
Bisect->>Bisect: resolve candidates & validate range
alt single candidate
Bisect-->>CLI: return found result (firstBadPatch, 0 steps)
else binary search
loop until found
Bisect->>Bisect: pick midpoint SHA
Bisect->>Graph: materialize(midpoint SHA)
Graph-->>Bisect: WarpStateV5
Bisect->>TestFn: testFn(state, midpoint SHA)
TestFn-->>Bisect: boolean (good/bad)
Bisect->>Bisect: narrow candidate range
end
Bisect-->>CLI: return found result (firstBadPatch, steps)
end
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/domain/services/QueryBuilder.js (1)
310-316: RenamepropsMaptopropsRecordfor consistency with the new model.The type is already
Record<string, unknown>, but variable naming still saysMap, which makes the migration harder to follow.♻️ Suggested rename
- * `@param` {{ id: string, propsMap: Record<string, unknown>, edgesOut: Array<{label: string, neighborId: string}>, edgesIn: Array<{label: string, neighborId: string}> }} params - Node data + * `@param` {{ id: string, propsRecord: Record<string, unknown>, edgesOut: Array<{label: string, neighborId: string}>, edgesIn: Array<{label: string, neighborId: string}> }} params - Node data @@ -function createNodeSnapshot({ id, propsMap, edgesOut, edgesIn }) { - const props = buildPropsSnapshot(propsMap); +function createNodeSnapshot({ id, propsRecord, edgesOut, edgesIn }) { + const props = buildPropsSnapshot(propsRecord); @@ - const propsMap = await getProps(nodeId); + const propsRecord = await getProps(nodeId); @@ - snapshot: createNodeSnapshot({ id: nodeId, propsMap, edgesOut, edgesIn }), + snapshot: createNodeSnapshot({ id: nodeId, propsRecord, edgesOut, edgesIn }), @@ - const propsMap = await getProps(nodeId); - const props = buildPropsSnapshot(propsMap); + const propsRecord = await getProps(nodeId); + const props = buildPropsSnapshot(propsRecord);Also applies to: 686-692, 750-752
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/QueryBuilder.js` around lines 310 - 316, Rename the parameter and local variable named propsMap to propsRecord for consistency: update the JSDoc param name and the function signature of createNodeSnapshot({ id, propsMap, edgesOut, edgesIn }) to use propsRecord, change the local const props = buildPropsSnapshot(propsMap) to buildPropsSnapshot(propsRecord), and update every internal reference and any callers in this file that pass or destructure propsMap (including the other functions around the other occurrences mentioned) to use propsRecord instead; ensure you also rename any related variable declarations and type annotations so the names are consistent across createNodeSnapshot and the other similar functions referenced.test/runtime/deno/lifecycle.test.ts (1)
39-49: Rename the test case title to remove “via Map.”The assertion now validates object/Record access (
props?.k), so keeping “via Map” in the test name is confusing during triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/runtime/deno/lifecycle.test.ts` around lines 39 - 49, The test title "lifecycle: node properties via Map" is misleading because the assertion checks object/Record access (props?.k); update the Deno.test call to a clearer name such as "lifecycle: node properties" by changing the first argument string in the Deno.test invocation so it no longer mentions "via Map" (locate the Deno.test with the same anonymous async function that creates repo/openGraph/createPatch/addNode/setProperty/materialize/getNodeProps).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/cli/commands/bisect.js`:
- Around line 29-42: The catch in runTestCommand currently swallows all execSync
errors and returns false; change it to only treat a non-zero child exit as a
test failure and rethrow actual command execution errors (like ENOENT/EACCES) so
typos/missing binaries surface; specifically, in runTestCommand's catch block
inspect the caught error (e.g., err.code and err.status): if err.code is
'ENOENT' or 'EACCES' (or err.errno indicates an exec failure) rethrow or throw a
new Error with the original error attached, otherwise (normal non-zero exit
where err.status is a number) return false to mark the test as failing. Ensure
the thrown error preserves the original message for debugging.
In `@docs/ROADMAP/COMPLETED.md`:
- Around line 307-308: The archived ROADMAP entry for the bisect feature still
lists "6 test vectors" but the PR test plan increased the bisect suite to 9
vectors; update the text that mentions BisectService and the `git warp bisect`
CLI to say "9 test vectors" (replace the "6" count), and search the file for any
other occurrences (e.g., the second archived mention) to make the counts
consistent across the document.
In `@ROADMAP.md`:
- Around line 168-170: Update the downstream roadmap text so sequencing matches
the new status of Milestone 11: locate the heading "## Milestone 11 — COMPASS II
✅ COMPLETE (v13.0.0)" and any occurrences of the phrase "M11 follows after M14"
and change them to reflect M11 is already complete (e.g., remove the “follows
after” language or reword to show M11 precedes or is archived after completion);
ensure links referencing M11 (such as the archive to COMPLETED.md) remain
correct and that all mentions of milestone order across the document present a
consistent execution sequence.
In `@src/domain/services/QueryBuilder.js`:
- Around line 267-273: The buildPropsSnapshot function and the _runAggregate
block should use defensive object-key handling: change the empty object
initializations to Object.create(null) (e.g., replace const props = {} with a
null-prototype object) and when iterating source records check ownership via
hasOwnProperty (or Object.prototype.hasOwnProperty.call(...)) before copying
keys; update both buildPropsSnapshot and the _runAggregate logic (referencing
those function names) to follow this pattern to prevent prototype-pollution
paths and match PropertyIndexBuilder/IncrementalIndexUpdater patterns.
In `@src/domain/warp/query.methods.js`:
- Around line 49-50: The function getNodeProps currently returns record || {}
which turns a null (missing node) into an empty object and violates the
documented contract; update the return to preserve null semantics by returning
record directly (or explicitly checking if record === null and returning null)
instead of using the || fallback. Locate the call to
this._propertyReader.getNodeProps in getNodeProps and mirror the null-preserving
behavior used by ObserverView.getNodeProps (lines referencing
ObserverView.getNodeProps) so callers relying on Promise<Record<string,
unknown>|null> receive null for missing nodes.
- Around line 63-69: The property-bag objects are created as plain {} and accept
user-controlled keys (decoded.propKey), allowing prototype pollution via keys
like "__proto__"; update the three initializations to use zero-prototype objects
(Object.create(null)) so they don't inherit Object.prototype: change the props
in getNodeProps (currently "const props = {}" at the start of that loop), the
similar bag in getEdgeProps (the "const props = {}" at that function), and the
property bag used in the neighbor-edges query function (the initialization at
line ~267) to use Object.create(null) instead of {}.
---
Nitpick comments:
In `@src/domain/services/QueryBuilder.js`:
- Around line 310-316: Rename the parameter and local variable named propsMap to
propsRecord for consistency: update the JSDoc param name and the function
signature of createNodeSnapshot({ id, propsMap, edgesOut, edgesIn }) to use
propsRecord, change the local const props = buildPropsSnapshot(propsMap) to
buildPropsSnapshot(propsRecord), and update every internal reference and any
callers in this file that pass or destructure propsMap (including the other
functions around the other occurrences mentioned) to use propsRecord instead;
ensure you also rename any related variable declarations and type annotations so
the names are consistent across createNodeSnapshot and the other similar
functions referenced.
In `@test/runtime/deno/lifecycle.test.ts`:
- Around line 39-49: The test title "lifecycle: node properties via Map" is
misleading because the assertion checks object/Record access (props?.k); update
the Deno.test call to a clearer name such as "lifecycle: node properties" by
changing the first argument string in the Deno.test invocation so it no longer
mentions "via Map" (locate the Deno.test with the same anonymous async function
that creates
repo/openGraph/createPatch/addNode/setProperty/materialize/getNodeProps).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/unit/domain/__snapshots__/WarpGraph.apiSurface.test.js.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
CHANGELOG.mdREADME.mdROADMAP.mdbin/cli/commands/bisect.jsbin/cli/commands/registry.jsbin/cli/infrastructure.jsbin/cli/schemas.jsbin/cli/types.jscontracts/type-surface.m8.jsondocs/GUIDE.mddocs/ROADMAP/COMPLETED.mdexamples/scripts/explore.jsexamples/scripts/setup.jsindex.d.tsindex.jsjsr.jsonpackage.jsonsrc/domain/WarpGraph.jssrc/domain/services/BisectService.jssrc/domain/services/ObserverView.jssrc/domain/services/QueryBuilder.jssrc/domain/warp/_wiredMethods.d.tssrc/domain/warp/patch.methods.jssrc/domain/warp/query.methods.jssrc/domain/warp/subscribe.methods.jstest/integration/api/edge-cases.test.jstest/integration/api/fork.test.jstest/integration/api/lifecycle.test.jstest/runtime/deno/lifecycle.test.tstest/type-check/consumer.tstest/unit/domain/WarpGraph.edgeProps.test.jstest/unit/domain/WarpGraph.invalidation.test.jstest/unit/domain/WarpGraph.lazyMaterialize.test.jstest/unit/domain/WarpGraph.noCoordination.test.jstest/unit/domain/WarpGraph.patchMany.test.jstest/unit/domain/WarpGraph.query.test.jstest/unit/domain/WarpGraph.subscribe.test.jstest/unit/domain/WarpGraph.watch.test.jstest/unit/domain/WarpGraph.writerInvalidation.test.jstest/unit/domain/services/BisectService.test.jstest/unit/domain/services/ObserverView.test.jstest/unit/domain/services/TrustPayloadParity.test.jstest/unit/domain/utils/CachedValue.test.js
- Fix ROADMAP.md inventory: move B44/B100/B124/B125/B146 to done (24→29)
- Remove M11 from Early Milestones table in COMPLETED.md (scope mismatch)
- Reorder M11 section in COMPLETED.md to numerical position (after M10)
- Fix stale Deno test name ("via Map" → generic)
- Add candidates-nonempty invariant comment in BisectService
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ROADMAP.md (1)
168-170:⚠️ Potential issue | 🟡 MinorReconcile M11 completion with downstream sequencing text.
These lines now correctly mark M11 as complete/archived, but downstream sections still say M11 comes after M14 (Line 287, Line 293, Line 434). Please align those sections so milestone order/status is internally consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` around lines 168 - 170, The milestone heading "Milestone 11 — COMPASS II ✅ COMPLETE (v13.0.0)" has been archived but other downstream references still place M11 after M14; find every occurrence of "M11", "COMPASS II", and the textual sequencing that mentions milestone order (including the paragraphs referencing M11 after M14) and update them to reflect that M11 is completed/archived and its correct position in the sequence (either move the mentions to the archived/COMPLETED.md section or rewrite the downstream sentences so milestone numbering/order is consistent with the heading). Ensure all references and ordering text match the archived status and the intended milestone sequence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Around line 184-187: The roadmap shows B44, B124, and B125 as completed in the
top table but they still appear as active "Near-term" priorities later; update
the standalone priority tiers by removing or marking B44, B124, and B125 as
completed and replace them with open items (e.g., B127 / DENO SMOKE TEST or
other backlog IDs) so the "Near-term" list remains actionable and consistent
with the table; ensure references to the specific IDs (B44, B124, B125, B127 and
the DENO SMOKE TEST entry) are updated or substituted accordingly.
In `@src/domain/services/BisectService.js`:
- Around line 129-130: The midpoint materialization uses a global Lamport
ceiling (this._graph.materialize({ ceiling: candidate.patch.lamport })) which
can include other writers' patches and skew testFn results; change to a
writer-scoped materialization so the state reflects only that writer's history
up to the candidate. Update BisectService to call a writer-scoped primitive
(e.g., this._graph.materializeForWriter or this._graph.materialize with
writer/seq args) using the candidate's author/writer id and the writer-local
sequence/timestamp from candidate.patch (not the global Lamport ceiling), so
testFn(state, candidate.sha) runs against the writer-only prefix for that
candidate.
---
Duplicate comments:
In `@ROADMAP.md`:
- Around line 168-170: The milestone heading "Milestone 11 — COMPASS II ✅
COMPLETE (v13.0.0)" has been archived but other downstream references still
place M11 after M14; find every occurrence of "M11", "COMPASS II", and the
textual sequencing that mentions milestone order (including the paragraphs
referencing M11 after M14) and update them to reflect that M11 is
completed/archived and its correct position in the sequence (either move the
mentions to the archived/COMPLETED.md section or rewrite the downstream
sentences so milestone numbering/order is consistent with the heading). Ensure
all references and ordering text match the archived status and the intended
milestone sequence.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mddocs/ROADMAP/COMPLETED.mdsrc/domain/services/BisectService.jstest/runtime/deno/lifecycle.test.ts
Comment on lines
+129
to
+130
| const state = await this._graph.materialize({ ceiling: candidate.patch.lamport }); | ||
| const isGood = await testFn(state, candidate.sha); |
There was a problem hiding this comment.
Bisect midpoint materialization is writer-ambiguous in multi-writer histories.
Line 129 materializes by Lamport ceiling, which can include patches from other writers. That can skew testFn outcomes and return the wrong first bad patch for the selected writer.
💡 Suggested direction (writer-scoped materialization primitive)
- * `@property` {(opts: {ceiling: number}) => Promise<import('./JoinReducer.js').WarpStateV5>} materialize
+ * `@property` {(opts: {writerId: string, sha: string}) => Promise<import('./JoinReducer.js').WarpStateV5>} materializeAtWriterPatch- const { index, steps } = await this._binarySearch(candidates, testFn);
+ const { index, steps } = await this._binarySearch(writerId, candidates, testFn);- async _binarySearch(candidates, testFn) {
+ async _binarySearch(writerId, candidates, testFn) {
...
- const state = await this._graph.materialize({ ceiling: candidate.patch.lamport });
+ const state = await this._graph.materializeAtWriterPatch({ writerId, sha: candidate.sha });
const isGood = await testFn(state, candidate.sha);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/services/BisectService.js` around lines 129 - 130, The midpoint
materialization uses a global Lamport ceiling (this._graph.materialize({
ceiling: candidate.patch.lamport })) which can include other writers' patches
and skew testFn results; change to a writer-scoped materialization so the state
reflects only that writer's history up to the candidate. Update BisectService to
call a writer-scoped primitive (e.g., this._graph.materializeForWriter or
this._graph.materialize with writer/seq args) using the candidate's
author/writer id and the writer-local sequence/timestamp from candidate.patch
(not the global Lamport ceiling), so testFn(state, candidate.sha) runs against
the writer-only prefix for that candidate.
- Move B100 from non-standard ### Breaking to ### Changed (CHANGELOG) - Remove done items B44/B124/B125 from priority sequence (ROADMAP) - Fix stale "6 test vectors" → "9 test vectors" in M11 gate (COMPLETED) - Replace BisectResult interface with discriminated union type (index.d.ts) - Add SHA format validation to bisect --good/--bad (schemas.js) - Remove spurious double-blank in COMPLETED.md - Add bisect schema unit tests for SHA validation
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/domain/services/BisectService.js (1)
137-138:⚠️ Potential issue | 🟠 MajorBisect midpoint materialization is still writer-ambiguous.
Line 137 materializes by global Lamport ceiling, so midpoint states can include other writers’ patches and skew
testFnoutcomes for the selected writer chain.Suggested direction
- const { index, steps } = await this._binarySearch(candidates, testFn); + const { index, steps } = await this._binarySearch(writerId, candidates, testFn); - async _binarySearch(candidates, testFn) { + async _binarySearch(writerId, candidates, testFn) { ... - const state = await this._graph.materialize({ ceiling: candidate.patch.lamport }); + const state = await this._graph.materializeAtWriterPatch({ writerId, sha: candidate.sha }); const isGood = await testFn(state, candidate.sha);/** * `@typedef` {Object} BisectGraph * `@property` {(writerId: string) => Promise<Array<{patch: {lamport: number}, sha: string}>>} getWriterPatches - * `@property` {(opts: {ceiling: number}) => Promise<import('./JoinReducer.js').WarpStateV5>} materialize + * `@property` {(opts: {writerId: string, sha: string}) => Promise<import('./JoinReducer.js').WarpStateV5>} materializeAtWriterPatch */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/BisectService.js` around lines 137 - 138, The midpoint materialization uses a global Lamport ceiling and can include other writers' patches, skewing testFn results: change the call in BisectService where you call this._graph.materialize({ ceiling: candidate.patch.lamport }) so it materializes up to the candidate's writer-specific ceiling (use the candidate patch's writer/chain identifier and its lamport as the ceiling for that writer) before invoking testFn(state, candidate.sha); locate the materialization call and ensure you pass a writer-scoped ceiling (e.g., ceiling object that includes writerId/chainId plus lamport or use the graph API method that accepts per-writer ceilings) so the midpoint state contains only patches from the selected writer chain.ROADMAP.md (1)
168-170:⚠️ Potential issue | 🟡 MinorRoadmap sequence still contradicts M11 completion status.
Line 168 marks M11 complete, but later execution-order text still says M11 comes after M14 (Line 293, Line 430, Line 434). Please reconcile those sections to keep milestone ordering internally consistent.
Suggested doc patch
-### Milestones: M10 → M12 → M13 → M14 → M11 +### Milestones: M10 → M12 → M13 → M14 (M11 complete, archived) -5. **M11 COMPASS II** — Developer experience (B2 impl, B3, B11) — after M14 +5. **M11 COMPASS II** — Developer experience (B2 impl, B3, B11) — ✅ DONE (v13.0.0), archived -Execution: M10 SENTINEL → **M12 SCALPEL** → **M13 SCALPEL II** → **M14 HYGIENE** → M11 COMPASS II. Standalone items fill the gaps. +Execution: M10 SENTINEL → **M12 SCALPEL** → **M13 SCALPEL II** → **M14 HYGIENE**. M11 COMPASS II is complete and archived. Standalone items fill the gaps. -M14 HYGIENE is the current priority — test hardening, DRY extraction, and SOLID quick-wins from the HEX_AUDIT. M11 follows after M14. +M14 HYGIENE is the current priority — test hardening, DRY extraction, and SOLID quick-wins from the HEX_AUDIT. M11 is complete and archived in COMPLETED.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` around lines 168 - 170, The document declares "Milestone 11 — COMPASS II ✅ COMPLETE (v13.0.0)" but later execution-order text still places M11 after M14; update the later sections so milestone ordering is consistent: locate the execution-order paragraphs that mention "M11" coming after "M14" and either move/relabel those references so M11 appears before M14 or mark them as referencing historical order (e.g., change phrasing to "previously M11" or remove the incorrect ordering), and ensure the milestone heading "Milestone 11 — COMPASS II ✅ COMPLETE (v13.0.0)" remains unchanged; check all occurrences of "M11" and "M14" in the document and reconcile them to a single, internally consistent sequence.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
35-35: Consider splitting this mega-bullet for readability.Line 35 is accurate but very dense. Splitting into 2–4 sub-bullets (CLI hardening, ROADMAP housekeeping, type/schema updates) would make release-note scanning much easier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 35, Split the single dense CHANGELOG bullet into 2–4 sub-bullets grouped by theme to improve readability: create a CLI hardening bullet referencing the added `--writer` validation and SHA format validation for the bisect CLI, a BisectService/type/schema bullet mentioning removal of dead code in `BisectService` and the replacement of the `BisectResult` interface with a discriminated union, and a housekeeping bullet for ROADMAP/COMPLETED.md/test fixes (reconciled ROADMAP counts, moved B100 from Breaking to Changed, fixed M11 placement, updated Deno test name and stale test vector counts). Use short phrases and keep each sub-bullet one line so the release notes scan easily.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ROADMAP.md`:
- Around line 168-170: The document declares "Milestone 11 — COMPASS II ✅
COMPLETE (v13.0.0)" but later execution-order text still places M11 after M14;
update the later sections so milestone ordering is consistent: locate the
execution-order paragraphs that mention "M11" coming after "M14" and either
move/relabel those references so M11 appears before M14 or mark them as
referencing historical order (e.g., change phrasing to "previously M11" or
remove the incorrect ordering), and ensure the milestone heading "Milestone 11 —
COMPASS II ✅ COMPLETE (v13.0.0)" remains unchanged; check all occurrences of
"M11" and "M14" in the document and reconcile them to a single, internally
consistent sequence.
In `@src/domain/services/BisectService.js`:
- Around line 137-138: The midpoint materialization uses a global Lamport
ceiling and can include other writers' patches, skewing testFn results: change
the call in BisectService where you call this._graph.materialize({ ceiling:
candidate.patch.lamport }) so it materializes up to the candidate's
writer-specific ceiling (use the candidate patch's writer/chain identifier and
its lamport as the ceiling for that writer) before invoking testFn(state,
candidate.sha); locate the materialization call and ensure you pass a
writer-scoped ceiling (e.g., ceiling object that includes writerId/chainId plus
lamport or use the graph API method that accepts per-writer ceilings) so the
midpoint state contains only patches from the selected writer chain.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 35: Split the single dense CHANGELOG bullet into 2–4 sub-bullets grouped
by theme to improve readability: create a CLI hardening bullet referencing the
added `--writer` validation and SHA format validation for the bisect CLI, a
BisectService/type/schema bullet mentioning removal of dead code in
`BisectService` and the replacement of the `BisectResult` interface with a
discriminated union, and a housekeeping bullet for ROADMAP/COMPLETED.md/test
fixes (reconciled ROADMAP counts, moved B100 from Breaking to Changed, fixed M11
placement, updated Deno test name and stale test vector counts). Use short
phrases and keep each sub-bullet one line so the release notes scan easily.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d146b406-d31e-4f97-9a23-fd2bf717a04f
📒 Files selected for processing (8)
CHANGELOG.mdROADMAP.mdbin/cli/schemas.jscontracts/type-surface.m8.jsondocs/ROADMAP/COMPLETED.mdindex.d.tssrc/domain/services/BisectService.jstest/unit/cli/schemas.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/cli/schemas.js
- Rethrow ENOENT/EACCES from bisect test command runner instead of swallowing all execSync errors (bin/cli/commands/bisect.js) - Use Object.create(null) for property bags in getNodeProps, getEdgeProps, getEdges, and buildPropsSnapshot to prevent prototype pollution (src/domain/warp/query.methods.js, src/domain/services/QueryBuilder.js) - Fix indexed-path null masking in getNodeProps — fall through to linear scan when reader returns null instead of returning empty object - Rename propsMap → propsRecord in QueryBuilder for consistency with Record<string, unknown> type - Reconcile M11 completion status with downstream sequencing text in ROADMAP.md (3 locations still said "after M14") - Split dense B148 CHANGELOG bullet into themed sub-bullets
Member
Author
Review Feedback — Round 3 ResponseAddressed in
|
| # | File | Issue | Fix |
|---|---|---|---|
| 1 | bin/cli/commands/bisect.js |
runTestCommand swallows ENOENT/EACCES |
Rethrow spawn failures; only return false for non-zero exit status |
| 2 | src/domain/warp/query.methods.js |
{} property bags vulnerable to prototype pollution |
Object.create(null) in getNodeProps, getEdgeProps, getEdges |
| 3 | src/domain/warp/query.methods.js |
record || {} masks null in indexed fast path |
Fall through to linear scan when reader returns null |
| 4 | src/domain/services/QueryBuilder.js |
buildPropsSnapshot uses {} |
Object.create(null) |
| 5 | src/domain/services/QueryBuilder.js |
propsMap naming stale after Map→Record migration |
Renamed to propsRecord (4 sites) |
| 6 | ROADMAP.md |
M11 "after M14" contradicts completed status | Reconciled 4 locations: heading, list item, execution paragraph, final command |
| 7 | CHANGELOG.md |
Dense B148 mega-bullet | Split into 4 themed sub-bullets |
Previously addressed
docs/ROADMAP/COMPLETED.md"6 test vectors" → "9 test vectors" ✅367efdfROADMAP.mdB44/B124/B125 in priority tiers ✅367efdftest/runtime/deno/lifecycle.test.ts"via Map" test rename ✅d7cf956
By design — BisectService global Lamport ceiling (flagged Major × 2 rounds)
The materialize({ ceiling }) call is intentionally global, not writer-scoped. Bisect answers: "Which patch, applied in the merged multi-writer state, first introduced the issue?" — this is the state users actually observe.
Writer-isolated materialization would:
- Produce states that never existed in the real system
- Miss bugs caused by cross-writer interactions (the most common class of CRDT merge issues)
- Contradict Paper II's tick semantics, where ticks materialize the full state at a causal frontier
If a future use case needs writer-isolated bisect, that would be a separate --isolated mode, not a change to the default. Logging this to cool_ideas.md for consideration.
Verification
- ESLint: ✅ clean
- tsc
--noEmit: ✅ 0 errors - IRONCLAD ratchet: ✅ 0/0 wildcards
- Unit tests: ✅ 4619 passed (252 files)
@coderabbitai — ready for another look 🥕
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the fixes applied in 🏁 Script executed: Length of output: 51928
However, after checking out the current PR branch, the 7 fixes from ❌ Item
|
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ROADMAP.md (1)
287-294:⚠️ Potential issue | 🟡 MinorMilestone order is still inconsistent within the same section.
Line 287/430 place M11 before M14, but Lines 292-294 enumerate M14 as
#4and M11 as#5. Please make the numbered list match the declared sequence to avoid roadmap ambiguity.Also applies to: 430-434
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` around lines 287 - 294, The milestone sequence in the header "Milestones: M10 → M12 → M13 → M11 → M14" does not match the numbered list order; update the numbered list under that header so its entries follow the declared sequence (M10, M12, M13, M11, M14) — reorder the bullets so M11 appears before M14 in the inline list to match the header (or alternatively change the header to match the list), and make the same correction for the duplicate block referenced (the other occurrence around the 430–434 section) so both header and enumerations are consistent.
🧹 Nitpick comments (2)
ROADMAP.md (1)
328-328: Clarify what “(29 done)” refers to in total tracking.
Total tracked | 123 (29 done)reads like global done-count, but 29 appears to be the standalone-done bucket. Consider labeling it explicitly (e.g.,123 total; 29 standalone done) for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 328, The table row showing "Total tracked | 123 (29 done)" is ambiguous; update the cell text for the "Total tracked" row (the table line containing the string "Total tracked") to explicitly label which counts are overall vs standalone—e.g., change the cell to "123 total; 29 standalone done" or "123 total (29 standalone done)" so readers immediately know that 29 refers to standalone-done rather than the global done count.src/domain/services/QueryBuilder.js (1)
675-677: Consider usingObject.create(null)for the fallback to maintain defensive consistency.The fallback
|| {}creates a standard object with a prototype chain, which is inconsistent with theObject.create(null)pattern now used inbuildPropsSnapshotandgetNodeProps(per PR comments). IfgetNodePropsreturnsnull, this memoized fallback flows to_runAggregatewhere user-specified property paths are accessed.Suggested change
- const propsRecord = (await this._graph.getNodeProps(nodeId)) || {}; + const propsRecord = (await this._graph.getNodeProps(nodeId)) || Object.create(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/QueryBuilder.js` around lines 675 - 677, The memo fallback uses {} which creates a prototypeful object; change the fallback in the props retrieval to use Object.create(null) so the memoized propsRecord is prototype-less and consistent with buildPropsSnapshot/getNodeProps patterns—update the expression where propsRecord is assigned from await this._graph.getNodeProps(nodeId) (and where propsMemo.set(nodeId, propsRecord) follows) so that when getNodeProps returns null the code assigns Object.create(null) instead, preventing prototype inheritance leaks into _runAggregate and any user-specified property path access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ROADMAP.md`:
- Around line 287-294: The milestone sequence in the header "Milestones: M10 →
M12 → M13 → M11 → M14" does not match the numbered list order; update the
numbered list under that header so its entries follow the declared sequence
(M10, M12, M13, M11, M14) — reorder the bullets so M11 appears before M14 in the
inline list to match the header (or alternatively change the header to match the
list), and make the same correction for the duplicate block referenced (the
other occurrence around the 430–434 section) so both header and enumerations are
consistent.
---
Nitpick comments:
In `@ROADMAP.md`:
- Line 328: The table row showing "Total tracked | 123 (29 done)" is ambiguous;
update the cell text for the "Total tracked" row (the table line containing the
string "Total tracked") to explicitly label which counts are overall vs
standalone—e.g., change the cell to "123 total; 29 standalone done" or "123
total (29 standalone done)" so readers immediately know that 29 refers to
standalone-done rather than the global done count.
In `@src/domain/services/QueryBuilder.js`:
- Around line 675-677: The memo fallback uses {} which creates a prototypeful
object; change the fallback in the props retrieval to use Object.create(null) so
the memoized propsRecord is prototype-less and consistent with
buildPropsSnapshot/getNodeProps patterns—update the expression where propsRecord
is assigned from await this._graph.getNodeProps(nodeId) (and where
propsMemo.set(nodeId, propsRecord) follows) so that when getNodeProps returns
null the code assigns Object.create(null) instead, preventing prototype
inheritance leaks into _runAggregate and any user-specified property path
access.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd1cb266-0c45-4ec8-8bf5-3afa3dd9ce13
📒 Files selected for processing (5)
CHANGELOG.mdROADMAP.mdbin/cli/commands/bisect.jssrc/domain/services/QueryBuilder.jssrc/domain/warp/query.methods.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/warp/query.methods.js
- CHANGELOG.md
Release Preflight
If you tag this commit as |
Member
Author
|
All 3 findings from round 4 addressed in `65f0f56`:
@coderabbitai review please 🥕 |
🧩 Analysis chainLet me verify the full state of the branch, including both round 3 and round 4 fixes. 🏁 Script executed: Length of output: 56024
Round 3 (
Round 4 (
Triggering the full review now. ✅ Actions performedFull review triggered. |
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
BisectTestFntypedef and unreachablecandidates.length === 0guard--writervalidation (throwsusageErrorwhen defaultcliwriter used), fixed exit code fromUSAGE+1to namedNOT_FOUNDconstantvi/beforeEachimports andCLI_TRUST_OVERRIDESconstantgetNodePropsreturn type description (Map→Record<string, unknown>)Test plan
npm run lint— ESLint cleannpm run test:local— 252 files, 4612 tests passed (including 3 new bisect vectors)WarpGraph.noCoordination.test.js— passednpm run release:preflight— all checks green (dirty-tree expected before commit)Summary by CodeRabbit
New Features
Breaking Changes