Skip to content

docs(T-PB-B-2): predicate-gapped G list for Testgen backlog - #727

Merged
briansrls merged 14 commits into
mainfrom
session/neat-swift-804
Apr 24, 2026
Merged

briansrls merged 14 commits into
mainfrom
session/neat-swift-804

Conversation

@briansrls

@briansrls briansrls commented Apr 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Adds docs/briefs/pb-b-2-predicate-gapped-g-list.md — the T-PB-B-2 "needs schema" 1-pager.
  • Enumerates six live TestPredicate shapes (seventh retracted per codex review — DiagnosticReference.kind is already the single authority for phase) needed to port the pipe_desugar.rs-style G bucket (post-compile structural queries) to .dag TestClaim values.
  • "What ships today" splits schema-on-main (declared in src/v3/std/verification.dag) from runner-wired variants, pointing at verification.dag as the single schema authority.
  • Non-landing: no Rust deletion, no .dag drafts until Testgen schema lands.

Test plan

  • Docs-only; no code paths touched.
  • Testgen manager reviews six shapes against current schema roadmap.

🤖 Generated with Claude Code

Non-landing brief. Enumerates the seven TestPredicate schema shapes
needed to port the pipe_desugar-style G bucket (structural queries
over the post-compile Dag) to .dag TestClaim values. Feeds Testgen
manager backlog; no Rust deletion, no .dag drafts until Testgen
schema lands or pre-approves.

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: cc2e652e · Trigger: schedule
  • Thinking: 17s wall

Verdict: APPROVE — docs-only brief adding a new non-landing file under docs/briefs/. No code, no substrate changes, no invariant surface touched. Content is internally consistent: it enumerates G-bucket predicate gaps, cites concrete test modules, and explicitly defers schema/ordering to Testgen (no unilateral design). Non-asks section correctly forbids textual-enforcement bridges and parallel test-DSLs, consistent with modeling discipline. Nothing to flag.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: cc2e652e · Trigger: schedule
  • Thinking: 149s wall

Findings:

  • NON-BLOCKING — docs/briefs/pb-b-2-predicate-gapped-g-list.md:100 proposes CompileErrorAtPhase because FailsWithDiagnostic is “phase-agnostic,” but the landed DiagnosticReference.kind already includes ParseError and the runner matches CompileError::Parse through that path. As written, this risks creating a second authority for diagnostic phase, contrary to INVARIANTS P2 / single-authority metadata. If the real missing assertion is span coverage or diagnostic code identity, the brief should name that instead.

Verdict: APPROVE_WITH_COMMENTS — The brief is bounded, non-landing, and mostly consistent with the modeling/testgen direction. I’d tighten the diagnostic-phase gap before it becomes schema work.

DiagnosticReference.kind already discriminates ParseError vs later
phases (test_runner.rs:402) and FailsWithDiagnostic matches on it.
A phase-pinning predicate would create a second authority for
diagnostic phase, against INVARIANTS P2. Phase-pinning G tests
move to the D bucket; list drops to six shapes.

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: 9d3e0a43 · Trigger: schedule
  • Thinking: 18s wall

APPROVE — Docs-only addition of a single brief at docs/briefs/pb-b-2-predicate-gapped-g-list.md. The content is well-scoped (Testgen backlog input, no code changes), correctly defers schema decisions to the Testgen manager, and the retraction note at line 100-112 explicitly cites the modeling-discipline single-authority concern (DiagnosticReference.kind as sole phase authority) — exactly the right reasoning. Nothing in this diff touches substrate, code, or tests, so INVARIANTS/CODING/TESTING have no concrete violations to flag.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 9d3e0a43 · Trigger: schedule
  • Thinking: 100s wall

Findings

Verdict
APPROVE_WITH_COMMENTS — the brief is doc-only, clearly marks the retracted duplicate-authority predicate, and does not change substrate/code. The only issue I found is stale count wording around the hand-off.

@briansrls

Copy link
Copy Markdown
Contributor Author

Feedback (T-PB-B-2 / Testgen backlog)

What works well

  • pipe_desugar.rs as the canonical G prototype is the right anchor: compile → named bind → transform target → literal ports is exactly the “structural query over post-compile substrate” story that Compiles / FailsWithDiagnostic / lens-output equality cannot express without tautology.
  • Retracting the phase-pinning predicate (Add LLM provider integration for OpenAI and Anthropic #7) is a good call: staying on DiagnosticKind (and tightening only if you truly need span/code identity) avoids a second authority for “where” a failure happened when FailsWithDiagnostic already carries the taxonomy.
  • “Non-asks” (no parallel traversal DSL; predicates name what, not how) lines up with the project’s lens / substrate discipline and keeps the backlog from turning into an imperative test harness in .dag.

Small doc fixes before merge

  1. Count / naming consistency — The PR body still says “seven” shapes; the file now has six live backlog items after the retraction. Consider aligning title/PR summary and the “Needs-schema list” header so readers do not grep for a missing Add LLM provider integration for OpenAI and Anthropic #7.
  2. Filename convention — pb-b-2-… reads like a typo next to t-pb-b-1.md / t-pb-b-brief-d.md. Renaming to something like t-pb-b-2-predicate-gapped-g-list.md would match the rest of docs/briefs/ and searchability.
  3. “Ships today” anchor — The “Runner predicates on main” bullet list includes several names that are not on src/v3/std/verification.dag in-tree today (ExecuteCommand, LensOutputEquals, …). Worth splitting into (a) schema that already exists on std.verification vs (b) predicates the Testgen roadmap treats as runner-level extensions, so this brief does not accidentally become a second schema authority.

Optional follow-ups (non-blocking)

  • For Consolidate binaries into gunbc-dag package #4 (NodeCountByBehavior), an early Testgen design pass might decide whether a declarative count predicate stays maintainable or whether those 91 cases should fold into surfaced substrate facts / a single lens carrier instead (fewer bespoke predicates).
  • Cross-link from docs/briefs/t-pb-b-brief-d.md (D/G/A/B matrix) or t-pb-b-1.md so the G backlog is discoverable from the inventory side.

Thanks — this is useful, actionable backlog input for the Testgen manager without touching code paths.

briansrls and others added 2 commits April 24, 2026 03:11
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e shapes

Codex reviewer flagged the 'ships today' list as risking a second
schema authority. Split into (a) variants declared in
src/v3/std/verification.dag vs (b) variants the runner evaluates
today, pointing at verification.dag as the single authority.
Also label the needs-schema header as 'six live shapes' for
grep-consistency after the #7 retraction.

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: 620e3a7c · Trigger: schedule
  • Thinking: 22s wall

Findings

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md:127 — stale count. Heading on line 70 says "six live shapes" and Add LLM provider integration for OpenAI and Anthropic #7 is explicitly retracted (line 110), but the G-modules intro still reads "covered by the seven shapes above." Minor doc inconsistency; not a principle violation, just internally inconsistent after the retraction commit.

Verdict

APPROVE_WITH_COMMENTS — docs-only, non-landing brief; respects the "authority is verification.dag" split (line 40) and the no-parallel-test-DSL invariant (line 156). Only nit is the lingering "seven shapes" wording on line 127.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 620e3a7c · Trigger: schedule
  • Thinking: 63s wall

Findings

  • NON-BLOCKING: docs/briefs/pb-b-2-predicate-gapped-g-list.md:127 says the modules are covered by “the seven shapes above,” but the brief now frames the backlog as six live shapes with item Add LLM provider integration for OpenAI and Anthropic #7 retracted. That leaves stale documentation in the handoff path, which conflicts with the P1 “Documentation Describes Live State” lens. Change this to “six live shapes above.”

Verdict: APPROVE_WITH_COMMENTS. The brief otherwise looks narrowly scoped and careful about avoiding second schema authority.

@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: f6a0296d · Trigger: schedule
  • Thinking: 217s wall

BLOCKING (1)

Root Cause

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md runner status was inferred from manager shorthand rather than verified against the actual runner dispatch table → update "What ships today" to distinguish schema-declared, runner-dispatched, and NYI predicates from src/v3/std/verification.dag and src/v3/compiler/src/test_runner.rs.

⚠️ The brief's core backlog split depends on a runner capability claim that is currently false.

Scaffold marker): `Compiles`, `FailsWithDiagnostic`,
`ExecuteCommand`, `ForAllTargets`, `LensOutputEquals`,
`DifferentialEquals`, `AlgebraicLaw`, `MockBackedInvariant`.
- **Runner-wired today** (per r1-testgen-manager working state):

This comment was marked as resolved.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: f6a0296d · Trigger: schedule
  • Thinking: 14s wall

Docs-only brief. Non-landing, clearly scoped, no code changes.

Verdict: APPROVE — pure documentation addition (one brief under docs/briefs/), explicitly non-landing and parallel-safe. Authority is correctly deferred to verification.dag and the Testgen manager; the retraction of #7 is well-reasoned (avoids a second authority for diagnostic phase, pointing at the existing DiagnosticReference.kind discriminator). Nothing in this diff touches substrate, code, or tests — no invariant/modeling concerns apply.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: f6a0296d · Trigger: schedule
  • Thinking: 202s wall

Findings

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md:34 says the runner-wired predicates today are only Compiles, FailsWithDiagnostic, ExecuteCommand, ForAllTargets, LensOutputEquals, and DifferentialEquals. That does not match the live runner, which also wires OutputEquals, PortHasState, and CostBounded in test_runner.rs. This violates INVARIANTS.md P1 “Documentation Describes Live State” and weakens the doc’s own single-authority guard. Either include the already-wired predicates or narrow the wording to the subset relevant to this brief.

Verdict: REQUEST_CHANGES. The brief is otherwise narrowly scoped and has the right authority-split framing, but the “what ships today” inventory needs to match the live schema/runner state before this becomes backlog input.

Codex review flagged the "Runner-wired today" list as inferred from
manager shorthand rather than verified against the actual dispatch
table. Audit of src/v3/compiler/src/test_runner.rs:101 shows the
runner matches five labels: Compiles, FailsWithDiagnostic,
OutputEquals, PortHasState, CostBounded — everything else (including
ExecuteCommand, ForAllTargets, LensOutputEquals, DifferentialEquals,
AlgebraicLaw, MockBackedInvariant) returns NotYetImplemented.

Update the brief to name three buckets explicitly: schema-declared,
runner-dispatched, and schema-declared-but-NYI. Also tighten the
D-bucket definition to "predicates the runner dispatches today" so
runner-NYI predicates route to Testgen's runner-wiring backlog
instead of being miscounted as directly portable.

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

Copy link
Copy Markdown
Contributor Author

Verified finding against code and fixed in 0210229.

Audit result: reviewer is correct. Runner dispatch at src/v3/compiler/src/test_runner.rs:101 matches five labels only: Compiles, FailsWithDiagnostic, OutputEquals, PortHasState, CostBounded. The brief's "Runner-wired today" list named four predicates (ExecuteCommand, ForAllTargets, LensOutputEquals, DifferentialEquals) that actually fall through to ClaimResult::NotYetImplemented, and omitted three that are wired (OutputEquals, PortHasState, CostBounded).

Fix: three explicit buckets in "What ships today" — schema-declared (11 variants in verification.dag), runner-dispatched (5, with file:line cite), schema-declared-but-NYI (6). Also tightened the D-bucket intro so runner-NYI predicates route to Testgen's runner-wiring backlog rather than being miscounted as directly portable.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 02102293 · Trigger: schedule
  • Thinking: 14s wall

APPROVE — docs-only brief, narrowly scoped, explicitly non-landing. The author correctly splits schema-declared vs runner-dispatched authority (citing verification.dag and test_runner.rs:101), explicitly names authority to avoid becoming a second source, retracts #7 after review to avoid creating a duplicate phase authority (a nice application of the single-authority invariant), and the "Non-asks" section actively defends against parallel-representation drift (no test DSL, no new carrier types). Nothing in this diff touches substrate, code, or invariants — just a backlog feed for Testgen.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 02102293 · Trigger: schedule
  • Thinking: 85s wall

Findings

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md:89 and the other proposed shapes repeat program inside TestPredicate. That conflicts with P2 / single-authority metadata: TestClaim.source / file_name are already the program-under-test authority, and existing predicate variants consume that enclosing claim rather than carrying a second program slot.
  • docs/briefs/pb-b-2-predicate-gapped-g-list.md:35 lists the schema-declared TestPredicate variants but omits BehavioralObservation, which is present in src/v3/std/verification.dag. Since this section is explicitly avoiding a second schema authority, the stale list violates the same single-authority/live-doc discipline.

Verdict: REQUEST_CHANGES. The brief is narrowly scoped, but it currently codifies two schema-authority mistakes in the handoff document. Fixing those should be small: remove program from the proposed predicate shapes or define it as the enclosing claim source, and include or intentionally classify BehavioralObservation.

Codex review flagged two schema-authority mistakes:

1. Proposed predicate shapes repeated `program` inside TestPredicate,
   conflicting with the single-authority rule at verification.dag:169
   where TestClaim.source / file_name name the program under test.
   Drop `program` from all six shapes and add an explicit "Program
   authority" note pointing at the enclosing claim.

2. The schema-declared list omitted `BehavioralObservation`
   (verification.dag:94). Add it to schema-declared and to the
   runner-NYI bucket so the brief mirrors the live schema.

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

Copy link
Copy Markdown
Contributor Author

Both findings verified and fixed in 1a8cf75.

  1. program redundancy — TestClaim.source / file_name at src/v3/std/verification.dag:169 is the single program authority; existing variants (PortHasState, CostBounded) consume the enclosing claim. Dropped program from all six proposed shapes and added an explicit "Program authority" note above the list.

  2. BehavioralObservation omitted — confirmed at verification.dag:94. Added to both schema-declared and schema-declared-but-runner-NYI buckets (runner dispatch at test_runner.rs:101 doesn't match it).

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 1a8cf752 · Trigger: schedule
  • Thinking: 15s wall

Docs-only PR. Content is self-consistent, cites real file:line authority (test_runner.rs:101, verification.dag:169, test_runner.rs:402), carefully maintains single-authority for the predicate schema (defers to verification.dag), explicitly flags the retracted item to avoid forking a second phase authority, and the "no parallel test DSL" non-ask aligns with modeling discipline (lenses not passes).

Verdict: APPROVE — docs-only brief, no code, no substrate change. Authority splits are clean (schema = verification.dag; dispatch = test_runner.rs), the retraction of CompileErrorAtPhase is exactly the right call to avoid a second phase authority, and the non-asks correctly forbid a traversal DSL. Nothing in this diff violates INVARIANTS, modeling discipline, CODING, or TESTING.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 1a8cf752 · Trigger: schedule
  • Thinking: 131s wall

Findings:

  • NON-BLOCKING: docs/briefs/pb-b-2-predicate-gapped-g-list.md:112 proposes NodeCountByBehavior { behavior_kind, count_rel }, but the shown shape has no expected count/bound value. With count_rel ∈ { Equals | AtLeast | AtMost } alone, a runner cannot mechanically evaluate “equals/at least/at most what?”, so the proposed boundary lacks sufficient declared information. This conflicts with INVARIANTS P2 / Boundary Discipline.

Verdict: APPROVE_WITH_COMMENTS. The brief is otherwise narrow, explicitly non-authoritative, and avoids the program-authority duplication issue; I only see the missing count operand as worth tightening before Testgen consumes it.

Non-blocking codex finding: the proposed shape declared only
behavior_kind + count_rel, leaving the runner with no value to
compare against. Add count: non-negative integer so the predicate
carries sufficient boundary information for Testgen to evaluate.

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

@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: 02102293 · Trigger: schedule
  • Thinking: 383s wall

BLOCKING (2)

Root Cause

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md pipe_desugar helper patterns were reduced to bind-rooted target/literal checks, dropping input-producer path facts → add a predicate that can name a root bind plus producer path/target, or narrow the unlock claim to only the direct cases.
  • docs/briefs/pb-b-2-predicate-gapped-g-list.md count comparison was sketched as a local helper label instead of reusing the existing comparison-plus-bound shape from CostBounded → model it as behavior_kind plus ComparisonOp plus Int bound.

⚠️ Two backlog-shape gaps would make the brief under-specify the Testgen work it is meant to hand off.

- `m2_field_access_binding_test.rs` — #1 #2 #3
- `m2_lens_cost_migration_test.rs` / `m2_lens_idempotency_*` /
`m2_lens_unused_parameters_migration_test.rs` /
`m2_lens_variant_payload_migration_test.rs` — #1 #2 #6

This comment was marked as resolved.

imperative walks block port.

5. **`DeclarationResolvedByStructure { program, name }`** — closes
the `AtomPayload::ResolvedByStructure(..)` / `ResolvedByName(..)`

This comment was marked as resolved.

Blocking codex finding: shapes #2/#3 as authored started from a
named Bind and reached only its direct inputs, so they could not
express the nested producer chains in 3 of the 5 pipe_desugar
tests — pipe_chains_left_to_right (outer double → inner add1),
pipe_result_can_feed_later_addition (+ → negate), and
pipe_result_can_feed_later_comparison (== → identity). The brief
claimed those tests were covered, violating live-state accuracy.

Generalize #2 and #3 with a producer_path: List<PortIndex> so the
predicate walks value.produced_by → inputs[path[0]].produced_by →
… (empty path = direct producer, which keeps the single-stage
tests working without change).

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

Copy link
Copy Markdown
Contributor Author

Finding verified and fixed in eb63b97.

Audit: reviewer is right. In pipe_desugar.rs, three of the five tests chain through nested produced_by → Transform:

  • pipe_chains_left_to_right (pipe_desugar.rs:131-156): outer double Transform whose inputs[0].produced_by is inner add1 Transform.
  • pipe_result_can_feed_later_addition (:168-191): + Transform whose inputs[0].produced_by is negate.
  • pipe_result_can_feed_later_comparison (:202-232): == Transform whose inputs[0].produced_by is identity.

Shapes #2 and #3 as authored started from the bind and only reached direct inputs, so listing pipe_desugar as covered by #1/#2/#3/#5 overclaimed.

Fix: generalize #2 and #3 with a producer_path: List<PortIndex> (empty = direct producer; non-empty walks inputs[path[k]].produced_by at each step). Single-stage tests still work with the empty path; nested cases get expressed without adding a new shape or splitting the authority. Called out the specific three tests in the shape rationale.

Blocking codex finding: the prior shape introduced a new count_rel
axis (Equals | AtLeast | AtMost) parallel to ComparisonOp, which is
already declared at src/v3/std/substrate.dag:141 and consumed by
CostBounded at verification.dag:91. That's parallel-representation
debt against an existing single authority.

Replace count_rel with comparator: ComparisonOp so the predicate
reuses the authoritative operator enum. The count operand remains
as added in the prior fix.

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: 60078c0e · Trigger: schedule
  • Thinking: 12s wall

Docs-only addition of a design brief. No code or schema changes; purely informational backlog for Testgen.

Verdict: APPROVE — docs-only brief, narrowly scoped, carefully cross-references the authoritative schema (verification.dag) rather than duplicating it, and the "Program authority" note explicitly defends single-authority on TestClaim.source. No invariants or modeling rules touched by this diff.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 60078c0e · Trigger: schedule
  • Thinking: 168s wall

Findings

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md:103 / :185 — The brief says no new carrier types are needed because OpKind and related names “already exist in v3,” but the proposed shape uses OpKind and PortIndex, neither of which exists as a current v3 carrier. The live Rust authority is OperatorKind, and PortIndex appears only in this new brief. That violates the P1 “Documentation Describes Live State” / P2 single-authority lens by handing Testgen ungrounded type names.

Verdict: REQUEST_CHANGES. The split between schema-declared, runner-wired, and predicate-gapped tests looks otherwise consistent with the current runner/schema, but the new backlog brief should not introduce nonexistent carrier names while claiming it does not.

Codex finding: the brief referenced OpKind and PortIndex, neither
of which is the live carrier name. substrate.dag:160 declares
OperatorKind; ports are PortId (substrate.dag:5), and
Transform.inputs is List<PortId> (substrate.dag:267), so a
producer-path step is a plain Int list index, not a distinct
PortIndex type.

Rename OpKind → OperatorKind in shape #2; respecify producer_path
as List<Int> with a cite to the inputs field; update the "no new
carrier types" non-ask to list the actual live names with
file:line cites.

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

Copy link
Copy Markdown
Contributor Author

Finding verified and fixed in 1a0ca55.

Audit: live authority is OperatorKind at src/v3/std/substrate.dag:160 (not OpKind) and PortId at src/v3/std/substrate.dag:5 (not PortIndex). Transform.inputs: List<PortId> (substrate.dag:267), so a producer-path step is a plain list index — Int — not a distinct carrier.

Fix: renamed OpKind → OperatorKind in shape #2; respecified producer_path as List<Int> with a cite to substrate.dag:267; expanded the "no new carrier types" non-ask to list the live names with file:line cites (OperatorKind, ComparisonOp, PortId, TypeShape, Behavior).

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 1a0ca55a · Trigger: schedule
  • Thinking: 13s wall

APPROVE — Pure docs PR adding a non-landing brief enumerating predicate-gapped tests. It's internally consistent: cites live authorities (verification.dag, test_runner.rs:101, substrate.dag:141/160/5/267), splits schema-declared vs runner-wired to avoid becoming a second schema authority, explicitly flags program single-authority on the enclosing TestClaim (lines 89–94), and retracts #7 on exactly the right grounds (would fork phase authority already held by DiagnosticReference.kind). No code under review, no invariants touched.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 1a0ca55a · Trigger: schedule
  • Thinking: 69s wall

Verdict: APPROVE — this is a docs-only backlog brief, and it stays disciplined about single authority: schema lives in verification.dag, the brief only records consumer gaps. I don’t see a concrete violation of the pinned invariants, coding guidance, or testing discipline in this diff.

@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: 1a0ca55a · Trigger: schedule
  • Thinking: 273s wall

BLOCKING (1)

Root Cause

  • docs/briefs/pb-b-2-predicate-gapped-g-list.md module-level bucketing collapsed distinct assertion shapes → rewrite the unlock section per assertion family or add the missing predicate shapes before claiming these modules are covered

⚠️ The prior inline issues are fixed, but the hand-off still overstates what the six predicate shapes unlock.

gap, not as a phase-pinning predicate.

## G-modules this list unlocks (non-exhaustive)

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 module unlock list overclaims coverage: entries like m2_field_access_binding_test.rs, m1_fn_external_body_reconciliation_test.rs, and the SG authority tests assert FieldBinding/ArrowBody/snapshot/harness facts that are not expressible by the six listed predicates, so the brief no longer describes the live G backlog accurately.

Blocking codex finding: the unlocks list overclaimed coverage.
m2_field_access_binding_test.rs asserts on
TypeRealization[_].FieldBinding[_] records (declaration-level, not
Bind-rooted). m1_fn_external_body_reconciliation_test.rs
discriminates ArrowBody::ExternalRealization vs Unparsed (also
declaration-level). The SG authority tests are snapshot-drift and
rustc-harness runs, not post-compile substrate queries.

Split the section into two lists: modules the six shapes actually
cover (Bind/Transform/port walks), and modules needing additional
predicates (FieldBindingEquals, ArrowBodyKindIs) or routing to the
runner-NYI ExecuteCommand/ForAllTargets path. Retract the >60%
module-count claim, which was inflated by the overclaimed modules.

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

Copy link
Copy Markdown
Contributor Author

Finding verified and fixed in 7e324d7.

Audit: reviewer is right — the unlocks list overclaimed.

  • m2_field_access_binding_test.rs asserts on TypeRealization[_].FieldBinding[_].dag_name / .access records (m2_field_access_binding_test.rs:52-72,164) — declaration-level structural facts, not Bind-rooted walks.
  • m1_fn_external_body_reconciliation_test.rs matches ArrowBody::ExternalRealization vs ArrowBody::Unparsed (m1_fn_external_body_reconciliation_test.rs:54-55) — declaration-body discrimination, not Transform-target identity.
  • sg1_tokenize_authority_test.rs / sg2_parse_authority_test.rs etc. are regen-snapshot and rustc-harness tests, not post-compile substrate queries — they route to ExecuteCommand/ForAllTargets (runner-NYI), not to new TestPredicate shapes.

Fix: split the section in two. The first list keeps only modules the six shapes actually cover (Bind/Transform/port walks). The second list names the overclaimed modules with the specific additional predicates they need (FieldBindingEquals, ArrowBodyKindIs) or marks them as runner-NYI rather than predicate-gapped. Retracted the ">60% of integration-test file count" figure since it was inflated by the overclaimed modules.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 7e324d7d · Trigger: schedule
  • Thinking: 12s wall

APPROVE — docs-only brief, adds a scoped backlog document with clear authority pointers (verification.dag, test_runner.rs:101, substrate.dag lines), explicitly avoids forking authority (program field, ComparisonOp reuse), and honestly retracts/narrows prior claims. No code changes; nothing in this diff touches substrate, runner, or schema. No violations observed.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 7e324d7d · Trigger: schedule
  • Thinking: 103s wall

Verdict: APPROVE

Docs-only diff is narrowly scoped, and the brief keeps verification.dag as the schema authority while clearly separating runner-gapped from predicate-gapped work. I found no concrete violations of the pinned invariants or testing/code discipline. git diff --check is clean.

@briansrls
briansrls merged commit 29fefc0 into main Apr 24, 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