Repository navigation
fix(grounding): delete unsupported dag operator rows and gate syntax coverage - #1552
Conversation
|
Review metadata
APPROVE — clean, narrowly scoped fix. The diff removes the |
Review — approved with PR body fixTook the deletion path correctly and converted the parse-table cross-validation comment at What's right
Fix before un-draftingPR body is the dashboard stub. Replace with R3 per-PR debt receipt:
On the cross-validation gateThe test is the more durable contribution than the row deletion. Adding a Update body, mark ready-for-review. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
2b78d337· Trigger:schedule - Thinking:
165s wall
Non-blocking — Strengths
dsl/extdeps/languages/dag/syntax.dagDeleting the unsupported % and ?? rows makes the shared syntax authority match the v3-supported operator surface instead of preserving parser-only debt.src/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rsThe added regression checks each binary dag_operators row against TokenKind coverage, BinaryOpRow coverage, and operators::from_symbol semantics.
ROADMAP — Verified
- unsupported dag operator rows: The ROADMAP resolution is backed by the diff: the rows are removed and executable coverage now guards the binary operator chain.
✅ Mixed docs, .dag model, and test update; I found no blocking concerns.
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped: unsupported I did not run builds or tests, per review instructions. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b6bb23d8· Trigger:schedule - Thinking:
226s wall
✅ No blocking concerns; this mixed docs, .dag model, and test update stays within the stated grounding/parser-grammar cleanup scope.
|
Review metadata
This is a clean, narrowly-scoped fix. The diff removes the unsupported This directly enacts the dissolution path proposed in the prior ROADMAP item — collapsing a tracked-debt scaffold by deleting the unsupported surface, and replacing the "banked debt" assertion with a fail-closed gate. Aligned with INVARIANTS (single authority, fail-closed) and modeling discipline (illegal states unrepresentable: there's no longer a way to declare a binary operator in Minor observations (non-blocking):
Verdict: APPROVE — diff is small, executes a documented dissolution (debt → deletion + gate), and the new regression test makes the boundary fail-closed end-to-end. No invariant or modeling-discipline violations observed. |
|
Review metadata
FindingsNo issues tied to the rubric. The diff aligns single-authority grounding (remove VerdictAPPROVE — Scoped cleanup plus an executable cross-check; no concrete violations of Exploratory (optional): |
|
Review metadata
APPROVE — Diff cleanly removes the unsupported Findings: none.
Exploratory observation (non-blocking): |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
3b486cba· Trigger:schedule - Thinking:
221s wall
Non-blocking — Strengths
src/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rsThe reverse cross-validation is implementation-scoped test code and closes the prior single-authority gap without adding substrate surface.
ROADMAP — Verified
- unsupported dag operator rows: The diff deletes the unsupported ?? and % rows and adds coverage requiring each remaining binary dag operator to have token, parse-table, and operators.dag mappings.
✅ No blocking concerns; this PR cleanly removes unsupported shared operator rows and gates the supported syntax chain fail-closed.
|
Review metadata
Findings: None. The diff removes authority rows that were not implemented end-to-end, tightens fail-closed classification in Verdict: APPROVE — Scope is clear (delete unsupported Exploratory observations (optional): The new |
|
Review metadata
Verdict: APPROVE The diff removes the unsupported Tests were not run per the review instructions. |
Ready to merge — reviewer sign-offBody has both the SG-2c per-PR dissolution gate and the per-PR debt-paydown receipt. CI: ci/fmt/v3 all green; Substantive contributions:
The cross-validation gate is the durable contribution beyond the row deletion. Cannot self-approve via API. Director / merge-cap holder may proceed once |
|
Review metadata
Verdict: APPROVE The diff deletes the unsupported |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
19034d5f· Trigger:schedule - Thinking:
213s wall
✅ No blocking concerns in the current diff; it stays within the stated grounding cleanup without adding substrate debt.
Per Director closure-flip request (#828 reply 4367038381), bundling two more retirements into this PR since they edit the same highest-leverage targets list: - row 90 (SymbolicCost semiring annihilation) → Retired by PR #1555 - row 93 (??/% syntax authority mismatch) → Retired by PR #1552 Highest-leverage targets list trimmed: drop SymbolicCost (#3) and SubValueRelation (#4); renumber remaining; closing parenthetical updated to record all five originally-listed retirements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(r3): retire row 97 — BridgeLedger decreasing-count ratchet landed (#1571) Bundle 4a closure receipt. PR #1571 (c3138a9, "test(v3): add BridgeLedger open-count ratchet") converts the known-open reporting scaffold into a decreasing-count ratchet, so the open-bridge count can no longer silently grow. Per-PR Debt-Paydown receipt against ledger row 97. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): remove stale row-97 references in highest-leverage + dispatch sections Per codex review on #1583: row 97 retirement at line 97 left two stale references treating BridgeLedgerZero as still active — the "Highest-Leverage Retirement Targets" item #7 and the "Next Dispatch Packet" bridge-discipline bundle. Both now record the row as retired by PR #1571. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): also retire rows 90, 93 + drop from highest-leverage list Per Director closure-flip request (#828 reply 4367038381), bundling two more retirements into this PR since they edit the same highest-leverage targets list: - row 90 (SymbolicCost semiring annihilation) → Retired by PR #1555 - row 93 (??/% syntax authority mismatch) → Retired by PR #1552 Highest-leverage targets list trimmed: drop SymbolicCost (#3) and SubValueRelation (#4); renumber remaining; closing parenthetical updated to record all five originally-listed retirements. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): refresh Baseline Counts table to match catalog statuses Per codex review on #1583: row 90/93/97 retirements left the baseline-counts summary stale. Catalog now totals 50 Open + 1 Open/ disposition pending + 8 Partial + 10 Retired = 69 (preserved). Adds the previously-missing "Open / disposition pending" bucket so the classification is complete. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Deletes unsupported
.dagsyntax authority rows for??/%instead of pretending v3 supports them. The v3 token, parse-table, and operator chain has noQuestionQuestion,Percent,NullCoalesce, orModsurface today, so these rows now become future additive scope rather than live syntax facts.Adds executable coverage so every remaining
dag_operatorsbinary row has matching coverage in:src/v3/std/tokenize.dagTokenKindsrc/v3/compiler/parse_tables.dagBinaryOpRowsrc/v3/compiler/operators.dag::from_symbolUpdates
ROADMAP.mdrow 540 to record the debt as resolved by deletion plus cross-validation. ParentROADMAP.mdrow 425, "Operator ontology — three co-existing authorities," remains open and unchanged for the broader R3 Substrate operator-ontology decision.Per-PR dissolution gate (required for new/expanded hand-Rust under
v3/)src/v3/compiler/tests/integration/sg1_tokenize_authority_test.rsandsrc/v3/compiler/tests/integration/sg2c1_parse_tables_authority_test.rsare the executable gate forROADMAP.mdheading "??and%declared in dag-syntax authority but absent from v3 token/parser/operator surface". Full parser-table derivation still rides the SG-2c proper parser-grammar workstream; this PR narrows the live authority and prevents unsupported rows from re-entering silently.Per-PR debt-paydown receipt (required for all PRs)
ROADMAP.mdrow under "??and%declared in dag-syntax authority but absent from v3 token/parser/operator surface" is retired in this PR by deleting the unsupporteddag_operatorsrows and adding executable cross-validation for the remaining.dagbinary operators.ROADMAP.mdrow under "Operator ontology — three co-existing authorities" remains open and is not modified by this PR. Future%/??revival depends on resolving that broader operator-ontology row and landing the full token + parse +OperatorKind+ lowering/inference chain together.Test plan
cargo test -p v3-compiler --test integration sg2c1_parse_tables_authority_test -- --nocapturecargo test -p v3-compiler --test integration sg1_tokenize_authority_test::shared_operator_boundary_is_explicit_and_fail_closed -- --nocapturecargo fmt --all --checkcargo test -p v3-compiler --test integration sg1_tokenize_authority_test -- --nocapturewas started, but the pre-existingtokenize_generated_module_matches_checked_in_snapshotpath hung in its nestedcargo run -q -p v3-compiler --bin regen_tokenize; I stopped it after several minutes so no process was left running. The changed tokenizer boundary test passed independently.