Repository navigation
test generalization - #638
Conversation
|
Review: APPROVE — solid cementing infrastructure, ready to promote from draft. This is the right implementation of the register's discipline rule #3 generalization. Scope is exactly what we discussed: build the mechanism, not blanket-mandate test coverage. Four design choices that get it right: What's load-bearingTESTING.md section is precise about the trigger condition. The "Where this applies" paragraph names the two cases (v2 counterpart exists / v3-native but COMPLETE) and the "Anti-scope" paragraph prevents scope creep. Explicitly: "This section is not a mandate to prove full lens equivalence for every
Register rule #6 closes the loop from register → test discipline. The register (which lands before cementing tests per ordering) now names the test discipline as its enforcement arm. Bidirectional: register rule #3 asks for cementing tests; rule #6 says they live in TESTING.md +
Why promoting provenance's cementing test anyway is good designProvenance is v3-native (N/A counterpart), so it doesn't strictly trigger the ratchet. But the worker cemented it anyway. This is exactly the right interpretation of the TESTING.md rule "No v2 counterpart but register marks COMPLETE: pin the lens's published behavioral contract on minimal constructed Dag shapes." It demonstrates two things in one artifact:
The Two minor observations (non-asks, worth noting)
VerdictReady to promote from draft. Once it's ready-for-review the CI run (now draft-aware thanks to #626) will pick it up. The mechanism is the right shape: minimal enforcement, maximal clarity about when it fires, example test showing the pattern. When the first v2-parity-COMPLETE claim comes through (complexity or cost after substrate-carrier work lands), the author has a clean template to follow. Suggest: before promoting, double-check that 🤖 Director review |
|
Review metadata
Findings
Verdict |
|
Review metadata
The monitor timed out after the cargo test had already completed and I confirmed the results. No re-arm needed — review is complete. Final verdict: APPROVE_WITH_COMMENTS. The two new tests pass, the diff is a narrowly-scoped process/discipline change (cementing-test section in |
CI fix — both failures are the same root causeThe 3 failing tests all trace to a single fact: your header-comment addition to Specific fixes (order matters; each captures a different snapshot)1. Parse-corpus manifest (fixes The only diff between
Everything else is byte-identical. Refresh command is in the failure output's first line: This runs the ignored 2. Bootstrap snapshots (fixes both The generated bootstrap snapshots also bake in That should update the checked-in After the regenCommit the refreshed manifest + regenerated bootstrap snapshots in the same commit (call it something like Small meta-observation (non-ask)Ironic context: your PR implements Band-C cementing-test discipline, and the failures are caused by parallel-authority ratchets (3 places track 🤖 Director review |
|
Review metadata
1. Story of the diffThis PR is trying to formalize a new “cementing test” lane for Band-C lens-subsumption claims. The docs changes in The executable part of that policy is wired in at 2. Invariant categories
N/A — this diff is docs/tests only.
Finding (BLOCKING) — single-authority metadata / facts flow forward:
Compliant — the new helper logic is small, free-function style, and explicit about dependencies:
Finding (BLOCKING) — regression tests should enforce the claimed behavior, not just an inert artifact: the comment at
N/A — no locked thesis/design decision is altered here; the diff stays in testing/process docs plus test wiring.
N/A — no explicit TODO, bridge, deprecation, or temporary scaffold is introduced as such. The issue is not missing debt bookkeeping; it is that the landed enforcement mechanism is weaker than the process it claims to ratchet. 3. VerdictREQUEST_CHANGES The direction is good, and most of the diff is clean implementation-only policy/test work. But the load-bearing new mechanism does not yet enforce what the prose says it enforces: it shadows the authoritative inventory in a manual slice and only verifies on-disk file presence, not that the required cementing module is actually registered and executed. |
|
Review metadata
Findings
Verdict |
|
Review metadata
Review complete — already posted above. The original background |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
d609bbc5· Trigger:schedule - Thinking:
399s wall
BLOCKING (1)
Root Cause
src/v3/compiler/tests/integration/cementing/cementing_lens_registry_dispatch_test.rsBehavioral completeness for provenance is defined over the full Origin carrier, but the new ratchet asserts only a lossy projection → match on Origin::{Source,Computed}{_0} and compare those payloads to the expected producer node ids from the compiled fixtures.
| use v3_compiler::dag::Behavior; | ||
| use v3_compiler::lens_provenance::{origin_of, Origin}; | ||
|
|
||
| fn origin_label(origin: &Origin) -> &'static str { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
Findings
VerdictAPPROVE. Small, process-oriented PR that hardens the Band-C cementing discipline without touching substrate. It (a) documents the rule in TESTING.md and the register, (b) lands a ratchet test module that's self-wired and checks both on-disk presence and |
Cross-check CEMENTING_MODULES_FOR_V2_COMPLETE_CLAIMS against docs/v3-lens-capability-register.md (COMPLETE + non-v3-native v2) joined with regen.dag LensRegistryEntry lens_file basenames. Addresses api-review REQUEST_CHANGES on manual slice drift; wiring checks for #[path] remain. Made-with: Cursor
|
Review metadata
Findings
Verdict |
Replace string-label collapse with structural checks against the value port's producing Behavior so wrong producer ids fail. Addresses PR #638 inline review (carrier shape / API-level contract). Made-with: Cursor
Re-export origin_for_behavior and assert full enum equality (all variants) vs origin_of on compile fixtures so the harness tracks provenance.dag, not a duplicated projection. Addresses PR #638 blocking review on full Origin carrier contract. Made-with: Cursor
TESTING.md naming: the test uses compile_to_dag, not minimal Dag ports. Made-with: Cursor
|
Review metadata
FindingsNone blocking. A few observations below. VerdictAPPROVE — This PR adds a Band-C cementing discipline (TESTING.md section + capability-register hook + in-tree ratchet test) and wires one supplementary cementing test for The ratchet is architecturally clean: Exploratory observations (non-blocking)
|
|
Review metadata
Findings
Verdict The process/docs ratchet is directionally clean, and the new tests pass ( |
Codex APPROVE_WITH_COMMENTS: origin_for_behavior is same artifact as origin_of; document split with lib.rs minimal-Dag mapping pins. Made-with: Cursor
|
Review metadata
ReviewThis is a test-infrastructure / documentation PR. Summary of what's in scope:
What I looked for and did not find
Exploratory observation (non-blocking)
VerdictAPPROVE — diff is narrowly scoped to test/doc infrastructure, the new ratchet is derived from a single authority rather than hand-maintained, and the known seam-vs-oracle caveat is documented at both the module and function level. No invariant violations grounded in the diff. |
|
Review metadata
Findings
Verdict |
Remove public re-export; duplicate the five-way Behavior→Origin table in the integration test with an explicit regen-sync note. Addresses PR #638 blocking review (API surface / TESTING.md boundary). Made-with: Cursor
|
Review metadata
Verdict: APPROVE. Diff is narrowly scoped to the new cementing-test ratchet and its documentation, and I don’t see a concrete violation of |
Addresses exploratory review feedback: the register ratchet treats markdown as data; lock \| escaping and column indices with a focused unit test. Made-with: Cursor
|
Review metadata
ReviewThis PR adds scaffolding for Band-C cementing tests (TESTING.md §Cementing tests, a dispatch ratchet in Things I checked against the rubric:
No findings. VerdictAPPROVE — scaffolding-only diff, no substrate changes, ratchet is correctly grounded in the register + Exploratory observations (optional)
|
|
Review metadata
Findings
Verdict The diff is otherwise clean and the new ratchets are directionally sound. I also ran |
Treat v2 counterpart cell N/A like None (v3-native) so COMPLETE rows without a real v2 path do not trip the v2 parity module ratchet. Adds register_row_has_real_v2_counterpart + unit pin. Made-with: Cursor
|
Review metadata
Review of PR #638 — test generalizationThis PR wires up a Band-C "cementing test" dispatch framework: it adds a section to Notes (no findings)
VerdictAPPROVE. The PR is disciplined test infrastructure: it adds enforcement without widening substrate or public API, the ratchet derives from existing authorities rather than creating a third, the one new crate-private helper ( |
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to test/docs ratchets, and I don’t see a concrete violation of |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
eb2ff4a5· Trigger:schedule - Thinking:
258s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/cementing/cementing_lens_registry_dispatch_test.rsThe new escalation slice is derived from the capability register plusLensRegistryEntryrows, so the cementing list stays a projection rather than becoming a competing authority.src/v3/compiler/tests/integration/sg6_hand_authored_census_test.rsOwning the integration-module wiring ratchet in SG-6 matches the layer that can actually observetests/integration.rslinkage and closes the prior self-disabling hole cleanly.
✅ The previously flagged gaps are fixed and I do not see a new thesis- or invariant-level concern in this test generalization pass.
|
Review metadata
1. Story of the diffThis PR turns “cementing tests” from a loose review expectation into an explicit Band‑C ratchet for lens-subsumption claims. The docs now say that when the capability register marks a lens 2. Invariant categories
N/A — no substrate semantics change.
Compliant — single-authority metadata / facts flow forward is handled correctly here: the new file explicitly says the local slice is not a second authority (
Compliant — the new logic is split into small free functions with explicit inputs/outputs (
Finding (BLOCKING) —
N/A — no locked thesis/design surface is altered; the new prose explicitly narrows itself to Band-C cementing dispatch and says it does not require new substrate accessors or full lens equivalence (
Compliant — the only intentional duplication is the local provenance 3. VerdictREQUEST_CHANGES The projection/ratchet design is thoughtful, and the provenance seam test plus census wiring show real discipline. But the core guarantee this PR is adding — “the cementing module is wired into the integration harness” — is currently just a text search over |
Raw substring matches on tests/integration.rs could false-green when #[path] / mod declarations were only present in // comments. Scan only active lines (trimmed line does not start with //) and require an active mod line for each cementing stem. Shared helper lives in tests/integration/common; pin with a small cementing-module test. Made-with: Cursor
|
Review metadata
Review of PR #638: test generalizationThis PR establishes Band-C cementing discipline: a TESTING.md section, a dispatch ratchet wired to FindingsNothing blocking. Some observations:
The bootstrap-generated diffs are mechanical span shifts from the new VerdictAPPROVE — the diff is tightly scoped to process/test infrastructure, the |
|
Review metadata
Findings
Verdict: APPROVE_WITH_COMMENTS. The diff is narrowly scoped and otherwise lines up with the new TESTING discipline; I didn’t find substrate/modeling violations in the actual PR delta. I also spot-checked the new cementing/SG-6 tests locally and they pass. |
Replace line-prefix heuristic with a small lexer (//, nested /* */, and " strings) so /* #[path] */ cannot satisfy Band-C wiring ratchets. Pins block-comment and string-literal behavior in cementing tests. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
7255543c· Trigger:schedule - Thinking:
348s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/common/mod.rsintegration_rs_active_line_contains follows the testing-invariant source-audit exception by ignoring commented and string-embedded tombstones rather than treating raw text presence as live wiring.
✅ The new Band-C docs and ratchets stay aligned with the thesis’s single-authority and testing discipline, and I do not see a new blocking concern in the added lines.
|
Review metadata
Reviewing the diff. The PR adds testing discipline for Band-C "cementing" claims: a TESTING.md section, a register/regen.dag comment, and a new FindingsNo violations worth flagging. The scope is test governance and docs; there's no substrate change, and the regen span updates in VerdictAPPROVE — This is a clean governance ratchet. Exploratory observations (optional)
|
|
Review metadata
Findings
Verdict |
Addresses exploratory review: if tests/integration.rs gains r#" / b" / br" literals the naive string state would desync silently. Detect openers in Code and panic with an explicit extend-scanner message; pin with should_panic test. Made-with: Cursor
Band-C dispatch must not pass on unrelated #[path] + mod pairs. Add integration_rs_cementing_path_attr_binds_mod_stem (code-only path match + mod <stem>; tail), refactor substring scan to collect positions, and pin mismatched-mod / stem_extra cases in cementing tests. Made-with: Cursor
|
Review metadata
Findings Nothing violating INVARIANTS.md, MODELING.md, CODING.md, or TESTING.md. The PR is test + docs infrastructure; the generated-code diff ( Verdict: APPROVE — Exploratory observations (non-blocking)
|
|
Review metadata
Verdict: APPROVE. Diff is narrowly scoped to test/doc ratchets and regen snapshots, and I don’t see a concrete violation of |
…tail Made-with: Cursor
|
Review metadata
FindingsNothing in this diff concretely violates INVARIANTS.md, MODELING.md, CODING.md, or TESTING.md. A couple of observations worth recording:
VerdictAPPROVE — the diff builds out the Band-C cementing dispatch in TESTING.md + the register + regen.dag + a wired ratchet test, plus one real cementing pin for |
|
Review metadata
APPROVE Diff is narrowly scoped to docs/test ratchets, and I didn’t find a concrete violation of |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
a855ac6a· Trigger:schedule - Thinking:
277s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/sg6_hand_authored_census_test.rsThe new SG-6 ratchet puts the dispatch-module wiring check at the integration-harness boundary, which matches TESTING.md's requirement that Band-C dispatch be enforced structurally rather than by convention.src/v3/compiler/tests/integration/cementing/cementing_lens_registry_dispatch_test.rscementing_escalation_slice_matches_capability_registerkeepsCEMENTING_MODULES_FOR_V2_COMPLETE_CLAIMSas a projection ofdocs/v3-lens-capability-register.mdplus theLensRegistryEntryregistry, not a second authority.
✅ No blocking concerns in the added lines; the Band-C ratchets now line up the register, regen registry, filesystem, and integration harness cleanly.
|
Director convergence review: READY TO MERGE. Cementing-test discipline generalization (847+/26-, claude + codex clean APPROVE on latest SHA). CI clean across all gates. Extensive iteration visible in the dashboard history — worker went through multiple feedback rounds, passing CI + cementing-ratchet + Dag-builder-exposure concerns along the way. The final shape is solid:
This is load-bearing infra for every future lens work. When the first substrate-carrier-port lands and a lens promotes to BEHAVIORALLY COMPLETE with v2 parity, the author has a clean template to follow and a ratchet that enforces cementing. Clean convergence. Merge. 🤖 Director review |
Opened from session-dashboard for session
valiant-swift-774.