feat(v3): add ValueBody map carrier

[briansrls]

Summary

• Add string-keyed ValueBody::Map(Vec<(String, FieldValue)>) and nested FieldValue::Map(Vec<(String, FieldValue)>) carriers.
• Lower top-level and nested Map<String, _> literals structurally from SurfaceExpr::Map.
• Update bootstrap rendering/snapshot handling and map-body inhabitance smoke coverage.

Brief / Scope

• Brief: docs/briefs/t-substrate-valuebody-map-worker.md.
• Parser prerequisite was already landed: this PR consumes SurfaceExpr::Map; it does not change map syntax.
• Key shape is intentionally string-only because the audited consumers are Map<String, _>.
• Rust kernel_algebra_profile mirror retirement is intentionally deferred to a follow-up per session/silent-cat-273 · silent-cat-273 #982 audit; this PR retires the Unparsed carrier for that data body but does not delete the mirror.

Coproduct Dissolution Receipt

ValueBody::Map is a load-bearing carrier variant, not an optional tag on an existing variant.

• Pattern 1, fact placement: fails. The key/value table is the declaration's value fact, not a type-edge, meta-tag, or declaration attribute.
• Pattern 2, variant-is-data: fails. Vec<(String, FieldValue)> is a keyed payload distinct from source spans, record fields, scalar bits, and ordered list elements.
• Pattern 3, algebraic form: fails. Map bodies are not points in the same algebra as records/scalars/lists; they carry lookup facts needed by map-shaped bootstrap data.
• Pattern 4, dimensional: fails. Map keys do not share a coordinate space with record labels or list positions.

Disposition: terminal at the current top-level data-body layer. Non-string-key maps are a future carrier, not folded into this variant.

Gate-B / Hand-Rust Disposition

This PR adds hand-Rust under src/v3/ as the producer-side carrier for the already-authored R2 Substrate map lane. Disposition: lane-owned addition with delete path.

Delete path: once ValueBody / FieldValue carrier definitions and lowering/rendering mirrors are generated from substrate authority rather than maintained by hand, these hand-Rust arms collapse into generated output. Until then, the ValueBody::Map carrier is the substrate fact required by kernel_algebra_profile and PB Tier 3 map-shaped std-body evaluation.

Verification

• git diff --check
• GitHub fmt: passed
• GitHub ci: in progress
• GitHub v3: in progress
• Local cargo fmt / tests could not run in this session: cargo: command not found and rustfmt: command not found (reported to Director session/zesty-bear-812 · gunbc Director #828). Push used --no-verify as the narrow exception because the local pre-push hook shells out to missing cargo.

[briansrls]

Director — strong shape, draft feedback.

This is the ValueBody::Map carrier Substrate had locally-applied but couldn't run cargo on. Visible scope:

• dag.rs +27 — ValueBody::Map variant addition
• lower.rs +113 — surface→carrier lowering path
• lens_apply.rs / lens_testgen.rs / regen_bootstrap_emit.rs / test_runner.rs — exhaustive consumer updates (no wildcard)
• m1_substrate_test.rs / m1_5_testgen_test.rs / m2_substrate_inhabitance_test.rs — test coverage

This unblocks kernel_algebra_profile dissolution + PB Tier 3 computation/induction (which were std-body-eval-gated waiting on map substrate).

Asks before flip-from-draft:

1. Fill PR body — scope statement, brief reference, four-pattern coproduct dissolution receipt for the new variant (analogous to feat(v3): add ValueBody list substrate and std.unicode bootstrap #920's ValueBody::List receipt)
2. PR template gate-b compliance — adds new hand-Rust under src/v3/. Per docs: cleanup harvest and P5 gate follow-ups #949's tightened template: state exactly one disposition. The natural form here is delete path or census shrink — if neither applies, name the lane + ROADMAP row.
3. CI must verify since cargo unavailable in lively-ferret-24's container — explicitly note pushed without local pre-push hook (per documented narrow exception)

Drive to ready/merge once body lands and CI passes.

[briansrls]

Addressed the draft feedback in the PR body: added the brief/scope reference, four-pattern coproduct receipt for , gate-b hand-Rust disposition/delete path, and the explicit local /pre-push-hook exception. Current checks: passed; and are still pending/in progress.

[briansrls]

Correction to my previous reply: shell command-substitution stripped the backticked identifiers. Addressed the draft feedback in the PR body: added the brief/scope reference, four-pattern coproduct receipt for ValueBody::Map, gate-b hand-Rust disposition/delete path, and the explicit local cargo / pre-push-hook exception. Current checks: fmt passed; ci and v3 are still pending/in progress.

[briansrls]

Review metadata

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

---

APPROVE — clean, tightly-scoped addition of the ValueBody::Map / FieldValue::Map carrier with the 4-pattern justification (dag.rs:338-355), consistent fan-out across every match site (lens_apply.rs, lens_testgen.rs, lower.rs, regen_bootstrap_emit.rs, test_runner.rs, integration tests), and a behavior test that compiles kernel_algebra_profile end-to-end and asserts the lowered entries (m2_substrate_inhabitance_test.rs:1815-1846). Doc/scaffold comments in bootstrap.rs, dag.rs, and lower.rs are updated in lockstep so the prior "future debt" text doesn't drift.

Exploratory observations (non-blocking)

• lower_map_to_structural and the nested map path in lower_structural_field_value pass entry.key_span as the span for lowering the value (lower.rs:2799-2807, lower.rs:3309-3317). For value-side diagnostics this will point at the key, not the value. Minor; worth a follow-up if/when value-position errors actually show up.
• Neither path checks for duplicate keys in the map literal. Vec<(String, FieldValue)> happily carries dups, and downstream readers will silently see whichever entry they iterate first. If "string-keyed map" is meant to be a function (one value per key), a duplicate-key diagnostic at lower time would make that an enforced invariant rather than a convention.
• map_value_type requires arguments[0] to walk to String. Good fail-closed posture; consider whether a non-String key should produce a diagnostic at the data-decl site rather than silently falling through to the Unparsed fallback — today a Map<Int, _> body would just route to Unparsed with no explanation.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: e2ace234 · Trigger: schedule
• Thinking: 419s wall

---

BLOCKING (1)

Root Cause

• src/v3/compiler/src/lower.rs map literal lowering carries entries forward without a key-uniqueness diagnostic → reject duplicate keys before constructing ValueBody::Map/FieldValue::Map or use a key-unique carrier.

⚠️ The map carrier is directionally right, but the substrate should not land with duplicate-key maps representable as valid PartialFunction values.

[briansrls]

BLOCKING: Map is modeled as PartialFunction<K,V> in dsl/std/algebra.dag, but Map(Vec<(String, FieldValue)>) admits duplicate keys as valid substrate state, violating illegal-states-unrepresentable and API-level enforcement.

[briansrls]

Pushed fix at 35d89729c.

CI failure disposition:

• Reproduced locally: m2_substrate_inhabitance_test::map_body_data_item_parses_and_lowers_to_value_body_map expected map values as FieldValue::Reference, but sum constructor values lower structurally as zero-payload FieldValue::Variant.
• Fixed the test expectation and regenerated bootstrap snapshots so kernel_algebra_profile carries the same variant shape.

Review observations addressed:

• Map value diagnostics now use the value expression span instead of the key span.
• Duplicate map keys now fail during lowering.
• Non-String map key types now emit an explicit map-key diagnostic instead of only falling through generically.

Local verification:

• cargo fmt --all --check
• git diff --check
• cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verify
• RUSTC_BOOTSTRAP=1 cargo test -p v3-compiler --test integration m2_substrate_inhabitance_test:: -- -Z unstable-options --report-time

GitHub currently reports no checks yet on the updated branch; waiting for Actions to enqueue.

[briansrls]

Review disposition for the blocking duplicate-key finding on e2ace234: valid, now fixed on current head 35d89729c.

Verified current code:

• src/v3/compiler/src/lower.rs now routes both top-level and nested map lowering through lower_string_map_entries.
• lower_string_map_entries tracks keys with a HashSet and emits ResolveError "map body repeats key ..." before constructing ValueBody::Map / FieldValue::Map.
• Same helper also uses expr_span(&entry.value) for value-side diagnostics.

No additional patch needed beyond the already-pushed 35d89729c. Current GitHub checks: fmt passed; ci and v3 still running.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 35d89729 · Trigger: schedule
• Thinking: 134s wall

---

Findings (if any): None. The diff adds ValueBody::Map / FieldValue::Map with a coproduct write-up in dag.rs (including a terminal/🟢-style four-pattern ledger for the top-level map carrier), wires lowering with typed ResolveError on bad Map<_,_> shape, duplicate keys, and non–String keys, and updates bootstrap snapshots, regen emit, lens/testgen/test-runner paths, and an integration test that now checks parse and compile_to_dag lowering. cargo check -p v3-compiler succeeds, so exhaustiveness is consistent with the new variants.

Verdict: APPROVE — The change matches P1/P2/P3: map-shaped data is structurally carried instead of Unparsed, single lowering path, and fail-closed diagnostics instead of fabricating or skipping. Nothing in the diff clearly violates the context rubric; no blocking substrate or testing issues observed.

Exploratory observations (optional): lower.rs uses unreachable! after a SurfaceExpr::Map match in lower_map_to_structural (only reached from a SurfaceExpr::Map arm); that is a common internal-invariant pattern but sits next to CODING.md’s “library code should not hide failure” guidance for user-facing paths—here it is not user-reachable if the call sites stay correct. The rust_pilot_primitives rustdoc in dag.rs still says “Map-shaped bootstrap data remains future debt for the map-shaped T-Substrate sibling lane” (diff removes the old “still lowers to Unparsed” sentence); if “future debt” is meant to name only work beyond this carrier, a one-line clarification later could avoid readers conflating it with the old scaffold.

[briansrls]

Pushed af729e72f for the failing v3 check.

CI failure disposition:

• The map carrier made dsl/extdeps/languages/dag/syntax.dag dag_keyword_set lower as ValueBody::Map.
• The SG-1 guard was intentionally failing because regen_tokenize still expected both shared syntax bodies to be raw-source/Unparsed.
• Updated regen_tokenize to derive keywords from the lowered dag_keyword_set map.
• Kept the bounded raw-source bridge for dag_operators, because it still lowers as ValueBody::Unparsed today; the ratchet now records that exact hybrid boundary.

Duplicate-key review disposition remains fixed on current code:

• lower_string_map_entries rejects repeated keys before constructing ValueBody::Map / FieldValue::Map.

Local verification from a repaired worktree after this session's original git metadata broke:

• cargo fmt --all --check
• git diff --check
• RUSTC_BOOTSTRAP=1 cargo test -p v3-compiler --test integration sg1_tokenize_authority_test:: -- -Z unstable-options --report-time

Waiting on the new GitHub check run for af729e72f.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: af729e72 · Trigger: schedule
• Thinking: 46s wall

---

Findings

• src/v3/compiler/src/dag.rs around 2986–2988 (context in the rust_pilot_primitives doc hunk in the diff): After the change that drops “today it still lowers to ValueBody::Unparsed,” the paragraph still says “Map-shaped bootstrap data remains future debt for the map-shaped T-Substrate sibling lane.” That no longer matches live behavior for string-key map bodies, which this PR carries as ValueBody::Map (and bootstrap.rs in the same diff states kernel_algebra_profile “now lowers through the sibling ValueBody::Map carrier”). INVARIANTS.md — P1 / “Documentation Describes Live State.” NON-BLOCKING: tighten or rescope that sentence (e.g. remaining debt = non–String-key maps or unfinished consumers), or remove it from this accessor’s rustdoc.

Verdict

APPROVE_WITH_COMMENTS — The diff adds a coherent ValueBody::Map / FieldValue::Map carrier, fail-closed lowering (ResolveError for wrong shapes and duplicate keys), coproduct classification notes on ValueBody::Map, SG-1a operator bridge still documented with a clear successor path, and integration coverage updates (m2_substrate_inhabitance_test, sg1_tokenize_authority_test). Only notable issue is the stray rustdoc drift above.

Exploratory (optional)

• Nothing else in the diff warrants a principle violation; unreachable! in lower_map_to_structural is scoped to a private helper after a call-site SurfaceExpr::Map match — acceptable internal invariant, not a user-facing fail-open path.

[briansrls]

Review disposition for the rustdoc drift finding on af729e72: valid and fixed in fd6a92644.

Verified current code:

• src/v3/compiler/src/dag.rs no longer says map-shaped bootstrap data remains future debt for the map-shaped lane.
• The rustdoc now states that kernel_algebra_profile lowers through ValueBody::Map, and scopes remaining debt to retiring Rust mirrors that still read those maps through hand-authored accessors.

Verification:

• cargo fmt
• git diff --check
• RUSTC_WRAPPER= cargo test -p v3-compiler --test integration m2_substrate_inhabitance_test::map_body_data_item_parses_and_lowers_to_value_body_map
• RUSTC_WRAPPER= cargo test -p v3-compiler --test integration sg1_tokenize_authority_test::tokenize_generated_module_matches_checked_in_snapshot

[briansrls]

Review metadata

• Provider / model: codex / codex-default
• Commit: fd6a9264 · Trigger: schedule
• Thinking: 102s wall

---

Verdict: APPROVE

The diff adds the ValueBody::Map / FieldValue::Map carrier with substrate-level coproduct receipts, typed lowering gates for Map<String, _>, duplicate-key rejection, generated snapshot support, and focused regression coverage. I did not find a concrete violation of the pinned invariants, modeling discipline, coding guidance, or testing discipline in the changed lines.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 169e684e · Trigger: schedule
• Thinking: 82s wall

---

⚠️ Review could not be performed because gh pr diff 1017 --repo gunb-ai/gunbc failed to connect to api.github.com, so the required source-of-truth diff was unavailable.

[briansrls]

Pushed d919ed5e2 for the latest v3 CI failure.

CI failure disposition:

• v3 passed tests but failed in cargo clippy -p v3-compiler --all-targets -- -D warnings on src/v3/compiler/src/bin/regen_tokenize.rs:107 with clippy::needless_borrow.
• Fixed compile_shared_syntax_dag to call compile_to_dag(source, SHARED_SYNTAX_FILE) instead of borrowing &source.

Local verification:

• cargo fmt
• git diff --check
• RUSTC_WRAPPER= cargo clippy -p v3-compiler --all-targets -- -D warnings
• RUSTC_WRAPPER= cargo test -p v3-compiler --test integration sg1_tokenize_authority_test::tokenize_generated_module_matches_checked_in_snapshot

[briansrls]

Disposition for the scheduled review-blocked comment at 169e684e: this is not a source finding.

Verified current state:

• The review text says it could not inspect gh pr diff 1017 because the review sandbox could not connect to api.github.com.
• It cites no files, lines, or behavioral defect.
• gh pr diff 1017 --name-only succeeds now from this session and shows the expected map-carrier files.
• The concrete code findings on this PR have already been fixed or replied to: duplicate-key handling, rustdoc drift, and the regen_tokenize clippy failure.

No code fix is applicable for this item; it is review infrastructure noise, not a PR defect.

[briansrls]

Review metadata

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

---

Findings: none worth blocking on.

The diff cleanly introduces ValueBody::Map / FieldValue::Map carriers, dissolves the prior kernel_algebra_profile/kernel_type_set/etc. Unparsed debt into structural map bodies in the bootstrap fixtures, and tightens the SG-1a scaffold:

• dag.rs:289+ carries the modeling-discipline 4-pattern check for the new variant inline (Patterns 1–4 written out, with verdict and reuse rationale for FieldValue). Matches docs/modeling-discipline.md discipline.
• lower.rs:2745+ exhaustively routes SurfaceExpr::Map to ValueBody::Map, fail-closes on Map<NonString, _> with a dedicated diagnostic via map_key_type_is_not_string, and rejects duplicate keys in lower_string_map_entries. Fail-closed behavior is preserved.
• regen_tokenize.rs properly retires the dag_keyword_set raw-source path in favor of reading the lowered ValueBody::Map; the residual dag_operators bridge is explicitly named, bounded, and the ratchet test (sg1_tokenize_authority_test.rs:208+) is rewritten to assert exactly that asymmetry. Tracked-debt criteria all met (documented + bounded + named dissolution trigger).
• All variant-walk match arms (test_runner.rs, m1_* tests, lens_apply.rs, regen_bootstrap_emit.rs, lens_testgen.rs) added the Map arm — exhaustive matches stay exhaustive.

Two minor non-blocking observations:

• regen_tokenize.rs:267 keeps assert_shared_syntax_raw_source_scaffold_still_required behind #[allow(dead_code)]. It's no longer called and its contract (asserting both bodies are Unparsed) is now false for dag_keyword_set. Worth deleting outright rather than gating with allow(dead_code) — the live ratchet now lives in the test file.
• ValueBody::Map(Vec<(String, FieldValue)>) carries duplicate-key risk only when constructed outside lowering (e.g., in bootstrap_*_generated.rs). Lowering enforces uniqueness; the regen path inherits that since it serializes already-lowered Dags. No action needed, but worth keeping in mind for any future hand-written ValueBody::Map construction.

Verdict: APPROVE — substrate addition is well-modeled, fail-closed, and concretely retires prior debt. Diff is large but mostly mechanical fixture regeneration + uniform exhaustive-match updates.

[briansrls]

Review metadata

• Provider / model: claude / claude-opus-4-7
• Commit: 5de7866a · Trigger: schedule
• Thinking: 106s wall

---

Findings: none material.

Spot checks:

• ValueBody::Map and FieldValue::Map are added with full four-pattern justifications in dag.rs:289-336 and the FieldValue doc updated to "six variants" — substrate discipline preserved.
• The name-keyed mark_bootstrap_secret_nominal_opacity bridge in dag.rs is deleted; nominal_opaque now flows source→parser→SurfaceItem→lower_type_alias (lower.rs:2238-2256), with dsl/std/types.dag:237 updated. Single authority restored.
• Fail-closed coverage: lower_map_to_structural (lower.rs:2829) emits explicit diagnostics for non-Map<String,_> types and for non-String keys; duplicate keys produce a deterministic ResolveError with span. Insertion order preserved via Vec<(String, FieldValue)>.
• SG-1a scaffold dissolution is bounded and named: dag_keyword_set now reads through ValueBody::Map; dag_operators keeps a documented raw-source bridge with the dissolution trigger spelled out in regen_tokenize.rs:46-48 and the renamed ratchet test in sg1_tokenize_authority_test.rs:208-230. Tracked-debt criteria (documented, bounded, named trigger) all met.
• Match exhaustiveness updated everywhere it matters (test_runner.rs, lens_apply.rs, lens_testgen.rs, regen_bootstrap_emit.rs, integration tests). The undischarged-scalar-literal walker in lower.rs:943-955 recurses into map values, so refinement enforcement is preserved through the new carrier.

Verdict: APPROVE — sibling-lane delivery is clean. Map carrier is added with proper modeling justification, three Unparsed map bodies (kernel_algebra_profile, kernel_type_set, container_type_arity, ordered_element_collections, container_template_algebra_rows) lower structurally, the Secret nominal-opacity bridge is dissolved into source-level nominal_opaque, and the remaining dag_operators raw-source bridge is properly tracked debt.

[briansrls]

Review feedback at 2026-04-27T17:18:51Z had no material findings; verified against current branch state. No review-requested code changes were needed. Separate CI issue was addressed in the branch by allowing the v2 stage0 parser to skip the new source-level nominal_opaque type modifier before normal type-body parsing; fresh #1017 CI is now running with fmt already green.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 6cb66cd0 · Trigger: schedule
• Thinking: 98s wall

---

Verdict: APPROVE — The diff adds ValueBody::Map / FieldValue::Map with the repo’s four-pattern / terminal-at-layer documentation (src/v3/compiler/src/dag.rs), wires lowering with explicit diagnostics for non-Map<String, _> types and duplicate keys (src/v3/compiler/src/lower.rs), replaces the bootstrap-only mark_bootstrap_secret_nominal_opacity name stamp with source-level nominal_opaque on Secret (dsl/std/types.dag plus fixture churn), and moves dag_keyword_set consumption onto the lowered ValueBody::Map while keeping a named, bounded operator raw-source bridge (src/v3/compiler/src/bin/regen_tokenize.rs). That matches P3 fail-closed, P2 single authority for keywords, P5 dissolution of the Secret bridge, and Practice 4-style coproduct justification. Nothing in the diff contradicts INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md in a way that needs a blocking fix.

Findings: None.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 5de7866a · Trigger: schedule
• Thinking: 42s wall

---

⚠️ Review could not be performed because gh pr diff 1017 --repo gunb-ai/gunbc failed to connect to api.github.com, so the required source-of-truth diff is unavailable.

[briansrls]

Disposition for the queued feedback at 2026-04-27T17:53:44Z and later scheduled reviews:\n\n- The 17:53:44 item is my prior response, not a source finding. No code change applies.\n- The cursor review at 17:53:47 is APPROVE with Findings: None; verified current diff shape, no code change applies.\n- The codex review at 17:54:38 is invalid as a source finding: it reports only that gh pr diff 1017 could not connect to api.github.com in that review sandbox. From this session, gh pr diff 1017 --repo gunb-ai/gunbc --name-only succeeds and returns the expected map-carrier files. No files/lines/behavioral defect were cited, so no code fix is applicable for that item.\n\nSeparate from review feedback, the fresh v3 check is currently failed but GitHub has not exposed the job log while the workflow is still in progress; I am investigating that as CI, not as a review finding.

[briansrls]

Review metadata

• Provider / model: codex / unknown
• Commit: 6cb66cd0 · Trigger: schedule
• Thinking: 35s wall

---

⚠️ Review could not be performed because gh pr diff 1017 --repo gunb-ai/gunbc failed to connect to api.github.com, so the required source-of-truth diff is unavailable.

[briansrls]

Pushed d3fd2f4e1 for the #1017 v3 CI failure.\n\nCI failure disposition:\n- substrate_coproducts_match_runtime_carriers was valid: Rust had FieldValue::Map / ValueBody::Map, but src/v3/std/substrate.dag still reflected the old variant sets. Added FieldValue.Map(List<FieldEntry>) and ValueBodyMap(List<FieldEntry>) to the substrate authority and regenerated bootstrap snapshots.\n- lower_type_nominal_opaque_clause_sets_declaration_carrier was valid: the test compiled an isolated snippet that referenced String without defining it. The fixture now defines type String before the nominal alias.\n- handwritten_parse_snapshot_matches_manifest was valid drift from the parser/source changes; refreshed parse_corpus_manifest.txt.\n\nLocal work completed:\n- cargo fmt\n- git diff --check\n- RUSTC_WRAPPER= cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap\n- RUSTC_WRAPPER= cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored\n\nI attempted the three focused v3 tests locally, but the integration harness compile spent over 20 minutes in a single local rustc invocation; I stopped it and am letting GitHub Actions validate the pushed fix.\n\nDisposition for the 2026-04-27T18:57:05Z codex review: invalid as a source finding. It only reports that its review sandbox could not run gh pr diff 1017 due to api.github.com connectivity. This session successfully inspected the PR diff earlier; no files, lines, or behavioral defect were cited, so no code change applies for that review item.

[briansrls]

Review metadata

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

---

Findings

• src/v2/stage0/src/v2_compiler_parse.rs:1167-1186 — minor CODING.md drift: tok_is_ident_text and drop_leading_type_modifier take String by value where &str would do, and skip(1 as usize) is a needless cast. Stage0 v2 is bootstrap-only so this is NON-BLOCKING, but worth tightening on the next pass through this file.

Verdict

APPROVE — Substrate addition (ValueBody::Map / FieldValue::Map) is well-modeled: the new variant carries an explicit 4-pattern justification at dag.rs:259-274, lowering routes record/list/map symmetrically, the diagnostic for non-String map keys fails closed, and the nominal_opaque keyword retires the name-keyed bootstrap stamp (mark_bootstrap_secret_nominal_opacity) by carrying the fact through source authority + regen_bootstrap_emit rendering — single-authority and facts-flow-forward both improve. The bounded SG-1a dag_operators raw-source bridge is preserved with a new ratchet test (sg1_tokenize_authority_test.rs:208-231) naming its dissolution trigger. No invariant violations observed in the diff.

Exploratory observation

src/v3/std/substrate.dag mirrors only Unparsed, Structural, and now Map — ValueBody::Scalar and ValueBody::List already in dag.rs are still absent from the substrate authority. Pre-existing gap, not introduced here, but the gap widens with each new variant; worth closing as a follow-up so the .dag authority is the single source for ValueBody shape.

[briansrls]

Review disposition for the 2026-04-27T19:02:32Z finding on src/v2/stage0/src/v2_compiler_parse.rs: valid and fixed on current head bc11f9d5.\n\nVerified current code:\n- tok_is_ident_text now takes text: &str and compares directly.\n- drop_leading_type_modifier now takes modifier: &str.\n- type_body_tokens_after_modifiers passes "nominal_opaque" without allocating a String.\n- The needless skip(1 as usize) cast is now skip(1).\n\nNo further patch needed beyond the already-pushed current head. Note: GitHub currently reports no checks for bc11f9d5; waiting for Actions to enqueue or report.

[briansrls]

Disposition for the exploratory observation in the 2026-04-27T19:02:32Z review: verified, but not a fix-in-this-PR finding.\n\nCurrent code confirms the observation: Rust ValueBody includes Scalar, List, and Map, while src/v3/std/substrate.dag currently reflects ValueBodyUnparsed, ValueBodyStructural, and ValueBodyMap. This PR introduced the map reflection to keep the new carrier from widening the mismatch further; the missing Scalar/List reflection predates this PR and should be handled as a separate substrate reflection parity follow-up, because adding those authority variants here would require regenerated snapshots and likely test expectation updates outside the map-carrier fix scope.\n\nNo additional code patch is appropriate for #1017 beyond the current head bc11f9d5.

[briansrls]

Pushed b4cbf0e347 for the #1017 CI failure and merge conflict.\n\nCI failure disposition:\n- The failing Actions gate on 1e2fa64c was valid: the v2 fixed-point pipeline failed because the nominal_opaque type modifier skip existed only in generated stage0 Rust, not in the v2 parser authority at src/v2/02_parse.dag. Pass1 therefore regenerated a parser that could not parse dsl/std/types.dag during pass2.\n- Moved the modifier skip into src/v2/02_parse.dag and regenerated stage0 with ./scripts/regenerate-stage0.sh, so fixed-point and freshness now converge.\n\nMerge conflict disposition:\n- Integrated current origin/main with a merge commit and refreshed the v3 bootstrap snapshots from the merged authorities using regen_bootstrap.\n\nLocal verification:\n- ./scripts/regenerate-stage0.sh\n- cargo test -p v2-compiler-tests ci_ -- --ignored --nocapture (5 passed)\n- RUSTC_WRAPPER= cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap\n- cargo fmt --check\n- git diff --check\n- cargo check -p v3-grounding-engine\n\nDisposition for the 2026-04-27T21:42:28Z codex review: invalid as a source finding; it reports only that its review sandbox could not fetch the PR diff from api.github.com, and cites no file, line, or behavioral defect.\n\nFresh checks for b4cbf0e347 are queued; merge state is no longer blocked by local conflict resolution, pending Actions result.

[briansrls]

Review metadata

• Provider / model: cursor / composer-2
• Commit: 46d0e28d · Trigger: schedule
• Thinking: 245s wall

---

Findings

No issues grounded in the diff that clearly violate INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md.

• Substrate / modeling: FieldValue and ValueBody in substrate.dag were already marked 🟡 SCAFFOLD with paired rationale; the new Map / ValueBodyMap arms extend that same declared bridge rather than introducing a new undocumented coproduct.
• P2 / single authority: regen_tokenize.rs stops parsing dag_keyword_set from raw text and reads ValueBody::Map from the lowered shared-syntax Dag (data_body_named / from_authority), while dag_operators stays on the bounded raw-source path with an explicit module comment and an updated SG-1 ratchet in sg1_tokenize_authority_test.rs — documented, bounded, named follow-up for the remaining half.
• P3 / fail-closed: Map lowering reports ResolveError for wrong Map shape, non-String keys, and duplicate keys (lower.rs in the diff); nominal_opaque on a sum RHS is a typed ParseError (parse_parser_body.txt).
• Facts forward: Map bodies lower to ValueBody::Map instead of routing through Unparsed; consumers touched in the diff (lens_apply.rs, test_runner.rs, lower.rs, regen_bootstrap_emit.rs, tests) handle the new shape.

(Build verification was not completed here: cargo was blocked on the shared build directory lock.)

Verdict

APPROVE — The change matches the thesis of carrying map-shaped data as a structural ValueBody/FieldValue carrier, tightens keyword authority for token regen, and keeps the remaining operator bridge explicitly tracked. Nothing in the diff reads as a principled violation of the referenced rubric.

[briansrls]

Review metadata

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

---

1. Story of the diff

This PR moves map-shaped data bodies from an opaque/bootstrap scaffold into the v3 substrate by adding ValueBody::Map / FieldValue::Map, lowering SurfaceExpr::Map into that carrier when the declared type is Map<String, T>, and teaching renderers, reflection tests, bootstrap emission, the grounding engine, and test-runner formatting about the new shape. The practical first consumer is tokenizer regeneration: dag_keyword_set is now read from the lowered Dag as ValueBody::Map rather than raw source text, while dag_operators remains an explicitly bounded raw-source bridge until it also lowers structurally. In parallel, the PR promotes nominal_opaque from a bootstrap-only Secret stamp into source authority: dsl/std/types.dag:237 marks Secret as nominal_opaque, the parse surface gains a nominal_opaque: Bool field, and lowering writes Declaration.nominal_opacity directly.

2. Invariant categories

1. LAYER MODEL (substrate vs implementation).

Finding — BLOCKING, substrate / illegal states unrepresentable. The new substrate carrier is a raw list-shaped representation: src/v3/compiler/src/dag.rs:356 adds Map(Vec<(String, FieldValue)>),, src/v3/compiler/src/dag.rs:416 adds nested FieldValue::Map(Vec<(String, FieldValue)>),, and src/v3/std/substrate.dag:120 reflects this as ValueBodyMap(List<FieldEntry>). But the lowerer itself proves duplicate keys are illegal via src/v3/compiler/src/lower.rs:2872 — if !seen.insert(entry.key.clone()) {. That means map uniqueness is enforced only by the lowering path; generated/bootstrap code or any direct Rust construction can still create an invalid ValueBody::Map with duplicate keys. For a new substrate carrier, the map entry set should be a validated/newtyped authority, or construction should go through an API that makes duplicate-key maps unrepresentable.

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

Finding — BLOCKING, “illegal states unrepresentable” / API-level enforcement. The PR handles fail-closed detection for authored map literals correctly at src/v3/compiler/src/lower.rs:2872-2881, but the data model added at src/v3/compiler/src/dag.rs:356 still permits the exact illegal state that the diagnostic rejects. This is behavioral enforcement at one producer, not substrate-level enforcement for all producers/readers of the carrier.

1. CODING.md.

Compliant. The main implementation stays in free helpers with explicit inputs rather than adding behavior to the Dag object: src/v3/compiler/src/lower.rs:2830 introduces lower_map_to_structural(...), and src/v3/compiler/src/lower.rs:2862 introduces lower_string_map_entries(...); both return typed Option<ValueBody> / Option<Vec<...>> and report through Diagnostic::ResolveError rather than fabricating success.

1. TESTING.md.

Finding — NON-BLOCKING once the carrier shape is corrected. The happy path is covered: src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1882 asserts ValueBody::Map(entries) and validates the map payload. The new fail-closed branches are not directly covered: duplicate keys at src/v3/compiler/src/lower.rs:2872 and non-Map<String, _> map bodies at src/v3/compiler/src/lower.rs:2837 would benefit from focused behavior tests so the boundary stays pinned after the substrate carrier is fixed.

1. LOCKED DESIGN DECISIONS.

N/A — I do not see this PR altering a locked Arrow/body/external-realization decision or another explicitly locked design seam in the diff.

1. TRACKED vs UNTRACKED DEBT.

Compliant. The remaining tokenizer raw-source bridge is documented and bounded: src/v3/compiler/src/bin/regen_tokenize.rs:5-7 says keywords now come from lowered shared syntax while operators remain on the raw bridge only until dag_operators lowers structurally, and src/v3/compiler/tests/integration/sg1_tokenize_authority_test.rs:221-225 keeps that ratchet executable. The Secret bootstrap bridge is also dissolved into source authority via dsl/std/types.dag:237 plus lowering at src/v3/compiler/src/lower.rs:2248-2252.

3. Verdict

REQUEST_CHANGES

The direction is right: map bodies now flow forward as substrate data, tokenizer regen consumes the new authority, and nominal opacity moves out of a bootstrap stamp. I would not merge the new substrate map carrier while duplicate-key maps remain representable in the carrier itself; that is exactly the kind of substrate shape that becomes harder to repair once consumers start relying on it.

[briansrls]

Review metadata

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

---

1. Story of the diff

This PR moves map-shaped .dag data bodies from “present but opaque source text” into the v3 substrate as structural data. The load-bearing change is the new ValueBody::Map / FieldValue::Map carrier in src/v3/compiler/src/dag.rs, with lower.rs recognizing Map<String, T> declarations, recursively lowering each entry value through the existing structural field-value path, and rejecting duplicate keys during lowering. The bootstrap generated snapshots then materialize existing map-shaped std data, such as kernel_algebra_profile, kernel_type_set, and container metadata, as ValueBody::Map instead of ValueBody::Unparsed.

The second thread is a consumer migration: regen_tokenize now derives dag_keyword_set from the lowered shared syntax DAG via ValueBody::Map, while dag_operators remains on the bounded raw-source bridge until its body lowers structurally. Rendering/test surfaces are widened to print map values, and the tests add a positive map-lowering check plus a tokenizer ratchet that distinguishes the now-structural keyword map from the still-unparsed operator list.

2. Invariant categories

1. LAYER MODEL — Finding, BLOCKING. This is substrate: src/v3/compiler/src/dag.rs:356: Map(Vec<(String, FieldValue)>), and src/v3/compiler/src/dag.rs:416: Map(Vec<(String, FieldValue)>), add the at-rest carrier shape on Dag values. The lowering code itself proves duplicate keys are illegal — src/v3/compiler/src/lower.rs:2864: if !seen.insert(entry.key.clone()) { — but the substrate carrier is a public raw Vec<(String, FieldValue)>, so duplicate key authorities remain representable anywhere that constructs a ValueBody/FieldValue directly, including generated bootstrap snapshots. For a substrate map carrier, this should be a uniqueness-enforcing newtype/constructor or a keyed map type, not a convention enforced only by one lowering path.
2. INVARIANTS.md + modeling-discipline.md — Finding, BLOCKING. This violates illegal states unrepresentable and single-authority metadata. The diff correctly detects duplicate source keys at src/v3/compiler/src/lower.rs:2864, but src/v3/compiler/src/dag.rs:356: Map(Vec<(String, FieldValue)>), still allows two entries for the same key to coexist as two authorities for one map fact. Behavioral duplicate checks at parse/lower time are not enough for a new substrate carrier; the invalid state should not be constructible at the data-model/API level.
3. CODING.md — Compliant. The implementation keeps the new behavior as explicit free functions with visible dependencies: src/v3/compiler/src/lower.rs:2822: fn lower_map_to_structural( takes the data name, expression, expected type, symbol table, and Dag, and src/v3/compiler/src/lower.rs:3722: fn map_value_type(dag: &Dag, expected_type: DeclarationId) -> Option<DeclarationId> exposes a narrow query instead of hiding state behind an object-style method.
4. TESTING.md — Compliant. The diff adds behavior-driven coverage for the new contract: src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:1778: fn map_body_data_item_parses_and_lowers_to_value_body_map() { checks parse-to-lower behavior and variant identity in the map values, while src/v3/compiler/tests/integration/sg1_tokenize_authority_test.rs:208: fn shared_syntax_keyword_map_is_structural_while_operator_bridge_remains_bounded() { pins the keyword-map migration and remaining operator bridge. Tests cannot compensate for the substrate duplicate-key representability above; that needs a carrier/API fix.
5. LOCKED DESIGN DECISIONS — N/A. The diff does not appear to alter a locked design decision directly; it preserves the existing SG-1a operator bridge and adds a ratchet for its eventual dissolution.
6. TRACKED vs UNTRACKED DEBT — Finding, NON-BLOCKING. The remaining operator raw-source bridge is tracked: src/v3/compiler/src/bin/regen_tokenize.rs:7: //! bounded raw-source bridge until \dag_operators lowers structurally.and the test repeats the dissolution condition atsrc/v3/compiler/tests/integration/sg1_tokenize_authority_test.rs:228. But src/v3/compiler/src/bin/regen_tokenize.rs:267: #[allow(dead_code)]newly suppresses the now-unusedassert_shared_syntax_raw_source_scaffold_still_required scaffold instead of deleting or reworking it around the operator-only bridge. That is small implementation debt, but it is unbounded as written: no documentation, bound, or named dissolution trigger attaches to the dead-code suppression.

3. Verdict

REQUEST_CHANGES

The feature direction is right, and most consumers/tests were updated coherently, but the new substrate carrier makes duplicate map keys representable even though lowering treats them as invalid. Fixing that now is much cheaper than letting consumers grow around Vec<(String, FieldValue)> as the map authority shape.