CR-01: Separate checkpoints from Echo anchor authority - #265
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughChangesThe PR separates jedit checkpoint declarations from Echo causal-anchor admission. It adds opaque admission evidence and checkpoint-association facts, validates their identities and references, updates runtime failure handling, and expands contract and runtime tests. Checkpoint anchor flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f7549ef30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code Lawyer self-audit: merge blockedThe current head cannot be merged as an Echo causal-anchor integration. The three unresolved Codex P2 threads are valid, and the deeper authority audit found two additional blocking defects.
Echo evidence: @codex review please: confirm whether #1 and #2 are correctly classified as merge-blocking authority violations, and inspect for any additional boundary mismatch. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f7549ef30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
flyingrobots
changed the title
Activity summary
All nine prior review threads are now outdated and resolved. Validation: The handwritten capability is explicitly transitional and will be replaced by a generated Edict client once native package installation/invocation is ready. @codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c386370b49
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Follow-up @codex review please |
![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 (2)
src/domain/graph-rope-runtime.ts (2)
1-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep
src/domain/graph-rope-runtime.tsunder the 500-line cap. It’s already at 485 lines, so one small addition will push it over; move more of the anchoring/checkpoint orchestration out of this file before it becomes a hard violation.🤖 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/graph-rope-runtime.ts` around lines 1 - 482, Reduce graph-rope-runtime.ts below the 500-line limit by extracting additional checkpoint/anchoring orchestration from createCheckpoint and anchorCheckpoint into the existing graph-rope-runtime-checkpoint or a focused helper module. Keep createGraphRopeRuntime’s public methods and result/error behavior unchanged, leaving the runtime file within the cap.Source: Coding guidelines
33-66: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winRemove the duplicate named imports from
./graph-rope-runtime-issues.js.GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_ADMISSION_FAILED,GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_UNAVAILABLE, andGRAPH_ROPE_RUNTIME_OBSTRUCTION_MISSING_CHECKPOINTare imported twice in the same file, which is a duplicate-identifier syntax error. Consolidate them into a single import block.🤖 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/graph-rope-runtime.ts` around lines 33 - 66, Remove the duplicate named imports for GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_ADMISSION_FAILED, GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_UNAVAILABLE, and GRAPH_ROPE_RUNTIME_OBSTRUCTION_MISSING_CHECKPOINT from the top-level graph-rope-runtime-issues.js import, while preserving their existing re-exports in the export block.
🤖 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/graph-rope-runtime-echo-adapter.ts`:
- Around line 10-45: Update requestCheckpointAnchorAdmission and
isUsableEchoEvidence to validate returned evidence against the submitted
request, including checkpointId, worldlineId, headId, reason, and
materializationRoots, rejecting stale or mismatched responses before returning
them. Preserve null for invalid admissions, but stop blanket-swallowing port
failures: distinguish and propagate genuine admitCheckpointAnchor errors
according to the adapter’s existing error contract.
In `@src/domain/graph-rope-types.ts`:
- Around line 302-310: Persist checkpoint materialization roots through the
anchoring flow: in src/domain/graph-rope-types.ts:302-310 add
materializationRoots to RopeCheckpointAnchoredFact and include it in
ropeCheckpointAnchorAssociationIdFor identity input if association identity
requires it; in src/domain/graph-rope-runtime-checkpoint.ts:75-95 extend
createCheckpointAnchorAssociation to accept and copy the roots; in
src/domain/graph-rope-runtime.ts:272-304 pass request.materializationRoots or
input.materializationRoots into that factory call.
- Around line 280-286: Extend RopeCheckpointAnchorAdmissionRequest with
retainedRoots, purpose, and all required retention metadata fields, matching the
retention payload used at the Echo boundary and the validation contract in
graph-rope-causal-anchor-validation.ts. Update the corresponding validation
logic so these fields are accepted and preserved rather than discarded, while
retaining validation for the existing checkpointId, worldlineId, headId, reason,
and materializationRoots fields.
---
Outside diff comments:
In `@src/domain/graph-rope-runtime.ts`:
- Around line 1-482: Reduce graph-rope-runtime.ts below the 500-line limit by
extracting additional checkpoint/anchoring orchestration from createCheckpoint
and anchorCheckpoint into the existing graph-rope-runtime-checkpoint or a
focused helper module. Keep createGraphRopeRuntime’s public methods and
result/error behavior unchanged, leaving the runtime file within the cap.
- Around line 33-66: Remove the duplicate named imports for
GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_ADMISSION_FAILED,
GRAPH_ROPE_RUNTIME_OBSTRUCTION_CAUSAL_ANCHOR_UNAVAILABLE, and
GRAPH_ROPE_RUNTIME_OBSTRUCTION_MISSING_CHECKPOINT from the top-level
graph-rope-runtime-issues.js import, while preserving their existing re-exports
in the export block.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bf72ba7-aace-4708-8804-cf36cf0e0a28
📒 Files selected for processing (16)
docs/design/0149-graph-backed-rope-runtime-discovery.mdspec/design-cycle-policy.spec.mjsspec/graph-rope-contract.spec.mjsspec/graph-rope-runtime.spec.mjsspec/support/test-echo-causal-anchor-admission.mjssrc/domain/graph-rope-causal-anchor-digest.tssrc/domain/graph-rope-causal-anchor-validation.tssrc/domain/graph-rope-checkpoint-identity.tssrc/domain/graph-rope-checkpoint-validation.tssrc/domain/graph-rope-fact-id.tssrc/domain/graph-rope-runtime-checkpoint.tssrc/domain/graph-rope-runtime-echo-adapter.tssrc/domain/graph-rope-runtime-issues.tssrc/domain/graph-rope-runtime.tssrc/domain/graph-rope-types.tssrc/domain/graph-rope-validation.ts
💤 Files with no reviewable changes (1)
- src/domain/graph-rope-causal-anchor-digest.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test / cycle-proofs
- GitHub Check: test / contract-api
- GitHub Check: test / echo-authority
- GitHub Check: release-gate / jedit-echo
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Noanytype usage in TypeScript code
Nounknowntype usage in TypeScript code
No TypeScript file over 500 lines of code
Make invalid states unrepresentable at runtime where possible
Prefer constructor or factory injection over ambient singletons for new code
Files:
src/domain/graph-rope-runtime-echo-adapter.tssrc/domain/graph-rope-fact-id.tssrc/domain/graph-rope-checkpoint-validation.tssrc/domain/graph-rope-runtime-issues.tssrc/domain/graph-rope-causal-anchor-validation.tssrc/domain/graph-rope-types.tssrc/domain/graph-rope-runtime.tssrc/domain/graph-rope-validation.tssrc/domain/graph-rope-runtime-checkpoint.tssrc/domain/graph-rope-checkpoint-identity.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js}: No ad hoc string comparison in core logic; preferinstanceof, numeric tags, symbols, or explicit runtime objects
No magic strings in code
No magic numbers in code
Files:
src/domain/graph-rope-runtime-echo-adapter.tssrc/domain/graph-rope-fact-id.tssrc/domain/graph-rope-checkpoint-validation.tssrc/domain/graph-rope-runtime-issues.tssrc/domain/graph-rope-causal-anchor-validation.tssrc/domain/graph-rope-types.tssrc/domain/graph-rope-runtime.tssrc/domain/graph-rope-validation.tssrc/domain/graph-rope-runtime-checkpoint.tssrc/domain/graph-rope-checkpoint-identity.ts
docs/design/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use the template from
docs/design/TEMPLATE.mdfor all full cycle designs
Files:
docs/design/0149-graph-backed-rope-runtime-discovery.md
🪛 LanguageTool
docs/design/0149-graph-backed-rope-runtime-discovery.md
[style] ~823-~823: To elevate your writing, try using an alternative expression here.
Context: ...edit declaration that a named rope head matters for a domain reason. That declaration d...
(MATTERS_RELEVANT)
🔇 Additional comments (20)
src/domain/graph-rope-types.ts (2)
22-25: LGTM!Also applies to: 252-268, 323-323
288-300: 🗄️ Data Integrity & IntegrationDrop this concern. These are local domain seam types in
src/domain/graph-rope-types.ts, andadmitCheckpointAnchoris consumed as an in-process port byrequestCheckpointAnchorAdmission/anchorCheckpoint; there’s no basis here for an Echo-exported contract/codec mismatch or an async requirement.> Likely an incorrect or invalid review comment.src/domain/graph-rope-checkpoint-identity.ts (1)
6-45: LGTM! TheidentityTuple/JSON.stringifyswitch is a genuine improvement over colon-joined strings — nice catch on the collision surface.src/domain/graph-rope-checkpoint-validation.ts (1)
1-118: LGTM!src/domain/graph-rope-fact-id.ts (1)
4-4: LGTM!Also applies to: 29-29, 79-81
src/domain/graph-rope-validation.ts (1)
3-5: LGTM!Also applies to: 29-29, 59-71, 313-315
src/domain/graph-rope-runtime-checkpoint.ts (2)
27-40: LGTM!Also applies to: 42-60, 62-73
2-13: 🎯 Functional CorrectnessImport path is correct.
src/domain/graph-rope-contract.tsre-exportsgraph-rope-types.js, so./graph-rope-contract.jsis a valid barrel import.> Likely an incorrect or invalid review comment.src/domain/graph-rope-runtime.ts (2)
84-86: LGTM! Constructor/factory injection ofcausalAnchorAdmissionwith anullfail-closed default is exactly the right pattern here.Also applies to: 166-172, 178-211
436-443: 🎯 Functional CorrectnessNo issue here:
cloneFactis generic, soechoEvidencetype-checks.> Likely an incorrect or invalid review comment.src/domain/graph-rope-runtime-issues.ts (1)
5-21: LGTM!src/domain/graph-rope-causal-anchor-validation.ts (2)
20-33: 🎯 Functional CorrectnessConfirm this request shape actually maps onto Echo's real boundary contract.
Good: validation runs fully before the admission port is invoked (matches the fix for the "invalid roots checked after port consumed request" self-audit finding), confirmed by
anchorCheckpointingraph-rope-runtime.tscallingvalidateCheckpointAnchorAdmissionRequestbefore checkingstate.causalAnchorAdmission.However,
RopeCheckpointAnchorAdmissionRequesthere only carriescheckpointId,worldlineId,headId,reason,materializationRoots. The PR objectives call for the Jim↔Echo boundary to carry subject, basis frontier, retained roots, purpose, and retention metadata, and the self-audit explicitly flags that Jim independently defining the anchor schema/digest (rather than Echo's exported contract) produces incompatible identities. If this file's shape is the actual payload sent to Echo (rather than an internal pre-image later translated by an adapter), it looks like that concern is still open.Since
graph-rope-runtime-echo-adapter.tsisn't in this review batch, can you confirm whether it translates this request into Echo's actual exported contract/codec, or whether this shape is what's sent to Echo?
35-75: LGTM!spec/graph-rope-contract.spec.mjs (1)
4-8: LGTM!Also applies to: 17-21, 189-199, 201-216, 218-252, 254-286, 415-421, 466-492
spec/graph-rope-runtime.spec.mjs (1)
4-4: LGTM!Also applies to: 13-15, 182-218, 220-239, 241-287, 289-326, 328-354, 356-389, 391-417, 442-447, 449-481, 483-494, 514-530
spec/support/test-echo-causal-anchor-admission.mjs (1)
1-29: LGTM!spec/design-cycle-policy.spec.mjs (1)
156-161: LGTM!docs/design/0149-graph-backed-rope-runtime-discovery.md (3)
87-89: 📐 Maintainability & Code Quality | ⚡ Quick winClarify Echo's evidence ownership wording.
"Echo owns causal-anchor and admission evidence" reads as two separate nouns ("causal-anchor [evidence]" and "admission evidence"), whereas the rest of the doc (lines 96, 825) consistently uses the compound "causal-anchor admission" as one concept. Worth tightening so the ownership boundary reads unambiguously.
✏️ Suggested wording
-- `RopeRewrite`, `RopeDiff`, application tick evidence, checkpoint declarations, - and anchor associations are retained jedit evidence; Echo owns causal-anchor - and admission evidence. +- `RopeRewrite`, `RopeDiff`, application tick evidence, checkpoint declarations, + and anchor associations are retained jedit evidence; Echo owns causal-anchor + admission evidence.
750-767: 🗄️ Data Integrity & Integration | ⚡ Quick winDurable-truth list omits the new
RopeCheckpointAnchoredFactevidence type.This section's rule is "If deleting it changes history, it is evidence." The newly introduced
RopeCheckpointAnchoredFact(lines 850-858) records an Echo-admitted causal-anchor association with a checkpoint — deleting it clearly changes recoverable history — yet it's absent from the durable-truth enumeration, which still only listsRopeCheckpoint. Leaving it out risks a future implementer treating the association fact as a rebuildable index.✏️ Suggested addition
- `RopeCheckpoint`. +- `RopeCheckpointAnchoredFact`.
470-476: LGTM!Also applies to: 509-509, 822-848, 849-859, 861-878
|
Resolved the three Codex P2 threads from the
Full local gate at @codex review please |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79f698e8c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Final-head review activity
Local @codex review please |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
RopeCheckpointFactdeclarations from optionalRopeCheckpointAnchoredFactassociationsspec/supportwith explicit injection and no Echo-authority claimSlices
Validation
npm run check: 1,257 tests; 1,244 passed; 13 intentionally skipped; 0 failednpm run quality: 381 files; regressions: nonenpx markdownlint-cli2 docs/design/0149-graph-backed-rope-runtime-discovery.md: 0 errorsTransitional boundary
The TypeScript capability is a narrow, single-operation seam, not the permanent Jim/Echo protocol. Once Echo can install and invoke the relevant generated Edict package, the generated Edict client replaces this port operation by operation. This PR deliberately does not implement that migration or copy Edict/Echo authority logic into Jim.
npm run devcurrently fails closed withMissingGraphRopeTextAuthorityErrorbecause the composition root does not yet install graph-rope authority. That separate CR-02 slice is tracked by #222; this PR does not weaken the production guard or reintroduce snapshot authority to conceal it.