Repository navigation
Preserve ordered effect guards in explicitly selected Target IR v2 - #219
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (25)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📓 Path-based instructions (8)Source excerpt: For every nontrivial behavior, contract, workflow, release, schema, validation, or public-surface change, Edict expects a visible RED/GREEN cycle: mark planned rows as implemented only when executable evidence exists; Source...📄 CodeRabbit inference engine (docs/topics/tests/README.md) Files:
Tests must assert software behavior and stable error kinds or structured artifacts, not implementation details, prose, paths, or merely `is_err()`; documentation-tool tests may test validator behavior.📄 CodeRabbit inference engine (AGENTS.md) Files:
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.📄 CodeRabbit inference engine (AGENTS.md) Files:
Never amend Git commits, use `git rebase` without explicit user approval, or force any Git operation; use new commits and regular merge commits instead.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: Fixtures should be reused across stages when the semantic artifact is the same:📄 CodeRabbit inference engine (docs/topics/tests/README.md) Files:
Topic shelves document landed behavior: `README.md` describes current HEAD truth, `test-plan.md` records verification and known gaps, and optional architecture or rationale pages contain durable supporting information.📄 CodeRabbit inference engine (AGENTS.md) Files:
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 gene...📄 CodeRabbit inference engine (AGENTS.md) Files:
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.📄 CodeRabbit inference engine (AGENTS.md) Files:
🧠 Learnings (1)📓 Common learnings🪛 ast-grep (0.45.3)scripts/consumer-witnesses/jedit-state-read.py[error] 16-19: Command coming from incoming request (subprocess-from-request) [info] 17-17: use jsonify instead of json.dumps for JSON output (use-jsonify) [info] 31-31: use jsonify instead of json.dumps for JSON output (use-jsonify) [info] 59-59: use jsonify instead of json.dumps for JSON output (use-jsonify) 🪛 Checkov (3.3.17)scripts/consumer-witnesses/jedit-state-read.Dockerfile[low] 1-17: Ensure that HEALTHCHECK instructions have been added to container images (CKV_DOCKER_2) [low] 1-17: Ensure that a user for the container has been created (CKV_DOCKER_3) 🪛 Hadolint (2.15.1)scripts/consumer-witnesses/jedit-state-read.Dockerfile[warning] 2-2: Pin versions in apt get install. Instead of (DL3008) [warning] 7-7: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006) [warning] 11-11: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006) 🪛 LanguageTooldocs/topics/providers/test-plan.md[grammar] ~144-~144: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) CHANGELOG.md[grammar] ~18-~18: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🪛 Ruff (0.16.7)scripts/consumer-witnesses/jedit-state-read.py[error] 17-17: (S603) [warning] 30-30: Avoid specifying long messages outside the exception class (TRY003) [warning] 37-37: Avoid specifying long messages outside the exception class (TRY003) [warning] 66-66: Avoid specifying long messages outside the exception class (TRY003) [warning] 71-71: Avoid specifying long messages outside the exception class (TRY003) 🪛 Trivy (0.74.0)scripts/consumer-witnesses/jedit-state-read.Dockerfile[error] 1-1: Image user should not be 'root' Specify at least 1 USER command in Dockerfile with non-root user as argument Rule: DS-0002 (IaC/Dockerfile) [info] 1-1: No HEALTHCHECK defined Add HEALTHCHECK instruction in your Dockerfile Rule: DS-0026 (IaC/Dockerfile) 🔇 Additional comments (23)
Summary by CodeRabbit
WalkthroughAdds explicitly selected ChangesOrdered Echo Target IR
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~50 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Source as Edict source
participant Lowering as Target IR lowering
participant Validator as Execution-order validator
participant Encoder as Canonical encoder
Source->>Lowering: Source-ordered effects, bindings, and requirements
Lowering->>Validator: Intent with executionOrder
Validator->>Encoder: Validated ordered artifact
Possibly related PRs
Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established; the identified failed-lowering case publishes no artifact. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new execution-order contract is opt-in, and compatibility checks remain in place. No introduced security bypass was established. Actual execution, partial-failure handling, and recovery remain outside this change and unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 44.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 52 functions across 14 files. (10 skipped: 10 unsupported.)
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. Effects step in source order Comment |
|
Code Lawyer self-audit at integrated candidate
Cc @codex for a second opinion. Tests remain Docker-only; existing Jim producer locks remain unchanged. |
|
@codex review please |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code Lawyer activity summaryCandidate
Exact-head Docker The current-head public witness uses exact Jim source The reusable COPY worker has no host mounts and is now stopped. Retained target cache is 2.7 GiB plus 446 MiB registry, within the project budget; host free space was 716 GiB. No host tests ran. Complete paginated review retrieval currently has no formal reviews or threads. CodeRabbit is actively reviewing, so its approval is still required before merge. The Codex fallback request returned a quota response, which is not approval. This PR does not repin Jim, implement Echo execution, or complete rope editing/admission/recovery acceptance. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
MERGE GATE: OPEN ✅ CodeRabbit formally approved exact head Raw scanner advisory disposition (separate from CodeRabbit's no-actionable-findings verdict):
No runtime execution, admitted edit, or recovery claim is made. The historical release-ledger advisory remains disclosed. Proceeding under the user's standing merge authorization, using a regular merge pinned to this reviewed commit with no protection bypass. |
1 participant
Plain-English Walkthrough
TL;DR
An authored effect followed by a guard on its result could compile to Core but could not lower to Target IR. This adds explicitly selected
echo.span-ir/v2, preserving the sequence in canonical bytes while keeping the existing v1 artifact contract unchanged. [claim:ordered-ir, confidence:1.00]This is the next compiler prerequisite for flyingrobots/jedit#296 and draft flyingrobots/jedit#302. It does not implement a runtime graph read or rope edit. Its prerequisite #217 has merged; it now targets main and preserves the existing Jim compiler/provider/evaluator pins. [claim:scope, confidence:0.99]
Walkthrough
The new artifact kind is
orderedTargetIrArtifact. Each intent carries anexecutionOrderpermutation of the ids in its binding, effect, and guard tables. Instruction payloads have one owner; the sequence determines when they run, and the result follows the complete successful sequence. The compiler validates Core types and local authority independently before lowering. The canonical encoder additionally rejects missing, duplicate, foreign, forward-dependent, and inconsistent references, including dependencies in predicates and obstruction payloads. [claim:ordering-integrity, confidence:1.00]Selection separates compatibility from new semantics:
Caption: Explicit ordering and provider compatibility
The CDDL contract describes the ordered shape separately. Its tagged family contains closed/closure-free alternatives, so the bounded schema validator now composes tag dispatch with required-key dispatch. A family tag must be common to every alternative. Memoized rule/depth analysis prevents shared rule graphs from expanding into an exponential walk. Generated contract-pack bytes and digests are refreshed. [claim:schema, confidence:1.00]
The public CLI exposes
executionOrderand its referenced pure-binding payloads. A standalone Docker witness fetches Jim's committed source and the old Echo provider at exact revisions: v1 still reportsTargetLoweringFailed; changing only the disposable adapter selection and corresponding source import digest advances toInvalidProviderInvocationat the old provider's schema gate. Neither attempt publishes application artifacts. No executable artifact is handwritten. [claim:consumer, confidence:1.00]Compatibility and remaining work
Legacy golden bytes and pre-step refusals remain covered. The Rust
TargetIrIntentmodel gains anexecution_orderfield; callers constructing it directly must useNonefor legacy artifacts. Loops, statement branches, and external-action requests are unsupported by this first ordered contract. Runtime reads, private writes, atomic publication, and receipt/recovery evidence remain downstream work in Echo #684 and Jim #296. [claim:compatibility, confidence:1.00]Audit integration
Merge
c6a8fdeintegrates mainline arithmetic and byte-length validation fixes, retaining their refusal and work-count tests. It reconciles a changelog conflict and assigns ordered execution TIR-REQ-050 / TIR-TP-072–074 to avoid duplicate IDs. Fixf6ee91dremoves Cargo compilation from the Dockerfile build layers and adds a runtime entrypoint with explicit target/binary selection. The standalone recipe documents a read-only root, 8 GiB build-cache filesystem, 256 MiB scratch filesystem, and CPU/memory/process bounds; existing compatible workers should be reused first. The Dockerfile static check passed; a fresh image build was not repeated. [claim:audit-integration, confidence:0.99]Validation
All validation ran in COPY-based Docker without mounting host repositories. Current candidate:
f6ee91dc94f83ca1af0a5a08c37acb6b111fb764. The reusable worker executes builds under its shared lock; generated build output stays outside image layers.sh scripts/consumer-witnesses/run-jedit-state-read.shemittedORDERED_PUBLIC_BUILD_REACHED_OLD_PROVIDER_GATEon the current candidate. Its public v1 control refused withTargetLoweringFailed; v2 reachedInvalidProviderInvocation/ArtifactSchemaMismatchat the pinned old provider. No application output was published.cargo xtask verify: passed (exit 0), including legacy byte/digest goldens, generated contract pack, component fixtures, and 27 topic shelves.v0.1.0-alpha.1asMissingSurfacein the policy block (non-fatal); this PR does not claim a release.Appendix: Citations
claim:audit-integrationscripts/consumer-witnesses/run-jedit-state-read.sh#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;scripts/consumer-witnesses/README.md#8@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764; mergec6a8fde10656ed38760e7b4344ed9a2f7cd0ee33claim:ordered-ircrates/edict-syntax/src/target_ir.rs#25@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;crates/edict-syntax/tests/target_ir/ordered.rs#47@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764claim:scopescripts/consumer-witnesses/README.md#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764claim:ordering-integritycrates/edict-syntax/src/target_ir/execution_order.rs#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;crates/edict-syntax/tests/target_ir/ordered.rs#124@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764claim:schemadocs/abi/edict-target-ir.cddl#21@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;crates/edict-provider-schema/src/recursive_dispatch.rs#548@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;crates/edict-provider-schema/tests/registry.rs#668@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764claim:consumerscripts/consumer-witnesses/jedit-state-read.py#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;scripts/consumer-witnesses/jedit-state-read.Dockerfile#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;crates/edict-cli/tests/jsonl_cli.rs#429@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764claim:compatibilitycrates/edict-syntax/tests/target_ir/ordered.rs#87@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;docs/topics/target-ir/README.md#1@f6ee91dc94f83ca1af0a5a08c37acb6b111fb764;cargo xtask verifyCloses #218