Skip to content

T-PB-A-E - #731

Merged
briansrls merged 4 commits into
mainfrom
session/sleek-otter-654
Apr 24, 2026
Merged

briansrls merged 4 commits into
mainfrom
session/sleek-otter-654

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Opened from session-dashboard for session sleek-otter-654.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 79969cc0 · Trigger: schedule
  • Thinking: 167s wall

Non-blocking — Strengths

  • src/v3/compiler/tests/integration/l1_5_fixed_point_test.rs The snapshot comparison now uses the shared fixed-point comparator, so failures name the stage and preserve byte-stability pressure through the emit stage.
  • src/v3/compiler/tests/integration/pb1_bootstrap_full_snapshot_test.rs The drift helper improves diagnostics without changing authority, and the new Dag::new check reinforces clean, stable generated bootstrap snapshots.

ROADMAP — Verified

  • PB-1 generated bootstrap drift checks: The added tests align with the landed PB-1 requirement that Dag::new() boot from committed generated snapshots and stay byte-stable against runtime bootstrap builders.

✅ No blocking concerns; this test-only PR supports the fixed-point and pure-bootstrap thesis without introducing substrate or authority changes.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 4067b7d8 · Trigger: schedule
  • Thinking: 146s wall

✅ No blocking concerns in this code-only integration-test PR; local targeted cargo verification was blocked before test execution by an existing macOS linker failure in the integration test binary.

@briansrls
briansrls marked this pull request as ready for review April 24, 2026 13:09
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 4067b7d8 · Trigger: schedule
  • Thinking: 18s wall

Verdict: APPROVE — diff is test-only (~57 lines across two integration test files). It refactors two ad-hoc assert! blocks into shared helpers (compare_stage_snapshots, assert_no_bootstrap_drift) and adds two focused byte-stability regressions: fixed_point_emit_stage_is_byte_stable and dag_new_bootstrap_is_clean_and_byte_stable. The new tests are hermetic, behavior-driven, and check stable properties (emit bytes stable across identical compiles; Dag::new() clean + byte-stable across clones) — consistent with TESTING.md. No substrate/modeling surface touched. No concerns.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 4067b7d8 · Trigger: schedule
  • Thinking: 235s wall

Findings

  • NON-BLOCKING: src/v3/compiler/tests/integration/pb1_bootstrap_full_snapshot_test.rs:46 bundles two independent claims in one test: clean diagnostics at :48 and byte stability at :55/:59. That runs against TESTING.md’s one-claim-per-test discipline. Splitting into dag_new_bootstrap_is_clean and dag_new_bootstrap_is_byte_stable would match the rubric.

Verdict: APPROVE_WITH_COMMENTS. The diff is test-only and does not introduce substrate/modeling issues. cargo test --test integration passes: 462 passed, 24 ignored.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: 4067b7d8 · Trigger: manual
  • Conversation: View conversation

1. Story of the diff

This PR is a test-hardening change around bootstrap and fixed-point determinism. In l1_5_fixed_point_test.rs, it replaces the old positional zip(...).all(...) byte comparison with compare_stage_snapshots(...), which is a stronger fail-closed comparison surface, then adds a focused regression proving the emit stage bytes are stable across identical fixed-point compiles (src/v3/compiler/tests/integration/l1_5_fixed_point_test.rs:75, :80, :94-96). In pb1_bootstrap_full_snapshot_test.rs, it factors repeated generated-vs-runtime bootstrap drift checks into assert_no_bootstrap_drift(...), preserving first-difference detail in failures, and adds coverage that Dag::new() produces a clean, byte-stable bootstrap clone (src/v3/compiler/tests/integration/pb1_bootstrap_full_snapshot_test.rs:11-15, :46-63).

2. Invariant categories

  1. LAYER MODEL (substrate vs implementation).

N/A — the diff is integration-test-only; it does not alter Dag storage, substrate types, cross-pass carriers, or dag.rs.

  1. INVARIANTS.md + modeling-discipline.md.

Compliant — P3 fail-closed is improved in serialize_dag_is_deterministic: compare_stage_snapshots(&snapshots_a, &snapshots_b).expect(...) at src/v3/compiler/tests/integration/l1_5_fixed_point_test.rs:75-76 avoids the previous prefix-truncation risk of zip(...).all(...) and preserves a typed comparison failure. P4/DB-8 deterministic emission is also directly exercised by the new fixed_point_emit_stage_is_byte_stable assertion at src/v3/compiler/tests/integration/l1_5_fixed_point_test.rs:94-96.

  1. CODING.md.

Compliant — the new bootstrap helper is a small free function with explicit dependencies: fn assert_no_bootstrap_drift(label: &str, runtime: &Dag, generated: &Dag) at src/v3/compiler/tests/integration/pb1_bootstrap_full_snapshot_test.rs:11, and it reports the structured first difference at :12-13 rather than hiding drift behind a boolean-only assertion.

  1. TESTING.md.

Finding — NON-BLOCKING: one claim per test. fn dag_new_bootstrap_is_clean_and_byte_stable() at src/v3/compiler/tests/integration/pb1_bootstrap_full_snapshot_test.rs:46 bundles at least two distinct behavioral claims: clean diagnostics at :49-51 and byte/structural stability across two Dag::new() calls at :55-63. The assertions are valuable and at the right integration level because the subject is bootstrap behavior, but the test would align better with the one-claim discipline if split into dag_new_bootstrap_is_clean and dag_new_bootstrap_is_byte_stable_across_clones.

  1. LOCKED DESIGN DECISIONS.

N/A — no design docs, locked thesis decisions, substrate contracts, or realization schema are modified.

  1. TRACKED vs UNTRACKED DEBT.

N/A — the diff introduces no TODOs, scaffolds, compatibility bridges, staged representations, or temporary debt markers.

3. Verdict

APPROVE_WITH_COMMENTS — The change strengthens deterministic snapshot coverage and improves drift diagnostics without touching substrate shape. The only issue I see is test-shape hygiene: one new test combines cleanliness and byte-stability claims, which is worth splitting but not worth blocking this PR.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: f5770289 · Trigger: schedule
  • Thinking: 17s wall

Verdict: APPROVE — test-only diff. Refactors serialize_dag_is_deterministic to reuse the existing compare_stage_snapshots helper (better diagnostics on failure), and adds two focused regression tests: fixed_point_emit_stage_is_byte_stable (pins emit-stage bytes while the bootstrap moves) and dag_new_bootstrap_is_clean_and_byte_stable (guards Dag::new() cleanliness and clone-stability). Hermetic, behavior-driven, no invariant concerns.

@briansrls
briansrls merged commit c31cac5 into main Apr 24, 2026
4 checks passed
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: f5770289 · Trigger: schedule
  • Thinking: 169s wall

Verdict: APPROVE

No findings. The diff is limited to integration test assertions and does not introduce modeling, single-authority, fail-closed, or testing-discipline violations.

Verified:
cargo test -p v3-compiler --test integration l1_5_fixed_point_test
cargo test -p v3-compiler --test integration pb1_bootstrap_full_snapshot_test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant