diff --git a/docs/review-findings/2026-04-27-forward-fix-audit-pr897-pr824.md b/docs/review-findings/2026-04-27-forward-fix-audit-pr897-pr824.md new file mode 100644 index 00000000000..78391afcd9a --- /dev/null +++ b/docs/review-findings/2026-04-27-forward-fix-audit-pr897-pr824.md @@ -0,0 +1,262 @@ +# Forward-fix audit: PR #897 and PR #824 + +**Auditor:** session `quick-carp-695` +**Method:** Map each prior codex / manual **BLOCKING** thread on the merged PRs to **current `main`** (this worktree), using file/line evidence. Cross-check integration tests that encode the acceptance surface. + +**Merge anchors (GitHub):** + +| PR | Title | Merge commit (`oid`) | +| --- | --- | --- | +| #897 | T-Modeling int-literal magnitude consumer | `fd21feed685700a212f3740ab498af031506441f` | +| #824 | B4.3 | `7111fbc399cb840d7dc9b340b79cf0dc5ef3a5bd` | + +**Runtime verification:** The audit environment did not have a Rust toolchain (`cargo` unavailable). Operators should still run the commands in §4 on a machine with `cargo` before treating CI as redundant. + +--- + +## 1. PR #897 — int-literal consumer / inference + +### 1.1 OpenAI-pro manual review (BLOCKING): `Behavior::Value` trusted non-`Int` without fit check + +**Review claim:** Returning `Decision::Retry` for any pre-resolved non-`Int` int-literal port without validating magnitude violates fail-closed behavior (e.g. `let x: UInt8 = 256`). + +**Current code:** `decide` handles `Behavior::Value` for integer literals by calling `int_literal_fits_expected_type` when the port is already resolved to a type other than the default `Int` shape. Outcomes: + +- `Ok(Some(true))` → `Decision::Retry` (defer restamp; literal fits). +- `Ok(Some(false))` with a known integer range → `Decision::Fail` with `magnitude_out_of_range(...)`. +- `Err(diag)` → `Decision::Fail` with that diagnostic. +- `Ok(None)` → fall through (non-range-backed / normal mismatch path). + +Evidence: + +```745:794:src/v3/compiler/src/infer.rs + Behavior::Value(v) => { + // `let x: UInt8 = 5` seeds the value port to `UInt8` after lowering; + // ... + if let LiteralBits::Int(literal) = &v.data { + if let PortState::Resolved(existing) = dag.port(v.output).state() { + if let Some(int_sh) = dag.int_shape() { + if !type_shapes_equivalent(dag, existing, &int_sh) { + match int_literal_fits_expected_type( + dag, + *literal, + existing.declaration, + ) { + Ok(Some(true)) => return Decision::Retry, + Ok(Some(false)) => { + match integer_range_for_decl(dag, existing.declaration) { + IntegerRangeLookup::Found(range) => { + return Decision::Fail( + v.output, + magnitude_out_of_range( + *literal, + *existing, + range, + v.span.clone(), + ), + ); + } + // ... + } + } + Err(diag) => return Decision::Fail(v.output, diag), + Ok(None) => {} + } + } + } + } + } +``` + +**Verdict:** Finding addressed. + +### 1.2 Negative regression: `let x: UInt8 = 256` → `MagnitudeOutOfRange` + +**Test:** `let_annotated_uint8_out_of_range_emits_magnitude_diagnostic` in `int_literal_cardinality_test.rs`. + +```88:115:src/v3/compiler/tests/integration/int_literal_cardinality_test.rs +fn let_annotated_uint8_out_of_range_emits_magnitude_diagnostic() { + let err = compile_to_dag("let x: UInt8 = 256\n", "let_u8_oob.v3") + .expect_err("annotated let UInt8 overflow must fail closed"); + // ... asserts Diagnostic::MagnitudeOutOfRange { literal == "256", target == "u8", ... } +} +``` + +**Verdict:** Acceptance test present on `main`. + +### 1.3 Conflict merge path still range-validates + +`int_literal_magnitude_narrow_merge` only allows merge when `int_literal_fits_expected_type` returns `Ok(Some(true))`. + +```716:734:src/v3/compiler/src/infer.rs +fn int_literal_magnitude_narrow_merge( + dag: &Dag, + port: PortId, + from: &TypeShape, + to: &TypeShape, +) -> bool { + // ... + matches!( + int_literal_fits_expected_type(dag, lit, to.declaration), + Ok(Some(true)) + ) +} +``` + +**Verdict:** Aligns with review requirement that merge-site narrowing stays range-backed. + +### 1.4 Call-site + Rust emit surface (prior CI triage) + +Integration tests on `main` cover: + +- `call_site_uint8_literal_narrows` — `id_u8(7)` narrows to `UInt8`. +- `emit_let_uint8_uses_narrow_rust_type` — emitted Rust mentions `u8` for annotated `UInt8` let. + +**Verdict:** Covered by tests in the same file as §1.2. + +### 1.5 Generic fail-closed diagnostics (thesis / M0) + +The review thread cited `thesis_validation_test::t1_4_type_mismatch_produces_a_typemismatch_diagnostic` as a canary. The test remains in-tree: + +```269:270:src/v3/compiler/tests/integration/thesis_validation_test.rs +fn t1_4_type_mismatch_produces_a_typemismatch_diagnostic() { + let dag = match compile_to_dag("let x: Bool = 1", "t1_4_type_mismatch.v3") { +``` + +**Verdict:** No evidence in static review of a remaining gap; **confirm with `cargo test`** (§4). + +--- + +## 2. PR #824 — `emit_participation` substrate (B4.3) + +### 2.1 Codex BLOCKING: reflection / `Dag::new()` field list out of sync + +**Review claim:** Declared substrate fields did not match what `Dag::new()` exposed for `BranchNode` / `BindNode`. + +**Current code:** `substrate_declares_expected_reflection_surface` expects `emit_participation` on both node types: + +```228:261:src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs + assert_eq!( + record_fields(&dag, "BranchNode"), + vec![ + "id", + "input", + "paths", + "result_port", + "span", + "emit_participation" + ] + ); + // ... + assert_eq!( + record_fields(&dag, "BindNode"), + vec![ + "id", + "name", + "result_port", + "params", + "span", + "lane2_workflow", + "emit_participation" + ] + ); +``` + +Canonical substrate declarations include the fields (`src/v3/std/substrate.dag`). + +**Verdict:** Finding addressed; test is a direct checkable ratchet. + +### 2.2 Claude review: stale bootstrap snapshot (`emit_participation: None` on user binds) + +**Current code:** `bootstrap_generated.rs` / `bootstrap_generated_without_parse_surface.rs` wire `BindNode` for std `two_terms` with `emit_participation: Some(BindEmitParticipation::UserCallable)`. Those files are generated and largely one line per snapshot; cite them with search, not fragile line numbers, e.g. `rg 'emit_participation: Some\\(BindEmitParticipation::UserCallable\\)' src/v3/compiler/src/bootstrap_generated*.rs` (one hit per file at time of audit). + +**Verdict:** Bootstrap matches lowering intent for at least the cited binds; **regen drift** should still be caught by normal `regen_bootstrap --check` in CI if present. + +### 2.3 Codex BLOCKING: “substrate without real consumer” + +**Disposition on `main`:** Beyond test-only selectors, production `emit.rs` tests use `bind.emit_participation()` / `branch.emit_participation()` to locate user callables vs `match` branches (replacing `span.file` hacks). Example: + +```3175:3223:src/v3/compiler/src/emit.rs + .find_map(|node| match node { + Behavior::Bind(bind) + if bind.name == "id" + && bind.emit_participation() + == Some(BindEmitParticipation::UserCallable) => + { + bind.params.first().copied() + } + _ => None, + }) + // ... + Behavior::Branch(branch) + if branch.output == classify_output + && branch.emit_participation() + == Some(BranchEmitParticipation::UserMatch) => +``` + +Python boundary serialization uses **Python string literals** for enum payloads (not Rust paths), with an explicit regression: + +```616:667:src/v3/compiler/tests/boundary/m1_4_emit_python_test.rs +fn serialize_opt_bind_emit_participation(p: Option) -> String { + match p { + None => "None".to_string(), + Some(BindEmitParticipation::UserCallable) => "\"UserCallable\"".to_string(), + } +} +// ... +#[test] +fn serialize_dag_embeds_valid_python_emit_participation_literals() { +``` + +**Verdict:** “Real consumer” + boundary shape are satisfied in-repo; the original codex concern is overtaken by later commits on `main`. + +### 2.4 Visibility asymmetry (`BranchNode.emit_participation` public vs accessor pattern) + +**Current code:** `BranchNode.emit_participation` is `pub(crate)` with `emit_participation()` accessor (`src/v3/compiler/src/dag.rs` ~1617–1627), matching the tightened review ask. + +**Verdict:** Addressed. + +--- + +## 3. Conclusion + +- **#897:** The **fail-closed** `Behavior::Value` path, **negative `UInt8 = 256`** coverage, **range-validated merge**, and **downstream integration tests** are present in the current tree. No additional code change is justified from this audit alone. +- **#824:** **Reflection**, **bootstrap**, **emit consumers**, **Python serialization**, and **visibility** align with the resolved review threads documented on the PR. + +**Follow-up PR:** **None** opened from this session — no unfixed BLOCKING gap was identified in current sources. + +--- + +## 4. Recommended verification commands (operator / CI) + +Run on a checkout of `main` (or this branch) with Rust installed: + +For `v3-compiler`, only **`tests/integration.rs`** and **`tests/determinism_test.rs`** at the package root become `--test integration` and `--test determinism_test`. The **`tests/boundary/`** directory is taxonomy only: those suites are **`mod`uled into** `tests/integration.rs` (see `#[path = "boundary/m1_4_emit_python_test.rs"] mod m1_4_emit_python_test`), so there is **no** `--test boundary` binary. + +```bash +cargo test -p v3-compiler --test integration int_literal_cardinality_test +cargo test -p v3-compiler --test integration thesis_validation_test::t1_4_type_mismatch_produces_a_typemismatch_diagnostic +cargo test -p v3-compiler --test integration substrate_declares_expected_reflection_surface +cargo test -p v3-compiler --test integration m1_4_emit_python_test::serialize_dag_embeds_valid_python_emit_participation_literals +``` + +Broader confidence: + +```bash +cargo test -p v3-compiler --test integration +``` + +--- + +## 5. Traceability + +| Source | Review / thread | Disposition | +| --- | --- | --- | +| #897 | OpenAI-pro BLOCKING: `Behavior::Value` retry without fit check | Fixed — see §1.1 | +| #897 | Negative `UInt8 = 256` | Test — §1.2 | +| #897 | Merge path validation | §1.3 | +| #897 | Call-site + emit | §1.4 | +| #824 | Reflection mismatch | Test + substrate — §2.1 | +| #824 | Bootstrap stale | Spot-check — §2.2 | +| #824 | No real consumer | emit + boundary tests — §2.3 | +| #824 | Visibility | §2.4 |