Skip to content

feat(v3): add string diagnostic axis vocabulary - #1465

Merged
briansrls merged 7 commits into
mainfrom
session/crisp-ibex-569-string-axis-vocabulary
May 2, 2026
Merged

briansrls merged 7 commits into
mainfrom
session/crisp-ibex-569-string-axis-vocabulary

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Summary

  • Add canonical string-family axis vocabulary in src/v3/std/emit_model.dag for future per-target diagnostic-ordering rows: StringOwnershipAxis, StringLifetimeAxis, StringGrowabilityAxis, and StringEncodingAxis.
  • Keep axis values structural declarations rather than strings, with NotApplicable as an explicit growability value.
  • Add focused bootstrap ratchets proving the axes live under emit-model authority and retain the intended closed values.
  • Regenerate bootstrap snapshots and refresh the handwritten parse corpus manifest.

Boundaries

  • Axis vocabulary only.
  • Does not extend TypeRealization.
  • Does not add std/lifetime.dag.
  • Does not populate Rust/string-family target rows or add a Grounding projection reader.

Validation

  • CARGO_INCREMENTAL=0 cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap
  • cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored
  • cargo test -p v3-compiler --test integration string_diagnostic_ordering_axes_live_in_emit_model_authority -- --nocapture
  • cargo test -p v3-compiler --test integration string_diagnostic_ordering_axes_are_closed_structural_values -- --nocapture
  • cargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocapture
  • cargo fmt --all --check
  • git diff --check
  • CARGO_INCREMENTAL=0 cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verify

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: 779d5938 · Trigger: schedule
  • Comparison: origin/main @ d25b89a6 ... review/pr-1465-779d5938 @ 779d5938
  • Thinking: 40s wall

Reviewed the filtered diff: substantive edits are emit_model.dag (four axis types + 🟢 TERMINAL comments), two integration tests, manifest hash lines, and regenerated bootstrap Rust. The large bootstrap_generated*.rs churn matches declaring new substrate types.

Findings: None. The new sum types include 🟢 TERMINAL scope notes and rationale (including distinction from ParameterDisposition), matching docs/modeling-discipline.md coproduct classification. Tests follow the same patterns as special_value_policy_axes_are_closed_sums and existing decl.span.file authority checks in this file (TESTING.md behavior-style pinning of substrate variant labels, not ad hoc message text). Single authority for these axes stays in emit_model.dag, with parse corpus manifest updated — consistent with boundary discipline.

Verdict: APPROVE — Narrow vocabulary + ratchet tests + regen; no invariant or style violations spotted in the substantive diff.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: f862e64d · Trigger: schedule
  • Thinking: 227s wall

Non-blocking — Strengths

  • src/v3/std/emit_model.dag Mixed PR, primarily .dag model: the new string diagnostic axes are closed emit-model vocabulary with terminal receipts and targeted bootstrap tests.

✅ No blocking concerns found.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: cursor / composer-2
  • Commit: dae39a13 · Trigger: schedule
  • Comparison: origin/main @ 31b0a7a2 ... review/pr-1465-dae39a13 @ dae39a13
  • Thinking: 24s wall

Findings

None. The diff adds closed substrate sums in emit_model.dag with explicit TERMINAL classification and scope notes (aligned with docs/modeling-discipline.md §4), places authority in the emission model with tests that pin declaration site and variant labels (supports P2 single authority and typed carriers vs ad hoc strings — consistent with DB-1 / C-5 direction). Nothing in the diff suggests fail-closed fabrication, duplicate competing authorities, or scaffold without a named forward path (encoding explicitly defers further variants to later LanguageSpec rows).

Verdict

APPROVE — Narrow, coherent slice: substrate vocabulary + regen + inhab tests that lock location and closed sums. No invariant violations identified from the substantive diff (emit_model.dag, m2_substrate_inhabitance_test.rs, manifest tweak; bootstrap churn is expected mechanical fallout).

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: ed283c8d · Trigger: schedule
  • Comparison: origin/main @ 17774325 ... review/pr-1465-ed283c8d @ ed283c8d
  • Thinking: 43s wall

Verdict: APPROVE — the diff is narrowly scoped to canonical .dag vocabulary plus generated bootstrap updates and regression checks. I don’t see a concrete violation of the pinned invariants, coding discipline, or testing guidance in the changed lines.

I did not run builds or tests, per instruction.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: ed283c8d · Trigger: schedule
  • Thinking: 174s wall

✅ Mixed PR, primarily a .dag model vocabulary addition with regenerated bootstrap/tests; no blocking concerns found.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 23a2bb97 · Trigger: schedule
  • Comparison: origin/main @ 3bebb824 ... review/pr-1465-23a2bb97 @ 23a2bb97
  • Thinking: 50s wall

Verdict: APPROVE

The diff adds canonical string diagnostic axis vocabulary in emit_model.dag, with coproduct classifications documented at the declarations and regression coverage confirming authority location and variant shape. I don’t see a concrete violation of the pinned invariants, coding, or testing discipline in the reviewed diff. Tests were not run per instruction.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: 6d6f87f0 · Trigger: schedule
  • Comparison: origin/main @ 4ba0a6d0 ... review/pr-1465-6d6f87f0 @ 6d6f87f0
  • Thinking: 49s wall

Verdict: APPROVE

The diff is narrowly scoped to adding canonical string diagnostic axis vocabulary in src/v3/std/emit_model.dag, regenerating bootstrap snapshots, and adding shape/authority assertions. I don’t see a concrete violation of the pinned modeling, coding, or testing discipline in the changed lines.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 6d6f87f0 · Trigger: schedule
  • Thinking: 192s wall

Non-blocking — Strengths

  • src/v3/std/emit_model.dag The added string-family axes are typed closed vocabulary in the emit-model authority, with terminal rationale and tests pinning the variant sets.

✅ No blocking concerns.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / codex-default
  • Commit: b3f70668 · Trigger: schedule
  • Comparison: origin/main @ 8c745cae ... review/pr-1465-b3f70668 @ b3f70668
  • Thinking: 47s wall

Verdict: APPROVE

The diff is narrowly scoped: it adds typed string diagnostic axis vocabulary in src/v3/std/emit_model.dag with terminal classification comments, regenerates bootstrap output, and adds focused regression checks for authority location and closed structural values. I don’t see a concrete violation of the pinned modeling, coding, or testing guidance in the changed lines.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: 6d6f87f0 · Trigger: manual
  • Comparison: main @ 8c745cae ... session/crisp-ibex-569-string-axis-vocabulary @ b3f70668
  • Conversation: View conversation

1. Story of the diff

This PR adds canonical substrate vocabulary for string-family diagnostic ordering, placing the axes in src/v3/std/emit_model.dag rather than letting future target rows or analyzers normalize string names locally. The new model separates carrier ownership from callable parameter disposition (StringOwnershipAxis at src/v3/std/emit_model.dag:87-95), names shared lifetime roles without Rust identifier leakage (StringLifetimeAxis at src/v3/std/emit_model.dag:97-103), makes growability’s “not relevant” state explicit (StringGrowabilityAxis at src/v3/std/emit_model.dag:105-111), and introduces the current UTF-8 / FreeMonoid<Char> encoding row as a structural value (StringEncodingAxis at src/v3/std/emit_model.dag:113-118). The generated bootstrap fixtures then materialize those declarations, e.g. StringOwnershipAxis through StringEncodingAxis appear as generated TypeConnective::Disj declarations at src/v3/compiler/src/bootstrap_generated.rs:898-901, with the corresponding no-parse-surface fixture regenerated as well. The tests add two substrate-inhabitance checks: one pins the authority file to emit_model.dag (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2553-2569), and one pins the closed variant vocabulary (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2573-2598).

2. Invariant categories

  1. LAYER MODEL (substrate vs implementation).

Compliant — this does touch substrate vocabulary, but it lands the fact at the emission-model authority rather than in implementation-local Rust: type StringOwnershipAxis is declared in src/v3/std/emit_model.dag:93, and the test explicitly requires the axis declarations to remain in src/v3/std/emit_model.dag at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2565-2568.

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

Compliant — illegal-states-unrepresentable / fail-closed modeling is handled by explicit closed sums instead of booleans, strings, or absence. The clearest example is StringGrowabilityAxis, where NotApplicable is a real variant at src/v3/std/emit_model.dag:108-111, and the comment states that this prevents the axis from becoming “unknown” by omission at src/v3/std/emit_model.dag:105-107. Coproduct discipline is also addressed: each new multi-variant axis carries a 🟢 terminal classification comment at src/v3/std/emit_model.dag:87, src/v3/std/emit_model.dag:97, and src/v3/std/emit_model.dag:105.

  1. CODING.md.

Compliant — the Rust-side additions are small, direct test functions over data, not new hidden state or object behavior. The tests read the generated Dag and ask for declarations by name at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2554-2564, then assert on structured declaration facts via disj_variant_labels at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2577-2595.

  1. TESTING.md.

Compliant — the PR adds behavior-focused regression coverage for the exact substrate contract being introduced. One test checks single-authority placement (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2553-2569); the other checks the closed variant sets and the explicit NotApplicable carrier (src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2573-2598). Using the generated bootstrap Dag is appropriate here because the claim is about generated substrate inhabitance, not an isolated Rust helper.

  1. LOCKED DESIGN DECISIONS.

N/A — I do not see a diff line that alters a locked design decision. The only design-facing language is alignment/scoping text for the encoding axis, where StringEncodingAxis names the current .dag String = FreeMonoid<Char> / UTF-8 row and says later byte or target-specific encodings belong in later LanguageSpec rows at src/v3/std/emit_model.dag:113-118.

  1. TRACKED vs UNTRACKED DEBT.

N/A — no new TODO, shim, temporary adapter, or scaffold is introduced in the diff. The “Rows land in target specs later” language at src/v3/std/emit_model.dag:90-92 and in the authority test message at src/v3/compiler/tests/integration/m2_substrate_inhabitance_test.rs:2567-2568 reads as scope delimitation for this vocabulary slice, not as a second representation or bridge that consumers can wire into.

3. Verdict

APPROVE. The PR is narrow and structurally consistent: it moves string diagnostic-ordering vocabulary into typed substrate declarations, regenerates the bootstrap fixtures, updates the manifest, and adds targeted regression tests for authority and closed variant shape. I did not find a diff-citable invariant, coding, testing, locked-design, or debt issue.

@briansrls
briansrls merged commit 42c7e45 into main May 2, 2026
4 checks passed
briansrls added a commit that referenced this pull request May 2, 2026
…d landing post-#1465

Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls added a commit that referenced this pull request May 2, 2026
…d landing post-#1465

Co-authored-by: Cursor <cursoragent@cursor.com>
briansrls added a commit that referenced this pull request May 2, 2026
…d landing post-#1465 (#1478)

* docs(audit): refresh lifetime-axes vocabulary spec — string-namespaced landing post-#1465

Co-authored-by: Cursor <cursoragent@cursor.com>

* docs(audit): clarify growability bijection and Substrate #1130 design question

Co-authored-by: Cursor <cursoragent@cursor.com>

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant