Add Edict executable-operation prerequisites - #167
Conversation
WalkthroughThe compiler now supports explicit intent bases, fixed-width integers, and bounded bytes. Core and Target IR preserve these values, Target IR emits digest-locked semantic closures, canonical encoding validates their identity, and schemas, fixtures, tests, and diagnostics cover the new representations. ChangesBasis and semantic closure
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SourceModule
participant Compiler
participant CoreIR
participant TargetLowering
participant CanonicalEncoder
participant ContractAssembler
SourceModule->>Compiler: parse basis, integer widths, and bounded bytes
Compiler->>CoreIR: produce typed Core intent
CoreIR->>TargetLowering: lower intent and source Core
TargetLowering->>CanonicalEncoder: encode Target artifact and semantic closure
CanonicalEncoder-->>TargetLowering: canonical identity or validation error
TargetLowering->>ContractAssembler: provide Target artifact and closure
ContractAssembler->>CoreIR: recompute semantic closure from supplied Core
ContractAssembler-->>TargetLowering: accept or reject identity mismatch
Possibly related PRs
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 |
![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: bb27bfc31f
ℹ️ 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".
![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/edict-syntax/src/compiler.rs`:
- Around line 657-672: Update the basis-clause collection in the source
type-checking flow to detect and reject more than one IntentClause::Basis
instead of using find_map, which silently ignores duplicates. Preserve the
existing missing-basis UnsupportedSourceShape error and only proceed to
check_expr when exactly one basis clause is present.
🪄 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: 2457b18f-2bec-494b-ab30-c969873d02e3
📒 Files selected for processing (18)
crates/edict-cli/src/main.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-syntax/src/canonical.rscrates/edict-syntax/src/compiler.rscrates/edict-syntax/src/core_ir.rscrates/edict-syntax/src/lib.rscrates/edict-syntax/src/semantic.rscrates/edict-syntax/src/target_ir.rscrates/edict-syntax/tests/operation_prerequisites.rsdocs/abi/edict-core.cddldocs/abi/edict-target-ir.cddldocs/design/canonical-target-ir-v0.11.mddocs/topics/compiler-spine/test-plan.mddocs/topics/core-ir/test-plan.mddocs/topics/target-ir/test-plan.mdfixtures/lang/operations/explicit-basis-u64.edictfixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.json
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: rust stable (fmt · clippy · test)
- GitHub Check: rust msrv 1.94.0 (fmt · clippy · test)
- GitHub Check: supply-chain (cargo-deny)
🧰 Additional context used
📓 Path-based instructions (2)
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/design/canonical-target-ir-v0.11.mddocs/topics/core-ir/test-plan.mddocs/topics/compiler-spine/test-plan.mddocs/topics/target-ir/test-plan.md
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/core-ir/test-plan.mddocs/topics/compiler-spine/test-plan.mddocs/topics/target-ir/test-plan.md
🪛 LanguageTool
docs/design/canonical-target-ir-v0.11.md
[grammar] ~92-~92: Ensure spelling is correct
Context: ... carries an explicit basis or imports a lawpack. It binds the exact canonical Core dige...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (19)
crates/edict-syntax/src/core_ir.rs (1)
70-82: LGTM!Also applies to: 83-109, 243-245
crates/edict-syntax/src/compiler.rs (1)
14-21: LGTM!Also applies to: 164-164, 266-266, 395-395, 410-410, 553-555, 568-569, 615-635, 636-642, 683-683, 1517-1521, 1549-1573, 1767-1769, 1796-1827
crates/edict-syntax/src/semantic.rs (1)
147-154: LGTM!docs/abi/edict-core.cddl (1)
213-213: LGTM!fixtures/lang/operations/explicit-basis-u64.edict (1)
1-29: LGTM!docs/topics/compiler-spine/test-plan.md (1)
49-52: LGTM!Also applies to: 81-84
docs/topics/core-ir/test-plan.md (1)
53-54: LGTM!Also applies to: 101-102
crates/edict-provider-schema/tests/provider_contract_pack.rs (1)
14-19: LGTM!Also applies to: 40-41, 255-278, 587-603
docs/topics/target-ir/test-plan.md (1)
54-62: LGTM!Also applies to: 103-105
crates/edict-syntax/src/canonical.rs (1)
8-21: LGTM!Also applies to: 463-497, 499-528, 555-580, 770-792, 944-965
crates/edict-syntax/src/target_ir.rs (2)
10-14: LGTM!Also applies to: 157-157, 181-197, 242-245, 284-284, 460-460
304-315: 🗄️ Data Integrity & IntegrationKeep the strict lowercase digest gate here. Lawpack import lowering and canonical Target IR rendering both require
sha256:<64 lowercase hex>review strings, so this check matches the existing contract.docs/abi/edict-target-ir.cddl (1)
26-37: LGTM!docs/design/canonical-target-ir-v0.11.md (1)
54-106: LGTM!crates/edict-syntax/src/lib.rs (1)
184-187: LGTM!crates/edict-cli/src/main.rs (1)
1435-1435: LGTM!crates/edict-syntax/tests/operation_prerequisites.rs (1)
99-452: LGTM!fixtures/provider-contracts/v1/edict-provider-contracts.cddl (1)
300-300: LGTM!Also applies to: 768-779
fixtures/provider-contracts/v1/manifest.json (1)
6-7: 🗄️ Data Integrity & IntegrationNo issue here The fixture’s
bytesHexandrawSha256line up, and the payload already includes the newsemanticClosure/basisrules.> Likely an incorrect or invalid review comment.
|
@codex Please confirm these resolved findings against current head
Validation at the current head: |
|
@codex review Please review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a747a181db
ℹ️ 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".
|
@codex review Please review the exact current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbd64b7d58
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/edict-syntax/src/canonical.rs (1)
462-484: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject empty
sourceCoreCoordinatehere
target_ir_resource_ref_valuealready rejects empty resource coordinates, butsourceCoreCoordinateis still emitted raw. A caller can handencode_target_ir_artifact/digest_target_ir_artifactan externally constructed legacyTargetIrArtifactwith"", and it will still canonicalize/digest. Add the same non-empty guard here, or reject it at construction.🤖 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/edict-syntax/src/canonical.rs` around lines 462 - 484, Update target_ir_artifact_value to reject an empty artifact.source_core_coordinate before emitting the raw sourceCoreCoordinate field, returning the existing canonical validation error used for invalid coordinates. Preserve canonicalization for non-empty coordinates and ensure encode_target_ir_artifact and digest_target_ir_artifact cannot accept the empty value.crates/edict-syntax/src/compiler.rs (1)
550-566: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReserve built-in scalar names for type declarations
type_ref_shapestill givesI32/I64/U32/U64precedence overnamed_types, and the semantic pass only reserves those names for binders—not fortypedeclarations. A localtype U64 = ...can still be accepted but never resolved in type position. Reject the collision at declaration time, or make the reservation explicit and test it.🤖 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/edict-syntax/src/compiler.rs` around lines 550 - 566, Update type_ref_shape and the type-declaration validation to consistently reserve built-in scalar names (I32, I64, U32, and U64): reject declarations using those names before adding them to named_types, or otherwise make the reservation explicit so such declarations cannot be accepted yet shadowed during type resolution. Add coverage for each collision if the project’s existing tests support it.
🤖 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-cli/tests/jsonl_cli.rs`:
- Around line 1140-1151: Update the semantic-closure assertions in the test to
include diagnostic messages consistent with the nearby basis/field assertion,
specifically for sourceCore/coordinate and lawpacks/0/coordinate. Immediately
assert that lawpacks/0/digest preserves the expected sha256 digest declared by
the demo.write@1 fixture.
In `@crates/edict-syntax/tests/contract_bundle.rs`:
- Around line 644-697: Add contract-bundle tests for the two untested branches
of corroborate_target_ir_semantic_closure: create a closure-free Core with an
injected Some semantic_closure and assert TargetIrSourceMismatch for
target_ir_artifact.semantic_closure, then mutate the artifact closure’s lawpacks
while keeping the Core and bundle lawpacks unchanged and assert
TargetIrSourceMismatch for the artifact closure lawpacks field. Follow the
existing assembly_from_target_ir_input test style and error assertions.
In `@crates/edict-syntax/tests/operation_prerequisites.rs`:
- Around line 447-463: Add an encoder-boundary test alongside the existing
prerequisite tests using a manually constructed legacy artifact with
source_core_coordinate empty and semantic_closure set to None. Invoke the
canonical encoder entry point and assert it rejects the artifact, covering the
validation path beyond lower_to_target_ir and preserving the expected failure
behavior.
In `@docs/abi/edict-target-ir.cddl`:
- Around line 23-28: Update the target-ir-artifact-common schema definition so
sourceCoreCoordinate requires a non-empty string, using the same regexp
constraint as target-ir-resource-ref.id. Preserve the existing field name and
type while rejecting empty wire values.
---
Outside diff comments:
In `@crates/edict-syntax/src/canonical.rs`:
- Around line 462-484: Update target_ir_artifact_value to reject an empty
artifact.source_core_coordinate before emitting the raw sourceCoreCoordinate
field, returning the existing canonical validation error used for invalid
coordinates. Preserve canonicalization for non-empty coordinates and ensure
encode_target_ir_artifact and digest_target_ir_artifact cannot accept the empty
value.
In `@crates/edict-syntax/src/compiler.rs`:
- Around line 550-566: Update type_ref_shape and the type-declaration validation
to consistently reserve built-in scalar names (I32, I64, U32, and U64): reject
declarations using those names before adding them to named_types, or otherwise
make the reservation explicit so such declarations cannot be accepted yet
shadowed during type resolution. Add coverage for each collision if the
project’s existing tests support it.
🪄 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: 64d109d3-e16c-4569-b898-ee5723a14589
📒 Files selected for processing (25)
crates/edict-cli/src/main.rscrates/edict-cli/tests/jsonl_cli.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-syntax/src/canonical.rscrates/edict-syntax/src/compiler.rscrates/edict-syntax/src/contract_bundle.rscrates/edict-syntax/src/target_ir.rscrates/edict-syntax/tests/contract_bundle.rscrates/edict-syntax/tests/operation_prerequisites.rscrates/edict-syntax/tests/provider_lowering.rsdocs/abi/edict-target-ir.cddldocs/design/canonical-target-ir-v0.11.mddocs/design/contract-bundle-assembly-v0.11.mddocs/topics/cli/README.mddocs/topics/cli/test-plan.mddocs/topics/compiler-spine/README.mddocs/topics/compiler-spine/test-plan.mddocs/topics/contract-bundles/README.mddocs/topics/contract-bundles/test-plan.mddocs/topics/core-ir/README.mddocs/topics/core-ir/test-plan.mddocs/topics/target-ir/README.mddocs/topics/target-ir/test-plan.mdfixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.json
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: rust msrv 1.94.0 (fmt · clippy · test)
- GitHub Check: rust stable (fmt · clippy · test)
- GitHub Check: supply-chain (cargo-deny)
🧰 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/cli/README.mddocs/topics/compiler-spine/README.mddocs/topics/contract-bundles/README.mddocs/topics/core-ir/test-plan.mddocs/topics/target-ir/README.mddocs/topics/contract-bundles/test-plan.mddocs/topics/core-ir/README.mddocs/topics/compiler-spine/test-plan.mddocs/topics/cli/test-plan.mddocs/topics/target-ir/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/cli/README.mddocs/design/contract-bundle-assembly-v0.11.mddocs/topics/compiler-spine/README.mddocs/topics/contract-bundles/README.mddocs/design/canonical-target-ir-v0.11.mddocs/topics/core-ir/test-plan.mddocs/topics/target-ir/README.mddocs/topics/contract-bundles/test-plan.mddocs/topics/core-ir/README.mddocs/topics/compiler-spine/test-plan.mddocs/topics/cli/test-plan.mddocs/topics/target-ir/test-plan.md
🪛 LanguageTool
docs/design/contract-bundle-assembly-v0.11.md
[grammar] ~104-~104: Ensure spelling is correct
Context: ...ure against the exact supplied Core and lawpacks before the digest enters the bundle pre...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/topics/target-ir/README.md
[grammar] ~97-~97: Ensure spelling is correct
Context: ...icit basis or the Core module imports a lawpack, the artifact carries a `TargetIrSemant...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (30)
crates/edict-cli/tests/jsonl_cli.rs (2)
32-50: LGTM!
1095-1100: LGTM!crates/edict-syntax/src/compiler.rs (2)
657-682: Duplicatebasisclause now properly rejected — past nightmare resolved.Previously this used a lazy
find_mapthat vaporized any secondbasisclause into the void. Now it explicitly drains the iterator and screamsUnsupportedSourceShapeon a second hit. Matchesdirect_type_check_refuses_duplicate_basis_clauses. I withdraw my earlier scorn, conditionally, on continued good behavior.
164-164: Everything else in this diff is exactly as advertised.
basis: Option<CoreExpr>threading throughTypedIntent/lower_core,TypeKind::Bytes { max }+bytes_shape, thecheck_expr/check_expr_with_expectedsplit,check_integer_literal's width-then-domain validation viaparse_core_integer, andis_bare_integer_literal's comparison-context inference all line up cleanly with the fixture, docs, and test evidence (operation_prerequisite_fixture_preserves_fixed_width_basis_and_lawpack_closure,bare_integer_literals_inherit_unambiguous_fixed_width_context,unconstrained_bare_integer_literal_refuses_before_core). Bare-literal-vs-bare-literal comparisons correctly fall through to the "no unambiguous width" rejection instead of silently defaulting to a host width.Also applies to: 266-266, 395-410, 568-634, 693-693, 784-798, 1014-1046, 1503-1646, 1747-1813
docs/topics/compiler-spine/README.md (1)
40-58: LGTM!docs/topics/compiler-spine/test-plan.md (1)
49-52: LGTM!Also applies to: 55-58, 81-85
crates/edict-syntax/src/target_ir.rs (1)
242-245: LGTM!Also applies to: 294-350, 396-403
crates/edict-syntax/src/canonical.rs (1)
494-508: Rest of the canonicalization changes hold up.
entries-vector-based optional-field encoding for both Target IR and Core intents, the source-core-coordinate cross-check against the semantic closure, and width-awareint_text_valuedomain validation are all consistent with the described contract and test evidence.Also applies to: 541-564, 782-802, 955-975, 1079-1094
docs/abi/edict-target-ir.cddl (1)
1-22: LGTM!Also applies to: 29-79
docs/design/canonical-target-ir-v0.11.md (1)
49-99: LGTM!Also applies to: 100-111
docs/topics/target-ir/README.md (1)
90-104: LGTM!Also applies to: 114-121, 135-149
crates/edict-cli/src/main.rs (1)
1096-1195: LGTM!Also applies to: 1475-1475
docs/topics/cli/README.md (1)
49-56: LGTM!docs/topics/cli/test-plan.md (1)
88-88: LGTM!crates/edict-syntax/tests/operation_prerequisites.rs (1)
101-158: LGTM!Also applies to: 161-175, 214-277, 279-307, 342-396
docs/topics/core-ir/test-plan.md (1)
53-54: LGTM!Also applies to: 101-102
docs/topics/core-ir/README.md (1)
43-45: LGTM!Also applies to: 64-68, 78-80
crates/edict-syntax/src/contract_bundle.rs (3)
559-618: Corroboration logic is correct — nice tight closure-vs-Core binding. All four match arms are structurally sound and the check correctly runs beforedigest_target_ir_artifactis called, so a mismatched closure never reaches the bundle preimage.One gap: the
(Some(_), None) | (None, Some(_))arm collapses two semantically distinct failure modes (missing required closure vs. an unexpected extra closure on a no-basis/no-lawpack Core) into one untested branch pairing. See the companion note on the test file for the specific missing cases.
27-34: LGTM!
620-628: 📐 Maintainability & Code QualityNo duplicate helper here.
canonical.rsonly encodesResourceRef/ResourceListvalues; it does not define a shared sort+dedup normalizer, socanonical_resource_setis just a local helper.> Likely an incorrect or invalid review comment.docs/design/contract-bundle-assembly-v0.11.md (1)
37-40: LGTM!Also applies to: 103-106
docs/topics/contract-bundles/README.md (1)
72-76: LGTM!docs/topics/contract-bundles/test-plan.md (1)
46-46: LGTM!Also applies to: 70-70
crates/edict-syntax/tests/provider_lowering.rs (1)
200-200: LGTM!crates/edict-provider-schema/tests/provider_contract_pack.rs (2)
14-19: Verified: schema-rejection test is internally consistent with the CDDL union.Stripping
semanticClosurefrom a basis-bearing intent correctly falls into the gap betweentarget-ir-closed-artifact(requiressemanticClosure) andtarget-ir-legacy-artifact(itstarget-ir-legacy-intentdoesn't permitbasis), soSchemaMismatchis the right outcome.Also applies to: 40-41, 191-198
263-286: 🎯 Functional CorrectnessThe context concern is incorrect. Both the new integration test and the
semantic_closure/basisbuilder change are present in the patch, so there’s enough context to review them.> Likely an incorrect or invalid review comment.fixtures/provider-contracts/v1/manifest.json (1)
6-7: LGTM!fixtures/provider-contracts/v1/edict-provider-contracts.cddl (1)
300-300: LGTM!Also applies to: 763-804
docs/topics/target-ir/test-plan.md (1)
54-55: LGTM!Also applies to: 56-62, 103-105
crates/edict-syntax/tests/contract_bundle.rs (1)
379-391: 🎯 Functional CorrectnessNo issue: the hardcoded digest is the expected 64 hex chars.
> Likely an incorrect or invalid review comment.
|
@codex review Please review the exact current head |
|
Codex Review: Didn't find any major issues. You're on a roll. 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
Plain-English Walkthrough
TL;DR
Edict now preserves the minimum generic inputs needed by a future executable operation: exact fixed-width integer domains, bounded byte payloads, an input-derived explicit basis, and a digest-locked Core/lawpack semantic closure in Target IR. [claim:operation-prerequisites, confidence:1.00]
This is Stage 1 only. The branch does not add an Echo evaluator, operation installation, runtime admission, executable rope semantics, or receipt claims. A Target IR digest remains semantic identity evidence and does not confer operation identity, invocability, or runtime authority. [claim:no-runtime-authority, confidence:1.00]
Walkthrough
Previously, the initial compiler subset recognized neither the required fixed-width source scalars nor bounded bytes, and it rejected every basis other than
basis none. That prevented a real basis-bearing operation from reaching canonical Core and Target IR without narrowing or losing its semantic dependencies.The new path preserves those inputs through the existing compiler and lowerer boundaries:
flowchart LR A[Edict operation source] --> B[Parse and type-check] B --> C[Canonical Core with basis] C --> D[Existing target lowerer] D --> E[Target IR with semantic closure]Caption: Stage 1 operation prerequisite flow
The diagram ends at semantic artifact production. No edge in this PR crosses into Echo execution or commitment.
Exact inputs and basis
The compiler recognizes
I32,I64,U32, andU64, propagates unambiguous expected widths into bare literals in supported comparisons, annotations, and record returns, folds unary-negative integer literals through the exact domain validator, and preservesBytes<max=N>. Signed minima reach Core; unconstrained bare integers, cross-width assignments, overflow, and negative unsigned values fail before Core identity exists. [claim:fixed-width-domain, confidence:1.00]An explicit basis is checked before body locals enter scope and is then retained in Core and Target IR. A body-local basis and missing or duplicate basis clauses fail with typed compiler errors rather than panicking. [claim:prebody-basis, confidence:1.00]
Semantic closure and substitution boundary
For an operation with an explicit basis or lawpack import, target lowering computes the canonical Core digest and binds it with a coordinate-keyed, digest-locked lawpack set. Equivalent lawpack order is identity-invariant; an empty Core coordinate, conflicting resource, or unidentifiable Core refuses without an artifact. On the computed bundle path, assembly recomputes that closure from the exact supplied Core and requires the Target IR closure and bundle lawpack set to match, then canonicalizes equivalent bundle-lawpack order and duplicate identical references before identity construction. [claim:semantic-closure, confidence:1.00]
A hand-constructed Target IR cannot substitute a different Core coordinate inside the closure while retaining the envelope coordinate, nor can it retain an explicit basis after dropping the closure. Canonical identity construction rejects both internal contradictions. Computed bundle assembly separately rejects a same-coordinate Core digest substitution, a stripped lawpack-only closure, or a substituted bundle lawpack set. [claim:core-substitution-refusal, confidence:1.00]
The Core and Target IR CDDL roots accept the new fields, require a non-empty source Core coordinate, and the Target IR root has separate closed and legacy variants. The structurally separate provider-schema path rejects a basis-bearing external artifact after its closure is removed. The generated provider contract pack was regenerated from those authoritative schema fragments. [claim:schema-conformance, confidence:1.00]
Review projections
The
projectcommand now includes an explicit basis in Core and Target IR review JSON and includes the source Core plus lawpack set in the Target IR review. Optional fields remain omitted for legacy basis-free projections. Review JSON remains display data; canonical identity still comes only from the canonical encoders. [claim:review-projection, confidence:1.00]Verifier grades
edict-provider-schemavalidates independently assembled canonical Core and Target IR values against the published CDDL roots, including missing-closure refusalThe distinction is deliberate: passing a different crate and CDDL path is structurally separate verification, not proof of an independently implemented compiler or lowerer. [claim:verifier-grades, confidence:0.99]
Compatibility and non-goals
When an artifact has
basis noneand no lawpack import, the optional closure and basis fields are omitted. Existing reviewed Core and Target IR golden bytes and digests remain unchanged. [claim:provider-v1-compatibility, confidence:1.00]No dependencies were added. This PR does not add
EchoOperationProgramV1, a hook-free Echo evaluator, Jedit rope semantics, a Jim invocation path, the Kitten runtime, TextWindow, child lanes, wormholes, or Continuum transport.Verification
cargo test -p edict-syntax --test operation_prerequisites— 18 passed, 0 failed.cargo test -p edict-syntax --test contract_bundle contract_bundle_assembly— 19 passed, 0 failed.cargo xtask verify— passed atdeed97b47b069c9492221ccada5b128385ac91c2, including format, strict Clippy, workspace/all-feature tests, doctests, reviewed goldens, provider fixtures and contract pack, dependency boundary checks, and all 23 topic shelves.origin/main.Appendix: Citations
claim:operation-prerequisitescrates/edict-syntax/src/compiler.rs#544@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#101@deed97b47b069c9492221ccada5b128385ac91c2claim:no-runtime-authoritydocs/design/canonical-target-ir-v0.11.md#88@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#1@deed97b47b069c9492221ccada5b128385ac91c2claim:fixed-width-domaincrates/edict-syntax/src/compiler.rs#1541@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/src/compiler.rs#1605@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#178@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#280@deed97b47b069c9492221ccada5b128385ac91c2claim:prebody-basiscrates/edict-syntax/src/compiler.rs#657@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#343@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#358@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#376@deed97b47b069c9492221ccada5b128385ac91c2claim:semantic-closurecrates/edict-syntax/src/target_ir.rs#294@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/src/contract_bundle.rs#562@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#101@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/contract_bundle.rs#645@deed97b47b069c9492221ccada5b128385ac91c2claim:core-substitution-refusalcrates/edict-syntax/src/canonical.rs#462@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/operation_prerequisites.rs#590@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/contract_bundle.rs#645@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/contract_bundle.rs#669@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-syntax/tests/contract_bundle.rs#684@deed97b47b069c9492221ccada5b128385ac91c2claim:schema-conformancedocs/abi/edict-core.cddl#210@deed97b47b069c9492221ccada5b128385ac91c2;docs/abi/edict-target-ir.cddl#21@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-provider-schema/tests/provider_contract_pack.rs#180@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-provider-schema/tests/provider_contract_pack.rs#263@deed97b47b069c9492221ccada5b128385ac91c2claim:review-projectioncrates/edict-cli/src/main.rs#1128@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-cli/src/main.rs#1146@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-cli/tests/jsonl_cli.rs#1096@deed97b47b069c9492221ccada5b128385ac91c2;crates/edict-cli/tests/jsonl_cli.rs#1135@deed97b47b069c9492221ccada5b128385ac91c2claim:verifier-gradescrates/edict-provider-schema/tests/provider_contract_pack.rs#180@deed97b47b069c9492221ccada5b128385ac91c2;docs/topics/target-ir/test-plan.md#103@deed97b47b069c9492221ccada5b128385ac91c2claim:provider-v1-compatibilitydocs/topics/target-ir/test-plan.md#105@deed97b47b069c9492221ccada5b128385ac91c2;xtask/src/tests.rs#60@deed97b47b069c9492221ccada5b128385ac91c2