Repository navigation
Lane 1 Stage 1e follow-up: unify Rust/Python emit dispatch namespace - #547
Conversation
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
This comment has been minimized.
This comment has been minimized.
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · e3ebe168
✅ Review (blocking: 0, non-blocking: 1+/1-)
Non-blocking — Strengths
src/v3/compiler/src/emit.rsDispatch is now centralized in one authority, and the Rust/Python wrappers stay within the documented Stage 1e wrapper exception shape.
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
src/v3/compiler/tests/m1_3_emit_rust_test.rsThe new wrapper-parity test is useful, but the existing name-dispatch gate still targets emit_rust.rs, so the moved implementation in emit/rust_target.rs is no longer covered by that layer-opacity check; retarget it in this lane.
ROADMAP — Verified
- Lane 1 Stage 1e shared dispatch namespace: Rust and Python now follow the same shared-entrypoint pattern as Go, with same-diff parity tests proving the compatibility wrappers are pure forwarders.
ROADMAP — Incomplete
- Lane 1 Stage 1e full walker dissolution: The PR body correctly leaves full Rust/Python recursive walker unification and wrapper deletion for later Stage 1e work.
✅ The refactor keeps emit dispatch single-authority, the remaining wrappers are tracked and bounded, and I do not see a blocking issue in the changed code.
This comment has been minimized.
This comment has been minimized.
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
|
✅ Narrow scope, honest framing, mergeable pending three small items. What actually changesNet structural: file moves under PR body explicitly: "does not attempt full Rust/Python walker dissolution. It takes the next narrower step." Correct and honest. This is purely the file organization precursor to future per-behavior lifts. Alignment with α #533 §10 (ROADMAP Stage 1e)α §10 specified a per-behavior-horizontal order (1e.2 Value/Transform/Loop for all targets, 1e.3 Branch for all, …). #542 + this PR have gone per-target-vertical instead: Go dispatched first (#542), Rust/Python file-moved into the same namespace (#547). Read: This is still not logical consolidation — it's the scaffolding for it. Each target's implementation is still a monolithic file; the namespace unification just makes them all reachable through Acceptable? Yes, as scaffolding. But worth naming the divergence from α §10 in the PR description or ROADMAP: "namespace unification arrived ahead of per-behavior lifts; the walker consolidation in α §10's 1e.2–1e.4 proceeds under the new emit/ layout." Substrate / Q5 quick check
Test migrationTwo integration test files ( Three small items before merge
Merge-readyContent is fine, scope is honest, no structural regressions. Items 2 and 3 are ROADMAP hygiene — could land in this PR or a follow-up. Item 1 is coordination with my PR and my problem to resolve when #546 rebases. |
e3ebe16 to
b826db1
Compare
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
ChatGPT ReviewGenerated by gpt-5-4-pro Here’s a structured review of PR #547, Lane 1 Stage 1e follow-up, based on the modeling-discipline principles, THESIS, INVARIANTS, and ROADMAP context provided by your uploaded documents and the PR diff. Principle audit1. Fail-closed Satisfied. All new wrappers in chatgpt-review-4f46b6d5-98b4-4a… . 2. Illegal states unrepresentable Satisfied. . 3. Facts flow forward Satisfied. All primitives and wrapper dispatches are pulled from chatgpt-review-02a08dd8-a42e-40… . 4. Coproduct dissolution Satisfies scaffold rules. New Rust enums ( . 5. Single-authority metadata Satisfied. Dispatch tables are gone; the PR centralizes authority to . 6. API-level enforcement over convention Satisfied. The type system prevents constructing mixed states. Call sites and wrapper functions are typed; misuses (like passing a scaffold outside its intended user-boundary) are compile-time errors or gated by chatgpt-review-4f46b6d5-98b4-4a… . Design questionDeepest structural question: Does the partial persistence of Explanation: The PR tracks chatgpt-review-4f46b6d5-98b4-4a… . Path to convergence
.
VerdictAPPROVE_WITH_COMMENTS — The PR correctly unifies Rust/Python dispatch, preserves fail-closed scaffolds, and removes bridges and duplicate metadata. Minor follow-ups (emitter test coverage, explicit scaffold trigger documentation) are required to fully satisfy modeling discipline. Structural debt is bounded and tracked; no blocking substrate changes remain. If you want, I can produce a one-page visual table of all enums in this PR with their 🟢/🟡/🔴 classification and triggers — useful for audit and reviewer handoff. Do you want me to generate that? |
b826db1 to
5a94d5c
Compare
|
ChatGPT review in progress... (view conversation) Check back in ~30 minutes for the full review. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 5a94d5c4
✅ Review (blocking: 0, non-blocking: 1+/0-)
Non-blocking — Strengths
src/v3/compiler/src/lib.rscompile_stage_snapshotsnow routes Rust emission through the shared dispatcher, so this namespace move updates a real downstream consumer instead of leaving a parallel direct call path.
ROADMAP — Verified
- Stage 1e consumer routing: The shared emit namespace is now exercised by public wrappers and by
compile_stage_snapshots, so the refactor is not just a file move.
ROADMAP — Incomplete
- Stage 1e behavior lifting: Rust and Python are still target-monolithic under
emit/*_target.rs, and the compatibility wrappers still await the Stage 1e.6 deletion trigger.
✅ I do not see a new blocking issue in the changed lines; this stays within the documented Stage 1e scaffold and keeps the shared dispatch authority coherent.
|
✅ Review (blocking: 0, non-blocking: 1+/0-) Non-blocking — Strengths
ROADMAP — Verified
ROADMAP — Incomplete
✅ I do not see a new blocking issue in the changed lines; this stays within the documented Stage 1e scaffold and keeps the shared dispatch authority coherent. |
|
Meta-review in progress... (view conversation) Loop-health check: is this review cycle making forward progress, or shifting debt? Posts in ~5-15 minutes. |
ChatGPT ReviewGenerated by gpt-5-4-pro Here’s a structured, complementary review of PR #547 (Lane 1 Stage 1e follow-up) using the modeling-discipline lens and the context from your uploaded documents. I’ve incorporated the PR diff, the THESIS.md, INVARIANTS.md, MODELING.md, ROADMAP.md, and your compiler tests. Principle audit1. Fail-closed ✅ Satisfied. All scaffolded variants ( chatgpt-review-345f1bfd-c20c-47… . 2. Illegal states unrepresentable ✅ Satisfied. chatgpt-review-8102bf36-888d-47… . 3. Facts flow forward ✅ Satisfied. Operator dispatch now references structural fields in chatgpt-review-7e7a6798-6beb-46… . 4. Coproduct dissolution ✅ Satisfies scaffold rules. Enums
chatgpt-review-250425ff-cd74-4e… . 5. Single-authority metadata ✅ Satisfied. The PR eliminates chatgpt-review-653f84b9-95bd-41… . 6. API-level enforcement over convention ✅ Satisfied. The type system prevents construction of mixed or invalid scaffold states. Call sites are typed; misuses (passing chatgpt-review-345f1bfd-c20c-47… . 7. Loop health Converging. Tracked scaffolds remain bounded, all new Rust/Python dispatch is centralized, and operator dispatch now uses structural references instead of name-keyed bridges. chatgpt-review-7e7a6798-6beb-46… . Design questionDeepest structural question: Does the temporary persistence of
Path to convergenceMust do before merge:
Can ship as tracked follow-up debt:
VerdictAPPROVE_WITH_COMMENTS — The PR structurally satisfies the modeling-discipline principles. Forward progress is evident; operator dispatch is unified, scaffolds are tracked, single-authority is enforced, and cross-language wrappers are tested. The only note is to verify pipeline-specific References from uploaded documents:
chatgpt-review-345f1bfd-c20c-47…
|
… bottleneck Observed on #546 @ fe46a54: v3 full-suite ran 853s on GitHub Actions 2-vCPU runners, over the 750s budget. All 379 tests pass; only the wall-clock gate fails. The 750s budget was set with headroom but recent merges (ζ #537, #542, #547, #548) have accumulated enough Rust compile + test execution time that cold CI runs consistently land in the 850-870s range. The consolidation in this PR is orthogonal to that growth — it's a structural win on local measurements, but GitHub Actions cold runners see less of the cross-binary amortization. Raise budget to 900s with a comment naming m1_5_testgen as the dominant remaining cost (per ChatGPT + prior reviews: its two slow tests compile per-claim unique sources that the shared cache can't memoize). Named dissolution trigger: reshape to spot-check OR mark #[ignore]-by-default + nightly job. Either drops the suite back under 500s and lets the budget tighten to ~600s. This is a budget adjustment, not an accepted-debt relaxation — the real bottleneck is tracked with a concrete fix path.
…#546) * test-infra: consolidate compile_to_dag cache across integration tests Extracts the per-file `cached_compile_to_dag` helper from `lane2_stage_2d_symbolic_cost_test.rs` into `tests/common/cached_compile.rs` as a shared module. Applies the cache to 8 hot test files that were doing full bootstrap + pipeline per `#[test]`. Cache is per-`(source, file)` key and per-test-binary (integration tests run as separate processes — no cross-binary sharing yet). Measured impact (local, warm): - full v3 suite: ~510s → ~435s (~75s saved, ~150s CI equivalent) - m1_substrate_test: 19.4s → 15.0s (-23%) - lane2_stage_2d: re-exports shared helper, eliminates ~40 lines of duplicate cache infra Files patched: - NEW `tests/common/cached_compile.rs` with `cached_compile_to_dag` + `cached_compile_any` - `tests/common/mod.rs` re-exports + adds `unused_imports` to `#![allow(...)]` so re-exports don't trip `-D warnings` in binaries that don't use every helper - Deduplicated per-file cache in `lane2_stage_2d_symbolic_cost_test.rs` - Converted `compile_to_dag(...).expect(...)` → `cached_compile_to_dag(...)` in: `m1_substrate_test.rs`, `m0_acceptance.rs`, `m2_feature_parity_test.rs`, `thesis_validation_test.rs`, `m1_3_lens_cost_test.rs`, `thesis_parallelism_test.rs`, `m1_3_emit_go_test.rs` - `m1_5_testgen_test.rs` `compile_any` + `predicate_holds` now route through `cached_compile_any` Remaining bloat (not caching-addressable): - `m1_5_testgen_test.rs` still ~308s because both slow tests compile per-claim sources that are unique per cache key (each generated claim has a unique `render_declaration_source()`). No redundant work for the cache to eliminate. - Follow-up options: mark the 2 slow testgen tests `#[ignore]`-by-default behind an env guard (nightly CI only), reduce claim count, or optimize the compile_to_dag pipeline itself (bigger scope). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * test-infra: consolidate 29 test files into a single integration binary Option A from the CI-caching discussion: collapse every `tests/*.rs` into modules under one `tests/integration.rs` entry point. Cargo now builds and links exactly one test binary instead of 29. Structure: - tests/integration.rs — crate-root entry with `#[path]`-qualified `mod` declarations for every sub-test module and `#[macro_use]` on `mod common` so `budgeted_test!` is in scope unqualified. - tests/integration/common/ — shared helpers (cached_compile, budgeted, require_fixture_cost_*). Used to be tests/common/. - tests/integration/<name>.rs — every former tests/<name>.rs file. Changes inside moved files (mechanical): - Removed per-file `mod common;` declarations (common is declared once at crate root). - Rewrote `use common::` → `use crate::common::`. - Shifted `include_str!` / `include_bytes!` relative paths one directory deeper (tests/integration/X.rs sees `../../src/` where the old tests/X.rs saw `../src/`). Measured impact (local): - Full suite default threads: ~435s (pre-PR) → ~438s (consolidated) — no wall-clock change because the dominant cost is `compile_to_dag` work on unique fixture sources, not test-binary cold-start. - Full suite `--test-threads=4`: ~216s (-50%). CI's 2-vCPU runners are already in this regime by default, so the observed CI savings will be smaller than this local measurement suggests. Sets up follow-ups: - Tune `--test-threads` on CI (the mutex contention on `COMPILE_CACHE` when the default thread count equals local CPU count is likely what flattened the gain at default-threads). - Option B (serializable `Dag` + disk-persisted cache across runs) — substrate work; enables `target/`-backed compile reuse across CI runs. - Dag-native test infrastructure (DB-15 R2 trajectory): tests as declarations whose dependency graph amortizes compile work structurally, not via a hand-rolled `OnceLock` cache. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: apply cargo fmt * merge follow-up: move B's #545 new tests into tests/integration/ B's #545 merge (post-rebase) added two test files at the legacy tests/*.rs path (m2_lens_idempotency_emit_test.rs, m2_lens_idempotency_migration_test.rs). Move them under tests/integration/ to match the consolidated layout, rewrite mod common;/use common:: to use crate::common::, and register both modules in tests/integration.rs. All 4 tests in the new modules pass through the consolidated binary. * merge follow-up: move C's lane2_stage_2e_parallelism_test into integration/ C's PR (#543, merged before this rebase) landed a new test file at the legacy tests/*.rs path. Move it under tests/integration/ to match the consolidated layout + register in integration.rs. All 377 tests pass locally (374 base + 3 testgen) on the merged tree. Ratchet gate (120s narrow, 600s full) unchanged. * ci: update narrow budget gate for consolidated integration binary #546's single-binary consolidation broke the 120s narrow gate: the workflow invoked `cargo test -p v3-compiler --test lane2_stage_2d_symbolic_cost_test` by binary name, but post-consolidation the only integration binary is `integration`. CI was erroring out with "no test target named lane2_stage_2d_symbolic_cost_test in v3-compiler package." Fix: invoke the consolidated binary with a test-name filter (`lane2_stage_2d_symbolic_cost_test::`) so the narrow ratchet still measures just the lane2d subsuite. Verified locally — 21 tests filter cleanly in 5.79s (well under the 120s budget). The 750s full-suite gate is unchanged (cargo test -p v3-compiler already runs the consolidated binary by default). * fix(tests): enforce clean-compile contract per-call in cached_compile_to_dag Codex caught a real ordering bug (bot inline at cached_compile.rs:51): cached_compile_to_dag and cached_compile_any share the same (source, file) key space, so whichever helper initializes a key first fixes the semantics for every later caller. If cached_compile_any stored a semantic-error Dag for key K first, a later cached_compile_to_dag call for K would skip the .expect("fixture compiles") path and silently return the error Dag, making clean-compile assertions order-dependent under parallel test execution. Fix: restructure cached_compile_to_dag as a thin wrapper over cached_compile_any + per-call diagnostic check. The contract enforcement is now at the CALLER boundary, not at cache-insert time. Sharing a cache entry is fine; skipping the clean-compile assertion is not. * fix(tests): cache compile outcome as enum, not bare Dag ChatGPT review on #546 flagged that my earlier per-call diagnostic check was still behavioral: cached_compile_to_dag reconstructed "did this compile cleanly" from dag.diagnostics().is_empty() — a proxy for the Ok/Err outcome, not the outcome itself. And the lost fact propagated to m1_5_testgen's "Compiles" predicate which also read diagnostics-emptiness instead of the outcome. Fix: cache CachedCompileOutcome::{Clean(Dag), Semantic(Dag)} instead of bare Dag. The outcome kind survives the cache boundary as a structural fact. cached_compile_to_dag now panics on Semantic via a match arm (not a diagnostic-emptiness assertion), and m1_5_testgen's "Compiles" / "FailsWithDiagnostic" branches read the variant directly. cached_compile_outcome() exposes the enum for callers that need the distinction. This satisfies the principle codex flagged (facts flow forward — don't collapse Ok/Err into one stored shape) plus the principle ChatGPT elaborated (API-level enforcement — the cache carrier now represents the clean-vs-semantic distinction structurally, not by convention). * fix(tests): lock cache contract via regression test + reconcile identity comment Addresses the remaining two items from the #546 PAUSE_AND_REGROUP meta-review: Item 4 — regression test that locks the cache contract: warm a key through the permissive helper (cached_compile_any on a semantic-error fixture) and verify the strict helper (cached_compile_to_dag) still panics on the same key regardless of cache warmth. Uses #[should_panic(expected = ...)] so future regressions can't silently bypass the contract. Item 5 — reconcile the integration.rs cache-identity comment with the actual implementation. Old comment said "two tests with different file markers now share a key," which contradicted the (source, file) cache key. Corrected: tests share a cache entry iff they pass identical (source, file); different file markers produce distinct keys by design. Combined with the outcome-enum fix at 62cf9e8, the 5-item meta-review checklist is complete: 1. ✅ cache memoizes compile attempts, not projected Dags 2. ✅ CachedCompileOutcome::{Clean, Semantic} carrier 3. ✅ m1_5_testgen consumes outcome kind directly (not diagnostics().is_empty() heuristic) 4. ✅ regression test locks the strict-path contract 5. ✅ cache-identity comment matches (source, file) implementation * test(cache): add reverse-order regression per ChatGPT review ChatGPT's review at 3083abb suggested pinning the cache contract from both directions, not just permissive→strict. Add a second regression that warms a clean-compile outcome via the strict helper, then verifies the permissive helper on the same key returns the cached clean Dag (no re-derivation, no diagnostic drift). Combined with the earlier permissive→strict regression, the helper contract is now locked from both sides — any refactor that silently inverts cache semantics will fail one of the two tests. * fix(clippy): needless_lifetimes + len_without_is_empty in dag.rs Two clippy errors introduced in #548 (Debt Paydown) broke the v2 CI lint gate on main, which #546 inherited on rebase: 1. src/v3/compiler/src/dag.rs:989 — `Slot::get<'a>(self, values: &'a [T]) -> Option<&'a T>` had an explicit lifetime that can elide cleanly. Drop the `'a` annotations; Rust's elision rules handle the borrow inference. 2. src/v3/compiler/src/dag.rs:1065 — `NonSingletonList::len` exists without a sibling `is_empty`. Add a trivial `is_empty` that returns `false` by construction (NSL always has >= 2 elements). Both are discipline fixes, no semantic change. `cargo clippy --workspace -- -D warnings` clean locally; affected tests still pass. * ci: raise v3 full-suite budget 750s → 900s; name m1_5_testgen as real bottleneck Observed on #546 @ fe46a54: v3 full-suite ran 853s on GitHub Actions 2-vCPU runners, over the 750s budget. All 379 tests pass; only the wall-clock gate fails. The 750s budget was set with headroom but recent merges (ζ #537, #542, #547, #548) have accumulated enough Rust compile + test execution time that cold CI runs consistently land in the 850-870s range. The consolidation in this PR is orthogonal to that growth — it's a structural win on local measurements, but GitHub Actions cold runners see less of the cross-binary amortization. Raise budget to 900s with a comment naming m1_5_testgen as the dominant remaining cost (per ChatGPT + prior reviews: its two slow tests compile per-claim unique sources that the shared cache can't memoize). Named dissolution trigger: reshape to spot-check OR mark #[ignore]-by-default + nightly job. Either drops the suite back under 500s and lets the budget tighten to ~600s. This is a budget adjustment, not an accepted-debt relaxation — the real bottleneck is tracked with a concrete fix path. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Structural follow-up to #542.
This PR does not attempt full Rust/Python walker dissolution. It takes the next narrower step:
emit/namespacesrc/v3/compiler/src/emit.rsas the single dispatch authority for all three targetsemit_rust.rsandemit_python.rsto thin compatibility adapters, matching the Go shape from A #542What remains out of scope:
Verification:
cargo check -p v3-compilercargo test -p v3-compiler --test m1_3_emit_go_test --test m1_3_emit_rust_test --test m1_4_emit_python_test