Skip to content

feat(SG-2c cluster): parse_primary prefix + atom family - #665

Merged
briansrls merged 7 commits into
mainfrom
session/cool-bear-423
Apr 22, 2026
Merged

briansrls merged 7 commits into
mainfrom
session/cool-bear-423

Conversation

@briansrls

@briansrls briansrls commented Apr 22, 2026 •

Copy link
Copy Markdown
Contributor

SG-2c cluster — parse_primary prefix + atom family

Structural lane: one parser region, two table-owned surfaces (SG-2c-6 + SG-2c-7).

Same-PR receipt

  • Authority — PrimaryPrefixRow (existing) + PrimaryAtomRow in parse_tables.dag
  • Regen — regen_parse_tables_emit.rs extends for PrimaryAtomRow; run regen_parse_tables
  • Generated projection — parse_tables_generated.rs checked in (primary_atom_class / PrimaryAtomClass)
  • Live consumer — parse_parser_body.txt parse_primary uses primary_prefix_dispatch + primary_atom_class; regen_parse → parse_generated.rs
  • Authority tests — sg2c1_parse_tables_authority_test.rs (token join + exact atomic set)
  • Deletion — open-coded match on post-bump atomic TokenKinds removed; closed set is table-only (payload match remains for IntLit / StringLit / Ident data)

Notes

  • parse_tables.dag exemption count: 7 types (ROADMAP + thesis)

Closes the gap called out in review: authority prep only → full cluster pattern.

Copy link
Copy Markdown
Contributor Author

This is directionally fine, but it is not converged as the briefed SG-2c cluster lane yet.

What I see right now:

  • new PrimaryAtomRow authority in parse_tables.dag
  • cluster framing around parse_primary

What I do not see yet:

  • regen extension for the new family
  • generated projection checked in
  • live parse_parser_body.txt consumer swap
  • authority tests / ratchet updates
  • deletion of the corresponding inline parser-owned dispatch data

So this is currently authority prep, not the full SG-2c cluster receipt.

Direction:

  • keep this exact parse_primary family if that is the intended cluster
  • finish the full same-PR extraction pattern
  • name the cluster explicitly in the body/title (parse_primary prefix + atom family, or whatever exact family you choose)
  • do not leave draft until the regen + live consumer + test + deletion steps are all present

@briansrls
briansrls marked this pull request as ready for review April 22, 2026 21:18
@briansrls briansrls changed the title SG-2c feat(SG-2c cluster): parse_primary prefix + atom family Apr 22, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2960bf7a82

ℹ️ 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".

Comment on lines +477 to +479
type PrimaryAtomRow {
token_variant: String
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Connect PrimaryAtomRow rows to generated parser logic

These new PrimaryAtomRow facts are currently inert: regen_parse_tables_emit.rs only collects/emits PrimaryPrefixRow data, and parse_primary still hard-codes the atomic token match (IntLit/KwTrue/KwFalse/StringLit/Pipe/Ident) instead of using a generated PrimaryAtomClass. That means this commit introduces a second, non-enforced source of truth: editing primary_atom_* rows will not change runtime parsing and can silently drift from actual behavior.

Useful? React with 👍 / 👎.

@briansrls

Copy link
Copy Markdown
Contributor Author

Heads-up: the automated Codex review run at 21:20Z was against 2960bf7a82 (parse_tables.dag-only WIP). Current head is 005ca00a2 — full SG-2c cluster receipt (regen + parse_tables_generated / parse_generated, parse_parser_body consumer, sg2c1 ratchet, fmt).

@codex review

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 005ca00a · Trigger: schedule
  • Thinking: 101s wall

Looking at the diff: it's a narrowly-scoped cluster extension — adds PrimaryAtomRow + regen projection (PrimaryAtomClass / primary_atom_class) paired with SG-2c-6's PrimaryPrefixRow family, swaps the open-coded match token.kind in parse_primary for a two-step primary_atom_class guard + (cls, kind) pair-match, and ratchets the ROADMAP / thesis type count from 6 → 7.

I checked the principles against the diff:

  • P3 fail-closed: primary_atom_class returns Option and is immediately converted to Diagnostic::ParseError via .ok_or_else at parse_generated.rs:1119; the (class, other) catch-all is also Err, not a panic or fabricated node. ✓
  • P2 single-authority / boundary: PrimaryAtomClass labels derived at regen from token_variant via the closed allowlist in regen_parse_tables_emit.rs:511-526 — panics on unknown, no second authored field. parse_tables.dag:473-481 is the sole authoring site; the generated enum is emitted, never substrate. ✓
  • Coproduct classification: PrimaryAtomRow is annotated 🟡 YELLOW with named dissolution trigger (SG-2c proper) at parse_tables.dag:474-476. ✓
  • Facts flow forward: span + value payloads still propagate through the pair-match arms (parse_generated.rs:1123-1137). ✓
  • Ratchet consistency: thesis doc and ROADMAP both bumped 6 → 7; parse_tables.dag header comment updated to include SG-2c-7; authority test adds the analogous two-test coverage (every_primary_atom_row_… + primary_atom_rows_cover_exactly_…). ✓
  • Scope discipline: ROADMAP text itself now declares the cluster "closed" and warns against pre-queuing further one-row extractions. This PR lives within that envelope.

Exploratory observations (non-blocking)

  • Doc glitch: parse_tables.dag:469 describes the Pipe token as "pipe-(|>) lambda start". Pipe is the | token (from local_punct_pipe: { pattern: "|", kind: Pipe } in tokenize.dag:122) — |> is the separate PipeArrow token. Tiny — reads as a stale/typo reference.
  • (cls, kind) pair-match as a defensive check: because PrimaryAtomClass is regenerated from the same token_variant rows that gate the later pattern arms, the (class, other) => Err(..) arm is structurally unreachable under correct regen. The ratchet test (primary_atom_rows_cover_exactly_…) already enforces that set. An alternative shape — if primary_atom_class(...).is_none() { Err } else { match token.kind { … } } — would collapse the duplicated destructuring. The current shape is more defensive and fine; just worth flagging that the "class" value is conceptually a single-source shared read, not a second authority, so the cross-check is belt-and-suspenders rather than closing a real drift surface.

Verdict

APPROVE — diff is narrowly scoped, structurally consistent with the SG-2c-6 precedent, fail-closed end-to-end, single-authority preserved via regen, ratchet and tests updated atomically. No blocking findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🚀

ℹ️ 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".

@briansrls
briansrls force-pushed the session/cool-bear-423 branch from 4ad1eed to 4e2b4c9 Compare April 22, 2026 21:44
@briansrls

Copy link
Copy Markdown
Contributor Author

Ingest: Codex inline on parse_tables.dag:479 (PrimaryAtomRow “inert”) — that note matches 2960bf7 / dag-only state. Current session/cool-bear-423 head includes the full SG-2c-7 wiring:

  • regen_parse_tables_emit.rs: collect_primary_atom_rows → primary_atom_class / PrimaryAtomClass in parse_tables_generated.rs
  • parse_parser_body.txt parse_primary: primary_atom_class + PrimaryAtomClass (not an open TokenKind set match for the closed atom set)
  • sg2c1_parse_tables_authority_test pins the primary_atom_* row set

Rows are load-bearing: changing parse_tables.dag without regen_parse_tables fails the sg2c1 snapshot, and the generated TokenKind map must match the parser’s payload match arms.

@briansrls

Copy link
Copy Markdown
Contributor Author

Ingest (dashboard relay): Brian’s 21:36Z note is correct: early Codex ran on 2960bf7a82 (dag-only WIP). Current PR head is now 4e2b4c97b — full SG-2c cluster receipt plus the follow-up parse_corpus_manifest refresh (fix: refresh parse_corpus_manifest after parse_tables / tokenize drift) so parse_stage4_prep::handwritten_parse_snapshot_matches_manifest stays green. Anything keyed off 005ca00a2 is superseded by that manifest commit.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / gpt-5.4
  • Commit: 005ca00a · Trigger: schedule
  • Thinking: 286s wall

Verdict: APPROVE

Diff is small and narrowly scoped, and I did not find any concrete violations of INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md in the patch itself. The new PrimaryAtomRow surface is documented as a tracked scaffold with a named dissolution trigger, stays fail-closed in parse_primary, and the companion ratchet tests cover the exact authored token set for the new table family.

I did not complete a local cargo test confirmation because unrelated concurrent cargo jobs in other worktrees were holding the artifact lock, but nothing in the reviewed diff raised a substantive concern.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: claude / claude-opus-4-7
  • Commit: 8953be72 · Trigger: schedule
  • Thinking: 103s wall

The diff is a clean extension of the existing SG-2c parse_tables scaffold — adds a PrimaryAtomRow family and primary_atom_class projection, then replaces the open-coded atomic-token match in parse_primary with a classify-then-dispatch pattern. Same M7 shape as the landed PrimaryPrefixRow.

Things I checked:

  • Same-PR consumer (E-6): primary_atom_class is consumed in parse_primary in the same PR. ✓
  • Dissolution trigger (P5): parse_tables.dag carries the existing 🟡 YELLOW classification and names SG-2c proper (blocked on recursive List<Token> list-body emission) as the trigger. ROADMAP and compiler-std-consolidation.md both bump their exemption counts 6 → 7 in the same PR. Tracked debt, not a silent scaffold. ✓
  • Fail-closed (P3): Top-level primary_atom_class(&token.kind).ok_or_else(...)? produces a typed Diagnostic::ParseError. The (class, other) => Err(...) catch-all is an unreachable-by-construction invariant arm; it returns a typed diagnostic naming the fix location (parse_tables.dag / regen_parse_tables) rather than panicking or fabricating. Not a C-series fabrication. ✓
  • Single authority (P2): The atomic-token set lives in parse_tables.dag; dispatch-label mapping (IntLit → IntLiteral, etc.) lives exactly once in primary_atom_class_label_from_token_variant in regen_parse_tables_emit.rs, mirroring the prefix-family pattern. Regen panics on unknown token_variant. The parse_corpus_manifest.txt refresh picks up both the parse_tables.dag bump and prior tokenize drift from main. ✓
  • Ratchet coverage: Two new tests in sg2c1_parse_tables_authority_test.rs verify (a) every PrimaryAtomRow.token_variant is a real TokenKind variant, (b) rows cover exactly {IntLit, KwTrue, KwFalse, StringLit, Pipe, Ident}. Behavior-driven, hermetic (parses the .dag in-memory). ✓

Exploratory observations (non-blocking):

  • The classify-then-match (cls, token.kind) pattern is logically a double dispatch: primary_atom_class already narrows the token, but Rust can't carry that narrowing into the payload extraction, so each arm re-matches the TokenKind. That's a fine Rust idiom, but it means the "class" column is never actually read at runtime — the cls value is used only to reach the catch-all for the structurally-impossible case. If at some point you want to cut the redundancy, the classifier could return something payload-aware (e.g., the SurfaceLiteral/lambda-entry directly) so the caller just matches on what to do next. Not worth doing now — the payload-extraction callbacks (parse_lambda, parse_ident_expr) are semantic and belong in the parser. Flagging as a shape observation for SG-2c-proper.

  • The PrimaryAtomClass arms are 1:1 with the allowlist in primary_atom_class_label_from_token_variant, which in turn is hand-maintained in regen_parse_tables_emit.rs. Same shape as PrimaryPrefixRow so no new authority is introduced, but worth noting that the allowlist in regen is the de-facto second authority that closes the set — if the v3 grammar ever wants a primary like FloatLit, two files move in lockstep: the row in .dag and the allowlist arm in regen_parse_tables_emit.rs. The .dag-only "token_variant" authority the file header claims is slightly weaker than advertised because of that. Precedent-matching with SG-2c-6, so not a regression here.

Verdict: APPROVE — small, narrowly scoped, matches the existing prefix-family pattern end to end, same-PR consumer wired, ratchet counts bumped in both ROADMAP.md and the thesis doc, tests added. No modeling-discipline violations observed.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / gpt-5.4
  • Commit: 8953be72 · Trigger: schedule
  • Thinking: 244s wall

Verdict: APPROVE. Diff is narrowly scoped and looks clean: the new PrimaryAtomRow authority is carried forward into primary_atom_class, consumed in parse_primary, and backed by same-PR ratchet tests, so I don’t see a concrete violation of INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md. I also ran cargo test -p v3-compiler sg2c1_parse_tables_authority_test -- --nocapture; all 15 targeted authority tests passed.

@briansrls
briansrls force-pushed the session/cool-bear-423 branch from 455ebc7 to eeb8f47 Compare April 22, 2026 22:06

@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 / gpt-5.4
  • Commit: 005ca00a · Trigger: schedule
  • Thinking: 366s wall

Non-blocking — Strengths

  • src/v3/compiler/parse_tables.dag I found no blocking issue here; PrimaryAtomRow follows the existing parse_tables scaffold discipline with one authored fact, a named SG-2c-proper trigger, and a same-PR consumer in parse_primary.
  • src/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rs The new atom-family tests pin both token-kind validity and exact post-prefix coverage, so this extraction fails closed instead of silently drifting.

ROADMAP — Verified

  • parse_tables exemption count: ROADMAP.md, docs/thesis/compiler-std-consolidation.md, and scripts/check-compiler-std-ratchet.sh all agree on the new 7-type parse_tables.dag exemption in the same PR.

✅ This looks clean: the new parse_primary atom-family extraction preserves single authority in parse_tables.dag, keeps failure paths diagnostic, and updates the ratchet/docs consistently.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

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

Review

I read the pinned context docs (INVARIANTS, modeling-discipline, CODING, TESTING) and the full diff.

This PR is a clean extension of the established SG-2c table-authority pattern:

  • Single authority + facts-forward: PrimaryAtomRow rows in parse_tables.dag:478-490 are the sole source; PrimaryAtomClass + primary_atom_class are regen-emitted projections (parse_tables_generated.rs:149-175).
  • Fail-closed, two layers:
    • regen-time panic for unknown token_variant (regen_parse_tables_emit.rs:531-534),
    • runtime "internal: primary-atom class does not match TokenKind" catch-all (parse_generated.rs:1137-1145) if the generated class and the bumped token disagree — good (cls, other) guard preserving the old "expected primary expression" diagnostic when the class is None.
  • Tracked scaffold: 🟡 YELLOW is declared with a named dissolution trigger (SG-2c-proper) in parse_tables.dag:474-478; ratchet exemption updated coherently in thesis doc (6 → 7) and scripts/check-compiler-std-ratchet.sh:52.
  • Tests cover coverage: every_primary_atom_row_token_variant_is_a_token_kind_variant and primary_atom_rows_cover_exactly_the_tokens_parse_primary_atomic_arm pin the row family to TokenKind and to the parser's closed set. Hermetic (in-memory compile), behavior-driven.

The SG-2c cluster framing (6+7 in one parse_primary function) is consistent with the "named same-PR consumer block" bar set by the growth-discipline checkpoint; ROADMAP.md:178 frames the cluster as closing out SG-2c row extractions pending capability pivot. No parallel-authority risk introduced (rows are data, trigger-gated, exempted from ratchet).

Verdict

APPROVE — narrow-scoped extension of an established, well-disciplined pattern; single authority, fail-closed, explicit scaffold with named dissolution, good test coverage. No violations observed.

Exploratory observations (non-blocking)

  • The regen allowlist primary_atom_class_label_from_token_variant (regen_parse_tables_emit.rs:524-539) is a second place to edit when rows are added (token_variant → dispatch-label mapping). Documented in the .dag comment block as a deliberate choice (kept rows single-field). Precedent-consistent; worth watching if the table grows further — at some point it may be tidier to author the label directly in the row (second field) rather than via the closed Rust allowlist. Not a concern at 6 rows.
  • The expected set in primary_atom_rows_cover_exactly_the_tokens_parse_primary_atomic_arm is a third hardcoded list (parser arms, regen allowlist, test expected). That's the pattern's cost; CI catches any two diverging. Again, worth watching if this cluster grows.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / gpt-5.4
  • Commit: eeb8f47d · Trigger: schedule
  • Thinking: 223s wall

Verdict: APPROVE. The diff is narrowly scoped and consistent with the pinned rubric: parse_primary moves another closed token-dispatch set under parse_tables.dag authority, the regen path and same-PR consumers land together, and the added ratchet coverage keeps the table and generated projection from drifting. I did not find a concrete invariant, coding, or testing-discipline violation in the changed lines.

Targeted cargo test -p v3-compiler ... runs were started, but the workspace build was still in progress when I wrapped the 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.

Review metadata

  • Provider / model: codex / gpt-5.4
  • Commit: eeb8f47d · Trigger: schedule
  • Thinking: 353s wall

✅ No concerns in the + lines from these commits; the new parse_primary atom-family extraction stays table-owned, fail-closed, and consistent with the updated ratchet/docs.

Complete same-PR extraction for PrimaryAtomRow (SG-2c-7) alongside SG-2c-6:
- regen_parse_tables_emit: collect rows, emit primary_atom_class/PrimaryAtomClass
- checked-in parse_tables_generated.rs + regen_parse parse_generated.rs
- parse_parser_body: table-owned dispatch; remove open match on atomic TokenKinds
- sg2c1 authority tests: row join + exact post-prefix token set
- ROADMAP + compiler-std-consolidation: 7 exempted types, cluster note

Cluster name: parse_primary prefix + atom family.

Made-with: Cursor
SG-2c-7 atomic tail uses TokenKind::Pipe (|), not PipeArrow (|>).

Made-with: Cursor
The Pipe vs PipeArrow doc fix changed source spans; fnv1a64 for the parse
surface snapshot moved (items/byte len unchanged). Unblocks v3 CI
handwritten_parse_snapshot_matches_manifest.

Made-with: Cursor
SG-2c-7 added `type PrimaryAtomRow` to parse_tables.dag; the #642 script
must list it next to PrimaryPrefixRow and the other parse_tables row shapes.

Unblocks: Compiler-std consolidation ratchet (v3 workflow).
Made-with: Cursor
@briansrls
briansrls force-pushed the session/cool-bear-423 branch from eeb8f47 to f3a40aa Compare April 22, 2026 23:46
@briansrls
briansrls merged commit a8f0825 into main Apr 22, 2026
3 checks passed
@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / gpt-5.4
  • Commit: f3a40aa5 · Trigger: schedule
  • Thinking: 177s wall

Verdict: APPROVE. The diff stays within the existing parse_tables.dag scaffold, keeps parse_primary fail-closed in src/v3/compiler/parse_parser_body.txt, and preserves single authority by deriving the new PrimaryAtomClass projection from PrimaryAtomRow facts rather than authoring a second source of truth in Rust (src/v3/compiler/parse_tables.dag, src/v3/compiler/src/regen_parse_tables_emit.rs). The matching authority tests for the new row family are present in src/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rs, so I don’t see a concrete violation of the pinned invariants, coding rules, or testing discipline.

@briansrls
briansrls deleted the session/cool-bear-423 branch June 1, 2026 18:42
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