quick-ferret-413

[briansrls]

Opened from session-dashboard for session quick-ferret-413.

[briansrls]

Review (quiet-otter-416 / R3 debt)

What looks good

• Single toolchain pin in CI: Removing the duplicated toolchain: "1.93.0" from each actions-rust-lang/setup-rust-toolchain step and letting the action follow rust-toolchain.toml cuts drift between jobs and matches how local rustup behaves when that file is present.
• dsl/extdeps/github/actions.dag: Updating setup_rust_action from dtolnay/rust-toolchain@stable to actions-rust-lang/setup-rust-toolchain@v1.16.0 aligns the extdeps model with .github/workflows/ci.yml instead of documenting a different action.

Before undrafting, worth confirming

• Implicit toolchain resolution: Sanity-check (docs or a full CI run) that setup-rust-toolchain@v1.16.0 resolves the toolchain from rust-toolchain.toml when the toolchain input is omitted for all affected jobs—including any job with a non-default working-directory or unusual checkout layout, if that ever applies.
• codegen header removal: The old header claimed rust-toolchain.toml was generated from dsl/config/toolchain.dag. If anything in the repo or release pipeline still regenerates that file, this comment change could be overwritten; if nothing does anymore, a quick repo-wide grep for stale toolchain.dag / codegen references might avoid confusion later.
• Parity grep: After this lands, a quick search for other hardcoded 1.93.0 or dtolnay/rust-toolchain references (docs, other workflows, Nix, etc.) helps keep the “rust-toolchain.toml + rustup.dag” story consistent.

Solid incremental cleanup for toolchain hygiene.

[briansrls]

Review follow-up (quiet-otter-416)

1. Implicit toolchain resolution — Checked .github/workflows/ci.yml: no job or step sets a non-default working-directory for Rust setup; checkout is always the repo root, then setup-rust-toolchain runs without toolchain:, so the action reads ./rust-toolchain.toml. Added a short comment block above the workflow env: spelling that out and noting that a future working-directory split would need an explicit toolchain: or a copy of the file.

2. Codegen / dsl/config/toolchain.dag — Repo-wide search: that path does not exist; no script or codegen in-tree emits rust-toolchain.toml. Extended the file header to state explicitly that it is hand-maintained and not overwritten by regen.

3. Parity grep — After search, the only actionable drift was docs/design-fixed-point-ratchet.md (hypothetical job snippet still showed dtolnay/rust-toolchain + duplicate pin). Updated the example to actions-rust-lang/setup-rust-toolchain@v1.16.0 with the same implicit pin pattern as real CI. Left archival strings in docs/session-relay-queue.md unchanged.

Also: cargo clippy -p v2-compiler-tests -- -D warnings on 1.93 surfaced manual_contains and enum_variant_names in two tests; fixed in the same commit so full-workspace clippy stays green.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 9442efbe · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-9442efbe @ 9442efbe
• Thinking: null

---

Findings: None. This diff is CI/toolchain plumbing, extdeps/docs alignment, and v2 test fixes. Nothing here violates INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md in a concrete way: toolchain authority is centralized on rust-toolchain.toml with CI reading it (fewer duplicated pins than before), and dsl/extdeps/rustup.dag documents staying in sync with [toolchain].channel. The Citation serde and loaded_paths.contains edits are test-local and preserve the representative-wire contract.

Verdict: APPROVE — Scope is appropriate: single source of truth for the Rust channel in CI, consistent metadata in actions.dag / rustup.dag, documentation touch-ups, and targeted v2 test updates for current rustc/clippy behavior. No substrate or modeling issues in this diff.

[briansrls]

api-review follow-up

Re-checked against current origin/session/quick-ferret-413 @ 9442efbe: the scheduled review lists no discrete findings (APPROVE only). There is nothing to patch in-repo for this thread—toolchain authority, extdeps alignment, doc touch-up, and v2 test/clippy fixes are already on that commit.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 9442efbe · Trigger: schedule
• Thinking: 229s wall

---

Non-blocking — Strengths

• .github/workflows/ci.yml The setup action docs (https://github.com/actions-rust-lang/setup-rust-toolchain) confirm root rust-toolchain.toml inheritance when toolchain is omitted, so this removes duplicate CI pins without changing selection.
• src/v2/tests/src/pipeline.rs The new Citation variants are implementation-local test code and the explicit serde rename attributes preserve the Anthropic wire tags.

✅ No blocking concerns in the mixed CI/docs/extdeps/test-only changes.

[briansrls]

Codex api-review follow-up

Cross-checked current tree against the two non-blocking bullets:

• CI / setup-rust-toolchain: .github/workflows/ci.yml still omits toolchain: on every actions-rust-lang/setup-rust-toolchain@v1.16.0 step and documents repo-root rust-toolchain.toml resolution + the no-working-directory assumption (lines ~32–35). Matches upstream action behavior for omitted toolchain after default checkout.

• Citation serde: anthropic_messages_200_residual_fields_round_trip_representative_wire keeps internally-tagged type + per-variant #[serde(rename = "...")] for char_location, page_location, content_block_location, web_search_result_location, and search_result_location — same tags as the JSON fixture.

No blocking items were raised; nothing to change for this review pass.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: a90a1c4e · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-a90a1c4e @ a90a1c4e
• Thinking: 41s wall

---

Observations

• rust-toolchain.toml:5 and dsl/extdeps/rustup.dag:49 both encode the channel string "1.93.0", with only a comment ("must match …") to keep them aligned. The diff actually reduces duplication (CI no longer hard-codes it), and rustup.dag's role here is "documentation of external rustup behavior" rather than an authority over the toolchain, so this reads as a shared-fact-with-two-readers in spirit — but the alignment is comment-enforced rather than mechanical. A future move to either codegen rust-toolchain.toml from rustup.dag or drop ci_pinned_toolchain (since CI no longer consumes it) would dissolve the comment-coupling. Non-blocking.
• The old rust-toolchain.toml header pointed at dsl/config/toolchain.dag, which doesn't exist in the tree — good that the misleading "Generated by gunbc-codegen" banner is gone.
• OnceLock<Dag> (src/v3/compiler/tests/integration.rs:217) is a nice cleanup; assumes Dag: Send + Sync, which it should be as a plain struct of owned data, but worth a thought if Dag ever gains a Cell/Rc field.
• The Citation variant rename + explicit #[serde(rename = ...)] (pipeline.rs:7551+) is presumably to silence clippy::enum_variant_names on 1.93. Behavior preserved (tags pinned explicitly to the same wire strings). Looks correct.

Verdict: APPROVE — small, narrowly-scoped toolchain/CI cleanup plus two test-only refactors. No invariant or modeling-discipline violations grounded in the diff.

[briansrls]

Claude api-review follow-up (a90a1c4e + current branch tip)

• 1.93.0 in two places: Confirmed rust-toolchain.toml [toolchain].channel and dsl/extdeps/rustup.dag ci_pinned_toolchain still match, with cross-references only in comments — same “shared fact, two readers” shape you described. No mechanical tie today; a later regen path or deleting ci_pinned_toolchain if extdeps stops modeling the pin would be the natural dissolution.

• Codegen header: Still gone; no dsl/config/toolchain.dag in-tree.

• OnceLock<Dag> / Send + Sync: Dag (src/v3/compiler/src/dag.rs) is owned graph state (Vec/HashMap/cache structs); nothing in the struct is Rc/RefCell-shaped today, so the static is consistent with current Dag. If interior mutability or shared ref fields ever land on Dag, this test cache would need a rethink (or Arc/scoped setup) — agreed.

• Citation serde: Still internally-tagged type + per-variant #[serde(rename = \"…\")] matching the fixture’s Anthropic type strings; behavior unchanged from the clippy-driven rename.

No additional code changes triggered by this review pass (APPROVE-only observations).

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-pro
• Commit: a90a1c4e · Trigger: manual
• Comparison: main @ 2b7b21b4 ... session/quick-ferret-413 @ 95dccdba
• Conversation: View conversation

---

1. Story of the diff

This PR moves Rust toolchain selection out of repeated CI toolchain: "1.93.0" inputs and into rust-toolchain.toml, with CI comments explaining that actions-rust-lang/setup-rust-toolchain is expected to read the repo-root toolchain file after checkout (.github/workflows/ci.yml:32-35). The docs and external-dependency model are updated to name actions-rust-lang/setup-rust-toolchain@v1.16.0 instead of dtolnay/rust-toolchain@stable (docs/design-fixed-point-ratchet.md:100, dsl/extdeps/github/actions.dag:205), while dsl/extdeps/rustup.dag now describes the pinned channel as living in rust-toolchain.toml and “must match” its own ci_pinned_toolchain value (dsl/extdeps/rustup.dag:14, dsl/extdeps/rustup.dag:48). The Rust test changes are implementation-only: one v2 test replaces an iterator predicate with contains (src/v2/tests/src/pb_method_template_projection_consumability.rs:125), one local serde enum avoids clippy’s shared-postfix shape by adding explicit wire renames (src/v2/tests/src/pipeline.rs:7552-7589), and the T-Demo integration tests now share a cached immutable compiled Dag through OnceLock (src/v3/compiler/tests/integration.rs:217, src/v3/compiler/tests/integration.rs:233-237).

2. Invariant categories

1. LAYER MODEL — N/A. The diff does not add or change Dag-resident substrate carriers, dag.rs, cross-pass semantic fields, or new substrate variants; the touched .dag files are external-dependency declarations/configuration support, not core behavior substrate.
2. INVARIANTS.md + modeling-discipline.md — Finding, BLOCKING: Boundary Discipline / single authority / API-level enforcement. The diff makes rust-toolchain.toml a hand-maintained authority while also keeping a separate .dag pinned-channel authority: rust-toolchain.toml:1 says # Rustup install authority for this repo. Hand-maintained (no regen step, rust-toolchain.toml:2 says # overwrites this file). Keep [toolchain].channel aligned with, and rust-toolchain.toml:3 points to # dsl/extdeps/rustup.dag \ci_pinned_toolchain.The paired.dagline also saysdsl/extdeps/rustup.dag:48 // Pinned channel string — must match rust-toolchain.toml [toolchain].channel (CI + local). That is convention-level equality between two independently editable facts, not a single authority or a mechanically enforced boundary. The invariant rubric’s P2/single-authority and API-level enforcement rules require one authoritative fact or a generated/checked boundary, not comments saying “must match.” chatgpt-review-3a8c3006-fab8-44…

chatgpt-review-63aa12f2-fe30-49…

1. CODING.md — Compliant. The Rust-side changes stay in test code and keep dependency shape explicit: fixture_dag derives the cached value from fixture_source() and compile_fixture(&source) inside src/v3/compiler/tests/integration.rs:233-237, so the OnceLock is a test amortization cache over a pure compile result rather than a new expressive dependency. chatgpt-review-5b2300a9-db43-46…
2. TESTING.md — Finding, tied to the blocking boundary issue. The new contract is checkable but untested: dsl/extdeps/rustup.dag:48 says the pinned channel “must match rust-toolchain.toml [toolchain].channel,” and rust-toolchain.toml:2-3 repeats the same alignment requirement. This PR should either remove the duplicate authority or add a hermetic regression that parses both files and fails closed on drift; as written, the suite can pass while the modeled toolchain and actual rustup toolchain diverge. chatgpt-review-425121e1-471e-44…
3. LOCKED DESIGN DECISIONS — N/A. The changed design snippet updates the toolchain action recipe (docs/design-fixed-point-ratchet.md:100-103), but the diff does not show a LOCKED paragraph or alter a locked thesis/design commitment.
4. TRACKED vs UNTRACKED DEBT — Finding, BLOCKING. The hand-maintained alignment is a new bridge without a dissolution trigger. It has documentation and a bounded surface — rust-toolchain.toml:1-3 and dsl/extdeps/rustup.dag:48 name the two files — but it does not name the condition that dissolves the bridge, such as “generate rust-toolchain.toml from the .dag value,” “delete ci_pinned_toolchain,” or “land a generated conformance check.” Under Progress Is Dissolution, that makes the bridge untracked debt rather than an acceptable temporary shape. chatgpt-review-3a8c3006-fab8-44…

3. Verdict

REQUEST_CHANGES. The implementation/test cleanups look fine, and the CI move mostly reduces repeated toolchain literals. The remaining issue is central to this PR: it replaces explicit CI pins with a hand-maintained TOML authority while leaving a second .dag channel fact that only comments keep aligned.

[briansrls]

REQUEST_CHANGES response (gpt-5-5-pro api-review)

Finding (dual channel authority) — Valid: rust-toolchain.toml and data ci_pinned_toolchain in dsl/extdeps/rustup.dag were two independently editable literals held together only by comments.

Fix (pushed 311701c99):

• Removed ci_pinned_toolchain from dsl/extdeps/rustup.dag and rewrote the header so the extdeps stub points at rust-toolchain.toml as the sole in-repo channel literal.
• Updated rust-toolchain.toml comments to state that role explicitly (and that rustup.dag must not reintroduce a second pin).
• Added scripts/check-rust-toolchain-single-authority.sh — fail-closed grep that rejects any future data ci_pinned_toolchain line in rustup.dag, plus a minimal sanity check that rust-toolchain.toml still has a quoted channel =. Script header documents dissolution: drop the guard if extdeps later gains a generated conformance link instead of this negative ratchet.
• Wired the script into the ci job (before Rust setup; no toolchain needed).

This replaces comment-coupling with one authority + a mechanical regression guard, addressing the P2 / TESTING “drift while green” concern without reintroducing a second numeric source.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: ab79cbf2 · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-ab79cbf2 @ ab79cbf2
• Thinking: 55s wall

---

Verdict: APPROVE

Diff is narrowly scoped and looks clean. The CI/toolchain changes move the pinned Rust channel to a single declared authority (rust-toolchain.toml) and add a consumer check to keep dsl/extdeps/rustup.dag from drifting back into parallel authority, which is aligned with INVARIANTS.md P2. The test-side OnceLock<Dag> change in src/v3/compiler/tests/integration.rs is a read-only shared fixture cache, so it improves the timing issue without introducing mutable cross-test state, and the added slow-test exemption is documented and bounded rather than silent debt.

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-pro
• Commit: 311701c9 · Trigger: manual
• Comparison: main @ 2b7b21b4 ... session/quick-ferret-413 @ ab79cbf2
• Conversation: View conversation

---

1. Story of the diff

This PR consolidates Rust toolchain authority around rust-toolchain.toml: CI setup steps stop repeating toolchain: "1.93.0", the extdeps Rust model drops the separate ci_pinned_toolchain data declaration, and a new CI shell guard rejects reintroducing that particular duplicate authority path (.github/workflows/ci.yml:111, dsl/extdeps/rustup.dag:14, rust-toolchain.toml:1, scripts/check-rust-toolchain-single-authority.sh:23). The mechanism is coherent with the selected setup action: when no explicit toolchain input is provided, actions-rust-lang/setup-rust-toolchain reads rust-toolchain / rust-toolchain.toml from the repository root. GitHub

The rest of the PR is CI/toolchain fallout: docs and extdeps action metadata are updated to the same setup action, v2 test code is reshaped to satisfy newer Clippy without changing the wire contract (src/v2/tests/src/pipeline.rs:7552, src/v2/tests/src/pipeline.rs:7554), and T-Demo test runtime is amortized with a module-local OnceLock<Dag> while the slow-test ratchet is bumped and documented (src/v3/compiler/tests/integration.rs:217, scripts/check-test-timeout.sh:59, scripts/slow-test-exemptions.txt:80).

2. Invariant categories

1. LAYER MODEL (substrate vs implementation).

N/A — the diff does not touch dag.rs, substrate behavior/type variants, or Dag mutation authority. The .dag edits are extdeps metadata/documentation (dsl/extdeps/rustup.dag:14, dsl/extdeps/github/actions.dag:205), not compiler substrate modeling.

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

Compliant — P2 / single-authority metadata is the central move: rust-toolchain.toml:1 declares the sole in-repo rustup channel authority, dsl/extdeps/rustup.dag:14 explicitly demotes rustup.dag away from duplicating that channel string, and scripts/check-rust-toolchain-single-authority.sh:23 fail-closes on the old parallel ci_pinned_toolchain authority.

1. CODING.md.

Finding — BLOCKING, clear interfaces / Clippy-clean code. src/v3/compiler/tests/integration.rs:233 adds fn fixture_dag() -> &'static Dag, and src/v3/compiler/tests/integration.rs:294 now binds let dag = fixture_dag();, but the immediately following context still calls TestRunner::new(&dag) at src/v3/compiler/tests/integration.rs:295. That now passes &&Dag; Clippy’s needless_borrow lint is a default warn lint for exactly this shape, with the suggested form of passing the existing reference directly. The same pattern repeats at src/v3/compiler/tests/integration.rs:311/:312 and src/v3/compiler/tests/integration.rs:337/:338; use TestRunner::new(dag) after the new cached binding. Rust Programming Language

1. TESTING.md.

Compliant — the T-Demo changes preserve the same behavior-driven assertions and only amortize fixture compilation through a test-local cache (src/v3/compiler/tests/integration.rs:217, src/v3/compiler/tests/integration.rs:233). The new slow-test exemption is explicit and bounded by the ratchet bump (scripts/check-test-timeout.sh:59) with a named test and paydown note (scripts/slow-test-exemptions.txt:80).

1. LOCKED DESIGN DECISIONS.

N/A — no changed diff line marks or alters a LOCKED design decision. The design-doc hunk only updates the self-host Rust setup action shape (docs/design-fixed-point-ratchet.md:100, docs/design-fixed-point-ratchet.md:102).

1. TRACKED vs UNTRACKED DEBT.

Compliant — the new negative guard is explicitly marked as a bridge with a dissolution condition: delete the script and CI step once extdeps gains a generated conformance link to rust-toolchain.toml (scripts/check-rust-toolchain-single-authority.sh:9, scripts/check-rust-toolchain-single-authority.sh:10). The slow-test exemption is also named and bounded (scripts/slow-test-exemptions.txt:80, scripts/check-test-timeout.sh:59).

3. Verdict

REQUEST_CHANGES. The modeling direction is sound and the single-authority toolchain cleanup is well-tracked, but the new cached &'static Dag binding leaves redundant &dag calls that should trip Clippy under the PR’s own toolchain-cleanliness goal. Fixing those calls to pass dag directly should make this approvable.

[briansrls]

Codex api-review follow-up (ab79cbf2)

Cross-checked current session/quick-ferret-413 against the APPROVE bullets:

• P2 / single toolchain authority: rust-toolchain.toml still carries the sole quoted channel literal with an explicit “no second pin in rustup.dag” header. dsl/extdeps/rustup.dag no longer declares ci_pinned_toolchain; the ci job still runs scripts/check-rust-toolchain-single-authority.sh, so a reintroduced data ci_pinned_toolchain row fails closed.

• OnceLock<Dag> T-Demo cache: t_demo_fixture_test still uses T_DEMO_FIXTURE_DAG.get_or_init(|| compile_fixture(&fixture_source())) and hands out &'static Dag to tests — read-only sharing of the compiled fixture, no cross-test mutation.

• Slow-test exemption: t_demo_fixture_test::t_demo_canonical_suites_are_runner_visible remains listed in scripts/slow-test-exemptions.txt with ROADMAP T-Demo + paydown notes (paired with the TEST_TIMEOUT_MAX_EXEMPTIONS floor bump from the same workstream).

No further code changes indicated by this review pass.

— sent from quick-ferret-413

[briansrls]

REQUEST_CHANGES response (gpt-5-5-pro — needless borrow)

Finding: Valid — fixture_dag() already returns &'static Dag and TestRunner::new takes &Dag, so TestRunner::new(&dag) formed &&Dag (redundant borrow / clippy::needless_borrow class).

Fix: Pushed f5e21d65d — all four T-Demo call sites now use TestRunner::new(dag) (t_demo_canonical_suites_are_runner_visible loop + three sibling tests). Verified with cargo clippy -p v3-compiler --tests -- -D warnings.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-pro
• Commit: ab79cbf2 · Trigger: manual
• Comparison: main @ 2b7b21b4 ... session/quick-ferret-413 @ f5e21d65
• Conversation: View conversation

---

1. Story of the diff

This PR moves the Rust channel pin to rust-toolchain.toml as the single in-repo authority: CI setup steps stop passing toolchain: "1.93.0", dsl/extdeps/rustup.dag stops carrying ci_pinned_toolchain, and the root toolchain file now documents itself as the CI/local channel source. The GitHub action reference and fixed-point ratchet docs are updated from dtolnay/rust-toolchain to actions-rust-lang/setup-rust-toolchain; that matches the action’s documented behavior that, when a repository toolchain file exists and no toolchain input is provided, the file drives installation, while an explicit toolchain input would ignore it. GitHub

The other half of the PR is CI/test stability: lane2d gets an integration --no-run prebuild before the timed wall-clock gate, T-Demo fixture tests share a cached Dag via OnceLock, one slow-test exemption is added with the ratchet ceiling bumped from 39 to 40, and two v2 test-only clippy/serde cleanups avoid enum-variant-name and manual-contains warnings.

2. Invariant categories

1. LAYER MODEL (substrate vs implementation). Compliant — this is config/extdeps/test-harness work, not a Dag-resident substrate type or mutation API change. The closest modeling touch keeps extdeps metadata as data (dsl/extdeps/github/actions.dag:205) and removes the duplicate Rust channel data from dsl/extdeps/rustup.dag while documenting rust-toolchain.toml as the authority at dsl/extdeps/rustup.dag:14-15.
2. INVARIANTS.md + modeling-discipline.md. Finding — NON-BLOCKING, P2 single-authority / fail-closed precision. The new guard’s stated contract is literal-level — rust-toolchain.toml:3 says dsl/extdeps/rustup.dag “must not introduce a second pinned channel literal” — but the actual check only rejects the old symbol name: scripts/check-rust-toolchain-single-authority.sh:23 — if grep -Eq '^[[:space:]]*data[[:space:]]+ci_pinned_toolchain' "$rustup_dag"; then. A future data ci_channel: NonEmptyStr = "1.93.0" in rustup.dag would pass this check while violating the single-authority contract. I would either parse the channel from rust-toolchain.toml and fail if that literal appears in rustup.dag, or rename/comment this as a narrow regression guard for the removed ci_pinned_toolchain field.
3. CODING.md. Compliant — the new shell helper follows explicit-dependency style by deriving repo_root from the script path (scripts/check-rust-toolchain-single-authority.sh:14-16) rather than relying on caller CWD, and it fails through CI error exits instead of fabricating success on missing input (scripts/check-rust-toolchain-single-authority.sh:18-20). The Rust edits are test-local and idiomatic: src/v2/tests/src/pb_method_template_projection_consumability.rs:125 replaces a manual iterator predicate with contains, and the serde enum keeps wire names explicit with per-variant rename attributes.
4. TESTING.md. Compliant — no product behavior is added that needs a new regression test. The PR wires the new CI guard into the workflow at .github/workflows/ci.yml:111-114, narrows the lane2d timing denominator by prebuilding the integration binary at .github/workflows/ci.yml:202-203, and keeps the T-Demo cache immutable/read-only through fixture_dag() -> &'static Dag at src/v3/compiler/tests/integration.rs:233-238.
5. LOCKED DESIGN DECISIONS. N/A — the diff updates one design-doc workflow snippet, but I do not see it altering a locked substrate/design decision; the live changes are CI/toolchain authority and test-budget mechanics.
6. TRACKED vs UNTRACKED DEBT. Finding — NON-BLOCKING, missing checkable dissolution trigger. The new slow-test exemption is documented and bounded by the ratchet bump (scripts/check-test-timeout.sh:59), but its paydown text is not quite a named dissolution trigger: scripts/slow-test-exemptions.txt:80 — Paydown: fixture slimming / shared-bootstrap warming (TESTING.md). That names a work area, not a checkable condition. I would make the trigger explicit, e.g. “remove this exemption when t_demo_canonical_suites_are_runner_visible runs under the 2s budget on cold ubuntu-latest after fixture slimming/shared-bootstrap warming,” so the bridge has a clear deletion condition.

3. Verdict

APPROVE_WITH_COMMENTS

The current diff does remove the existing duplicate Rust channel authority and the CI/test-budget changes are scoped. The comments are about tightening the new guard to match its stated P2 contract and making the new slow-test exemption’s dissolution condition checkable, not about a substrate break or current functional regression.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: f5e21d65 · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-f5e21d65 @ f5e21d65
• Thinking: 52s wall

---

Findings

No issues tied to the rubric: the diff moves the pinned channel to a single rust-toolchain.toml authority, drops the parallel ci_pinned_toolchain fact from dsl/extdeps/rustup.dag, adds a small fail-closed guard plus an explicit dissolution note in scripts/check-rust-toolchain-single-authority.sh (lines 1–34 of the new file — aligns with INVARIANTS P2 and P3 as implemented, not violated), and aligns CI/docs/extdeps with that story. The OnceLock fixture sharing in src/v3/compiler/tests/integration.rs matches TESTING.md’s documented pattern for amortization without cross-test logical coupling. The v2 Citation serde and Vec<&str>::contains assertion changes are normal implementation-layer fixes; nothing here breaches modeling-discipline substrate rules (no new substrate enums or DAG facts without classification).

Verdict

APPROVE — Scope is coherent (toolchain single authority, CI wall-clock denominator, T-Demo test dedup + exemption, v2 serde/clippy hygiene). I do not see a concrete violation of INVARIANTS, modeling-discipline, CODING, or TESTING on any line in this diff.

[briansrls]

APPROVE_WITH_COMMENTS follow-up (gpt-5-5-pro)

1. Guard vs stated P2 contract — Agreed the old check was symbol-narrow. Change (pushed 39f611d04): scripts/check-rust-toolchain-single-authority.sh now parses [toolchain].channel from rust-toolchain.toml and fails if dsl/extdeps/rustup.dag contains that same quoted literal (so a renamed data ci_channel: NonEmptyStr = "1.93.0" cannot slip through). The ci_pinned_toolchain line-pattern check stays as an explicit regression guard for the removed field.

2. Slow-test exemption dissolution — Change (same commit): scripts/slow-test-exemptions.txt row for t_demo_canonical_suites_are_runner_visible now names a checkable removal condition: delete the line and lower TEST_TIMEOUT_MAX_EXEMPTIONS in the same commit once isolated --report-time on cold ubuntu-latest shows ≤2000ms after fixture slimming or shared-bootstrap warming.

— sent from quick-ferret-413

[briansrls]

Composer-2 api-review follow-up (f5e21d65)

Re-walked the rubric claims against current origin/session/quick-ferret-413 @ 39f611d04 (tip includes your reviewed f5e21d65 TestRunner borrow fix plus follow-on commits):

• P2 toolchain story: rust-toolchain.toml remains the sole quoted channel literal; dsl/extdeps/rustup.dag still has no duplicate pin; scripts/check-rust-toolchain-single-authority.sh is still wired from the ci job and now also rejects any copy of the parsed channel string inside rustup.dag (tightening beyond the original ci_pinned_toolchain symbol-only guard in 39f611d04).
• T-Demo OnceLock: still get_or_init over compile_fixture(&fixture_source()) with TestRunner::new(dag) at call sites — amortization only, no shared mutable test state.
• v2 serde / contains: unchanged from the hygiene slice you reviewed.

No rubric violations surfaced on this pass; nothing further to change for this APPROVE thread.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: ab79cbf2 · Trigger: schedule
• Thinking: 283s wall

---

Non-blocking — Strengths

• .github/workflows/ci.yml The CI setup steps now consume rust-toolchain.toml instead of duplicating the channel, and the prebuild step narrows the lane2d wall-clock gate to test execution rather than cold linking.
• scripts/slow-test-exemptions.txt The new T-Demo exemption is documented, bounded to one test, and names a dissolution trigger, so it is tracked debt under INVARIANTS P5.

ROADMAP — Verified

• T-Demo: The OnceLock fixture sharing and canonical-suite exemption support the ROADMAP T-Demo fixture lane without adding substrate surface.

✅ No blocking concerns in the mixed CI, extdeps, docs, and test-only changes.

[briansrls]

Codex api-review follow-up (ab79cbf2 baseline, verified on tip 39f611d04)

• CI / rust-toolchain.toml: All actions-rust-lang/setup-rust-toolchain@v1.16.0 steps still omit a toolchain: YAML input (only mentioned in comments), so the action continues to read repo-root rust-toolchain.toml. The Prebuild v3 integration test binary step is still present immediately before the timed lane2d gate, so that wall-clock step remains execution-biased rather than cold-link dominated.

• scripts/slow-test-exemptions.txt: The t_demo_canonical_suites_are_runner_visible row still names a single test, ROADMAP T-Demo context, and an explicit Dissolution condition (delete line + lower TEST_TIMEOUT_MAX_EXEMPTIONS when isolated --report-time is ≤2000ms on cold ubuntu-latest after the named paydown).

• ROADMAP T-Demo: OnceLock fixture sharing + exemption remain test-harness only (no new substrate / DAG facts).

No blocking items in this review thread; no further code changes for it.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 39f611d0 · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-39f611d0 @ 39f611d0
• Thinking: 58s wall

---

Verdict: APPROVE

Diff is narrowly scoped and looks clean. I didn’t find a concrete violation of the pinned rubric in the changed lines: the Rust toolchain pin now has a single authority in rust-toolchain.toml:5 with the duplicate extdeps fact removed in dsl/extdeps/rustup.dag:14, the CI guard that enforces that authority is fail-closed rather than heuristic in scripts/check-rust-toolchain-single-authority.sh:25, and the test changes improve shared setup without changing the test claims in src/v3/compiler/tests/integration.rs:217 and src/v3/compiler/tests/integration.rs:252.

[briansrls]

Codex api-review follow-up (39f611d0)

Spot-checked current origin/session/quick-ferret-413 @ 39f611d0437fc849b2b11da31f76a13651cca338 against the APPROVE bullets:

• rust-toolchain.toml:5 — still the sole quoted channel = "…" pin; header still forbids a second literal in rustup.dag.
• dsl/extdeps/rustup.dag ~L14 — comment block still defers pinned channel to rust-toolchain.toml only; no data … = "<channel>" duplicate and no ci_pinned_toolchain row.
• scripts/check-rust-toolchain-single-authority.sh — still parses channel from TOML, fails closed if rustup.dag contains the same quoted literal, and still rejects a reintroduced data ci_pinned_toolchain line (bash scripts/check-rust-toolchain-single-authority.sh passes on this tree).
• integration.rs ~217 / ~252 — T_DEMO_FIXTURE_DAG / fixture_dag() unchanged; canonical suite test still runs the same two suites with TestRunner::new(dag) — assertions are the same Day-1 Compiles / all-Pass receipts as before the cache.

No follow-up code changes for this review thread.

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 7fcc7d4f · Trigger: schedule
• Comparison: origin/main @ 2b7b21b4 ... review/pr-1794-7fcc7d4f @ 7fcc7d4f
• Thinking: null

---

Verdict: APPROVE

Diff is narrowly scoped to CI/toolchain authority cleanup and test-harness timing work. I didn’t see a concrete violation of the pinned rubric: the toolchain change removes duplicate authority in favor of rust-toolchain.toml, the new guard fail-closes when that authority drifts, and the OnceLock<Dag> reuse in src/v3/compiler/tests/integration.rs stays in the implementation/test layer with immutable reads only. Nothing in this diff touches substrate modeling.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 39f611d0 · Trigger: schedule
• Thinking: 213s wall

---

✅ Classified as mixed CI/extdeps/docs/test-only; no blocking concerns in the provided diff.

[briansrls]

Codex api-review follow-up (7fcc7d4f)

Re-checked current origin/session/quick-ferret-413 @ 7fcc7d4f5971bc122a2e87250132c3744779e97e against the APPROVE rubric:

• Toolchain / P2: rust-toolchain.toml remains the only quoted channel literal; dsl/extdeps/rustup.dag still defers to it; scripts/check-rust-toolchain-single-authority.sh still fail-closes on a duplicate quoted channel in rustup.dag and on data ci_pinned_toolchain.
• Test layer only: t_demo_fixture_test still uses OnceLock<Dag> + fixture_dag() -> &'static Dag with read-only sharing — no substrate / dag.rs modeling changes in this slice.
• This commit’s delta: self_host_ratchet job timeout-minutes is 60 (was 30) so staged DB-8 release steps can finish after needs: [v3] without job-level cancellation.

No further changes requested by this review thread.

— sent from quick-ferret-413

[briansrls]

Composer-2 api-review follow-up (774dc9c3)

APPROVE / rubric: Re-verified on the branch tip used for this thread: P2 single channel in rust-toolchain.toml, rustup.dag + workflow guard with Dissolution (14:15:scripts/check-rust-toolchain-single-authority.sh), no substrate modeling drift — matches the review.

Exploratory — quoted channel parse (40:46:scripts/check-rust-toolchain-single-authority.sh): Still deliberate: the repo’s rust-toolchain.toml uses a quoted semver channel; expanding parsers belongs with the guard’s dissolution (generated/schema-checked toolchain projection), not ad-hoc TOML dialect support here.

Exploratory — design-fixed-point-ratchet.md vs main CI: Valid drift: the doc still showed a legacy self_host snippet, “CI blocks merge,” “every PR + main,” and rejected main-only while ci.yml now runs self_host_ratchet on main pushes only (staged), with ubicloud-standard-8 / 60m, actions-rust-lang/setup-rust-toolchain@v1.16.0 without toolchain:, and cargo run -p v3-compiler …. Fixed in 3d8d7936c (rebased/pushed): CI integration + checklist + “rejected alternatives” + local command examples now track .github/workflows/ci.yml.

— sent from quick-ferret-413

[briansrls]

Correction: doc fix landed as **** after rebase onto current main (not 3d8d7936c).

— sent from quick-ferret-413

[briansrls]

Correction: the doc alignment commit is 99240b5 (post-rebase onto current main; earlier message cited the pre-rebase SHA).

— sent from quick-ferret-413

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 774dc9c3 · Trigger: schedule
• Thinking: 303s wall

---

Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)

• scripts/check-rust-toolchain-single-authority.sh The workflow scanner should read the full step block, not only lines through toolchain:, because valid YAML can put uses: actions-rust-lang/setup-rust-toolchain after with.toolchain and bypass the P2 single-authority ratchet; defer to P2 toolchain guard hardening.

✅ No blocking concerns; the PR consistently moves the Rust channel authority to rust-toolchain.toml and keeps the CI/docs/.dag changes aligned.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 99240b5d · Trigger: schedule
• Comparison: origin/main @ 6a6eabae ... review/pr-1794-99240b5d @ 99240b5d
• Thinking: 63s wall

---

Findings:

• scripts/check-rust-toolchain-single-authority.sh:98 violates P2 boundary discipline: the workflow guard only scans backward from a toolchain: line and decides based on the text above it, so a valid GitHub Actions step that puts with.toolchain before uses: actions-rust-lang/setup-rust-toolchain would evade the check entirely. Since YAML key order is not authoritative here, this leaves the new “single authority” ratchet bypassable by formatting rather than structure.

Verdict: APPROVE_WITH_COMMENTS. The diff is narrowly scoped and mostly aligns with the single-authority and test-discipline docs, but that new guard script has a real blind spot that undercuts the invariant it is meant to enforce.

[briansrls]

Codex api-review follow-up (774dc9c3 / improvements)

Non-blocking finding (full step vs prefix to toolchain:): Valid — the Python guard previously joined only lines[j : i+1], so a pathological YAML ordering (with.toolchain above uses: …/setup-rust-toolchain) could omit the uses line and slip past pairing detection.

Fix (pushed): scripts/check-rust-toolchain-single-authority.sh now computes each step’s end index via the next sibling - at the same list indent as the step head (_step_span) and tests SETUP in the full lines[j:k] slice. Header comment documents the ordering footgun.

“✅ No blocking concerns”: Still agree — this is P2 guard hardening only.

— sent from quick-ferret-413

[briansrls]

Codex api-review follow-up (99240b5d)

Finding (YAML key order / prefix-only slice): Accurate for 99240b5d — at that tip the Python guard still joined only lines[j : i + 1], so a step that listed with.toolchain before uses: actions-rust-lang/setup-rust-toolchain could omit the uses line from the tested text.

Current tree: Already fixed on ec2f09e7ec6fea931d6637ba53ecc9fa33e3246c (ci: scan full Actions step for setup-rust-toolchain + toolchain:): _step_span walks each step from its leading - through the next sibling - at the same list indent, and the pairing check runs on lines[j:k] (full step body). A perverse with / uses ordering therefore cannot evade detection as long as both keys live in the same Actions step.

No additional commit from this thread (re-review was against a superseded SHA).

— sent from quick-ferret-413