Preserve typed result projections for Echo targets - #174
Conversation
|
@coderabbitai review |
WalkthroughAdds a compiler-owned ChangesResult projection support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant TargetLowering
participant ApplicationBuild
participant Provider
Compiler->>TargetLowering: derive Core/Target result projection
TargetLowering->>ApplicationBuild: return projection or failure
ApplicationBuild->>ApplicationBuild: verify exactly one projection
ApplicationBuild->>Provider: bind canonical bytes and digest
Provider->>Provider: consume seven semantic inputs
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 |
|
@codex please review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07e9c8c4c7
ℹ️ 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: 15
🤖 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/src/application_build.rs`:
- Around line 1048-1067: Add RED/GREEN unit coverage in the existing tests
module and update the owning test-plan.md for the negative paths enforced by
single_result_projection and with_result_projection_input: zero projections,
multiple projections, failures alongside a valid projection, and a digest
mismatch. Construct deterministic small BTreeMap fixtures and a mutated artifact
digest, verify the expected ResultProjectionUnavailable or
ResultProjectionDigestMismatch errors, then implement only the minimal changes
needed for the tests to pass and mark the plan cases implemented.
In `@crates/edict-syntax/src/result_projection.rs`:
- Around line 275-294: Remove the explicit validate_projection_bounds call from
decode_result_projection and rely on encode_result_projection’s existing
validation during the canonical round-trip check. Preserve the current error
propagation and exact-byte comparison behavior.
- Around line 412-434: Replace the bare "arg.0" literal in
resolve_application_input with a named constant representing the Core lowering’s
single application-input local, shared with or explicitly tied to the Core
emitter. Document the invariant that this local must use that name, and preserve
the existing matching and duplicate-rejection behavior.
- Around line 37-45: Make ResultProjection and ResultProjectionArtifact fields
private to preserve canonical and digest invariants, matching the encapsulation
used by VerifiedResultProjection. Add or retain read-only accessors and update
consumers such as verify_result_projection and application_build to use them,
while preserving construction and validation behavior.
- Around line 932-978: Add a remaining expression-depth parameter or counter to
projection_from_value and expression_from_value, decrementing it before
recursively parsing nested record field expressions and rejecting when the
configured MAX_RESULT_PROJECTION_NODES limit is exhausted. Propagate this state
through every projection-expression parsing call so deeply nested records fail
before further recursion, while preserving existing parsing behavior within the
limit.
- Around line 687-693: Update resolve_type to avoid constructing a formatted
prefix during each lookup: after the direct core.types lookup misses, derive the
relative coordinate by checking for core.coordinate followed by a '.' and
stripping that prefix without allocation, then retry core.types with the
relative value. Preserve the existing absolute-key precedence and fallback
behavior.
- Around line 695-698: Introduce a dedicated named recursion-depth constant for
the type-compatibility walk, separate from MAX_RESULT_PROJECTION_PATH_SEGMENTS.
Update types_are_compatible to use the new constant while preserving its
existing fail-closed behavior, leaving the field-path limit unchanged for
source-reference validation.
In `@crates/edict-syntax/src/target_ir.rs`:
- Around line 305-329: Update lower_result_projections and
emit_result_projection to reuse target_ir.semantic_closure or a precomputed
ProjectionSemantics basis, rather than constructing it from
semantic_closure_for_core for each intent. Preserve the existing per-intent
projection and failure collection while ensuring the Core module closure and
digest are computed only once.
- Around line 177-178: Document the public fields result_projections and
result_projection_failures in Lowered, explicitly stating that a Lowered report
may contain zero projections while still carrying non-empty projection failures,
and that an empty failures collection does not imply any projections exist. Make
clear that admission of these results is the caller’s responsibility.
In `@crates/edict-syntax/tests/result_projection.rs`:
- Around line 416-457: Expand the negative coverage around result_projection.rs
by splitting path_text_artifact_and_structure_bounds_fail_closed into focused
tests. Add cases that assert empty text rejects as InvalidCanonicalArtifact,
decoded zero maxOutputBytes rejects as InvalidOutputBound, an absent target-IR
step rejects as UnknownCapabilityStep, and missing or duplicate arg.0
application-input intent rejects as InvalidApplicationInput. Construct each case
through the relevant encode_result_projection or decode_result_projection path
and assert the specific failure kind.
- Around line 460-494: Replace the vacuous BTreeMap insertion-order loop in
canonical_encoding_is_insertion_order_independent_for_fixed_seed_cases with a
test that constructs a CanonicalValue::Map in deliberately non-canonical order
and verifies encode_canonical_cbor normalizes it. Include keys such as "z" and
"aa" to cover the canonical length-boundary ordering difference, and assert the
resulting encoded bytes or structured artifact against the canonical expectation
rather than relying on BTreeMap ordering.
In `@docs/abi/edict-result-projection.cddl`:
- Line 18: Update the canonical CDDL schema for result-projection paths and
field maps to encode the same occurrence limits enforced by
validate_expression_bounds: cap path segments at
MAX_RESULT_PROJECTION_PATH_SEGMENTS and field entries according to
MAX_RESULT_PROJECTION_NODES. Use the exact constant values and add comments
referencing both constants so future changes remain synchronized.
- Line 10: Add a projection fixture covering maxOutputBytes set to zero and
assert that validation returns SchemaMismatch. Reuse the existing maxOutputBytes
fixture structure or generated-field variant, preserving the missing-field
coverage while directly exercising the uint .gt 0 constraint.
In `@docs/topics/cli/README.md`:
- Around line 76-82: Rewrite the singleton executable-operation paragraph in the
documentation to distinguish target lowering from application build: lowering
carries a per-intent projection map and failure map, while
single_result_projection in application_build.rs enforces exactly one projection
and rejects recorded failures before independently invoking
verify_result_projection. Preserve the existing provider semantic-input and Echo
`#698` details, but attribute cardinality and verification to the build route
rather than lowering.
In `@fixtures/provider-contracts/v1/edict-provider-contracts.cddl`:
- Around line 798-826: Update the result-projection CDDL definitions around
result-projection, result-projection-record, and result-projection-source to
mirror validate_projection_bounds exactly: bound maxOutputBytes, record nesting
and field counts, source path segments, and total artifact size. Add
provider-schema validation tests proving each over-limit projection is rejected,
while valid boundary values remain accepted.
🪄 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 Plus
Run ID: 9410e446-d3ff-486e-be28-91bd647f4626
📒 Files selected for processing (22)
CHANGELOG.mdcrates/edict-cli/src/application_build.rscrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-syntax/src/lib.rscrates/edict-syntax/src/result_projection.rscrates/edict-syntax/src/target_ir.rscrates/edict-syntax/tests/result_projection.rsdocs/abi/edict-result-projection.cddldocs/topics/README.mddocs/topics/cli/README.mddocs/topics/result-projections/README.mddocs/topics/result-projections/test-plan.mddocs/topics/target-ir/README.mdfixtures/lawpack/hello-echo/README.mdfixtures/lawpack/hello-echo/create-greeting.result-projection.cborfixtures/lawpack/hello-echo/create-greeting.result-projection.sha256fixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.jsonxtask/src/lawpack_goldens.rsxtask/src/provider_contract_pack.rsxtask/src/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: supply-chain (cargo-deny)
- GitHub Check: rust stable (fmt · clippy · test)
- GitHub Check: rust msrv 1.94.0 (fmt · clippy · test)
🧰 Additional context used
📓 Path-based instructions (6)
**/*
📄 CodeRabbit inference engine (AGENTS.md)
**/*: Never amend Git commits, usegit rebasewithout explicit user approval, or force any Git operation; use new commits and regular merge commits instead.
Do not create draft pull requests, and never use acodexprefix in branch names, pull request titles, or commit messages.
Pull requests for issue work must include GitHub auto-close text such asCloses#123`` for every issue they intend to close.
Usecodex-think --remember --jsonwhen starting a session, entering the repository, or regaining context, and record significant durable events with `codex-think "..." --json`. Treat Think as memory rather than repository truth.
Every pull request body must contain `## Plain-English Walkthrough` with `### TL;DR` and `### Walkthrough`, explaining the prior behavior, new model and dataflow, invariants, failures, compatibility, and verification as applicable.
Use Mermaid diagrams for nontrivial flow, lifecycle, ownership, or component interaction when clearer than prose; every diagram requires an introductory paragraph, the diagram, the exact collapsed caption structure, and a concluding interpretation.
Tag each material technical claim at first occurrence as `[claim:, confidence:]`, cite evidence using repository-relative paths, line numbers, and Git SHAs, and end the explanatory body with a collapsed citations appendix.
If CodeRabbit is actively reviewing, obtain its approval before merge; if unavailable due to limits or credits, request `@codex review please` and wait for the alternate response. Do not treat unavailability as approval unless a maintainer explicitly overrides the gate.
For release preparation, write the release thesis first, reconcile changes from the previous tag, update release policy and tests, verify the milestone has no open issues and no unauthorized crates.io publication occurred, and record a durable release report.
Run `cargo xtask verify` before claiming a branch is ready.
Files:
fixtures/lawpack/hello-echo/create-greeting.result-projection.sha256docs/topics/README.mdfixtures/lawpack/hello-echo/create-greeting.result-projection.cbordocs/topics/result-projections/README.mdCHANGELOG.mdfixtures/lawpack/hello-echo/README.mddocs/topics/cli/README.mdfixtures/provider-contracts/v1/edict-provider-contracts.cddlcrates/edict-syntax/src/lib.rsxtask/src/provider_contract_pack.rsdocs/abi/edict-result-projection.cddldocs/topics/target-ir/README.mddocs/topics/result-projections/test-plan.mdxtask/src/lawpack_goldens.rsxtask/src/tests.rsfixtures/provider-contracts/v1/manifest.jsoncrates/edict-syntax/tests/result_projection.rscrates/edict-syntax/src/target_ir.rscrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-cli/src/application_build.rscrates/edict-syntax/src/result_projection.rs
docs/topics/**
📄 CodeRabbit inference engine (AGENTS.md)
docs/topics/**: Topic shelves document landed behavior:README.mddescribes current HEAD truth,test-plan.mdrecords verification and known gaps, and optional architecture or rationale pages contain durable supporting information.
For every nontrivial behavior, contract, workflow, release, schema, validation, or public-surface change, identify or create the owning topic shelf, updatetest-plan.md, add executable evidence, updateREADME.mdonly after behavior exists, and runcargo xtask verify.
Do not update topic shelves for purely mechanical edits that do not change a contract; explain the omission in the pull request or final report.
Files:
docs/topics/README.mddocs/topics/result-projections/README.mddocs/topics/cli/README.mddocs/topics/target-ir/README.mddocs/topics/result-projections/test-plan.md
**/*.{rs,md}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,md}: Tests must assert software behavior and stable error kinds or structured artifacts, not implementation details, prose, paths, or merelyis_err(); documentation-tool tests may test validator behavior.
For nontrivial behavior, contract, workflow, release, schema, validation, or public-surface changes, follow RED/GREEN TDD: update the owningtest-plan.md, write the deterministic test first, observe the RED failure, implement the smallest coherent fix, then mark the case implemented only after executable evidence exists.
Files:
docs/topics/README.mddocs/topics/result-projections/README.mdCHANGELOG.mdfixtures/lawpack/hello-echo/README.mddocs/topics/cli/README.mdcrates/edict-syntax/src/lib.rsxtask/src/provider_contract_pack.rsdocs/topics/target-ir/README.mddocs/topics/result-projections/test-plan.mdxtask/src/lawpack_goldens.rsxtask/src/tests.rscrates/edict-syntax/tests/result_projection.rscrates/edict-syntax/src/target_ir.rscrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-cli/src/application_build.rscrates/edict-syntax/src/result_projection.rs
**/*.{md,mdx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{md,mdx}: Documentation pages must have one primary reader job, separate user task help from contributor architecture and evidence maps, use concrete valid examples with expected results when relevant, and keep exact public facts in validated or generated reference material.
Update affected documentation in the same change as behavior, schema, release, workflow, or public-surface changes, or statedocs-impact: nonewith a concise rationale.
Files:
docs/topics/README.mddocs/topics/result-projections/README.mdCHANGELOG.mdfixtures/lawpack/hello-echo/README.mddocs/topics/cli/README.mddocs/topics/target-ir/README.mddocs/topics/result-projections/test-plan.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Use tables for consistent-shape comparisons and evidence, bullets for unordered sets, numbered lists for ordered procedures or states, and focused branch-accurate snippets for exact syntax.
Files:
docs/topics/README.mddocs/topics/result-projections/README.mdCHANGELOG.mdfixtures/lawpack/hello-echo/README.mddocs/topics/cli/README.mddocs/topics/target-ir/README.mddocs/topics/result-projections/test-plan.md
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: For Rust changes, preserve claim integrity by providing executable evidence, keep compiler and validation paths deterministic and free of hidden I/O, and prefer structured public failures with stable error kinds over prose-only diagnostics.
Do not add Rust dependencies without pull-request rationale and contract-impact notes; treat planned lint, dependency, and fuzzing ratchets as planned until executable checks land.
Files:
crates/edict-syntax/src/lib.rsxtask/src/provider_contract_pack.rsxtask/src/lawpack_goldens.rsxtask/src/tests.rscrates/edict-syntax/tests/result_projection.rscrates/edict-syntax/src/target_ir.rscrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-cli/src/application_build.rscrates/edict-syntax/src/result_projection.rs
🪛 LanguageTool
docs/topics/result-projections/test-plan.md
[grammar] ~38-~38: Ensure spelling is correct
Context: ...ction.rs | Prevents target-owned result authorship. | | RESULT-PROJ-TP-004 | implemented |...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (22)
crates/edict-provider-schema/src/contract_pack.rs (2)
78-78: 📐 Maintainability & Code QualityConfirm the public API break is intentional.
ProviderContractPackInput<'a>is public, so adding a required field breaks every downstream struct literal that is not updated. In-repository callers are covered, but external consumers will fail to compile; use a constructor/builder or document this as an intentional breaking release with migration guidance.
15-16: LGTM!Also applies to: 36-43, 53-58, 303-314
crates/edict-provider-schema/tests/provider_contract_pack.rs (1)
15-21: LGTM!Also applies to: 32-33, 46-55, 80-81, 281-296, 543-543, 597-597, 611-630
fixtures/provider-contracts/v1/manifest.json (1)
6-7: LGTM!Also applies to: 16-16, 27-27
xtask/src/provider_contract_pack.rs (1)
15-15: LGTM!Also applies to: 38-38, 47-47
xtask/src/tests.rs (1)
144-144: LGTM!Also applies to: 177-180, 283-283
crates/edict-syntax/src/lib.rs (1)
86-86: LGTM!Also applies to: 199-207
crates/edict-syntax/src/result_projection.rs (2)
148-245: LGTM!Also applies to: 488-507, 569-603, 605-685, 783-850
852-891: 🗄️ Data Integrity & IntegrationNo change needed for canonical CBOR map ordering.
encode_valueencodes every map key, deduplicates against encoded key bytes, then sorts the(key_bytes, value)pairs by encoded key bytes before writing the map, soBTreeMapinput order does not dictate the emitted byte order.> Likely an incorrect or invalid review comment.docs/abi/edict-result-projection.cddl (1)
1-7: LGTM!Also applies to: 14-17, 21-23, 27-34
crates/edict-syntax/src/target_ir.rs (1)
16-16: LGTM!Also applies to: 284-303, 796-797
crates/edict-syntax/tests/result_projection.rs (1)
130-160: LGTM!Also applies to: 162-184, 186-209, 211-227, 229-250, 252-268, 270-286, 288-325, 327-362, 364-399, 401-413, 496-513
crates/edict-cli/src/application_build.rs (1)
1-1: LGTM!Also applies to: 18-30, 42-42, 94-94, 304-326, 377-377, 734-734, 838-838, 911-968, 977-1034, 1746-1757, 1836-1878
xtask/src/lawpack_goldens.rs (1)
35-38: LGTM!Also applies to: 462-468, 480-480, 506-513
fixtures/lawpack/hello-echo/README.md (1)
28-32: LGTM!Also applies to: 49-53
fixtures/lawpack/hello-echo/create-greeting.result-projection.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/create-greeting.result-projection.sha256 (1)
1-2: LGTM!docs/topics/README.md (1)
68-70: LGTM!docs/topics/result-projections/README.md (1)
1-142: LGTM!docs/topics/result-projections/test-plan.md (1)
1-72: LGTM!docs/topics/target-ir/README.md (1)
44-56: LGTM!CHANGELOG.md (1)
24-34: LGTM!
|
Merge gate open at 8cd6bbe. CI: rust stable, Rust 1.94 MSRV, cargo-deny, and CodeRabbit all green. Review threads: 0 unresolved after the operator-authorized second and final Code Lawyer pass. Local gate: cargo xtask verify passed. The repository is solo-maintained and the ruleset requires 0 approvals, so criteria 1-4 substitute for human approval. No admin override is required. |
Plain-English Walkthrough
TL;DR
[claim:compiler-owned-result-projection, confidence:1.00] Edict now preserves an application operation's authored result assembly as a bounded, canonical
edict.result-projection/v1artifact derived from exact Core and Target IR. The public application build independently verifies that artifact and passes the same bytes and digest into both provider component closures.This closes the compiler-side contract required by Echo #698. It does not evaluate the projection, reconstruct an application result in native code, or add runtime authority.
Walkthrough
Previously, Target IR retained the intent result expression, but the executable-operation provider closure did not carry a separately typed, canonical description of how the public application result is assembled. Echo therefore had no compiler-owned result contract to evaluate or recover.
[claim:projection-dataflow, confidence:1.00] The new flow keeps result authorship in Edict and makes every boundary explicit:
flowchart LR A[Edict source and lawpack closure] --> B[Core result] B --> C[Target IR result] C --> D[Bounded canonical projection] D --> E[Independent reverse verification] E --> F[Lowerer and verifier semantic-input closures]Caption: Compiler-owned result projection flow
The provider receives data, not an application callback. Projection emission and verification have no filesystem, network, process, model, or runtime imports.
[claim:closed-and-bounded, confidence:1.00] The v1 expression language permits records, declared application input, capability results addressed by Target IR step identity, and bounded field paths. Unsupported calls, undeclared locals, shape mismatches, zero or exceeded bounds, noncanonical bytes, substituted digests, altered Target results, and altered Core or lawpack closure identities fail with structured rejection kinds.
[claim:independent-verification, confidence:1.00] Verification does not call the emitter. It decodes and reproduces canonical bytes, recomputes the domain-framed identity, reconstructs the complete Core-derived semantic closure, reverse-maps projection sources to Core, and requires the reconstructed result to equal both Core and Target IR.
[claim:provider-closure-binding, confidence:1.00] The standalone application build requires exactly one admitted projection, independently verifies it, then binds the same canonical artifact into both lowerer and verifier semantic-input sets under role
07-result-projection. A provider that does not declare the domain refuses at the existing schema-admission boundary.Compatibility and ownership
[claim:target-ir-compatibility, confidence:1.00] General Target IR remains available for operations outside the first projection subset: per-intent projection failures are reported separately and legacy artifacts without an explicit semantic closure claim neither a projection nor a projection failure. The executable application build is intentionally stricter and refuses a missing, ambiguous, or failed projection.
Echo #698 owns generic package inclusion, runtime evaluation, durable result binding, and recovery. The current external Hello Echo build therefore reaches the expected
UnsupportedArtifactDomainrefusal at Echo's six-input provider package; no provisional Echo-owned projection format is introduced here. Hello Echo #18 owns the later concreteGreetingCreatedwitness.No dependencies were added.
RED / GREEN evidence
cargo test -p edict-syntax --test result_projectionfailed at the committedNotImplementedboundary in6a422147echo_target_lowering_emits_the_verified_result_projectionfailed before3d98675fcompiler_result_projection_is_bound_into_the_provider_closurefailed withResultProjectionUnavailablebefored88f8a8mutated_target_lawpack_closure_fails_closedfailed atf4bdc39ecargo xtask verifypassed: format, clippy, all workspace tests and doctests, goldens, provider fixtures/contracts, dependency boundary, contract graph, and diff checkAppendix: Citations
claim:compiler-owned-result-projectioncrates/edict-syntax/src/result_projection.rs#187@8cd6bbe201ad963cf0e253be4b7378e1947afc86;exact_core_and_target_ir_emit_the_typed_hello_echo_projectionincrates/edict-syntax/tests/result_projection.rsclaim:projection-dataflowcrates/edict-syntax/src/target_ir.rs#297@8cd6bbe201ad963cf0e253be4b7378e1947afc86;crates/edict-cli/src/application_build.rs#304@8cd6bbe201ad963cf0e253be4b7378e1947afc86claim:closed-and-boundedcrates/edict-syntax/src/result_projection.rs#180@8cd6bbe201ad963cf0e253be4b7378e1947afc86;docs/topics/result-projections/test-plan.md#36@8cd6bbe201ad963cf0e253be4b7378e1947afc86claim:independent-verificationcrates/edict-syntax/src/result_projection.rs#249@8cd6bbe201ad963cf0e253be4b7378e1947afc86;crates/edict-syntax/src/result_projection.rs#443@8cd6bbe201ad963cf0e253be4b7378e1947afc86;independent_verifier_reconstructs_the_authored_core_resultincrates/edict-syntax/tests/result_projection.rsclaim:provider-closure-bindingcrates/edict-cli/src/application_build.rs#314@8cd6bbe201ad963cf0e253be4b7378e1947afc86;crates/edict-cli/src/application_build.rs#918@8cd6bbe201ad963cf0e253be4b7378e1947afc86;crates/edict-cli/src/application_build.rs#984@8cd6bbe201ad963cf0e253be4b7378e1947afc86;crates/edict-cli/src/application_build.rs#1112@8cd6bbe201ad963cf0e253be4b7378e1947afc86claim:target-ir-compatibilitycrates/edict-syntax/src/target_ir.rs#317@8cd6bbe201ad963cf0e253be4b7378e1947afc86;target_lowering_exposes_an_unsupported_result_projection_without_claiming_oneincrates/edict-syntax/tests/result_projection.rsclaim:full-local-gatecargo xtask verify— passed at8cd6bbe201ad963cf0e253be4b7378e1947afc86Closes #173.