Repository navigation
docs(briefs): 4 receipt-closure dispatch briefs + ROADMAP debt rows - #614
Conversation
Briefs for the next wave per SG manager recommendation: - docs/briefs/sg-4b-1-fix-declaration-lookup-cleanup.md (S) — delete DeclarationLookup + find_declaration parallel authority from #609's merged tranche; consumers use Dag::declaration(id). - docs/briefs/sg-3g-b-lower-helpers-wire-in.md (M) — wire 16 expr_span call sites in lower.rs to consume the generated lower_helpers helper from #612's staging. - docs/briefs/sg-2c-2-next-parser-table.md (M-L) — next bounded parser-data extraction from parse_parser_body.txt following SG-2c-1 pattern; item-keyword or punct/operator table. - docs/briefs/1e-2b-class-5-gap-1-or-different-cluster.md (M-XL, decision-first) — retry Lane 1e-2 after #610 miss. Path A: Bool → BooleanAlgebra grounding (Class 5 Gap 1) then logical ops via existing OperatorRealization. Path B: different verified cluster after Phase 0 verification. ROADMAP debt rows added to "Post-merge debt (2026-04-21 receipt-closure wave)": - Class 5 Gap 1 (Bool grounding) — owner 1e-2b Path A - emit_rust_module SurfaceLiteral→LiteralBits rename gap (from #612) - emit_rust_module render_variant_constructor tuple variant gap (from #612) - SG-4b-1 DeclarationLookup parallel authority (from #609 override) Receipt-closure wave posture: no full-file retirement, no walker collapse, no new XXL umbrella lanes without verified clusters. Closes design debts surfaced by prior wave; validates patterns end-to-end on small slices before next step-function dispatch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Latest read: this package is mostly in good shape, and the overall queue is aligned with the post-merge state. I have one material correction to make before I’d treat the brief set as fully clean. Blocking direction: SG-3g-b brief is mis-stating the real blockerIn the
and then says those are not blocking wire-in. That is not the current state of Per the merged
The two emit gaps above were blockers for the earlier candidate slices that were abandoned. They are important debt rows, but they are not the gating reason Requested fix to the briefPlease revise
Right now the brief risks dispatching a worker toward the wrong prerequisite. Everything elseOutside that, the packet reads well:
So from my side: one real fix on |
Codex inline review on #608 caught a 5th audit error: Cluster E's proposed OptionalTypeRendering.wrapper_template is already covered by TypeApplicationSyntax.optional in emit_model.dag (consumed by go/python/rust emit — e.g., go_type_applications.optional: "*{element}"). 1e-2b's Phase 0 verification section now explicitly names Cluster E alongside A, B, C, F, H as known-to-skip. Path B workers will correctly bypass it; Path A is unaffected. The #608 audit document itself remains un-fixed on main — this is a forward-looking brief update rather than a retroactive correction. The 1e-2b "treat #608's classification as hypothesis, verify before implementing" posture covers the remaining uncontested clusters. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Review metadata
Verdict: APPROVE — diff is docs-only (4 new dispatch briefs under Exploratory observations (optional):
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32446d5fd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - **Class 5 Gap 1 — `Bool` inhabits `BooleanAlgebra<Bool>` grounding not wired**: surfaced during #610's closing. Current `OperatorKind::Logical(_)` dispatch in the three emitters is hardcoded (`if let OperatorKind::Logical(_)` branches bypass the algebra-field resolution that arithmetic operators use). The substrate already models `BooleanAlgebra<T>` with `meet/join/complement` per `dsl/std/algebra.dag:230-236`, and comments there state *"Bool inhabits BooleanAlgebra"* — but the `inhabits` edge isn't structurally landed. Dissolution: wire `Bool inhabits BooleanAlgebra<Bool>` + extend `resolve_operator_arrow` for `OperatorKind::Logical(_)` + delete the hardcoded emitter branches. Owner: 1e-2b lane (Path A). | ||
| - **`emit_rust_module` gap: SurfaceLiteral → LiteralBits rename**: substrate declares `LitInt/LitBool/LitString` in `LiteralBits`; Rust enum is `Int/Bool/String`; no rename facility in `emit_rust_module` causes connective-producing slices (per #612's staging choice notes) to fail on this. Dissolution: add per-variant name mapping in spec or rename-facility in emit. Owner: unassigned; blocks deeper SG-3g slices. | ||
| - **`emit_rust_module` gap: `render_variant_constructor` fails on external tuple variants**: `src/v3/compiler/src/emit/rust_target.rs:4296` unconditionally emits `Variant { _0: value }` which is wrong for external tuple variants. Dissolution: spec-driven variant-constructor template per source kind. Owner: unassigned; blocks deeper SG-3g slices. | ||
| - **SG-4b-1 `DeclarationLookup` parallel authority**: PR #609 merged with a fail-open `find_declaration` / `DeclarationLookup` path that duplicates `Dag::declaration(id)` and converts Track-9 constructor-validation failures into normal inference outcomes (C-8 violation). Dissolution: replace consumers with `Dag::declaration(id)`, delete the parallel path. Owner: SG-4b-1-fix lane. |
There was a problem hiding this comment.
Remove nonexistent SG-4b-1 debt from roadmap
This new debt row says a live find_declaration/DeclarationLookup path still exists, but in this revision those symbols are already absent from both src/v3/lenses/infer_helpers.dag and src/v3/compiler/src/infer_helpers_generated.rs (the helper file only contains TemplateArgumentLookup and related functions). Keeping this as an active lane makes the associated cleanup brief unexecutable and its acceptance criteria impossible to verify, which can misdirect follow-up work.
Useful? React with 👍 / 👎.
Codex inline review on #608 flagged internal contradiction: PR-4.2's claim "new target = one spec file, zero walker changes" contradicted Category 3's named per-target walker hooks (Go modules, Python indent, Go Loop feature gap). Both statements can't hold: if the walker has per-target hooks for those three concerns, a new target with different module semantics / whitespace rules would require a new hook (walker change). Calibrated: PR-4.2's claim now reads "new target = one spec file + reuse of existing per-target hook categories." Targets fitting the residual hook surface need zero walker changes; targets with new language-intrinsic concerns add one narrow hook (same small surface as the three existing residuals). Two edits: - Phase 4 table: PR-4.2 description reworded - Estimated Impact section: "Thesis claim validated (calibrated)" explains the walker-change surface IS the per-target-hook surface, not zero — but that surface is narrow and matches the three named residuals. Doc-only correction to the merged #608 audit; the calibrated falsifier is what Phase 4 actually tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Earlier fix (cbd7b70) marked all of Cluster E as "already covered" — too broad. Codex's nuanced re-read caught that Cluster E bundled two unrelated concerns: 1. `wrapper_template` (type-wrapper syntax Option<T> / *T / Optional[T]) — really is covered by TypeApplicationSyntax.optional. 2. `none_literal` / `some_constructor` / `deref_syntax` / `none_check` (expression-level None/Some rendering + check strategy) — NOT covered by TypeApplicationSyntax (that's type-level). LiteralSyntax covers true/false/string_delimiter but not None/Some. The expression-level optional behavior may be handled via PatternBindingRule + VariantPayloadFieldAccessRule (structural variant dispatch) OR may be hardcoded — Phase 0 verification required before implementing. 1e-2b's Cluster E skip entry now reflects the split: wrapper_template → skip (Category 1), expression-level fields → verify before implementing. If verification shows they're structurally handled, also Category 1. If verification shows they're hardcoded branches, the narrow Category 2 scope is just those expression-level facts — not the wrapper_template. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer (briansrls) caught: SG-3g-b brief mis-stated the blockers for wire-in. Per #612's actual body: **Primary wire-in blocker**: parse vs parse_surface convergence. Generated helper types against parse_surface::SurfaceExpr; lower.rs uses parse::SurfaceExpr. The available From bridge deep-clones across 16 call sites, which regresses the lowerer — #612 explicitly rejects this. **Secondary blocker**: render_variant_constructor external tuple-variant handling (blocks connective-producing lens slices generally, not specific to expr_span). **NOT a wire-in blocker**: SurfaceLiteral → LiteralBits rename. That was a reason for abandoning *alternative* candidate slices during #612, not a blocker for the expr_span slice that was actually selected. Changes to the brief: 1. "Read first" now names the type-convergence blocker as primary, keeps the tuple-variant concern as secondary, and explicitly says LiteralBits rename is NOT a wire-in blocker (just named debt from candidate-slice abandonment). 2. New "Phase 0 verify" step before touching call sites — confirm the deep-clone situation matches #612's body. 3. New "Core wire-in (resolve type-convergence)" section with three options A/B/C for the worker to evaluate (unify types, lower.rs adopts new type, helper generates against old type). Worker picks + justifies in PR body. 4. STOP-AND-ESCALATE updated: primary STOP is now "wire-in requires per-call deep cloning across the boundary" — that's the critical no-compromise gate. Deep-clone wire-in is worse than the current staging state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewer correct — my SG-3g-b brief mis-stated the real wire-in blocker. Fixed at Re-read of #612's body confirms: Primary wire-in blocker: Secondary blocker: NOT a wire-in blocker: Changes to SG-3g-b brief
Other three briefs (SG-4b-1-fix, SG-2c-2, 1e-2b) unchanged per your "rest looks sound" signal. Ready for re-read on SG-3g-b when convenient. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
32446d5f· Trigger:schedule - Thinking:
440s wall
BLOCKING (2)
Root Cause
docs/briefs/sg-4b-1-fix-declaration-lookup-cleanup.mdHistorical #609 review context was copied into the brief without re-verifying the current infer-helper authority → re-check the referenced files on the target branch before adding debt rows or dispatch docs.docs/briefs/sg-3g-b-lower-helpers-wire-in.mdThe brief rechecked earlier emit-gap blockers but not the current helper authority header that names the actual gate → either retarget SG-3g-b to the convergence work or explain how that type mismatch is closed first.
Non-blocking — Strengths
docs/briefs/1e-2b-class-5-gap-1-or-different-cluster.mdThe Path A / Path B split plus mandatory Phase 0 verification is well-calibrated against the thesis and does a good job preventing another carrier-style workaround.
Non-blocking — Improvements (fix in-PR if easy, else defer to roadmap)
docs/briefs/sg-2c-2-next-parser-table.mdThe ratchet reference should point atsrc/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rs, which is the existing parse-tables authority check, notsg2_parse_authority_test.rs.
ROADMAP — Verified
- Class 5 Gap 1 — Bool grounding gap: The new Bool/BooleanAlgebra debt row matches live code: infer still documents the missing structural Bool link and the emitters still special-case logical operators.
ROADMAP — Incomplete
- SG-4b-1 DeclarationLookup parallel authority: This new debt row is stale because the referenced infer-helper parallel authority is not present in the current branch state.
|
|
||
| ## Work | ||
|
|
||
| 1. **Delete `DeclarationLookup` enum and `find_declaration` fn** from `infer_helpers.dag`. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ## Work | ||
|
|
||
| 1. **Identify all 16 `expr_span` call sites** in `lower.rs` (grep for the pattern, verify count matches #612's body claim). | ||
| 2. **Replace each call site** with a call to the generated `lower_helpers::expr_span(...)` fn. Import the generated module as needed in `lib.rs` / `lower.rs`. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…anup Codex reviewer correctly flagged: find_declaration / DeclarationLookup are NOT in infer_helpers.dag / infer_helpers_generated.rs on main. Those files were cleaned up independently after #609; they now contain only TemplateArgumentLookup. The pattern DOES persist on main — in `src/v3/lenses/variant_payload.dag`: type DeclarationLookup = LookupMissing | LookupFound(Declaration) fn find_declaration(decls: List<Declaration>, target: ...) -> DeclarationLookup = ... Same concerns (parallel O(n) walk vs Dag::declaration; fail-open vs fail-closed C-8) — just different file. Variant names are slightly different (LookupMissing / LookupFound vs MissingDeclaration / FoundDeclaration) reflecting the separate author path. Updates: - ROADMAP debt row now cites variant_payload.dag - SG-4b-1-fix brief retargeted: Read first, Context, Work, Acceptance all point at src/v3/lenses/variant_payload.dag - Acceptance now includes a sanity grep confirming zero hits post-cleanup - The "originally surfaced via #609; that file cleaned up independently; pattern persists in variant_payload.dag" framing makes the migration context honest The underlying work is unchanged in shape: delete the parallel authority, consume the canonical Dag::declaration accessor, treat missing-id as a substrate-integrity violation not a normal lens outcome. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewer correct — but with a nuance: Verified on The pattern variants are slightly renamed ( Fix pushed
The debt is real and the brief is now executable against the actual file. Thanks for the catch — saves the worker from reading the wrong file. |
Codex reviewer flagged (at line 44, but the issue was broader): previous retargeting at 19eeb5c2 missed stale references in the STOP-AND-ESCALATE section (line 58 mentioned "regen_infer_helpers ratchet"; line 59 named infer_helpers.dag / infer_helpers_generated.rs as scope gates) and the Size section (line 70 said "deleted from infer_helpers.dag, corresponding deletion in infer_helpers_generated.rs"). Those were leftover from the original infer-focused draft and would have misdirected the worker. Changes: - STOP item 2: "regen_infer_helpers" → "variant_payload regen ratchet (or equivalent lens freshness check)" - STOP item 3: "infer_helpers.dag / infer_helpers_generated.rs" scope-gate → "variant_payload.dag / variant_payload_generated.rs" - Non-goals: explicit "not touching infer_helpers.dag" line (confirm via sanity grep; do not modify) - Size estimate: retargeted to variant_payload with 30-80 LOC (more accurate for a lens file vs the earlier infer estimate) - Dispatch note: "inference tests" → "variant-payload + adjacent tests" Remaining `infer_helpers` references are honest — they document the migration context (pattern was originally in infer_helpers, cleaned up independently, persists in variant_payload). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Verified — reviewer pointed at line 44 but the issue was broader. My previous fix at
Both would have misdirected a worker to the wrong file mid-execution. Scrubbed at
The remaining Thanks for the catch; the line-44 point made me re-scan and find the other stale references. |
|
Already addressed at Verified at current HEAD of #614:
The finding matches the fix I pushed at No new action needed — the brief already states exactly what the reviewer is asking for. Sanity-check line numbers against current HEAD before concluding stale. |
Codex reviewer (non-blocking): SG-2c-2 brief pointed at the wrong ratchet test. Both files exist on main: - src/v3/compiler/tests/integration/sg2_parse_authority_test.rs (older parser-staging authority, from #589) - src/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rs (the SG-2c-1 parse-tables ratchet — the actual template SG-2c-2 should replicate) Fixed two references: - "Read first" line now cites sg2c1_parse_tables_authority_test.rs with an explicit "not sg2_parse_authority_test.rs" clarification - "Extend the ratchet test" step points at the correct file Ergonomic fix — prevents worker from editing the wrong file or copy-pasting the wrong pattern during SG-2c-2 execution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewing the consolidated codex review (sha Item 1 — SG-4b-1-fix stale infer contextAlready addressed at Item 2 — SG-3g-b convergence gateAlready addressed at Non-blocking improvement — SG-2c-2 ratchet file referenceFixed now at
Brief's two references (Read first + Work step 5) now point at ROADMAP — Incomplete (reviewer stale)Reviewer flagged "SG-4b-1 DeclarationLookup parallel authority" debt row as stale. This was already corrected at ROADMAP — VerifiedReviewer confirms Class 5 Gap 1 row matches live code. ✓ Non-blocking strength1e-2b Path A / Path B decision-first split noted as well-calibrated. Thanks — intent to reuse the pattern for future ambiguous lanes. SummaryAll live items addressed; no new action beyond this commit. Ready for re-review. |
|
Review metadata
Verdict: APPROVE — docs-only PR. Four brief files + four ROADMAP debt rows + a calibration tweak to The four ROADMAP debt entries (lines 164-167) each carry the three properties the scaffold-boundary rule asks for: a named symptom, a bounded scope, and an explicit dissolution trigger with an owner lane. That's the right shape for tracked debt. The briefs themselves are planning artifacts — they call out their own STOP-AND-ESCALATE conditions, name Phase 0 verification before implementation, and explicitly flag "wrong outcome is another misclassified carrier like #610" (1e-2b brief). The One small observation (non-blocking): The 1e-2b brief cites |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.4 - Commit:
9b0bca71· Trigger:schedule - Thinking:
468s wall
BLOCKING (1)
Root Cause
src/v3/std/substrate.dagreflected substrate exposes keyedport/nodeaccessors but no keyeddeclarationaccessor → add typed declaration lookup authority first, or explicitly rescope SG-4b-1 as substrate-extension work instead of a variant_payload-only cleanup
Non-blocking — Strengths
docs/briefs/sg-2c-2-next-parser-table.mdThe brief now points SG-2c-2 at the livesg2c1_parse_tables_authority_test.rsratchet, which is the authority test that actually guardsparse_tables.dag/parse_tables_generated.rs.docs/briefs/1e-2b-class-5-gap-1-or-different-cluster.mdPath A / Path B plus mandatory Phase 0 verification matches the live logical-operator state and avoids reintroducing the rejected parallelLogicalOperatorCarrier.docs/briefs/sg-3g-b-lower-helpers-wire-in.mdThis now reflects the real wire-in gate:lower_helpers_generated.rsis typed againstparse_surface::SurfaceExpr,lower.rsstill owns its localexpr_span, and the available mirror path is a deep-cloning bridge.
ROADMAP — Verified
- Class 5 Gap 1 — Bool grounding gap: The new debt row matches live code:
resolve_operator_arrowstill falls back Logical ops to Bool-monomorphic scaffolding and all three emitters still special-case logical symbols. - SG-4b-1 DeclarationLookup parallel authority: The new debt row now points at the live
variant_payload.dag/variant_payload_generated.rsauthority split rather than the already-cleanedinfer_helperssite.
| ## Work | ||
|
|
||
| 1. **Delete `DeclarationLookup` enum and `find_declaration` fn** from `src/v3/lenses/variant_payload.dag`. | ||
| 2. **Replace consumer sites** in the same file (grep for `find_declaration(` within `variant_payload.dag`) with direct consumption of the canonical declaration accessor — whatever `.dag`-side surface resolves to `Dag::declaration(id)` without going through a parallel enum. |
There was a problem hiding this comment.
BLOCKING: This brief assumes a live .dag equivalent of Dag::declaration(id), but the reflected substrate still does not expose that accessor, so the promised variant_payload-only cleanup cannot honestly close the single-authority / fail-closed issue as scoped.
|
Verified at current HEAD — this is a stale relay. Fixes landed at Line 44 at current HEAD (verified just now): Other sections targeting
Remaining The finding would be real if the brief still pointed at infer_helpers. It doesn't. Reviewer sha predates commits |
|
Verified at current HEAD — stale relay, second time for this same finding. Fix landed at Line 17 (Read first) at current HEAD:
Lines 23-30 (Work / Phase 0) at current HEAD explicitly name verification of the convergence blocker, with grep instructions for The brief does NOT assume Reviewer sha is pre-fix (same issue as the 06:40:44Z relay, which I responded to at Sanity-check line numbers against current HEAD before flagging blockers — relayed sha |
Codex reviewer surfaced real prerequisite gap: `src/v3/std/substrate.dag`
exposes keyed accessors for `port`, `node`, `resolve_producer`,
`lane2_workflow_at` — but NOT for declarations by id. `.dag` lens
consumers literally cannot call `Dag::declaration(id)`; they must
either walk `d.declarations` via fold or use the parallel
`find_declaration` helper.
So the "variant_payload-only cleanup" framing was incorrect — the
parallel authority exists BECAUSE the reflected substrate is
incomplete. Deleting `find_declaration` without a substrate
extension would force consumers to inline the same walk under a
different name.
Rescoped as two-step lane:
**Step 1 — Substrate extension (prerequisite)**:
Add `fn declaration_by_id(d: Dag, id: DeclarationId) -> Declaration?`
to substrate.dag with `host declaration_by_id` realization binding
to Rust's `Dag::declaration`. Preferred return type: `Declaration?`
for symmetry with existing `port` / `node` accessors; C-8
fail-closed enforced at caller.
**Step 2 — variant_payload migration**:
Delete DeclarationLookup + find_declaration; replace consumer
sites with typed `match declaration_by_id(d, id) { Some(decl) =>
... / None => diagnostic }`. None-handling treats as
constructor-layer violation via typed diagnostic, NOT as a
normal lens outcome (which was LookupMissing's sin).
Size: S → S-M. Acceptance split into Step 1 / Step 2 / End-state
sections.
End-state: reflected substrate now has id-keyed lookup parity
with port / node accessors, and the parallel authority is gone.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Reviewer correct — real finding, not stale. Fixed at Verified the gap: No
Deleting option 2 without adding a real accessor just pushes consumers to option 1 — same parallel authority, different shape. Rescope pushedSG-4b-1-fix is now a two-step lane (size upgraded S → S-M): Step 1 — Substrate extension (prerequisite): Step 2 — variant_payload migration: End-state
The substrate extension is the actual load-bearing work here; the variant_payload cleanup is the receipt. Acks on other items from consolidated review
Only the SG-4b-1-fix scoping needed this one more pass. Ready for re-review at current HEAD. |
Summary
Four dispatch briefs for the next wave (receipt-closure, not step-function) per the SG manager's recommended next-wave queue, plus ROADMAP debt-row updates capturing items surfaced during the 2026-04-20 → 2026-04-21 merge wave that didn't land in lanes.
Briefs
DeclarationLookup+find_declarationparallel authority from #609; consumers useDag::declaration(id). Cleans up the C-8/fail-open issue.lower_helpers.expr_spaninto lower.rs's 16 hand-authored match sites. Turns #612 from staging → real receipt.ROADMAP debt rows added
Post-merge debt (2026-04-21 receipt-closure wave):
Wave framing
This is a receipt-closure wave, not step-function. No full-file retirement, no walker collapse, no new XXL umbrella lanes without verified clusters. Closes design debts surfaced by prior wave; validates patterns end-to-end on small slices.
Meta-observation: pattern holding well since lessons of #608/#610 — small bounded receipts work; big-bang XXL lanes without verified clusters produce flawed outputs fast.
Test plan
🤖 Generated with Claude Code