Build verified external applications through provider components - #170
Conversation
|
@coderabbitai review |
|
@codex please review |
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThe PR adds canonical lawpack and direct-adapter loaders, standalone ChangesLawpack build pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
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: 49173d80bc
ℹ️ 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: 25
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/edict-provider-schema/src/recursive_dispatch.rs (1)
666-704: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate-occurrence detection is copy-pasted between the tagged and required-key branches.
specialize_choice'stagged_choicebranch (Lines 676-684) and the newrequired_key_choicebranch (Lines 692-698) both implement "find at most one map entry whose key matches a target text, else reject" with slightly different filter/collect idioms. Worth extracting a small shared helper (e.g.fn single_matching_entry<'a>(entries: &'a [(CanonicalValue, CanonicalValue)], key: &str) -> Result<Option<&'a CanonicalValue>, ()>) to keep this fail-closed invariant defined once.🤖 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-provider-schema/src/recursive_dispatch.rs` around lines 666 - 704, Extract the duplicated map-entry occurrence logic from specialize_choice into a shared helper such as single_matching_entry, returning the sole matching value, None when absent, and an error for duplicates. Use it in both the tagged_choice and required_key_choice branches, preserving their existing fail-closed behavior and discriminator/type handling.docs/schemas/edict.compiler-settings.v1.schema.json (1)
189-218: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEvery
ifclause here is missingrequired, so all three fire simultaneously on a document that omitsoperation.
{"properties": {"operation": {"const": "build"}}}succeeds vacuously whenoperationis absent —propertiesimposes nothing on properties that are not present. Thecheck,project, andbuildconditions therefore all evaluate true for anoperation-less document, and all threethenblocks apply at once:typemust simultaneously be"compilerSettings"and"settings",applicationmust be both absent and required. The document is already invalid via the top-levelrequiredat Line 8, so nothing is let through — but callers get a pile of contradictory sub-errors instead of "missingoperation", and the momentoperationgains a default this becomes a live bug.This is the canonical JSON Schema if/then footgun. Add
"required": ["operation"]to all three conditions, including the pre-existingcheckclause at Lines 174-178.🐛 Proposed fix (apply the same shape to all three branches)
{ "if": { + "required": ["operation"], "properties": { "operation": { "const": "build" } } },🤖 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 `@docs/schemas/edict.compiler-settings.v1.schema.json` around lines 189 - 218, Add "required": ["operation"] to the condition schemas of all three operation branches, including the existing check branch and the project/build branches identified by their operation const values. Keep each branch’s then constraints unchanged so they only apply when operation is explicitly present.crates/edict-cli/src/main.rs (2)
507-520: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe
checkbranch now rejectsapplicationwhile telling the user it is a "project-only" setting.You added
settings.application.is_some()to the guard at Line 508 but left the message at Line 517 untouched. A caller who sendsoperation: "check"with anapplicationpath is now told:
emit,compilerContext, andtargetare project-only settings— which names three fields they did not send, omits the one they did, and misclassifies a build-only field as project-only. You got this exactly right for the
Projectbranch at Lines 523-530 with its own dedicated"application" is a build-only setting"message. Do the same here instead of smuggling a fourth condition into a three-field message.🐛 Proposed fix: split the application check out of the shared message
Operation::Check => { - if settings.application.is_some() - || !settings.emit.is_empty() + if settings.application.is_some() { + return Err(CliFailure { + command: COMMAND_CHECK, + kind: "InvalidSettings", + line: Some(line), + message: "`application` is a build-only setting".to_owned(), + }); + } + if !settings.emit.is_empty() || settings.compiler_context.is_some() || settings.target.is_some() {🤖 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-cli/src/main.rs` around lines 507 - 520, Update the Operation::Check validation to handle settings.application separately from the existing emit, compiler_context, and target validation, using a dedicated message that identifies application as a build-only setting. Keep the shared project-only message limited to emit, compilerContext, and target.
378-385: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA
buildrequest carrying compiler-input records is accepted, then the inputs are thrown away in silence.Line 378 was relaxed to skip the
MissingInputerror forBuild, but nothing was added to reject a build request that supplies input records.run_requestthen short-circuits at Line 714 intorun_build_request(&request.settings), which never touchesrequest.inputs. The caller gets exit0and anokstatus while everyedict.compiler.input/v1record they sent was parsed, validated, and discarded.
docs/topics/cli/README.mdLine 26 states the contract flatly: "Abuildrequest contains one settings record and no compiler-input records." Silently ignoring input on a fail-closed compiler boundary is the wrong reading of that sentence. Reject it.🐛 Proposed fix: fail closed on unexpected build inputs
- if inputs.is_empty() && settings.operation != Operation::Build { + if settings.operation == Operation::Build { + if !inputs.is_empty() { + return Err(CliFailure { + command: COMMAND_BUILD, + kind: "InvalidRecord", + line: None, + message: "build requests must not contain compiler input records".to_owned(), + }); + } + } else if inputs.is_empty() { return Err(CliFailure { command: command_for_operation(settings.operation), kind: "MissingInput", line: None, message: "request must contain at least one compiler input record".to_owned(), }); }Also applies to: 714-716
🤖 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-cli/src/main.rs` around lines 378 - 385, Update the request validation in run_request around the existing inputs.is_empty() check and the build short-circuit to reject any Build request with compiler-input records, using the established CliFailure/MissingInput error pattern or an appropriate validation error. Preserve the current requirement for non-Build operations to include at least one input, and ensure invalid Build requests do not reach run_build_request with inputs silently discarded.xtask/src/tests.rs (1)
2020-2043: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact compiler-settings schema contract.
json_string_array_containspermits unsupported extra enum values, whileproperties.contains_key("application")accepts an arbitrary or incomplete property definition. This test can pass after the public build contract is widened or the application schema loses required validation. Compare the exact allowed enum values and validate theapplicationproperty's type and required fields.🤖 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 `@xtask/src/tests.rs` around lines 2020 - 2043, Update the compiler-settings schema assertions around record_type, operation, and application to enforce the exact public contract: compare enum arrays against the complete expected values rather than using json_string_array_contains, and inspect application to require the expected object type and all required validation fields. Keep the existing missing-property diagnostics while rejecting unsupported enum values and incomplete application definitions.
🤖 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/Cargo.toml`:
- Around line 19-25: Remove the direct sha2 dependency from the CLI and update
application_build.rs::provider_digest to reuse an exported digest-framing helper
from edict-syntax, preserving the canonical [CORE_DIGEST_FRAME, domain, value]
behavior. Expose the existing digest_value logic in edict-syntax rather than
duplicating the hash implementation, and use the workspace’s standard dependency
version convention instead of pinning a patch release.
In `@crates/edict-cli/src/application_build.rs`:
- Around line 748-754: The adapter identity must match the profile-selected
descriptor and must not index the optional adapter list. Expose the selected
descriptor’s adapter ID on ValidatedLawpackAdapter, then use it in both
01-lawpack-adapter semantic_input calls at
crates/edict-cli/src/application_build.rs lines 748-754 and 806-812, replacing
the target_adapters[0] lookup.
- Around line 100-102: Replace the unchecked indexing of config.sources and
loaded_lawpacks in the application build flow with local, total access that
handles missing entries without panicking. Preserve the existing successful-path
behavior for the first source and lawpack, and return the established structured
error result when either collection is empty; update the code near resolve and
the loaded_lawpacks use rather than relying on validate_application_manifest.
- Around line 516-539: Remove the duplicated effect projection and single-target
validation from validate_target_configuration_binding and single_configuration
by introducing or reusing one shared accessor for the sole target configuration.
Have both paths obtain the configuration through that accessor, while keeping
the existing InvalidLawpackAdapter failure and preserving
validate_target_configuration_binding’s digest check against the exact bytes
used by the provider.
- Around line 1032-1069: Update the output publication function around the
package/report staging and renames to use process-unique temporary filenames,
fsync both staged files before publication, and clean up all temporary files on
every failure path. Publish the verification report before the executable
package so the package is the final visible artifact, and preserve the existing
failure type/messages and successful output paths; if pair-atomicity is required
by the surrounding contract, stage both files in a temporary directory and
rename that directory once.
- Around line 541-555: Refactor invoke_lowerer and invoke_verifier to accept one
borrowed context struct containing coordinate, core_bytes, target_profile,
loaded, adapter, source_bytes, and target_ir_bytes, then update their call sites
and field accesses while preserving behavior. Replace the bare
#[allow(clippy::too_many_arguments)] on both functions with the file’s existing
reason = "…" suppression style, or remove it if the refactor eliminates the
warning.
- Around line 346-381: Update validate_application_manifest and its callers to
enforce root confinement for every resolved application path, using the
manifest’s parent directory as the root. Reject absolute paths and paths
escaping via .. for sources, lawpacks, target.providerPackage, and
outputDirectory before build performs any reads or writes; preserve existing
validation errors for unrelated manifest fields.
- Around line 618-626: Update the diagnostic handling around
response.diagnostics so only ProviderDiagnosticSeverity values that are
designated build-blocking return failure. Preserve non-blocking warning/info
diagnostics without failing the build, and ensure failure_kind strings such as
ProviderLowererFailed are used only for blocking severities; align this logic
with the advertised max_diagnostic_count behavior.
- Around line 1072-1094: Align response_limits() and host_limits() so the
requested response envelope matches the enforced host envelope. Either enforce
the 64 KiB response_limits().max_total_response_bytes value in
preflight_limits(), or, if 3 MiB is the intended limit, update
host_limits().max_output_bytes accordingly; ensure the two limits cannot drift.
- Around line 487-514: Validate provider roles in check_artifacts before
read_provider_artifact constructs paths, rejecting empty or unsafe values
containing path separators, traversal segments, or absolute/drive-path syntax.
Preserve the existing uniqueness checks and ensure invalid roles produce the
established manifest-validation failure rather than reaching filesystem reads.
- Around line 1011-1023: Replace the any-based acceptance check in the provider
verification gate with a single lookup of the “outcome” key, then require that
sole binding to equal “accepted”; preserve the existing
ProviderVerificationRejected error for missing, non-text, or non-accepted
outcomes.
In `@crates/edict-cli/src/main.rs`:
- Around line 734-753: Replace the hard-coded processed-input count in
run_build_request’s status_record call with a meaningful build-specific value,
preferably derived from the application manifest’s source count. If that value
cannot be obtained there, document the build count semantics in the CLI
documentation so the published status field is unambiguous and comparable across
commands.
- Around line 415-427: Use the single "compilerSettings" record type for all
operations, including Build, by removing the Operation::Build-specific
expected_record_type branch and validating settings.record_type against that
constant. Update the corresponding published schema and documentation references
so the contract no longer permits "settings" for build.
- Around line 482-506: Update the Operation::Build validation to reject
non-default directory_extensions values, matching the stated build contract, and
ensure run_build_request does not accept or silently ignore custom directory
extensions. Consolidate the duplicate application-required validation so one
owner validates and returns the application path, removing the unreachable
re-validation in the later build flow.
In `@crates/edict-cli/tests/jsonl_cli.rs`:
- Around line 57-82: The test
build_accepts_application_request_without_compiler_input_records only proves
manifest-read failure, so add executable Hello Echo end-to-end cases covering
successful outputs and fail-closed provider refusal, non-accepted verification,
and diagnostics, or downgrade and split CLI-REQ-015 accordingly. Also create the
missing application path under a TempDir instead of relying on the bare relative
path "missing-edict-application.json".
In `@crates/edict-syntax/src/contract_bundle.rs`:
- Around line 575-603: Document the trust boundary in the (None, Some(actual))
arm that derives expected_lawpacks from actual.lawpacks: explain that this is
safe only because the later supplied_lawpacks check corroborates the closure,
and state that callers must populate lawpacks from a source independent of the
provider-asserted Target IR closure. Keep the existing source_core validation
and closure comparison behavior unchanged.
In `@crates/edict-syntax/src/lawpack_adapter.rs`:
- Around line 706-723: Remove the local DIGEST_FRAME and digest_value
implementation in crates/edict-syntax/src/lawpack_adapter.rs:706-723, replacing
its callers with crate::canonical::digest_canonical_value as used by lawpack.rs.
Remove the duplicated review-string implementation at
crates/edict-syntax/src/lawpack_adapter.rs:877-885, add or reuse a shared
sha256_review_string in the canonical module, and update both this site and
lawpack.rs:1921-1930 to call it.
- Around line 231-240: Update the obstruction coordinate collection in the
adapter effects loop to gather mapped target obstruction names from each failure
mapping’s values, rather than extending with the lawpack failure-ident keys.
Preserve the existing local effect and TargetEffectLowering construction, and
populate TargetIrLoweringFacts.obstruction_coordinates with the declared target
coordinates used for obstruction-arm validation.
In `@crates/edict-syntax/src/lawpack.rs`:
- Around line 759-791: Update parse_array_field to accept a coordinate projector
for the parsed T value and use it for duplicate detection after parse returns,
removing the second traversal through export_coordinate and the raw
CanonicalValue. Update each caller to provide the appropriate coordinate
accessor for its parsed item type, preserving required nonempty coordinate
validation and DuplicateIdentity behavior.
In `@crates/edict-syntax/tests/lawpack.rs`:
- Around line 198-221: Extract the repeated decode–mutate–re-encode–validate
setup from lawpack_adapter_requires_a_typed_target_configuration_reference,
lawpack_adapter_requires_complete_exported_effect_coverage, and
lawpack_adapter_corroborates_footprint_cost_and_failure_obligations into a
shared test helper. Have the helper accept a mutation closure and expected
LawpackAdapterFailureKind, preserve the existing digest rebinding order, bundle
construction, canonical encoding, decode_lawpack_adapter call, and single-kind
assertion, then replace each test’s duplicated choreography with the helper
invocation.
In `@docs/abi/edict-lawpack-adapter.cddl`:
- Around line 46-47: Align the write-class vocabulary across all affected sites:
in docs/abi/edict-lawpack-adapter.cddl lines 46-47, replace "modify" with
"replace" and add "custom"; in crates/edict-syntax/src/lawpack_adapter.rs lines
639-655, preserve the existing accepted set and thread the effect coordinate
into the InvalidWriteClass failure path instead of using the hardcoded constant;
in fixtures/provider-contracts/v1/edict-provider-contracts.cddl lines 585-586,
make no direct edit and regenerate the fixture with the provider-contract-pack
xtask after updating the upstream schema.
In `@docs/topics/cli/README.md`:
- Around line 24-44: Expand the build-request documentation around the
implemented operations to state that its settings record uses type "settings",
not "compilerSettings", and add a concrete valid edict.application/v1 manifest
example covering coordinate, sources, lawpacks, target, and outputDirectory.
Document that outputDirectory is resolved relative to the manifest’s parent and
contains the two emitted artifacts, or alternatively add the authoritative JSON
Schema under docs/schemas and link it from the Stream Contract Artifacts table.
In `@docs/topics/cli/test-plan.md`:
- Line 99: Update the Fixtures table in test-plan.md to add entries for the
standalone Hello Echo lawpack corpus under fixtures/lawpack/hello-echo/,
covering its manifest, exports, adapter, target-IR, configuration, and .sha256
sidecars. Name cargo xtask lawpack-goldens --check as the oracle, matching the
existing CLI golden-check row style, and ensure CLI-TP-027 references the
documented fixture entry so requirements, case IDs, fixtures, and oracles remain
linked.
In `@docs/topics/lawpacks/test-plan.md`:
- Line 74: Update the TP-008 oracle and evidence entries to include the
InvalidWriteClass adapter failure mode, with a dedicated case ID, requirement
linkage, fixture, and executable oracle. Add or reference a negative test
covering the decoder/edict-lawpack-adapter.cddl write-class vocabulary mismatch,
and assert the stable LawpackAdapterFailureKind::InvalidWriteClass result rather
than only checking for a generic failure.
In `@xtask/src/lawpack_goldens.rs`:
- Around line 99-102: Update the digest_value call that computes
configuration_digest to use the declared CAUSAL_CELL_CONFIGURATION_COORDINATE
instead of the API-version string, while preserving the existing
configuration_value input and error propagation.
---
Outside diff comments:
In `@crates/edict-cli/src/main.rs`:
- Around line 507-520: Update the Operation::Check validation to handle
settings.application separately from the existing emit, compiler_context, and
target validation, using a dedicated message that identifies application as a
build-only setting. Keep the shared project-only message limited to emit,
compilerContext, and target.
- Around line 378-385: Update the request validation in run_request around the
existing inputs.is_empty() check and the build short-circuit to reject any Build
request with compiler-input records, using the established
CliFailure/MissingInput error pattern or an appropriate validation error.
Preserve the current requirement for non-Build operations to include at least
one input, and ensure invalid Build requests do not reach run_build_request with
inputs silently discarded.
In `@crates/edict-provider-schema/src/recursive_dispatch.rs`:
- Around line 666-704: Extract the duplicated map-entry occurrence logic from
specialize_choice into a shared helper such as single_matching_entry, returning
the sole matching value, None when absent, and an error for duplicates. Use it
in both the tagged_choice and required_key_choice branches, preserving their
existing fail-closed behavior and discriminator/type handling.
In `@docs/schemas/edict.compiler-settings.v1.schema.json`:
- Around line 189-218: Add "required": ["operation"] to the condition schemas of
all three operation branches, including the existing check branch and the
project/build branches identified by their operation const values. Keep each
branch’s then constraints unchanged so they only apply when operation is
explicitly present.
In `@xtask/src/tests.rs`:
- Around line 2020-2043: Update the compiler-settings schema assertions around
record_type, operation, and application to enforce the exact public contract:
compare enum arrays against the complete expected values rather than using
json_string_array_contains, and inspect application to require the expected
object type and all required validation fields. Keep the existing
missing-property diagnostics while rejecting unsupported enum values and
incomplete application definitions.
🪄 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: 000c492f-9b55-4e39-baa1-dc727174e497
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (67)
CHANGELOG.mdcrates/edict-cli/Cargo.tomlcrates/edict-cli/src/application_build.rscrates/edict-cli/src/main.rscrates/edict-cli/tests/jsonl_cli.rscrates/edict-provider-schema/src/contract_pack.rscrates/edict-provider-schema/src/recursive_dispatch.rscrates/edict-provider-schema/tests/provider_contract_pack.rscrates/edict-provider-schema/tests/registry.rscrates/edict-syntax/src/contract_bundle.rscrates/edict-syntax/src/lawpack.rscrates/edict-syntax/src/lawpack_adapter.rscrates/edict-syntax/src/lib.rscrates/edict-syntax/src/parser.rscrates/edict-syntax/src/target_profile.rscrates/edict-syntax/tests/contract_bundle.rscrates/edict-syntax/tests/lawpack.rscrates/edict-syntax/tests/target_profile.rsdocs/REQUIREMENTS.mddocs/SPEC_edict-lawpack-abi-v1.mddocs/SPEC_edict-target-profile-abi-v1.mddocs/abi/edict-lawpack-adapter.cddldocs/abi/edict-target-profile.cddldocs/schemas/edict.compiler-settings.v1.schema.jsondocs/topics/cli/README.mddocs/topics/cli/test-plan.mddocs/topics/compiler-spine/test-plan.mddocs/topics/contract-bundles/README.mddocs/topics/contract-bundles/test-plan.mddocs/topics/fixtures/test-plan.mddocs/topics/lawpacks/README.mddocs/topics/lawpacks/test-plan.mddocs/topics/providers/test-plan.mddocs/topics/target-ir/test-plan.mddocs/topics/target-profiles/README.mddocs/topics/target-profiles/test-plan.mdfixtures/README.mdfixtures/lawpack/causal-cell/README.mdfixtures/lawpack/causal-cell/adapter.cborfixtures/lawpack/causal-cell/adapter.sha256fixtures/lawpack/causal-cell/echo-operation-configuration.cborfixtures/lawpack/causal-cell/echo-operation-configuration.sha256fixtures/lawpack/causal-cell/exports.cborfixtures/lawpack/causal-cell/exports.sha256fixtures/lawpack/causal-cell/manifest.cborfixtures/lawpack/causal-cell/manifest.sha256fixtures/lawpack/hello-echo/README.mdfixtures/lawpack/hello-echo/adapter.cborfixtures/lawpack/hello-echo/adapter.sha256fixtures/lawpack/hello-echo/create-greeting.core.cborfixtures/lawpack/hello-echo/create-greeting.core.sha256fixtures/lawpack/hello-echo/create-greeting.edictfixtures/lawpack/hello-echo/create-greeting.target-ir.cborfixtures/lawpack/hello-echo/create-greeting.target-ir.sha256fixtures/lawpack/hello-echo/echo-operation-configuration.cborfixtures/lawpack/hello-echo/echo-operation-configuration.sha256fixtures/lawpack/hello-echo/exports.cborfixtures/lawpack/hello-echo/exports.sha256fixtures/lawpack/hello-echo/manifest.cborfixtures/lawpack/hello-echo/manifest.sha256fixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.jsonfixtures/providers/components/inventory.jsonxtask/src/lawpack_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/**/*.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/SPEC_edict-lawpack-abi-v1.mddocs/REQUIREMENTS.mddocs/topics/target-profiles/README.mddocs/topics/target-ir/test-plan.mddocs/topics/compiler-spine/test-plan.mddocs/topics/contract-bundles/README.mddocs/SPEC_edict-target-profile-abi-v1.mddocs/topics/target-profiles/test-plan.mddocs/topics/providers/test-plan.mddocs/topics/cli/README.mddocs/topics/lawpacks/README.mddocs/topics/cli/test-plan.mddocs/topics/lawpacks/test-plan.mddocs/topics/fixtures/test-plan.mddocs/topics/contract-bundles/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/target-profiles/README.mddocs/topics/target-ir/test-plan.mddocs/topics/compiler-spine/test-plan.mddocs/topics/contract-bundles/README.mddocs/topics/target-profiles/test-plan.mddocs/topics/providers/test-plan.mddocs/topics/cli/README.mddocs/topics/lawpacks/README.mddocs/topics/cli/test-plan.mddocs/topics/lawpacks/test-plan.mddocs/topics/fixtures/test-plan.mddocs/topics/contract-bundles/test-plan.md
🪛 LanguageTool
fixtures/lawpack/hello-echo/README.md
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...omain-framed identity. - exports.cbor is the canonical export surface. - `export...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~10-~10: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ical export surface. - exports.sha256 is its hello.echo.exports/v1 domain-fram...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~11-~11: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...omain-framed identity. - adapter.cbor is the canonical direct declarative Echo t...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~12-~12: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...Echo target adapter. - adapter.sha256 is its manifest-bound domain-framed identi...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~13-~13: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .... - echo-operation-configuration.cbor is the target-owned, adapter-bound progr...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/SPEC_edict-lawpack-abi-v1.md
[grammar] ~269-~269: Ensure spelling is correct
Context: ...udget maps must exactly cover the owning lawpack's exported operation profiles, runtime ef...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
fixtures/lawpack/causal-cell/README.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...le capability surface. - adapter.cbor and adapter.sha256 bind the direct declar...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~18-~18: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: .... - echo-operation-configuration.cbor and its digest sidecar bind the generic E...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/topics/cli/README.md
[grammar] ~28-~28: Ensure spelling is correct
Context: ...es one exact Edict source, its complete lawpack closure, the selected target profile an...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
docs/topics/lawpacks/README.md
[grammar] ~38-~38: Ensure spelling is correct
Context: ...rrent executable Rust surfaces touching lawpacks are: - parser support for `ImportKind:...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
CHANGELOG.md
[grammar] ~35-~35: Ensure spelling is correct
Context: ...elected and digest-bound by a validated lawpack, must completely discharge exported o...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (77)
crates/edict-syntax/src/parser.rs (1)
121-121: LGTM!crates/edict-syntax/tests/lawpack.rs (1)
1-927: LGTM!crates/edict-syntax/src/lawpack.rs (5)
331-370: LGTM!Also applies to: 382-466
468-655: LGTM!
657-723: LGTM!
1150-1665: LGTM!
1667-1946: LGTM!crates/edict-syntax/src/lib.rs (1)
79-80: LGTM!Also applies to: 139-153
crates/edict-syntax/src/lawpack_adapter.rs (1)
159-191: LGTM!Also applies to: 268-300, 403-475, 477-528, 530-586, 613-637
docs/abi/edict-lawpack-adapter.cddl (1)
1-45: LGTM!Also applies to: 48-49
docs/abi/edict-target-profile.cddl (1)
33-36: LGTM!fixtures/provider-contracts/v1/edict-provider-contracts.cddl (1)
539-584: LGTM!Also applies to: 587-589, 623-626
docs/topics/lawpacks/README.md (1)
8-12: LGTM!Also applies to: 34-52, 64-86, 107-110
docs/topics/lawpacks/test-plan.md (1)
10-22: LGTM!Also applies to: 33-35, 45-73, 75-76, 85-96
docs/topics/fixtures/test-plan.md (1)
14-21: LGTM!Also applies to: 31-36, 56-57, 66-71, 85-85
docs/topics/compiler-spine/test-plan.md (1)
52-52: LGTM!Also applies to: 87-87
docs/topics/contract-bundles/README.md (2)
73-82: LGTM!
83-83: 📐 Maintainability & Code QualityNo change needed.
[BUNDLE-REQ-010]is defined indocs/topics/contract-bundles/test-plan.mdand cited byBUNDLE-TP-016.crates/edict-syntax/src/target_profile.rs (2)
8-8: LGTM!Also applies to: 159-169
70-70: 📐 Maintainability & Code QualityCall out the public API break from this variant rename.
TargetProfileConformanceFailureKindis re-exported bycrates/edict-syntax/src/lib.rs; replacingDeferredLawpackAdapterAbiUnsupportedwithUnsupportedLawpackAdapterAbibreaks downstream exhaustive matches. Please record the breaking rename/migration inCHANGELOG.mdand confirm it is intentional for this release line.CHANGELOG.md (1)
13-46: LGTM!Also applies to: 68-78
docs/REQUIREMENTS.md (1)
90-90: LGTM!docs/SPEC_edict-lawpack-abi-v1.md (1)
267-274: LGTM!docs/SPEC_edict-target-profile-abi-v1.md (1)
101-116: LGTM!docs/topics/contract-bundles/test-plan.md (1)
46-48: LGTM!Also applies to: 71-77
docs/topics/target-ir/test-plan.md (1)
55-55: LGTM!Also applies to: 107-107
docs/topics/target-profiles/README.md (1)
58-58: LGTM!Also applies to: 69-72
docs/topics/target-profiles/test-plan.md (1)
14-14: LGTM!Also applies to: 48-48, 72-72
crates/edict-syntax/tests/target_profile.rs (1)
10-10: LGTM!Also applies to: 42-42, 133-154
crates/edict-provider-schema/src/contract_pack.rs (1)
35-38: LGTM!Also applies to: 72-72, 296-296, 306-306
crates/edict-provider-schema/src/recursive_dispatch.rs (2)
262-264: LGTM!Traced
required_key_choiceagainstREQUIRED_KEY_RECURSIVE_SCHEMAinregistry.rsline by line: the discriminator search correctly fails closed on optional/overlapping/wildcard keys (exact_text_map_key_requirementsrejects non-literal keys and duplicate-in-arm keys outright), and the "required-in-exactly-one-arm, absent-from-the-other" criterion is sound given CDDL's closed-map default (an arm that doesn't declare the key can never validate a value that carries it).Also applies to: 457-463, 490-539
688-704: LGTM!Verified against
recursive_required_key_dispatch_accepts_compatible_untagged_armsandchecked_target_ir_root_constructs_through_required_key_dispatchincrates/edict-provider-schema/tests/registry.rs— presence/absence dispatch and the >1-occurrence rejection both match expected outcomes for closed/legacy and nested-child cases.crates/edict-provider-schema/tests/provider_contract_pack.rs (1)
28-28: LGTM!Contract-count bump and new
lawpack-adapterfixture/representative-instance wiring are consistent withcontract_pack.rs'sCONTRACT_BINDINGSextension.Also applies to: 41-42, 74-74, 518-518, 566-570
crates/edict-provider-schema/tests/registry.rs (1)
35-47: LGTM!
REQUIRED_KEY_RECURSIVE_SCHEMA'sclosed/legacyarms share the non-discriminatingkind: "artifact"field on purpose, exercising the code path that must skip over shared-but-non-discriminating required keys before landing onsemanticClosure/valueas the true discriminator. Good coverage of the recursive nested-child case too.Also applies to: 629-666
crates/edict-syntax/tests/contract_bundle.rs (1)
376-376: LGTM!Traced each new/updated test against the corresponding
corroborate_target_ir_semantic_closurematch arm incontract_bundle.rs: exact provider-supplied closure acceptance, Core-substitution rejection, lawpack-substitution rejection, missing-closure rejection, and the updatedsource_core-specific error field on the mismatched-closure case all line up with the actual control flow.Also applies to: 477-504, 727-800, 819-828
docs/topics/providers/test-plan.md (1)
144-144: LGTM!
PROVIDERS-TP-048's expanded failure-category list and the two newly cited case IDs (recursive_required_key_dispatch_accepts_compatible_untagged_arms,checked_target_ir_root_constructs_through_required_key_dispatch) match the new tests added incrates/edict-provider-schema/tests/registry.rsin this same change, satisfying the requirement to update the test plan alongside the behavior it documents.docs/topics/cli/test-plan.md (2)
45-45: The overclaimedimplementedstatus on CLI-REQ-015 is already covered in my note oncrates/edict-cli/tests/jsonl_cli.rsLines 57-82, where the missing evidence actually lives.
16-24: LGTM!Also applies to: 114-115
crates/edict-cli/src/application_build.rs (7)
29-78: LGTM!
383-420: LGTM!
422-460: LGTM!Also applies to: 462-485
872-969: LGTM!
1096-1122: LGTM!
311-339: 🎯 Functional CorrectnessNo change needed:
ValidatedTargetProviderManifest<'_>isCopy, so the double by-value use is legal.
971-996: 🔒 Security & PrivacyNo changes needed: the digest comparison is domain-framed and string-rendered consistently.
manifest_artifact.resource.digestis only validated as a lowercasesha256:hex string; the code does not publish a plain artifact-byte digest semantics claim, and the provider digest path usesCORE_DIGEST_FRAME,TARGET_PROFILE_API_VERSION, andrendered_digestformatting is lowercase inedict-cli.docs/schemas/edict.compiler-settings.v1.schema.json (1)
13-23: LGTM!docs/topics/cli/README.md (1)
87-94: LGTM!Also applies to: 124-126, 144-148
fixtures/lawpack/hello-echo/manifest.sha256 (1)
1-1: LGTM!fixtures/provider-contracts/v1/manifest.json (1)
6-12: 🗄️ Data Integrity & IntegrationGenerated artifact digest is consistent.
fixtures/providers/components/inventory.json (1)
10-10: 🗄️ Data Integrity & IntegrationNo change needed. The
sourceDigestis a validsha256:+ 64 lowercase hex value, and it is not the onlysourceDigestchange in the repo.fixtures/lawpack/hello-echo/create-greeting.edict (1)
1-31: LGTM!fixtures/lawpack/hello-echo/create-greeting.target-ir.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/create-greeting.target-ir.sha256 (1)
1-1: LGTM!fixtures/lawpack/hello-echo/echo-operation-configuration.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/echo-operation-configuration.sha256 (1)
1-1: LGTM!fixtures/lawpack/hello-echo/exports.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/exports.sha256 (1)
1-1: LGTM!fixtures/lawpack/hello-echo/manifest.cbor (1)
1-1: LGTM!xtask/src/main.rs (1)
6-6: LGTM!Also applies to: 26-26, 95-95, 134-134, 154-165, 206-206
fixtures/lawpack/causal-cell/exports.sha256 (1)
1-2: LGTM!fixtures/lawpack/causal-cell/manifest.cbor (1)
1-2: LGTM!fixtures/lawpack/causal-cell/manifest.sha256 (1)
1-2: LGTM!fixtures/lawpack/hello-echo/README.md (1)
1-48: LGTM!fixtures/lawpack/hello-echo/adapter.cbor (1)
1-2: LGTM!fixtures/lawpack/hello-echo/adapter.sha256 (1)
1-2: LGTM!fixtures/lawpack/hello-echo/create-greeting.core.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/create-greeting.core.sha256 (1)
1-2: LGTM!xtask/src/provider_contract_pack.rs (1)
12-12: LGTM!Also applies to: 34-42
xtask/src/tests.rs (1)
29-29: LGTM!Also applies to: 75-80, 140-140, 275-275
fixtures/README.md (1)
47-48: LGTM!fixtures/lawpack/causal-cell/README.md (2)
3-4: 📐 Maintainability & Code QualityVerify the claimed standalone consumer.
The supplied implementation shows
causal_cell_golden_artifactsusing this fixture to compile an internal source witness, while the standalone Hello Echo build evidence usesfixtures/lawpack/hello-echo/. Please either identify the actual standalone consumer or describe this fixture as generator-validated; otherwise users may treat an internal golden as a currently supported application input.Also applies to: 33-37
1-2: LGTM!Also applies to: 5-32
fixtures/lawpack/causal-cell/adapter.cbor (1)
1-1: LGTM!fixtures/lawpack/causal-cell/adapter.sha256 (1)
1-1: LGTM!fixtures/lawpack/causal-cell/echo-operation-configuration.cbor (1)
1-1: LGTM!fixtures/lawpack/causal-cell/echo-operation-configuration.sha256 (1)
1-1: LGTM!fixtures/lawpack/causal-cell/exports.cbor (1)
1-1: LGTM!
|
@codex please review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
crates/edict-cli/src/application_build.rs (1)
705-713: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEvery diagnostic is still a hanging offence.
ProviderDiagnosticSeverityexists; you advertisemax_diagnostic_countinresponse_limits(); and then any singleinfo-severity note from the lowerer kills the build withProviderLowererFailed. Same shape at Lines 815-823 for the verifier, where it is escalated all the way toProviderVerificationRejected. Gate on the severities that are actually contracted to block.🤖 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-cli/src/application_build.rs` around lines 705 - 713, Update the lowerer error handling around the diagnostic check to reject only diagnostics whose ProviderDiagnosticSeverity is contractually blocking, while allowing informational and other non-blocking diagnostics and preserving the existing failure response for blocking ones. Apply the same severity-aware filtering to the verifier path that currently returns ProviderVerificationRejected, using the advertised max_diagnostic_count contract rather than treating any non-empty diagnostics collection as fatal.
🤖 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 1686-1691: Update the resource fixture helper resource so digest
constructs the sha256 prefix once and repeats only the two-character hexadecimal
digest body 32 times, producing a valid 64-character digest while preserving the
existing ResourceRef behavior.
- Around line 1160-1185: Update the application output locking flow around
lock_path and try_lock so the published output never retains
.edict-application-build.lock. Prefer writing the build into a uniquely created
temporary output directory and atomically renaming it into place after success,
while preserving contention failures; alternatively, ensure production cleanup
always removes the lock file before completion.
In `@crates/edict-cli/tests/jsonl_cli.rs`:
- Around line 87-145: Update
build_rejects_compiler_input_records_instead_of_ignoring_them and
build_rejects_unused_directory_extension_settings to reference the
guaranteed-missing application path under temp_tree, matching the sibling test
setup. Remove both temporary trees at the end of each test, consistent with the
cleanup at line 84. In build_rejects_unused_directory_extension_settings, also
assert the diagnostic command is "build" alongside the existing kind assertion.
In `@crates/edict-syntax/src/lawpack.rs`:
- Around line 761-821: The raw recursion in validate_pure_callees_in_value
incorrectly interprets arbitrary literal maps as call expressions. Replace this
scan with traversal of parsed expression/predicate nodes used by
validate_pure_function_callees, validating callee coordinates only for actual
call nodes; ensure traversal does not recurse into const.value literal contents
as executable syntax.
In `@crates/edict-syntax/src/target_ir.rs`:
- Around line 686-699: Update the obstruction lowering in `lower_to_target_ir`
so mapped failures resolving to the same target coordinate, including collisions
with unmapped failure names, are detected before collecting into `BTreeMap`.
Reject the lowering explicitly with the existing failure/error path, ensuring
`unsupported_obstructions` and artifact emission are not reached with a
deduplicated map.
In `@crates/edict-syntax/tests/provider.rs`:
- Around line 253-271: Update provider_manifest_rejects_path_like_artifact_roles
to assert the report’s failure_kinds contains UnsafeArtifactRole for the
artifacts.role field path, rather than only checking Invalid. Reuse the existing
failure_kinds structure and extend unsafe_role cases to cover colon-containing
and control-character roles, preserving the current path-like cases.
In `@docs/topics/cli/test-plan.md`:
- Line 45: Update the CLI-REQ-015 status in the test plan to partial/planned
unless executable repository-owned evidence exists for the complete build flow.
If implementing it, add the missing CLI-TP-028 end-to-end fixture and test
covering closure validation, provider lowering, independent verification, and
accepted package/report publication, then retain implemented status only with
that evidence.
In `@xtask/src/tests.rs`:
- Around line 1999-2006: Update assert_compiler_settings_projection_properties
to compare the projected emit enum against the exact expected list, including
its ordering, rather than merely checking that each value is present. Preserve
the existing operation assertion’s exact-contract behavior and use the expected
values syntax, diagnostics, core, targetIr, and digests.
---
Duplicate comments:
In `@crates/edict-cli/src/application_build.rs`:
- Around line 705-713: Update the lowerer error handling around the diagnostic
check to reject only diagnostics whose ProviderDiagnosticSeverity is
contractually blocking, while allowing informational and other non-blocking
diagnostics and preserving the existing failure response for blocking ones.
Apply the same severity-aware filtering to the verifier path that currently
returns ProviderVerificationRejected, using the advertised max_diagnostic_count
contract rather than treating any non-empty diagnostics collection as fatal.
🪄 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: 1f9089e6-5417-490a-88ba-7f0a43ff7f65
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
CHANGELOG.mdcrates/edict-cli/Cargo.tomlcrates/edict-cli/src/application_build.rscrates/edict-cli/src/main.rscrates/edict-cli/tests/jsonl_cli.rscrates/edict-syntax/src/authority_facts.rscrates/edict-syntax/src/canonical.rscrates/edict-syntax/src/contract_bundle.rscrates/edict-syntax/src/lawpack.rscrates/edict-syntax/src/lawpack_adapter.rscrates/edict-syntax/src/lib.rscrates/edict-syntax/src/provider.rscrates/edict-syntax/src/target_ir.rscrates/edict-syntax/tests/contract_bundle.rscrates/edict-syntax/tests/lawpack.rscrates/edict-syntax/tests/operation_prerequisites.rscrates/edict-syntax/tests/provider.rscrates/edict-syntax/tests/provider_lowering.rscrates/edict-syntax/tests/target_ir.rsdocs/abi/edict-lawpack-adapter.cddldocs/schemas/edict.compiler-settings.v1.schema.jsondocs/topics/cli/README.mddocs/topics/cli/test-plan.mddocs/topics/lawpacks/test-plan.mdfixtures/lawpack/causal-cell/adapter.cborfixtures/lawpack/causal-cell/adapter.sha256fixtures/lawpack/causal-cell/manifest.cborfixtures/lawpack/causal-cell/manifest.sha256fixtures/lawpack/hello-echo/create-greeting.target-ir.cborfixtures/lawpack/hello-echo/create-greeting.target-ir.sha256fixtures/provider-contracts/v1/edict-provider-contracts.cddlfixtures/provider-contracts/v1/manifest.jsonfixtures/providers/components/inventory.jsonxtask/src/goldens.rsxtask/src/lawpack_goldens.rsxtask/src/tests.rs
💤 Files with no reviewable changes (1)
- crates/edict-cli/Cargo.toml
📜 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/cli/test-plan.mddocs/topics/cli/README.mddocs/topics/lawpacks/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/test-plan.mddocs/topics/cli/README.mddocs/topics/lawpacks/test-plan.md
🔇 Additional comments (47)
crates/edict-syntax/tests/operation_prerequisites.rs (1)
52-52: LGTM!crates/edict-syntax/tests/provider_lowering.rs (2)
98-98: LGTM!
137-137: LGTM!crates/edict-syntax/tests/target_ir.rs (4)
231-231: LGTM!
248-248: LGTM!
694-699: LGTM!
950-950: LGTM!crates/edict-syntax/tests/lawpack.rs (3)
141-150: LGTM!
226-245: LGTM!
535-592: LGTM!docs/topics/cli/README.md (2)
36-38: 🎯 Functional CorrectnessVerify the build discriminator before publishing this example.
Line 37 still uses
"type":"compilerSettings", while this route is documented as a settings-only record. The prior dispatcher contract reportedly requires"type":"settings"; if that remains true, this copy-paste example is rejected before the build starts. Align the example and its golden fixture with the actual dispatcher contract.As per coding guidelines, documentation must use concrete, valid examples and exact public facts.
rg -n -C 4 'InvalidSettings|compilerSettings|type.*settings|operation.*build' crates/edict-cli/src/main.rs docs/schemas/edict.compiler-settings.v1.schema.jsonSource: Coding guidelines
24-35: LGTM!Also applies to: 40-80, 123-130, 160-162, 180-181
crates/edict-syntax/src/provider.rs (1)
134-134: LGTM!Also applies to: 253-259, 342-347
crates/edict-syntax/tests/contract_bundle.rs (1)
376-376: LGTM!Also applies to: 477-504, 543-543, 728-738, 741-763, 765-783, 786-798, 801-829
crates/edict-syntax/src/contract_bundle.rs (1)
575-607: LGTM!Also applies to: 625-627
docs/abi/edict-lawpack-adapter.cddl (1)
1-49: LGTM!fixtures/provider-contracts/v1/edict-provider-contracts.cddl (1)
539-589: LGTM!Also applies to: 623-626
docs/topics/lawpacks/test-plan.md (1)
10-22: LGTM!Also applies to: 33-35, 45-53, 54-62, 63-76, 85-96
CHANGELOG.md (1)
11-18: LGTM!Also applies to: 77-87
docs/topics/cli/test-plan.md (1)
100-100: Document the Hello Echo fixture corpus and its oracle.The referenced Hello Echo integration witness still has no corresponding Fixtures-table row linking its lawpack artifacts to
cargo xtask lawpack-goldens --check.As per coding guidelines, “update
test-plan.mdbefore or alongside tests with requirement IDs, case IDs, fixtures, and oracles”.Source: Coding guidelines
crates/edict-syntax/src/authority_facts.rs (1)
16-17: LGTM!crates/edict-syntax/src/canonical.rs (1)
187-211: LGTM!crates/edict-syntax/src/lawpack.rs (1)
12-13: LGTM!crates/edict-syntax/src/lib.rs (1)
79-80: LGTM!Also applies to: 110-115, 139-153
docs/schemas/edict.compiler-settings.v1.schema.json (1)
17-22: LGTM!Also applies to: 175-222
fixtures/lawpack/causal-cell/manifest.sha256 (1)
1-1: LGTM!fixtures/lawpack/hello-echo/create-greeting.target-ir.cbor (1)
1-1: LGTM!fixtures/lawpack/hello-echo/create-greeting.target-ir.sha256 (1)
1-1: LGTM!fixtures/provider-contracts/v1/manifest.json (1)
6-12: LGTM!fixtures/providers/components/inventory.json (1)
10-10: LGTM!crates/edict-syntax/src/lawpack_adapter.rs (2)
11-13: LGTM!Also applies to: 230-240, 721-732
365-368: LGTM!Also applies to: 480-543, 654-670
crates/edict-syntax/src/target_ir.rs (1)
113-117: LGTM!Also applies to: 119-135
crates/edict-cli/src/application_build.rs (5)
113-119: LGTM!Also applies to: 154-159
366-445: LGTM!
606-624: LGTM!Also applies to: 957-996
1186-1296: LGTM!Also applies to: 1298-1416
1470-1685: LGTM!Also applies to: 1692-1716
crates/edict-cli/src/main.rs (1)
378-385: LGTM!Also applies to: 423-423, 486-534, 1078-1078
crates/edict-cli/tests/jsonl_cli.rs (1)
58-85: LGTM!Also applies to: 1312-1312
xtask/src/tests.rs (1)
1984-1998: LGTM!Also applies to: 2045-2075
xtask/src/lawpack_goldens.rs (2)
59-59: 🗄️ Data Integrity & Integration | ⚡ Quick winCorrect fix — now prove the other fixture did not inherit the same bug.
Framing the configuration digest with the declared coordinate is right, and it matches
crates/edict-cli/src/application_build.rsLine 616, which recomputes it asprovider_digest(&reference.id, bytes). Thecausal-celladapter fixture agrees: itstargetConfiguration.idisecho.operation-lowering-configuration/v1.Two things I want evidence for before this is closed. First, that
TARGET_CONFIGURATION_COORDINATEused at Line 423 is byte-identical to whateverhello_echo_adapterwrites intotargetConfiguration.id— that is the exact mismatch you just repaired forcausal-cell, and the Hello Echo path is the one the CLI actually builds. Second,CAUSAL_CELL_CONFIGURATION_COORDINATEno longer names a causal-cell-specific resource; if both fixtures now declare the same configuration coordinate over different bytes, two distinct digests are framed under one domain. Legal, but the constant's name is now a lie and the collision should be deliberate.#!/bin/bash # What coordinate constants exist, and what do the adapter builders actually emit? rg -nP -C2 '(CAUSAL_CELL_)?(TARGET_)?CONFIGURATION_COORDINATE\s*:\s*&str' xtask/src/lawpack_goldens.rs ast-grep run --pattern 'fn hello_echo_adapter($$$) { $$$ }' --lang rust xtask/src/lawpack_goldens.rs ast-grep run --pattern 'fn causal_cell_adapter($$$) { $$$ }' --lang rust xtask/src/lawpack_goldens.rs # Same coordinate declared by both fixtures? rg -na -o 'echo\.operation-lowering-configuration/v1' fixtures/lawpackAlso applies to: 99-100
90-152: LGTM!Also applies to: 414-495
fixtures/lawpack/causal-cell/adapter.cbor (1)
1-1: LGTM!fixtures/lawpack/causal-cell/adapter.sha256 (1)
1-1: LGTM!fixtures/lawpack/causal-cell/manifest.cbor (1)
1-2: LGTM!xtask/src/goldens.rs (1)
349-364: LGTM!Also applies to: 366-381
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e3f52f9e6
ℹ️ 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 review |
|
@codex please review |
✅ Action performedReview finished.
|
|
Merge gate OPEN at |
There was a problem hiding this comment.
💡 Codex Review
edict/crates/edict-provider-schema/src/recursive_dispatch.rs
Lines 255 to 258 in 0595abf
Update the recursive-schema contract for required-key dispatch
This change newly admits recursive alternatives selected by the presence of a required key, but the providers topic README still states that every recursive alternative must expose one common singleton text discriminator and that missing discriminators reject. That is now false for the checked Target IR union, leaving the repository's current-contract documentation inconsistent with executable behavior; document the bounded two-arm required-key exception alongside the implementation.
AGENTS.md reference: AGENTS.md:L58-L60
ℹ️ 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".
| coordinate: required_nonempty_text(&fields, "coordinate", path)?, | ||
| definition: required_nonempty_text(&fields, "definition", path)?, |
There was a problem hiding this comment.
When a lawpack supplies an unbounded or otherwise invalid definition such as String, this branch accepts it solely because the text is non-empty and returns it from ValidatedLawpackBundle; the same opaque-string treatment is used for constant, parameter, return, effect, and payload types. Such a bundle bypasses the lawpack ABI's bounded Core-type contract and can expose unbounded values to compilation or provider processing, so these fields need to be parsed and validated against the bounded Core type universe before the bundle is considered valid.
Useful? React with 👍 / 👎.
| target_profile: ResourceRef { | ||
| coordinate: adapter.target_profile().id.clone(), | ||
| digest: Some(adapter.target_profile().digest_review_string()), | ||
| }, | ||
| target_ir_domain: adapter.target_ir().id.clone(), |
There was a problem hiding this comment.
When a lawpack manifest's acceptedTargetIr.digest is substituted and the manifest/source import are re-digested, preparation still succeeds because only the reference ID is copied into target_ir_domain; the digest is never consumed anywhere in the build path. Consequently the purportedly digest-locked Target IR ABI can be changed without affecting compilation or rejection, so the descriptor's complete resource identity must be compared with the selected target profile's Target IR identity before deriving trusted lowering facts.
Useful? React with 👍 / 👎.
| let loaded = loaded_lawpacks.first().ok_or_else(|| { | ||
| failure( | ||
| "InvalidApplicationConfig", | ||
| "the executable-operation build requires a root lawpack", | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
For an application whose root lawpack has dependencies, the graph validates their manifests and digests but this selection discards every loaded entry except the first; compilation and both provider semantic-input builders subsequently receive only that root's manifest, exports, adapter, and configuration. The lowerer and independent verifier therefore cannot bind or inspect the dependency bytes even though the CLI reports that the complete referenced closure was checked, so all reachable closure artifacts must be carried into the compilation/provider contracts rather than reduced to one LoadedLawpack.
Useful? React with 👍 / 👎.
| | CLI-TP-027 | implemented | Build request dispatch | CLI-REQ-015 | A `build` settings record is accepted without compiler input records and reaches application-config loading; a missing application config fails as a structured `ApplicationConfigReadFailed` build diagnostic rather than as invalid settings or missing compiler input. | build_accepts_application_request_without_compiler_input_records | crates/edict-cli/tests/jsonl_cli.rs | Covers the public request shape and dispatch boundary only. | | ||
| | CLI-TP-028 | planned | Verified application build | CLI-REQ-015 | A repository-owned provider-component fixture builds one exact source and complete lawpack closure, independently accepts the package, and publishes the canonical package/report pair. | - | - | The standalone external Hello Echo build is a green integration witness; this planned case makes the full crossing reproducible inside Edict's own automated suite. | |
There was a problem hiding this comment.
The new public build route is described as implemented, but this row explicitly leaves its only end-to-end success case planned; the added CLI tests stop at request dispatch and missing-config rejection, so no repository-owned executable evidence invokes the lowerer/verifier or verifies successful publication and rollback behavior. Add the deterministic provider-component fixture and mark this case implemented only after it passes, as required for nontrivial behavior claims.
AGENTS.md reference: AGENTS.md:L49-L60
Useful? React with 👍 / 👎.
| for (coordinate, effect) in &adapter.effects { | ||
| let local_effect = local_coordinate(&alias, &prefix, coordinate)?; | ||
| compiler_context = compiler_context | ||
| .with_effect_write_class(local_effect.clone(), effect.write_class.clone()); | ||
| obstruction_coordinates.extend(effect.failure_mappings.values().cloned()); |
There was a problem hiding this comment.
When an exported effect declares multiple named failures, preparation gives the compiler only its write class and never supplies the declared failure set or authority classes. A source can therefore handle only one failure, or author-map a non-domain-mappable failure, and still compile and lower successfully because Target IR includes only the arms that happened to appear in source; the resulting package has no behavior for the omitted provider failure. Propagate the exact exported failure contract into compiler checking and require exhaustive, authority-valid handlers before emitting Core.
Useful? React with 👍 / 👎.
| let lowerer_artifact = | ||
| unique_artifact(&provider_manifest, ProviderArtifactKind::Lowerer, None)?; | ||
| let verifier_artifact = | ||
| unique_artifact(&provider_manifest, ProviderArtifactKind::Verifier, None)?; |
There was a problem hiding this comment.
For a provider package whose sole lowerer or verifier differs from the digest-locked component references inside the selected target profile, these lookups still choose those package components solely by artifact kind. The host verifies each component against the provider manifest, but nothing corroborates that it is the lowerer or verifier authorized by the selected profile; valid multi-component packages are also rejected rather than resolved through those profile references. Decode the selected profile and use its exact lowerer/verifier resource identities to select and verify the invocation components.
Useful? React with 👍 / 👎.
| "local" => { | ||
| ensure_allowed_fields(fields, path, &["kind", "ref"]).map_err(as_pure_body_failure)?; | ||
| validate_local_ref( | ||
| required(fields, "ref", path).map_err(as_pure_body_failure)?, | ||
| &format!("{path}.ref"), | ||
| ) |
There was a problem hiding this comment.
When a pure helper expression contains a structurally valid local reference whose ID was never declared as a parameter, local, or preceding binding, this branch accepts it because validate_local_ref checks only that its three strings are non-empty. It likewise accepts a reused ID with a conflicting alpha name or type, so ValidatedLawpackBundle can expose a supposedly total Core helper that cannot be evaluated deterministically. Track the function scope and require every reference to match exactly one in-scope declaration before accepting the body.
Useful? React with 👍 / 👎.
| let mut loaded_lawpacks = Vec::with_capacity(config.lawpacks.len()); | ||
| for lawpack in &config.lawpacks { | ||
| loaded_lawpacks.push(load_lawpack(&root, lawpack)?); |
There was a problem hiding this comment.
A build manifest can fit thousands of repeated lawpack entries inside the 1 MiB config limit, and this loop loads four independently permitted 1 MiB artifacts for every entry before duplicate identities or graph validity are checked. A crafted local project can therefore force multi-gigabyte allocation and I/O even though each individual read is bounded. Impose a closure-count and aggregate-byte limit, and reject duplicates before retaining all artifact buffers.
Useful? React with 👍 / 👎.
| "int" => { | ||
| ensure_allowed_fields(fields, path, &["kind", "width", "value"]) | ||
| .map_err(as_pure_body_failure)?; | ||
| required_nonempty_text(fields, "width", path).map_err(as_pure_body_failure)?; | ||
| match required(fields, "value", path).map_err(as_pure_body_failure)? { | ||
| CanonicalValue::Integer(_) => Ok(()), |
There was a problem hiding this comment.
For an inline pure helper containing an integer constant such as {kind: "int", width: "U8", value: 256} or an unknown width, this branch accepts the value because it checks only for non-empty width text and an integer-shaped value. The canonical Core encoder rejects these values through parse_core_integer, so the lawpack loader can currently return a validated helper body that cannot correspond to valid canonical Core. Apply the same supported-width and range validation before accepting the helper.
Useful? React with 👍 / 👎.
1 participant
Compile standalone applications from exact Edict source and a validated lawpack, adapter, target, and provider closure. Invoke the checked provider lowerer and structurally separate verifier, then publish the accepted package and report as one staged pair.
Validation:
cargo xtask verify8a602e9bf2dfeae1a3bc033d299dc7b6b348a08c777756c7b1b855bd099dab938a72bff53649a448d7f880ed27bb21a52daab3a43eb2e0579e46ac35296e34ddThis compiles and verifies artifacts. It does not execute them or define an application runtime lifecycle.
Closes #169.