feat: complete phase 8 runtime schema freeze#306
Open
flyingrobots wants to merge 19 commits intomainfrom
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new crate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WasmABI as "echo-wasm-abi (ABI layer)"
participant WarpCore as "warp-core"
participant Schema as "echo-runtime-schema"
Client->>WasmABI: pack ControlIntentV1{ WriterHeadKey(AbiWorldlineId, AbiHeadId) }
WasmABI->>Schema: AbiWorldlineId -> WorldlineId (from_bytes)
WasmABI->>Schema: AbiHeadId -> HeadId (from_bytes)
WasmABI->>WarpCore: deliver ControlIntent{ WriterHeadKey(WorldlineId, HeadId) }
WarpCore->>Schema: use GlobalTick/RunId/WorldlineTick types
WarpCore-->>Client: acknowledge / process result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2af8725a51
ℹ️ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Line 24: Remove the .gitignore exception for .vscode/settings.json or replace
the hardcoded Unix path in that settings file (/tmp/echo-rust-analyzer-target)
with a cross-platform solution: either use a relative path (e.g.,
target/rust-analyzer) or reference an environment-variable fallback (e.g.,
${RA_TARGET_DIR}) so Windows users aren’t broken; then update CONTRIBUTING.md to
document the chosen approach and any editor setup steps and expected optional vs
committed settings so contributors know whether to commit their .vscode settings
or keep local overrides.
In @.vscode/settings.json:
- Around line 2-4: Replace the absolute /tmp path in the rust-analyzer config so
it is repository-relative and portable: update the
"rust-analyzer.cargo.extraEnv" entry to set CARGO_TARGET_DIR to a repo-local
directory name (e.g. "target-ra") instead of "/tmp/echo-rust-analyzer-target" to
avoid platform, collision and permission issues; also add the chosen directory
(e.g. target-ra/) to .gitignore near the existing target-* entries so build
artifacts are not committed.
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 75-89: WorldlineId exposes as_bytes() but lacks a symmetric
constructor like HeadId::from_bytes, forcing callers to construct it via the
public tuple field; add a const constructor WorldlineId::from_bytes(bytes:
RuntimeIdBytes) -> Self to provide a stable API surface matching HeadId and make
construction explicit and ergonomic alongside the existing as_bytes() method.
In `@crates/echo-wasm-abi/Cargo.toml`:
- Line 21: The Cargo.toml for echo-wasm-abi is forwarding the crate feature
"std" to serde, ciborium and half but not to echo-runtime-schema, so when
building with --features std the schema crate still compiles no-std; fix this by
updating the echo-runtime-schema dependency entry in Cargo.toml to enable its
std feature (mirror how serde/ciborium/half are forwarded) — e.g. change the
echo-runtime-schema declaration to include features = ["std"] (and workspace =
true as already present) so the schema types compile with std when the crate's
std feature is enabled.
In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 111-114: The schema's InboxPolicyBudgeted.maxPerTick field is
documented to be positive but uses plain Int, so enforce the positivity either
by introducing/using a PositiveInt custom scalar in the schema and replacing
maxPerTick: Int! with maxPerTick: PositiveInt! (and add its implementation), or
add explicit runtime validation in the resolver/input handling for the
InboxPolicyBudgeted type that throws a GraphQLError when maxPerTick <= 0;
reference the type name InboxPolicyBudgeted and the field name maxPerTick when
making the change so callers/validators are updated accordingly.
In `@scripts/generate-dependency-dags.js`:
- Around line 562-565: The fallback never runs because parseArgs sets
args.tasksDagPath to the string "TASKS-DAG.md"; update the parseArgs option for
--tasks-dag (the default value for tasksDagPath) to undefined (or remove the
default) so args.tasksDagPath is nullish when the flag is not provided, and keep
the existing tasksDagPath resolution using: const tasksDagPath =
path.resolve(process.cwd(), args.tasksDagPath ??
path.join("docs","archive","tasks","TASKS-DAG.md")); this ensures the fallback
to docs/archive/tasks/TASKS-DAG.md is used.
In `@scripts/verify-local.sh`:
- Around line 710-717: The loop variable name md_file is misleading because the
loop iterates CHANGED_FILES looking for .graphql/.mjs files; rename md_file to a
clearer identifier like changed_file or file_in_changes inside the while loop
and update all references (the case pattern matching and any uses that set
should_validate_runtime_schema) so the logic remains identical but the variable
name accurately reflects it (look for occurrences of CHANGED_FILES, md_file, and
should_validate_runtime_schema to update).
- Around line 732-735: The script calls "node
scripts/validate-runtime-schema-fragments.mjs" when the variable
should_validate_runtime_schema is set but does not verify Node is installed;
update the if-block that checks should_validate_runtime_schema to first verify
Node is available (e.g., use a command existence check against "node"), and if
missing print a clear error and exit non‑zero before attempting to run
validate-runtime-schema-fragments.mjs; reference the existing
should_validate_runtime_schema check and the node invocation so you modify that
branch only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cd2f967-fe13-426c-abc7-972cd5c1af2d
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockdocs/assets/dags/tasks-dag.dotis excluded by!**/*.dotdocs/assets/dags/tasks-dag.svgis excluded by!**/*.svg
📒 Files selected for processing (34)
.gitignore.vscode/settings.jsonCargo.tomlcrates/echo-runtime-schema/Cargo.tomlcrates/echo-runtime-schema/README.mdcrates/echo-runtime-schema/src/lib.rscrates/echo-wasm-abi/Cargo.tomlcrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/clock.rscrates/warp-core/src/head.rscrates/warp-core/src/observation.rscrates/warp-core/src/worldline.rscrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/dependency-dags.mddocs/plans/adr-0008-and-0009.mddocs/plans/phase-8-runtime-schema-conformance.mddocs/plans/phase-8-runtime-schema-mapping-contract.mddocs/plans/phase-8-schema-freeze-inventory.mddocs/spec/SPEC-0009-wasm-abi-v3.mdpackage.jsonschemas/runtime/README.mdschemas/runtime/artifact-a-identifiers.graphqlschemas/runtime/artifact-b-routing-and-admission.graphqlschemas/runtime/artifact-c-playback-control.graphqlschemas/runtime/artifact-d-scheduler-results.graphqlscripts/check-append-only.jsscripts/generate-dependency-dags.jsscripts/generate-tasks-dag.jsscripts/validate-runtime-schema-fragments.mjsscripts/verify-local.shtests/hooks/test_runtime_schema_validation.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 76-89: Make WorldlineId's inner field private (remove pub from the
tuple field) and add a constructor method
WorldlineId::from_bytes(RuntimeIdBytes) -> Self (preferably const fn) that
constructs the new opaque id; keep the existing as_bytes(&self) returning
&RuntimeIdBytes. This mirrors HeadId's opacity and provides a canonical factory
to replace direct tuple construction (update tests and call sites from
WorldlineId([...]) to WorldlineId::from_bytes([...])). Ensure all references use
the new constructor and that RuntimeIdBytes remains the parameter/return type.
In `@scripts/generate-dependency-dags.js`:
- Around line 173-175: The fallback date label uses local-time formatting via
formatDateYYYYMMDD(new Date()), causing non-deterministic labels across
timezones; change this to produce a UTC-based date string instead—either update
formatDateYYYYMMDD to use getUTCFullYear/getUTCMonth/getUTCDate or call a new
UTC-aware helper (e.g., formatDateYYYYMMDDUTC) from the fallback assignment that
uses new Date() but formats with UTC methods; ensure the code referencing
generatedAt and the fallback const still returns { mode: "date", label:
<UTC-formatted-string> } so snapshot labels are consistent across environments.
In `@tests/hooks/test_dependency_dags.sh`:
- Line 92: The test's grep is brittle because it looks for the exact attribute
ordering 'i1 -> i2 \[color="red"' in tests/hooks/test_dependency_dags.sh; change
the check that uses that literal grep to a more flexible pattern that matches an
edge line for "i1 -> i2" with a bracketed attribute list that contains
color="red" anywhere (e.g., use a regex like i1 -> i2 \[[^]]*color="red"[^]]*\]
or equivalent POSIX/egrep form) so the assertion no longer depends on attribute
ordering while still verifying the color attribute is present.
- Around line 88-91: Before invoking node on scripts/generate-dependency-dags.js
in the temp workspace ($tmpdir), create a package.json that sets the module type
so Node treats files as ESM; add a step to write a package.json with
{"type":"module"} into $tmpdir (i.e., create the file in the same block that cds
into "$tmpdir" before running node scripts/generate-dependency-dags.js
>"$output_file" 2>&1) so the import-based script runs under ESM semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 685201e4-296b-43db-a931-e4069b76a073
📒 Files selected for processing (8)
crates/echo-runtime-schema/Cargo.tomlcrates/echo-runtime-schema/README.mdcrates/echo-runtime-schema/src/lib.rscrates/echo-wasm-abi/Cargo.tomlcrates/warp-core/Cargo.tomldocs/guide/cargo-features.mdscripts/generate-dependency-dags.jstests/hooks/test_dependency_dags.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONTRIBUTING.md`:
- Line 106: Replace the machine-specific absolute path
"/Users/james/git/echo/.vscode/settings.json" in the CONTRIBUTING.md sentence
with a repo-relative link to the tracked config, e.g. use
[.vscode/settings.json] (or ./ .vscode/settings.json) so the link works for all
users and does not expose a local filesystem path.
In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1210: Update the wasm32 test accessors to match the new
WorldlineId API: replace any use of tuple-field access like wl.0 with the new
method call *wl.as_bytes(). Specifically, change occurrences in
wasm_tests::test_parse_helpers (and related wasm32 tests) that read wl.0 to use
*wl.as_bytes(), and adjust test_parse_worldline_id_inner_valid expectations to
remain consistent with that API.
In `@crates/warp-core/src/provenance_store.rs`:
- Line 2564: Multiple fork tests repeat the literal [99u8; 32] when constructing
WorldlineId via WorldlineId::from_bytes([99u8; 32]); introduce a single test
fixture (either a const like TEST_WORLDLINE_BYTES: [u8; 32] = [99u8; 32] or a
helper fn like test_worldline_id() -> WorldlineId that calls
WorldlineId::from_bytes(TEST_WORLDLINE_BYTES)) and replace each occurrence of
WorldlineId::from_bytes([99u8; 32]) (seen in the file where
WorldlineId::from_bytes is used around the fork tests) with the shared
constant/helper to avoid duplication and reduce drift risk.
In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 49-55: The PositiveInt scalar currently only guarantees "greater
than zero" but must be documented to map to Rust's u32 range; update the
mapping/docs for PositiveInt to explicitly state it represents the full u32
range (1–4,294,967,295) and note that cycle_limit (Option<u32>) and fields
cycleLimit / maxPerTick consume this scalar, so generators that treat GraphQL
Int as 32-bit signed need the explicit u32 upper-bound to accept values above
2,147,483,647; reference PositiveInt, cycle_limit, cycleLimit, maxPerTick and
Option<u32> in the doc update.
In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 67-69: Unify the field name for the exact-head coordinate across
the schema by renaming ExactHeadTarget.key to match IngressTargetInput.headKey
(or vice versa) before the schema freeze; update the GraphQL type definitions
(e.g., ExactHeadTarget and IngressTargetInput) so both use the identical field
name (headKey or key), adjust any associated input/output types and usages in
the same file (including the occurrences around lines 84-89), and ensure all
references, resolvers, and generated DTO/mapper expectations are updated
accordingly so the schema is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eb4deab0-eee9-46a7-a5e6-0b13108d7c51
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.gitignore.vscode/settings.jsonCHANGELOG.mdCONTRIBUTING.mdcrates/echo-dind-harness/tests/digest_golden_vectors.rscrates/echo-runtime-schema/src/lib.rscrates/echo-wasm-abi/Cargo.tomlcrates/echo-wasm-abi/src/codec.rscrates/ttd-browser/src/lib.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/engine_impl.rscrates/warp-core/src/head.rscrates/warp-core/src/head_inbox.rscrates/warp-core/src/observation.rscrates/warp-core/src/playback.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/snapshot.rscrates/warp-core/src/worldline.rscrates/warp-core/src/worldline_registry.rscrates/warp-core/src/worldline_state.rscrates/warp-core/tests/checkpoint_fork_tests.rscrates/warp-core/tests/common/mod.rscrates/warp-core/tests/golden_vectors_phase0.rscrates/warp-core/tests/inbox.rscrates/warp-core/tests/invariant_property_tests.rscrates/warp-core/tests/outputs_playback_tests.rscrates/warp-core/tests/slice_theorem_proof.rscrates/warp-wasm/src/warp_kernel.rsdocs/guide/cargo-features.mdpackage.jsonschemas/runtime/artifact-a-identifiers.graphqlschemas/runtime/artifact-b-routing-and-admission.graphqlschemas/runtime/artifact-d-scheduler-results.graphqlscripts/generate-dependency-dags.jsscripts/verify-local.shtests/hooks/test_dependency_dags.sh
|
|
||
| ### Shared Workspace Settings | ||
|
|
||
| - The repo tracks a minimal [.vscode/settings.json](/Users/james/git/echo/.vscode/settings.json) for project-safe tooling settings only. |
Contributor
There was a problem hiding this comment.
Replace machine-specific absolute path with a repo-relative link.
Line 106 currently points to /Users/james/git/echo/.vscode/settings.json, which is broken for everyone except one local machine and leaks local path details.
🔧 Proposed fix
-- The repo tracks a minimal [.vscode/settings.json](/Users/james/git/echo/.vscode/settings.json) for project-safe tooling settings only.
+- The repo tracks a minimal [.vscode/settings.json](.vscode/settings.json) for project-safe tooling settings only.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - The repo tracks a minimal [.vscode/settings.json](/Users/james/git/echo/.vscode/settings.json) for project-safe tooling settings only. | |
| - The repo tracks a minimal [.vscode/settings.json](.vscode/settings.json) for project-safe tooling settings only. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CONTRIBUTING.md` at line 106, Replace the machine-specific absolute path
"/Users/james/git/echo/.vscode/settings.json" in the CONTRIBUTING.md sentence
with a repo-relative link to the tracked config, e.g. use
[.vscode/settings.json] (or ./ .vscode/settings.json) so the link works for all
users and does not expose a local filesystem path.
Comment on lines
1206
to
1210
| fn test_parse_worldline_id_inner_valid() { | ||
| let wl = parse_worldline_id_inner(&[0u8; 32]); | ||
| assert!(wl.is_ok()); | ||
| assert_eq!(wl.unwrap().0, [0u8; 32]); | ||
| assert_eq!(*wl.unwrap().as_bytes(), [0u8; 32]); | ||
| } |
Contributor
There was a problem hiding this comment.
Complete the wasm32 test migration.
This assertion was updated, but wasm_tests::test_parse_helpers at Line 1110 still reads wl.0. wasm32 test builds will fail until that assertion is switched to *wl.as_bytes().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/ttd-browser/src/lib.rs` around lines 1206 - 1210, Update the wasm32
test accessors to match the new WorldlineId API: replace any use of tuple-field
access like wl.0 with the new method call *wl.as_bytes(). Specifically, change
occurrences in wasm_tests::test_parse_helpers (and related wasm32 tests) that
read wl.0 to use *wl.as_bytes(), and adjust test_parse_worldline_id_inner_valid
expectations to remain consistent with that API.
| let mut store = LocalProvenanceStore::new(); | ||
| let source = test_worldline_id(); | ||
| let target = WorldlineId([99u8; 32]); | ||
| let target = WorldlineId::from_bytes([99u8; 32]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce repeated literal worldline IDs in fork tests.
[99u8; 32] is repeated across multiple tests. A small test helper/const would reduce drift risk if this fixture value changes later.
Also applies to: 2614-2614, 2782-2782
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/provenance_store.rs` at line 2564, Multiple fork tests
repeat the literal [99u8; 32] when constructing WorldlineId via
WorldlineId::from_bytes([99u8; 32]); introduce a single test fixture (either a
const like TEST_WORLDLINE_BYTES: [u8; 32] = [99u8; 32] or a helper fn like
test_worldline_id() -> WorldlineId that calls
WorldlineId::from_bytes(TEST_WORLDLINE_BYTES)) and replace each occurrence of
WorldlineId::from_bytes([99u8; 32]) (seen in the file where
WorldlineId::from_bytes is used around the fork tests) with the shared
constant/helper to avoid duplication and reduce drift risk.
Comment on lines
+49
to
+55
| """ | ||
| Positive signed integer used where GraphQL `Int` needs an explicit positivity | ||
| contract. | ||
|
|
||
| Semantic rule: values must be greater than zero. | ||
| """ | ||
| scalar PositiveInt |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "PositiveInt|cycleLimit|maxPerTick|u32|2_147_483_647" \
schemas/runtime docs/plans crates/warp-wasm/src/warp_kernel.rs crates/echo-wasm-abi/srcRepository: flyingrobots/echo
Length of output: 6998
Pin PositiveInt to explicit u32 range (1–4,294,967,295) in mapping documentation.
cycle_limit is Option<u32> and cycleLimit/maxPerTick consume it as PositiveInt. The scalar is defined as "greater than zero" with no upper bound. Without explicit documentation pinning this to u32 semantics, generators treating this as GraphQL Int will reject values above 2,147,483,647. Documentation acknowledges the mapping but labels the positive-value rule as "semantic validation"—clarify the full u32 range constraint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/runtime/artifact-a-identifiers.graphql` around lines 49 - 55, The
PositiveInt scalar currently only guarantees "greater than zero" but must be
documented to map to Rust's u32 range; update the mapping/docs for PositiveInt
to explicitly state it represents the full u32 range (1–4,294,967,295) and note
that cycle_limit (Option<u32>) and fields cycleLimit / maxPerTick consume this
scalar, so generators that treat GraphQL Int as 32-bit signed need the explicit
u32 upper-bound to accept values above 2,147,483,647; reference PositiveInt,
cycle_limit, cycleLimit, maxPerTick and Option<u32> in the doc update.
Comment on lines
+67
to
+69
| type ExactHeadTarget { | ||
| key: WriterHeadKey! | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Normalize key vs headKey before the schema freeze.
ExactHeadTarget.key and IngressTargetInput.headKey represent the same exact-head coordinate with two different field names. Freezing that asymmetry now will leak into every generated DTO, mapper, and doc set.
Patch
type ExactHeadTarget {
- key: WriterHeadKey!
+ headKey: WriterHeadKey!
}Also applies to: 84-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/runtime/artifact-b-routing-and-admission.graphql` around lines 67 -
69, Unify the field name for the exact-head coordinate across the schema by
renaming ExactHeadTarget.key to match IngressTargetInput.headKey (or vice versa)
before the schema freeze; update the GraphQL type definitions (e.g.,
ExactHeadTarget and IngressTargetInput) so both use the identical field name
(headKey or key), adjust any associated input/output types and usages in the
same file (including the occurrences around lines 84-89), and ensure all
references, resolvers, and generated DTO/mapper expectations are updated
accordingly so the schema is consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
schemas/runtime/with validation and conformance docsecho-runtime-schemaas the shared home for the frozen logical counters and core runtime ids/keyVerification
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores