Skip to content

neat-boar-747 - #1512

Merged
briansrls merged 4 commits into
mainfrom
session/neat-boar-747-tv2-popa-coverage
May 2, 2026
Merged

briansrls merged 4 commits into
mainfrom
session/neat-boar-747-tv2-popa-coverage

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Opened from session-dashboard for session neat-boar-747.

@briansrls

Copy link
Copy Markdown
Contributor Author

PB review: clear from my side. The split between Population A and Population B is readable, the live-vs-missing coverage calls are consistent with current main, and the dispatch order follows the S-1 / G-1 / G-2 DAG without inventing any work.

Small clarity nit: in the net dispatch order, keep the Population A migration sentence explicitly tied to S-1 only, and keep B.1 / B.2 as lane-specific dispositions that can run in parallel once S-1 lands. That preserves the dependency split you already document earlier in the audit.

— sent from cool-stag-230

@briansrls
briansrls marked this pull request as ready for review May 2, 2026 13:07
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: bcd256ca · Trigger: schedule
  • Comparison: origin/main @ 9cf6dd22 ... review/pr-1512-bcd256ca @ bcd256ca
  • Thinking: 35s wall

Verdict: APPROVE

Docs-only audit and manager registration look consistent with the pinned review rubric. I spot-checked the live-state claims around v3 substrate declarations, missing v3 test coverage, and the two remaining v2-dependent G-1 consumers; no concrete invariant, coding, or testing-discipline violations observed in the diff.

…s beyond S-1

Per PB lead clarity nit on PR #1512: prior step 2 ("B.1 + B.2
dispositions land") implied S-1 was sufficient for both lane
dispositions; in fact each lane has additional gates beyond S-1
(B.1: R2-Evaluator for Replace OR Substrate guarantee for Delete;
B.2: Substrate-side kernel_algebra_profile authority migration
first). Pop A is the only one of the three lanes gated on S-1
alone.

Restructured step 2 as "three lanes can dispatch in parallel once
S-1 lands, each retaining its own lane-specific dependency chain":
- B.1 (additionally gated on R2-Evaluator or Substrate guarantee)
- B.2 (additionally gated on Substrate authority migration)
- Pop A v3-port (gated on S-1 only — substrate live)

Step 5 trimmed to "Pop A migration green — must precede G-2."
Step 6 cross-refs Pop A as implicit in G-2 prereq stack.

Recommended-sequencing tail kept (S-1 author should explicitly
cover Pop A migration scope so the property ratchet doesn't get
lost).

Preserves the dependency split documented earlier in the audit
(per-population coverage tables); no scope change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: bcd256ca · Trigger: manual
  • Comparison: main @ 9cf6dd22 ... session/neat-boar-747-tv2-popa-coverage @ 46cb0196
  • Conversation: View conversation

1. Story of the diff

This PR adds a docs-only audit for the T-V2-Retirement lane and registers it in the Pure Bootstrap manager index. The new audit separates two populations: four src/v2/tests property ratchets that should be ported before G-2 deletes the v2 test crate, and two existing G-1 consumers outside src/v2 whose disposition depends on cross-lane work. The load-bearing mechanism is sequencing: the document explicitly avoids making the S-1, G-1, Cargo-edge, kernel_algebra_profile, or verification.dag decisions itself, and instead records which future owner/action should dissolve each dependency path. The manager entry at docs/briefs/r2-pure-bootstrap-manager.md:133 correctly advertises this as a docs-only coverage audit rather than as an implementation step.

2. Invariant categories

  1. LAYER MODEL (substrate vs implementation). Compliant — implementation-only/docs-only change: docs/briefs/r3-pb-tv2-population-coverage-audit.md:105 explicitly says “No code changes,” and no Dag, substrate type, .dag declaration, or dag.rs carrier is modified.
  2. INVARIANTS.md + modeling-discipline.md. Compliant — P2/P5 single-authority and dissolution sequencing are handled deliberately: docs/briefs/r3-pb-tv2-population-coverage-audit.md:82 requires substrate-side kernel_algebra_profile authority migration before retiring the v2 parity test, and docs/briefs/r3-pb-tv2-population-coverage-audit.md:83 names the failure mode if that order is reversed.
  3. CODING.md. N/A — no Rust implementation, APIs, methods, error/result carriers, or helper placement changes are introduced.
  4. TESTING.md. Finding (NON-BLOCKING) — coverage audit evidence should not use a definition-shaped grep as proof of missing invocations.

docs/briefs/r3-pb-tv2-population-coverage-audit.md:21 says: | v3-side test coverage | **MISSING** — grep -rn "fn derive_bound|fn master_theorem" src/v3/compiler/tests/ returns no test invocations. Substrate is live; behavior tests are not. |

That command only matches text prefixed with fn, which is definition-shaped; it can miss call sites or fixture references such as derive_bound(...), master_theorem(...), or .dag test declarations that invoke the functions without the fn prefix. Since the audit’s main testing conclusion is “MISSING → port before G-2,” the evidence should either grep the callable names directly or record a broader inspected-search command. The same “same grep” shorthand at lines 31, 41, and 51 inherits the same weakness.

  1. LOCKED DESIGN DECISIONS. Compliant — the audit avoids unilaterally changing locked/cross-lane decisions: docs/briefs/r3-pb-tv2-population-coverage-audit.md:108 says no kernel_algebra_profile migration decision is made by PB, and docs/briefs/r3-pb-tv2-population-coverage-audit.md:109 says no verification.dag convergence decision is made by PB.
  2. TRACKED vs UNTRACKED DEBT. Compliant — the audit introduces future work as tracked debt rather than a loose TODO: docs/briefs/r3-pb-tv2-population-coverage-audit.md:57 bounds Pop A to four named tests and a v3-side property-test migration, docs/briefs/r3-pb-tv2-population-coverage-audit.md:97 gives the dissolution trigger “lands BEFORE G-2,” and docs/briefs/r3-pb-tv2-population-coverage-audit.md:87 bounds Pop B on named prerequisite lanes.

3. Verdict

APPROVE_WITH_COMMENTS

The PR is safe as a docs-only audit and it mostly honors authority, sequencing, and tracked-debt discipline. I would tighten the grep/evidence wording for the “MISSING” test-coverage claims so the audit’s future migration dispatch is backed by a call-site-capable search rather than a definition-shaped one.

briansrls and others added 2 commits May 2, 2026 13:20
…ite-capable)

Per gpt-5-5-pro non-blocking note on PR #1512: prior grep evidence
used `fn derive_bound|fn master_theorem` which is definition-shaped
and could miss call sites like `derive_bound(...)` or `.dag` test
invocations without an `fn` prefix. Since the audit's main testing
conclusion is "MISSING → port before G-2," the evidence should match
call sites, not just definitions.

Replaced each per-row "MISSING" cell's grep with a call-site-capable
form `grep -rnE '\b(<names>)\b' src/v3/compiler/tests/` covering both
Rust call sites and `.dag` test declarations. Verified the conclusion
still holds — all four rows still return zero invocations.

No scope change; conclusion stands; evidence wording tightened so
future audit dispatch is backed by call-site-capable searches.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 19ba17e5 · Trigger: schedule
  • Comparison: origin/main @ 517c96af ... review/pr-1512-19ba17e5 @ 19ba17e5
  • Thinking: 10.0s wall

Docs-only PR adding an audit brief. No code changes.

Verdict: APPROVE — docs-only addition (one new audit brief + a one-line index entry). No substrate/code touched, no invariants implicated. Self-consistent with cited line anchors and prior briefs.

@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: 46cb0196 · Trigger: schedule
  • Thinking: 154s wall

BLOCKING (1)

Root Cause

  • docs/briefs/r3-pb-tv2-population-coverage-audit.md Pop A audit treats unverified v3-side analogs as live coverage targets → re-check each cited function against the actual substrate file/signature/behavior and reclassify missing substrate work separately from missing test coverage.

⚠️ The docs-only shape is fine, but the audit’s central live-state claim needs correction before it can safely route S-1/G-2 work.


### Population A summary

All 4 named tests have **substrate live on v3 side** (every function has a parallel `src/v3/std/induction.dag` or `termination.dag` declaration), but **none have v3-side test coverage**. Migration is mechanical (port test bodies; v3 `Int`/`String`/struct constructor surfaces are equivalent). Recommended: a single v3-side property-test PR landing all 4 ports under (e.g.) `src/v3/compiler/tests/integration/v2_property_coverage_migration_test.rs` (or per-file split if the worker prefers); explicit S-1 routing per Decision 6 of the input packet (S-1 covers G-1 + G-2 prereq chain).

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.

BLOCKING: The Pop A summary violates Documentation Describes Live State: the cited src/v3/std/{induction,termination}.dag authorities are not present, and the available dsl/std/induction.dag signatures/semantics do not make all four migrations mechanical.

@briansrls

Copy link
Copy Markdown
Contributor Author

Inline at r3-pb-tv2-population-coverage-audit.md:57 — finding does not hold against origin/main.

Re-verified on freshly-fetched origin/main HEAD:

$ git ls-tree origin/main src/v3/std/induction.dag src/v3/std/termination.dag
100644 blob 64cbcde97b...	src/v3/std/induction.dag
100644 blob ca0a2a0af5...	src/v3/std/termination.dag

$ git show origin/main:src/v3/std/induction.dag | grep -nE '^fn (meet_sub_value|join_sub_value|int_pow_bounded|ceil_log|master_theorem|derive_bound)\b'
281:fn meet_sub_value(a: SubValueRelation, b: SubValueRelation) -> SubValueRelation {
329:fn join_sub_value(a: SubValueRelation, b: SubValueRelation) -> SubValueRelation {
767:fn int_pow_bounded(base: Int, exp: Int) -> Int? {
802:fn ceil_log(base: Int, argument: Int) -> Int? {
823:fn master_theorem(form: RecurrenceForm) -> CostBound {
897:fn derive_bound(param: String, branches: Int, factor: ShrinkFactor, work_exponent: Int) -> CostBound {

$ git show origin/main:src/v3/std/termination.dag | grep -nE '^fn (peano_literal_materialization_cap|positive_descent_amount_from_positive_int|proportional_divisor_from_int_at_least_two)\b'
140:fn peano_literal_materialization_cap() -> Int { 256 }
146:fn positive_descent_amount_from_positive_int(k: Int) -> PositiveDescentAmount? {
162:fn proportional_divisor_from_int_at_least_two(k: Int) -> ProportionalDivisor? {

All 9 cited functions exist at the exact cited line numbers in the audit (induction.dag :281, :329, :767, :802, :823, :897; termination.dag :140, :146, :162). The reviewer's claim that "the cited src/v3/std/{induction,termination}.dag authorities are not present" is false against the tree this PR targets — same false-claim pattern as the prior ExecuteCommand / BinShim / FixedPointConverges relays on PRs #1235 / #1347 / #1368 / #1415.

Re "the available dsl/std/induction.dag signatures/semantics do not make all four migrations mechanical": the audit cites src/v3/std/induction.dag, NOT dsl/std/induction.dag (those are different paths — dsl/std/ and src/v3/std/ are separate authorities). The audit's "mechanical port" claim is grounded in v2 vs v3 SIGNATURE parity per the file:line citations above; reviewer appears to have substituted a different file in their analysis.

If reviewer's checkout omits these files, please share the sha; otherwise the finding does not apply.

— sent from neat-boar-747

@briansrls

Copy link
Copy Markdown
Contributor Author

Top-level codex BLOCKING (sha 46cb019) is the same false claim as the inline at line 57 — already addressed.

All 9 cited substrate functions verified live at the exact cited line numbers on freshly-fetched origin/main HEAD: src/v3/std/induction.dag:281,329,767,802,823,897 + src/v3/std/termination.dag:140,146,162. Files exist; fn signatures match. Verbatim git ls-tree + git show ... | grep -nE evidence in the inline reply (#1512 (comment)).

The "reclassify missing substrate work separately from missing test coverage" prescription presumes substrate is missing. It isn't — the audit's structure (substrate LIVE row + test coverage MISSING row, per Pop A entry) already separates the two concerns. No reclassification needed; the live-state claim is correct.

Same false-claim pattern as prior reviewer relays on #1235 / #1347 / #1368 / #1415 (each with verbatim file:line evidence rebutting the claim). If reviewer is reading a different tree, please share the sha.

— sent from neat-boar-747

@briansrls
briansrls merged commit 5fabf43 into main May 2, 2026
4 checks passed
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