Repository navigation
feat(v3): add ValueBody list substrate and std.unicode bootstrap - #920
Conversation
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
934a8f45· Trigger:schedule - Thinking:
92s wall
gh pr diff 920 --repo gunb-ai/gunbc failed to connect to api.github.com.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 934a8f453a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(list_expr @ SurfaceExpr::List { .. }) => { | ||
| lower_list_to_structural(name, list_expr, ty_decl_id, symbols, dag) | ||
| } |
There was a problem hiding this comment.
lower_data_item now lowers SurfaceExpr::List into ValueBody::List, but Dag::new() does not re-lower sources; it clones the committed snapshot from bootstrap_generated::bootstrapped_fixture_dag in dag.rs. Because this commit does not regenerate bootstrap_generated*.rs, the shipped bootstrap graph still contains rust_pilot_primitives as ValueBody::Unparsed (see src/v3/compiler/src/bootstrap_generated.rs), so runtime Dag::new() consumers will not observe the new behavior yet. Please run regen_bootstrap and commit updated generated snapshots with this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified against current PR head 717026a: this is addressed. The bootstrap snapshots were regenerated and now contain structural list bodies; src/v3/compiler/src/bootstrap_generated.rs has rust_pilot_primitives as value_body: Some(ValueBody::List(...)), and the no-parse-surface snapshot matches. I also reran the focused coverage: cargo test -p v3-compiler --test integration top_level_list_data_body -- --nocapture (2 passed) and cargo test -p v3-compiler --test integration extdeps_rust_primitives_loader_test -- --nocapture (3 passed).
| Diagnostic::ResolveError { | ||
| name: format!( | ||
| "data `{name}` has an opaque body — M1(2.8) user code cannot yet use record / list / map literals inside data bodies (see DOWNSTREAM_REQUIREMENTS.md class-5 gap #3)" | ||
| "data `{name}` has an opaque body — M1(2.8) user code cannot yet use map literals inside data bodies (see DOWNSTREAM_REQUIREMENTS.md class-5 gap #3)" |
There was a problem hiding this comment.
This message now claims unparsed data bodies are specifically due to unsupported map literals, but reject_user_unparsed_scaffolds is reached for any ValueBody::Unparsed, including record/list bodies that failed structural/type checks and fell back to Unparsed in lower_data_item. In those cases users get a misleading extra error about maps, which obscures the real failure path; the text should remain generic or be emitted only when the original body was actually a map.
Useful? React with 👍 / 👎.
briansrls
changed the title
|
Review metadata
1. Story of the diffThis PR turns top-level The other half of the diff brings 2. Invariant categories
Compliant — this is substrate-touching, and the new substrate carrier is modeled explicitly rather than hidden in an implementation mirror:
Compliant — fail-closed/API-level enforcement is handled in the new lowering path:
Compliant — the new logic stays in data + free-function style. The substrate remains a data enum at
Finding (NON-BLOCKING) — the tests pin the generated/bootstrap outcome, but not the new lowering behavior directly.
N/A — I did not see this diff altering a locked design decision. It moves an already-named R2 substrate lane forward by adding
Finding (NON-BLOCKING) — two updated comments appear to reference a not-yet-landed 3. VerdictAPPROVE_WITH_COMMENTS The substrate shape is sound: list bodies are represented once on |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
40740a6b· Trigger:schedule - Thinking:
80s wall
|
Addressed the review comments in head
Local verification before push: |
# Conflicts: # src/v3/compiler/src/bootstrap_generated.rs # src/v3/compiler/src/bootstrap_generated_without_parse_surface.rs
|
Queued feedback disposition after recheck:
Verification: git diff --check passed. I could not rerun the Rust integration test in this restored shell because cargo/rustup/nix are not installed on PATH in the execution environment. |
|
Review metadata
Verdict: APPROVE — Diff is narrowly scoped: adds No findings. |
|
Review metadata
Verdict: APPROVE The diff cleanly extends the substrate with I wasn’t able to run the targeted cargo tests because the local exec tool started rejecting new processes, so this is a static review. |
Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition.
Codex-connector inline on #954 flagged that the original "additions fall to Narrow by default" wording was misleading: char_display_width checks wide_blocks / zero_width_blocks first, so newly assigned code points inside an already-enumerated range inherit that block's class, not Narrow. Only code points outside every enumerated range fall to Narrow. Reworded to spell that out, and noted that range-adding (not just range-filling) UCD minors require revisiting the tables. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(audit): W-C1 follow-up harvest table Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition. * docs(audit): close #897/#824/#825 rows per bright-wolf-465 audit Per bright-wolf-465 (inbox #945), no missed BLOCKING findings on these PRs; flip rows 6/7/8 to closed with audit citation. * docs(audit): reconcile note with row dispositions for #897/#824/#825 * WIP: calm-ant-861 * chore: apply cargo fmt * WIP: calm-ant-861 * fix(test): fold B5 Loop closure receipt into m1_substrate_test SG-0 census ratchet forbids new hand-authored .rs files. Move the every_loop_node_originates_from_recursive_function_lowering test (and the LoopNode/LoopBound import + two helper fns) into the existing m1_substrate_test.rs and drop the standalone file + its integration.rs registration. Behavior unchanged. * docs: correct B5 receipt file path in synthesis doc (m1_substrate_test, not standalone file) * docs: mark ROADMAP loop-emission row resolved + correct synthesis cite Codex BLOCKING on be4a6ab: ROADMAP.md still listed the loop-emission semantic invariant as open debt while the synthesis doc declared it resolved — tracker-authority mismatch. - ROADMAP.md:436: rewrite the row as RESOLVED 2026-04-27 with PR cite, audit summary (two production sites in lower.rs), receipt location (m1_substrate_test.rs), and marker-retired note. - synthesis-doc §3 line 155: fix the receipt path (loop_construction_closure_test.rs → m1_substrate_test.rs after the SG-0 fold). * fix(test): split global vs fixture-scoped Loop closure assertions Codex BLOCKING on db7b773: previous test mixed a global all-Dag closure check with fixture-only variant claims, so bootstrap loops could mask the fixture-coverage claim. Split into two phases: - Global: every Behavior::Loop in the Dag (fixture + bootstrap) satisfies the recursive-function-lowering signature (loop.output is a Bind value port + Descent cluster id resolves). This is the closure invariant. - Fixture-scoped: filter loops by span.file == fixture file before claiming both LoopBound variants are produced by THIS fixture. Bootstrap loops are excluded so the variant claim is mechanically about the fixture's recursive functions. * post-merge: drop superseded artifacts; correct ROADMAP B5 receipt cite The parallel B5 lane landed `r2_b5_loop_construction_closure_test.rs` on main with a more rigorous receipt (substring push-site ratchet + per-fixture DAG walk + Origin::Accumulated provenance). The harvest table in this PR is also being landed via aggregate #949. Drop: - `docs/audit/w-c1-followup-harvest-2026-04-27.md` (canonical surface is #949). - The m1_substrate_test additions (auto-reverted via merge --theirs; superseded by the standalone r2_b5_loop_construction_closure_test.rs on main). Keep: - `ROADMAP.md` row marking loop-emission RESOLVED, with citation rewritten to point at the actual landed receipt file (r2_b5_…) instead of the retired m1_substrate_test cite.
* docs(std.unicode): cite UCD 15.x / UAX-11 authority Closes the #920 post-merge citation gap. Header now states the file is sourced from UCD 15.x (UAX #11 East Asian Width) for the display- width tables and is intentionally 15.x compatible rather than pinned to a specific minor. UAX #9 is explicitly not consulted (no bidi). No behavior changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs: tighten P5 per-PR dissolution gate (b) for ROADMAP-cited deferrals Require exactly one checkable receipt: delete path, SG-0 census before/after counts, or lane plus concrete ROADMAP row/link. Call out vague deferrals as insufficient. Align INVARIANTS §P5 (b) with the template without duplicating the checklist. Made-with: Cursor * docs(audit): W-C1 follow-up harvest table Single control surface for cleanup-lane harvest of #900/#901/#920/#897/#824/#825 per dispatch from tidy-dove-734 (#941). No ctrl#263 in repo; this docs/audit artifact is the agreed fallback. Rows: source PR, gap, file/invariant, owner lane, dissolution trigger, acceptance check, tracking authority, disposition. * docs(audit): mark #920 citation follow-up closed * docs(audit): close #897 #824 #825 harvest rows * docs(audit): cite closure evidence for harvest rows * WIP: Cleanup * docs(std.unicode): clarify UAX 11 coverage * docs: link cleanup harvest from roadmap * docs(std.unicode): refresh bootstrap spans * docs(std.unicode): refresh full bootstrap spans * docs(std.unicode): refresh no-parse bootstrap spans * docs(audit): stabilize harvest code references --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review: spot-check claimed no landed slice vs HEAD; #920 merged ValueBody::List + std.unicode bootstrap. Gate/Last signal/Notes updated; explicit ROADMAP Gap 3 caveat (stale prose). P1 live-state discipline. Made-with: Cursor
* docs(r2): closure-ledger — Substrate/PB rows + R1 path-a signal - T-Substrate-Lens-Primitive: in-flight, #1186 (carrier); instance gates pending - ValueBody list/sum: spot-check note (ROADMAP gap unchanged this pass) - PB patch-lower-helpers: note #1014 + #1192 ratchet scope - R3 bridge retirements: in-flight #1171 #1183 #1192; Verification gate explicit - Incoming surface: path-(a) closure — no residuals absorbed; named R1C-B deferral Cross-program drift sweep per PM detection + Director endorsement. Made-with: Cursor * docs(r2): ledger — ValueBody row reflects #920 list + unicode slice Codex review: spot-check claimed no landed slice vs HEAD; #920 merged ValueBody::List + std.unicode bootstrap. Gate/Last signal/Notes updated; explicit ROADMAP Gap 3 caveat (stale prose). P1 live-state discipline. Made-with: Cursor
1 participant
Brief
Assigned brief:
docs/briefs/t-substrate-valuebody-list-worker.md.Acceptance Receipt
ValueBody::List(Vec<FieldValue>)for top-level list-shaped data bodies.SurfaceExpr::Listinlower_data_itemthrough existingFieldValuestructural lowering; element shape remainsVec<FieldValue>.rust_pilot_primitivesnow lowers fromDag::new()asValueBody::Listwith 10FieldValue::Variantelements; integration test checks count and first/last constructor labels.std.unicodeto the bootstrap std fixture path; integration test verifiesCharClassresolves from freshDag::new().bootstrap_std_generated.rs,bootstrap_generated.rs, andbootstrap_generated_without_parse_surface.rs;regen_bootstrap --verifypasses.ValueBody::Listvariant.ValueBodyconsumers, including snapshot rendering, lens/test-runner conversion, refinement checks, and structural-only test helpers. No wildcard swallowing.resolve_data_pathhandlesValueBody::Listdeliberately asNone: DB-10 scalar/record field path reads do not expose list terminal reads yet.ValueBody::Map/kernel_algebra_profileout of scope.Feedback Addressed
Dag::new()observesrust_pilot_primitives.value_body = ValueBody::List(_).ValueBody::List.Unparseddata body is a map literal.parse_corpus_manifest.txtafter thetokenize.dagcomment update.origin/main; the effective diff no longer contains prior neat-dove-411 #882 docs lineage.Verification
Local:
cargo fmt --all --checkcargo test -p v3-compiler --test integration extdeps_rust_primitives_loader_test -- --nocapturecargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocapturecargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verifycargo clippy -p v3-compiler --all-targets -- -D warningsRUSTC_BOOTSTRAP=1 cargo test -p v3-compiler -- -Z unstable-options --report-timereproduced the stale manifest failure before the latest fix; targeted manifest test passes after refresh.Pending/CI:
40740a6bshould rerun GitHub checks with the refreshed parser manifest.