feat(warp-core): add anchored-node create-if-absent program#686
feat(warp-core): add anchored-node create-if-absent program#686flyingrobots wants to merge 12 commits into
Conversation
Widen EchoOperationProgramV1::AnchoredNodeAttachmentCompareAndSet's per-invocation precondition from a required digest to Option<Hash>. Some(digest) is exactly today's update semantics, unchanged; None asserts the node and its typed alpha attachment are both entirely absent, and Echo creates both atomically in one patch using the package-declared node type, or refuses with PreconditionMismatch. The corridor's only program kind previously required the node and attachment to already exist at both the coarse admission-time application-basis check and the fine-grained evaluation-time check. That was invisible to ReplaceRange (#684) because Jedit's buffer nodes are created through CreateBufferWorldline, outside this corridor. Any caller whose writes are new facts rather than edits to a node created elsewhere hits this gap immediately. current_application_basis widens symmetrically through a new, domain-separated absent proposition so a stale or dishonest absence claim is caught at admission, not only at evaluation. anchored_node_footprint declares node-write iff creation was claimed, keeping the declared/actual footprint equality check exact in both branches. WarpOp::UpsertNode already existed and is reused; no new program kind, WarpOp kind, or application-specific intrinsic is introduced. All 13 pre-existing tests in executable_operation_pipeline_tests.rs pass with unmodified behavior. Three new tests exercise the full TrustedRuntimeHost pipeline for create-from-absence, create-refuses- when-exists, and update-refuses-when-absent. Full warp-core suite (81 binaries, 686 library tests, doctests) passes; clippy reports the identical pre-existing error set before and after. See ADR 0024 for the full decision record. Closes #685
|
Warning Review limit reached
Next review available in: 48 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe executable-operation corridor adds a distinct create-if-absent program. It separately defines creation schemas, identities, absence admission, atomic node-and-attachment evaluation, program-bound WAL validation, and tests while preserving compare-and-set update semantics. ChangesAnchored-node creation corridor
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant TrustedRuntimeHost
participant EchoOperation
participant StateStore
participant WAL
Caller->>TrustedRuntimeHost: submit create-if-absent invocation
TrustedRuntimeHost->>EchoOperation: admit and prepare against installed program
EchoOperation->>StateStore: verify node and attachment absence
EchoOperation-->>TrustedRuntimeHost: emit atomic UpsertNode plus SetAttachment patch
TrustedRuntimeHost->>StateStore: commit patch
TrustedRuntimeHost->>WAL: record program-bound executable-operation tick
WAL-->>TrustedRuntimeHost: validate canonical recovery shape
TrustedRuntimeHost-->>Caller: return committed operation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/warp-core/src/echo_operation.rs`:
- Around line 2391-2423: In the `store.node(&node.local_id)` `None` branch,
check `store.node_attachment(&node.local_id)` before constructing the create
operations. If an attachment already exists, return
`obstruction(EchoOperationObstructionKindV1::PreconditionMismatch)`; otherwise
preserve the existing budget charging, footprint updates, and atomic
`UpsertNode`/`SetAttachment` flow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fb8afe6e-f111-48ae-91d7-3415051c05f9
📒 Files selected for processing (7)
CHANGELOG.mdcrates/warp-core/src/echo_operation.rscrates/warp-core/src/lib.rscrates/warp-core/tests/executable_operation_pipeline_tests.rsdocs/adr/0024-anchored-node-creation-from-absence.mddocs/adr/README.mdtests/docs/test_adr_namespace.sh
![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: 3b61778cd7
ℹ️ 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-review (CodeRabbit-style pass)Reviewed Summary table
Verified clean (no finding)
Recommended mitigation prompts#1 (High): In #2a/#2b (Medium): Add two tests to #3 (Medium): Add a test constructing a create-from-absence invocation with #4 (Low, optional): Add a create-from-absence variant of @codex — requesting confirmation on finding #1 in particular (the result-identity claim in ADR 0024). Does the "no real committed data yet" framing in CHANGELOG.md fully cover this, or does the ADR need a precision edit before merge? |
Self-code-review (CodeRabbit-style pass, posted to PR #686) surfaced one high-severity and three medium-severity findings against 3b61778. All addressed: - operation_result_id's previous_value_digest now goes through hash_optional_id for both Some and None, which adds a discriminant tag byte the pre-existing Some path never had. This changes EchoOperationResultIdV1 (and everything chained from it) for the update path too, not only the new create path -- contradicting ADR 0024's "byte-for-byte unchanged" / "unchanged bit-for-bit" claims as originally written. Corrected the ADR's Verification Evidence Grade and Consequences sections to precisely scope what's actually unchanged (EchoOperationInvocationV1's canonical bytes and observable behavior) versus what changed but is judged acceptable (this one internal digest, for both branches, given no real committed result/receipt identity exists yet). - The new admission-time budget precheck in current_application_basis ("read_bytes < 64") turned out to be unreachable dead code on closer inspection while writing its test: admit_invocation_v1's existing minimum-budget gate already requires invocation.delegated_budget.read_bytes >= 64 before current_application_basis is ever called. Removed the dead guard rather than writing an untestable test for it, with a comment explaining why no local guard is needed. - Added three tests closing real coverage gaps: create-from-absence against a node that exists with the wrong NodeRecord.ty (NodeTypeMismatch, not a generic precondition failure); against a node that exists with the correct type but no attachment (PreconditionMismatch -- creation is atomic over both slots or it refuses); and a basis-changed TOCTOU race for a prepared create-from-absence patch, proving the existing exact-basis commit protection is generic and not update-specific. New fixture_host_with_bare_node helper backs the first two. 19 tests now pass in executable_operation_pipeline_tests.rs (was 16). Full warp-core suite (81 binaries, 686 library tests, doctests) still green; clippy identical pre-existing error set, independently verified against a clean origin/main worktree by exact file:line location.
Findings resolved (53fed09)
19 tests now pass in @codex — the dead-code removal (finding #3) is the one I'd most like a second pair of eyes on: confirming |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53fed09957
ℹ️ 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".
…very operation_patch_scope_v1 only recognized the single-SetAttachment-op update shape. A create-from-absence patch (WarpOp::UpsertNode then WarpOp::SetAttachment, two-element out_slots) never matched, so validate_operation_receipt_state_delta rejected every durable create-from-absence commit with EchoOperationExecutionMismatch -- not only on recovery by a fresh host, but synchronously at the original commit whenever the runtime WAL is enabled. Found by Codex's automated review on PR #686. Extended the function to recognize both canonical patch shapes explicitly, rather than adding a second unrelated function, since both are the same program's only two possible outputs. RED: filesystem_wal_recovers_a_create_from_absence_commit failed at commit_prepared_echo_operation_v1 with exactly this error before the fix. GREEN: same test now passes; full pipeline suite (20 tests) and existing update-path WAL recovery test both remain green.
…ment Two review findings, same root cause: GraphStore::set_node_attachment never requires the owning node to exist, so an attachment present with no NodeRecord (an orphan) is a real reachable state, not hypothetical. Neither admission nor evaluation independently checked for it when the node itself was absent. - current_application_basis (Codex, P2): derived "absent" from node presence, so it never inspected the attachment when the node was missing. A create-from-absence invocation dishonestly claiming absence against a real orphan was admitted. Fixed by deriving absence from the attachment slot alone -- the only thing this primitive actually mutates -- so an orphan corroborates as present, using its real value, exactly like the ordinary case. - prepare_operation_v1's create branch (CodeRabbit, Major): built the atomic create ops without checking whether the attachment slot was already occupied, since node absence doesn't imply attachment absence. A caller with a basis honestly reflecting an orphan's real present value (passing the now-fixed admission check) could still reach evaluation and silently overwrite it. Fixed by checking store.node_attachment(&node.local_id).is_some() before proceeding, refusing with PreconditionMismatch -- creation is atomic over both slots or it refuses, per ADR 0024. New fixture_host_with_orphan_attachment helper backs two tests, one per layer: admission refuses a dishonest absence claim with BasisMismatch; evaluation refuses (and leaves the orphan untouched) when the claimed basis honestly reflects the orphan's real value, isolating the evaluation-time check specifically. The two fixes had to land in that order -- before the admission fix, the evaluation test's premise (an honest "present" basis matching what admission computes) wasn't reachable, since old admission unconditionally computed "absent" whenever the node was missing. 22 tests pass (was 20); full pipeline suite green.
The create-from-absence write charge counted only 32 (attachment type id) + replacement_len, but the same patch also writes WarpOp::UpsertNode's 32-byte NodeRecord.ty. An invocation delegated only enough write budget for the attachment could commit 32 more bytes than declared, and the receipt's consumed_budget underreported actual resource consumption. Found by Codex's automated review on PR #686. Changed the charge to 64 (node type + attachment type) + replacement_len; the update path's 32 + replacement_len is untouched, since that branch never writes a node record. RED: create_from_absence_charges_the_node_record_against_the_write_budget delegated a write budget between the old (39) and correct (71) requirement for a 7-byte replacement and expected BudgetExceeded; the buggy charge let it commit instead. GREEN: same test passes; updated create_from_absence_commits_one_new_node_and_attachment_patch's consumed-budget assertion (39 -> 71) to match the corrected charge. 23 tests pass (was 22); full pipeline suite green.
…Mismatch record.ty was checked before expects_creation in the Some(record) arm, so create-from-absence against an existing node with the wrong type returned NodeTypeMismatch instead of PreconditionMismatch -- contradicting ADR 0024's own stated contract that every occupied create target refuses uniformly as a precondition failure, regardless of what specifically differs. Found by Codex's automated review on PR #686. Moved the expects_creation check to the top of the Some(record) arm, before the type/attachment checks that exist to serve the update path. This also simplified the code: the now-unreachable expects_creation branches inside the attachment-lookup ternary and the redundant standalone check after it are gone. Updated create_from_absence_refuses_when_the_node_exists_with_the_wrong_type to expect PreconditionMismatch. This left NodeTypeMismatch completely untested, so added update_precondition_refuses_when_the_node_has_the_wrong_type proving it's still correctly reachable on the update path, where a caller who correctly expected the node to exist named the wrong type. 24 tests pass (was 23); full pipeline suite green.
operation_result_id routed previous_value_digest through hash_optional_id, which prepends a discriminant tag for both Some and None. This silently changed EchoOperationResultIdV1 (and everything chained from it -- preparation_id, receipt identities) for the update path too, not only the new create path, contradicting this branch's own "byte-for-byte unchanged" claims. A prior commit corrected the ADR's wording to document this honestly rather than overclaim; this commit fixes it for real instead, per Codex's automated review on PR #686 and my own self-review's finding #1. Some(digest) now hashes exactly digest -- the same raw 32 bytes the pre-ADR-0024 code always wrote, no tag -- so the update path's result identity is truly unchanged, not merely behaviorally equivalent. None uses a new domain-separated marker (RESULT_ID_ABSENT_PREVIOUS_VALUE_DOMAIN) whose byte length differs from every possible 32-byte digest, so it cannot collide with Some(digest) for any digest value -- unlike current_application_basis's Option<Hash> field (which needed hash_optional_id's shared tag scheme because that field's encoding is otherwise fixed-width), this field's own two encodings already differ in length, so no tag is needed here. RED: operation_result_id_preserves_the_legacy_hash_for_updates (new unit test in echo_operation.rs's own mod tests) asserted Some(...) matches a hand-reconstructed reference hash and failed against the tagged encoding. GREEN: same test passes after the fix. Updated ADR 0024's Verification Evidence Grade and Consequences sections to state the digests are unchanged, not merely judged acceptable to differ. Full warp-core suite (81 binaries, 687 library tests, doctests) green; clippy identical pre-existing error set.
Code Lawyer: Activity SummaryAll 5 review-thread findings addressed, RED->GREEN->COMMIT per issue, one commit each.
What each fix actually proved wrong, briefly
Net test delta16 -> 24 tests in All 6 review threads resolved. @codex, @coderabbitai -- re-review requested. |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 387b7bffd7
ℹ️ 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
Code Lawyer self-audit findingsThe current head fixes the previously reported compatibility, occupancy, budget, obstruction, identity, and restart-recovery defects. A fresh
I will repair these one at a time with separate RED → GREEN → VERIFY → commit cycles, then resolve only the older threads whose claims are demonstrably satisfied by the current head. @codex second opinion requested on S1 in particular: recovery must bind more than the two-operation silhouette. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbab1b74f0
ℹ️ 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 Activity SummaryAudited
Validation
Scope judgmentThis PR now establishes an exact, durable single anchored node plus alpha attachment creation primitive. It does not claim real Edict compiler emission, scheduler/Action batch integration, atomic multi-record Graft creation, or completion of the Graft migration corridor. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/architecture/application-contract-hosting.md`:
- Around line 151-156: Narrow the recovery-validation claim in the architecture
documentation to match the checks currently enforced, removing assertions that
WAL recovery validates exact node/attachment types or payload-size limits.
Retain only the documented guarantees that are actually implemented until the S1
recovery validation gap is fixed.
In `@docs/topics/GeneratedRules.md`:
- Around line 183-187: Update the wording in the documented self-validation
scope to exclude Graft-style or general multi-record mutation and scheduler
batching, while acknowledging that the create-if-absent program intentionally
writes one node and one alpha attachment. Preserve the surrounding claims about
deterministic validation and avoid excluding multi-record creation itself.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 444d1027-09e1-4f1b-8c8d-903d7ca8d44f
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdcrates/warp-core/src/echo_operation.rscrates/warp-core/src/lib.rscrates/warp-core/src/trusted_runtime_host.rscrates/warp-core/tests/executable_operation_pipeline_tests.rsdocs/adr/0024-anchored-node-creation-from-absence.mddocs/adr/README.mddocs/architecture/application-contract-hosting.mddocs/topics/GeneratedRules.md
1 participant
Summary
EchoOperationProgramV1::AnchoredNodeAttachmentCompareAndSetupdate-only and preserves its canonical program bytes, invocation bytes, result identity calculation, schemas, footprint contract, and target profile.EchoOperationProgramV1::AnchoredNodeAttachmentCreateIfAbsentas a distinct executable program with its own program kind, footprint contract, invocation/input/result/application-basis schemas, target profile, result identity domain, and canonical two-operation recovery shape.PreconditionMismatch, regardless of the occupant's type.64 + payload length), while update retains its existing32 + payload lengthwrite charge. The step counter measures semantic evaluator stages, not emittedWarpOpcount.UpsertNode+SetAttachmentpatch with node + attachment outputs; update accepts onlySetAttachmentwith the attachment output.This closes the single anchored-node-plus-alpha-attachment creation gap in the executable-operation corridor. It does not establish a real Edict compiler crossing, a structurally separate application verifier, or atomic execution of Graft's
recordGitWarpImportBatch, whose declared footprint creates bothGitWarpImportBatchandStructuralBasis.See
docs/adr/0024-anchored-node-creation-from-absence.mdfor the complete compatibility, identity, budget, recovery, and scope decision.Closes #685
Test plan
PreconditionMismatch.64 + payload lengthwrite boundary and refuses one byte below it; update retains its separate32 + payload lengthboundary.warp-coresuite passes withnative_rule_bootstrap,trusted_runtime,host_test: 689 library tests, all integration test binaries, and 33 doctests.cargo fmt --all --check, Markdown lint/Prettier,git diff --check, andtests/docs/test_adr_namespace.shpass.warp-core --libClippy pre-commit gate passes.Summary by CodeRabbit