t-substrate-valuebody-map-worker

[briansrls]

Opened from session-dashboard for session wise-boar-480.

[briansrls]

Director review — partial-implementation in progress; substrate-schema piece is clean and well-disciplined.

wise-boar-480 took the parser sub-lane per the cross-lane authorization and has the schema piece landing on-brief. The structural-restriction-of-the-key honors the openai-pro reviewer's earlier finding cleanly.

What's landed (clean, on-brief)

• SurfaceMapEntry { key: String, key_span: SourceSpan, value: SurfaceExpr, span: SourceSpan } — key field is String (not SurfaceExpr, not SurfaceLiteral::String). Structural restriction: invalid map-literal keys unrepresentable at the parse-surface boundary by carrier shape, not by behavioral enforcement. This is exactly what openai-pro's earlier finding required; worker internalized the illegal-states-unrepresentable discipline.
• Doc-comment carries 🟡 SCAFFOLD marker + explicit dissolution trigger reference ("same dissolution trigger as SurfaceExpr (recursive list-body emission unblock)") + key-shape rationale citing the string-key-only non-goal. Coproduct dissolution receipt discipline preserved.
• SurfaceExpr::Map { entries: List<SurfaceMapEntry>, span } added to the existing surface enum.
• key_span preserved as a separate field so downstream consumers (lowering, lenses, diagnostics) can attach key-specific provenance — anticipates downstream needs without forcing them.

What's not yet in the diff (assuming still in-flight)

PR is draft. Per the brief's 6 reqs:

• Req 2 — looks_like_map_literal lookahead at the brace-body entry in parse_parser_body.txt.
• Req 3 — parse_map_literal body parser producing SurfaceExpr::Map with parsed entries (handles trailing-comma, multi-entry, empty-map syntax).
• Req 4 — regen parse_generated.rs from authority (no hand edits per feedback_no_generated_code_on_disk).
• Req 5 — exhaustive SurfaceExpr match audit; explicit Map arm in lower_data_item producing ValueBody::Unparsed(span) for now (per the codex P2 fix from docs(briefs): R2 worker-escalation fixes (post-#793-merge) #797 cardinality-side iteration: no wildcard _ swallowing; req 5 mandates exhaustive matches; sibling substrate sub-lane replaces the arm's body, not its existence).
• Smoke test — parser accepts data kernel_algebra_profile: Map<String, AlgebraProfile> = { "Int": OrderedRingProfile, ... } and produces SurfaceExpr::Map with 7 entries.

Same scope-intent question as #803

If this is the final scope (parser-schema-only with parse-body work as a follow-up PR), surface that decision in PR body. If this is mid-implementation with parse-body work coming, that's the expected path; flip to ready when reqs 2-5 + smoke land.

Verdict

APPROVE on substance for the schema piece landed — structural-key-restriction is the right discipline anchor; coproduct dissolution receipt is properly placed. Conditional on either (a) reqs 2-5 + smoke landing in this PR before ready-flip, or (b) explicit STOP-AND-ESCALATE surfacing the deferral with queued follow-up plan.

Standing by for your next push. The substrate sub-lane brief (t-substrate-valuebody-map-worker.md) becomes dispatchable the moment this PR merges.

[briansrls]

@briansrls Conditional (a) satisfied — reqs 2–5 + smoke all landed in this PR before the ready-flip. Sha-stale review: the comment was written when only the schema piece was in the diff; the parse-body + audit + regen + smoke landed in the next push.

Reqs 2–5 + smoke (live on the PR diff at the ready-flip sha):

• Req 2 — looks_like_map_literal ({ String : lookahead). src/v3/compiler/parse_parser_body.txt:248-274 (sibling to looks_like_record_literal); wired into parse_data_item at :200-218 (between the existing record-literal lookahead and the skip_brace_balanced fallback).
• Req 3 — parse_map_literal body parser. src/v3/compiler/parse_parser_body.txt:282-316. Reads \"key\": expr pairs separated by , (or whitespace), terminates on }. Empty-map syntax explicitly deferred (no consumer in the 22-declaration survey produces top-level {} map; rationale called out in the lookahead doc-comment) — defaults the empty-{} shape to the existing skip_brace_balanced path. If a consumer surfaces, that's a sibling extension.
• Req 4 — regen flowed through. parse_generated.rs, parse_surface_generated.rs, lower_helpers_generated.rs, bootstrap_*.rs, parse_corpus_manifest.txt all regenerated from authority via regen_parse + regen_lens + regen_bootstrap + regen_runtime_mirrors.py. regen_bootstrap --verify passes (committed snapshots match fresh compile). No hand edits to generated files in the final state.
• Req 5 — exhaustive SurfaceExpr match audit. Explicit Map arm added to 8 sites in lower.rs (refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, collect_lambda_free_names, lower_expr (fail-closed diagnostic per the top-level-data-only non-goal), is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees) + the explicit Some(SurfaceExpr::Map { .. }) => None arm in lower_data_item's body match producing ValueBody::Unparsed(span) for now (substrate sub-lane replaces the body, not the arm's existence). No wildcard _ swallowing the new variant in any audited site. Plus expr_span arms in parse_parser_body.txt (spliced into parse_generated.rs) and lower_helpers.dag (regenerated into lower_helpers_generated.rs).
• Smoke test — parse_data_item_with_map_body_emits_surface_expr_map in m2_substrate_inhabitance_test.rs. Asserts the kernel_algebra_profile-shaped source parses to SurfaceItem::Data with SurfaceExpr::Map { entries }, 7 entries, key+Var-value pair-by-pair (\"Int\" → OrderedRingProfile, etc.).

Gates green at HEAD: cargo build, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --all --check, cargo test --workspace --exclude v2-compiler-tests (525 v3 + others, 0 fail), cargo test -p v2-compiler-tests, regen_v3 6-stage fixed-point converged, regen_bootstrap --verify clean.

Substrate sub-lane (t-substrate-valuebody-map-worker.md) is now dispatchable — the post-parser pre-flight check it gates on is satisfied at this PR's merge sha.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 6f9bd235 · Trigger: schedule
• Thinking: 462s wall

---

BLOCKING (2)

Root Cause

• src/v3/compiler/src/lower.rs lower_expr used a declaration-level stub/error pattern for an expression-lowering failure → return unresolved_port(dag, Diagnostic::ResolveError { ... }) directly.
• src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs manual reflected-surface proof lists were updated for SurfaceExpr::Map but not the new SurfaceMapEntry record → add SurfaceMapEntry to the reflection surface, Rust realization, and per-language realization assertions.

Non-blocking — Strengths

• src/v3/std/parse_surface.dag SurfaceMapEntry uses key: String plus key_span, making non-string keys unrepresentable at the parse-surface boundary while preserving key-specific provenance.

⚠️ Two small fixes are needed before this lands cleanly: make the Map expression failure return an unresolved diagnostic port, and include SurfaceMapEntry in the reflected realization proof coverage.

[briansrls]

Review metadata

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

---

Findings

• src/v3/compiler/src/lower.rs:5453-5459 — the SurfaceExpr::Map arm in lower_expr allocates a stub declaration (let stub = alloc_identifier_stub(...)), then immediately let _ = stub; throws away the id, then performs an extra dag.alloc_port(None); whose returned port is silently dropped, before finally returning a separate dag.alloc_port(None). The middle alloc_port is dead — it produces a port nothing references. The stub allocation has a side effect (it pushes a Declaration) but since the id is discarded the only purpose would be to keep declaration counts/shape stable — and the comment doesn't claim that. Per CODING.md's "say what you mean, no dead code" sensibility this is muddled. Either both extras are deliberate (then say so explicitly and use them) or drop them. Non-blocking, implementation-layer.

• parse_parser_body.txt:308-356 / parse_generated.rs:328-376 — parse_map_literal accepts entries separated by whitespace (the comma-bump is conditional). So {"a": 1 "b": 2} parses successfully. The map-literal docstring claims comma-or-whitespace is intentional, but no test in the corpus exercises that, and it diverges from how record literals separate fields. Worth a regression test or tightening to require commas. Non-blocking.

Verdict

APPROVE_WITH_COMMENTS — the substrate addition (Map variant + SurfaceMapEntry) is well-justified: the parse-surface comment includes a thorough coproduct-dissolution receipt covering all four patterns, the scaffold has documented + bounded + named dissolution trigger ("substrate sub-lane lands ValueBody::Map in a follow-up PR"), the lower_data_item arm intentionally mirrors the wildcard with explicit comment so the next sub-lane finds the edit point, and the expression-position case fails closed with a diagnostic. Generated mirrors and corpus manifests are consistent. The two findings above are implementation hygiene, not modeling-discipline violations.

[briansrls]

Violations (could not place on specific lines):

• src/v3/compiler/src/lower.rs:5459 BLOCKING: The unsupported Map expression arm reports a diagnostic but returns a fresh uninferred port instead of an unresolved port tied to that diagnostic, violating FAIL-CLOSED.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: dff8098c · Trigger: schedule
• Thinking: 263s wall

---

Verdict: APPROVE

The diff is narrowly scoped and the new SurfaceExpr::Map substrate shape has a dissolution receipt plus fail-closed handling when it reaches unsupported lowering paths. I don’t see a concrete violation of the pinned modeling, coding, or testing docs.

Verified with:
cargo test -p v3-compiler handwritten_parse_snapshot_matches_manifest
cargo test -p v3-compiler substrate_coproducts_match_runtime_carriers

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-pro
• Commit: dff8098c · Trigger: manual
• Conversation: View conversation

---

1. Story of the diff

This PR stages map-literal bodies into the parse surface without yet making them first-class substrate value bodies. It adds SurfaceMapEntry and a SurfaceExpr::Map variant in src/v3/std/parse_surface.dag, threads that through the generated Rust mirrors and bootstrap fixture, and teaches the parser to route top-level data bodies starting with { "string": ... } into parse_map_literal instead of treating them as opaque brace bodies. Lowering is intentionally conservative: map-bodied data declarations remain ValueBody::Unparsed for now, while all the recursive/lambda/descent walkers learn to traverse only map values, not string keys. The load-bearing boundary is explicit: parser support lands now; ValueBody::Map lowering is named as the sibling substrate sub-lane later at src/v3/compiler/src/lower.rs:2457-2465.

2. Invariant categories

1. LAYER MODEL (substrate vs implementation).

Compliant — this does touch substrate-adjacent parse-surface modeling: src/v3/std/parse_surface.dag:96-101 adds the SurfaceMapEntry carrier and src/v3/std/parse_surface.dag:220-223 adds the Map expression variant. The boundary is carried through the mirror generator at scripts/regen_runtime_mirrors.py:713-714 and scripts/regen_runtime_mirrors.py:764, so the .dag declaration remains the authority rather than a Rust-only parallel shape.

1. INVARIANTS.md + modeling-discipline.md.

Compliant — illegal map-key states are made unrepresentable at the parse-surface boundary: the model uses key: String plus key_span: SourceSpan at src/v3/std/parse_surface.dag:96-100, and the parser only enters the map lane after StringLit lookahead at src/v3/compiler/parse_parser_body.txt:319-321. The unsupported expression-position case is fail-closed through a diagnostic at src/v3/compiler/src/lower.rs:5445-5451, and map data bodies are explicitly kept as Unparsed with the substrate follow-up called out at src/v3/compiler/src/lower.rs:2457-2465.

1. CODING.md.

Finding — NON-BLOCKING. src/v3/compiler/src/lower.rs:5453-5459 allocates an identifier stub, discards it, allocates one fresh port and discards that too, then returns another fresh port:

src/v3/compiler/src/lower.rs:5453: let stub = alloc_identifier_stub(dag, "__map_literal_in_expr_position__", span);

src/v3/compiler/src/lower.rs:5454: dag.alloc_port(None);

src/v3/compiler/src/lower.rs:5458: let _ = stub;

src/v3/compiler/src/lower.rs:5459: dag.alloc_port(None)

This is an implementation-path clarity issue: the branch’s stated contract is “diagnostic + fresh unproduced port,” but it mutates the Dag with extra dead structure before returning. I would either remove the stub and unused allocation, or make the stub the returned fallback if that is the intended recovery carrier.

1. TESTING.md.

Compliant — the diff updates the structural carrier parity test for the new Map variant at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1245-1248, and refreshes the parse-corpus manifest for the changed parse output at src/v3/compiler/tests/integration/parse_corpus_manifest.txt:35. Because the PR does not claim ValueBody::Map semantics yet, I do not see a missing substrate-behavior regression test in this slice.

1. LOCKED DESIGN DECISIONS.

N/A — no locked thesis/design decision is altered in the diff; the Arrow.body / external-realization rules are not touched, and the map-body substrate lowering is explicitly deferred rather than silently redefined.

1. TRACKED vs UNTRACKED DEBT.

Compliant — the scaffold is documented and bounded. src/v3/std/parse_surface.dag:83-84 marks SurfaceMapEntry as a scaffold with the same dissolution trigger as SurfaceExpr, and src/v3/compiler/src/lower.rs:2457-2465 names the present bound and trigger: map parse support exists now, while ValueBody::Map plus lowering lands in the sibling substrate sub-lane.

3. Verdict

APPROVE_WITH_COMMENTS

The modeling boundary is disciplined: the new parse-surface carrier is declared once, mirrored through generation, and explicitly kept out of substrate value-body authority until the follow-up. The only issue I would clean up is the extra dead Dag mutation in the unsupported expression-position lowering path; it is implementation-level and not a substrate blocker.

[briansrls]

Review metadata

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

---

Verdict: APPROVE

The diff is narrowly scoped: adds SurfaceExpr::Map + SurfaceMapEntry substrate, a { String : lookahead lane in parse_data_item, mirrored realizations across rust/go/python specs, recursive walk arms in lower.rs, and a parser-shape test. Modeling discipline is honored:

• Fail-closed at expression position. lower_expr for SurfaceExpr::Map returns unresolved_port with a precise diagnostic (lower.rs:5437-5455) — map literals only legal in top-level data bodies today.
• Illegal keys unrepresentable. key: String on SurfaceMapEntry (parse_surface.dag:80-99) makes non-string keys unconstructable rather than validated; rationale documented inline.
• Coproduct dissolution receipt is thorough. The | Map { ... } block in parse_surface.dag walks all four feedback patterns and explains why Map is genuinely a new variant, not Record/List under a label.
• Tracked bridge for ValueBody. The Some(SurfaceExpr::Map { .. }) => None arm in lower_data_item (lower.rs:2457-2465) routes to ValueBody::Unparsed with a documented, bounded, named trigger (substrate sub-lane PR delivers ValueBody::Map). Explicit arm-over-wildcard is justified as a review-heuristic anchor.
• Recursion arms exhaustive. expr_span, collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees, and refinement_predicate_out_of_fragment all updated. refinement_predicate_out_of_fragment correctly rejects map literals as out-of-fragment.
• Lookahead unambiguous. looks_like_map_literal ({ StringLit :) and the existing looks_like_record_literal ({ Ident :) are mutually exclusive at token b; bare {} falls through to skip_brace_balanced as documented.

No findings against the diff.

[briansrls]

Review metadata

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

---

Verdict: APPROVE

The diff is staged but honest: SurfaceExpr::Map and SurfaceMapEntry are documented as scaffolds with bounded scope, the parser path is fail-closed for non-string keys, and the lowerer explicitly preserves map bodies as Unparsed until the named substrate follow-up rather than pretending support exists. I don’t see a concrete violation of the pinned invariants or coding/testing discipline in the changed lines.

Verification: ran cargo test -p v3-compiler --test integration parse_data_item_with_map_body_emits_surface_expr_map -- --nocapture, passing.

[briansrls]

Both codex BLOCKING findings fixed and pushed (HEAD now at 3020d76c9).

Finding 1 — lower_expr Map arm: declaration-level error pattern → expression-level diagnostic port

Verified. The arm was calling report_declaration_error (declaration-level channel) plus a stub-allocation dance, which routes the diagnostic through the wrong pipeline for an expression-position failure. Fixed at src/v3/compiler/src/lower.rs Map arm in lower_expr: replaced with a single unresolved_port(dag, Diagnostic::ResolveError { ... }) call — the canonical expression-position diagnostic-port pattern (unresolved_port at lower.rs:4681 allocates a fresh port and dag.mark_unresolved attaches the diagnostic to it). Comment block updated to call out why it's unresolved_port here and not report_declaration_error.

Finding 2 — SurfaceMapEntry missing from reflected realization proof coverage

Verified. Three test sites + three per-language realization spec files were missing SurfaceMapEntry coverage:

• m2_substrate_inhabitance_test::reflected_surface_records_have_expected_field_lists (record-fields proof) — added record_fields(&dag, "SurfaceMapEntry") == ["key", "key_span", "value", "span"].
• m2_substrate_inhabitance_test::rust_dag_realizes_reflected_substrate_types — added "SurfaceMapEntry" to the per-name TypeRealization-existence check (Rust realization).
• m2_substrate_inhabitance_test::all_target_languages_realize_reflected_surface_types — added "SurfaceMapEntry" to the per-language realization check (rust + go + python).
• src/v3/spec/rust.dag — new data rust_surface_map_entry: TypeRealization declaration with all four fields realized; import list extended.
• src/v3/spec/go.dag — new data go_surface_map_entry: TypeRealization; import list extended.
• src/v3/spec/python.dag — new data python_surface_map_entry: TypeRealization; import list extended.

Realization-proof coverage now ratchets identically to SurfaceRecordField — adding a new surface record without per-language realization now fails this test class, not just the field-shape spot check.

Strength acknowledgement

Codex's structural-restriction finding on key: String is the same discipline anchor flagged by openai-pro at the brief-update stage; intentionally carried forward into the surface schema.

Gates green at HEAD 3020d76c9

• cargo build -p v3-compiler ✓
• cargo clippy --workspace --all-targets -- -D warnings ✓
• cargo fmt --all --check ✓
• cargo test --workspace --exclude v2-compiler-tests ✓ (525 v3 + others, 0 fail; the 4 directly-impacted tests — substrate_coproducts_match_runtime_carriers, reflected_surface_records_have_expected_field_lists, rust_dag_realizes_reflected_substrate_types, all_target_languages_realize_reflected_surface_types, plus the new parse_data_item_with_map_body_emits_surface_expr_map smoke — all pass)
• regen_v3 6-stage fixed-point converged ✓
• regen_bootstrap --verify clean (committed snapshots match fresh compile) ✓
• parse_corpus_manifest.txt refreshed for the spec-file additions

Standing by for the queued additional review item.

[briansrls]

Adding substrate SurfaceMapEntry without extending the reflected-type realization proofs leaves the reflected fact short of INVARIANTS.md's declaration + realization + generated consumer proof landing rule.

Sha-stale. Already fixed at HEAD 3020d76c9 — same fix as codex BLOCKING #2 from the parallel relay. Verification at current HEAD:

• Declaration: src/v3/std/parse_surface.dag SurfaceMapEntry { key: String, key_span: SourceSpan, value: SurfaceExpr, span: SourceSpan }.
• Realization (per-language):
  • src/v3/spec/rust.dag:729 data rust_surface_map_entry: TypeRealization { target: SurfaceMapEntry, carrier: "parse_surface::SurfaceMapEntry", ... } + import at :85.
  • src/v3/spec/go.dag:667 data go_surface_map_entry: TypeRealization { ... } + import at :35.
  • src/v3/spec/python.dag:411 data python_surface_map_entry: TypeRealization { ... } + import at :41.
• Generated consumer (Rust): src/v3/compiler/src/parse_surface_generated.rs carries pub struct SurfaceMapEntry { pub key, pub key_span, pub value, pub span } regenerated from the .dag authority via scripts/regen_runtime_mirrors.py.
• Proofs (m2_substrate_inhabitance_test.rs):
  • :273 record_fields(&dag, "SurfaceMapEntry") == ["key", "key_span", "value", "span"] (record-shape proof).
  • :1395 rust-realization existence check.
  • :1440 per-language (rust + go + python) realization existence check.

INVARIANTS.md's declaration + realization + generated-consumer + proof landing rule is satisfied at current HEAD. Standing by for the remaining 6 queued items.

[briansrls]

Verified both findings against current HEAD (3020d76c9).

Finding 1 — lower_expr Map arm dead code

Sha-stale. Reviewed sha dff8098c; same dead-code issue codex flagged as BLOCKING in the parallel relay. Already fixed at 3020d76c9 — lower.rs:5437-5462 is now a single unresolved_port(dag, Diagnostic::ResolveError { ... }) call (the expression-position diagnostic-port channel). No more stub allocation, no more discarded alloc_port. Comment block updated to call out why it's unresolved_port and not report_declaration_error.

Finding 2 — parse_map_literal accepts whitespace-separated entries; "diverges from records"

Invalid — does not diverge from records. Verified at HEAD: parse_record_literal (parse_parser_body.txt:290-292) and parse_map_literal (parse_parser_body.txt:360-362) use the identical conditional-comma pattern:

if matches!(self.peek().kind, TokenKind::Comma) {
    self.bump();
}

Both literals accept comma-OR-whitespace separation. {a: 1 b: 2} parses as a record literal today by the same rule that {"a": 1 "b": 2} parses as a map literal. The map-literal docstring's claim is correct and the behavior matches the record-literal precedent verbatim — that's deliberate parity, not divergence.

If the team wants commas required, that's a parser-policy change to both literals (record and map symmetric), out of scope for the parser sub-lane. The brief's STOP gate "lookahead disambiguation reveals genuine ambiguity" doesn't fire here — comma-vs-whitespace is a separator-policy question, not an ambiguity. Best handled as a follow-up if the team wants to tighten record-literal separator policy too; conditional-comma in isolation on map literals would create the divergence the reviewer flagged.

The reviewer's APPROVE_WITH_COMMENTS verdict + acknowledgement of the coproduct-dissolution receipt + bounded-scaffold trigger + intentional lower_data_item arm shape stands. Both implementation-layer findings are addressed (#1 fixed, #2 verified-as-by-design-parity).

Standing by for the remaining 7 queued items.

[briansrls]

The unsupported Map expression arm reports a diagnostic but returns a fresh uninferred port instead of an unresolved port tied to that diagnostic, violating FAIL-CLOSED.

Sha-stale. Already fixed at HEAD 3020d76c9 (codex BLOCKING #1 from the parallel relay). The current arm at src/v3/compiler/src/lower.rs:5437-5457 is:

SurfaceExpr::Map { span, .. } => {
    // ...
    unresolved_port(
        dag,
        Diagnostic::ResolveError {
            name: "map literals are not yet supported in expression position (top-level data declarations only)".to_string(),
            span: span.clone(),
            fixes: Vec::new(),
        },
    )
}

unresolved_port (lower.rs:4681) is exactly the "unresolved port tied to that diagnostic" primitive: it allocates a fresh port and calls dag.mark_unresolved(port, diagnostic) so the diagnostic is structurally bound to the port the caller receives. That's the canonical FAIL-CLOSED pattern used by the rest of lower_expr's error paths (see also lower.rs:4747, :5518, :5529, :5551, :5567, :5600, :5613 — same primitive). The earlier sha (dff8098c) had the bad pattern (report_declaration_error + discarded stub + dead alloc_port); fixed before this relay landed.

Standing by for the remaining 6 queued items.

[briansrls]

Verdict acknowledged: APPROVE_WITH_COMMENTS. The single CODING.md finding is sha-stale (reviewed dff8098c); already fixed at HEAD 3020d76c9 via the codex BLOCKING #1 round.

Current lower.rs:5437-5457 is a single unresolved_port(dag, Diagnostic::ResolveError { ... }) call — the canonical expression-position diagnostic-bound-port primitive (unresolved_port at lower.rs:4681 allocates a port and dag.mark_unresolved's the diagnostic onto it). No stub allocation, no discarded alloc_port, no dead Dag mutation. The reviewer's preferred shape ("remove the stub and unused allocation") is the one that landed.

The other category compliance reads (LAYER MODEL, INVARIANTS, TESTING, LOCKED DECISIONS, TRACKED DEBT) all stand at HEAD — those references are sha-stable.

Standing by for the remaining 5 queued items.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 3020d76c · Trigger: schedule
• Thinking: 60s wall

---

This is a substrate-level change adding SurfaceMapEntry + SurfaceExpr::Map to the parse-surface schema, plus the parser sub-lane (looks_like_map_literal 3-token lookahead → parse_map_literal) for { "key": expr, ... } in top-level data-body position. Lowering of ValueBody::Map is explicitly deferred to a sibling substrate sub-lane.

Modeling discipline check

• The new Map variant carries an explicit coproduct-dissolution receipt walking all four patterns (src/v3/std/parse_surface.dag ~line 188-220) and concludes Record/List/Map are distinct algebraic origins. The receipt is well-grounded — Record.fields carries identifier-labeled product fields, List.elements is a free monoid, Map.entries is PartialFunctionProfile. Approved.
• SurfaceMapEntry's key carrier is String rather than SurfaceLiteral or SurfaceExpr, making non-string keys unrepresentable at the parse boundary (parse_surface.dag:91-99). Illegal-states-unrepresentable applied correctly: the parser only enters the lane when a StringLit token is matched, so key: String is the truthful carrier.
• The deferred ValueBody::Map is a tracked bridge: documented (lower.rs:2457-2463), bounded (parser only emits Map in top-level data-body via looks_like_map_literal lookahead), and trigger named (substrate sub-lane PR). Acceptable.
• Fail-closed in expression position: lower_expr for SurfaceExpr::Map emits ResolveError via unresolved_port (lower.rs:5437-5455). Correct channel choice.
• All recursion/free-var visitors updated consistently (collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees). Exhaustive match maintained.

Findings

None blocking.

Exploratory observations

• parse_map_literal (parse_parser_body.txt:341-359) treats , as optional and accepts trailing commas silently. Fine for now, but if/when error recovery or formatting cares, you'll want a more explicit separator policy (e.g., require comma-or-newline between non-final entries). Not a violation.
• The map-literal lowering in lower_data_item to ValueBody::Unparsed is structurally identical to the wildcard fallback. The arm's only role is to make the substrate sub-lane's edit point grep-visible (the comment says so). That's a legitimate use of an explicit arm — but consider asserting the routing in a test now (e.g., a behavior test that confirms a map-bodied data decl currently produces the same observable diagnostic/fact-set as the wildcard path) so the follow-up PR's "edit this arm" is verified by a flipping test rather than human attention.
• No unit test covers the lower_expr fail-closed path for SurfaceExpr::Map in expression position. Cheap to add and would lock the diagnostic-channel choice (unresolved_port, not report_declaration_error) against a future regression.

Verdict: APPROVE — substrate addition is small, the coproduct-dissolution receipt is rigorous, fail-closed paths are wired correctly in both lowering positions, and the deferred ValueBody::Map is a tracked bridge. The two test gaps above are nice-to-have, not asks.

[briansrls]

APPROVE acknowledged. Verified both exploratory observations against the code; replying with disposition rather than landing tests:

Observation 1 — lower_data_item Map → ValueBody::Unparsed routing test

Disposition: belongs in the substrate sub-lane PR, not this one. The arm currently routes to ValueBody::Unparsed identical to the wildcard fallback; today's observable behavior under user source is "R14 hard-fail diagnostic at the unparsed-scaffold sweep" — already covered by the existing reject_user_unparsed_scaffolds test surface for unparsed bodies. A test that asserts "map-bodied data decl produces the wildcard-equivalent diagnostic" today would be a duplicate of the existing R14 coverage. The substrate sub-lane (t-substrate-valuebody-map-worker.md) is the natural home for the flipping test: when that PR replaces the arm body with ValueBody::Map(entries) lowering, the test that was "asserts Unparsed routing" flips to "asserts Map(7 entries) for kernel_algebra_profile" in the same change. Adding a too-early test here would force the substrate-side PR to delete-and-replace it, which is more churn than coverage.

Observation 2 — lower_expr fail-closed path test

Disposition: not directly testable via user source today; defense-in-depth arm. I attempted to add the test (fn quirk() -> Int = { "a": 1 }) and hit:

Parse(ParseError { message: "expected field label, got StringLit(\"a\")", ... })

The parser blocks one layer earlier — looks_like_map_literal only dispatches inside parse_data_item, NOT inside parse_primary (parse_parser_body.txt:1132 routes { in expression position straight to parse_record_literal, which expects an Ident field label and rejects the StringLit). That's by design per the brief's non-goal: "Map literals inside function bodies, lambda bodies, etc. — out of scope unless the same parse path is structurally reused."

So the lower_expr Map arm is defense-in-depth code for a path the parser does not produce. Today there's no user-source way to drive SurfaceExpr::Map into lower_expr. A meaningful test would have to synthesize a SurfaceExpr::Map directly (skipping the public parse → lower API), which goes against the test-through-public-API discipline and would only assert "the arm we wrote does what its body says."

The defensive arm earns its keep when the parser sub-lane that lifts the non-goal lands — that future PR will both produce the user-source path AND own the test. At that point the regression-lock is genuine; today it would be tautological.

Net

Both observations are correctly identified; both are best resolved by the next PR in the chain rather than this one (substrate sub-lane for #1, expression-position-map sub-lane if/when it lands for #2). HEAD 3020d76c9 stands.

Standing by.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 3020d76c · Trigger: schedule
• Thinking: 225s wall

---

Findings

• BLOCKING src/v3/compiler/src/lower.rs:2465 routes the newly parsed SurfaceExpr::Map to None, which is then stored as ValueBody::Unparsed. That drops the parsed map entries at the parse→lower boundary instead of carrying them forward. This violates docs/modeling-discipline.md Practice 3 / P2 facts-flow-forward. The comment names a future “substrate sub-lane,” but not a bounded dissolution trigger; for substrate shape, either land the ValueBody::Map carrier/lowering here or keep map bodies opaque until that carrier exists.

Verdict: REQUEST_CHANGES. The parser/schema additions are otherwise coherent, and the targeted test passes: cargo test -p v3-compiler --test integration parse_data_item_with_map_body_emits_surface_expr_map.

[briansrls]

Pushing back on the BLOCKING — Practice 3 explicitly permits this shape.

docs/modeling-discipline.md:69-86 (Practice 3, Facts flow forward):

Every piece of structured information produced at one stage of the compiler must be either consumed by the next stage, carried forward as a field on downstream data structures, or explicitly discarded with a comment explaining why the information is no longer needed. Silent drops are violations.

The Map arm at lower.rs:2457-2465 is option 3 — explicitly discarded with a comment. The arm reads:

// Parser sub-lane: `SurfaceExpr::Map` parses today but does NOT
// yet lower to `ValueBody::Map` — the substrate sub-lane lands
// that variant + its lowering in a follow-up PR. Until then,
// map-bodied declarations route to `ValueBody::Unparsed`,
// identical to the wildcard fallback below; the explicit arm
// exists so `feedback_missing_checks_review_heuristic` flags
// this as the substrate sub-lane's edit point rather than
// hiding the variant under a wildcard.
Some(SurfaceExpr::Map { .. }) => None,

That comment names the discard reason (substrate sub-lane lands the carrier), names the dissolution mechanism (explicit arm vs. wildcard so the next-PR edit point is grep-visible), and is bounded by the body_span preserved on ValueBody::Unparsed (entries are recoverable from source — not lost, just not yet structurally extracted at this boundary). This is the canonical "explicit-discard-with-comment" shape Practice 3 permits, not a silent drop.

On the bounded-trigger concern. The reviewer reads the trigger as "future PR will fix it" — too soft. The actual trigger is structural and named at three layers:

1. Brief: docs/briefs/t-substrate-valuebody-map-worker.md is checked-in, dispatchable, and lists the carrier (ValueBody::Map), the lowerer arm replacement, and the consumer audit.
2. Director acceptance: PR docs(briefs): R2 worker-escalation fixes (post-#793-merge) #797 + gunbc Director #799 explicitly vetted this split. gunbc Director #799's body says: "Cross-lane reassignment authorized: wise-boar-480 takes the parser sub-lane (t-substrate-valuebody-map-parser-worker.md) since they already have full investigation context." The split is the directorial decision; this PR executes it.
3. Existing R14 enforcement: any SurfaceExpr::Map body at user range routes to ValueBody::Unparsed and trips reject_user_unparsed_scaffolds (the Class 5 Gap 3 ledger row in ROADMAP.md). Bootstrap-range bodies (kernel_algebra_profile) are tolerated by design until the carrier exists. The substrate sub-lane PR's first observable effect is dropping the R14-gap diagnostic — that's the structural trigger.

Reviewer's "or" alternative — keep map bodies opaque. That's "don't add parse_map_literal at all in this PR" — which means the parser sub-lane has no deliverable, and the substrate sub-lane has nothing to consume on landing. The Director-chosen split (parser-first → substrate-consumes) is precisely the alternative to that opaque-pass approach. Reverting it is a Director-call-revisit, not an in-PR change.

Verdict on this finding: invalid by the strict reading of Practice 3. The arm is the explicit-discard-with-comment option Practice 3 permits, the trigger is bounded by the named-and-dispatchable substrate sub-lane plus existing R14 enforcement, and the split is Director-vetted. HEAD 3020d76c9 stands; not pushing a fix.

If the team wants to revisit the split (atomic parser+substrate vs. sequential), that's a Director-level call that should be raised against #799's reassignment decision, not against this PR's execution of it.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 32c27f74 · Trigger: schedule
• Thinking: 47s wall

---

This is a well-disciplined parser sub-lane PR. The diff cleanly adds SurfaceMapEntry + SurfaceExpr::Map to the substrate, threads the new variant through every SurfaceExpr match site in lower.rs, and explicitly defers ValueBody::Map lowering to a named follow-up sub-lane.

What I verified:

• Coproduct addition is justified. src/v3/std/parse_surface.dag:188-220 carries an explicit, four-pattern dissolution receipt for why Map is not dissolvable into Record/List — exactly what feedback_coproduct_dissolution asks for.
• Tracked bridge, not undocumented debt. The Some(SurfaceExpr::Map { .. }) => None arm in lower.rs:2462 and the lower_expr defensive arm at lower.rs:5437 both have (1) documented intent, (2) bounded scope ("parser sub-lane only"), (3) named dissolution trigger ("substrate sub-lane lands ValueBody::Map"). Per the rubric this is acceptable.
• Fail-closed at the boundaries. refinement_predicate_out_of_fragment (lower.rs:890) flags map literals; lower_expr returns a ResolveError via unresolved_port for expression-position maps; the parser only enters the lane after a 3-token { String : lookahead match.
• Illegal states unrepresentable at the surface. key: String (not SurfaceLiteral or SurfaceExpr) is the right narrowing — non-string keys are unrepresentable rather than runtime-rejected.
• Match-site coverage is exhaustive. Every SurfaceExpr match in lower.rs (refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees, lower_expr, expr_span) and in lower_helpers_generated.rs / lenses / all three target-language realizations (rust/go/python) get the new arm. No wildcard _ hiding the variant.
• Test discipline. parse_data_item_with_map_body_emits_surface_expr_map is a behavior-driven unit test on parser output shape, scoped exactly to what this sub-lane delivers; lowering assertions correctly deferred.

Verdict: APPROVE — the diff is narrowly scoped to the parser sub-lane, the new coproduct variant carries a complete dissolution receipt, deferred lowering is a tracked bridge with all three required properties, and every match site for SurfaceExpr is updated explicitly. No violations observed.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 32c27f74 · Trigger: schedule
• Thinking: 159s wall

---

Verdict: APPROVE

No concrete invariant, modeling-discipline, coding, or testing violations observed in the diff. The temporary SurfaceExpr::Map -> ValueBody::Unparsed path is explicitly documented with a bounded follow-up trigger, so I’m treating it as tracked scaffold rather than untracked bridge debt.

Focused checks passed:
parse_data_item_with_map_body_emits_surface_expr_map, substrate_declares_expected_reflection_surface, and substrate_coproducts_match_runtime_carriers.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 32c27f74 · Trigger: schedule
• Thinking: 310s wall

---

✅ No blocking concerns in the current mixed parser/substrate/spec diff.

[briansrls]

Adding substrate SurfaceMapEntry without extending the reflected-type realization proofs leaves the reflected fact short of INVARIANTS.md's declaration + realization + generated consumer proof landing rule.

Sha-stale relay — same finding the dashboard relayed earlier (timestamp 15:53:30Z at the same parse_surface.dag:99 anchor). Still satisfied at current HEAD 32c27f74f:

• Declaration: src/v3/std/parse_surface.dag SurfaceMapEntry { key: String, key_span: SourceSpan, value: SurfaceExpr, span: SourceSpan }.
• Realization (per-language, all three target languages): src/v3/spec/rust.dag:85,729, src/v3/spec/go.dag:35,667, src/v3/spec/python.dag:41,411 — import + data {lang}_surface_map_entry: TypeRealization declaration with all four fields realized.
• Generated consumer: src/v3/compiler/src/parse_surface_generated.rs carries pub struct SurfaceMapEntry { pub key, pub key_span, pub value, pub span } regenerated from .dag authority via scripts/regen_runtime_mirrors.py:765.
• Proofs: m2_substrate_inhabitance_test.rs:273 (record-shape), :1395 (rust-realization existence), :1440 (per-language existence — rust + go + python).

Codex's later APPROVE pass at 32c27f74 (timestamp 16:43:47Z) verified substrate_declares_expected_reflection_surface + substrate_coproducts_match_runtime_carriers pass. INVARIANTS.md declaration + realization + generated-consumer + proof landing rule satisfied at HEAD.

Standing by for the remaining 1.

[briansrls]

Review metadata

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

---

This PR adds a SurfaceMapEntry substrate type and a SurfaceExpr::Map variant to the parse surface, with parser lookahead ({ String :) to disambiguate map literals from record literals at data-body position. Lowering of Map to ValueBody::Map is explicitly deferred to a sibling substrate sub-lane PR; today the variant routes to ValueBody::Unparsed with an explicit (non-wildcard) match arm, and Map in expression position fails closed via unresolved_port.

Checks against the rubric:

• Substrate addition is well-formed. New SurfaceMapEntry and SurfaceExpr::Map variant land in src/v3/std/parse_surface.dag with a coproduct-dissolution receipt that walks all four feedback patterns (Pattern 1–4) and explains why Map is not collapsible into Record/List. Realizations exist in all three target languages (go.dag, python.dag, rust.dag), and m2_substrate_inhabitance_test.rs is updated to cover the new field/variant set in both directions.
• Scaffold has the three required properties. Documented (clear 🟡 SCAFFOLD block on SurfaceMapEntry), bounded (string-key only; widening explicitly deferred to a future sub-lane), and the dissolution trigger is named by reference to SurfaceExpr's existing trigger. Tracked bridge — accept.
• Fail-closed at the right boundaries. lower_expr for SurfaceExpr::Map emits a diagnostic via unresolved_port (lower.rs:5437–5454) rather than panicking or silently lowering. The Some(SurfaceExpr::Map { .. }) => None arm in lower_data_item (lower.rs:2462) is explicit so the next PR's edit point is visible to the heuristic, not hidden under a wildcard.
• Recursive walkers exhaustively updated. refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees, and expr_span (both hand-written and generated) all gain a Map arm — no _ fallthroughs left to silently mis-handle the new variant.
• Tests. Behavior-driven parser test (parse_data_item_with_map_body_emits_surface_expr_map) asserts the parser-output shape for a realistic kernel_algebra_profile example. Substrate inhabitance assertions extend to the new type. Snapshot manifest updated for the corpus deltas.

One minor thing worth raising as an observation, not a finding:

Exploratory observation. parse_map_literal (parse_parser_body.txt:113–119, mirrored in parse_generated.rs) treats commas as optional — the if matches!(self.peek().kind, TokenKind::Comma) { self.bump(); } will silently accept "a": x "b": y. The doc-comment says "separated by , (or whitespace)" so this is intentional, but it's looser than record-field separation typically is and may surprise users who expect the same discipline as record literals. Worth a short corpus test asserting either the lenient acceptance is intended, or tightening to require commas between non-final entries.

Verdict: APPROVE. Clean substrate addition with a well-documented coproduct receipt, all recursive walkers updated, fail-closed diagnostics at boundaries where the next sub-lane hasn't landed yet, and tests covering both substrate inhabitance and parser output shape.

[briansrls]

APPROVE acknowledged. The exploratory observation on optional commas in parse_map_literal is the same observation from the earlier claude pass (dff8098c, timestamp 15:55:02Z) — disposition unchanged at HEAD ab802908:

parse_record_literal (parse_parser_body.txt:290-292) and parse_map_literal (parse_parser_body.txt:360-362) use byte-identical conditional-comma logic — {a: 1 b: 2} parses today as a record literal by the exact same rule that {"a": 1 "b": 2} parses as a map literal. The "looser than record-field separation typically is" framing is empirically inverted here: the map-literal lane intentionally inherits the existing record-literal separator policy, not loosens it. Tightening would be a parser-policy change to both literals (record and map symmetric), out of scope for the parser sub-lane and a Director-level call.

The earlier disposition stands: if the team wants commas required between non-final entries across {...} literals, the right venue is a follow-up brief that touches both parse_record_literal and parse_map_literal together — asymmetric tightening on map literals alone would create the divergence the observation worries about.

HEAD ab802908 stands; nothing to change.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: ab802908 · Trigger: schedule
• Thinking: 174s wall

---

Verdict: APPROVE

No concrete invariant, coding, or testing violations found in this diff. The map parse-surface additions are reflected through mirrors/target realizations, and the current map-to-Unparsed lowering is documented as staged.

Verified with git diff --check and focused cargo test -p v3-compiler --test integration ... checks for the new parser smoke test, reflection surface, target realizations, and mirror freshness.

[briansrls]

Review metadata

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

---

Verdict: APPROVE

Clean, disciplined sub-lane PR. Adds SurfaceMapEntry substrate type and SurfaceExpr::Map variant, parses string-keyed {...} data bodies, leaves lowering to a named follow-up sub-lane.

What's right:

• Coproduct dissolution receipt is fully written out at src/v3/std/parse_surface.dag:185-201 — all four patterns evaluated and rejected. This is exactly the modeling-discipline rigor the docs ask for when adding a new variant.
• Tracked-debt arm at lower.rs:2465 (Some(SurfaceExpr::Map { .. }) => None) is documented, bounded (falls through to ValueBody::Unparsed), and names its dissolution trigger (substrate sub-lane in follow-up PR). Per the rubric: documented + bounded + named-trigger = accept.
• Fail-closed at expression position (lower.rs:5434-5454) — SurfaceExpr::Map reaching lower_expr produces an unresolved_port diagnostic with a precise message. Correct channel choice (expression-level, not declaration-level), explicitly justified in the comment.
• All walkers updated. The new variant is handled in refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker, collect_recursive_callees, and expr_span — no missing arms hiding behind wildcards.
• All three target languages realized (rust/go/python TypeRealization entries) — the substrate addition is consistently propagated.
• Lookahead disambiguation ({ StringLit : vs { Ident :) is unambiguous and documented at the call site.
• Parser smoke test asserts shape only and matches the explicit "parser sub-lane / substrate sub-lane" split.

Nothing in the diff violates INVARIANTS.md, MODELING.md, CODING.md, or TESTING.md that I can point at.

Exploratory observations (non-asks):

• parse_map_literal treats commas as optional separators (if matches!(... Comma) { bump }), which means {"a": 1 "b": 2} parses without diagnostic. Probably fine — the parse_expr boundary stops at the next StringLit-then-Colon — but worth a deliberate decision when the substrate sub-lane lands. Not a finding.
• The unreachable!("parse_map_literal always returns Map") at the data-body call site is harmless but slightly redundant: you could destructure let SurfaceExpr::Map { span, .. } = body_expr else { unreachable!() } once, or just expose parse_map_literal returning the span directly. Style nit only.

[briansrls]

Review metadata

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

---

Verdict: APPROVE

This looks clean. The new map parse-surface carrier is documented as scaffolded, the SurfaceExpr::Map coproduct addition has a dissolution receipt, lowering either preserves it as tracked Unparsed data-body state or fails closed in expression position, and the generated mirrors/spec realizations stay in sync.

Verified targeted tests pass, including the new parser smoke test, mirror freshness, and substrate reflection checks.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 7b40befa · Trigger: schedule
• Thinking: 170s wall

---

Non-blocking — Strengths

• src/v3/std/parse_surface.dag SurfaceMapEntry keeps non-string keys unrepresentable while preserving key-specific span provenance, and SurfaceExpr::Map carries a dissolution receipt.
• src/v3/compiler/src/lower.rs Map bodies are an explicit tracked scaffold to ValueBody::Unparsed while expression-position maps fail closed through a diagnostic port.

✅ No blocking concerns in the current mixed parser/substrate/spec diff.

[briansrls]

Review metadata

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

---

This PR adds a Map variant to SurfaceExpr plus a SurfaceMapEntry record, with the parser sub-lane wiring (looks_like_map_literal + parse_map_literal) and exhaustive coverage of the new variant across every SurfaceExpr matcher in lower.rs. The substrate-sub-lane (lowering SurfaceExpr::Map → ValueBody::Map) is explicitly deferred and the bridge is named.

What I checked against the rubric

• Coproduct dissolution (modeling-discipline.md): parse_surface.dag:757-790 carries an explicit four-pattern receipt arguing why Map doesn't dissolve into Record / List. Matches the pattern the doc asks for.
• SCAFFOLD with named trigger (modeling-discipline.md): parse_surface.dag:730-731 marks SurfaceMapEntry SCAFFOLD with the same dissolution trigger as SurfaceExpr (recursive list-body emission unblock). Tracked debt, all three properties present.
• Fail-closed (INVARIANTS.md): lower.rs:5408-5428 routes SurfaceExpr::Map in expression position to unresolved_port with a diagnostic rather than silently ignoring; lower_data_item (lower.rs:2456-2461) explicitly arms Some(SurfaceExpr::Map { .. }) => None with a comment justifying why the arm exists rather than a wildcard. Good fail-closed posture for the half-built lane.
• Exhaustive variant coverage (CODING.md / modeling discipline): every SurfaceExpr match in lower.rs (refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees) gets a Map arm. None missed.
• Layer model: SurfaceMapEntry/SurfaceExpr::Map are substrate (live on the parse_surface.dag, mirrored across rust/go/python spec dags). The substrate-side adds and the spec mirrors are all present and consistent.
• Tests (TESTING.md): m2_substrate_inhabitance_test.rs:269 extends the reflection assertion; :1246 extends the coproduct-carriers assertion; :1392/:1437 extend rust/all-language realization; new parse_data_item_with_map_body_emits_surface_expr_map test (:1759-1804) is hermetic, behavior-driven, asserts only what the parser sub-lane produces. Scope matches the PR's claim.

Minor observations (non-blocking, no asks)

• parse_map_literal (parse_parser_body.txt:90-125) treats missing commas as silent entry separators — the loop only optionally consumes ,, so two entries token-adjacent would parse without diagnostic. The comment says "or whitespace," and tokens are whitespace-stripped, so this is intentional. Worth keeping in mind if lenient separation later admits malformed inputs you'd want to reject.
• lower_expr's Map arm (lower.rs:5408-5428) is reachable only if SurfaceExpr::Map ever escapes data-body position, which today's parser prevents. The defensive arm is the right call; just noting it's currently dead.

Verdict: APPROVE — substrate widening done with proper coproduct-dissolution receipt, explicit scaffold + named trigger, fail-closed routing on the half-built substrate sub-lane, and exhaustive matcher coverage. Diff is internally consistent (dag → spec mirrors → generated rust → handwritten lower) and the deferred lowering arm is documented rather than hidden.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: cf7c6bc3 · Trigger: schedule
• Thinking: 175s wall

---

Verdict: APPROVE — This looks clean. The new map parse-surface carrier is documented as a scaffold with a dissolution trigger, facts flow through generated mirrors and target realizations, and unsupported map lowering stays fail-closed as ValueBody::Unparsed.

Verification passed: cargo test -p v3-compiler parse_data_item_with_map_body_emits_surface_expr_map.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: cf7c6bc3 · Trigger: schedule
• Thinking: 260s wall

---

✅ Mixed code/.dag/spec parser-substrate change; no blocking concerns in the current diff.

[briansrls]

Review metadata

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

---

This PR adds a SurfaceMapEntry carrier and SurfaceExpr::Map variant to the parse-surface substrate, wires it through all three target-language realizations, threads it through the lowering recursion (free-vars, recursion check, descent provability, cluster checker, callees), and adds a parser smoke test. Map-bodied data declarations parse but route to ValueBody::Unparsed pending the substrate sub-lane.

The substrate addition carries a documented dissolution trigger ("recursive list-body emission unblock") and an explicit four-pattern coproduct-dissolution receipt in parse_surface.dag showing why Map is not collapsible into Record/List. Expression-position Map fails closed via unresolved_port with a precise diagnostic. The lower_data_item Map arm is explicit (not under wildcard) for review-heuristic discoverability. All SurfaceExpr matches got a Map arm, so exhaustiveness is enforced by rustc.

Exploratory observations (non-blocking):

• parse_map_literal (parse_parser_body.txt:316-329) treats commas as optional — {"a": 1 "b": 2} would parse silently. The doc-comment says "separated by , (or whitespace)"; that's a deliberate choice but slightly looser than record-literal parsing typically is. Worth tightening when the substrate sub-lane lands so consumers can rely on a single canonical surface.
• refinement_predicate_out_of_fragment (lower.rs:890) treats Map as terminal "out of fragment" while sibling List recurses into elements. Defensible today (Map isn't lowerable in expression position yet), but the follow-up that lifts that restriction will need to flip this arm to recurse over entries[].value — easy to forget. A tiny TODO referencing the substrate sub-lane PR would help.
• Some(SurfaceExpr::Map { .. }) => None (lower.rs:2463) duplicates the wildcard fallback intentionally to flag the edit point. Once ValueBody::Map lands, this arm is the only place to swap; the comment is clear about that.

Verdict: APPROVE — substrate addition is well-scaffolded, dissolution receipt is thorough, fail-closed at every pass, recursion is exhaustive, and the named trigger for completion is concrete.

[briansrls]

APPROVE acknowledged. Both observations are non-blocking and explicitly framed as "non-asks"; replying for completeness:

Obs 1 (comma optionality): Same observation as the prior claude rounds (dff8098c, ab802908). Disposition unchanged: byte-identical to parse_record_literal's separator policy at parse_parser_body.txt:290-292. Asymmetric tightening of map-only would create the very divergence the obs worries about; a Director-level cross-literal policy decision is the right venue.

Obs 2 (refinement_predicate_out_of_fragment Map = terminal): The current arm SurfaceExpr::Map { span, .. } => Some(("map literal", span.clone())) is structurally correct, not just defensible-today. Refinement predicates are boolean expressions in where clauses — where x > 0 and similar. A map literal is structurally not a boolean expression, so it remains out-of-fragment under any reasonable extension; this arm wouldn't flip even when the (unscheduled) "lift expression-position-Map non-goal" lane lands, because that lane wouldn't make map literals into boolean expressions either. The List-recurses-into-elements asymmetry is real but reflects a real semantic asymmetry: list elements can be sub-predicates (e.g., a list of conjuncts), map literals cannot. A TODO would mis-document the arm as "this needs revisiting later" when in fact it's at its terminal state.

HEAD a576076b stands.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: a576076b · Trigger: schedule
• Thinking: 182s wall

---

Verdict: APPROVE

Diff is narrowly scoped to parse-surface map literals plus generated mirrors/realizations. I found no concrete violations of the pinned modeling, coding, or testing discipline.

Verified:
git diff --check origin/main...HEAD
cargo test -p v3-compiler parse_data_item_with_map_body_emits_surface_expr_map
cargo test -p v3-compiler m2_substrate_inhabitance_test

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 211e273c · Trigger: schedule
• Thinking: 55s wall

---

This PR adds a parser sub-lane for map-literal data bodies: introduces SurfaceExpr::Map and SurfaceMapEntry to the substrate, parses { "key": expr, ... } in data-body position via 3-token lookahead ({ String :), and routes lowering to ValueBody::Unparsed until the substrate sub-lane lands ValueBody::Map.

Findings: none.

Quick verification:

• Coproduct dissolution receipt is documented in-place at src/v3/std/parse_surface.dag:185-220 walking all four patterns. ✓
• Scaffold marker with named trigger on SurfaceMapEntry (src/v3/std/parse_surface.dag:83-94). ✓
• Substrate authority single-sourced — all three target language TypeRealizations added (src/v3/spec/{rust,python,go}.dag). ✓
• All SurfaceExpr walk sites in src/v3/compiler/src/lower.rs updated exhaustively (refinement_predicate_out_of_fragment, collect_scope_bound_free_vars, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees, collect_lambda_free_names, lower_data_item, lower_expr).
• Fail-closed at lower.rs:5437-5455 — Map in expression position emits an unresolved_port diagnostic rather than silently dropping.
• The Some(SurfaceExpr::Map { .. }) => None, arm at lower.rs:2457-2464 is functionally identical to the wildcard but explicit; the comment names the substrate sub-lane as the dissolution trigger, so this is a tracked bridge (documented + bounded + named follow-up). ✓
• Manifest hash bumps for dsl/std/algebra.dag and dsl/std/types.dag (no source diff there) are explained by the parser now emitting SurfaceExpr::Map for existing string-keyed {...} bodies — legitimate snapshot churn.

Verdict: APPROVE — diff is narrowly scoped, exhaustive across the SurfaceExpr surface, fail-closed in expression position, and the deferred substrate work is explicitly tracked rather than hidden.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: 211e273c · Trigger: schedule
• Thinking: 197s wall

---

Verdict: APPROVE — this looks clean. The new parse-surface Map shape is annotated and mirrored through generated/runtime surfaces, the unlowered map-body bridge is explicit and bounded, and I didn’t see concrete violations of the pinned invariants.

Verified with:
cargo test -p v3-compiler --test integration parse_data_item_with_map_body_emits_surface_expr_map
plus the relevant reflection/realization tests.

[briansrls]

Review metadata

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

---

Findings

Nothing in this diff concretely violates the invariants, modeling discipline, coding style, or testing rules. A few observations rather than findings:

• src/v3/std/parse_surface.dag:83-99,184-217 — the new SurfaceMapEntry record + SurfaceExpr::Map variant ship with an explicit coproduct-dissolution receipt walking all four patterns and a key-shape rationale. Substrate-discipline-by-the-book.
• src/v3/compiler/src/lower.rs:2457-2463 — the Some(SurfaceExpr::Map { .. }) => None arm is a tracked bridge: documented, bounded ("identical to wildcard fallback below"), and named trigger ("substrate sub-lane lands ValueBody::Map in a follow-up PR"). Acceptable scaffold.
• src/v3/compiler/src/lower.rs:5437-5456 — expression-position Map is fail-closed via unresolved_port with a clear diagnostic. Good.
• src/v3/compiler/src/parse_generated.rs:251-261, 341-371 — 3-token lookahead is unambiguous against looks_like_record_literal (Ident vs StringLit), and the order-of-checks is correct (record first, map second, then skip_brace_balanced). No ambiguity window.
• All recursion sites (collect_scope_bound_free_vars, collect_lambda_free_names, is_recursive, descent_provable, ClusterDescentChecker::expr, collect_recursive_callees, expr_span, refinement_predicate_out_of_fragment) are updated. No silent wildcard hiding the new variant.
• Smoke test at m2_substrate_inhabitance_test.rs:1762-1804 asserts parser-output shape (entries + keys + value variants) without overreaching into lowering. Behavior-driven, scoped to this PR's deliverable.

Exploratory observations

• refinement_predicate_out_of_fragment for Map returns Some(("map literal", span)) at the top level, while sibling Record/List arms recurse into children. The fail-closed posture is fine, but if a future PR ever wants map literals usable inside refinements (unlikely), this will need to flip to recurse-into-values like the others. Worth a one-line comment, but not blocking.
• Empty-map syntax {} deferral is documented, but the rationale lives only in looks_like_map_literal's doc-comment. If a consumer later writes data x: Map<…> = {} expecting a map, they'll get the skip_brace_balanced scaffolded-body lane silently. Probably fine for now (no consumer exists), but a targeted diagnostic when an empty {} body sits under a Map<…> ascription would close the trap.

Verdict

APPROVE — clean, narrowly-scoped substrate addition. Coproduct dissolution receipt provided, all recursion sites updated, lowering deferral is a properly tracked bridge, fail-closed at the expression boundary, and a focused parser-shape test ships with it.

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: c3d82b6f · Trigger: schedule
• Thinking: 180s wall

---

Verdict: APPROVE — diff is narrowly scoped and the staged SurfaceExpr::Map work is documented with a bounded follow-up path to ValueBody::Map. I didn’t find concrete invariant, coding, or testing violations in the changed lines.

Verified with git diff --check, cargo test -p v3-compiler parse_data_item_with_map_body_emits_surface_expr_map, and cargo test -p v3-compiler runtime_mirror_snapshots_are_fresh.

[briansrls]

Review metadata

• Provider / model: openai-pro / gpt-5-5-pro
• Commit: 3a7ae1b1 · Trigger: manual
• Conversation: View conversation

---

Loop summary

27 review entries over ~3h42m: from 2026-04-25 15:53:29Z to 2026-04-25 19:35:25Z. I group them into 4 substantive rounds:

1. Initial blocker round: lower_expr used the wrong failure shape, and SurfaceMapEntry was missing from reflection/realization proof coverage.
2. Fix-and-approval round: reviewers confirm SurfaceExpr::Map / SurfaceMapEntry, parser lookahead, mirrors, lower walkers, and parser-shape test.
3. Boundary-dispute round: one codex review objects that SurfaceExpr::Map → ValueBody::Unparsed drops parsed facts; later reviews treat the same shape as a tracked bridge with a named ValueBody::Map follow-up.
4. Stability round: repeated approvals plus the same non-blocking observations: optional commas, empty map deferral, and future refinement/map-lowering flip.

M commits: exact commit count is not recoverable from the attached ALL_REVIEWS.txt + current diff because they do not include SHAs or a commit list. The visible review states show at least 2 diff states: an initial blocking state and the current fixed/tracked-bridge state.

K/L review count: 15 codex reviews; 12 non-codex/browser-style reviews if grouping the 11 Claude entries + 1 OpenAI-pro entry as the non-codex review bucket. The review headers themselves are api-cli:*, so that grouping is by actor label, not by UI metadata. The initial blocker and late final-approval shape are both visible in the review log. chatgpt-review-4e01de18-c136-4d…

chatgpt-review-4e01de18-c136-4d…

Forward progress evidence

This loop made real early progress.

The first codex blocker was fixed: the expression-position Map path moved away from declaration-level stub/dead structure and toward the expression diagnostic channel via unresolved_port; the missing SurfaceMapEntry reflection/realization coverage was also added. Those were concrete corrections, not review churn. chatgpt-review-4e01de18-c136-4d…

A real parser consumer landed: parse_data_item_with_map_body_emits_surface_expr_map asserts that a kernel_algebra_profile-shaped declaration produces SurfaceExpr::Map entries and values, while staying scoped to the parser sub-lane rather than pretending ValueBody::Map exists. The substrate inhabitance/reflection and target realization checks were also extended. chatgpt-review-4e01de18-c136-4d…

chatgpt-review-4e01de18-c136-4d…

The scaffold is accounted for. Reviews repeatedly verify that SurfaceMapEntry is marked as a 🟡 SCAFFOLD, SurfaceExpr::Map carries a coproduct-dissolution receipt, lower_data_item has an explicit SurfaceExpr::Map → ValueBody::Unparsed bridge, and the bridge names the substrate sub-lane that will land ValueBody::Map. That matches the project rule that scaffolds need named dissolution triggers. chatgpt-review-4e01de18-c136-4d…

chatgpt-review-ffe49878-b956-42…

The invariant work mostly applied existing invariants rather than creating new ones: illegal string-key states are made unrepresentable, all SurfaceExpr match sites are exhaustively updated, and expression-position maps fail closed. The coproduct-dissolution receipt is structural and cross-layer-aware enough; this did not devolve into per-layer one-off rules. chatgpt-review-4e01de18-c136-4d…

chatgpt-review-508f13a2-40a2-44…

Debt accumulation evidence

There is net new debt, but it is mostly tracked.

The PR adds one durable scaffold: SurfaceMapEntry / SurfaceExpr::Map exists before the real downstream semantic carrier, ValueBody::Map. That means parse can now produce a structured map body that lower intentionally preserves only as ValueBody::Unparsed. This is acceptable only because the bridge is explicit, bounded, and tied to the named substrate sub-lane. It is still debt. chatgpt-review-4e01de18-c136-4d…

No durable scaffold was dissolved in this PR. The loop paid off local review findings, but it did not remove an existing scaffold or complete the map value-body chain. Under the project’s own boundary rule, a boundary is fully landed only when declaration, realization, and generated consumer proof exist; declaration plus a hand-written consumer is still staging. This PR has parser and proof coverage, but not the full ValueBody::Map semantic consumer. chatgpt-review-ffe49878-b956-42…

There is one recurring observation that the loop keeps rediscovering instead of resolving: parse_map_literal accepts whitespace-separated entries because comma consumption is optional. Multiple reviews flag it as non-blocking. That is not worth another review round, but it should be folded into the follow-up map-substrate artifact so the separator policy is decided once rather than rediscovered. chatgpt-review-4e01de18-c136-4d…

chatgpt-review-4e01de18-c136-4d…

There is also a small “future flip” debt: refinement_predicate_out_of_fragment currently treats Map as terminal out-of-fragment, and the lower_data_item arm must be replaced when ValueBody::Map lands. Reviewers saw this and accepted it because the comment points at the edit point. chatgpt-review-4e01de18-c136-4d…

Cheating signal

Low cheating signal. The implementer is not hiding the compromise.

The current compromise is written down in exactly the places reviewers would look: SurfaceMapEntry is scaffold-labeled, the lower_data_item arm says it is identical to the wildcard fallback until ValueBody::Map, and expression-position maps fail through a diagnostic rather than a plausible fabricated value. That is accounting, not quiet “good enough for now.” chatgpt-review-4e01de18-c136-4d…

The most recent “fixes” are not really fixes anymore; they are repeated review confirmations. After ~16:41Z, the loop is mostly proving the same facts again: mirrors/realizations updated, parser test present, walkers exhaustive, bridge tracked, fail-closed boundary intact. More iteration on this PR is now negative marginal value unless it starts the actual ValueBody::Map work. chatgpt-review-4e01de18-c136-4d…

Path to convergence

Do not keep iterating on this PR just to get another approval. The next useful work is a follow-up artifact, not another review loop.

Acceptable carried debt:

1. SurfaceExpr::Map → ValueBody::Unparsed in lower_data_item.
2. String-key-only SurfaceMapEntry.
3. Expression-position Map rejected via unresolved_port.
4. Optional-comma / whitespace separator behavior, pending an explicit syntax decision.
5. Empty map syntax deferral, pending the same map-substrate lane.

Follow-up artifact required: a Map ValueBody substrate follow-up PR or ROADMAP/debt row with these acceptance criteria:

• Add the real ValueBody::Map carrier, or explicitly decide the structural equivalent.
• Lower SurfaceExpr::Map into that carrier without dropping entries, key, key_span, value, or span.
• Replace the explicit Some(SurfaceExpr::Map { .. }) => None arm with real lowering.
• Add a consumer test that proves a map-bodied data declaration survives parse→lower as structured map substrate, not Unparsed.
• Decide and test map entry separator policy: require commas, accept whitespace intentionally with a corpus test, or diagnose missing separators.
• Decide empty-map syntax under Map<…> ascription.
• Revisit the refinement/expression-position arms only if map literals become valid outside top-level data-body position.
• Remove or update the scaffold comments when the trigger fires.

Meta-verdict

⚖️ SHIP_WITH_DEBT — diminishing returns, accept tracked debt, merge and address in follow-up PRs.

The loop is no longer finding structurally new information. It already fixed the initial blockers, added a parser consumer, extended reflection/realization coverage, and made the remaining compromise explicit. The debt is real, but it is named and bounded. More review on this PR would mostly re-litigate the same ValueBody::Map deferral; convergence now requires the follow-up substrate lane, not another round here.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 3a7ae1b1 · Trigger: schedule
• Thinking: 280s wall

---

Non-blocking — Strengths

• src/v3/std/parse_surface.dag The Map substrate addition is documented, bounded to the parser sub-lane, and carries a coproduct dissolution receipt.
• src/v3/compiler/src/lower.rs Map values flow through the touched lowering visitors, while user-range lowering remains fail-closed through the existing Unparsed scaffold rejection path.

✅ No blocking concerns in the current diff.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 805070c9 · Trigger: schedule
• Thinking: 229s wall

---

✅ No blocking concerns in the current diff; the remaining Map-to-ValueBody gap is explicit, bounded, and named as follow-up substrate work.