Publish canonical provider contract pack - #162
Conversation
WalkthroughThe change introduces a deterministic provider contract pack with CDDL assembly, root and resource validation, digest-bound manifests, checked fixtures, Target IR schema coverage, xtask drift detection, and supporting requirements and documentation. ChangesProvider Contract Pack
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Xtask
participant ContractPack
participant Fixtures
Xtask->>ContractPack: Read CDDL and contract resources
ContractPack->>ContractPack: Assemble and validate schema pack
ContractPack->>Fixtures: Render CDDL and manifest
Xtask->>Fixtures: Check or write golden artifacts
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Code Lawyer: self-audit findings
Cc: @codex for second opinion. |
![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: 747fca795a
ℹ️ 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 Summary
Verification at final head
No additional concrete logic, authority-boundary, determinism, dependency, documentation, or style findings remain from the two independent Code Lawyer audits. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/edict-provider-schema/src/contract_pack.rs`:
- Around line 35-60: Make the ordering and uniqueness invariant explicit in
build_manifest before CONTRACT_BINDINGS and DOMAIN_BINDINGS are copied into the
manifest, using the existing assert_sorted_unique helper or an equivalent
validation. Validate each table by its lookup key (contract and domain
respectively), and fail immediately if either table is unsorted or contains
duplicates so validate_contract, validate_domain, and supports_domain cannot
binary-search invalid data.
- Around line 560-637: Update unresolved_control_reference to handle
Control::Cbor explicitly by traversing its contained node through
unresolved_node_reference, while preserving the existing handling for size and
comparison controls and the unsupported fallback for other variants.
🪄 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: 013a247e-2d32-4e8a-a95b-d0f062bc0e4e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
CHANGELOG.mdcrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/src/lib.rscrates/edict-provider-schema/tests/provider_contract_pack.rsdocs/REQUIREMENTS.mddocs/abi/edict-target-ir.cddldocs/topics/fixtures/README.mddocs/topics/fixtures/test-plan.mddocs/topics/providers/README.mddocs/topics/providers/test-plan.mddocs/topics/target-ir/README.mddocs/topics/target-ir/test-plan.mdfixtures/README.mdfixtures/provider-contracts/v1/README.mdfixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.jsonfixtures/providers/components/inventory.jsonxtask/Cargo.tomlxtask/src/goldens.rsxtask/src/main.rsxtask/src/provider_contract_pack.rsxtask/src/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: rust msrv 1.94.0 (fmt · clippy · test)
- GitHub Check: supply-chain (cargo-deny)
- GitHub Check: rust stable (fmt · clippy · test)
🧰 Additional context used
📓 Path-based instructions (2)
docs/topics/**/{README.md,test-plan.md,architecture.md,rationale.md}
📄 CodeRabbit inference engine (AGENTS.md)
docs/topics/**/{README.md,test-plan.md,architecture.md,rationale.md}: For every nontrivial behavior, contract, workflow, release, schema, validation, or public-surface change, identify the owning topic shelf before editing code; create one if none exists; updatetest-plan.mdbefore or alongside tests with requirement IDs, case IDs, fixtures, and oracles; write executable evidence; update the topicREADME.mdonly after behavior exists; mark planned cases implemented only when executable evidence exists; and runcargo xtask verifybefore claiming the shelf is current.
Do not churn topic shelves for purely mechanical edits that do not change a contract; when a change intentionally does not update a topic shelf, state why in the pull request body or final report.
TopicREADME.mdfiles must not describe intended behavior before it lands;test-plan.mdmay include planned cases and known gaps;policyrows are for human-review workflow contracts and must not be used to avoid writing behavior tests; tests must assert code behavior and stable contract artifacts, not prose; negative tests should assert stable error kinds or structured artifacts, not merelyis_err()or diagnostic text; release, CI, and publication workflows count as behavior when they define a project contract; avoid ceremonial documentation and update shelves because the contract changed, not because a path changed.
Files:
docs/topics/target-ir/README.mddocs/topics/fixtures/test-plan.mddocs/topics/fixtures/README.mddocs/topics/providers/README.mddocs/topics/target-ir/test-plan.mddocs/topics/providers/test-plan.md
docs/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
When creating or changing documentation, give each page one primary reader job; keep user-facing task help separate from contributor architecture and evidence maps; use concrete, valid examples and show expected results when the result matters; put exact public facts in reference material and validate or generate them from authoritative sources when practical; and update affected documentation in the same change as behavior, schema, release, workflow, or public-surface changes, or state
docs-impact: nonewith a concise rationale.
Files:
docs/topics/target-ir/README.mddocs/topics/fixtures/test-plan.mddocs/topics/fixtures/README.mddocs/topics/providers/README.mddocs/REQUIREMENTS.mddocs/topics/target-ir/test-plan.mddocs/topics/providers/test-plan.md
🔇 Additional comments (40)
xtask/Cargo.toml (1)
14-14: LGTM!xtask/src/goldens.rs (1)
622-626: LGTM!Also applies to: 639-644
xtask/src/tests.rs (6)
6-17: LGTM!Also applies to: 29-31
83-97: LGTM!
174-235: LGTM!
259-327: LGTM!
237-253: 🎯 Functional CorrectnessNo MSRV issue here
usize::is_multiple_ofis available on the workspace MSRV, so this change is fine.> Likely an incorrect or invalid review comment.
146-171: 📐 Maintainability & Code QualityNo change needed —
edict.lowering-requirements/v1is the published domain string here, and this file already mirrors the same binding used elsewhere; there’s no named constant to swap in.> Likely an incorrect or invalid review comment.docs/topics/providers/test-plan.md (1)
74-74: LGTM!Also applies to: 89-90, 136-139
docs/topics/target-ir/README.md (1)
116-124: LGTM!docs/topics/target-ir/test-plan.md (1)
17-17: LGTM!Also applies to: 53-53, 65-65, 99-99
xtask/src/main.rs (1)
7-7: LGTM!Also applies to: 26-26, 119-119, 131-131, 171-184, 194-194
xtask/src/provider_contract_pack.rs (2)
1-45: LGTM!Also applies to: 50-74
46-48: 🚀 Performance & ScalabilityRemove the
serde::de::IgnoredAnysuggestionxtaskhas no directserdedependency, and the workspace doesn’t provide one here, so that replacement won’t compile as written.> Likely an incorrect or invalid review comment.CHANGELOG.md (1)
13-21: LGTM!docs/REQUIREMENTS.md (1)
80-80: LGTM!docs/topics/fixtures/README.md (1)
30-33: LGTM!Also applies to: 53-57, 75-78
docs/topics/fixtures/test-plan.md (1)
32-32: LGTM!Also applies to: 50-51, 63-63
docs/topics/providers/README.md (1)
69-86: LGTM!crates/edict-provider-schema/src/contract_pack.rs (5)
1-34: LGTM!
62-277: LGTM!
279-408: LGTM!
410-558: LGTM!
639-819: LGTM!crates/edict-provider-schema/src/lib.rs (1)
24-36: LGTM!fixtures/README.md (1)
19-19: LGTM!Also applies to: 47-48
crates/edict-provider-schema/tests/provider_contract_pack.rs (3)
1-127: LGTM!
129-295: LGTM!
297-731: LGTM!fixtures/providers/components/inventory.json (1)
10-10: 🗄️ Data Integrity & Integration
sourceDigestmoves withCargo.lockandxtask/Cargo.toml, both of which changed here, so this bump is expected.docs/abi/edict-target-ir.cddl (4)
1-19: LGTM!
21-27: 🗄️ Data Integrity & IntegrationConfirm
target-ir-artifactintentionally omitsapiVersion.Every other top-level root in the assembled pack that isn't a bare aggregate (core-module, lawpack-manifest, target-profile-manifest, authority-facts, lowering-requirements, intrinsics-document, operation-profiles-document) carries an
apiVersionstring;target-ir-artifactinstead only haskind: "targetIrArtifact"with no version marker, anddomain/sourceCoreCoordinateare unconstrainedtstr. Given the header's stated goal — matching the canonical Target IR encoder's actual emitted value rather than restating types — this is plausibly intentional (the encoder simply doesn't emit an apiVersion field), but worth confirming against the encoder implementation so a future encoder change doesn't silently drift from this schema.
29-42: LGTM!
44-52: 🗄️ Data Integrity & IntegrationClarify the
obstructionFailures/obstructionArmsrelationship.
target-ir-stepcarries both a flat list of failure idents (obstructionFailures) and a map of handled arms (obstructionArms). CDDL can't enforce any set relationship between the two (e.g. arms-keys ⊆ failures, or vice-versa), so a producer could emit an arm for an undeclared failure or declare a failure with no arm, and this schema would accept it. The header comment says the lowering/encoder contracts "separately enforce semantic identifier rules," which likely covers this, but it'd help future readers to have a one-line comment here (in the file's own establishedEDICT-*-001-style annotation convention) stating the intended invariant.fixtures/provider-contracts/v1/edict-provider-contracts.cddl (2)
1-4: LGTM!
741-793: 🗄️ Data Integrity & IntegrationByte-identity of the embedded Target IR fragment vs.
docs/abi/edict-target-ir.cddlneeds scripted confirmation.Manual line-by-line diff shows this section matching the source file exactly, but manual diffing 50+ lines of CDDL isn't a substitute for an exact byte comparison — a single stray whitespace/newline difference here would silently break
cargo xtask provider-contract-pack --checkfor anyone regenerating the pack. See consolidated verification comment.fixtures/provider-contracts/v1/manifest.json (3)
9-27: LGTM!
1-8: 🗄️ Data Integrity & IntegrationSchema digest claims need scripted recomputation, not eyeballing.
schema.bytesHex/schema.rawSha256are the load-bearing determinism claim for the whole pack. Digest hex lengths check out (64 chars), but actual SHA-256 correctness and exact byte-for-byte equivalence tofixtures/provider-contracts/v1/edict-provider-contracts.cddlcan't be confirmed by manual review. See consolidated verification comment.
28-79: 🗄️ Data Integrity & IntegrationResource digest claims (
rawSha256,domainFramedDigest) need scripted recomputation.Same concern as the schema digest: hex lengths are structurally valid, but
rawSha256must equal SHA-256 ofcanonicalBytesHex, anddomainFramedDigestcorrectness depends on the (not-shown-here) domain-framing routine incontract_pack.rs. See consolidated verification comment.fixtures/provider-contracts/v1/README.md (1)
1-32: LGTM!
|
@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". |
Code Lawyer: Final Activity SummaryThis supersedes the earlier interim summary.
Final-head evidence:
No unresolved self-audit, Codex, or CodeRabbit finding remains. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 796dd46859
ℹ️ 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".
| (!rules.contains_key(&rule.name) && !generic_parameters.contains(&rule.name)) | ||
| .then(|| SchemaClosureFailure::ExternalRule(rule.name.clone())) |
There was a problem hiding this comment.
When a fragment references a generic rule with the wrong number of arguments, e.g. foo<T> = T and a published root uses foo, this check treats the reference as resolved just because the rule name exists. cddl-cat rejects that later with GenericError, which validate_contract/validate_domain collapse into SchemaMismatch for otherwise valid values, so the assembly boundary can publish an unusable contract root instead of returning a schema failure.
Useful? React with 👍 / 👎.
| ("lawpack-exports", "lawpack-exports"), | ||
| ("lawpack-manifest", "lawpack-manifest"), |
There was a problem hiding this comment.
When this publishes lawpack-exports and lawpack-manifest as callable contract roots, every_published_root_validates_reference_and_rejects_mutation now provides executable instance-validation evidence for them, but the lawpacks shelf still marks LAWPACKS-REQ-005/TP-005 as a gap and lists CDDL manifest/export validation as deferred. That violates the repo's topic-shelf contract for public-surface changes and leaves HEAD documentation contradicting the shipped pack.
Useful? React with 👍 / 👎.
1 participant
Summary
Plain-English Walkthrough
TL;DR
Edict now publishes one canonical, Rust-neutral bundle of provider-facing contracts instead of requiring Echo or another runtime owner to reconstruct them from Edict's Rust crates. [claim:canonical-contract-pack, confidence:1.00] The checked artifact contains nine logical contract roots, six artifact-domain bindings, five digest-locked target-profile resources, and an Apache-2.0 license declaration.
This closes the portable-contract prerequisite for Echo's generated provider artifacts. It does not generate an Echo provider, load packages, or weaken the production host's schema authority boundary; those remain downstream work.
Walkthrough
Previously, the relevant CDDL fragments and canonical resources existed in Edict, but a foreign generator had no single checked artifact that assembled their exact bytes, roots, identities, and provenance. The new assembler accepts every source byte explicitly and performs no repository discovery, registry lookup, network access, or lazy loading. [claim:explicit-input-boundary, confidence:0.99]
The implemented flow is:
flowchart LR A[Edict ABI CDDL fragments] --> C[Pure contract-pack assembler] B[Canonical target-profile resources] --> C C --> D[Compile and close CDDL roots] C --> E[Validate resource bytes, digests, provenance] D --> F[Self-contained CDDL] E --> G[Exact JSON manifest] F --> H[xtask check or write] G --> H H --> I[Reviewed Rust-neutral fixtures]Caption: Canonical provider contract publication
The diagram separates semantic assembly from repository I/O. A runtime-owned generator can consume the resulting fixtures without importing Edict Rust types, while Edict retains ownership of the source contracts and regeneration gate.
Authority And Failure Boundaries
The generated manifest is transport data, not self-authenticating authority. A caller must compare it with an already assembled Edict authority pack; validation recomputes schema and resource hashes and rejects metadata, binding, ordering, byte, digest, or provenance disagreement with stable failure kinds. [claim:manifest-admission, confidence:1.00]
Generation-time validation through the publication pack remains separate from
ProviderArtifactSchemaRegistry. The pack contains trusted Edict schemas, including productive Core recursion; untrusted provider schemas must still cross the production registry's stricter manifest-authority and structural-safety checks. [claim:registry-separation, confidence:0.99]Closure And Lookup Invariants
The closure walker now rejects
.cborwith the distinct stableSchemaControlUnsupportedkind becausecddl-catdoes not expose the nested node needed to prove complete rule closure. [claim:closure-control-safety, confidence:1.00] This is conservative: the pack never claims self-containment for a graph it cannot inspect.Before manifest construction, the static contract and domain tables must be strictly sorted and unique by their binary-search keys. [claim:binding-order-guard, confidence:1.00] Invalid order or duplicate keys fail immediately instead of creating silent lookup failures.
Target IR Parity
The pack adds
target-ir-artifact, reusing Edict's existing Core schema vocabulary for expressions, predicates, budgets, local references, and obstruction data. [claim:target-ir-schema-parity, confidence:1.00] Its nonempty target-profile coordinate rule matches the canonical encoder even for an LF-only nonempty string; empty coordinates and malformed nested artifacts still reject.Review And Drift Workflow
cargo xtask provider-contract-pack --checkrebuilds both artifacts and fails without rewriting when either differs;--writeis the explicit regeneration action. [claim:drift-gate, confidence:1.00] Drift failures now name the exact supported regeneration command.Compatibility And Scope
No new third-party dependency was added. [claim:dependency-scope, confidence:1.00] The unpublished workspace
xtaskcrate now depends by path on the existing unpublishededict-provider-schemacrate. TheCargo.lockchange only updates that workspace edge, and the provider component inventory was regenerated because its source digest covers the lockfile; all five component byte digests remained unchanged.The published artifacts are Apache-2.0. This PR does not publish crates, implement provider resolution, or expose the publication pack as the provider host's untrusted schema-validator capability.
RED / GREEN
RED:
cargo test -p edict-provider-schema --test provider_contract_packinitially failed because the pack API and Target IR CDDL did not exist.cargo test -p edict-provider-schema --test provider_contract_pack target_ir_root_matches_reference_encoder -- --exactexposed that an empty target-profile coordinate was incorrectly admitted.cargo test -p edict-provider-schema --test provider_contract_pack target_ir_root_accepts_encoder_valid_line_feed_coordinate -- --exactfailed withSchemaMismatch, exposing the wildcard/LF parity bug.cargo test -p xtask tests::provider_contract_pack_check_rejects_drift_without_rewriting -- --exactfailed because the diagnostic named the unrelated generic*-goldenscommand family.cargo test -p edict-provider-schema --test provider_contract_pack assembly_rejects_uninspectable_schema_controls_explicitly -- --exactfailed before the unsupported-control kind and conservative closure path existed.cargo test -p edict-provider-schema contract_pack::tests::static_binding_guard_rejects_unsorted_and_duplicate_keys -- --exactfailed before the production binding guard existed.GREEN:
cargo test -p edict-provider-schema --test provider_contract_pack: 8 passed.cargo test -p edict-provider-schema contract_pack::tests::static_binding_guard_rejects_unsorted_and_duplicate_keys -- --exact: passed.cargo test -p xtask: 76 passed.cargo xtask provider-contract-pack --check: passed.cargo xtask verify: passed at796dd46. [claim:full-verification, confidence:1.00]Documentation Impact
Updated the provider, fixture, and Target IR topic shelves and test plans, the requirements registry, fixture reference, and changelog. The generated manifest documents exact hex transport, ordering, raw versus domain-framed identity, provenance, and licensing.
Unblocks flyingrobots/echo#652.
Appendix: Citations
claim:canonical-contract-packcrates/edict-provider-schema/src/contract_pack.rs#20@796dd46;contract_pack_is_self_contained_and_repeatableincrates/edict-provider-schema/tests/provider_contract_pack.rs#48@796dd46;fixtures/provider-contracts/v1/manifest.json#7@796dd46claim:explicit-input-boundarycrates/edict-provider-schema/src/contract_pack.rs#62@796dd46;crates/edict-provider-schema/src/contract_pack.rs#286@796dd46;xtask/src/provider_contract_pack.rs#26@796dd46claim:manifest-admissioncrates/edict-provider-schema/src/contract_pack.rs#430@796dd46;manifest_validation_recomputes_schema_and_resource_identityincrates/edict-provider-schema/tests/provider_contract_pack.rs@796dd46claim:registry-separationcrates/edict-provider-schema/src/contract_pack.rs#251@796dd46claim:closure-control-safetycrates/edict-provider-schema/src/contract_pack.rs#118@796dd46;crates/edict-provider-schema/src/contract_pack.rs#584@796dd46;assembly_rejects_uninspectable_schema_controls_explicitlyincrates/edict-provider-schema/tests/provider_contract_pack.rs#438@796dd46claim:binding-order-guardcrates/edict-provider-schema/src/contract_pack.rs#390@796dd46;static_binding_guard_rejects_unsorted_and_duplicate_keysincrates/edict-provider-schema/src/contract_pack.rs#847@796dd46claim:target-ir-schema-paritydocs/abi/edict-target-ir.cddl#14@796dd46;target_ir_root_matches_reference_encoderincrates/edict-provider-schema/tests/provider_contract_pack.rs#176@796dd46;target_ir_root_accepts_encoder_valid_line_feed_coordinateincrates/edict-provider-schema/tests/provider_contract_pack.rs#252@796dd46claim:drift-gatextask/src/provider_contract_pack.rs#50@796dd46;provider_contract_pack_check_rejects_drift_without_rewritinginxtask/src/tests.rs#259@796dd46claim:dependency-scopextask/Cargo.toml#13@796dd46;crates/edict-provider-schema/Cargo.toml#14@796dd46claim:full-verificationcargo xtask verifyat796dd46: observed pass;docs/topics/providers/test-plan.md#136@796dd46;docs/topics/target-ir/test-plan.md#99@796dd46Closes #161