feat(core): advance v0.1 contract evidence and retention#371
Open
flyingrobots wants to merge 7 commits into
Open
feat(core): advance v0.1 contract evidence and retention#371flyingrobots wants to merge 7 commits into
flyingrobots wants to merge 7 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements semantic retention indexing above ChangesSemantic Retention, Contract Evidence, and Reading Identity
Sequence Diagram(s)sequenceDiagram
participant Test
participant Coordinator
participant Engine
participant ObsService
participant RetIndex
Test->>Coordinator: ingest_ticketed(mutation)
Coordinator->>Coordinator: dispatch via scheduler
Test->>ObsService: observation_request(query)
ObsService->>Engine: contract_query_observer_package_evidence(query_id)
Engine-->>ObsService: Some(contract_evidence)
ObsService->>ObsService: execute query observer
ObsService->>ObsService: compute query_reading_identity
ObsService-->>Test: ReadingEnvelope { contract, query_identity, ... }
Test->>RetIndex: retain(semantic_coord_reading, payload)
Test->>RetIndex: retain(semantic_coord_receipt, correlation)
Test->>RetIndex: load(semantic_coord_reading)
Test->>RetIndex: load(semantic_coord_receipt)
Test->>Coordinator: replay witnessed submissions
Coordinator-->>Test: same observed intent outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 94018001f9
ℹ️ 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".
Owner
Author
|
@codex Self-review findings for PR #371. Please confirm the severity/mitigation before merge.
Validation run during review: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94018001f9
ℹ️ 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".
Owner
Author
|
Self-review fixes addressed in commit
Verification run locally:
✅ Addressed in commit |
Owner
Author
|
@codex Second self-review findings for PR #371. Please confirm before merge.
No files changed during this review. Validation run during review: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 021dc592a6
ℹ️ 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".
Owner
Author
|
@codex Follow-up self-review fixes are pushed.
Verification run locally:
|
Summary
Testing
|
Owner
Author
|
Code Lawyer pass complete. No new branch-caused self-audit issues were found. All previously unresolved review threads are resolved against pushed commits.
Verification evidence remains the pushed local run set from |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/src/coordinator.rs (1)
1497-1508:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject same-id replays when
contractdiffers.This duplicate path is now too weak for the new record shape.
ticketed_ingress_iddoes not includecontract, and the duplicate check never compares it, so the first staged value wins silently. A submission first staged withNoneand later staged throughingest_installed_contract_invocation()will keep the wrong record and propagate missing/stale contract evidence intoReceiptCorrelationRecord.Suggested fix
if let Some(existing_id) = self .ticketed_runtime_ingress_by_submission .get(&submission_id) .copied() { let Some(record) = self.ticketed_runtime_ingress.get(&existing_id).cloned() else { return Err(RuntimeError::TicketedIngressAlreadyStaged(submission_id)); }; if existing_id == ticketed_ingress_id { + if record.contract != contract { + return Err(RuntimeError::TicketedIngressAlreadyStaged(submission_id)); + } return Ok(TicketedRuntimeIngressDisposition::Duplicate { record }); } return Err(RuntimeError::TicketedIngressAlreadyStaged(submission_id)); }🤖 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 `@crates/warp-core/src/coordinator.rs` around lines 1497 - 1508, The current duplicate-path only compares IDs (existing_id vs ticketed_ingress_id) and so allows replayed submissions with the same id but different contract to silently win; update the logic in the block that looks up self.ticketed_runtime_ingress_by_submission / self.ticketed_runtime_ingress to also compare the existing record's contract with the incoming record's contract (the new record created by ingest_installed_contract_invocation path) and treat any mismatch as a conflict by returning Err(RuntimeError::TicketedIngressAlreadyStaged(submission_id)); specifically, when you fetch record (bound to record) and before returning TicketedRuntimeIngressDisposition::Duplicate { record }, check record.contract (or the appropriate contract field on the incoming record) and if they differ, return the same error instead of Ok::Duplicate so stale/missing contract evidence cannot be propagated into ReceiptCorrelationRecord.
🤖 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/echo-cas/src/retention.rs`:
- Around line 148-158: The current retain path always calls store.put(bytes)
even when self.descriptors already maps coordinate to the same content, causing
redundant hashing/writes; modify the block handling if let Some(existing) =
self.descriptors.get(&coordinate) so that when existing.content_hash ==
content_hash and existing.byte_len == bytes.len() as u64 you skip store.put and
only call store.pin(&content_hash) and return Ok(existing.clone()), and keep the
SemanticCoordinateConflict error branch unchanged for mismatches; locate and
update the code around self.descriptors, existing, content_hash, bytes,
store.put, store.pin to short-circuit the idempotent case.
In `@crates/warp-core/src/observation.rs`:
- Around line 2103-2110: The current basis_digest computation encodes full ABIs
from resolved.to_abi() and parent_basis_posture.to_abi(), which include
freshness like observed_after_global_tick and breaks stable-question identity;
instead serialize and hash only the causal-basis fields (exclude
observed_after_global_tick/freshness) before push_len_prefixed. Modify the code
around resolved.to_abi() and parent_basis_posture.to_abi() so you either call a
helper that returns a causal-only ABI (e.g., to_causal_abi or
strip_freshness_from_abi) or clone and clear observed_after_global_tick from the
structs, then encode_cbor those causal-only objects and feed them to the hasher
with QUERY_READING_BASIS_DOMAIN to produce the basis_digest.
---
Outside diff comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 1497-1508: The current duplicate-path only compares IDs
(existing_id vs ticketed_ingress_id) and so allows replayed submissions with the
same id but different contract to silently win; update the logic in the block
that looks up self.ticketed_runtime_ingress_by_submission /
self.ticketed_runtime_ingress to also compare the existing record's contract
with the incoming record's contract (the new record created by
ingest_installed_contract_invocation path) and treat any mismatch as a conflict
by returning Err(RuntimeError::TicketedIngressAlreadyStaged(submission_id));
specifically, when you fetch record (bound to record) and before returning
TicketedRuntimeIngressDisposition::Duplicate { record }, check record.contract
(or the appropriate contract field on the incoming record) and if they differ,
return the same error instead of Ok::Duplicate so stale/missing contract
evidence cannot be propagated into ReceiptCorrelationRecord.
🪄 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
Run ID: ffef2b57-94ca-4c7d-b628-93e446ff0373
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
CHANGELOG.mdcrates/echo-cas/src/lib.rscrates/echo-cas/src/retention.rscrates/echo-cas/tests/semantic_retention.rscrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/echo-wesley-gen/tests/generation.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/contract_registry.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/lib.rscrates/warp-core/src/observation.rscrates/warp-core/src/optic/tests.rscrates/warp-core/tests/installed_contract_intent_pipeline_tests.rscrates/warp-core/tests/installed_contract_registry_tests.rscrates/warp-wasm/README.mdcrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/BEARING.mddocs/design/v0.1.0-contract-artifact-retention-in-echo-cas.mddocs/design/v0.1.0-contract-aware-receipts-and-readings.mddocs/design/v0.1.0-contract-reading-identity-and-bounded-payloads.mddocs/design/v0.1.0-contract-retention-and-semantic-lookup-seams.mddocs/design/v0.1.0-external-contract-proof-fixture.mddocs/design/v0.1.0-release-plan.mddocs/method/backlog/v0.1.0/KERNEL_contract-aware-receipts-and-readings.mddocs/method/backlog/v0.1.0/KERNEL_contract-reading-identity-and-bounded-payloads.mddocs/method/backlog/v0.1.0/PLATFORM_contract-artifact-retention-in-echo-cas.mddocs/method/backlog/v0.1.0/PLATFORM_contract-retention-and-semantic-lookup-seams.mddocs/method/backlog/v0.1.0/PLATFORM_external-contract-proof-fixture.mddocs/spec/SPEC-0009-wasm-abi-v3.md
Comment on lines
+148
to
+158
| if let Some(existing) = self.descriptors.get(&coordinate) { | ||
| if existing.content_hash != content_hash || existing.byte_len != bytes.len() as u64 { | ||
| return Err(RetentionError::SemanticCoordinateConflict { | ||
| coordinate: Box::new(coordinate), | ||
| existing_content_hash: existing.content_hash, | ||
| new_content_hash: content_hash, | ||
| }); | ||
| } | ||
| store.put(bytes); | ||
| store.pin(&content_hash); | ||
| return Ok(existing.clone()); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid unconditional put on idempotent retain path.
When the coordinate already maps to the same content, Line 156 still recomputes/stores bytes. This creates avoidable hashing/write work on repeated retains.
Suggested patch
if let Some(existing) = self.descriptors.get(&coordinate) {
if existing.content_hash != content_hash || existing.byte_len != bytes.len() as u64 {
return Err(RetentionError::SemanticCoordinateConflict {
coordinate: Box::new(coordinate),
existing_content_hash: existing.content_hash,
new_content_hash: content_hash,
});
}
- store.put(bytes);
+ if !store.has(&existing.content_hash) {
+ let restored_hash = store.put(bytes);
+ debug_assert_eq!(restored_hash, existing.content_hash);
+ }
store.pin(&content_hash);
return Ok(existing.clone());
}🤖 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 `@crates/echo-cas/src/retention.rs` around lines 148 - 158, The current retain
path always calls store.put(bytes) even when self.descriptors already maps
coordinate to the same content, causing redundant hashing/writes; modify the
block handling if let Some(existing) = self.descriptors.get(&coordinate) so that
when existing.content_hash == content_hash and existing.byte_len == bytes.len()
as u64 you skip store.put and only call store.pin(&content_hash) and return
Ok(existing.clone()), and keep the SemanticCoordinateConflict error branch
unchanged for mismatches; locate and update the code around self.descriptors,
existing, content_hash, bytes, store.put, store.pin to short-circuit the
idempotent case.
Comment on lines
+2103
to
+2110
| let resolved_bytes = echo_wasm_abi::encode_cbor(&resolved.to_abi()) | ||
| .map_err(|err| ObservationError::CodecFailure(err.to_string()))?; | ||
| let posture_bytes = echo_wasm_abi::encode_cbor(&parent_basis_posture.to_abi()) | ||
| .map_err(|err| ObservationError::CodecFailure(err.to_string()))?; | ||
| let mut hasher = Hasher::new(); | ||
| hasher.update(QUERY_READING_BASIS_DOMAIN); | ||
| push_len_prefixed(&mut hasher, &resolved_bytes); | ||
| push_len_prefixed(&mut hasher, &posture_bytes); |
Contributor
There was a problem hiding this comment.
Exclude freshness metadata from basis_digest.
resolved.to_abi() includes observed_after_global_tick, so the same QueryView against the same resolved commit can produce a different query_identity.reading_id after unrelated runtime progress. That breaks the stable-question identity contract. Hash only causal basis fields here, and add a regression that advances the runtime global tick without changing the observed commit.
As per coding guidelines, crates/warp-core/** is the deterministic kernel and every non-determinism risk must be flagged.
🤖 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 `@crates/warp-core/src/observation.rs` around lines 2103 - 2110, The current
basis_digest computation encodes full ABIs from resolved.to_abi() and
parent_basis_posture.to_abi(), which include freshness like
observed_after_global_tick and breaks stable-question identity; instead
serialize and hash only the causal-basis fields (exclude
observed_after_global_tick/freshness) before push_len_prefixed. Modify the code
around resolved.to_abi() and parent_basis_posture.to_abi() so you either call a
helper that returns a causal-only ABI (e.g., to_causal_abi or
strip_freshness_from_abi) or clone and clear observed_after_global_tick from the
structs, then encode_cbor those causal-only objects and feed them to the hasher
with QUERY_READING_BASIS_DOMAIN to produce the basis_digest.
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
This PR completes the next five v0.1.0 contract-host slices:
echo-casChanges
ContractEvidenceIdentityto installed QueryViewReadingEnvelopes and installed mutation receipt correlations.QueryReadingIdentityfor QueryView readings, binding query id, vars digest, resolved basis digest, aperture digest, observer plan, and installed contract evidence when present.echo-cassemantic retention primitives above content-only CAS:SemanticBlobCoordinate,RetainedBlobIndex, descriptors, typed retention errors, and bounded range lookup.Authority boundaries preserved
Verification
cargo test -p echo-wasm-abicargo test -p echo-cascargo test -p warp-core --features native_rule_bootstrap --test installed_contract_registry_tests installed_contract_package_binds_supported_mutation_and_querycargo test -p warp-core --features native_rule_bootstrap,host_test --test installed_contract_intent_pipeline_testscargo clippy -p warp-core --features native_rule_bootstrap,host_test --test installed_contract_intent_pipeline_testscargo fmt --checkgit diff --checkSummary by CodeRabbit
New Features
Updates