Skip to content

[codex] SG-1 debt cleanup: structural token rows - #566

Merged
briansrls merged 4 commits into
session/neat-pike-779from
session/snappy-lark-771-cleanup
Apr 19, 2026
Merged

briansrls merged 4 commits into
session/neat-pike-779from
session/snappy-lark-771-cleanup

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

What changed

This stacked PR pays down the remaining SG-1 row-schema debt on top of #556.

  • replace KeywordSpec.kind_name: String with KeywordSpec.kind: TokenKind
  • replace PunctSpec { pattern, width, kind_name } with PunctSpec { pattern, kind }
  • teach regen_tokenize to read structural nullary TokenKind variants from the lowered Dag instead of validating string labels at runtime
  • derive punctuation width from pattern bytes during codegen instead of storing a second authority field
  • add an SG-1 authority test that locks the row shape so kind_name / width cannot drift back in

Why

tokenize.dag was still carrying semantic facts by convention:

  • kind_name duplicated TokenKind variant identity as a string
  • width duplicated information already present in pattern

The substrate can already carry nullary sum constructors structurally, so this PR removes both duplicate facts instead of banking them as ongoing debt.

Impact

This is a schema-tightening cleanup only. The generated tokenizer behavior stays the same; the change is that SG-1 authority rows now encode the facts structurally and regen_tokenize projects from that shape directly.

Validation

  • cargo run -q -p v3-compiler --bin regen_tokenize
  • cargo test -p v3-compiler --test integration sg1_tokenize_authority_test -- --nocapture

Notes

  • Stacked on session/neat-pike-779 / PR #556
  • The only extra commit on top of the cleanup is the pre-push formatter fix required by the repo hook

@briansrls

Copy link
Copy Markdown
Contributor Author

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

@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.

codex · gpt-5.4 · 0c430b5f

⚠️ Review (blocking: 1, non-blocking: 2+/0-)

BLOCKING (1)

Root Cause

  • src/v3/compiler/tokenize.dag The row schema needs a first-class fixed-token/nullary-constructor carrier (or a documented scaffold with a named dissolution trigger); reusing full TokenKind leaves regen_tokenize and the test suite to re-impose the invariant after lowering.

Non-blocking — Strengths

  • src/v3/compiler/src/bin/regen_tokenize.rs Deriving punctuation width from pattern removes the second authority instead of validating a redundant width field at codegen time.
  • src/v3/compiler/tests/integration/sg1_tokenize_authority_test.rs The new SG-1 authority test is a useful ratchet against kind_name and width drifting back into the row schema.

⚠️ The duplicate string and width authorities are gone, but the row kind carrier is still too wide for the invariant this PR is trying to settle.

Comment thread src/v3/compiler/tokenize.dag Outdated
type KeywordSpec {
spelling: String
kind_name: String
kind: TokenKind

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.

BLOCKING: KeywordSpec.kind and PunctSpec.kind use the full TokenKind, so SG-1 rows can still encode nonsensical authorities like Eof (and payload-bearing variants are only rejected later by panic), which violates illegal-states-unrepresentable and API-level enforcement at the substrate boundary.

@briansrls

Copy link
Copy Markdown
Contributor Author

codex · gpt-5.4 · 0c430b5f

⚠️ Review (blocking: 1, non-blocking: 2+/0-)

BLOCKING (1)

Root Cause

  • src/v3/compiler/tokenize.dag The row schema needs a first-class fixed-token/nullary-constructor carrier (or a documented scaffold with a named dissolution trigger); reusing full TokenKind leaves regen_tokenize and the test suite to re-impose the invariant after lowering.

Non-blocking — Strengths

  • src/v3/compiler/src/bin/regen_tokenize.rs Deriving punctuation width from pattern removes the second authority instead of validating a redundant width field at codegen time.
  • src/v3/compiler/tests/integration/sg1_tokenize_authority_test.rs The new SG-1 authority test is a useful ratchet against kind_name and width drifting back into the row schema.

⚠️ The duplicate string and width authorities are gone, but the row kind carrier is still too wide for the invariant this PR is trying to settle.

@briansrls

Copy link
Copy Markdown
Contributor Author

ChatGPT Review

Generated by gpt-5-4-pro

I have reviewed the materials, including the code diff and relevant documentation. Here's the analysis for the PR #566 review:

Findings:

  • No major concerns regarding silent errors or unhandled failure paths: All failure paths are correctly diagnosed, following the principle of fail-closed, and no silent errors or panics are observed in this diff. This satisfies the Fail-Closed principle.
  • Data models look well-formed: There are no obvious illegal states in the code, and the data structures used seem to avoid mutually exclusive values or combinations that violate the design. This satisfies the Illegal States Unrepresentable principle.
  • Fact Flow: The code consistently carries facts between stages, with no obvious loss of data between steps. Fields produced by one stage either flow forward or are explicitly discarded. This adheres to the Facts Flow Forward principle.
  • Coproducts and Enum Usage: There doesn't appear to be any flat coproducts with more than 1 variant in the diff. The design uses appropriate abstractions for handling variants in enums and avoids leaving incomplete coproducts. No violations of the Coproduct Dissolution principle.
  • Single Authority: No duplicate representations of facts are observed. The metadata and state transitions are correctly managed without redundant copies or conflicting representations. This meets the Single Authority Metadata principle.
  • API-level enforcement: The code seems to have proper API-level enforcement of modeling principles, ensuring that violations would be difficult for a new contributor to introduce. This follows the API-Level Enforcement principle.

Deepest Question (optional):

  • None. The diff seems to follow the expected structures and principles well.

Verdict:

APPROVE — The changes appear to be clean, following the established modeling principles. No structural issues were found in the diff.

If you need any further clarification or have specific parts of the code you'd like to discuss, feel free to let me know!


View conversation

@briansrls
briansrls marked this pull request as ready for review April 19, 2026 17:48
@briansrls

Copy link
Copy Markdown
Contributor Author

claude-review (director) — ✅ LGTM substantively. Strong execution of
the SG-1 secondary debt I named.

Validated against original intent:

  • My SG-1 #556 director comment named: "kind_name string conventions
    carrying semantic facts. Either typed TokenKind enum (preferred) or
    explicit Scheduled-Deletion entry with grammar-extension trigger."
  • This PR took the preferred path (typed TokenKind variant field)
    • went further on a second debt I hadn't named (PunctSpec.width
      was a duplicated authority derivable from pattern.bytes)

Structural wins packed cleanly:

  1. Eliminated runtime string validation — validate_kind_names and
    its HashSet<String> lookup deleted. extract_nullary_variant_field
    now reads the structural TokenKind variant from the lowered Dag.
    That's the move from convention-level to type-level enforcement.
  2. KeywordSpec.kind_name: String → KeywordSpec.kind: TokenKind
    — typed handle replaces stringly-typed marker (cite
    feedback_no_metadata_markers, feedback_opaque_strings_attract_heuristics).
  3. PunctSpec { pattern, width, kind_name } → PunctSpec { pattern, kind }
    — width derived from pattern.bytes at codegen, not stored as a
    second authority field. Single-authority discipline applied (cite
    feedback_substrate_principle_audit Q3 — no duplicated facts).
  4. Authority test added locking the row shape against drift.

Coordination — stacked on #556:

This PR is stacked on #556 SG-1 per the description. Land order matters:

Next steps:

  • Move from draft to ready when SG-1 #556 has a clear merge horizon
  • CI will run; expect green given the structural-only nature of the
    changes
  • If CI flags anything (especially the snapshot freshness test from
    SG-1), it's likely the new kind: TokenKind shape needs the snapshot
    updated — mechanical fix

Push to convergence. No structural concerns. This is exactly the
"surface debt → typed structural fix → land" loop the SG paydown
discipline was built for. Strong work.

@briansrls

Copy link
Copy Markdown
Contributor Author

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

@briansrls
briansrls merged commit b8c21b1 into session/neat-pike-779 Apr 19, 2026
@briansrls briansrls mentioned this pull request Apr 19, 2026
briansrls added a commit that referenced this pull request Apr 19, 2026
* WIP: SG-1

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* WIP: SG-1

* [codex] SG-1 debt cleanup: structural token rows (#566)

* WIP: Debt

* chore: apply cargo fmt

* Tighten SG-1 token row carriers

* chore: apply cargo fmt

* WIP: SG-1

* WIP: SG-1

* fix(regen_tokenize): use slice::contains to satisfy clippy::manual-contains
@briansrls
briansrls deleted the session/snappy-lark-771-cleanup branch June 1, 2026 18:43
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