proud-owl-696

[briansrls]

Opened from session-dashboard for session proud-owl-696.

[briansrls]

claude-review · director-mode · proud-owl-696 (#541)

⚠️ Half of the asked-for ratchet; important gaps. Useful infrastructure but doesn't fix the actual regression or implement the coarse CI gate the user explicitly requested.

What landed

Layer 2 (per-test budget helper):

• budgeted.rs — DEFAULT_BUDGET_MS = 2000; with_budget_ms(budget_ms, f) panics if wall time exceeds budget. Panic message correctly cites the root pattern the user named: "share expensive setup (OnceLock / module cache) or collapse fine-grained tests; full bootstrap + compile per test is not allowed."
• budgeted_test! macro in common/mod.rs — ergonomic test declaration with default + custom-budget forms.

The infrastructure itself is well-designed. Matches feedback_test_timeout_2s memo.

Three gaps (blocking)

1. Doesn't implement Layer 1 (CI wall-clock gate)

The user's exact ask was "put a coarse ratchet on it (like 1 minute) to make sure we don't run into this again." That's the CI-level wall-clock gate — a single check in the v3 job that fails if the whole test suite takes longer than, say, 90-120 seconds. Layer 2 (per-test) is finer-grained — useful but not the coarse gate that was asked for. The coarse gate is what catches the dump-many-expensive-tests pattern globally; the per-test budget only catches individual hot tests.

Fix: add the .github/workflows/ci.yml change — something like:

- name: v3 tests with 120s budget
  run: |
    start=$(date +%s)
    cargo test -p v3-compiler
    elapsed=$(( $(date +%s) - start ))
    echo "v3 test wall time: ${elapsed}s"
    if [ $elapsed -gt 120 ]; then
      echo "::error::v3 tests took ${elapsed}s (budget: 120s) — share bootstrap setup via OnceLock or collapse fine-grained tests"
      exit 1
    fi

2. No first consumer — E-6 violation

The helper lands with zero tests opting in. Per E-6 (from this session's PR #533 Stage 1d §6 candidate invariants): "No target-spec field lands without a same-PR consumer." That discipline applies equally to test-infra helpers. Without a first consumer, nothing in the repo today actually benefits from this ratchet — a fresh regression of the ζ shape could land tomorrow and budgeted_test! would still be unused.

Fix: apply the macro to at least one test in this PR. The natural choice is the ζ regression itself — see #3.

3. Doesn't address the ζ regression — the motivating problem

src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs has 20 tests that each call compile_to_dag(source, file) independently, which is why v3 CI went from 65s → 650s (measured on main: δ #536 was 65s, ζ #537 was 644s, every merge after remained ~10x slower). Today's v3 suite takes ~10 minutes of wall time on main. Dropping a 120s CI gate on top would immediately red-flag every PR — including ones that don't cause new regressions. The ratchet is only useful once the current baseline is back under budget.

Fix: either

• (a) refactor lane2_stage_2d_symbolic_cost_test.rs to share a cached Dag via OnceLock (so the 20 tests collectively take <5s), OR
• (b) apply budgeted_test! to those 20 tests, watch them fail, and use the failure as the forcing function for a follow-up refactor PR.

Either way, the Layer 1 gate can't land until the baseline is restored.

Recommended path

1. Keep the Layer 2 infrastructure as-is in this PR — the design is clean.
2. Add the Layer 1 CI gate (YAML change) — gated at 120s behind a comment explaining why (with a plan to tighten to 90s post-ζ cleanup).
3. Apply budgeted_test! to the ζ test file OR refactor it to share compile setup via OnceLock — whichever the author prefers. Both are viable: the first forces the later refactor via test failures; the second does the refactor now. Refactoring now is cleaner because it lets the Layer 1 gate tighten sooner.
4. After 1-3 land, the combined ratchet is active: Layer 1 catches CI-wide regressions; Layer 2 catches per-test dump-without-dependency-analysis at author time.

One more concern

The Layer 2 macro is #[macro_export]-at-the-integration-test-crate-root. Each test binary is its own crate, so $crate::common::budgeted::with_budget_ms resolves relative to the importing test binary's crate — works only when that test binary has mod common; declared. That's a reasonable constraint for integration tests but worth documenting in the macro's rustdoc so consumers know to add mod common; before calling budgeted_test!.

Summary

Good foundation work on Layer 2. Blocked on shipping without Layer 1 + a first consumer + addressing the baseline regression that motivated the ratchet in the first place. The order to land this properly is (refactor ζ or apply budget to it) → Layer 1 CI gate → Layer 2 helper with first consumer — in one PR or tight sequence.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4520405399

ℹ️ 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".

[chatgpt-codex-connector]

Call budget helpers through public path in macro

budgeted_test! is exported to the integration-test crate root, but it expands to $crate::common::budgeted::... even though budgeted is declared as a private module (mod budgeted;). As a result, any test that starts using this macro will fail to compile with E0603: module budgeted is private. Route macro calls through the existing public re-exports ($crate::common::with_budget_ms / DEFAULT_BUDGET_MS) or make the module public.

Useful? React with 👍 / 👎.

[briansrls]

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

[briansrls]

claude-review · director-mode · proud-owl-696 (#541) — nearly there

Thanks for the comprehensive second pass — Layer 1 CI gate landed, ζ test file refactored, Layer 2 helper kept. All three gaps from my first review addressed.

One trivial compile error blocking CI:

error: unused imports: `DEFAULT_BUDGET_MS` and `with_budget_ms`
28 | pub use budgeted::{with_budget_ms, DEFAULT_BUDGET_MS};

The pub use re-exports at common::{with_budget_ms, DEFAULT_BUDGET_MS} aren't consumed anywhere — the budgeted_test! macro uses the fully-qualified $crate::common::budgeted::with_budget_ms path directly (which is the right choice structurally; re-exports would be redundant).

Fix: drop line 28 of common/mod.rs:

pub use budgeted::{with_budget_ms, DEFAULT_BUDGET_MS};

The mod budgeted; declaration at line 26 is all that's needed. Everything else flows through the macro.

Once CI is green we'll see whether the ζ test refactor dropped the v3 job back under the 120s budget — that's the real proof the Layer 1 gate + refactor combination works.

[briansrls]

codex · gpt-5.4 · 45204053

⚠️ Review (blocking: 1, non-blocking: 1+/0-)

BLOCKING (1)

Root Cause

• src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs The shared-setup fix is modeled as one global Mutex<HashMap<...>>, which serializes all cache misses; switch to a per-key OnceLock/LazyLock cache or compile outside the lock and insert afterward so the budget measures each test’s own work.

Non-blocking — Strengths

• .github/workflows/ci.yml The suite-level 120s check is a reasonable fail-closed performance ratchet and fits the repo’s existing CI-budget pattern.

⚠️ The performance-ratchet direction looks right, but the current cache lock can make the new per-test budget fail for contention rather than for a real regression.

[briansrls]

Violations (could not place on specific lines):

• src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs:43 BLOCKING: compile_to_dag runs while the global cache mutex is still held, so parallel budgeted_test! cases can exceed the 2s ratchet by waiting behind an unrelated cold compile instead of doing slow work themselves (Performance Invariant: the gate is no longer measuring the fact it claims to measure).

[briansrls]

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

[briansrls]

codex · gpt-5.4 · 6995abb8

✅ Review (blocking: 0, non-blocking: 2+/0-)

Non-blocking — Strengths

• src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs The test helper change matches the root cause from the prior review: compile work is shared per fixture key, but lock scope is limited to map lookup/insert only.
• .github/workflows/ci.yml The new 120s and 600s timing gates are scoped to the real regression surface, and the PR head’s v3 GitHub Actions job passed with the full-suite step logging 513s.

✅ This looks clean: the previous contention bug is fixed, the budget helper stays in implementation-only test code, and the new CI ratchet is behaving as intended on the current head commit.

[briansrls]

codex · gpt-5.4 · 6995abb8

✅ Review (blocking: 0, non-blocking: 2+/0-)

Non-blocking — Strengths

• src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs The test helper change matches the root cause from the prior review: compile work is shared per fixture key, but lock scope is limited to map lookup/insert only.
• .github/workflows/ci.yml The new 120s and 600s timing gates are scoped to the real regression surface, and the PR head’s v3 GitHub Actions job passed with the full-suite step logging 513s.

✅ This looks clean: the previous contention bug is fixed, the budget helper stays in implementation-only test code, and the new CI ratchet is behaving as intended on the current head commit.

[briansrls]

ChatGPT Review

_{Generated by gpt-5-4-pro}

Principle audit.

Fail-closed. Grounded in the thesis’s “broken causal link → diagnostic / explicit failure” framing and the active review checklist, this round looks good. The new CI gates in .github/workflows/ci.yml:101-129 fail explicitly with ::error::... + exit 1, and the local test helper in src/v3/compiler/tests/common/budgeted.rs:17-31 fails with a concrete assertion message rather than silently tolerating slow tests. That is behavioral enforcement, not type-level enforcement, but for test/CI code that is the right shape. chatgpt-review-8aae53ca-fd3a-42…

chatgpt-review-3898741a-e40e-48…

Illegal states unrepresentable. Satisfied. The new cache shape in src/v3/compiler/tests/lane2_stage_2d_symbolic_cost_test.rs:27-53 uses OnceLock<Dag> for exactly one absence meaning—“not initialized yet”—rather than overloading None/sentinel states. I do not see a new ambiguous state being admitted here. This is structural enough for the scope of the helper.

Facts flow forward. Satisfied. The expensive fact this suite keeps re-deriving—compile_to_dag(source, file)—now survives as a keyed cache and is actually consumed downstream by the individual tests via cached_compile_to_dag(...) instead of being silently recomputed. Likewise, the performance expectation is now explicit at both the per-test layer (budgeted_test!) and the coarse CI layer (120s/600s gates), rather than living as an implicit hope. The cache change is structural within the test binary; the budget gates are behavioral.

Coproduct dissolution. Satisfied / not really in play. This diff does not introduce a new substrate enum or a new Rust enum that would need 🟢/🟡/🔴 classification. The additions are a helper module, a macro, a cache, and CI wiring.

Single-authority metadata. Mostly satisfied. The budget mechanism now has one reusable authority in common::budgeted plus the macro wrapper in tests/common/mod.rs:246-273, and the lane2 suite has one cache authority in COMPILE_CACHE rather than a bunch of per-test setup snippets. Also, the current version in front of me already uses the better per-key OnceLock shape in lane2_stage_2d_symbolic_cost_test.rs:33-53, so I do not see the “global serial bottleneck” shape in this diff. My only watch-item is future duplication: if a second slow integration binary needs compile sharing, I would lift cached_compile_to_dag into tests/common before another lane-local cache lands.

API-level enforcement. Partly satisfied. Within this suite, budgeted_test! makes the budget discipline easy to apply and hard to forget. Repo-wide, though, this is still opt-in convention because plain #[test] remains available. I do not think that is a merge blocker here because the PR is clearly scoped to one regression surface rather than claiming repo-wide enforcement.

Design question. Should compile-sharing be a common test-fixture authority, or stay as an ad hoc cache inside each hot integration binary?

What is at stake is not this one cache—it is whether the repo learns one pattern or several. This PR correctly centralizes the budget mechanism in tests/common, but the shared compile result still lives only in lane2_stage_2d_symbolic_cost_test.rs:27-53. That is fine for one consumer. It becomes debt when a second suite copies the same idea with slightly different keys, locking, or failure behavior.

Path to convergence. I do not see a must-fix-before-merge structural issue in the diff as shown. The current per-key OnceLock cache shape is the right local form, the budget helper has a real same-PR consumer, and the CI ratchets fail closed enough for this round. The one thing I would explicitly treat as follow-up debt, not merge debt, is this: if any second integration binary wants shared compile_to_dag setup, move cached_compile_to_dag into src/v3/compiler/tests/common/ immediately so compile-sharing and budgeting live under one common authority instead of starting a family of lane-local caches.

Verdict. APPROVE.

This looks like a good round. It turns an untracked performance pain into explicit ratchets with a real consumer, and I do not see it baking new substrate/modeling debt into the compiler itself.

LOOP HEALTH: converging — this round makes forward progress by replacing repeated per-test full compiles with a bounded shared-setup path and explicit CI/test ratchets, without adding new substrate scaffolds or shifting debt across layers.

---

View conversation

[briansrls]

Meta-review in progress... (view conversation)

Loop-health check: is this review cycle making forward progress, or shifting debt? Posts in ~5-15 minutes.

[briansrls]

⚠️ ChatGPT review abandoned — still generating after 77 minutes

The conversation did not finish within the timeout window. The bot will start a fresh conversation on the next push.

View conversation