feat(core): add runtime support v0 boundary#354
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces a RuntimeSupport v0 boundary for optic invocation admission in warp-core: after basis/aperture/budget resolution the registry requires Echo-owned runtime-support facts keyed by artifact requirements digest, publishes RuntimeSupportRecorded facts when recorded, and maps missing vs recorded support to RuntimeSupportUnavailable or InvocationAdmissionUnavailable obstructions. ChangesRuntimeSupport v0 Boundary Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 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 |
|
@codex please confirm these self-review findings before merge.
|
![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: 19895005bf
ℹ️ 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".
|
Resolved the self-review findings.
Validation:
|
![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/optic_artifact.rs`:
- Around line 1097-1128: The current flow validates handles in
record_runtime_support_v0_fixture_for_artifact then unconditionally calls
record_runtime_support_v0_fixture_for_requirements_digest which inserts into
runtime_support_v0_by_requirements and always calls
publish_runtime_support_recorded_fact, causing duplicate RuntimeSupportRecorded
facts when multiple artifacts share a requirements_digest; to fix, make
record_runtime_support_v0_fixture_for_requirements_digest check whether
runtime_support_v0_by_requirements already contains the requirements_digest (or
the same support_digest) and only call publish_runtime_support_recorded_fact
when inserting a new mapping, leaving
record_runtime_support_v0_fixture_for_artifact and
runtime_support_v0_fixture_digest unchanged.
🪄 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: ffaf3ad5-f37f-4371-9d1c-8a6cb4616017
📒 Files selected for processing (6)
CHANGELOG.mdcrates/warp-core/src/causal_facts.rscrates/warp-core/src/optic_artifact.rscrates/warp-core/tests/causal_fact_publication_tests.rscrates/warp-core/tests/optic_invocation_admission_tests.rsdocs/design/optic-admission-ladder-checkpoint.md
|
@codex please confirm this remaining self-review finding before merge.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e87d891a4b
ℹ️ 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".
|
Resolved the remaining self-review finding.
Validation:
|
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 `@crates/warp-core/src/optic_artifact.rs`:
- Around line 1299-1315: The equality branch in
resolve_runtime_support_v0_for_requirements is unreachable because
record_runtime_support_v0_fixture_for_requirements_digest only ever stores
runtime_support_v0_fixture_digest(); fix by simplifying the logic in
resolve_runtime_support_v0_for_requirements: if
self.runtime_support_v0_by_requirements.get(requirements_digest) returns
Some(_), return None immediately (remove the ==
runtime_support_v0_fixture_digest() check and the unreachable
RuntimeSupportUnavailable branch), or alternatively keep the check but add a
clear comment above the comparison explaining it is defensive for future
multi-version support and reference
record_runtime_support_v0_fixture_for_requirements_digest so reviewers
understand intent.
In `@crates/warp-core/tests/optic_artifact_registry_tests.rs`:
- Around line 148-151: The test currently only ensures no
GraphFact::RuntimeSupportRecorded was published; instead assert that
registry.published_graph_facts() is entirely empty to block any fact emission on
failed registration—replace the existing assertion using
published_graph_facts().iter().all(...) with a direct emptiness check (e.g.,
assert!(registry.published_graph_facts().is_empty())) so no other fact types can
be emitted; reference the registry variable and its published_graph_facts()
method and remove the GraphFact::RuntimeSupportRecorded pattern-based check.
🪄 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: 155f99ba-240c-442c-a67d-ed7f3976288b
📒 Files selected for processing (4)
CHANGELOG.mdcrates/warp-core/src/optic_artifact.rscrates/warp-core/tests/optic_artifact_registry_tests.rsdocs/design/optic-admission-ladder-checkpoint.md
| assert!(registry | ||
| .published_graph_facts() | ||
| .iter() | ||
| .all(|published| !matches!(published.fact, GraphFact::RuntimeSupportRecorded { .. }))); |
There was a problem hiding this comment.
Strengthen side-effect assertion to block any fact publication on failed registration.
This currently only forbids GraphFact::RuntimeSupportRecorded. If another fact type is accidentally emitted on failure, the test still passes. Assert the publication log is empty to enforce the invariant.
Suggested diff
- assert!(registry
- .published_graph_facts()
- .iter()
- .all(|published| !matches!(published.fact, GraphFact::RuntimeSupportRecorded { .. })));
+ assert!(registry.published_graph_facts().is_empty());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert!(registry | |
| .published_graph_facts() | |
| .iter() | |
| .all(|published| !matches!(published.fact, GraphFact::RuntimeSupportRecorded { .. }))); | |
| assert!(registry.published_graph_facts().is_empty()); |
🤖 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/tests/optic_artifact_registry_tests.rs` around lines 148 -
151, The test currently only ensures no GraphFact::RuntimeSupportRecorded was
published; instead assert that registry.published_graph_facts() is entirely
empty to block any fact emission on failed registration—replace the existing
assertion using published_graph_facts().iter().all(...) with a direct emptiness
check (e.g., assert!(registry.published_graph_facts().is_empty())) so no other
fact types can be emitted; reference the registry variable and its
published_graph_facts() method and remove the GraphFact::RuntimeSupportRecorded
pattern-based check.
|
@coderabbitai Thanks! This appears to be a misalignment with the refusal-evidence invariant. Failed artifact registration is intentionally allowed to publish |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
@coderabbitai review please 🥕 |
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26253 🏁 Script executed: Length of output: 18433
What's correctAuthority boundary (P2 — resolved) Digest invariant (P2 — resolved) The stored requirements payload and the registered label are bound at registration time before any handle is issued, so no Admission ladder (correct)
Minor observations (non-blocking)1. fn resolve_runtime_support_v0_for_requirements(&self, requirements_digest: &str)
-> Option<OpticInvocationObstruction>
{
if self.runtime_support_v0_by_requirements.contains_key(requirements_digest) {
return None; // stored digest value not checked
}
Some(OpticInvocationObstruction::RuntimeSupportUnavailable)
}Safe today — there is exactly one write path and it always writes 2. Both 3. Handle index saturation
All correctness requirements are met. The two P2 findings and the P4 docs finding are properly closed. The test coverage (RED/GREEN regressions for unknown handle, mismatched inner digest, admission ordering, caller testimony exclusion) is sufficient for this slice. [approve] |
1 participant
Summary
InvocationAdmissionUnavailableas the terminal obstruction after runtime support resolvesTests
cargo test -p warp-core --test causal_fact_publication_testscargo test -p warp-core --test optic_invocation_admission_testscargo test -p warp-corecargo checkcargo fmt --checkgit diff --checkpnpm exec markdownlint-cli2 CHANGELOG.md docs/design/optic-admission-ladder-checkpoint.mdSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation