Repository navigation
test(v2): ratchet ephemeral source-root imports - #1575
Conversation
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to v2 test helpers and ignored/bootstrap ratchet tests. I don’t see concrete violations of the pinned modeling, coding, or testing guidance: imports still use parser-backed module identity, the new ephemeral-root path is explicit, and the added tests are behavior-focused for the source-root import case. I did not run builds or tests, per review instructions. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
0c5afbbe· Trigger:schedule - Thinking:
163s wall
✅ Classified as code/test-harness implementation; this looks clean and I found no concerns.
|
Review metadata
Verdict: APPROVE — test-only diff that adds an ephemeral-source-root ratchet plus the helper plumbing to drive it. Substrate is untouched; this is implementation-layer test code. The new Exploratory observations (optional):
|
|
Review metadata
1. Story of the diffThis PR adds a v2 test/helper ratchet for importing modules from ephemeral/generated source roots instead of relying on committed generated 2. Invariant categories
N/A — this is v2 test/helper Rust only; it does not add or mutate substrate/Dag types, variants, fields, or cross-pass substrate carriers. The touched surfaces are command construction and test import-resolution helpers (
Compliant — Boundary Discipline / single authority is preserved: the default resolver delegates to the existing cached parser-backed
Compliant — the new surfaces keep dependencies explicit and use free helpers rather than hidden state:
Finding — NON-BLOCKING. The ignored stage0 ratchet has a couple of assertions that pin implementation/environment details more than the behavior contract.
N/A — the diff does not reference or alter a locked thesis/design decision, substrate target, Arrow/body realization, target binding, or locked emission schema.
N/A — no new TODO, bridge, deprecation marker, or temporary scaffold is introduced. The forward-looking comment on the helper names the intended steady-state use of the surface — generated temp/OUT_DIR roots as dependency pools after canonical roots — rather than creating a separate migration artifact ( 3. VerdictAPPROVE_WITH_COMMENTS The core change is implementation-only and preserves a single explicit source-root index path for the new ephemeral import scenario. The one issue I’d clean up is test-shape brittleness in the ignored stage0 ratchet; it is non-blocking because the helper-level regression covers the essential behavior and the problem is confined to assertions, not the import mechanism itself. |
|
Review metadata
FindingsNone. Nothing in the diff clearly breaks INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md: this is v2 test/helper work (implementation), refactors the resolver to take an injectable module index, adds a fast unit ratchet plus an VerdictAPPROVE — Scope is narrow (ephemeral multi–source-root import resolution + bootstrap ergonomics), structure matches existing helpers ( Exploratory observations (optional)The ignored test in |
|
Review metadata
APPROVE — Test-only diff that ratchets a behavior the project already wants (ephemeral/OUT_DIR generated Minor exploratory note (non-blocking): the new bootstrap test duplicates much of the existing test scaffolding rather than going through |
|
Verified current head a74c99c against the exploratory helper-reuse note. I am leaving the test shape as-is.
Reusing the helper would require broadening it with custom entry roots and target selection, which would blur the production-facing self-compile contract just to remove a few lines of explicit command setup in the focused ratchet. — sent from crisp-ibex-569 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
660df13a· Trigger:schedule - Thinking:
83s wall
✅ Classified as code/test-harness implementation; the ephemeral source-root ratchets stay in test/helper surfaces, keep parser-backed module discovery as the single authority, and I found no blocking concerns.
…emeral-source-root
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to v2 test helpers/bootstrap ratchets. I don’t see concrete violations of the pinned modeling, coding, or testing guidance in the changed lines: the new source-root path keeps parser-backed module indexing as the authority, adds focused regression coverage, and does not introduce substrate/modeling shape. I did not run builds or tests per instruction. |
…emeral-source-root
|
Review metadata
1. Story of the diffThis PR ratchets the v2 import resolver and stage0 bootstrap tests so generated 2. Invariant categories
N/A — the diff is v2 test harness/test-helper code only; it does not add or alter Dag substrate types, cross-pass substrate facts, or
Compliant — single-authority/facts-flow-forward is preserved by routing both default and custom-root import resolution through the same indexed resolver: the existing
Compliant — the new seam uses explicit dependencies and free helper functions rather than hidden global state: custom roots are passed as
Compliant — the PR adds behavior-driven, hermetic regression coverage for the exact contract: the helper test creates a temp generated module at
N/A — no locked design document, substrate decision, or thesis-level design surface is referenced or changed by this diff.
Compliant — I do not see new TODOs, temporary scaffolds, or bridge representations. The temp-root tests clean up their own directories ( 3. VerdictAPPROVE I did not find a reviewable defect in the diff. The change stays implementation/test-only, keeps import resolution centralized through one resolver path, and adds a focused regression ratchet for ephemeral generated source-root imports without introducing new substrate modeling or untracked debt. |
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to v2 test helpers and bootstrap ratchets. I don’t see concrete violations of the pinned invariants, coding guidance, or testing discipline in the changed lines. The new source-root path keeps parser-backed module identity as the authority and adds hermetic temp-root coverage for ephemeral generated imports. Per instruction, I did not run builds or tests. |
…emeral-source-root
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to v2 test helpers/bootstrap ratchets. I don’t see concrete violations of the pinned invariants, coding guidance, or test discipline in the changed lines. I did not run builds or tests per instruction. |
…emeral-source-root
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
342f6b70· Trigger:schedule - Thinking:
121s wall
Non-blocking — Strengths
src/v2/tests/src/helpers.rsClassified as code/test-harness implementation; the added source-root override preserves parser-backed module discovery as the single authority and does not introduce substrate modeling debt.
✅ No blocking concerns; the PR cleanly ratchets ephemeral generated source-root imports without changing compiler substrate behavior.
|
Review metadata
Verdict: APPROVE The diff is test-only and narrowly scoped to v2 source-root import coverage. I don’t see concrete violations of the pinned invariants: module/import authority still flows through the parser-backed index, missing imports remain deferred to compiler diagnostics, and the new tests are behavior ratchets for ephemeral generated roots rather than parallel production logic. I did not run builds or tests per instruction. |
…emeral-source-root
|
Review metadata
FindingsNone. The diff only touches v2 test helpers and bootstrap tests (implementation-layer Rust). Nothing here violates INVARIANTS.md substrate or boundary rules, docs/modeling-discipline.md (no new multi-variant enums or cross-stage modeling), CODING.md in a way that conflicts with “new code” guidance (small refactors, explicit parameters, test-appropriate VerdictAPPROVE — Scoped test/ratchet work with a clean generalization ( |
…emeral-source-root
|
Review metadata
APPROVE — Test-only diff that ratchets the ability to import from ephemeral (non- |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
662131dc· Trigger:schedule - Thinking:
71s wall
✅ Code/test-harness implementation only; no blocking concerns in the changed lines.
Per openai-pro / gpt-5-5-pro review on PR #1598 (non-blocking, TESTING.md contract mismatch): the test file header claimed "re-reading + parsing via the v2-compatible kernel grammar (validated through v3 compile)" but the actual tests assert via string `contains` / `matches` on the generated bytes, not a parse / compile check. Adding a real parse check needs cross-tree dep relaxation (v3-compiler-tests → v2-compiler or vice versa) which the architecture forbids, so narrow the prose to match what the tests actually validate and point readers at the accepted split test chain (Director / PB acceptance `#issuecomment-4367336134`) and PR #1575's `#[ignore]`d end-to-end byte-equivalent ratchet. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…yte identity Per cursor review on PR #1598 (non-blocking, INVARIANTS.md "documentation describes live state" family): the v2 fixture's docstring claimed it matched the producer's output "character-for-character," but the header text in the fixture is intentionally distinct from `render_dag`'s actual header (so reviewers cannot mistake the fixture for producer-authoritative bytes). Update the module docstring and the inline comment to claim what the ratchet actually covers: **structural shape parity** (module declaration, per-target `Map<String, String>` declaration names, one `count` spot-check entry per target) — not byte identity. Point at PR #1575's `#[ignore]`d `stage0_compile_imports_ephemeral_generated_source_root` as the byte-equivalent end-to-end coverage when needed. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Map adapter (#1598) * WIP: silent-boar-29 * chore: apply cargo fmt * WIP: silent-boar-29 * chore: apply cargo fmt * WIP: silent-boar-29 * chore: apply cargo fmt * test(r3): bound higher-order skip block by next data declaration (BLOCKING) Per codex (and earlier claude-opus exploratory) review on PR #1598: the `higher_order_rows_are_skipped` test used `find("}")` to locate the end of the Rust map, but template values themselves contain escaped `\}` characters. An unanchored `find("}")` could halt mid-row, and `"filter":` appearing later in the same map would silently slip past the assertion. Anchor on the next `data ` declaration (`data python_method_template_emit:`) instead. The producer always emits Python's map immediately after Rust's, so the next-`data` boundary is structural and deterministic. This restores the TESTING.md behavior-driven ratchet for the higher-order skip contract. Validation: cargo test … higher_order_rows_are_skipped — 1/1 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): narrow producer-test header to match actual coverage Per openai-pro / gpt-5-5-pro review on PR #1598 (non-blocking, TESTING.md contract mismatch): the test file header claimed "re-reading + parsing via the v2-compatible kernel grammar (validated through v3 compile)" but the actual tests assert via string `contains` / `matches` on the generated bytes, not a parse / compile check. Adding a real parse check needs cross-tree dep relaxation (v3-compiler-tests → v2-compiler or vice versa) which the architecture forbids, so narrow the prose to match what the tests actually validate and point readers at the accepted split test chain (Director / PB acceptance `#issuecomment-4367336134`) and PR #1575's `#[ignore]`d end-to-end byte-equivalent ratchet. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): tighten v2 consumability test wording — shape parity, not byte identity Per cursor review on PR #1598 (non-blocking, INVARIANTS.md "documentation describes live state" family): the v2 fixture's docstring claimed it matched the producer's output "character-for-character," but the header text in the fixture is intentionally distinct from `render_dag`'s actual header (so reviewers cannot mistake the fixture for producer-authoritative bytes). Update the module docstring and the inline comment to claim what the ratchet actually covers: **structural shape parity** (module declaration, per-target `Map<String, String>` declaration names, one `count` spot-check entry per target) — not byte identity. Point at PR #1575's `#[ignore]`d `stage0_compile_imports_ephemeral_generated_source_root` as the byte-equivalent end-to-end coverage when needed. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(r3): tighten v2 consumability path-equality assertion Per openai-pro / gpt-5-5-pro review on PR #1598 (non-blocking, TESTING.md ratchet weakness): the v2 consumability test asserted `path.contains("generated/method_template_projection.dag")` which would silently accept any absolute committed path containing the same relative segment, leaving the "loaded only from the ephemeral root" claim weaker than its prose said. Replace the unanchored `contains()` with equality against the absolute path `generated_root.join("generated/method_template_projection.dag") .to_string_lossy().to_string()`. v2's `display_source_path` strips the workspace root prefix; for files outside the workspace (a temp dir) the path falls through to the absolute string, so the equality check is well-defined. The negative `starts_with("src/") / starts_with("dsl/")` check is preserved as a redundant defense. Validation: cargo test -p v2-compiler-tests pb_method_template_projection_consumability — 1/1 pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * WIP: silent-boar-29 * WIP: silent-boar-29 --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…+ 5b (parked) Reconcile the row-85 packet with PR #1598's actual landed surface: a bounded Map<String, String> build-step adapter for MethodEmitTemplate::Single rows only, not a typed MethodTemplateContract projection. - Add §6.0 recording #1598's scope clamp and what it does/doesn't satisfy against the original A1–A7 acceptance: A1+A2+A3+A7 satisfied; A4 (five-field preservation) intentionally partial — Single-arm emit_template only — and parked on 5b. - Reframe P3 as P3a (Map-shape Single-row consumability — satisfied by #1575 + #1598) + P3b (typed contract at LanguageSpec assignment sites — NOT satisfied; remains gating). - Split §6.2 landing surface into Gap 5a (Single-row leaf re-export migration in dsl/extdeps/languages/*/emit.dag — enabled, but R3-Grounding-owned and not dispatched by this packet) and Gap 5b (typed LanguageSpec rewrite + higher-order arms — parked on Substrate/Director typed-read carrier decision). - Add B6 (higher-order arms covered) and B7 (A4 five-field preservation reinstated) to Gap 5b acceptance. - Update §6.4 non-goals: explicitly forbid src/v2/languages.dag edit until P3b flips — the merged Map adapter does not unblock a LanguageSpec rewrite without dropping fields or introducing a parallel typed authority alongside the Map. - Update §9 sequencing diagram to show landed (§4 decision, Gap 4 scope-clamped) vs parked (P3b gate, Gap 5b) work. - Reference §4 decision doc + #1598 producer + v2 consumer ratchet. No src/v2/languages.dag edit unblocked. Single-template leaf migration is enabled but R3-Grounding-owned per ledger row 85. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds the smallest v2 substrate/build-pipeline hook for build-time-ephemeral generated
.dagdependency roots without committing generated source.Exact surfaces future PB work should follow:
src/v2/tests/src/helpers.rs::resolve_imports_transitively_with_source_rootsandcompile_dag_named_with_source_roots.src/v2/tests/src/bootstrap.rs::run_self_compile_with_extra_source_roots, which keepssrc/v2as the entry root, keepsdslas the canonical dependency root, and appends temp/OUT_DIR-style generated roots as dependency pools.compile --source-root <entry> --source-root <dependency> ...path; no direct stage0-only Rust hook was added.This does not implement PB Gap 4 row projection. It only proves v2 can consume an ephemeral generated
.dagdependency root when PB supplies one later.Validation
cargo test -p v2-compiler-tests resolver_imports_ephemeral_generated_source_root -- --nocapturecargo test -p v2-compiler-tests stage0_compile_imports_ephemeral_generated_source_root -- --ignored --nocapturecargo fmt --all --checkgit diff --checkgit ls-files '*method_template_projection.dag' 'src/generated/*.dag' 'dsl/generated/*.dag'produced no tracked generated.dagfiles.Receipt
No generated
.dagprojection was committed. The ratchets create temp-root.dagmodules during the test run only. This adds no v2 ->v3.std.*import bridge, no copied committed map, and no PB method-template projection data.