feat: complete phase 8 runtime schema freeze#306
Open
flyingrobots wants to merge 22 commits intomainfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new crate Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WasmABI as "echo-wasm-abi (ABI)"
participant WarpCore as "warp-core"
participant Schema as "echo-runtime-schema"
Client->>WasmABI: send ControlIntentV1 { AbiWriterHeadKey(AbiWorldlineId, AbiHeadId) }
WasmABI->>Schema: AbiWorldlineId::from_bytes(...) → WorldlineId
WasmABI->>Schema: AbiHeadId::from_bytes(...) → HeadId
WasmABI->>WarpCore: deliver ControlIntent { WriterHeadKey(WorldlineId, HeadId) }
WarpCore->>Schema: reference GlobalTick / WorldlineTick / RunId (pub use)
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
Owner
Author
|
@coderabbitai review please 🥕 |
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 260485
Executive summary
Blocking
High-priority follow-ups (pre-merge optional, post-merge acceptable)
diff --git a/scripts/validate-runtime-schema-fragments.mjs b/scripts/validate-runtime-schema-fragments.mjs
@@
-import { spawnSync } from "node:child_process";
+import { spawnSync } from "node:child_process";
+import { existsSync } from "node:fs";
@@
function runPrettierCheck(files) {
const formattingErrors = [];
+ // Try a few execution strategies in order of preference
+ const localPrettier = resolve(REPO_ROOT, "node_modules", ".bin", process.platform === "win32" ? "prettier.cmd" : "prettier");
+ const strategies = [
+ { cmd: "npx", args: ["prettier"] },
+ { cmd: "pnpm", args: ["exec", "prettier"] },
+ { cmd: localPrettier, args: [], onlyIfExists: true },
+ ];
+
+ const pickPrettier = () => {
+ for (const s of strategies) {
+ if (s.onlyIfExists && !existsSync(s.cmd)) continue;
+ const probe = spawnSync(s.cmd, [...s.args, "--version"], { encoding: "utf8" });
+ if (probe.error || probe.status !== 0) continue;
+ return s;
+ }
+ return null;
+ };
+
+ const runner = pickPrettier();
+ if (!runner) {
+ throw new Error("failed to find a runnable prettier (tried npx, pnpm exec, and local node_modules/.bin/prettier)");
+ }
+
for (const file of files) {
const source = readFileSync(file, "utf8");
const syntheticFilePath = join(
REPO_ROOT,
"schemas/runtime",
basename(file),
);
- const result = spawnSync(
- "npx",
- [
- "prettier",
- "--parser",
- "graphql",
- "--stdin-filepath",
- syntheticFilePath,
- ],
- {
- encoding: "utf8",
- input: source,
- },
- );
+ const result = spawnSync(runner.cmd, [...runner.args, "--parser", "graphql", "--stdin-filepath", syntheticFilePath], {
+ encoding: "utf8",
+ input: source,
+ });
@@
if (result.error) {
- throw new Error(
- `failed to run npx prettier for schema validation: ${result.error.message}`,
- );
+ throw new Error(`failed to run prettier for schema validation: ${result.error.message}`);
}
#[test]
fn test_worldline_id_rejects_33_byte_sequence() {
use crate::kernel_port::WorldlineId;
use ciborium::value::Value;
#[derive(Debug, PartialEq, Eq, Deserialize)]
struct Wrapper { id: WorldlineId }
// Sequence form: [u8; 33]
let arr = Value::Array((0..33).map(|_| Value::Integer(0u8.into())).collect());
let bytes = encode_value(&Value::Map(vec![(Value::Text("id".into()), arr)])).unwrap();
let err = decode_cbor::<Wrapper>(&bytes).unwrap_err();
assert!(err.toString().contains("32 bytes"));
}
#[test]
fn test_worldline_id_accepts_array_sequence_exactly_32() {
use crate::kernel_port::WorldlineId;
use ciborium::value::Value;
#[derive(Debug, PartialEq, Eq, Deserialize)]
struct Wrapper { id: WorldlineId }
let arr = Value::Array((0..32).map(|_| Value::Integer(7u8.into())).collect());
let bytes = encode_value(&Value::Map(vec![(Value::Text("id".into()), arr)])).unwrap();
let decoded: Wrapper = decode_cbor(&bytes).unwrap();
assert_eq!(decoded.id.as_bytes(), &[7u8; 32]);
}
Design and correctness review (OK/notes by area)
Non-blocking nits and small polish
What I verified explicitly
I retained prior context from earlier reviews:
If you want, I can open follow-up issues for: (a) schema-validator portability (npx fallback), (b) ABI id negative decode cases, and (c) optional no_std smoke lane for echo-wasm-abi. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (4)
crates/ttd-browser/src/lib.rs (1)
1206-1209:⚠️ Potential issue | 🟠 MajorThe wasm32 test module still uses the old private-field accessor.
This native test was migrated, but
wasm_tests::test_parse_helpersat Line 1110 still doeswl.0.WorldlineIdis opaque now, so wasm32 test builds remain broken until that assertion also switches to*wl.as_bytes().🔧 Required follow-up
- assert_eq!(wl.0, [0u8; 32]); + assert_eq!(*wl.as_bytes(), [0u8; 32]);🤖 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 - 1209, Update the wasm32 test in wasm_tests::test_parse_helpers to stop using the old private-field accessor on WorldlineId (wl.0) and use the public accessor instead: call parse_worldline_id_inner (or reuse the existing test variable wl) and assert against *wl.as_bytes() similarly to the native test; replace any occurrence of wl.0 with *wl.as_bytes() so the opaque WorldlineId is accessed via its as_bytes() method.docs/plans/phase-8-runtime-schema-conformance.md (1)
85-87:⚠️ Potential issue | 🟠 Major
u32does not map cleanly to GraphQLInt!.GraphQL
Inttops out at 2,147,483,647, somax_per_tick: u32cannot be described as a cleanInt!mapping across its full runtime range. The audit should either call out the signed-32-bit ceiling explicitly or constrain the allowed range instead of blessing the mapping as-is.What range does the GraphQL specification allow for the built-in `Int` scalar?As per coding guidelines, "Echo is a deterministic simulation engine. Documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering. Flag factual errors and stale cross-references."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/phase-8-runtime-schema-conformance.md` around lines 85 - 87, The doc incorrectly states that InboxPolicy::Budgeted { max_per_tick: u32 } maps cleanly to GraphQL maxPerTick: Int! — GraphQL Int is a signed 32-bit value with range -2,147,483,648 to 2,147,483,647, so update the text to either (a) explicitly call out the 2,147,483,647 positive ceiling and note the semantic validation required for u32 values above that, or (b) change the schema to use a constrained/custom scalar or validation rule that limits maxPerTick to <= 2,147,483,647 (and document that constraint) so the mapping is accurate; reference InboxPolicy::Budgeted, max_per_tick and maxPerTick in the update.crates/echo-runtime-schema/src/lib.rs (1)
19-27: 🛠️ Refactor suggestion | 🟠 MajorHide the raw
u64field on the shared counters.
pub struct $name(pub u64)makes the representation itself part of the cross-crate API, bypasses the checked helper surface, and leaves the public field undocumented. Before this crate becomes the canonical shared owner, make the field private and force external code ontofrom_raw(...)/as_u64().As per coding guidelines, "Every public API across crates (warp-core, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-runtime-schema/src/lib.rs` around lines 19 - 27, The public tuple field on counters created by the logical_counter! macro exposes the u64 representation and bypasses helpers; change the macro so the generated pub struct $name has a private field (e.g., struct $name(u64)) and expose a documented public API: a checked constructor from_raw(...) and an accessor as_u64() (or similar) and ensure both functions have rustdoc comments describing invariants/usage; update any existing impls or derives inside logical_counter! to use the private field and refer to these helpers so external crates must use from_raw/as_u64 rather than accessing the raw u64 directly.schemas/runtime/artifact-b-routing-and-admission.graphql (1)
67-69:⚠️ Potential issue | 🟠 MajorNormalize the exact-head field name before freezing this schema.
Line 68 uses
key, while Line 88 usesheadKeyfor the same exact-head coordinate. Freezing both names bakes an avoidable asymmetry into every DTO, mapper, and doc that touches this surface. Pick one name on both input and output types.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, The ExactHeadTarget GraphQL type uses inconsistent field names for the same coordinate (field "key" on ExactHeadTarget vs "headKey" elsewhere); pick one canonical name (e.g., headKey) and update all schema definitions and related input/output types so the field name is identical across ExactHeadTarget and every related type (including the other occurrence at lines 84–89 referenced in the review), then regenerate DTOs/mappers/docs so the symbol ExactHeadTarget and any usages reference the chosen field name uniformly.
🤖 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/README.md`:
- Around line 18-19: Update the README sentence about serde feature gating to
reflect the crate's default behavior: state that serde derives are feature-gated
and consumers must enable the crate's `serde` feature unless they are using the
crate with default features enabled (or unless they compile with
`default-features = false` to force explicit opt-in); reference the `serde`
feature name and the `default-features = false` flag so readers know when
explicit enabling is required.
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 79-80: WorldlineId and HeadId newtypes currently use
#[serde(transparent)] which serializes the inner [u8;32] as a 32-element integer
array instead of a byte string; implement explicit serde::Serialize and
serde::Deserialize for WorldlineId and HeadId to serialize/deserialze as
canonical bytes(32) (e.g., as a 32-byte byte string for CBOR/JSON) and update
WriterHeadKey usage to rely on those impls; add a unit test that serializes and
deserializes WorldlineId and HeadId (and a WriterHeadKey instance) to verify
round-trip produces the spec-compliant bytes(32) representation.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 519-535: The test test_worldline_id_rejects_non_32_byte_payloads
only checks a 31-byte underflow; add a symmetric overlong case to ensure
deserialization rejects >32 bytes too by constructing a CBOR map with "id" set
to 33 bytes and asserting decode_cbor::<Wrapper>(&bytes).unwrap_err() contains
"32 bytes"; modify the test (which uses WorldlineId and Wrapper and calls
decode_cbor) to include both the 31-byte and 33-byte failure assertions to pin
strict 32-byte enforcement from both sides.
In `@crates/warp-wasm/src/warp_kernel.rs`:
- Around line 170-176: The helpers parse_worldline_id and parse_head_id are not
parsers and should be renamed to to_core_worldline_id and to_core_head_id (or
similar) to reflect direct conversion; change their signatures to return
WorldlineId and HeadId directly (no Result) by returning
WorldlineId::from_bytes(*worldline_id.as_bytes()) and
HeadId::from_bytes(*head_id.as_bytes()); likewise rename and change
parse_head_key (and the similar function referenced around lines 460-464) to
to_core_head_key and return the HeadKey directly instead of Result, and update
all call sites (including the call at the former line 439) to remove dead error
handling/unwraps accordingly.
In `@docs/dependency-dags.md`:
- Line 89: The docs line referencing the scheduled workflow uses incorrect
platform casing; update the sentence that mentions
`.github/workflows/refresh-dependency-dags.yml` so the platform name reads
"GitHub" instead of "GITHUB" (search for the Generator line mentioning
`scripts/generate-tasks-dag.js` and
`.github/workflows/refresh-dependency-dags.yml` in docs/dependency-dags.md and
replace the uppercase token with the proper "GitHub" casing).
In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 127-133: Update the blocker paragraph to reflect the current
typed-id migration: remove or reword the claim that `HeadId`, `WorldlineId`, and
`WriterHeadKey` are still exposed as raw `Vec<u8>` and instead list only the
actual runtime schema types that remain as raw byte vectors (if any); reference
the typed ABI wrappers for `HeadId`, `WorldlineId`, and `WriterHeadKey` so the
note is narrowed to the true remaining raw-byte ids and mark the previous
statement as stale/updated to avoid misleading the audit.
In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-105: Artifact A's identifier list omits WorldlineId, causing
inconsistency with the Phase 8 freeze candidates and downstream artifacts;
update the Artifact A identifier enumeration (the block listing HeadId,
WriterHeadKey, IntentKind, WorldlineTick, GlobalTick) to include WorldlineId so
the artifact definition matches the freeze candidate set and downstream
dependencies that reference WorldlineId.
In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 70-88: The script currently hard-codes spawning "npx" (see the
spawnSync call that runs prettier) which breaks pnpm-only environments; change
the launcher resolution to try a cascade: first "npx", then "pnpm exec", and
finally fall back to the local node_modules/.bin prettier executable if present,
using the same args and input so pnpm users can run the check. Also fix
extractTypeNames (the function that parses GraphQL field type expressions) so it
stops parsing when it encounters directive markers (@) and ignores text inside
string literals (respecting quoted boundaries), ensuring identifiers inside
directives or strings aren't collected as type names; update the
tokenization/state machine in extractTypeNames to exit on '@' and to treat
quoted strings as atomic tokens.
- Around line 112-147: The current sanitizeLines and extractTypeNames
implementations are brittle string-based heuristics; replace them by parsing the
SDL with a GraphQL parser (e.g., graphql-js parse/visit) and derive the
information from the AST: use parse(source, { noLocation: false }) to get
DocumentNode, iterate DocumentNode.definitions to collect type/operation
definitions (replacing sanitizeLines' role) and use a visitor to collect
NamedType nodes (replacing extractTypeNames) while ignoring descriptions,
comments and directive argument content; ensure you import and use graphql's
parse/visit functions, return the same shape expected by downstream code, and
remove the fragile line-based logic in sanitizeLines and extractTypeNames.
In `@tests/hooks/test_dependency_dags.sh`:
- Around line 98-103: The test currently calls fail and then immediately cats
"$tmpdir/docs/assets/dags/issue-deps.dot", which under set -e can hide the
original assertion if the DOT file is missing; change the branch so that after
the grep condition fails you first call fail with a clear message and only
attempt to dump the DOT file conditionally (e.g. check for existence or make the
cat a non-failing operation) to avoid a secondary “file not found” error; target
the block that references "$tmpdir/docs/assets/dags/issue-deps.dot" and the grep
check (the if grep -Eq ... then ... else ... fi) and ensure the fail() call runs
before any optional diagnostic dump or that the dump is guarded so it cannot
cause the test to exit with a different error.
---
Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 19-27: The public tuple field on counters created by the
logical_counter! macro exposes the u64 representation and bypasses helpers;
change the macro so the generated pub struct $name has a private field (e.g.,
struct $name(u64)) and expose a documented public API: a checked constructor
from_raw(...) and an accessor as_u64() (or similar) and ensure both functions
have rustdoc comments describing invariants/usage; update any existing impls or
derives inside logical_counter! to use the private field and refer to these
helpers so external crates must use from_raw/as_u64 rather than accessing the
raw u64 directly.
In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1209: Update the wasm32 test in
wasm_tests::test_parse_helpers to stop using the old private-field accessor on
WorldlineId (wl.0) and use the public accessor instead: call
parse_worldline_id_inner (or reuse the existing test variable wl) and assert
against *wl.as_bytes() similarly to the native test; replace any occurrence of
wl.0 with *wl.as_bytes() so the opaque WorldlineId is accessed via its
as_bytes() method.
In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 85-87: The doc incorrectly states that InboxPolicy::Budgeted {
max_per_tick: u32 } maps cleanly to GraphQL maxPerTick: Int! — GraphQL Int is a
signed 32-bit value with range -2,147,483,648 to 2,147,483,647, so update the
text to either (a) explicitly call out the 2,147,483,647 positive ceiling and
note the semantic validation required for u32 values above that, or (b) change
the schema to use a constrained/custom scalar or validation rule that limits
maxPerTick to <= 2,147,483,647 (and document that constraint) so the mapping is
accurate; reference InboxPolicy::Budgeted, max_per_tick and maxPerTick in the
update.
In `@schemas/runtime/artifact-b-routing-and-admission.graphql`:
- Around line 67-69: The ExactHeadTarget GraphQL type uses inconsistent field
names for the same coordinate (field "key" on ExactHeadTarget vs "headKey"
elsewhere); pick one canonical name (e.g., headKey) and update all schema
definitions and related input/output types so the field name is identical across
ExactHeadTarget and every related type (including the other occurrence at lines
84–89 referenced in the review), then regenerate DTOs/mappers/docs so the symbol
ExactHeadTarget and any usages reference the chosen field name uniformly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 33ede45b-a8aa-4f5f-b399-91a409595a16
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockdocs/assets/dags/tasks-dag.dotis excluded by!**/*.dotdocs/assets/dags/tasks-dag.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
.gitignore.vscode/settings.jsonCHANGELOG.mdCONTRIBUTING.mdCargo.tomlcrates/echo-dind-harness/tests/digest_golden_vectors.rscrates/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/codec.rscrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/ttd-browser/src/lib.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/clock.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/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.mddocs/dependency-dags.mddocs/guide/cargo-features.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_dependency_dags.shtests/hooks/test_runtime_schema_validation.sh
Owner
Author
|
@coderabbitai review please 🥕 |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
docs/plans/phase-8-runtime-schema-conformance.md (1)
56-59:⚠️ Potential issue | 🟠 MajorThe
HeadIdrow is stale and contradicts the later blocker section.The table says the ABI still keeps a byte DTO for
HeadId, but the later section correctly says the ABI now carries typed wrappers forHeadId/WorldlineId/WriterHeadKey. Withcrates/echo-wasm-abi/src/lib.rsandcrates/warp-wasm/src/warp_kernel.rsalready consuming typed ids directly, this row misstates the freeze boundary.Suggested edit
-| `HeadId` | `crates/echo-runtime-schema/src/lib.rs` | Aligned | Opaque hash-backed newtype matches the scalar intent. `warp-core` now consumes the shared owner and the ABI keeps a byte DTO. | +| `HeadId` | `crates/echo-runtime-schema/src/lib.rs` | Aligned | Opaque hash-backed newtype matches the scalar intent. `warp-core` and the ABI now consume the typed shared owner; CBOR still encodes it as bytes(32). |As per coding guidelines, "Documentation accuracy matters — especially anything touching determinism guarantees, hash stability, or canonical ordering. Flag factual errors and stale cross-references."
Also applies to: 127-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/phase-8-runtime-schema-conformance.md` around lines 56 - 59, The table entry for `HeadId` is stale and contradicts the blocker section; update the table row(s) so they state that the ABI now carries typed wrappers for `HeadId`, `WorldlineId`, and `WriterHeadKey` (remove any claim that the ABI keeps a byte DTO) to match the later text and current code in `crates/echo-wasm-abi/src/lib.rs` and `crates/warp-wasm/src/warp_kernel.rs`; ensure both the `HeadId` and `WorldlineId` rows (and the similar rows at 127-132) reflect the "Aligned" status with typed ABI wrappers and adjust the Notes column to reference consumption by the listed crates.scripts/validate-runtime-schema-fragments.mjs (1)
75-79:⚠️ Potential issue | 🟠 MajorPrefer the workspace-pinned Prettier before the indirect launchers.
This validator is part of the runtime-freeze toolchain, so formatter resolution should be deterministic. Probing
npxbefore the repo-local binary keeps version selection tied to external launcher behavior instead of the workspace install. Prefernode_modules/.bin/prettierfirst, thenpnpm exec, and keepnpxonly as a last-resort--no-installfallback.Patch
const candidates = [ - { command: "npx", prefix: ["prettier"] }, - { command: "pnpm", prefix: ["exec", "prettier"] }, - { command: localPrettier, prefix: [], requireExists: true }, + { command: localPrettier, prefix: [], requireExists: true }, + { command: "pnpm", prefix: ["exec", "prettier"] }, + { command: "npx", prefix: ["--no-install", "prettier"] }, ];Also applies to: 85-97, 111-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-runtime-schema-fragments.mjs` around lines 75 - 79, Change the Prettier resolution order to prefer the workspace-pinned binary first: put the entry that uses localPrettier (requireExists: true) at the front of the candidates array, followed by the pnpm exec entry, and leave the npx entry last (use npx with --no-install as a safe fallback). Update every place where the candidates array is defined (the instances around the current candidates declaration and the other occurrences referenced in the review: the blocks covering lines ~85-97 and ~111-119) so they all use the same ordering (localPrettier, pnpm exec, then npx) to ensure deterministic formatter resolution.
🤖 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 214-228: Add serde sequence-path regression tests exercising
RuntimeIdVisitor::visit_seq: add one acceptance test that deserializes a
32-element sequence of u8 values into the expected [u8; 32] (round-trip or
direct deserialization asserting Ok) and one rejection test that attempts to
deserialize a 33-element sequence and asserts an Err with invalid_length; target
the same module that defines RuntimeIdVisitor (the code around fn visit_seq and
the RuntimeIdVisitor type) so the sequence-based path is exercised in addition
to the existing CBOR byte-string tests.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 484-545: Add tests mirroring the WorldlineId CBOR checks for
HeadId to prevent drift: create a round-trip CBOR test (e.g.,
test_head_id_round_trip_uses_cbor_bytes) that encodes a small Wrapper struct
containing HeadId::from_bytes([7u8;32]) and asserts the CBOR map stores
Value::Bytes(32) and that decoding yields the same HeadId, and create a
rejection test (e.g., test_head_id_rejects_non_32_byte_payloads) that tries to
decode 31- and 33-byte Value::Bytes payloads into a Wrapper and asserts the
errors mention "32 bytes"; also add equivalent tests for the types that embed
HeadId (WriterHeadKey and ControlIntentV1::SetHeadEligibility) following the
same round-trip and 31/33-byte rejection patterns so all wrappers enforce the
exact-32-byte invariant.
In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 44-46: The docs omitted the scalar PositiveInt from the frozen
supporting-types list and affected artifacts; add PositiveInt to the list of
supporting types alongside WorldlineId/InboxAddress/etc., include it in the
candidate table of frozen types, and ensure Artifact A (the identifiers
artifact) explicitly declares the PositiveInt scalar so Artifact B (the
routing/admission artifact) that references InboxPolicyBudgeted.maxPerTick and
InboxPolicyInput.maxPerTick is fully specified; update the three affected
sections (supporting-type summary, candidate table, and Artifact A description)
to mention PositiveInt.
In `@package.json`:
- Line 13: The project has duplicated invocation paths for the runtime schema
validator: the npm script "schema:runtime:check" and the shell wrapper in
scripts/verify-local.sh; consolidate by having scripts/verify-local.sh call the
npm script instead of invoking the validator directly (or remove the
package.json script if you prefer the shell wrapper), e.g., update
verify-local.sh to run npm run schema:runtime:check (or yarn run
schema:runtime:check) where it currently shells into
scripts/validate-runtime-schema-fragments.mjs so there is a single canonical
entrypoint named "schema:runtime:check".
In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 17-23: Update the WorldlineId scalar documentation to specify it
is a fixed-width, 32-byte (256-bit) hash-backed opaque identifier (similar to
HeadId): state that implementations MUST produce and accept exactly 32 bytes and
that generators should treat it as a 32-byte hash value; update the semantic
rule paragraph for WorldlineId to explicitly require the 32-byte invariant and
mention any canonical encoding (e.g., raw bytes or hex) expected by the
runtime/ABI so consumers like generators have an unambiguous contract.
---
Duplicate comments:
In `@docs/plans/phase-8-runtime-schema-conformance.md`:
- Around line 56-59: The table entry for `HeadId` is stale and contradicts the
blocker section; update the table row(s) so they state that the ABI now carries
typed wrappers for `HeadId`, `WorldlineId`, and `WriterHeadKey` (remove any
claim that the ABI keeps a byte DTO) to match the later text and current code in
`crates/echo-wasm-abi/src/lib.rs` and `crates/warp-wasm/src/warp_kernel.rs`;
ensure both the `HeadId` and `WorldlineId` rows (and the similar rows at
127-132) reflect the "Aligned" status with typed ABI wrappers and adjust the
Notes column to reference consumption by the listed crates.
In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 75-79: Change the Prettier resolution order to prefer the
workspace-pinned binary first: put the entry that uses localPrettier
(requireExists: true) at the front of the candidates array, followed by the pnpm
exec entry, and leave the npx entry last (use npx with --no-install as a safe
fallback). Update every place where the candidates array is defined (the
instances around the current candidates declaration and the other occurrences
referenced in the review: the blocks covering lines ~85-97 and ~111-119) so they
all use the same ordering (localPrettier, pnpm exec, then npx) to ensure
deterministic formatter resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e8663ec-9f7c-4beb-9fba-01feffefbba7
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
CHANGELOG.mdCONTRIBUTING.mdcrates/echo-runtime-schema/Cargo.tomlcrates/echo-runtime-schema/README.mdcrates/echo-runtime-schema/src/lib.rscrates/echo-wasm-abi/src/lib.rscrates/warp-core/src/provenance_store.rscrates/warp-wasm/src/warp_kernel.rsdocs/dependency-dags.mddocs/plans/phase-8-runtime-schema-conformance.mddocs/plans/phase-8-schema-freeze-inventory.mdpackage.jsonschemas/runtime/artifact-a-identifiers.graphqlschemas/runtime/artifact-b-routing-and-admission.graphqlscripts/validate-runtime-schema-fragments.mjstests/hooks/test_dependency_dags.shtests/hooks/test_runtime_schema_validation.sh
Comment on lines
+214
to
+228
| fn visit_seq<A>(self, mut seq: A) -> Result<Self::Value, A::Error> | ||
| where | ||
| A: SeqAccess<'de>, | ||
| { | ||
| let mut bytes = [0u8; 32]; | ||
| for (index, byte) in bytes.iter_mut().enumerate() { | ||
| *byte = seq | ||
| .next_element()? | ||
| .ok_or_else(|| de::Error::invalid_length(index, &self))?; | ||
| } | ||
| if seq.next_element::<u8>()?.is_some() { | ||
| return Err(de::Error::invalid_length(33, &self)); | ||
| } | ||
| Ok(bytes) | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add a regression test for the sequence-based serde path.
RuntimeIdVisitor::visit_seq() is the code that enforces exact length for decoders emitting a 32-element u8 array, but the current serde tests only round-trip CBOR byte strings. Add one acceptance case for an exact 32-element sequence and one rejection case for 33 elements so alternate decoders cannot regress silently.
Also applies to: 340-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-runtime-schema/src/lib.rs` around lines 214 - 228, Add serde
sequence-path regression tests exercising RuntimeIdVisitor::visit_seq: add one
acceptance test that deserializes a 32-element sequence of u8 values into the
expected [u8; 32] (round-trip or direct deserialization asserting Ok) and one
rejection test that attempts to deserialize a 33-element sequence and asserts an
Err with invalid_length; target the same module that defines RuntimeIdVisitor
(the code around fn visit_seq and the RuntimeIdVisitor type) so the
sequence-based path is exercised in addition to the existing CBOR byte-string
tests.
| "docs:build": "vitepress build docs", | ||
| "test:e2e": "playwright test" | ||
| "test:e2e": "playwright test", | ||
| "schema:runtime:check": "node scripts/validate-runtime-schema-fragments.mjs" |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid a second source of truth for runtime-schema validation.
package.json now exposes schema:runtime:check, but the real local gate in scripts/verify-local.sh still shells into the validator directly. That leaves two invocation paths to keep in sync. Make the shell gate delegate to this script, or drop the wrapper here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 13, The project has duplicated invocation paths for the
runtime schema validator: the npm script "schema:runtime:check" and the shell
wrapper in scripts/verify-local.sh; consolidate by having
scripts/verify-local.sh call the npm script instead of invoking the validator
directly (or remove the package.json script if you prefer the shell wrapper),
e.g., update verify-local.sh to run npm run schema:runtime:check (or yarn run
schema:runtime:check) where it currently shells into
scripts/validate-runtime-schema-fragments.mjs so there is a single canonical
entrypoint named "schema:runtime:check".
Comment on lines
+17
to
+23
| """ | ||
| Opaque stable identifier for a worldline. | ||
|
|
||
| Semantic rule: this remains an opaque identifier even though implementations | ||
| often derive it from the initial worldline state. | ||
| """ | ||
| scalar WorldlineId |
Contributor
There was a problem hiding this comment.
Freeze the byte-width invariant for WorldlineId.
HeadId is documented as fixed-width/hash-backed, but WorldlineId is only described as “opaque”. The runtime and ABI in this PR now enforce exact 32-byte worldline ids, so leaving that out makes Artifact A underspecified for generators.
Suggested edit
"""
Opaque stable identifier for a worldline.
-Semantic rule: this remains an opaque identifier even though implementations
-often derive it from the initial worldline state.
+Semantic rule: this is a fixed-width 32-byte opaque identifier. Consumers must
+preserve opacity and byte width even though implementations may derive it from
+the initial worldline state.
"""
scalar WorldlineId🤖 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 17 - 23, Update
the WorldlineId scalar documentation to specify it is a fixed-width, 32-byte
(256-bit) hash-backed opaque identifier (similar to HeadId): state that
implementations MUST produce and accept exactly 32 bytes and that generators
should treat it as a 32-byte hash value; update the semantic rule paragraph for
WorldlineId to explicitly require the 32-byte invariant and mention any
canonical encoding (e.g., raw bytes or hex) expected by the runtime/ABI so
consumers like generators have an unambiguous contract.
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
package.json (1)
13-13: 🧹 Nitpick | 🔵 TrivialConsolidate runtime schema validation to one canonical entrypoint.
Line 13 adds a package script, but
scripts/verify-local.shstill calls the validator file directly. Keep a single path (prefer delegatingverify-local.shtopnpm schema:runtime:check) to prevent drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 13, Consolidate the runtime schema validation by changing calls in verify-local.sh to invoke the new package script instead of the validator file directly: replace any direct invocation of scripts/validate-runtime-schema-fragments.mjs (e.g., node scripts/validate-runtime-schema-fragments.mjs) with a call to the package script "pnpm run schema:runtime:check" so the canonical entrypoint is schema:runtime:check and the validator path is not duplicated.crates/echo-wasm-abi/src/lib.rs (1)
441-545:⚠️ Potential issue | 🟡 Minor
HeadIdstill appears to lack symmetric exact-32 wire coverage.
SetHeadEligibilityproves the nested happy path, but it does not pinHeadId/WriterHeadKeyagainst 31/33-byte payloads the wayWorldlineIdis now pinned. Add the same direct round-trip and negative cases here so the two opaque 32-byte ID wrappers cannot drift independently.Verify whether equivalent tests already exist anywhere under
crates/echo-wasm-abi; expected result is that only the nestedSetHeadEligibilitypath appears, not a directHeadIdexact-32 rejection test.#!/bin/bash set -euo pipefail sed -n '439,545p' crates/echo-wasm-abi/src/lib.rs rg -n -C2 --type=rust 'fn test_.*head_id|HeadId::from_bytes|WriterHeadKey|32 bytes' crates/echo-wasm-abi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-wasm-abi/src/lib.rs` around lines 441 - 545, Add direct tests for HeadId (and WriterHeadKey if desired) mirroring the WorldlineId tests: create a Wrapper struct containing a HeadId, assert CBOR round-trip serializes as Value::Bytes(32 bytes) and decodes back to the same HeadId (like test_worldline_id_round_trip_uses_cbor_bytes), and add negative cases that feed 31- and 33-byte Value::Bytes payloads and assert decoding fails with an error mentioning "32 bytes" (like test_worldline_id_rejects_non_32_byte_payloads). Locate the test additions near the existing WorldlineId tests and reference HeadId::from_bytes, WriterHeadKey (for nested tests), and any existing SetHeadEligibility assertions to ensure symmetric coverage.crates/ttd-browser/src/lib.rs (1)
1206-1209:⚠️ Potential issue | 🟠 MajorFinish the wasm32
WorldlineIdtest migration.The native test was updated here, but
wasm_tests::test_parse_helpersstill readswl.0at Line 1110. That leaves the wasm32 test target on the pre-freeze API and will fail as soon as those tests are built.Patch
fn test_parse_helpers() { let wl = parse_worldline_id(&[0u8; 32]).unwrap(); - assert_eq!(wl.0, [0u8; 32]); + assert_eq!(*wl.as_bytes(), [0u8; 32]); let invalid = parse_worldline_id(&[0u8; 16]); assert!(invalid.is_err());🤖 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 - 1209, The wasm32 test helper wasm_tests::test_parse_helpers still accesses the old tuple field (wl.0) of WorldlineId; update it to use the current API (call as_bytes() on the WorldlineId result) consistent with the native test changes: locate parse_worldline_id_inner and the wasm_tests::test_parse_helpers reference to wl.0, change the access to use wl.unwrap().as_bytes() (or call as_bytes() on the unwrapped WorldlineId) so the wasm test uses the new WorldlineId API.schemas/runtime/artifact-a-identifiers.graphql (1)
8-15:⚠️ Potential issue | 🟠 MajorFreeze the exact byte width for
HeadIdandWorldlineId.Artifact A is supposed to be the frozen boundary contract, but these docs still leave the ABI-critical width implicit. The runtime crate and ABI wrappers now treat both IDs as exact 32-byte values; without that sentence, generators can still model them as variable-length bytes or strings and remain "schema compliant".
Doc patch
""" Opaque stable identifier for a writer head. -Semantic rule: this is a fixed-width hash-backed identifier. Schema consumers +Semantic rule: this is an exact 32-byte hash-backed identifier. Schema consumers must preserve opacity and must not reinterpret it as a human-readable string label or derive ordering semantics beyond byte-stable identity. """ scalar HeadId """ Opaque stable identifier for a worldline. -Semantic rule: this remains an opaque identifier even though implementations -often derive it from the initial worldline state. +Semantic rule: this is an exact 32-byte opaque identifier. Consumers must +preserve opacity and byte width even though implementations often derive it +from the initial worldline state. """ scalar WorldlineIdAlso applies to: 17-23
🤖 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 8 - 15, Update the scalar docs to explicitly freeze the ABI width: change the HeadId and WorldlineId docblocks to state they are exact 32-byte (256-bit) opaque identifiers and must be encoded/handled as fixed 32-byte values (not variable-length strings or byte arrays). Locate the scalar definitions for HeadId and WorldlineId in this file (symbols HeadId and WorldlineId) and replace the current vague "fixed-width hash-backed identifier" wording with a precise sentence such as "This is an opaque, ABI-fixed 32-byte identifier; consumers must preserve opacity and treat it as exactly 32 bytes." Apply the same precise wording to both docblocks so generators cannot model them as variable-length.docs/plans/phase-8-schema-freeze-inventory.md (2)
97-106:⚠️ Potential issue | 🟡 MinorArtifact A enumeration is missing
PositiveInt.The scalar is defined in
artifact-a-identifiers.graphqlalongside the other identifiers and counters but not listed here.Proposed fix
### Artifact A: Runtime identifiers and logical counters - `HeadId` - `WorldlineId` - `WriterHeadKey` - `IntentKind` - `WorldlineTick` - `GlobalTick` +- `PositiveInt`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 97 - 106, The docs list for "Artifact A: Runtime identifiers and logical counters" forgot to include the PositiveInt scalar defined alongside other identifiers in artifact-a-identifiers.graphql; update the enumeration under Artifact A to include `PositiveInt` (alongside `HeadId`, `WorldlineId`, `WriterHeadKey`, `IntentKind`, `WorldlineTick`, `GlobalTick`) and ensure the source reference to artifact-a-identifiers.graphql explicitly mentions PositiveInt so the schema and docs remain in sync.
44-46:⚠️ Potential issue | 🟡 Minor
PositiveIntis missing from the supporting-type list.The schema fragment
artifact-a-identifiers.graphqldefinesscalar PositiveIntwhich is referenced byartifact-b-routing-and-admission.graphqlforInboxPolicyBudgeted.maxPerTick. This document lists supporting types at line 45-46 but omitsPositiveInt.Proposed fix
5. Freezing those runtime surfaces also requires a few supporting types that the earlier plan text underspecified: `WorldlineId`, `InboxAddress`, `SeekThen`, - `SchedulerMode`, and `RunId`. + `SchedulerMode`, `RunId`, and `PositiveInt`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 44 - 46, The supporting-type list omitted the PositiveInt scalar used by InboxPolicyBudgeted.maxPerTick; update the supporting types to include PositiveInt so the runtime freeze plan accounts for the scalar referenced in artifact-a-identifiers.graphql (used by artifact-b-routing-and-admission.graphql) and ensure any mention of InboxPolicyBudgeted.maxPerTick is covered by the added PositiveInt entry.crates/echo-runtime-schema/src/lib.rs (1)
214-228: 🧹 Nitpick | 🔵 TrivialSequence-path deserialization lacks explicit test coverage.
visit_seqenforces exactly 32 elements (lines 218-227) but no test exercises this path. The CBOR tests useValue::Bytes, notValue::Array. A past review comment flagged this gap.Add tests that:
- Deserialize a 32-element
u8array intoWorldlineId/HeadId.- Reject a 33-element array with
invalid_length.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-runtime-schema/src/lib.rs` around lines 214 - 228, Add unit tests that cover the sequence-path in visit_seq by deserializing array forms into WorldlineId and HeadId: construct a serde_cbor/serde Value::Array (or equivalent CBOR array bytes) containing exactly 32 u8 elements, call the crate's Deserialize (e.g., deserialize/from_slice) for WorldlineId and HeadId and assert success and equality to the expected 32-byte value; then construct a 33-element u8 array and assert deserialization fails with de::Error::invalid_length (or results in an Err matching invalid_length). Reference the visit_seq implementation and exercise WorldlineId and HeadId deserialization to ensure both the valid-32 and invalid-33 paths are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 397-445: Update the "T-10-8-10: Feature-Gate Contract
Verification" task spec to mandate the maintenance entrypoint be a cargo xtask
command (e.g., "cargo xtask feature-gate-verify") by adding that shape to the
Requirements (R2) and Acceptance Criteria (AC1) so the verification is
implemented under the repo's xtask surface; apply the same change to the other
backlog item in this document so both tasks explicitly require a cargo xtask
entrypoint and mention xtask/ and .cargo/config.toml as the expected
location/configuration.
In `@scripts/generate-tasks-dag.js`:
- Line 10: INPUT_FILE is currently used both for reading the filesystem and for
emitting DOT labels, which causes Windows backslashes to be escaped and produce
non-deterministic output; fix by separating the two concerns: keep a
filesystem-native path (e.g., INPUT_FILE_FS = path.join(...)) for fs operations
and create a POSIX-normalized display path (e.g., INPUT_FILE_DISPLAY =
path.posix.join(...) or use path.posix.normalize) for any DOT label emission
passed into escapeDotString(), and update all places that emit file paths to use
INPUT_FILE_DISPLAY (including the similar uses around the escapeDotString()
calls referenced in the 175-185 area).
In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 11-17: DEFINITION_KIND_NAMES currently omits GraphQL interfaces so
nodes with kind "InterfaceTypeDefinition" won't be recognized; update the
DEFINITION_KIND_NAMES Map (the constant named DEFINITION_KIND_NAMES in
scripts/validate-runtime-schema-fragments.mjs) to include
["InterfaceTypeDefinition", "interface"] so interface definitions are collected
like other types and won't trigger false missing-type errors.
---
Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 214-228: Add unit tests that cover the sequence-path in visit_seq
by deserializing array forms into WorldlineId and HeadId: construct a
serde_cbor/serde Value::Array (or equivalent CBOR array bytes) containing
exactly 32 u8 elements, call the crate's Deserialize (e.g.,
deserialize/from_slice) for WorldlineId and HeadId and assert success and
equality to the expected 32-byte value; then construct a 33-element u8 array and
assert deserialization fails with de::Error::invalid_length (or results in an
Err matching invalid_length). Reference the visit_seq implementation and
exercise WorldlineId and HeadId deserialization to ensure both the valid-32 and
invalid-33 paths are covered.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 441-545: Add direct tests for HeadId (and WriterHeadKey if
desired) mirroring the WorldlineId tests: create a Wrapper struct containing a
HeadId, assert CBOR round-trip serializes as Value::Bytes(32 bytes) and decodes
back to the same HeadId (like test_worldline_id_round_trip_uses_cbor_bytes), and
add negative cases that feed 31- and 33-byte Value::Bytes payloads and assert
decoding fails with an error mentioning "32 bytes" (like
test_worldline_id_rejects_non_32_byte_payloads). Locate the test additions near
the existing WorldlineId tests and reference HeadId::from_bytes, WriterHeadKey
(for nested tests), and any existing SetHeadEligibility assertions to ensure
symmetric coverage.
In `@crates/ttd-browser/src/lib.rs`:
- Around line 1206-1209: The wasm32 test helper wasm_tests::test_parse_helpers
still accesses the old tuple field (wl.0) of WorldlineId; update it to use the
current API (call as_bytes() on the WorldlineId result) consistent with the
native test changes: locate parse_worldline_id_inner and the
wasm_tests::test_parse_helpers reference to wl.0, change the access to use
wl.unwrap().as_bytes() (or call as_bytes() on the unwrapped WorldlineId) so the
wasm test uses the new WorldlineId API.
In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-106: The docs list for "Artifact A: Runtime identifiers and
logical counters" forgot to include the PositiveInt scalar defined alongside
other identifiers in artifact-a-identifiers.graphql; update the enumeration
under Artifact A to include `PositiveInt` (alongside `HeadId`, `WorldlineId`,
`WriterHeadKey`, `IntentKind`, `WorldlineTick`, `GlobalTick`) and ensure the
source reference to artifact-a-identifiers.graphql explicitly mentions
PositiveInt so the schema and docs remain in sync.
- Around line 44-46: The supporting-type list omitted the PositiveInt scalar
used by InboxPolicyBudgeted.maxPerTick; update the supporting types to include
PositiveInt so the runtime freeze plan accounts for the scalar referenced in
artifact-a-identifiers.graphql (used by
artifact-b-routing-and-admission.graphql) and ensure any mention of
InboxPolicyBudgeted.maxPerTick is covered by the added PositiveInt entry.
In `@package.json`:
- Line 13: Consolidate the runtime schema validation by changing calls in
verify-local.sh to invoke the new package script instead of the validator file
directly: replace any direct invocation of
scripts/validate-runtime-schema-fragments.mjs (e.g., node
scripts/validate-runtime-schema-fragments.mjs) with a call to the package script
"pnpm run schema:runtime:check" so the canonical entrypoint is
schema:runtime:check and the validator path is not duplicated.
In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 8-15: Update the scalar docs to explicitly freeze the ABI width:
change the HeadId and WorldlineId docblocks to state they are exact 32-byte
(256-bit) opaque identifiers and must be encoded/handled as fixed 32-byte values
(not variable-length strings or byte arrays). Locate the scalar definitions for
HeadId and WorldlineId in this file (symbols HeadId and WorldlineId) and replace
the current vague "fixed-width hash-backed identifier" wording with a precise
sentence such as "This is an opaque, ABI-fixed 32-byte identifier; consumers
must preserve opacity and treat it as exactly 32 bytes." Apply the same precise
wording to both docblocks so generators cannot model them as variable-length.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac7c8b57-ecbb-409d-9065-0672f405a259
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockdocs/assets/dags/tasks-dag.dotis excluded by!**/*.dotdocs/assets/dags/tasks-dag.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
.gitignore.vscode/settings.jsonCHANGELOG.mdCONTRIBUTING.mdCargo.tomlcrates/echo-dind-harness/tests/digest_golden_vectors.rscrates/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/codec.rscrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/ttd-browser/src/lib.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/clock.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/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.mddocs/dependency-dags.mddocs/guide/cargo-features.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_dependency_dags.shtests/hooks/test_runtime_schema_validation.sh
Comment on lines
+397
to
+445
| ## T-10-8-10: Feature-Gate Contract Verification | ||
|
|
||
| **User Story:** As a contributor, I want explicit feature-contract checks for | ||
| no-std / alloc-only crates so that feature-gating regressions are caught before | ||
| PR review or CI. | ||
|
|
||
| **Requirements:** | ||
|
|
||
| - R1: Identify crates whose manifests promise meaningful `--no-default-features` | ||
| or alloc-only support | ||
| - R2: Add a local and CI-visible verification path that exercises those feature | ||
| contracts directly | ||
| - R3: Keep the lane scoped so it stays fast enough to run during review-fix | ||
| loops | ||
| - R4: Document which crates are covered and what the lane is proving | ||
|
|
||
| **Acceptance Criteria:** | ||
|
|
||
| - [ ] AC1: At least the current runtime-schema / ABI crates have an explicit | ||
| `--no-default-features` check | ||
| - [ ] AC2: A deliberate `std` leak in a gated crate fails the lane | ||
| - [ ] AC3: Contributor docs explain when to run the lane and what it covers | ||
| - [ ] AC4: The covered crate list is easy to keep aligned with manifest truth | ||
|
|
||
| **Definition of Done:** | ||
|
|
||
| - [ ] Code reviewed and merged | ||
| - [ ] Tests pass (CI green) | ||
| - [ ] Documentation updated (if applicable) | ||
|
|
||
| **Scope:** Feature-gate verification for crates that claim no-std or alloc-only | ||
| support. | ||
| **Out of Scope:** Broad workspace-wide no-std support or changing crate feature | ||
| semantics. | ||
|
|
||
| **Test Plan:** | ||
|
|
||
| - **Goldens:** n/a | ||
| - **Failures:** Intentionally introduce a `std` dependency in a gated path and | ||
| verify the lane fails | ||
| - **Edges:** `default-features = false`, alloc-only mode, transitive feature | ||
| forwarding | ||
| - **Fuzz/Stress:** n/a | ||
|
|
||
| **Blocked By:** none | ||
| **Blocking:** none | ||
|
|
||
| **Est. Hours:** 2h | ||
| **Expected Complexity:** ~60 LoC (lane wiring + docs) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Define the maintenance entrypoint in the task spec.
Both backlog items describe new repo-facing commands, but neither says they should land under cargo xtask. Leaving that open makes it too easy for the eventual implementation to become another one-off script surface. Add the expected cargo xtask ... shape to the requirements or AC1 for both tasks now.
Based on learnings: "Preferred repo maintenance entrypoint is cargo xtask ... (see xtask/ and .cargo/config.toml)".
Also applies to: 449-493
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/ROADMAP/backlog/tooling-misc.md` around lines 397 - 445, Update the
"T-10-8-10: Feature-Gate Contract Verification" task spec to mandate the
maintenance entrypoint be a cargo xtask command (e.g., "cargo xtask
feature-gate-verify") by adding that shape to the Requirements (R2) and
Acceptance Criteria (AC1) so the verification is implemented under the repo's
xtask surface; apply the same change to the other backlog item in this document
so both tasks explicitly require a cargo xtask entrypoint and mention xtask/ and
.cargo/config.toml as the expected location/configuration.
| import { escapeDotString } from "./dag-utils.js"; | ||
|
|
||
| const INPUT_FILE = "TASKS-DAG.md"; | ||
| const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, does path.join("docs","archive","tasks","TASKS-DAG.md") use platform-specific separators on Windows, and does Graphviz DOT treat backslashes inside label strings as escapes?
💡 Result:
Yes, on Windows, path.join("docs","archive","tasks","TASKS-DAG.md") uses platform-specific backslash separators, producing "docs\archive\tasks\TASKS-DAG.md". Yes, Graphviz DOT treats backslashes in label strings as escape characters; a literal backslash must be written as \ in the DOT source.
Citations:
- 1: https://nodejs.org/api/path.html
- 2: https://stackoverflow.com/questions/39733135/path-module-in-node-js-reversing-slashes-for-some-reason
- 3: https://nodejs.org/docs/latest-v18.x/api/path.html
- 4: https://graphviz.org/docs/attr-types/escString/
- 5: https://stackoverflow.com/questions/34294445/how-can-i-use-in-a-label-in-graphviz-using-dot
- 6: https://stackoverflow.com/questions/74200698/how-do-you-escape-strings-when-generating-graphviz
🏁 Script executed:
#!/bin/bash
# First, check the file exists and examine line 10 and surrounding context
echo "=== Line 10 and context ==="
sed -n '1,20p' scripts/generate-tasks-dag.js
echo -e "\n=== Lines 175-185 context ==="
sed -n '170,190p' scripts/generate-tasks-dag.jsRepository: flyingrobots/echo
Length of output: 1671
🏁 Script executed:
#!/bin/bash
# Find and display escapeDotString function implementation
echo "=== Searching for escapeDotString function ==="
rg -n "escapeDotString" scripts/generate-tasks-dag.js -B 2 -A 8Repository: flyingrobots/echo
Length of output: 2800
🏁 Script executed:
#!/bin/bash
# Check if generated output files are in version control
echo "=== Checking for generated DOT/graph files in repo ==="
git ls-files | grep -E '\.(dot|svg|png|gv)$'
echo -e "\n=== Checking git status for potential generated files ==="
git status --short | head -20Repository: flyingrobots/echo
Length of output: 4717
🏁 Script executed:
#!/bin/bash
# Find and display escapeDotString function implementation
echo "=== escapeDotString function in dag-utils.js ==="
find scripts -name "dag-utils.js" -type f | head -1 | xargs catRepository: flyingrobots/echo
Length of output: 1614
Separate filesystem path from DOT display path to ensure deterministic output.
INPUT_FILE serves double duty: filesystem access and DOT label emission. On Windows, path.join() produces backslashes, which escapeDotString() then doubles for DOT escaping, resulting in platform-dependent committed output. Keep filesystem paths OS-native and display paths POSIX-normalized for reproducible generated artifacts.
Suggested fix
-const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md");
+const INPUT_FILE_DISPLAY = "docs/archive/tasks/TASKS-DAG.md";
+const INPUT_FILE = path.join(...INPUT_FILE_DISPLAY.split("/"));
@@
- ` label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE)})\\nGenerated by scripts/generate-tasks-dag.js";`,
+ ` label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE_DISPLAY)})\\nGenerated by scripts/generate-tasks-dag.js";`,
@@
- ` LG [label="confirmed in ${escapeDotString(INPUT_FILE)}", color="green", fontcolor="green"];`,
+ ` LG [label="confirmed in ${escapeDotString(INPUT_FILE_DISPLAY)}", color="green", fontcolor="green"];`,Also applies to: 175-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/generate-tasks-dag.js` at line 10, INPUT_FILE is currently used both
for reading the filesystem and for emitting DOT labels, which causes Windows
backslashes to be escaped and produce non-deterministic output; fix by
separating the two concerns: keep a filesystem-native path (e.g., INPUT_FILE_FS
= path.join(...)) for fs operations and create a POSIX-normalized display path
(e.g., INPUT_FILE_DISPLAY = path.posix.join(...) or use path.posix.normalize)
for any DOT label emission passed into escapeDotString(), and update all places
that emit file paths to use INPUT_FILE_DISPLAY (including the similar uses
around the escapeDotString() calls referenced in the 175-185 area).
Comment on lines
+11
to
+17
| const DEFINITION_KIND_NAMES = new Map([ | ||
| ["ScalarTypeDefinition", "scalar"], | ||
| ["ObjectTypeDefinition", "type"], | ||
| ["InputObjectTypeDefinition", "input"], | ||
| ["EnumTypeDefinition", "enum"], | ||
| ["UnionTypeDefinition", "union"], | ||
| ]); |
Contributor
There was a problem hiding this comment.
InterfaceTypeDefinition is not handled.
The DEFINITION_KIND_NAMES map omits InterfaceTypeDefinition. If the runtime schema ever includes GraphQL interfaces, they won't be collected as definitions, causing false "missing referenced type" errors when other types implement them.
Proposed fix
const DEFINITION_KIND_NAMES = new Map([
["ScalarTypeDefinition", "scalar"],
["ObjectTypeDefinition", "type"],
["InputObjectTypeDefinition", "input"],
["EnumTypeDefinition", "enum"],
["UnionTypeDefinition", "union"],
+ ["InterfaceTypeDefinition", "interface"],
]);📝 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
| const DEFINITION_KIND_NAMES = new Map([ | |
| ["ScalarTypeDefinition", "scalar"], | |
| ["ObjectTypeDefinition", "type"], | |
| ["InputObjectTypeDefinition", "input"], | |
| ["EnumTypeDefinition", "enum"], | |
| ["UnionTypeDefinition", "union"], | |
| ]); | |
| const DEFINITION_KIND_NAMES = new Map([ | |
| ["ScalarTypeDefinition", "scalar"], | |
| ["ObjectTypeDefinition", "type"], | |
| ["InputObjectTypeDefinition", "input"], | |
| ["EnumTypeDefinition", "enum"], | |
| ["UnionTypeDefinition", "union"], | |
| ["InterfaceTypeDefinition", "interface"], | |
| ]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/validate-runtime-schema-fragments.mjs` around lines 11 - 17,
DEFINITION_KIND_NAMES currently omits GraphQL interfaces so nodes with kind
"InterfaceTypeDefinition" won't be recognized; update the DEFINITION_KIND_NAMES
Map (the constant named DEFINITION_KIND_NAMES in
scripts/validate-runtime-schema-fragments.mjs) to include
["InterfaceTypeDefinition", "interface"] so interface definitions are collected
like other types and won't trigger false missing-type errors.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
schemas/runtime/artifact-a-identifiers.graphql (1)
17-23:⚠️ Potential issue | 🟠 Major
WorldlineIdscalar is still underspecified for frozen ABI/schema parity.Line 20 keeps opacity semantics, but the fixed-width invariant is missing. For Phase 8 freeze, document
WorldlineIdas exactly 32 bytes (opaque, no ordering semantics), otherwise generators and adapters can drift on accepted payload width.Proposed doc fix
""" Opaque stable identifier for a worldline. -Semantic rule: this remains an opaque identifier even though implementations -often derive it from the initial worldline state. +Semantic rule: this is a fixed-width 32-byte opaque identifier. Consumers must +preserve opacity and accept/emit exactly 32 bytes (256 bits), even though +implementations may derive it from initial worldline state. """ scalar WorldlineId🤖 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 17 - 23, The WorldlineId scalar is underspecified: update the scalar documentation for scalar WorldlineId to state it is an opaque, fixed-width 32-byte identifier (exactly 32 bytes), with no ordering semantics, to guarantee frozen ABI/schema parity for Phase 8; modify the comment block above scalar WorldlineId to explicitly document "exactly 32 bytes (opaque, no ordering semantics)" and include any expected external encoding note (e.g., raw 32 bytes or canonical hex/base64) so generators and adapters have a clear payload width.crates/echo-wasm-abi/src/lib.rs (1)
440-482:⚠️ Potential issue | 🟡 MinorFreeze
HeadIdwire shape explicitly.The new
SetHeadEligibilityround-trip only proves semantic equality after decode. It would still pass ifHeadId/WriterHeadKeydrifted from CBOR bytes(32) to another serde shape, so this file only pins the frozen wire shape forWorldlineIdright now. Add the same bytes(32) and 31/33-byte rejection coverage for theHeadIdpath.Also applies to: 484-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-wasm-abi/src/lib.rs` around lines 440 - 482, The test_control_intent_round_trip needs to explicitly pin HeadId's CBOR wire shape like WorldlineId does: update the SetHeadEligibility branch to (1) assert that packing/unpacking preserves a HeadId constructed from exactly 32 bytes (e.g. HeadId::from_bytes([2u8;32])) and (2) add negative-case assertions that decoding/reconstruction rejects 31-byte and 33-byte inputs for HeadId (mirror the WorldlineId byte-length checks), using the same helpers pack_control_intent_v1/unpack_control_intent_v1 and the WriterHeadKey type so the test fails if HeadId’s serde wire shape drifts.docs/ROADMAP/backlog/tooling-misc.md (1)
405-419:⚠️ Potential issue | 🟡 MinorPin both new tooling tasks to
cargo xtask.Both specs still say “one command” / “local and CI-visible path” without naming the repo maintenance entrypoint. Leave that vague and the eventual implementation can easily become another standalone script surface. Require explicit
cargo xtask ...commands in the Requirements and AC for both tasks. Based on learnings: "Preferred repo maintenance entrypoint is cargo xtask ... (see xtask/ and .cargo/config.toml)."Also applies to: 457-469
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ROADMAP/backlog/tooling-misc.md` around lines 405 - 419, Update the roadmap entries (the "R1-R4" checklist and the corresponding Acceptance Criteria) to require invoking both new tooling tasks via the repository xtask entrypoint (i.e., prepend "cargo xtask ..." to the one-command/local-and-CI-visible paths), and update AC items (AC1–AC4 and the similar block later in the file) to explicitly state "run via cargo xtask" and reference the repo xtask surface (xtask/ and .cargo/config.toml) so implementers know where the command lives; ensure both task specs and acceptance checks are changed consistently wherever the tasks are described in the document.scripts/generate-tasks-dag.js (1)
10-10:⚠️ Potential issue | 🟡 MinorSeparate the filesystem path from the DOT display path.
INPUT_FILEnow feeds both file I/O and label text. On Windows,path.join()will emit backslashes, so the generated DOT/SVG labels become platform-dependent once passed throughescapeDotString(). Keep a native path for reads and a POSIX-normalized path for emitted labels.Suggested fix
-const INPUT_FILE = path.join("docs", "archive", "tasks", "TASKS-DAG.md"); +const INPUT_FILE_DISPLAY = "docs/archive/tasks/TASKS-DAG.md"; +const INPUT_FILE = path.join(...INPUT_FILE_DISPLAY.split("/")); @@ - ` label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE)})\\nGenerated by scripts/generate-tasks-dag.js";`, + ` label="Echo — Tasks DAG (from ${escapeDotString(INPUT_FILE_DISPLAY)})\\nGenerated by scripts/generate-tasks-dag.js";`, @@ - ` LG [label="confirmed in ${escapeDotString(INPUT_FILE)}", color="green", fontcolor="green"];`, + ` LG [label="confirmed in ${escapeDotString(INPUT_FILE_DISPLAY)}", color="green", fontcolor="green"];`,In Node.js, does path.join("docs","archive","tasks","TASKS-DAG.md") use platform-specific separators on Windows, and does Graphviz DOT treat backslashes in label strings as escapes?Also applies to: 175-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/generate-tasks-dag.js` at line 10, INPUT_FILE is currently used both for filesystem reads and as the label text, causing backslashes on Windows to leak into DOT labels; change the code to keep a native path for I/O (leave INPUT_FILE = path.join(...)) and create a separate POSIX-normalized label path (e.g., use path.posix.join or replace backslashes via path.resolve(...).split(path.sep).join("/")) for any text passed to escapeDotString()/DOT generation (update all usages where INPUT_FILE is passed into escapeDotString or label creation, including the later block around the previous 175-185 region) so labels always use forward slashes while file I/O still uses the platform-native path.scripts/validate-runtime-schema-fragments.mjs (1)
11-17:⚠️ Potential issue | 🟡 Minor
InterfaceTypeDefinitionis still missing.If the runtime schema ever includes GraphQL interfaces, they won't be collected, causing false "missing referenced type" errors.
Fix
const DEFINITION_KIND_NAMES = new Map([ ["ScalarTypeDefinition", "scalar"], ["ObjectTypeDefinition", "type"], ["InputObjectTypeDefinition", "input"], ["EnumTypeDefinition", "enum"], ["UnionTypeDefinition", "union"], + ["InterfaceTypeDefinition", "interface"], ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/validate-runtime-schema-fragments.mjs` around lines 11 - 17, DEFINITION_KIND_NAMES map is missing "InterfaceTypeDefinition" which causes GraphQL interfaces to be uncollected and trigger false "missing referenced type" errors; add an entry mapping "InterfaceTypeDefinition" to "interface" in the DEFINITION_KIND_NAMES Map so interface definitions are recognized (update the constant DEFINITION_KIND_NAMES where the other kind-name pairs are defined).crates/echo-runtime-schema/src/lib.rs (1)
164-170:⚠️ Potential issue | 🟡 MinorAdd local regression coverage for the non-canonical runtime-id decodes.
This module owns the exact-length serde contract, but the tests only pin happy-path CBOR byte strings. Add local cases for 31-byte and 33-byte byte strings plus 32-element and 33-element
u8sequences sodecode_runtime_id()andvisit_seq()cannot drift behind downstream ABI coverage.Also applies to: 214-228, 340-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/echo-runtime-schema/src/lib.rs` around lines 164 - 170, Add unit tests that assert decoding rejects non-canonical lengths: add cases that attempt to deserialize 31-byte and 33-byte byte strings and 32-element and 33-element u8 sequences to ensure decode_runtime_id() and visit_seq() enforce exact 32-byte length; specifically, create tests that call the deserializer on CBOR/serde inputs representing a 31-byte bytes, a 33-byte bytes, a seq of 32 u8s (valid) and a seq of 33 u8s (invalid) and assert success only for the 32-byte cases and errors for others, which will exercise decode_runtime_id() and the visitor implementation (visit_seq) to prevent regressions.
🤖 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`:
- Line 35: The struct currently exposes its inner counter as pub (pub struct
$name(pub u64)), allowing callers to bypass checked arithmetic; change the inner
field to private (e.g., pub struct $name(u64)) and ensure all external access
uses the existing from_raw and as_u64 functions (or associated methods) as the
documented escape hatches; update any constructors or direct field accesses in
code to call $name::from_raw(...) and use .as_u64() instead of referencing .0 so
all arithmetic goes through the checked-arithmetic API boundary.
In `@docs/guide/cargo-features.md`:
- Around line 80-90: The documentation snapshot metadata at the top still reads
"generated as of 2026-03-07" and is now stale; update that generated-snapshot
date string to the current correct date to match the new `echo-runtime-schema`
section content (search for the literal "generated as of 2026-03-07" in
docs/guide/cargo-features.md and replace it with the updated date), ensuring the
snapshot date reflects when the file was regenerated.
In `@docs/plans/phase-8-schema-freeze-inventory.md`:
- Around line 97-106: The Artifact A documentation list is missing the
scalar/type PositiveInt; update the Artifact A inventory to include
`PositiveInt` alongside HeadId, WorldlineId, WriterHeadKey, IntentKind,
WorldlineTick, and GlobalTick so that the listing matches the definition in
artifact-a-identifiers.graphql (where PositiveInt is declared) and reflects its
use (PositiveInt!) in artifact-b and artifact-d.
In `@schemas/runtime/artifact-d-scheduler-results.graphql`:
- Around line 91-100: The SchedulerStatus type lacks in-schema documentation of
nullability semantics for fields clients branch on; update the SchedulerStatus
definition (fields: activeMode, runId, latestCycleGlobalTick,
latestCommitGlobalTick, lastQuiescentGlobalTick, lastRunCompletion) to include
short GraphQL field descriptions that state what a null vs non-null value means
(e.g., null = scheduler inactive, null = pre-first-run, null = impossible given
current state), and if any field should be non-null by invariant, make its type
non-nullable (or explicitly document why it can be null) so the SDL itself
encodes the state-coupled behavior alongside the field (use GraphQL description
strings or comments directly above each field in the SchedulerStatus type).
---
Duplicate comments:
In `@crates/echo-runtime-schema/src/lib.rs`:
- Around line 164-170: Add unit tests that assert decoding rejects non-canonical
lengths: add cases that attempt to deserialize 31-byte and 33-byte byte strings
and 32-element and 33-element u8 sequences to ensure decode_runtime_id() and
visit_seq() enforce exact 32-byte length; specifically, create tests that call
the deserializer on CBOR/serde inputs representing a 31-byte bytes, a 33-byte
bytes, a seq of 32 u8s (valid) and a seq of 33 u8s (invalid) and assert success
only for the 32-byte cases and errors for others, which will exercise
decode_runtime_id() and the visitor implementation (visit_seq) to prevent
regressions.
In `@crates/echo-wasm-abi/src/lib.rs`:
- Around line 440-482: The test_control_intent_round_trip needs to explicitly
pin HeadId's CBOR wire shape like WorldlineId does: update the
SetHeadEligibility branch to (1) assert that packing/unpacking preserves a
HeadId constructed from exactly 32 bytes (e.g. HeadId::from_bytes([2u8;32])) and
(2) add negative-case assertions that decoding/reconstruction rejects 31-byte
and 33-byte inputs for HeadId (mirror the WorldlineId byte-length checks), using
the same helpers pack_control_intent_v1/unpack_control_intent_v1 and the
WriterHeadKey type so the test fails if HeadId’s serde wire shape drifts.
In `@docs/ROADMAP/backlog/tooling-misc.md`:
- Around line 405-419: Update the roadmap entries (the "R1-R4" checklist and the
corresponding Acceptance Criteria) to require invoking both new tooling tasks
via the repository xtask entrypoint (i.e., prepend "cargo xtask ..." to the
one-command/local-and-CI-visible paths), and update AC items (AC1–AC4 and the
similar block later in the file) to explicitly state "run via cargo xtask" and
reference the repo xtask surface (xtask/ and .cargo/config.toml) so implementers
know where the command lives; ensure both task specs and acceptance checks are
changed consistently wherever the tasks are described in the document.
In `@schemas/runtime/artifact-a-identifiers.graphql`:
- Around line 17-23: The WorldlineId scalar is underspecified: update the scalar
documentation for scalar WorldlineId to state it is an opaque, fixed-width
32-byte identifier (exactly 32 bytes), with no ordering semantics, to guarantee
frozen ABI/schema parity for Phase 8; modify the comment block above scalar
WorldlineId to explicitly document "exactly 32 bytes (opaque, no ordering
semantics)" and include any expected external encoding note (e.g., raw 32 bytes
or canonical hex/base64) so generators and adapters have a clear payload width.
In `@scripts/generate-tasks-dag.js`:
- Line 10: INPUT_FILE is currently used both for filesystem reads and as the
label text, causing backslashes on Windows to leak into DOT labels; change the
code to keep a native path for I/O (leave INPUT_FILE = path.join(...)) and
create a separate POSIX-normalized label path (e.g., use path.posix.join or
replace backslashes via path.resolve(...).split(path.sep).join("/")) for any
text passed to escapeDotString()/DOT generation (update all usages where
INPUT_FILE is passed into escapeDotString or label creation, including the later
block around the previous 175-185 region) so labels always use forward slashes
while file I/O still uses the platform-native path.
In `@scripts/validate-runtime-schema-fragments.mjs`:
- Around line 11-17: DEFINITION_KIND_NAMES map is missing
"InterfaceTypeDefinition" which causes GraphQL interfaces to be uncollected and
trigger false "missing referenced type" errors; add an entry mapping
"InterfaceTypeDefinition" to "interface" in the DEFINITION_KIND_NAMES Map so
interface definitions are recognized (update the constant DEFINITION_KIND_NAMES
where the other kind-name pairs are defined).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c9510ed-4f96-4019-92af-60ebd00a54e5
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockdocs/assets/dags/tasks-dag.dotis excluded by!**/*.dotdocs/assets/dags/tasks-dag.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
.gitignore.vscode/settings.jsonCHANGELOG.mdCONTRIBUTING.mdCargo.tomlcrates/echo-dind-harness/tests/digest_golden_vectors.rscrates/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/codec.rscrates/echo-wasm-abi/src/kernel_port.rscrates/echo-wasm-abi/src/lib.rscrates/ttd-browser/src/lib.rscrates/warp-core/Cargo.tomlcrates/warp-core/src/clock.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/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/ROADMAP/backlog/README.mddocs/ROADMAP/backlog/tooling-misc.mddocs/dependency-dags.mddocs/guide/cargo-features.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_dependency_dags.shtests/hooks/test_runtime_schema_validation.sh
| #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Default)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] | ||
| #[cfg_attr(feature = "serde", serde(transparent))] | ||
| pub struct $name(pub u64); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Examine the file around line 35
head -80 crates/echo-runtime-schema/src/lib.rs | tail -50Repository: flyingrobots/echo
Length of output: 1727
🏁 Script executed:
# Get the full macro definition to understand what's being generated
sed -n '1,100p' crates/echo-runtime-schema/src/lib.rsRepository: flyingrobots/echo
Length of output: 3246
🏁 Script executed:
# Search for from_raw and as_u64 references in the file
rg "from_raw|as_u64" crates/echo-runtime-schema/src/lib.rs -A 2 -B 2Repository: flyingrobots/echo
Length of output: 1020
🏁 Script executed:
# Find where the logical_counter! macro is invoked to see if the generated types have rustdoc
rg "logical_counter!" crates/echo-runtime-schema/src/lib.rs -A 1 -B 1Repository: flyingrobots/echo
Length of output: 309
🏁 Script executed:
# Check if there's rustdoc on the macro definition itself
sed -n '20,32p' crates/echo-runtime-schema/src/lib.rsRepository: flyingrobots/echo
Length of output: 416
Make the logical counter field private to enforce the checked-arithmetic API boundary.
pub struct $name(pub u64); allows callers to bypass checked arithmetic via direct construction Type(value) and field access .0. Change the field to private and route all access through from_raw and as_u64, which already exist as documented escape hatches.
Suggested change
- pub struct $name(pub u64);
+ pub struct $name(u64);📝 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
| pub struct $name(pub u64); | |
| pub struct $name(u64); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/echo-runtime-schema/src/lib.rs` at line 35, The struct currently
exposes its inner counter as pub (pub struct $name(pub u64)), allowing callers
to bypass checked arithmetic; change the inner field to private (e.g., pub
struct $name(u64)) and ensure all external access uses the existing from_raw and
as_u64 functions (or associated methods) as the documented escape hatches;
update any constructors or direct field accesses in code to call
$name::from_raw(...) and use .as_u64() instead of referencing .0 so all
arithmetic goes through the checked-arithmetic API boundary.
Comment on lines
+80
to
+90
| | Feature | Default | Description | | ||
| | ------- | ------- | ---------------------------------------------------------------------------------------------------------- | | ||
| | `std` | **yes** | Standard library support (enables `serde/std`, `ciborium/std`, `half/std`, and `echo-runtime-schema/std`). | | ||
| | `alloc` | no | Alloc-only mode for `no_std` environments. | | ||
|
|
||
| ## echo-runtime-schema | ||
|
|
||
| | Feature | Default | Description | | ||
| | ------- | ------- | --------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `std` | **yes** | Standard library support. | | ||
| | `serde` | **yes** | Enables serde derives for shared runtime-schema ids, counters, and structural keys. Consumers opt in explicitly when default features are disabled. | |
Contributor
There was a problem hiding this comment.
Update generated snapshot date to match the new feature content.
The new echo-runtime-schema section is current, but the file still claims it is generated “as of 2026-03-07” (Line 6), which is now stale metadata.
Proposed patch
-Generated from `Cargo.toml` files as of 2026-03-07. Run
+Generated from `Cargo.toml` files as of 2026-03-24. RunAs per coding guidelines, docs/**/*.md: Documentation accuracy matters — especially stale cross-references and factual accuracy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guide/cargo-features.md` around lines 80 - 90, The documentation
snapshot metadata at the top still reads "generated as of 2026-03-07" and is now
stale; update that generated-snapshot date string to the current correct date to
match the new `echo-runtime-schema` section content (search for the literal
"generated as of 2026-03-07" in docs/guide/cargo-features.md and replace it with
the updated date), ensuring the snapshot date reflects when the file was
regenerated.
Comment on lines
+97
to
+106
| ### Artifact A: Runtime identifiers and logical counters | ||
|
|
||
| - `HeadId` | ||
| - `WorldlineId` | ||
| - `WriterHeadKey` | ||
| - `IntentKind` | ||
| - `WorldlineTick` | ||
| - `GlobalTick` | ||
|
|
||
| Source file: `schemas/runtime/artifact-a-identifiers.graphql` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if PositiveInt is defined in artifact-a-identifiers.graphql
echo "=== Checking PositiveInt definition in artifact-a ==="
grep -n "PositiveInt" schemas/runtime/artifact-a-identifiers.graphql || echo "Not found in artifact-a"
echo ""
echo "=== Checking all PositiveInt references ==="
grep -rn "PositiveInt" schemas/runtime/Repository: flyingrobots/echo
Length of output: 597
Add PositiveInt to the Artifact A listing.
The documentation at lines 97-106 omits PositiveInt, which is defined at line 56 of artifact-a-identifiers.graphql. Since PositiveInt! is used in artifact-b and artifact-d, it must be included in the Artifact A inventory for completeness.
Updated snippet:
### Artifact A: Runtime identifiers and logical counters
- `HeadId`
- `WorldlineId`
- `WriterHeadKey`
- `IntentKind`
- `WorldlineTick`
- `GlobalTick`
- `PositiveInt`
Source file: `schemas/runtime/artifact-a-identifiers.graphql`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plans/phase-8-schema-freeze-inventory.md` around lines 97 - 106, The
Artifact A documentation list is missing the scalar/type PositiveInt; update the
Artifact A inventory to include `PositiveInt` alongside HeadId, WorldlineId,
WriterHeadKey, IntentKind, WorldlineTick, and GlobalTick so that the listing
matches the definition in artifact-a-identifiers.graphql (where PositiveInt is
declared) and reflects its use (PositiveInt!) in artifact-b and artifact-d.
Comment on lines
+91
to
+100
| type SchedulerStatus { | ||
| state: SchedulerState! | ||
| activeMode: SchedulerMode | ||
| workState: WorkState! | ||
| runId: RunId | ||
| latestCycleGlobalTick: GlobalTick | ||
| latestCommitGlobalTick: GlobalTick | ||
| lastQuiescentGlobalTick: GlobalTick | ||
| lastRunCompletion: RunCompletion | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the SchedulerStatus nullability invariants.
activeMode, runId, the tick fields, and lastRunCompletion are the fields clients will branch on, but this fragment does not say whether null means inactive, pre-first-run, or impossible in the current state. Freeze that state-coupled behavior next to the fields, not only in external docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@schemas/runtime/artifact-d-scheduler-results.graphql` around lines 91 - 100,
The SchedulerStatus type lacks in-schema documentation of nullability semantics
for fields clients branch on; update the SchedulerStatus definition (fields:
activeMode, runId, latestCycleGlobalTick, latestCommitGlobalTick,
lastQuiescentGlobalTick, lastRunCompletion) to include short GraphQL field
descriptions that state what a null vs non-null value means (e.g., null =
scheduler inactive, null = pre-first-run, null = impossible given current
state), and if any field should be non-null by invariant, make its type
non-nullable (or explicitly document why it can be null) so the SDL itself
encodes the state-coupled behavior alongside the field (use GraphQL description
strings or comments directly above each field in the SchedulerStatus type).
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