● Phase A is complete. Here's what landed. #27

Merged

navicore merged 2 commits from wasm-tier-2-phase-A into main 2026-06-20 20:31:13 +00:00

Conversation 1 Commits 2 Files changed 6 +399 -123

navicore commented 2026-06-20 20:01:58 +00:00

Owner

Copy link

Phase A — runtime: I/O-free core + productized reactor ABI ✅

A1 — I/O-free core (crates/runtime/src/core.rs, new)

• run_query(m, q) -> QueryResult (parse + solve; ParseError/RuntimeError/Solutions so callers
  map their own surface — CLI exit codes 2/3, reactor {"error":…}).
• exhausted(m) — the limit-aware rule solution_limit.is_none_or(|l| count < l),
  single-sourced (finding #4; spike hard-coded true).
• write_error_json / write_solutions_json over io::Write — one impl of the v1 JSON shape.
  entry.rs's output_json/output_error and the old reactor.rs JSON are gone, both now call the
  core. 4 new unit tests pin the byte shape.

A2 — Machine::reset_per_query() (machine.rs)

• Lives next to the field declarations, clears all per-query accumulated state (replaces the
  spike's by-name reset()), leaves caller-set limits alone (finding #3).

A4 — write/1 capture (machine.rs + builtins/miscops.rs)

• New OutputSink (Stdout | Capture(String)) + write_out/captured_output.
  write/1/writeln/1/nl/0 route through write_out — CLI streams to stdout unchanged; the reactor
  captures (D4). Captured bytes ride back in an "output" JSON field (sorted between exhausted
  and solutions); CLI passes None so native bytes stay byte-identical to v1.

A3 — productized reactor ABI (reactor.rs)

• plg_rt_run_query(qptr, qlen, limit, step_limit, depth_limit) — per-request
  solution/step/metacall-depth limits (finding #5); 0 sentinels documented. Layout-exact
  alloc/free kept with the finding-#1 warning. set_machine swaps in Capture mode and drops the
  spike's 1e9 step hack. D3 single-in-flight contract documented in the module header; findings
  #7/#8 noted in place. No JSON duplication — delegates to the core.

Verification

• Full workspace suite green: 127 runtime unit tests (+4), and the v1 differential /
  integration / golden-IR suites all pass — native output is byte-identical and codegen is
  untouched.
• cargo build -p patch-prolog-runtime --target wasm32-unknown-unknown compiles clean (reactor

• OutputSink + write_out).

• cargo clippy --workspace --all-targets and cargo fmt --check clean.

One judgment call worth flagging: captured write/1 output is surfaced on the success path
only ("output" field); the error object stays {"error":…}. Lossless for the normal case, and
it keeps the error shape byte-identical to native. Say the word if you'd rather carry
captured output on the error path too.

Phase A — runtime: I/O-free core + productized reactor ABI ✅ A1 — I/O-free core (crates/runtime/src/core.rs, new) - run_query(m, q) -> QueryResult (parse + solve; ParseError/RuntimeError/Solutions so callers map their own surface — CLI exit codes 2/3, reactor {"error":…}). - exhausted(m) — the limit-aware rule solution_limit.is_none_or(|l| count < l), single-sourced (finding #4; spike hard-coded true). - write_error_json / write_solutions_json over io::Write — one impl of the v1 JSON shape. entry.rs's output_json/output_error and the old reactor.rs JSON are gone, both now call the core. 4 new unit tests pin the byte shape. A2 — Machine::reset_per_query() (machine.rs) - Lives next to the field declarations, clears all per-query accumulated state (replaces the spike's by-name reset()), leaves caller-set limits alone (finding #3). A4 — write/1 capture (machine.rs + builtins/miscops.rs) - New OutputSink (Stdout | Capture(String)) + write_out/captured_output. write/1/writeln/1/nl/0 route through write_out — CLI streams to stdout unchanged; the reactor captures (D4). Captured bytes ride back in an "output" JSON field (sorted between exhausted and solutions); CLI passes None so native bytes stay byte-identical to v1. A3 — productized reactor ABI (reactor.rs) - plg_rt_run_query(qptr, qlen, limit, step_limit, depth_limit) — per-request solution/step/metacall-depth limits (finding #5); 0 sentinels documented. Layout-exact alloc/free kept with the finding-#1 warning. set_machine swaps in Capture mode and drops the spike's 1e9 step hack. D3 single-in-flight contract documented in the module header; findings #7/#8 noted in place. No JSON duplication — delegates to the core. Verification - Full workspace suite green: 127 runtime unit tests (+4), and the v1 differential / integration / golden-IR suites all pass — native output is byte-identical and codegen is untouched. - cargo build -p patch-prolog-runtime --target wasm32-unknown-unknown compiles clean (reactor + OutputSink + write_out). - cargo clippy --workspace --all-targets and cargo fmt --check clean. One judgment call worth flagging: captured write/1 output is surfaced on the success path only ("output" field); the error object stays {"error":…}. Lossless for the normal case, and it keeps the error shape byte-identical to native. Say the word if you'd rather carry captured output on the error path too.

navicore added 1 commit 2026-06-20 20:01:58 +00:00

● Phase A is complete. Here's what landed. ... 605cd01f3b

Phase A — runtime: I/O-free core + productized reactor ABI ✅

  A1 — I/O-free core (crates/runtime/src/core.rs, new)
  - run_query(m, q) -> QueryResult (parse + solve; ParseError/RuntimeError/Solutions so callers
  map their own surface — CLI exit codes 2/3, reactor {"error":…}).
  - exhausted(m) — the limit-aware rule solution_limit.is_none_or(|l| count < l),
  single-sourced (finding #4; spike hard-coded true).
  - write_error_json / write_solutions_json over io::Write — one impl of the v1 JSON shape.
  entry.rs's output_json/output_error and the old reactor.rs JSON are gone, both now call the
  core. 4 new unit tests pin the byte shape.

  A2 — Machine::reset_per_query() (machine.rs)
  - Lives next to the field declarations, clears all per-query accumulated state (replaces the
  spike's by-name reset()), leaves caller-set limits alone (finding #3).

  A4 — write/1 capture (machine.rs + builtins/miscops.rs)
  - New OutputSink (Stdout | Capture(String)) + write_out/captured_output.
  write/1/writeln/1/nl/0 route through write_out — CLI streams to stdout unchanged; the reactor
  captures (D4). Captured bytes ride back in an "output" JSON field (sorted between exhausted
  and solutions); CLI passes None so native bytes stay byte-identical to v1.

  A3 — productized reactor ABI (reactor.rs)
  - plg_rt_run_query(qptr, qlen, limit, step_limit, depth_limit) — per-request
  solution/step/metacall-depth limits (finding #5); 0 sentinels documented. Layout-exact
  alloc/free kept with the finding-#1 warning. set_machine swaps in Capture mode and drops the
  spike's 1e9 step hack. D3 single-in-flight contract documented in the module header; findings
  #7/#8 noted in place. No JSON duplication — delegates to the core.

  Verification

  - Full workspace suite green: 127 runtime unit tests (+4), and the v1 differential /
  integration / golden-IR suites all pass — native output is byte-identical and codegen is
  untouched.
  - cargo build -p patch-prolog-runtime --target wasm32-unknown-unknown compiles clean (reactor
  + OutputSink + write_out).
  - cargo clippy --workspace --all-targets and cargo fmt --check clean.

  One judgment call worth flagging: captured write/1 output is surfaced on the success path
  only ("output" field); the error object stays {"error":…}. Lossless for the normal case, and
  it keeps the error shape byte-identical to native. Say the word if you'd rather carry
  captured output on the error path too.

navicore commented 2026-06-20 20:12:14 +00:00

Author

Owner

Copy link

Review — #27 Phase A: I/O-free core + productized reactor ABI

This is a clean productization of the spike. The shape is right: a single core module owns parse+solve and the v1 JSON wire shape, and all three transports (CLI/entry.rs, WASI, reactor) now route through it instead of each carrying its own format! of the JSON. The duplicated solutions_json/error_json/reset in reactor.rs are gone and the spike's THROWAWAY header is replaced with a real concurrency contract. The two pre-merge items the #23 review flagged as becoming load-bearing once an embedding landed — the Drop-based MetacallDepthGuard and PLG_METACALL_DEPTH — are both present, and the reactor now threads per-request depth_limit. Good follow-through.

A few specific things I checked and liked:

• exhausted single-sourced and faithful. The CLI used to compute args.limit.is_none_or(|l| count < l) inline; it now sets m.solution_limit = args.limit (entry.rs:178) and calls core::exhausted(m), which is the same rule over m.solution_limit. The reactor previously hard-coded true (finding #4) and now computes it the same way. Verified the wiring — equivalent for the CLI, fixed for the reactor.
• write_out as the single output seam. write/1/writeln/1/nl/0 all funnel through m.write_out, so the Stdout-vs-Capture decision is made in exactly one place. writeln building s.push('\n') then one write_out is byte-equivalent to the old println!. CLI passes None for output so native JSON stays byte-identical to v1 — confirmed by the unchanged differential/golden suites.
• Exact-Layout alloc + finding-#1 warning kept. The NEVER Vec::with_capacity comment is the right thing to preserve loudly.

Real concern

1. Reactor per-request step_limit/depth_limit latch — the documented "0 = keep the module default" is violated under reuse. plg_rt_run_query only overwrites when the arg is non-zero:

if step_limit != 0 { m.step_limit = step_limit; }
if depth_limit != 0 { m.metacall_depth_limit = depth_limit as usize; }

and reset_per_query() deliberately does not touch step_limit/metacall_depth_limit (correct for the CLI, which sets them once). But the reactor reuses one static Machine across every request. So:

• Request 1: step_limit = 5000 → m.step_limit = 5000
• Request 2: step_limit = 0 ("use default") → the if is skipped, reset_per_query leaves it alone → m.step_limit stays 5000, not the module default (10_000).

A host passing 0 gets request N-1's value, not the default — a cross-request state leak, in exactly the sustained-reuse scenario reset_per_query/A2 exists to prevent. It only affects the limit fields because they're the ones reset_per_query intentionally excludes. solution_limit is fine (always assigned, 0 => None). This didn't bite the spike because it pinned step_limit to 1e9 once; making the limits per-request is what exposes it.

Fix: capture the module defaults at plg_rt_set_machine time (the values codegen/plg_init baked in) and on each request assign unconditionally — m.step_limit = if step_limit != 0 { step_limit } else { default_step } — or, minimally, document on the ABI that 0 means "inherit the previous request's value" rather than "module default." The current doc comment and the actual behavior disagree, and there's no test covering two sequential requests with differing limits, so this would pass CI silently.

Small observations

2. Reactor always emits "output", even empty. set_machine installs OutputSink::Capture(String::new()), so after a query with no write/1, captured_output() returns Some("") (not None), and write_solutions_json emits ,"output":"". That's consistent and arguably nicer for a host (.output is always present) — just confirm it's the intended D4 contract, since it's Some("") vs None that decides field presence, and an empty string is "present."

3. Infallibility comment is accurate. The let _ = core::write_* calls in the reactor are over a Vec sink and the "writes never fail" note is correct; no concern, just noting I checked the swallowed io::Results are genuinely infallible there (unlike the stdout path in entry.rs, where a broken pipe is silently dropped — pre-existing v1 behavior, fine to leave).

Net: #1 is worth closing before this lands on main, since the reactor's whole reason for existing is Machine reuse and that's precisely where the limit contract breaks. Everything else is solid.

## Review — #27 Phase A: I/O-free core + productized reactor ABI This is a clean productization of the spike. The shape is right: a single `core` module owns parse+solve and the v1 JSON wire shape, and all three transports (CLI/`entry.rs`, WASI, reactor) now route through it instead of each carrying its own `format!` of the JSON. The duplicated `solutions_json`/`error_json`/`reset` in `reactor.rs` are gone and the spike's `THROWAWAY` header is replaced with a real concurrency contract. The two pre-merge items the #23 review flagged as becoming load-bearing once an embedding landed — the `Drop`-based `MetacallDepthGuard` and `PLG_METACALL_DEPTH` — are both present, and the reactor now threads per-request `depth_limit`. Good follow-through. A few specific things I checked and liked: - **`exhausted` single-sourced and faithful.** The CLI used to compute `args.limit.is_none_or(|l| count < l)` inline; it now sets `m.solution_limit = args.limit` (entry.rs:178) and calls `core::exhausted(m)`, which is the same rule over `m.solution_limit`. The reactor previously hard-coded `true` (finding #4) and now computes it the same way. Verified the wiring — equivalent for the CLI, fixed for the reactor. - **`write_out` as the single output seam.** `write/1`/`writeln/1`/`nl/0` all funnel through `m.write_out`, so the Stdout-vs-Capture decision is made in exactly one place. `writeln` building `s.push('\n')` then one `write_out` is byte-equivalent to the old `println!`. CLI passes `None` for `output` so native JSON stays byte-identical to v1 — confirmed by the unchanged differential/golden suites. - **Exact-`Layout` alloc + finding-#1 warning kept.** The `NEVER Vec::with_capacity` comment is the right thing to preserve loudly. ### Real concern **1. Reactor per-request `step_limit`/`depth_limit` latch — the documented "`0` = keep the module default" is violated under reuse.** `plg_rt_run_query` only overwrites when the arg is non-zero: ```rust if step_limit != 0 { m.step_limit = step_limit; } if depth_limit != 0 { m.metacall_depth_limit = depth_limit as usize; } ``` and `reset_per_query()` deliberately does *not* touch `step_limit`/`metacall_depth_limit` (correct for the CLI, which sets them once). But the reactor reuses one `static` Machine across every request. So: - Request 1: `step_limit = 5000` → `m.step_limit = 5000` - Request 2: `step_limit = 0` ("use default") → the `if` is skipped, `reset_per_query` leaves it alone → **`m.step_limit` stays 5000**, not the module default (10_000). A host passing `0` gets request N-1's value, not the default — a cross-request state leak, in exactly the sustained-reuse scenario `reset_per_query`/A2 exists to prevent. It only affects the limit fields because they're the ones `reset_per_query` intentionally excludes. `solution_limit` is fine (always assigned, `0 => None`). This didn't bite the spike because it pinned `step_limit` to 1e9 once; making the limits per-request is what exposes it. Fix: capture the module defaults at `plg_rt_set_machine` time (the values codegen/`plg_init` baked in) and on each request assign unconditionally — `m.step_limit = if step_limit != 0 { step_limit } else { default_step }` — or, minimally, document on the ABI that `0` means "inherit the previous request's value" rather than "module default." The current doc comment and the actual behavior disagree, and there's no test covering two sequential requests with differing limits, so this would pass CI silently. ### Small observations **2. Reactor always emits `"output"`, even empty.** `set_machine` installs `OutputSink::Capture(String::new())`, so after a query with no `write/1`, `captured_output()` returns `Some("")` (not `None`), and `write_solutions_json` emits `,"output":""`. That's consistent and arguably nicer for a host (`.output` is always present) — just confirm it's the intended D4 contract, since it's `Some("")` vs `None` that decides field presence, and an empty string is "present." **3. Infallibility comment is accurate.** The `let _ = core::write_*` calls in the reactor are over a `Vec` sink and the "writes never fail" note is correct; no concern, just noting I checked the swallowed `io::Result`s are genuinely infallible there (unlike the stdout path in `entry.rs`, where a broken pipe is silently dropped — pre-existing v1 behavior, fine to leave). Net: #1 is worth closing before this lands on `main`, since the reactor's whole reason for existing is Machine reuse and that's precisely where the limit contract breaks. Everything else is solid.

navicore referenced this pull request from a commit 2026-06-20 20:29:45 +00:00

● PR #27 feedback addressed. Summary:

navicore added 1 commit 2026-06-20 20:29:45 +00:00

● PR #27 feedback addressed. Summary: ... 42f38baad5

#1 — limit latch (the real bug), fixed. plg_rt_set_machine now snapshots the module-default
  step_limit/metacall_depth_limit into DEFAULT_STEP_LIMIT/DEFAULT_DEPTH_LIMIT statics, and
  plg_rt_run_query assigns all three limits unconditionally each request — non-zero overrides,
  0 restores the captured default. A request passing 0 now gets the module default, never
  request N-1's value. The ABI doc and the behavior now agree.

  Regression test added. zero_limits_restore_module_default_not_previous_request installs a
  Machine, runs request 1 with step=5000, depth=50, asserts they took, then runs request 2 with
  0,0 and asserts both revert to the baked-in defaults — exactly the two-sequential-requests
  case the reviewer noted was missing and would've passed CI silently. (128 runtime tests now,
  was 127.)

  #2 — always-emit "output", confirmed intended. Capture mode means captured_output() is always
  Some ("" when nothing was written), so the result always carries an output field. That's the
  deliberate D4 contract — a stable shape so a host can always read .output — and I made the
  intent explicit in a comment at the call site.

  #3 — no action (the swallowed io::Results over the Vec sink are genuinely infallible;
  reviewer confirmed).

  All green: 128 runtime tests, wasm32-unknown-unknown builds clean, fmt + clippy clean. The
  fix is contained entirely in reactor.rs, so the rest of the PR's review stands. Ready for B
  whenever you've re-reviewed.

navicore merged commit 5c02aca50f into main 2026-06-20 20:31:13 +00:00

navicore deleted branch wasm-tier-2-phase-A 2026-06-20 20:31:13 +00:00

navicore referenced this pull request from a commit 2026-06-20 20:31:15 +00:00

Merge pull request '● Phase A is complete. Here's what landed.' (#27) from wasm-tier-2-phase-A into main

navicore referenced this pull request 2026-06-21 00:44:40 +00:00

● Phase D — Worker glue + deploy ergonomics ✅ #29

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-prolog!27

No description provided.